RE: [PATCH v4 06/10] memory: batch processing in acquire_resource()

From: Paul Durrant <xadimgnik@gmail.com>
To: "'Julien Grall'" <julien@xen.org>,
	"'Michał Leszczyński'" <michal.leszczynski@cert.pl>,
	xen-devel@lists.xenproject.org
Cc: 'Stefano Stabellini' <sstabellini@kernel.org>,
	tamas.lengyel@intel.com, 'Wei Liu' <wl@xen.org>,
	'Andrew Cooper' <andrew.cooper3@citrix.com>,
	'Ian Jackson' <ian.jackson@eu.citrix.com>,
	'George Dunlap' <george.dunlap@citrix.com>,
	'Jan Beulich' <jbeulich@suse.com>,
	luwei.kang@intel.com
Subject: RE: [PATCH v4 06/10] memory: batch processing in acquire_resource()
Date: Fri, 3 Jul 2020 12:40:01 +0100	[thread overview]
Message-ID: <004a01d6512e$b0b5fab0$1221f010$@xen.org> (raw)
In-Reply-To: <481e8ee7-561a-10d6-4358-7b07a8911ce8@xen.org>

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: 03 July 2020 12:18
> To: paul@xen.org; 'Michał Leszczyński' <michal.leszczynski@cert.pl>; xen-devel@lists.xenproject.org
> Cc: luwei.kang@intel.com; tamas.lengyel@intel.com; 'Andrew Cooper' <andrew.cooper3@citrix.com>;
> 'George Dunlap' <george.dunlap@citrix.com>; 'Ian Jackson' <ian.jackson@eu.citrix.com>; 'Jan Beulich'
> <jbeulich@suse.com>; 'Stefano Stabellini' <sstabellini@kernel.org>; 'Wei Liu' <wl@xen.org>
> Subject: Re: [PATCH v4 06/10] memory: batch processing in acquire_resource()
> 
> Hi,
> 
> On 03/07/2020 11:52, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Julien Grall <julien@xen.org>
> >> Sent: 03 July 2020 11:36
> >> To: Michał Leszczyński <michal.leszczynski@cert.pl>; xen-devel@lists.xenproject.org
> >> Cc: luwei.kang@intel.com; tamas.lengyel@intel.com; Andrew Cooper <andrew.cooper3@citrix.com>;
> George
> >> Dunlap <george.dunlap@citrix.com>; Ian Jackson <ian.jackson@eu.citrix.com>; Jan Beulich
> >> <jbeulich@suse.com>; Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>;
> paul@xen.org
> >> Subject: Re: [PATCH v4 06/10] memory: batch processing in acquire_resource()
> >>
> >> (+ Paul as the author XENMEM_acquire_resource)
> >>
> >> Hi,
> >>
> >> On 30/06/2020 13:33, 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.
> >>>
> >>> Signed-off-by: Michal Leszczynski <michal.leszczynski@cert.pl>
> >>> ---
> >>>    xen/common/memory.c | 32 +++++++++++++++++++++++++++++---
> >>>    1 file changed, 29 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/xen/common/memory.c b/xen/common/memory.c
> >>> index 714077c1e5..3ab06581a2 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
> >>> @@ -1077,8 +1079,17 @@ static int acquire_resource(
> >>>            return 0;
> >>>        }
> >>>
> >>> +    total_frames = xmar.nr_frames;
> >>
> >> On 32-bit, the start_extent would be 26-bits wide which is not enough to
> >> cover all the xmar.nr_frames. Therefore, you want that check that it is
> >> possible to encode a continuation. Something like:
> >>
> >> /* Is the size too large for us to encode a continuation? */
> >> if ( unlikely(xmar.nr_frames > (UINT_MAX >> MEMOP_EXTENT_SHIFT)) )
> >>
> >>> +
> >>> +    if ( *start_extent ) > +    {
> >>> +        xmar.frame += *start_extent;
> >>> +        xmar.nr_frames -= *start_extent;
> >>
> >> As start_extent is exposed to the guest, you want to check if it is not
> >> bigger than xmar.nr_frames.
> >>
> >>> +        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);
> >>
> >> The documentation of the hypercall suggests that if you pass NULL, then
> >> it will return the maximum number value for nr_frames supported by the
> >> implementation. So technically a domain cannot use more than
> >> ARRAY_SIZE(mfn_list).
> >>
> >> However, you new addition conflict with the documentation. Can you
> >> clarify how a domain will know that it can use more than
> >> ARRAY_SIZE(mfn_list)?
> >
> > The domain should not need to know. It should be told the maximum number of frames of the type it
> wants. If we have to carve that up into batches inside Xen then the caller should not need to care,
> right?
> 
> In the current implementation, we tell the guest how many frames it can
> request in a batch. This number may be much smaller that the maximum
> number of frames of the type.
> 
> Furthermore this value is not tie to the xmar.type. Therefore, it is
> valid for a guest to call this hypercall only once at boot to figure out
> the maximum batch.
> 
> So while the change you suggest looks a good idea, I don't think it is
> possible to do that with the current hypercall.
> 

Oh, I was clearly misremembering what the semantic was; I thought it was implementation max for the given type but indeed we do just return the array size, so we expect the caller to know the individual resource type limitations.
So, as Jan says, passing back UINT_MAX >> MEMOP_EXTENT_SHIFT seems to be what we need.

  Paul