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

[Jürgen Groß <jgross@xxxxxxxx>]

On 25.11.19 17:22, 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".
>
> > --- a/xen/arch/x86/domctl.c
> > +++ b/xen/arch/x86/domctl.c
> > @@ -425,6 +425,26 @@ long arch_do_domctl(
> >                   ret = -EFAULT;
> >                   break;
> >               }
> > +
> > +            /*
> > +             * Avoid checking for preemption when the `hostp2m' lock isn't
> > +             * involve, i.e. non-translated guest, and avoid preemption on
> > +             * the last iteration.
> > +             */
> > +            if ( paging_mode_translate(d) &&
> > +                 likely((i + 1) < num) && hypercall_preempt_check() )
> > +            {
> > +                domctl->u.getpageframeinfo3.num = num - i - 1;
> > +                domctl->u.getpageframeinfo3.array.p =
> > +                    guest_handle + ((i + 1) * width);
> > +                if ( __copy_to_guest(u_domctl, domctl, 1) )
> > +                {
> > +                    ret = -EFAULT;
> > +                    break;
> > +                }
> > +                return hypercall_create_continuation(__HYPERVISOR_domctl,
> > +                                                     "h", u_domctl);
> > +            }
> >           }
> >   
> >           break;
> > 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/ ?
>
> If Juergen wants to still allow this in, I'd be fine taking care of both
> remarks while committing.
Sorry, was only scanning for an Ack.

Release-acked-by: Juergen Gross <jgross@xxxxxxxx>


Juergen

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