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 16:45:25 +0800 Message-ID: <5576A7A5.9020703@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> <55765A49.9010101@cn.fujitsu.com> <557693CA.9090700@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <557693CA.9090700@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/09/2015 03:20 PM, Andrew Cooper wrote: > On 09/06/2015 04:15, Yang Hongyang wrote: >> >> >> 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. >> >> ok. > > (Merging the other thread) > >> how about merge_secondary_dirty_bitmap()? > > Much better! > >> >>> >>>> +{ >>>> + 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. >> >> but the size is unknown for the caller.pfn_list[0] is the count of >> pfn. >> >>> >>>> + assert(pfn_list); >>> >>> This should turn into an error rather than an abort(). >> >> Even if there are no dirty pages on secondary, pfn_list shouldn't be >> NULL, it's just that pfn_list[0] will be 0. if pfn_list is NULL, >> there might be unexpected error happened. > > get_dirty_pfn() should be declared alongside a > > struct pfn_data > { > uint64_t count; > uint64_t *pfns; > }; > > and this function here should create one of these on the stack and pass > it by pointer to get_dirty_pfn(). I might also be tempted to rename > this to get_remote_logdirty() or similar, to indicate that it is a > source of logdirty data from something other than the current hypervisor. This is a callback, I can't find a way to pass pointer from libxc to libxl, libxl can not access the pointer data...The struct can be used for represent the data however. I like with the rename part, sounds much better. > >> >>> >>>> + >>>> + 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) ? >> >> It should be mandatory, it shouldn't be noop under COLO. perhaps we >> should >> add sanity check at the beginning. But problem is save side do not >> have a param >> passed from libxl to indicate the stream type(like checkpointed_stream in >> restore side). So we may need to add another XCFLAGS? Currently there is >> XCFLAGS_CHECKPOINTED which represents Remus, we might need to change >> this to >> XCFLAGS_STREAM_REMUS >> XCFLAGS_STREAM_COLO >> so that we can know what kind of stream we are handling? > > checkpointed_stream started out as a bugfix for a legacy stream > migration breakage. Really, this information should have been passed > right from the start. Did I miss the bugfix? is it not in upstream? > > It would probably be best to take the enum{} suggested elsewhere and > make it a top level ctx item, and have it present for both save and > restore, with sutable parameters passed in from the top. (When I am > finally able to take out the legacy code, there is going to be a severe > pruning/consolidation of the parameters.) This is what I thought when I saw the enum{} suggested. > > ~Andrew > >> >>> >>> ~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.