Re: [RFC PATCH 5/8] migration/dirtyrate: Compare hash results for recorded ramblock

[Zheng Chuan]

On 2020/8/5 1:29, Dr. David Alan Gilbert wrote:
> * Chuan Zheng (zhengchuan@huawei.com) wrote:
>> From: Zheng Chuan <zhengchuan@huawei.com>
>>
>> Compare hash results for recorded ramblock.
>>
>> Signed-off-by: Zheng Chuan <zhengchuan@huawei.com>
>> Signed-off-by: YanYing Zhang <ann.zhuangyanying@huawei.com>
>> ---
>>  migration/dirtyrate.c | 77 
>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 77 insertions(+)
>>
>> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
>> index 45cfc91..7badc53 100644
>> --- a/migration/dirtyrate.c
>> +++ b/migration/dirtyrate.c
>> @@ -202,6 +202,83 @@ static int record_block_hash_info(struct 
>> dirtyrate_config config,
>>      return 0;
>>  }
>>  
>> +static int cal_block_dirty_rate(struct block_dirty_info *info)
>> +{
>> +    uint8_t *md = NULL;
>> +    size_t hash_len;
>> +    int i;
>> +    int ret = 0;
>> +
>> +    hash_len = qcrypto_hash_digest_len(QCRYPTO_HASH_ALG_MD5);
>> +    md = g_new0(uint8_t, hash_len);
> 
> Is 'hash_len' actually constant for a given algorithm, like MD5 ?
> i.e. can we just have a nice fixed size array?
> 
>> +    for (i = 0; i < info->sample_pages_count; i++) {
>> +        ret = get_block_vfn_hash(info, info->sample_page_vfn[i], &md, 
>> &hash_len);
>> +        if (ret < 0) {
>> +            goto out;
>> +        }
>> +
>> +        if (memcmp(md, info->hash_result + i * hash_len, hash_len) != 0) {
>> +            info->sample_dirty_count++;
> 
> When the page doesn't match, do we have to update info->hash_result with
> the new hash?   If the page is only modified once, and we catch it on
> this cycle, we wouldn't want to catch it next time around.
> 
For now, we only support calculate once for each qmp command, thus there is no 
need
to update it.

However, it is indeed in our plan to add support for calculate multiple times 
for each qmp command to enhance
dirty rate preciseness:)

>> +        }
>> +    }
>> +
>> +out:
>> +    g_free(md);
>> +    return ret;
>> +}
>> +
>> +static bool find_block_matched(RAMBlock *block, struct block_dirty_info 
>> *infos,
>> +                               int count, struct block_dirty_info **matched)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < count; i++) {
>> +        if (!strcmp(infos[i].idstr, qemu_ram_get_idstr(block))) {
>> +            break;
>> +        }
>> +    }
>> +
>> +    if (i == count) {
>> +        return false;
>> +    }
>> +
>> +    if (infos[i].block_addr != qemu_ram_get_host_addr(block) ||
>> +        infos[i].block_pages !=
>> +            (qemu_ram_get_used_length(block) >> DIRTYRATE_PAGE_SIZE_SHIFT)) 
>> {
> 
> How does this happen?
> 
>> +        return false;
>> +    }
>> +
>> +    *matched = &infos[i];
>> +    return true;
>> +}
>> +
>> +static int compare_block_hash_info(struct block_dirty_info *info, int 
>> block_index)
>> +{
>> +    struct block_dirty_info *block_dinfo = NULL;
>> +    RAMBlock *block = NULL;
>> +
>> +    RAMBLOCK_FOREACH_MIGRATABLE(block) {
>> +        if (ram_block_skip(block) < 0) {
>> +            continue;
>> +        }
>> +        block_dinfo = NULL;
>> +        if (!find_block_matched(block, info, block_index + 1, 
>> &block_dinfo)) {
>> +            continue;
>> +        }
>> +        if (cal_block_dirty_rate(block_dinfo) < 0) {
>> +            return -1;
>> +        }
>> +        update_dirtyrate_stat(block_dinfo);
>> +    }
>> +    if (!dirty_stat.total_sample_count) {
>> +        return -1;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +
>>  static void calculate_dirtyrate(struct dirtyrate_config config, int64_t 
>> time)
>>  {
>>      /* todo */
>> -- 
>> 1.8.3.1
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
> 
> .
>