| Message ID | 1598260480-64862-4-git-send-email-zhengchuan@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | *** A Method for evaluating dirty page rate *** | expand |
On Monday, 2020-08-24 at 17:14:31 +08, Chuan Zheng wrote: > Add RamlockDirtyInfo to store sampled page info of each ramblock. > > Signed-off-by: Chuan Zheng <zhengchuan@huawei.com> > --- > migration/dirtyrate.h | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h > index 33669b7..70000da 100644 > --- a/migration/dirtyrate.h > +++ b/migration/dirtyrate.h > @@ -19,6 +19,11 @@ > */ > #define DIRTYRATE_DEFAULT_SAMPLE_PAGES 512 > > +/* > + * Record ramblock idstr > + */ > +#define RAMBLOCK_INFO_MAX_LEN 256 > + > /* Take 1s as default for calculation duration */ > #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC 1 > > @@ -27,6 +32,19 @@ struct DirtyRateConfig { > int64_t sample_period_seconds; /* time duration between two sampling */ > }; > > +/* > + * Store dirtypage info for each ramblock. > + */ > +struct RamblockDirtyInfo { > + char idstr[RAMBLOCK_INFO_MAX_LEN]; /* idstr for each ramblock */ > + uint8_t *ramblock_addr; /* base address of ramblock we measure */ > + uint64_t ramblock_pages; /* ramblock size in 4K-page */ It's probably a stupid question, but why not store a pointer to the RAMBlock rather than copying some of the details? > + uint64_t *sample_page_vfn; /* relative offset address for sampled page */ > + uint64_t sample_pages_count; /* count of sampled pages */ > + uint64_t sample_dirty_count; /* cout of dirty pages we measure */ "cout" -> "count" > + uint32_t *hash_result; /* array of hash result for sampled pages */ > +}; > + > void *get_dirtyrate_thread(void *arg); > #endif > > -- > 1.8.3.1 dme.
On 2020/8/26 17:29, David Edmondson wrote: > On Monday, 2020-08-24 at 17:14:31 +08, Chuan Zheng wrote: > >> Add RamlockDirtyInfo to store sampled page info of each ramblock. >> >> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com> >> --- >> migration/dirtyrate.h | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> >> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h >> index 33669b7..70000da 100644 >> --- a/migration/dirtyrate.h >> +++ b/migration/dirtyrate.h >> @@ -19,6 +19,11 @@ >> */ >> #define DIRTYRATE_DEFAULT_SAMPLE_PAGES 512 >> >> +/* >> + * Record ramblock idstr >> + */ >> +#define RAMBLOCK_INFO_MAX_LEN 256 >> + >> /* Take 1s as default for calculation duration */ >> #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC 1 >> >> @@ -27,6 +32,19 @@ struct DirtyRateConfig { >> int64_t sample_period_seconds; /* time duration between two sampling */ >> }; >> >> +/* >> + * Store dirtypage info for each ramblock. >> + */ >> +struct RamblockDirtyInfo { >> + char idstr[RAMBLOCK_INFO_MAX_LEN]; /* idstr for each ramblock */ >> + uint8_t *ramblock_addr; /* base address of ramblock we measure */ >> + uint64_t ramblock_pages; /* ramblock size in 4K-page */ > > It's probably a stupid question, but why not store a pointer to the > RAMBlock rather than copying some of the details? > >> + uint64_t *sample_page_vfn; /* relative offset address for sampled page */ >> + uint64_t sample_pages_count; /* count of sampled pages */ >> + uint64_t sample_dirty_count; /* cout of dirty pages we measure */ > > "cout" -> "count" > Hi, David. Thank you for review. Actually I have resent [PATCH V5], but it's my fault to forget to add resent tag which may lead to confusing. In new patch series, this spell mistake is fixed. Please refer to the newer patch series:) >> + uint32_t *hash_result; /* array of hash result for sampled pages */ >> +}; >> + >> void *get_dirtyrate_thread(void *arg); >> #endif >> >> -- >> 1.8.3.1 > > dme. >
On 2020/8/26 17:40, Zheng Chuan wrote: > > > On 2020/8/26 17:29, David Edmondson wrote: >> On Monday, 2020-08-24 at 17:14:31 +08, Chuan Zheng wrote: >> >>> Add RamlockDirtyInfo to store sampled page info of each ramblock. >>> >>> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com> >>> --- >>> migration/dirtyrate.h | 18 ++++++++++++++++++ >>> 1 file changed, 18 insertions(+) >>> >>> diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h >>> index 33669b7..70000da 100644 >>> --- a/migration/dirtyrate.h >>> +++ b/migration/dirtyrate.h >>> @@ -19,6 +19,11 @@ >>> */ >>> #define DIRTYRATE_DEFAULT_SAMPLE_PAGES 512 >>> >>> +/* >>> + * Record ramblock idstr >>> + */ >>> +#define RAMBLOCK_INFO_MAX_LEN 256 >>> + >>> /* Take 1s as default for calculation duration */ >>> #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC 1 >>> >>> @@ -27,6 +32,19 @@ struct DirtyRateConfig { >>> int64_t sample_period_seconds; /* time duration between two sampling */ >>> }; >>> >>> +/* >>> + * Store dirtypage info for each ramblock. >>> + */ >>> +struct RamblockDirtyInfo { >>> + char idstr[RAMBLOCK_INFO_MAX_LEN]; /* idstr for each ramblock */ >>> + uint8_t *ramblock_addr; /* base address of ramblock we measure */ >>> + uint64_t ramblock_pages; /* ramblock size in 4K-page */ >> >> It's probably a stupid question, but why not store a pointer to the >> RAMBlock rather than copying some of the details? >> Missing this question:) Since I 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. Again, Please refer to the newer patch series:) >>> + uint64_t *sample_page_vfn; /* relative offset address for sampled page */ >>> + uint64_t sample_pages_count; /* count of sampled pages */ >>> + uint64_t sample_dirty_count; /* cout of dirty pages we measure */ >> >> "cout" -> "count" >> > Hi, David. > Thank you for review. > Actually I have resent [PATCH V5], but it's my fault to forget to add resent tag which may lead to confusing. > In new patch series, this spell mistake is fixed. > Please refer to the newer patch series:) > >>> + uint32_t *hash_result; /* array of hash result for sampled pages */ >>> +}; >>> + >>> void *get_dirtyrate_thread(void *arg); >>> #endif >>> >>> -- >>> 1.8.3.1 >> >> dme. >>
* David Edmondson (dme@dme.org) wrote: > On Monday, 2020-08-24 at 17:14:31 +08, Chuan Zheng wrote: > > > Add RamlockDirtyInfo to store sampled page info of each ramblock. > > > > Signed-off-by: Chuan Zheng <zhengchuan@huawei.com> > > --- > > migration/dirtyrate.h | 18 ++++++++++++++++++ > > 1 file changed, 18 insertions(+) > > > > diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h > > index 33669b7..70000da 100644 > > --- a/migration/dirtyrate.h > > +++ b/migration/dirtyrate.h > > @@ -19,6 +19,11 @@ > > */ > > #define DIRTYRATE_DEFAULT_SAMPLE_PAGES 512 > > > > +/* > > + * Record ramblock idstr > > + */ > > +#define RAMBLOCK_INFO_MAX_LEN 256 > > + > > /* Take 1s as default for calculation duration */ > > #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC 1 > > > > @@ -27,6 +32,19 @@ struct DirtyRateConfig { > > int64_t sample_period_seconds; /* time duration between two sampling */ > > }; > > > > +/* > > + * Store dirtypage info for each ramblock. > > + */ > > +struct RamblockDirtyInfo { > > + char idstr[RAMBLOCK_INFO_MAX_LEN]; /* idstr for each ramblock */ > > + uint8_t *ramblock_addr; /* base address of ramblock we measure */ > > + uint64_t ramblock_pages; /* ramblock size in 4K-page */ > > It's probably a stupid question, but why not store a pointer to the > RAMBlock rather than copying some of the details? I think I figured that out in the last round; this code runs as: rcu lock { calculate initial CRCs } <sleep 1 second ish> rcu lock { calculate new CRCs } A RAMBlock might get deleted between the two. Dave > > + uint64_t *sample_page_vfn; /* relative offset address for sampled page */ > > + uint64_t sample_pages_count; /* count of sampled pages */ > > + uint64_t sample_dirty_count; /* cout of dirty pages we measure */ > > "cout" -> "count" > > > + uint32_t *hash_result; /* array of hash result for sampled pages */ > > +}; > > + > > void *get_dirtyrate_thread(void *arg); > > #endif > > > > -- > > 1.8.3.1 > > dme. > -- > Please forgive me if I act a little strange, for I know not what I do. >
On Wednesday, 2020-08-26 at 11:33:30 +01, Dr. David Alan Gilbert wrote: > * David Edmondson (dme@dme.org) wrote: >> On Monday, 2020-08-24 at 17:14:31 +08, Chuan Zheng wrote: >> >> > Add RamlockDirtyInfo to store sampled page info of each ramblock. >> > >> > Signed-off-by: Chuan Zheng <zhengchuan@huawei.com> >> > --- >> > migration/dirtyrate.h | 18 ++++++++++++++++++ >> > 1 file changed, 18 insertions(+) >> > >> > diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h >> > index 33669b7..70000da 100644 >> > --- a/migration/dirtyrate.h >> > +++ b/migration/dirtyrate.h >> > @@ -19,6 +19,11 @@ >> > */ >> > #define DIRTYRATE_DEFAULT_SAMPLE_PAGES 512 >> > >> > +/* >> > + * Record ramblock idstr >> > + */ >> > +#define RAMBLOCK_INFO_MAX_LEN 256 >> > + >> > /* Take 1s as default for calculation duration */ >> > #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC 1 >> > >> > @@ -27,6 +32,19 @@ struct DirtyRateConfig { >> > int64_t sample_period_seconds; /* time duration between two sampling */ >> > }; >> > >> > +/* >> > + * Store dirtypage info for each ramblock. >> > + */ >> > +struct RamblockDirtyInfo { >> > + char idstr[RAMBLOCK_INFO_MAX_LEN]; /* idstr for each ramblock */ >> > + uint8_t *ramblock_addr; /* base address of ramblock we measure */ >> > + uint64_t ramblock_pages; /* ramblock size in 4K-page */ >> >> It's probably a stupid question, but why not store a pointer to the >> RAMBlock rather than copying some of the details? > > I think I figured that out in the last round; this code runs as: > > rcu lock { > calculate initial CRCs > } > > <sleep 1 second ish> > rcu lock { > calculate new CRCs > } > > A RAMBlock might get deleted between the two. Makes sense, thanks. dme.
diff --git a/migration/dirtyrate.h b/migration/dirtyrate.h index 33669b7..70000da 100644 --- a/migration/dirtyrate.h +++ b/migration/dirtyrate.h @@ -19,6 +19,11 @@ */ #define DIRTYRATE_DEFAULT_SAMPLE_PAGES 512 +/* + * Record ramblock idstr + */ +#define RAMBLOCK_INFO_MAX_LEN 256 + /* Take 1s as default for calculation duration */ #define DEFAULT_FETCH_DIRTYRATE_TIME_SEC 1 @@ -27,6 +32,19 @@ struct DirtyRateConfig { int64_t sample_period_seconds; /* time duration between two sampling */ }; +/* + * Store dirtypage info for each ramblock. + */ +struct RamblockDirtyInfo { + char idstr[RAMBLOCK_INFO_MAX_LEN]; /* idstr for each ramblock */ + uint8_t *ramblock_addr; /* base address of ramblock we measure */ + uint64_t ramblock_pages; /* ramblock size in 4K-page */ + uint64_t *sample_page_vfn; /* relative offset address for sampled page */ + uint64_t sample_pages_count; /* count of sampled pages */ + uint64_t sample_dirty_count; /* cout of dirty pages we measure */ + uint32_t *hash_result; /* array of hash result for sampled pages */ +}; + void *get_dirtyrate_thread(void *arg); #endif
Add RamlockDirtyInfo to store sampled page info of each ramblock. Signed-off-by: Chuan Zheng <zhengchuan@huawei.com> --- migration/dirtyrate.h | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)