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

[Doi.Tsunehisa@xxxxxxxxxxxxxx]

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 ?

>>   In this code, I assumed that hypervisor might be destructing the
>> domain. In this case, the page counter might be zero.
> 
> Such case doesn't occur because we called get_page().
> It is the reason that the page reference counter exists for, isn't it?

  I understand it.

Thanks,
- Tsunehisa Doi

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