Re: [Xen-devel] [XEN PATCH for-4.13 v2] x86/domctl: have XEN_DOMCTL_getpageframeinfo3 preemptible

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Mon, Nov 25, 2019 at 05:22:19PM +0100, Jan Beulich wrote:
> On 25.11.2019 15:59, Anthony PERARD wrote:
> > This hypercall can take a long time to finish because it attempts to
> > grab the `hostp2m' lock up to 1024 times. The accumulated wait for the
> > lock can take several seconds.
> > 
> > This can easily happen with a guest with 32 vcpus and plenty of RAM,
> > during localhost migration.
> > 
> > Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> 
> As indicated on v1 already, this being a workaround rather than a fix
> should be stated clearly in the description. Especially if more such
> operations turn up, it'll become increasingly obvious that the root
> of the problem will need dealing with rather than papering over some
> of the symptoms. With this taken care of I'd be (still hesitantly)
> willing to give my ack for this as a short term "solution".

Sorry to have lead you to believe that the patch was *the* solution to
the problem described. I don't think the patch itself is a workaround or
a fix, it is simply an improvement to the hypercall. That improvement
could be used to remove the limit on `num' (something that I've read on
xen-devel as a possible improvement).

Would it be enough to add this following paragraph to the commit description?

    While the patch doesn't fix the problem with the lock contention and
    the fact that the `hostp2m' lock is currently global (and not on a
    single page), it is still an improvement to the hypercall.


I don't like the terms "workaround" or "short term solution" as a
description for this patch. Both implies that the patch could be
reverted once the root issue is taking care of.

I'll keep working to try to improve the use of the hostp2m lock, at
least with that hypercall, but I don't have a solution yet and it would
be nice to have this patch in the release.

> > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> > index a03e80e5984a..1b69eb75cb20 100644
> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -163,6 +163,10 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_getdomaininfo_t);
> >  #define XEN_DOMCTL_PFINFO_LTAB_MASK (0xfU<<28)
> >  
> >  /* XEN_DOMCTL_getpageframeinfo3 */
> > +/*
> > + * Both value `num' and `array' are modified by the hypercall to allow
> > + * preemption.
> 
> s/are/may be/ ?

I don't think the distinction is necessary. How would that be useful to
know that both values may not be modified? I though the goal of the
added description was to warn against reusing the values after calling
the hypercall.

Thanks,

-- 
Anthony PERARD

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