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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan

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