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

[George Dunlap <george.dunlap@citrix.com>]

On 1/22/20 4:55 PM, Jan Beulich wrote:
> On 22.01.2020 17:51, Tamas K Lengyel wrote:
>> On Wed, Jan 22, 2020 at 8:23 AM Jan Beulich <jbeulich@suse.com> 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.

I agree with everything Jan said. :-)

Also, since this is a general bugfix, you might consider moving it to
the top of your series, so it can be checked in as soon as it's ready.

 -George

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