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

[Tamas K Lengyel <tamas.k.lengyel@xxxxxxxxx>]

On Thu, Jan 23, 2020 at 8:32 AM George Dunlap <george.dunlap@xxxxxxxxxx> wrote:
>
> 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@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.
>
> 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.

Sure - although it can be checked in before patch 1 & 2, it's not
dependent on them. In fact, all the patches in this series up to and
including Patch 14 could be checked in out-of-order.

Tamas

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