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

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

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-ppc-devel mailing list

Reply via email to