From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v6 COLO 06/15] libxc/save: support COLO save Date: Mon, 8 Jun 2015 14:04:00 +0100 Message-ID: <557592C0.6020709@citrix.com> References: <1433735159-26739-1-git-send-email-yanghy@cn.fujitsu.com> <1433735159-26739-7-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: <1433735159-26739-7-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, 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 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. > +{ > + 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 )