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

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(-)

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..

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
>

在 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
>>
>