On Wednesday, 2020-08-26 at 13:30:16 +01, Dr. David Alan Gilbert wrote:
> * David Edmondson (d...@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 <zhengch...@huawei.com>
>> > Signed-off-by: YanYing Zhuang <ann.zhuangyany...@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.
