Re: [PATCH v7 6/7] migration/dirtyrate: move init step of calculation to main thread

[Hyman Huang]

在 2021/6/17 23:34, Peter Xu 写道:
> On Thu, Jun 17, 2021 at 10:12:07PM +0800, huangy81@chinatelecom.cn wrote:
> > From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> >
> > since main thread may "query dirty rate" at any time, it's better
> > to move init step into main thead so that synchronization overhead
> > between "main" and "get_dirtyrate" can be reduced.
> >
> > Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> > ---
> >   migration/dirtyrate.c | 23 +++++++++++++++++++----
> >   1 file changed, 19 insertions(+), 4 deletions(-)
> >
> > diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> > index a9bdd60..8a9dcf7 100644
> > --- a/migration/dirtyrate.c
> > +++ b/migration/dirtyrate.c
> > @@ -26,6 +26,7 @@
> >   
> >   static int CalculatingState = DIRTY_RATE_STATUS_UNSTARTED;
> >   static struct DirtyRateStat DirtyStat;
> > +static DirtyRateMeasureMode dirtyrate_mode = DIRTY_RATE_MEASURE_MODE_NONE;
> >   
> >   static int64_t set_sample_page_period(int64_t msec, int64_t initial_time)
> >   {
> > @@ -111,6 +112,11 @@ static void init_dirtyrate_stat(int64_t start_time,
> >       }
> >   }
> >   
> > +static void cleanup_dirtyrate_stat(struct DirtyRateConfig config)
> > +{
> > +    /* TODO */
> > +}
> > +
> >   static void update_dirtyrate_stat(struct RamblockDirtyInfo *info)
> >   {
> >       DirtyStat.page_sampling.total_dirty_samples += info->sample_dirty_count;
> > @@ -380,7 +386,6 @@ void *get_dirtyrate_thread(void *arg)
> >   {
> >       struct DirtyRateConfig config = *(struct DirtyRateConfig *)arg;
> >       int ret;
> > -    int64_t start_time;
> >       rcu_register_thread();
> >   
> >       ret = dirtyrate_set_state(&CalculatingState, DIRTY_RATE_STATUS_UNSTARTED,
> > @@ -390,9 +395,6 @@ void *get_dirtyrate_thread(void *arg)
> >           return NULL;
> >       }
> >   
> > -    start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME) / 1000;
> > -    init_dirtyrate_stat(start_time, config);
> > -
> >       calculate_dirtyrate(config);
> >   
> >       ret = dirtyrate_set_state(&CalculatingState, DIRTY_RATE_STATUS_MEASURING,
> > @@ -411,6 +413,7 @@ void qmp_calc_dirty_rate(int64_t calc_time, bool 
> > has_sample_pages,
> >       static struct DirtyRateConfig config;
> >       QemuThread thread;
> >       int ret;
> > +    int64_t start_time;
> >   
> >       /*
> >        * If the dirty rate is already being measured, don't attempt to start.
> > @@ -451,6 +454,18 @@ void qmp_calc_dirty_rate(int64_t calc_time, bool 
> > has_sample_pages,
> >       config.sample_period_seconds = calc_time;
> >       config.sample_pages_per_gigabytes = sample_pages;
> >       config.mode = DIRTY_RATE_MEASURE_MODE_PAGE_SAMPLING;
> > +
> > +    cleanup_dirtyrate_stat(config);
>
> This line should ideally be moved into the next patch, as sampling itself
> doesn't need it. >
> > +
> > +    /*
> > +     * update dirty rate mode so that we can figure out what mode has
> > +     * been used in last calculation
> > +     **/
> > +    dirtyrate_mode = DIRTY_RATE_MEASURE_MODE_PAGE_SAMPLING;
>
> This line is odd. Would page sampling broken if without this line?  We need to
> make sure each commit keeps the old way working..
yes, i'll drop this to make sure each commit keeps the old way working
> Thanks,
>
> > +
> > +    start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME) / 1000;
> > +    init_dirtyrate_stat(start_time, config);
> > +
> >       qemu_thread_create(&thread, "get_dirtyrate", get_dirtyrate_thread,
> >                          (void *)&config, QEMU_THREAD_DETACHED);
> >   }
> > --
> > 1.8.3.1

--
Best regard

Hyman Huang(黄勇)