From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yang Hongyang Subject: Re: [PATCH v4 09/14] libxc/save: adjust the memory allocation for migration Date: Wed, 13 May 2015 08:49:36 +0800 Message-ID: <55529FA0.2090500@cn.fujitsu.com> References: <1431429922-15344-1-git-send-email-yanghy@cn.fujitsu.com> <1431429922-15344-10-git-send-email-yanghy@cn.fujitsu.com> <5551EE9C.4030003@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5551EE9C.4030003@citrix.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: Andrew Cooper , 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 05/12/2015 08:14 PM, Andrew Cooper wrote: > 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. Ok, will drop the bracketing in all DECLARE_HYPERCALL_BUFFER_SHADOW() calls > >> >> 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 > >> } >> >> /* > > . > -- Thanks, Yang.