Re: [Xen-devel] [RFC/PATCH] XENMEM_claim_pages (subop of an existing) hypercall

[Dan Magenheimer <dan.magenheimer@xxxxxxxxxx>]

> From: Keir Fraser [mailto:keir.xen@xxxxxxxxx]
> Subject: Re: [Xen-devel] [RFC/PATCH] XENMEM_claim_pages (subop of an 
> existing) hypercall
> 
> On 14/11/2012 17:42, "Dan Magenheimer" <dan.magenheimer@xxxxxxxxxx> wrote:
> 
> >> It would be nice if d->tot_pages adjustments didn't take the global
> >> heap_lock in this case. There's probably some way to bail out of those new
> >> update functions before doing the locked work, in that case.
> >
> > I agree.  I played with atomics for a bit but couldn't
> > see a way it would work without races.  The generic
> > pseudo-code is
> >
> > if (A) {
> > atomically {
> > modify A
> > modify d->B
> > }
> > } else {
> > modify d->B
> > }
> >
> > Any ideas?
> 
> E.g.,
>     /* Fast path (new). */
>     if ( !d->unclaimed_pages )
>         return d->tot_pages += pages;
>     /* Slower path (as in your patch). */
>     spin_lock(&heap_lock);
>     d->tot_pages += pages;
>     if ( d->unclaimed_pages )
>         ....
> 
> The only danger is around any necessary interlocking with d->unclaimed_pages
> becoming non-zero. But the fast-path test is still under d->page_alloc_lock,
> and you set up d->unclaimed_pages under that lock, so it looks okay to me.

Hmmm, nice, I think you are right.  I had just removed the
d->page_alloc_lock in the "set" code at Jan's suggestion,
but this is a good reason to keep it there.  I'll also
add a comment to document this subtle lock interdependency.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel