Re: [XenPPC] [PATCH 1/4] xencomm take 2: xen side varisous fixes and preparation for consolidation

[Hollis Blanchard <hollisb@us.ibm.com>]

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