Re: [PATCH v6 01/11] memory: batch processing in acquire_resource()

["Roger Pau Monné" <roger.pau@citrix.com>]

On Tue, Jul 07, 2020 at 09:39:40PM +0200, Michał Leszczyński wrote:
> From: Michal Leszczynski <michal.leszczynski@cert.pl>
> 
> Allow to acquire large resources by allowing acquire_resource()
> to process items in batches, using hypercall continuation.
> 
> Be aware that this modifies the behavior of acquire_resource
> call with frame_list=NULL. While previously it would return
> the size of internal array (32), with this patch it returns
> the maximal quantity of frames that could be requested at once,
> i.e. UINT_MAX >> MEMOP_EXTENT_SHIFT.
> 
> Signed-off-by: Michal Leszczynski <michal.leszczynski@cert.pl>
> ---

FWIW, I think I've also said on a previous version, I would prefer if
the changelog between versions is added to each patch, having it on
the cover letter is not very helpful as I usually care about specific
changes made to each patch.

I've just got one comment that needs addressing below.

>  xen/common/memory.c | 49 ++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 44 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 714077c1e5..eb42f883df 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1046,10 +1046,12 @@ static int acquire_grant_table(struct domain *d, unsigned int id,
>  }
>  
>  static int acquire_resource(
> -    XEN_GUEST_HANDLE_PARAM(xen_mem_acquire_resource_t) arg)
> +    XEN_GUEST_HANDLE_PARAM(xen_mem_acquire_resource_t) arg,
> +    unsigned long *start_extent)
>  {
>      struct domain *d, *currd = current->domain;
>      xen_mem_acquire_resource_t xmar;
> +    uint32_t total_frames;
>      /*
>       * The mfn_list and gfn_list (below) arrays are ok on stack for the
>       * moment since they are small, but if they need to grow in future
> @@ -1069,7 +1071,7 @@ static int acquire_resource(
>          if ( xmar.nr_frames )
>              return -EINVAL;
>  
> -        xmar.nr_frames = ARRAY_SIZE(mfn_list);
> +        xmar.nr_frames = UINT_MAX >> MEMOP_EXTENT_SHIFT;
>  
>          if ( __copy_field_to_guest(arg, &xmar, nr_frames) )
>              return -EFAULT;
> @@ -1077,8 +1079,28 @@ static int acquire_resource(
>          return 0;
>      }
>  
> +    total_frames = xmar.nr_frames;
> +
> +    /* Is the size too large for us to encode a continuation? */
> +    if ( unlikely(xmar.nr_frames > (UINT_MAX >> MEMOP_EXTENT_SHIFT)) )
> +        return -EINVAL;
> +
> +    if ( *start_extent )
> +    {
> +        /*
> +         * Check whether start_extent is in bounds, as this
> +         * value if visible to the calling domain.
> +         */
> +        if ( *start_extent > xmar.nr_frames )
> +            return -EINVAL;
> +
> +        xmar.frame += *start_extent;
> +        xmar.nr_frames -= *start_extent;
> +        guest_handle_add_offset(xmar.frame_list, *start_extent);
> +    }
> +
>      if ( xmar.nr_frames > ARRAY_SIZE(mfn_list) )
> -        return -E2BIG;
> +        xmar.nr_frames = ARRAY_SIZE(mfn_list);
>  
>      rc = rcu_lock_remote_domain_by_id(xmar.domid, &d);
>      if ( rc )
> @@ -1135,6 +1157,14 @@ static int acquire_resource(
>          }
>      }
>  
> +    if ( !rc )
> +    {
> +        *start_extent += xmar.nr_frames;
> +
> +        if ( *start_extent != total_frames )
> +            rc = -ERESTART;
> +    }
> +
>   out:
>      rcu_unlock_domain(d);
>  
> @@ -1599,8 +1629,17 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>  #endif
>  
>      case XENMEM_acquire_resource:
> -        rc = acquire_resource(
> -            guest_handle_cast(arg, xen_mem_acquire_resource_t));
> +        do {
> +            rc = acquire_resource(
> +                guest_handle_cast(arg, xen_mem_acquire_resource_t),
> +                &start_extent);

I think it would be interesting from a performance PoV to move the
xmar copy_from_guest here, so that each call to acquire_resource
in the loop doesn't need to perform a copy from guest. That's
more relevant for translated callers, where a copy_from_guest involves
a guest page table and a p2m walk.

> +
> +            if ( hypercall_preempt_check() )

You are missing a rc == -ERESTART check here, you don't want to encode
a continuation if rc is different than -ERESTART AFAICT.

Thanks, Roger.