From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hongyang Yang Subject: Re: [PATCH Remus v1 1/8] tools/libxc: adjust the memory allocation for migration Date: Thu, 7 May 2015 21:42:14 +0800 Message-ID: <554B6BB6.6050202@cn.fujitsu.com> References: <1430980646-316-1-git-send-email-yanghy@cn.fujitsu.com> <1430980646-316-2-git-send-email-yanghy@cn.fujitsu.com> <554B34F7.5000009@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <554B34F7.5000009@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, eddie.dong@intel.com, yunhong.jiang@intel.com, ian.jackson@eu.citrix.com, rshriram@cs.ubc.ca List-Id: xen-devel@lists.xenproject.org On 05/07/2015 05:48 PM, Andrew Cooper wrote: > On 07/05/15 07:37, 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 >> --- >> tools/libxc/xc_sr_save.c | 53 +++++++++++++++++++----------------------------- >> 1 file changed, 21 insertions(+), 32 deletions(-) >> >> diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c >> index 5d9c267..7fed668 100644 >> --- a/tools/libxc/xc_sr_save.c >> +++ b/tools/libxc/xc_sr_save.c >> @@ -3,6 +3,8 @@ >> >> #include "xc_sr_common.h" >> >> +DECLARE_HYPERCALL_BUFFER(unsigned long, to_send); > > This unfortunately causes an issue when concurrent calls to > xc_domain_save() in the same process. While this is a highly > ill-advised action, I did try to avoid breaking it. > > Please move this declaration into the ctx.save union. I know the best way is to put this into ctx.save union, but I haven't found a method to put it in, the DECLARE_HYPERCALL_BUFFER macro can not be used there, should I just define a unsigned long var at ctx.save union, and use other macro(what macro?) define at save()? > >> + >> /* >> * Writes an Image header and Domain header into the stream. >> */ >> @@ -475,25 +477,11 @@ 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, to_send); >> xc_shadow_op_stats_t stats = { 0, ctx->save.p2m_size }; >> char *progress_str = NULL; >> unsigned x; >> int rc = -1; >> >> - to_send = xc_hypercall_buffer_alloc_pages( >> - xch, to_send, 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 || !to_send || !ctx->save.deferred_pages ) >> - { >> - ERROR("Unable to allocate memory for to_{send,fix}/batch bitmaps"); >> - goto out; >> - } >> - >> rc = enable_logdirty(ctx); >> if ( rc ) >> goto out; >> @@ -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, to_send, >> - 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,9 +604,6 @@ 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; >> } >> >> @@ -645,6 +615,20 @@ static int save(struct xc_sr_context *ctx, uint16_t guest_type) >> xc_interface *xch = ctx->xch; >> int rc, saved_rc = 0, saved_errno = 0; >> >> + to_send = xc_hypercall_buffer_alloc_pages( >> + xch, to_send, NRPAGES(bitmap_size(ctx->save.p2m_size))); > > This allocation only needs to be made if ctx->save.live is set. For > plain suspend and resume, logdirty handling is not required. > >> + 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 || !to_send || !ctx->save.deferred_pages ) >> + { >> + ERROR("Unable to allocate memory for to_{send,fix}/batch bitmaps"); >> + rc = -1; >> + errno = ENOMEM; >> + goto err; >> + } >> + > > Instead of complicating save() like this, could you introduce two new > static functions setup() and cleanup() which subsume the > ctx->save.ops.{setup,cleanup}() calls and also do these allocations and > free. Sure, will do this in next version. > > I think the SHADOW_OP_OFF hypercall can also be moved into the cleanup() > function. > > ~Andrew > >> IPRINTF("Saving domain %d, type %s", >> ctx->domid, dhdr_type_to_str(guest_type)); >> >> @@ -704,6 +688,11 @@ static int save(struct xc_sr_context *ctx, uint16_t guest_type) >> if ( rc ) >> PERROR("Failed to clean up"); >> >> + xc_hypercall_buffer_free_pages(xch, to_send, >> + NRPAGES(bitmap_size(ctx->save.p2m_size))); >> + free(ctx->save.deferred_pages); >> + free(ctx->save.batch_pfns); >> + >> if ( saved_rc ) >> { >> rc = saved_rc; > > . > -- Thanks, Yang.