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

[Peter Xu <peterx@redhat.com>]

On Wed, Jun 16, 2021 at 09:12:31AM +0800, huangy81@chinatelecom.cn wrote:
> From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> 
> since main thread could "query dirty rate" at any time, then it's
> better to move init step into main thead so that synchronization
> overhead of dirty stat can be reduced.
> 
> since not sure whether "only one QMP iothread" will still keep true
> forever, always introduce a mutex and protect dirty stat.

Sorry to have misguided you on that "only one QMP iothread" statement - that's
partly a joke.. I still think it's possible but let's not worry too much on
that now. :)

What I really wanted to suggest is moving the init data phase into main thread
(which you did in this patch, thanks!), then it's very safe already even
without mutex, afaict.. so we never do partial read DirtyStat anymore, which is
already a "safe race" since it doesn't crash anything anyways.

Btw, I think the mutex will lose it's most usefulness too if you don't take it
in the dirty rate thread (which I think is missing in this patch).  But before
looking into that, please see below..

> 
> Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> ---
>  migration/dirtyrate.c | 34 ++++++++++++++++++++++++++++++----
>  1 file changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index b97f6a5..d7b41bd 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -26,6 +26,8 @@
>  
>  static int CalculatingState = DIRTY_RATE_STATUS_UNSTARTED;
>  static struct DirtyRateStat DirtyStat;
> +static QemuMutex dirtyrate_lock;
> +static DirtyRateMeasureMode dirtyrate_mode = DIRTY_RATE_MEASURE_MODE_NONE;
>  
>  static int64_t set_sample_page_period(int64_t msec, int64_t initial_time)
>  {
> @@ -70,6 +72,7 @@ static int dirtyrate_set_state(int *state, int old_state, int new_state)
>  
>  static struct DirtyRateInfo *query_dirty_rate_info(void)
>  {
> +    qemu_mutex_lock(&dirtyrate_lock);
>      int64_t dirty_rate = DirtyStat.dirty_rate;
>      struct DirtyRateInfo *info = g_malloc0(sizeof(DirtyRateInfo));
>  
> @@ -83,6 +86,8 @@ static struct DirtyRateInfo *query_dirty_rate_info(void)
>      info->calc_time = DirtyStat.calc_time;
>      info->sample_pages = DirtyStat.sample_pages;
>  
> +    qemu_mutex_unlock(&dirtyrate_lock);
> +
>      trace_query_dirty_rate_info(DirtyRateStatus_str(CalculatingState));
>  
>      return info;
> @@ -91,6 +96,7 @@ static struct DirtyRateInfo *query_dirty_rate_info(void)
>  static void init_dirtyrate_stat(int64_t start_time,
>                                  struct DirtyRateConfig config)
>  {
> +    qemu_mutex_lock(&dirtyrate_lock);
>      DirtyStat.dirty_rate = -1;
>      DirtyStat.start_time = start_time;
>      DirtyStat.calc_time = config.sample_period_seconds;
> @@ -108,6 +114,12 @@ static void init_dirtyrate_stat(int64_t start_time,
>      default:
>          break;
>      }
> +    qemu_mutex_unlock(&dirtyrate_lock);
> +}
> +
> +static void cleanup_dirtyrate_stat(struct DirtyRateConfig config)
> +{
> +    /* TODO */
>  }
>  
>  static void update_dirtyrate_stat(struct RamblockDirtyInfo *info)
> @@ -379,7 +391,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,
> @@ -389,9 +400,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,
> @@ -410,6 +418,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.
> @@ -450,6 +459,23 @@ 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;
> +
> +    if (unlikely(dirtyrate_mode == DIRTY_RATE_MEASURE_MODE_NONE)) {
> +        /* first time to calculate dirty rate */
> +        qemu_mutex_init(&dirtyrate_lock);
> +    }

Is the 'none' mode only for init the mutex?  If so, I'd suggest we drop the
"none" mode.  A side note is that if you want to init a mutex, AFAIU the best
way is define this:

static void __attribute__((__constructor__)) dirty_rate_init(void)
{
        qemu_mutex_init(...);
}

But hold on..

I see the mutex seems to already have brought even more trouble than benefits,
maybe let's drop the mutex too along with "none" mode?  Let's keep this patch
"moving init to main thread" only, and IMHO it's good enough.

There's a special care we need to look for with dirty ring measurements, that
we need to make sure to not reference the *vcpu pointer unless the state is
DIRTY_RATE_STATUS_MEASURED.  I'll comment in the next patch for that soon.

> +
> +    cleanup_dirtyrate_stat(config);
> +
> +    /*
> +     * 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;
> +
> +    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
> 

-- 
Peter Xu