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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Zheng Chuan (zhengchuan@huawei.com) wrote:
> 
> 
> 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?

I meant just use RAMBlock*, but I think I see why you don't;
because you only hold the RCU around each part separately, the RAMBlock
could disappear between the initial hash, and the later compare;  so
using the name makes it safe.

Dave

> >> +    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
> >>
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK