Re: [PATCH v6 06/16] libs/call: cache up to 4 pages in hypercall bounce buffers

[Frediano Ziglio <freddy77@xxxxxxxxx>]

On Tue, 7 Jul 2026 at 14:51, Anthony PERARD <anthony.perard@xxxxxxxxxx> wrote:
>
> On Fri, Jun 19, 2026 at 02:04:51PM +0100, Frediano Ziglio wrote:
> > From: Edwin Török <edwin.torok@xxxxxxxxxx>
> >
> > During migration there are a lot of mmap/munmap calls,
> > because `xc_get_pfn_type_batch` exceeds the default hypercall bounce
> > buffer cache size, and needs to allocate every time it is called.
>
> I think xc_get_pfn_type_batch() would allocate a buffer of 2 page top,
> in write_batch(), right ?
>

Yes. That however does not contradict the sense of the sentence (or
even the commit message).

> >
> > `munmap` is slow, especially in a PV Dom0 (takes an emulation fault),
> > so is best avoided.
> >
> > Eventually it'd be good if the memory pool from  xmalloc_tlsf.c
> > was reused here, but for now make it handle the commonly encountered
> > sizes (so far up to 4 pages).
> >

If a program uses 3/4 pages it will use the additional cache, if not
there's no much difference.

> > Signed-off-by: Edwin Török <edwin.torok@xxxxxxxxxx>
> > Signed-off-by: Frediano Ziglio <frediano.ziglio@xxxxxxxxxx>
> > --
> > Changes since v2:
> > - change prefix in subject.
> >
> > Changes since v4:
> > - fix off-by-one bug.
> >
> > Changes since v5:
> > - avoids potential buffer underflow if nr_pages is 0 calling cache_alloc.
> > ---
> >  tools/libs/call/buffer.c  | 31 ++++++++++++++++++++-----------
> >  tools/libs/call/core.c    |  3 ++-
> >  tools/libs/call/private.h |  8 +++++---
> >  3 files changed, 27 insertions(+), 15 deletions(-)
> >
> > diff --git a/tools/libs/call/buffer.c b/tools/libs/call/buffer.c
> > index 155e4f9d43..2f0515c273 100644
> > --- a/tools/libs/call/buffer.c
> > +++ b/tools/libs/call/buffer.c
> > @@ -49,6 +49,9 @@ static void *cache_alloc(xencall_handle *xcall, size_t 
> > nr_pages)
> >  {
> >      void *p = NULL;
> >
> > +    if ( nr_pages == 0 )
> > +        return NULL;
>
> By doing that check here, we don't update the stat anymore. And it's
> getting out-of-sync with the updates done in cache_free().
>
> Before, we where returning a cache entry for that, and cache_hit++. I
> think it's ok to return cache_miss++ instead.
>

Well... requesting 0 pages is weird by definition, even malloc(0) is
not well defined.
In theory in this case returning NULL would cause cache_free to not be
called as filtered by xencall_free_buffer_pages.

I think the most symmetric think would be adding a similar test in
cache_free, like

static int cache_free(xencall_handle *xcall, void *p, size_t nr_pages)
{
    int rc = 0;

    if ( nr_pages == 0 )
        return 1;

    cache_lock(xcall);


(the return 1 is needed to prevent the attempt to munmap the pointer
which does not make sense).

> >      cache_lock(xcall);
> >
> >      xcall->buffer_total_allocations++;
> > @@ -56,13 +59,13 @@ static void *cache_alloc(xencall_handle *xcall, size_t 
> > nr_pages)
> >      if ( xcall->buffer_current_allocations > 
> > xcall->buffer_maximum_allocations )
> >          xcall->buffer_maximum_allocations = 
> > xcall->buffer_current_allocations;
> >
> > -    if ( nr_pages > 1 )
> > +    if ( nr_pages > ARRAY_SIZE(xcall->buffer_cache) )
> >      {
> >          xcall->buffer_cache_toobig++;
> >      }
> > -    else if ( xcall->buffer_cache_nr > 0 )
> > +    else if ( xcall->buffer_cache_nr[nr_pages-1] > 0 )
> >      {
> > -        p = xcall->buffer_cache[--xcall->buffer_cache_nr];
> > +        p = 
> > xcall->buffer_cache[nr_pages-1][--xcall->buffer_cache_nr[nr_pages-1]];
> >          xcall->buffer_cache_hits++;
> >      }
> >      else
> > @@ -84,10 +87,10 @@ static int cache_free(xencall_handle *xcall, void *p, 
> > size_t nr_pages)
> >      xcall->buffer_total_releases++;
> >      xcall->buffer_current_allocations--;
> >
> > -    if ( nr_pages == 1 &&
> > -         xcall->buffer_cache_nr < BUFFER_CACHE_SIZE )
> > +    if ( nr_pages && nr_pages <= ARRAY_SIZE(xcall->buffer_cache) &&
> > +         xcall->buffer_cache_nr[nr_pages-1] < BUFFER_CACHE_SIZE )
> >      {
> > -        xcall->buffer_cache[xcall->buffer_cache_nr++] = p;
> > +        
> > xcall->buffer_cache[nr_pages-1][xcall->buffer_cache_nr[nr_pages-1]++] = p;
> >          rc = 1;
> >      }
> >
> > @@ -108,17 +111,23 @@ void buffer_release_cache(xencall_handle *xcall)
> >      DBGPRINTF("current allocations:%d maximum allocations:%d",
> >                xcall->buffer_current_allocations,
> >                xcall->buffer_maximum_allocations);
> > -    DBGPRINTF("cache current size:%d",
> > -              xcall->buffer_cache_nr);
> > +    for ( unsigned i = 0; i < ARRAY_SIZE(xcall->buffer_cache_nr); ++i )
> > +    {
> > +        DBGPRINTF("cache current size[%u pages]:%d", i+1,
> > +                xcall->buffer_cache_nr[i]);
> > +    }
> >      DBGPRINTF("cache hits:%d misses:%d toobig:%d",
> >                xcall->buffer_cache_hits,
> >                xcall->buffer_cache_misses,
> >                xcall->buffer_cache_toobig);
> >
> > -    while ( xcall->buffer_cache_nr > 0 )
> > +    for ( unsigned i = 0; i < ARRAY_SIZE(xcall->buffer_cache_nr); ++i )
> >      {
> > -        p = xcall->buffer_cache[--xcall->buffer_cache_nr];
> > -        osdep_free_pages(xcall, p, 1);
> > +        while ( xcall->buffer_cache_nr[i] > 0 )
> > +        {
> > +            p = xcall->buffer_cache[i][--xcall->buffer_cache_nr[i]];
> > +            osdep_free_pages(xcall, p, i + 1);
> > +        }
> >      }
> >
> >      cache_unlock(xcall);
> > diff --git a/tools/libs/call/core.c b/tools/libs/call/core.c
> > index 02c4f8e1ae..dd8877c1a0 100644
> > --- a/tools/libs/call/core.c
> > +++ b/tools/libs/call/core.c
> > @@ -14,6 +14,7 @@
> >   */
> >
> >  #include <stdlib.h>
> > +#include <string.h>
> >
> >  #include "private.h"
> >
> > @@ -44,7 +45,7 @@ xencall_handle *xencall_open(xentoollog_logger *logger, 
> > unsigned open_flags)
> >      xentoolcore__register_active_handle(&xcall->tc_ah);
> >
> >      xcall->flags = open_flags;
> > -    xcall->buffer_cache_nr = 0;
> > +    memset(xcall->buffer_cache_nr, 0, sizeof(xcall->buffer_cache_nr));
> >
> >      xcall->buffer_total_allocations = 0;
> >      xcall->buffer_total_releases = 0;
> > diff --git a/tools/libs/call/private.h b/tools/libs/call/private.h
> > index 9c3aa432ef..8e6a208975 100644
> > --- a/tools/libs/call/private.h
> > +++ b/tools/libs/call/private.h
> > @@ -31,13 +31,15 @@ struct xencall_handle {
> >      Xentoolcore__Active_Handle tc_ah;
> >
> >      /*
> > -     * A simple cache of unused, single page, hypercall buffers
> > +     * A simple cache of unused, small, hypercall buffers
> > +     * buffer_cache[i]'s size is (i+1) pages
> >       *
> >       * Protected by a global lock.
> >       */
> >  #define BUFFER_CACHE_SIZE 4
> > -    int buffer_cache_nr;
> > -    void *buffer_cache[BUFFER_CACHE_SIZE];
> > +#define BUFFER_CACHE_NRPAGES 4
> > +    int buffer_cache_nr[BUFFER_CACHE_NRPAGES];
> > +    void *buffer_cache[BUFFER_CACHE_NRPAGES][BUFFER_CACHE_SIZE];
> >
> >      /*
> >       * Hypercall buffer statistics. All protected by the global

Frediano