Re: [PATCH Remus v1 2/8] tools/libxc: reuse send_some_pages() in send_all_pages()

[Hongyang Yang <yanghy@cn.fujitsu.com>]

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 <yanghy@cn.fujitsu.com>
>
> 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.