Re: [PATCH v5 06/12] migration/dirtyrate: Record hash results for each sampled page

[David Edmondson <dme@dme.org>]

On Wednesday, 2020-08-26 at 13:30:16 +01, Dr. David Alan Gilbert wrote:

> * David Edmondson (dme@dme.org) wrote:
>> On Monday, 2020-08-24 at 17:14:34 +08, Chuan Zheng wrote:
>> 
>> > Record hash results for each sampled page, crc32 is taken to calculate
>> > hash results for each sampled 4K-page.
>> >
>> > Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
>> > Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
>> > ---
>> >  migration/dirtyrate.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++
>> >  migration/dirtyrate.h |  15 ++++++
>> >  2 files changed, 151 insertions(+)
>> >
>> > diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
>> > index f6a94d8..66de426 100644
>> > --- a/migration/dirtyrate.c
>> > +++ b/migration/dirtyrate.c
>> > @@ -10,6 +10,7 @@
>> >   * See the COPYING file in the top-level directory.
>> >   */
>> >  
>> > +#include <zlib.h>
>> >  #include "qemu/osdep.h"
>> >  #include "qapi/error.h"
>> >  #include "crypto/hash.h"
>> > @@ -66,6 +67,141 @@ static void update_dirtyrate(uint64_t msec)
>> >      DirtyStat.dirty_rate = dirtyrate;
>> >  }
>> >  
>> > +/*
>> > + * get hash result for the sampled memory with length of 4K byte in ramblock,
>> > + * which starts from ramblock base address.
>> > + */
>> > +static uint32_t get_ramblock_vfn_hash(struct RamblockDirtyInfo *info,
>> > +                                      uint64_t vfn)
>> > +{
>> > +    struct iovec iov_array;
>> 
>> There's no need for an iovec now that crc32() is being used.
>> 
>> > +    uint32_t crc;
>> > +
>> > +    iov_array.iov_base = info->ramblock_addr +
>> > +                         vfn * DIRTYRATE_SAMPLE_PAGE_SIZE;
>> > +    iov_array.iov_len = DIRTYRATE_SAMPLE_PAGE_SIZE;
>> > +
>> > +    crc = crc32(0, iov_array.iov_base, iov_array.iov_len);
>> > +
>> > +    return crc;
>> > +}
>> > +
>> > +static int save_ramblock_hash(struct RamblockDirtyInfo *info)
>> > +{
>> > +    unsigned int sample_pages_count;
>> > +    int i;
>> > +    int ret = -1;
>> 
>> There's no need to initialise "ret".
>> 
>> > +    GRand *rand = g_rand_new();
>> 
>> Calling g_rand_new() when the result may not be used (because of the
>> various conditions immediately below) seems as though it might waste
>> entropy. Could this be pushed down just above the loop? It would even
>> get rid of the gotos ;-)
>> 
>> > +    sample_pages_count = info->sample_pages_count;
>> > +
>> > +    /* ramblock size less than one page, return success to skip this ramblock */
>> > +    if (unlikely(info->ramblock_pages == 0 || sample_pages_count == 0)) {
>> > +        ret = 0;
>> > +        goto out;
>> > +    }
>> > +
>> > +    info->hash_result = g_try_malloc0_n(sample_pages_count,
>> > +                                        sizeof(uint32_t));
>> > +    if (!info->hash_result) {
>> > +        ret = -1;
>> > +        goto out;
>> > +    }
>> > +
>> > +    info->sample_page_vfn = g_try_malloc0_n(sample_pages_count,
>> > +                                            sizeof(uint64_t));
>> > +    if (!info->sample_page_vfn) {
>> > +        g_free(info->hash_result);
>> > +        ret = -1;
>> > +        goto out;
>> > +    }
>> > +
>> > +    for (i = 0; i < sample_pages_count; i++) {
>> > +        info->sample_page_vfn[i] = g_rand_int_range(rand, 0,
>> > +                                                    info->ramblock_pages - 1);
>> > +        info->hash_result[i] = get_ramblock_vfn_hash(info,
>> > +                                                     info->sample_page_vfn[i]);
>> > +    }
>> > +    ret = 0;
>> > +
>> > +out:
>> > +    g_rand_free(rand);
>> > +    return ret;
>> > +}
>> > +
>> > +static void get_ramblock_dirty_info(RAMBlock *block,
>> > +                                    struct RamblockDirtyInfo *info,
>> > +                                    struct DirtyRateConfig *config)
>> > +{
>> > +    uint64_t sample_pages_per_gigabytes = config->sample_pages_per_gigabytes;
>> > +
>> > +    /* Right shift 30 bits to calc block size in GB */
>> > +    info->sample_pages_count = (qemu_ram_get_used_length(block) *
>> > +                                sample_pages_per_gigabytes) >>
>> > +                                DIRTYRATE_PAGE_SHIFT_GB;
>> 
>> Doing the calculation this way around seems odd. Why was it not:
>> 
>>     info->sample_pages_count = (qemu_ram_get_used_length(block) >>
>>                                 DIRTYRATE_PAGE_SHIFT_GB) *
>>                                 sample_pages_per_gigabytes;
>> 
>> ?
>
> Because that would give 0 for a 0.5GB block

Ouch, obviously :-) Thanks.

dme.
-- 
And removed his hat, in respect of her presence.