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

From: Tamas K Lengyel <tamas.k.lengyel@gmail.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Tamas K Lengyel" <tamas.lengyel@intel.com>,
	"Wei Liu" <wl@xen.org>,
	"George Dunlap" <george.dunlap@eu.citrix.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	Xen-devel <xen-devel@lists.xenproject.org>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v5 03/18] x86/p2m: Allow p2m_get_page_from_gfn to return shared entries
Date: Wed, 22 Jan 2020 09:51:06 -0700	[thread overview]
Message-ID: <CABfawhkzPyFas7kasNjBoCyCMi+kkfC6DrvYv4PCuPtK7A5MNA@mail.gmail.com> (raw)
In-Reply-To: <6cb72a36-d9af-5b96-76df-2c6746dfa245@suse.com>

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.

Tamas

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