Re: [Xen-devel] [PATCH v5 03/18] x86/p2m: Allow p2m_get_page_from_gfn to return shared entries

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.01.2020 17:51, Tamas K Lengyel wrote:
> On Wed, Jan 22, 2020 at 8:23 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> On 21.01.2020 18:49, Tamas K Lengyel wrote:
>>> The owner domain of shared pages is dom_cow, use that for get_page
>>> otherwise the function fails to return the correct page.
>>
>> I think this description needs improvement: The function does the
>> special shared page dance in one place (on the "fast path")
>> already. This wants mentioning, either because it was a mistake
>> to have it just there, or because a new need has appeared to also
>> have it on the "slow path".
> 
> It was a pre-existing error not to get the page from dom_cow for a
> shared entry in the slow path. I only ran into it now because the
> erroneous type_count check move in the previous version of the series
> was resulting in all pages being fully deduplicated instead of mostly
> being shared. Now that the pages are properly shared running LibVMI on
> the fork resulted in failures do to this bug.
> 
>>> --- a/xen/arch/x86/mm/p2m.c
>>> +++ b/xen/arch/x86/mm/p2m.c
>>> @@ -594,7 +594,10 @@ struct page_info *p2m_get_page_from_gfn(
>>>      if ( p2m_is_ram(*t) && mfn_valid(mfn) )
>>>      {
>>>          page = mfn_to_page(mfn);
>>> -        if ( !get_page(page, p2m->domain) )
>>> +        if ( !get_page(page, p2m->domain) &&
>>> +             /* Page could be shared */
>>> +             (!dom_cow || !p2m_is_shared(*t) ||
>>> +              !get_page(page, dom_cow)) )
>>
>> While there may be a reason why on the fast path two get_page()
>> invocations are be necessary, couldn't you get away with just
>> one
>>
>>         if ( !get_page(page, !dom_cow || !p2m_is_shared(*t) ? p2m->domain
>>                                                             : dom_cow) )
>>
>> at least here? It's also not really clear to me why here and
>> there we need "!dom_cow || !p2m_is_shared(*t)" - wouldn't
>> p2m_is_shared() return consistently "false" when !dom_cow ?
> 
> I simply copied the existing code from the slow_path as-is. IMHO it
> would suffice to do a single get_page(page, !p2m_is_shared(*t) ?
> p2m->domain : dom_cow);  since we can't have any entries that are
> shared when dom_cow is NULL so this is safe, no need for the extra
> !dom_cow check. If you prefer I can make the change for both
> locations.

If the change is correct to make also in the other place, I'd
definitely prefer you doing so.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel