Re: [PATCH v4 05/16] libs/guest: allocate various migration arrays just once

[Frediano Ziglio <freddy77@xxxxxxxxx>]

On Mon, 8 Jun 2026 at 16:49, Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
>
> On Wed, Jun 03, 2026 at 02:05:52PM +0100, Frediano Ziglio wrote:
> > From: Edwin Török <edwin.torok@xxxxxxxxxx>
> >
> > Allocate these array just once at the start of migration,
> > using the maximum batch size, and free them at the end.
> >
> > Signed-off-by: Edwin Török <edwin.torok@xxxxxxxxxx>
> > Signed-off-by: Frediano Ziglio <frediano.ziglio@xxxxxxxxxx>
>
> Jan made a comment about this patch (and how it related to a still
> pending patch of his):
>
> https://lore.kernel.org/xen-devel/e3f22fa6-c497-4afc-9498-12449548acfd@xxxxxxxx/
>
> That is still unresolved AFAICT.
>
> > --
> > Changes since v2:
> > - change prefix in subject.
> >
> > Changes since v3:
> > - fix comment style
> > ---
> >  tools/libs/guest/xg_sr_common.h | 13 +++++++
> >  tools/libs/guest/xg_sr_save.c   | 66 +++++++++++++--------------------
> >  2 files changed, 39 insertions(+), 40 deletions(-)
> >
> > diff --git a/tools/libs/guest/xg_sr_common.h 
> > b/tools/libs/guest/xg_sr_common.h
> > index f1573aefcb..82549b5589 100644
> > --- a/tools/libs/guest/xg_sr_common.h
> > +++ b/tools/libs/guest/xg_sr_common.h
> > @@ -209,6 +209,18 @@ static inline int update_blob(struct xc_sr_blob *blob,
> >      return 0;
> >  }
> >
> > +struct xc_sr_context_save_buffers
> > +{
> > +    xen_pfn_t batch_pfns[MAX_BATCH_SIZE];
> > +    xen_pfn_t mfns[MAX_BATCH_SIZE];
> > +    xen_pfn_t types[MAX_BATCH_SIZE];
> > +    int errors[MAX_BATCH_SIZE];
>
> FWIW: I would possibly place errors at the end of the structure.  It
> seems more natural and is the only array that has 4 byte alignment
> instead of 8 (on 64bits at least).
>

Moved. It disappears later in "libs/guest: finalize PoC" but won't hurt.

> > +    void *guest_data[MAX_BATCH_SIZE];
> > +    void *local_pages[MAX_BATCH_SIZE];
> > +    struct iovec iov[MAX_BATCH_SIZE + 2]; /* Headers + data. */
> > +    uint64_t rec_pfns[MAX_BATCH_SIZE];
> > +};
> > +
> >  struct xc_sr_context
> >  {
> >      xc_interface *xch;
> > @@ -244,6 +256,7 @@ struct xc_sr_context
> >              unsigned long *deferred_pages;
> >              unsigned long nr_deferred_pages;
> >              xc_hypercall_buffer_t dirty_bitmap_hbuf;
> > +            struct xc_sr_context_save_buffers *buffers;
> >          } save;
> >
> >          struct /* Restore data. */
> > diff --git a/tools/libs/guest/xg_sr_save.c b/tools/libs/guest/xg_sr_save.c
> > index 8c4e760f8d..7d8055a3f9 100644
> > --- a/tools/libs/guest/xg_sr_save.c
> > +++ b/tools/libs/guest/xg_sr_save.c
> > @@ -86,16 +86,16 @@ static int write_checkpoint_record(struct xc_sr_context 
> > *ctx)
> >  static int write_batch(struct xc_sr_context *ctx)
> >  {
> >      xc_interface *xch = ctx->xch;
> > -    xen_pfn_t *mfns = NULL, *types = NULL;
> > +    xen_pfn_t *mfns, *types;
> >      void *guest_mapping = NULL;
> > -    void **guest_data = NULL;
> > -    void **local_pages = NULL;
> > -    int *errors = NULL, rc = -1;
> > +    void **guest_data;
> > +    void **local_pages;
> > +    int *errors, rc = -1;
> >      unsigned int i, p, nr_pages = 0, nr_pages_mapped = 0;
> >      unsigned int nr_pfns = ctx->save.nr_batch_pfns;
> >      void *page, *orig_page;
> > -    uint64_t *rec_pfns = NULL;
> > -    struct iovec *iov = NULL; int iovcnt = 0;
> > +    uint64_t *rec_pfns;
> > +    struct iovec *iov; int iovcnt = 0;
> >      struct {
> >          struct xc_sr_rhdr rec;
> >          struct xc_sr_rec_page_data_header page_data;
> > @@ -105,26 +105,24 @@ static int write_batch(struct xc_sr_context *ctx)
> >      };
> >
> >      assert(nr_pfns != 0);
> > +    assert(nr_pfns <= MAX_BATCH_SIZE);
> > +    assert(ctx->save.buffers);
> >
> >      /* Mfns of the batch pfns. */
> > -    mfns = malloc(nr_pfns * sizeof(*mfns));
> > +    mfns = ctx->save.buffers->mfns;
> >      /* Types of the batch pfns. */
> > -    types = malloc(nr_pfns * sizeof(*types));
> > +    types = ctx->save.buffers->types;
> >      /* Errors from attempting to map the gfns. */
> > -    errors = malloc(nr_pfns * sizeof(*errors));
> > +    errors = ctx->save.buffers->errors;
> >      /* Pointers to page data to send.  Mapped gfns or local allocations. */
> > -    guest_data = calloc(nr_pfns, sizeof(*guest_data));
> > +    guest_data = ctx->save.buffers->guest_data;
> > +    memset(guest_data, 0, sizeof(*guest_data) * nr_pfns);
> >      /* Pointers to locally allocated pages.  Need freeing. */
> > -    local_pages = calloc(nr_pfns, sizeof(*local_pages));
> > +    local_pages = ctx->save.buffers->local_pages;
> > +    memset(local_pages, 0, sizeof(*local_pages) * nr_pfns);
>
> See below - I think it's possible to avoid the memset() and keep the
> same guarantees.
>

What's the issue with memset?
It's more consistent with the old "calloc" in the code and in the
compilers we use (gcc/clang) memset is a builtin that's optimized
better than setting as single fields.
Note that "guest_data" disappears in "libs/guest: fill directly iov
structure" commit.

> >      /* iovec[] for writev(). */
> > -    iov = malloc((nr_pfns + 2) * sizeof(*iov));
> > -
> > -    if ( !mfns || !types || !errors || !guest_data || !local_pages || !iov 
> > )
> > -    {
> > -        ERROR("Unable to allocate arrays for a batch of %u pages",
> > -              nr_pfns);
> > -        goto err;
> > -    }
> > +    iov = ctx->save.buffers->iov;
> > +    rec_pfns = ctx->save.buffers->rec_pfns;
> >
> >      for ( i = 0; i < nr_pfns; ++i )
> >      {
> > @@ -210,14 +208,6 @@ static int write_batch(struct xc_sr_context *ctx)
> >          }
> >      }
> >
> > -    rec_pfns = malloc(nr_pfns * sizeof(*rec_pfns));
> > -    if ( !rec_pfns )
> > -    {
> > -        ERROR("Unable to allocate %zu bytes of memory for page data pfn 
> > list",
> > -              nr_pfns * sizeof(*rec_pfns));
> > -        goto err;
> > -    }
> > -
> >      hdrs.rec.length = sizeof(hdrs.page_data);
> >      hdrs.rec.length += nr_pfns * sizeof(*rec_pfns);
> >      hdrs.rec.length += nr_pages * PAGE_SIZE;
> > @@ -267,17 +257,13 @@ static int write_batch(struct xc_sr_context *ctx)
> >      rc = ctx->save.nr_batch_pfns = 0;
> >
> >   err:
> > -    free(rec_pfns);
> >      if ( guest_mapping )
> >          xenforeignmemory_unmap(xch->fmem, guest_mapping, nr_pages_mapped);
> >      for ( i = 0; local_pages && i < nr_pfns; ++i )
> > +    {
> >          free(local_pages[i]);
> > -    free(iov);
> > -    free(local_pages);
> > -    free(guest_data);
> > -    free(errors);
> > -    free(types);
> > -    free(mfns);
> > +        local_pages[i] = NULL;
>
> If you are doing this cleanup here, you could also do guest_data[i] =
> NULL and avoid the memset, since at the start of each write_batch()
> the arrays will already be zeroed (either because they are allocated
> with calloc() on the first call, and always cleaned up in
> write_batch() after usage.
>

Given that there's a patch removing "guest_data" entirely, maybe I
would remove the "memset" for "local_pages" in another commit, maybe
"libs/guest: finalize PoC".

> Thanks, Roger.

Frediano