|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v4 05/16] libs/guest: allocate various migration arrays just once
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). > + 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.
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |