This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
Home Products Support Community News


[Xen-devel] Re: [XenPPC] [PATCH 1/4] xencomm take 2: xen side varisous f

To: Isaku Yamahata <yamahata@xxxxxxxxxxxxx>
Subject: [Xen-devel] Re: [XenPPC] [PATCH 1/4] xencomm take 2: xen side varisous fixes and preparation for consolidation
From: Hollis Blanchard <hollisb@xxxxxxxxxx>
Date: Mon, 13 Aug 2007 15:08:58 -0500
Cc: xen-ppc-devel@xxxxxxxxxxxxxxxxxxx, xen-devel@xxxxxxxxxxxxxxxxxxx, xen-ia64-devel@xxxxxxxxxxxxxxxxxxx
Delivery-date: Mon, 13 Aug 2007 13:09:38 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <20070813035910.GA20100%yamahata@xxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Organization: IBM Linux Technology Center
References: <20070813035910.GA20100%yamahata@xxxxxxxxxxxxx>
Reply-to: Hollis Blanchard <hollisb@xxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
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-devel mailing list