From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hongyang Yang Subject: Re: [PATCH Remus v1 2/8] tools/libxc: reuse send_some_pages() in send_all_pages() Date: Thu, 7 May 2015 21:48:58 +0800 Message-ID: <554B6D4A.70702@cn.fujitsu.com> References: <1430980646-316-1-git-send-email-yanghy@cn.fujitsu.com> <1430980646-316-3-git-send-email-yanghy@cn.fujitsu.com> <554B399E.2080105@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <554B399E.2080105@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 06:08 PM, Andrew Cooper wrote: > On 07/05/15 07:37, Yang Hongyang wrote: >> introduce bitmap_set() to set the entire bitmap. >> in send_all_pages(), set the entire bitmap and call send_some_pages(). >> >> Signed-off-by: Yang Hongyang > > Ah - I see now why you unconditionally allocated the to_send bitmap. > > I suppose it is probably better to have less divergence between the live > and non-live cases, and the size of the bitmap is substantially less > than other structures in use. > > In which case, can I suggest the following which partially supersedes my > review in patch 1. > > * Instead of having the bitmap called "to_send", having it called > something more appropriate like "dirty_bitmap". > * s/send_some_pages/send_dirty_pages/ and drop the *bitmap parameter. > ('entries' should stay as it is a useful sanity check). > > This way, send_all_page() logically sets all pages as dirty, then sends > any dirty pages. Yeah, "dirty_bitmap" sounds good, will use this name as well as change the name send_dirty_pages. > > ~Andrew > >> --- >> tools/libxc/xc_bitops.h | 5 +++++ >> tools/libxc/xc_sr_save.c | 41 +++++++++++------------------------------ >> 2 files changed, 16 insertions(+), 30 deletions(-) >> >> diff --git a/tools/libxc/xc_bitops.h b/tools/libxc/xc_bitops.h >> index dfce3b8..cd749f4 100644 >> --- a/tools/libxc/xc_bitops.h >> +++ b/tools/libxc/xc_bitops.h >> @@ -26,6 +26,11 @@ static inline unsigned long *bitmap_alloc(int nr_bits) >> return calloc(1, bitmap_size(nr_bits)); >> } >> >> +static inline void bitmap_set(unsigned long *addr, int nr_bits) >> +{ >> + memset(addr, 0xff, bitmap_size(nr_bits)); >> +} >> + >> static inline void bitmap_clear(unsigned long *addr, int nr_bits) >> { >> memset(addr, 0, bitmap_size(nr_bits)); >> diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c >> index 7fed668..2993ec3 100644 >> --- a/tools/libxc/xc_sr_save.c >> +++ b/tools/libxc/xc_sr_save.c >> @@ -346,36 +346,6 @@ static int suspend_domain(struct xc_sr_context *ctx) >> } >> >> /* >> - * Send all pages in the guests p2m. Used as the first iteration of the live >> - * migration loop, and for a non-live save. >> - */ >> -static int send_all_pages(struct xc_sr_context *ctx) >> -{ >> - xc_interface *xch = ctx->xch; >> - xen_pfn_t p; >> - int rc; >> - >> - for ( p = 0; p < ctx->save.p2m_size; ++p ) >> - { >> - rc = add_to_batch(ctx, p); >> - if ( rc ) >> - return rc; >> - >> - /* Update progress every 4MB worth of memory sent. */ >> - if ( (p & ((1U << (22 - 12)) - 1)) == 0 ) >> - xc_report_progress_step(xch, p, ctx->save.p2m_size); >> - } >> - >> - rc = flush_batch(ctx); >> - if ( rc ) >> - return rc; >> - >> - xc_report_progress_step(xch, ctx->save.p2m_size, >> - ctx->save.p2m_size); >> - return 0; >> -} >> - >> -/* >> * Send a subset of pages in the guests p2m, according to the provided bitmap. >> * Used for each subsequent iteration of the live migration loop. >> * >> @@ -417,6 +387,17 @@ static int send_some_pages(struct xc_sr_context *ctx, >> return 0; >> } >> >> +/* >> + * Send all pages in the guests p2m. Used as the first iteration of the live >> + * migration loop, and for a non-live save. >> + */ >> +static int send_all_pages(struct xc_sr_context *ctx) >> +{ >> + bitmap_set(to_send, ctx->save.p2m_size); >> + >> + return send_some_pages(ctx, to_send, ctx->save.p2m_size); >> +} >> + >> static int enable_logdirty(struct xc_sr_context *ctx) >> { >> xc_interface *xch = ctx->xch; > > . > -- Thanks, Yang.