Re: [PATCH v3 02/10] migration/dirtyrate: Add RamlockDirtyInfo to store sampled page info

[Zheng Chuan]

On 2020/8/21 0:20, Dr. David Alan Gilbert wrote:
> * Chuan Zheng (zhengchuan@huawei.com) wrote:
>> Add RamlockDirtyInfo to store sampled page info of each ramblock.
>>
>> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
>> Signed-off-by: YanYing Zhuang <ann.zhuangyanying@huawei.com>
>> ---
>>  migration/dirtyrate.h | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h
>> index 914c363..9650566 100644
>> --- a/migration/dirtyrate.h
>> +++ b/migration/dirtyrate.h
>> @@ -19,6 +19,11 @@
>>   */
>>  #define DIRTYRATE_DEFAULT_SAMPLE_PAGES            256
>>  
>> +/*
>> + * Record ramblock idstr
>> + */
>> +#define RAMBLOCK_INFO_MAX_LEN                     256
>> +
>>  /* Take 1s as default for calculation duration */
>>  #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC          1
>>  
>> @@ -39,6 +44,19 @@ typedef enum {
>>      CAL_DIRTY_RATE_END,
>>  } CalculatingDirtyRateState;
>>  
>> +/*
>> + * Store dirtypage info for each ramblock.
>> + */
>> +struct RamblockDirtyInfo {
>> +    char idstr[RAMBLOCK_INFO_MAX_LEN]; /* idstr for each ramblock */
>> Can you remind me; why not just use RAMBlock* here of the block you're
> interested in, rather than storing the name?
>
idstr is used to store which ramblock is sampled page in, we test it in
find_page_matched().
so you mean we just RAMBlock*, and take idstr out of RAMBlock* when it need to
find matched page?

>> +    uint8_t *ramblock_addr; /* base address of ramblock we measure */
>> +    size_t ramblock_pages; /* sum of dividation by 4K pages for ramblock */
> 
> 'dividation' is the wrong word, and 'sum' is only needed where you're
> adding things together.  I think this is 'ramblock size in TARGET_PAGEs'
> 
>> +    size_t *sample_page_vfn; /* relative offset address for sampled page */
>> +    unsigned int sample_pages_count; /* sum of sampled pages */
>> +    unsigned int sample_dirty_count; /* sum of dirty pages we measure */
> 
> These are both 'count' rather than 'sum'
> 
OK, will be fixed in V4:)

>> +    uint8_t *hash_result; /* array of hash result for sampled pages */
>> +};
>> +
>>  void *get_dirtyrate_thread(void *arg);
>>  #endif
>>  
>> -- 
>> 1.8.3.1
>>