From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v4 09/14] libxc/save: adjust the memory allocation for migration Date: Tue, 12 May 2015 13:14:20 +0100 Message-ID: <5551EE9C.4030003@citrix.com> References: <1431429922-15344-1-git-send-email-yanghy@cn.fujitsu.com> <1431429922-15344-10-git-send-email-yanghy@cn.fujitsu.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1431429922-15344-10-git-send-email-yanghy@cn.fujitsu.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Yang Hongyang , xen-devel@lists.xen.org Cc: wei.liu2@citrix.com, ian.campbell@citrix.com, wency@cn.fujitsu.com, ian.jackson@eu.citrix.com, yunhong.jiang@intel.com, eddie.dong@intel.com, rshriram@cs.ubc.ca List-Id: xen-devel@lists.xenproject.org On 12/05/15 12:25, Yang Hongyang wrote: > Move the memory allocation before the concrete live/nolive save > in order to avoid the free/alloc memory loop when using Remus. > > Signed-off-by: Yang Hongyang > CC: Ian Campbell > CC: Ian Jackson > CC: Wei Liu > CC: Andrew Cooper > --- > tools/libxc/xc_sr_common.h | 1 + > tools/libxc/xc_sr_save.c | 61 ++++++++++++++++++++++------------------------ > 2 files changed, 30 insertions(+), 32 deletions(-) > > diff --git a/tools/libxc/xc_sr_common.h b/tools/libxc/xc_sr_common.h > index c0f90d4..276d00a 100644 > --- a/tools/libxc/xc_sr_common.h > +++ b/tools/libxc/xc_sr_common.h > @@ -190,6 +190,7 @@ struct xc_sr_context > unsigned nr_batch_pfns; > unsigned long *deferred_pages; > unsigned long nr_deferred_pages; > + xc_hypercall_buffer_t dirty_bitmap_hbuf; > } save; > > struct /* Restore data. */ > diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c > index 368aacb..0953c35 100644 > --- a/tools/libxc/xc_sr_save.c > +++ b/tools/libxc/xc_sr_save.c > @@ -475,24 +475,12 @@ static int update_progress_string(struct xc_sr_context *ctx, > static int send_domain_memory_live(struct xc_sr_context *ctx) > { > xc_interface *xch = ctx->xch; > - DECLARE_HYPERCALL_BUFFER(unsigned long, dirty_bitmap); > xc_shadow_op_stats_t stats = { 0, ctx->save.p2m_size }; > char *progress_str = NULL; > unsigned x; > int rc = -1; > - > - dirty_bitmap = xc_hypercall_buffer_alloc_pages( > - xch, dirty_bitmap, NRPAGES(bitmap_size(ctx->save.p2m_size))); > - > - ctx->save.batch_pfns = malloc(MAX_BATCH_SIZE * > - sizeof(*ctx->save.batch_pfns)); > - ctx->save.deferred_pages = calloc(1, bitmap_size(ctx->save.p2m_size)); > - > - if ( !ctx->save.batch_pfns || !dirty_bitmap || !ctx->save.deferred_pages ) > - { > - ERROR("Unable to allocate memory for to_{send,fix}/batch bitmaps"); > - goto out; > - } > + DECLARE_HYPERCALL_BUFFER_SHADOW(unsigned long, dirty_bitmap, > + (&ctx->save.dirty_bitmap_hbuf)); You can drop this extra bracketing now that DECLARE_HYPERCALL_BUFFER_SHADOW() has been fixed. > > rc = enable_logdirty(ctx); > if ( rc ) > @@ -593,10 +581,6 @@ static int send_domain_memory_live(struct xc_sr_context *ctx) > out: > xc_set_progress_prefix(xch, NULL); > free(progress_str); > - xc_hypercall_buffer_free_pages(xch, dirty_bitmap, > - NRPAGES(bitmap_size(ctx->save.p2m_size))); > - free(ctx->save.deferred_pages); > - free(ctx->save.batch_pfns); > return rc; > } > > @@ -609,17 +593,6 @@ static int send_domain_memory_nonlive(struct xc_sr_context *ctx) > xc_interface *xch = ctx->xch; > int rc = -1; > > - ctx->save.batch_pfns = malloc(MAX_BATCH_SIZE * > - sizeof(*ctx->save.batch_pfns)); > - ctx->save.deferred_pages = calloc(1, bitmap_size(ctx->save.p2m_size)); > - > - if ( !ctx->save.batch_pfns || !ctx->save.deferred_pages ) > - { > - PERROR("Failed to allocate memory for nonlive tracking structures"); > - errno = ENOMEM; > - goto err; > - } > - > rc = suspend_domain(ctx); > if ( rc ) > goto err; > @@ -631,15 +604,30 @@ static int send_domain_memory_nonlive(struct xc_sr_context *ctx) > goto err; > > err: > - free(ctx->save.deferred_pages); > - free(ctx->save.batch_pfns); > - > return rc; > } > > static int setup(struct xc_sr_context *ctx) > { > + xc_interface *xch = ctx->xch; > int rc; > + DECLARE_HYPERCALL_BUFFER_SHADOW(unsigned long, dirty_bitmap, > + (&ctx->save.dirty_bitmap_hbuf)); > + > + dirty_bitmap = xc_hypercall_buffer_alloc_pages( > + xch, dirty_bitmap, NRPAGES(bitmap_size(ctx->save.p2m_size))); > + ctx->save.batch_pfns = malloc(MAX_BATCH_SIZE * > + sizeof(*ctx->save.batch_pfns)); > + ctx->save.deferred_pages = calloc(1, bitmap_size(ctx->save.p2m_size)); > + > + if ( !ctx->save.batch_pfns || !dirty_bitmap || !ctx->save.deferred_pages ) > + { > + ERROR("Unable to allocate memory for dirty bitmaps, batch pfns and" > + " deferred pages"); > + rc = -1; > + errno = ENOMEM; > + goto err; > + } > > rc = ctx->save.ops.setup(ctx); > if ( rc ) > @@ -655,12 +643,21 @@ static int setup(struct xc_sr_context *ctx) > static void cleanup(struct xc_sr_context *ctx) > { > xc_interface *xch = ctx->xch; > + DECLARE_HYPERCALL_BUFFER_SHADOW(unsigned long, dirty_bitmap, > + (&ctx->save.dirty_bitmap_hbuf)); And also drop this bracketing. > + > > xc_shadow_control(xch, ctx->domid, XEN_DOMCTL_SHADOW_OP_OFF, > NULL, 0, NULL, 0, NULL); > > if ( ctx->save.ops.cleanup(ctx) ) > PERROR("Failed to clean up"); > + > + xc_hypercall_buffer_free_pages(xch, dirty_bitmap, > + NRPAGES(bitmap_size(ctx->save.p2m_size))); > + free(ctx->save.deferred_pages); > + free(ctx->save.batch_pfns); > + Stray blank line. Otherwise, Reviewed-by: Andrew Cooper > } > > /*