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

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

On Wed, Jun 03, 2026 at 02:05:52PM +0100, Frediano Ziglio wrote:
> From: Edwin Török <edwin.torok@citrix.com>
> 
> 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@citrix.com>
> Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>

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@suse.com/

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).

> +    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.

>      /* 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.

Thanks, Roger.