Re: [Xen-ia64-devel] [Patch] Fix for re-enabling PV-on-HVM on IPF

[Isaku Yamahata <yamahata@xxxxxxxxxxxxx>]

On Thu, Mar 08, 2007 at 05:54:07PM +0900, Doi.Tsunehisa@xxxxxxxxxxxxxx wrote:
> You (yamahata) said:
> >>>> +
> >>>>              guest_physmap_remove_page(d, gpfn, mfn);
> >>>> +
> >>>> +            /* post-set PGC_allocated flag */
> >>>> +            do {
> >>>> +                x = mfn_to_page(mfn)->count_info;
> >>>> +                if ((x & PGC_count_mask) == 0)
> >>>> +                    goto out;
> >>>> +                nx = x | PGC_allocated;
> >>>> +            } while (cmpxchg_acq(&mfn_to_page(mfn)->count_info, x, nx)
> >>>>  != x);
> >>>> +        }
> >>> 
> >>> checking == 0 is non-sense because we incremented it.
> >>> Probably you want to 
> >>>    if (!test_and_set_bit(page->count_info, _PGC_allocated)) {
> >>>   put_page(page);
> >>>   goto out;
> >>>    }
> >> 
> >>   guest_physmap_remove_page() unsets PGC_allocated flag, thus
> >> this test_and_set_bit() returns zero allways, I think.
> > 
> > Sorry, I was somewhat confused.
> > Probably it should looks like the following.
> > if (page->count_info != 1) { /* no one but us is using this page */
> >     put_page(page);
> >     goto out;
> > }
> > __set_bit(&page->count_info, _PGC_allocated);
> 
>   Does `page->count_info' have to be operated with atomic ?

Atomic operation is safe bet.
XENMAPSPACE hypercall isn't performance critical.
If you want to be paranoia, use "if (test_and_set_bit())" to check whether
some else set the bit.

-- 
yamahata

_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ia64-devel