From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yang Hongyang Subject: Re: [PATCH v6 COLO 06/15] libxc/save: support COLO save Date: Tue, 9 Jun 2015 11:18:37 +0800 Message-ID: <55765B0D.2000403@cn.fujitsu.com> References: <1433735159-26739-1-git-send-email-yanghy@cn.fujitsu.com> <1433735159-26739-7-git-send-email-yanghy@cn.fujitsu.com> <557592C0.6020709@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <557592C0.6020709@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, guijianfeng@cn.fujitsu.com, yunhong.jiang@intel.com, eddie.dong@intel.com, rshriram@cs.ubc.ca, ian.jackson@eu.citrix.com List-Id: xen-devel@lists.xenproject.org On 06/08/2015 09:04 PM, Andrew Cooper wrote: > On 08/06/15 04:45, Yang Hongyang wrote: >> call callbacks->get_dirty_pfn() after suspend primary vm to >> get dirty pages on secondary vm, and send pages both dirty on >> primary/secondary to secondary. >> >> Signed-off-by: Yang Hongyang >> Signed-off-by: Wen Congyang >> CC: Andrew Cooper >> --- >> tools/libxc/xc_sr_save.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 48 insertions(+), 1 deletion(-) >> >> diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c >> index d63b783..cda61ed 100644 >> --- a/tools/libxc/xc_sr_save.c >> +++ b/tools/libxc/xc_sr_save.c >> @@ -515,6 +515,31 @@ static int send_memory_live(struct xc_sr_context *ctx) >> return rc; >> } >> >> +static int update_dirty_bitmap(uint8_t *(*get_dirty_pfn)(void *), void *data, >> + unsigned long p2m_size, unsigned long *bitmap) > > This function should take a ctx rather than having the caller expand 3 > parameters. Also, "update_dirty_bitmap" is a little misleading, as it > isn't querying the hypervisor for the dirty bitmap. how about merge_secondary_dirty_bitmap()? > >> +{ >> + uint64_t *pfn_list; >> + uint64_t count, i; >> + uint64_t pfn; >> + >> + pfn_list = (uint64_t *)get_dirty_pfn(data); > > This looks like a recipe for width-errors. The get_dirty_pfn() call > should take a pointer to a struct for it to fill. > >> + assert(pfn_list); > > This should turn into an error rather than an abort(). > >> + >> + count = pfn_list[0]; >> + for (i = 0; i < count; i++) { > > style > >> + pfn = pfn_list[i + 1]; >> + if (pfn > p2m_size) { >> + errno = EINVAL; >> + return -1; >> + } >> + >> + set_bit(pfn, bitmap); >> + } >> + >> + free(pfn_list); >> + return 0; >> +} >> + >> /* >> * Suspend the domain and send dirty memory. >> * This is the last iteration of the live migration and the >> @@ -555,6 +580,19 @@ static int suspend_and_send_dirty(struct xc_sr_context *ctx) >> >> bitmap_or(dirty_bitmap, ctx->save.deferred_pages, ctx->save.p2m_size); >> >> + if ( !ctx->save.live && ctx->save.callbacks->get_dirty_pfn ) >> + { > > Shouldn't get_dirty_pfn be mandatory for COLO streams (even if it is a > noop to start with) ? > > ~Andrew > >> + rc = update_dirty_bitmap(ctx->save.callbacks->get_dirty_pfn, >> + ctx->save.callbacks->data, >> + ctx->save.p2m_size, >> + dirty_bitmap); >> + if ( rc ) >> + { >> + PERROR("Failed to get secondary vm's dirty pages"); >> + goto out; >> + } >> + } >> + >> rc = send_dirty_pages(ctx, stats.dirty_count + ctx->save.nr_deferred_pages); >> if ( rc ) >> goto out; >> @@ -784,7 +822,16 @@ static int save(struct xc_sr_context *ctx, uint16_t guest_type) >> if ( rc ) >> goto err; >> >> - ctx->save.callbacks->postcopy(ctx->save.callbacks->data); >> + rc = ctx->save.callbacks->postcopy(ctx->save.callbacks->data); >> + if ( !rc ) { >> + if ( !errno ) >> + { >> + /* Postcopy request failed (without errno, using EINVAL) */ >> + errno = EINVAL; >> + } >> + rc = -1; >> + goto err; >> + } >> >> rc = ctx->save.callbacks->checkpoint(ctx->save.callbacks->data); >> if ( rc <= 0 ) > > . > -- Thanks, Yang.