On Mon, 2007-08-13 at 12:59 +0900, Isaku Yamahata wrote:
> - Xencomm should get_page()/put_page() after address conversion from paddr
> to maddr because xen supports SMP and balloon driver.
> Otherwise another vcpu may free the page at the same time.
> Such a domain behaviour doesn't make sense, however nothing prevents it.
Unfortunately my test system is currently down, so I can't test this
today.
However, one initial comment: I really dislike the way get/put_page()
are scattered through this code. Maybe every pair is balanced today, but
it will be difficult to maintain, and especially to test all the error
paths.
I think this needs a more symmetrical API. Right now get_page() and
put_page() are being done at multiple levels, and in
xencomm_get_address() we're calling put_page() only to call get_page() a
moment later in xencomm_paddr_to_vaddr(). I don't have a concrete
proposal for simplifying this.
Also, it looks like we're calling put_page() on the 'desc' page itself
before we're done with it, so that's a bug.
> +static int
> +xencomm_paddr_to_vaddr(unsigned long paddr, unsigned long *vaddr,
> + struct page_info **page)
Since we can use page_to_vaddr(), I don't think you need to pass 'vaddr'
here. That should simplify the code a little bit.
By the way, this looks bogus:
>
> +static int
> +xencomm_get_address(const void *handle, struct xencomm_desc *desc,
> int i,
> + unsigned long **address, struct page_info **page)
> +{
> + if (i == 0)
> + *address = &desc->address[0];
> + else
> + (*address)++;
Shouldn't that be *address = &desc->address[i] ?
I definitely agree that some of these fixes are needed (especially
checking for the descriptor page overlap, and using get/put_page()).
However, this code is difficult to follow already, and these patches are
confusing *me* (and I wrote it! :) so I'm very nervous about increasing
the complexity.
Since the only issue you've identified is populate_physmap, and that has
an easy workaround, I would prefer the easier solution.
--
Hollis Blanchard
IBM Linux Technology Center
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home