Re: [PATCH v4 6/6] migration/dirtyrate: implement dirty-ring dirtyrate calculation

[Peter Xu <peterx@redhat.com>]

On Wed, Jun 16, 2021 at 09:12:32AM +0800, huangy81@chinatelecom.cn wrote:
> From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> 
> use dirty ring feature to implement dirtyrate calculation.
> 
> introduce mode option in qmp calc_dirty_rate to specify what
> method should be used when calculating dirtyrate, either
> page-sampling or dirty-ring should be passed.
> 
> introduce "dirty_ring:-r" option in hmp calc_dirty_rate to
> indicate dirty ring method should be used for calculation.
> 
> Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>

Mostly good to me, thanks; still some more comments below.

> ---
>  hmp-commands.hx        |   7 +-
>  migration/dirtyrate.c  | 183 ++++++++++++++++++++++++++++++++++++++++++++++---
>  migration/trace-events |   2 +
>  qapi/migration.json    |  16 ++++-
>  4 files changed, 195 insertions(+), 13 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 8e45bce..f7fc9d7 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1738,8 +1738,9 @@ ERST
>  
>      {
>          .name       = "calc_dirty_rate",
> -        .args_type  = "second:l,sample_pages_per_GB:l?",
> -        .params     = "second [sample_pages_per_GB]",
> -        .help       = "start a round of guest dirty rate measurement",
> +        .args_type  = "dirty_ring:-r,second:l,sample_pages_per_GB:l?",
> +        .params     = "[-r] second [sample_pages_per_GB]",
> +        .help       = "start a round of guest dirty rate measurement (using -d to"
> +                      "\n\t\t\t specify dirty ring as the method of calculation)",
>          .cmd        = hmp_calc_dirty_rate,
>      },
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index d7b41bd..7c9515b 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -16,6 +16,7 @@
>  #include "cpu.h"
>  #include "exec/ramblock.h"
>  #include "qemu/rcu_queue.h"
> +#include "qemu/main-loop.h"
>  #include "qapi/qapi-commands-migration.h"
>  #include "ram.h"
>  #include "trace.h"
> @@ -23,11 +24,20 @@
>  #include "monitor/hmp.h"
>  #include "monitor/monitor.h"
>  #include "qapi/qmp/qdict.h"
> +#include "sysemu/kvm.h"
> +#include "sysemu/runstate.h"
> +#include "exec/memory.h"
> +
> +typedef struct DirtyPageRecord {
> +    uint64_t start_pages;
> +    uint64_t end_pages;
> +} DirtyPageRecord;
>  
>  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 DirtyPageRecord *dirty_pages;

I think this can be a local var.  See below.

>  
>  static int64_t set_sample_page_period(int64_t msec, int64_t initial_time)
>  {
> @@ -72,9 +82,11 @@ static int dirtyrate_set_state(int *state, int old_state, int new_state)
>  
>  static struct DirtyRateInfo *query_dirty_rate_info(void)
>  {
> +    int i;
>      qemu_mutex_lock(&dirtyrate_lock);
>      int64_t dirty_rate = DirtyStat.dirty_rate;
>      struct DirtyRateInfo *info = g_malloc0(sizeof(DirtyRateInfo));
> +    DirtyRateVcpuList *head = NULL, **tail = &head;
>  
>      if (qatomic_read(&CalculatingState) == DIRTY_RATE_STATUS_MEASURED) {
>          info->has_dirty_rate = true;
> @@ -85,9 +97,22 @@ static struct DirtyRateInfo *query_dirty_rate_info(void)
>      info->start_time = DirtyStat.start_time;
>      info->calc_time = DirtyStat.calc_time;
>      info->sample_pages = DirtyStat.sample_pages;
> +    info->mode = dirtyrate_mode;
> +
> +    if (dirtyrate_mode == DIRTY_RATE_MEASURE_MODE_DIRTY_RING) {
> +        /* set sample_pages with 0 to indicate page sampling isn't enabled */
> +        info->sample_pages = 0;
> +        info->has_vcpu_dirty_rate = true;
> +        for (i = 0; i < DirtyStat.dirty_ring.nvcpu; i++) {
> +            DirtyRateVcpu *rate = g_malloc0(sizeof(DirtyRateVcpu));
> +            rate->id = DirtyStat.dirty_ring.rates[i].id;
> +            rate->dirty_rate = DirtyStat.dirty_ring.rates[i].dirty_rate;
> +            QAPI_LIST_APPEND(tail, rate);
> +        }
> +        info->vcpu_dirty_rate = head;
> +    }

I think it's nicer to move this chunk into the previous block:

    if (qatomic_read(&CalculatingState) == DIRTY_RATE_STATUS_MEASURED) {
        ...
    }

Then as mentioned previously I think we can drop the mutex in previous patch.

>  
>      qemu_mutex_unlock(&dirtyrate_lock);
> -
>      trace_query_dirty_rate_info(DirtyRateStatus_str(CalculatingState));
>  
>      return info;
> @@ -119,7 +144,11 @@ static void init_dirtyrate_stat(int64_t start_time,
>  
>  static void cleanup_dirtyrate_stat(struct DirtyRateConfig config)
>  {
> -    /* TODO */
> +    /* last calc-dirty-rate qmp use dirty ring mode */
> +    if (dirtyrate_mode == DIRTY_RATE_MEASURE_MODE_DIRTY_RING) {
> +        free(DirtyStat.dirty_ring.rates);
> +        DirtyStat.dirty_ring.rates = NULL;
> +    }
>  }
>  
>  static void update_dirtyrate_stat(struct RamblockDirtyInfo *info)
> @@ -356,7 +385,97 @@ static bool compare_page_hash_info(struct RamblockDirtyInfo *info,
>      return true;
>  }
>  
> -static void calculate_dirtyrate(struct DirtyRateConfig config)
> +static void record_dirtypages(CPUState *cpu, bool start)
> +{
> +    if (start) {
> +        dirty_pages[cpu->cpu_index].start_pages = cpu->dirty_pages;
> +    } else {
> +        dirty_pages[cpu->cpu_index].end_pages = cpu->dirty_pages;
> +    }
> +}

I suggest to drop this helper and inline them.  More below.

> +
> +static void dirtyrate_global_dirty_log_start(void)
> +{
> +    qemu_mutex_lock_iothread();
> +    memory_global_dirty_log_start(GLOBAL_DIRTY_DIRTY_RATE);
> +    qemu_mutex_unlock_iothread();
> +}
> +
> +static void dirtyrate_global_dirty_log_stop(void)
> +{
> +    qemu_mutex_lock_iothread();
> +    memory_global_dirty_log_stop(GLOBAL_DIRTY_DIRTY_RATE);
> +    qemu_mutex_unlock_iothread();
> +}
> +
> +static int64_t do_calculate_dirtyrate_vcpu(int idx)
> +{
> +    uint64_t memory_size_MB;
> +    int64_t time_s;
> +    uint64_t start_pages = dirty_pages[idx].start_pages;
> +    uint64_t end_pages = dirty_pages[idx].end_pages;
> +    uint64_t dirty_pages = 0;
> +
> +    dirty_pages = end_pages - start_pages;
> +
> +    memory_size_MB = (dirty_pages * TARGET_PAGE_SIZE) >> 20;
> +    time_s = DirtyStat.calc_time;
> +
> +    trace_dirtyrate_do_calculate_vcpu(idx, dirty_pages, time_s);
> +
> +    return memory_size_MB / time_s;
> +}
> +
> +static void calculate_dirtyrate_dirty_ring(struct DirtyRateConfig config)
> +{
> +    CPUState *cpu;
> +    int64_t msec = 0;
> +    int64_t start_time;
> +    uint64_t dirtyrate = 0;
> +    uint64_t dirtyrate_sum = 0;
> +    int nvcpu = 0;
> +    int i = 0;
> +
> +    CPU_FOREACH(cpu) {
> +        nvcpu++;
> +    }
> +
> +    dirty_pages = malloc(sizeof(*dirty_pages) * nvcpu);

I think dirty_pages can be a local var in this function and should be enough.

> +
> +    DirtyStat.dirty_ring.nvcpu = nvcpu;
> +    DirtyStat.dirty_ring.rates = malloc(sizeof(DirtyRateVcpu) * nvcpu);
> +
> +    dirtyrate_global_dirty_log_start();
> +
> +    CPU_FOREACH(cpu) {
> +        record_dirtypages(cpu, true);

Here we expand it so reference dirty_pages will have no problem.

> +    }
> +
> +    start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> +    DirtyStat.start_time = start_time / 1000;
> +
> +    msec = config.sample_period_seconds * 1000;
> +    msec = set_sample_page_period(msec, start_time);
> +    DirtyStat.calc_time = msec / 1000;
> +
> +    CPU_FOREACH(cpu) {
> +        record_dirtypages(cpu, false);

Same here.

> +    }
> +
> +    dirtyrate_global_dirty_log_stop();
> +
> +    for (i = 0; i < DirtyStat.dirty_ring.nvcpu; i++) {
> +        dirtyrate = do_calculate_dirtyrate_vcpu(i);

We may need to pass in dirty_pages here too, but this should be the last thing
we do to make it local.

> +        DirtyStat.dirty_ring.rates[i].id = i;
> +        DirtyStat.dirty_ring.rates[i].dirty_rate = dirtyrate;
> +        dirtyrate_sum += dirtyrate;
> +    }
> +
> +    DirtyStat.dirty_rate = dirtyrate_sum;
> +    free(dirty_pages);
> +}
> +
> +static void calculate_dirtyrate_sample_vm(struct DirtyRateConfig config)
>  {
>      struct RamblockDirtyInfo *block_dinfo = NULL;
>      int block_count = 0;
> @@ -387,6 +506,17 @@ out:
>      free_ramblock_dirty_info(block_dinfo, block_count);
>  }
>  
> +static void calculate_dirtyrate(struct DirtyRateConfig config)
> +{
> +    if (config.mode == DIRTY_RATE_MEASURE_MODE_DIRTY_RING) {
> +        calculate_dirtyrate_dirty_ring(config);
> +    } else {
> +        calculate_dirtyrate_sample_vm(config);
> +    }
> +
> +    trace_dirtyrate_calculate(DirtyStat.dirty_rate);
> +}
> +
>  void *get_dirtyrate_thread(void *arg)
>  {
>      struct DirtyRateConfig config = *(struct DirtyRateConfig *)arg;
> @@ -412,8 +542,12 @@ void *get_dirtyrate_thread(void *arg)
>      return NULL;
>  }
>  
> -void qmp_calc_dirty_rate(int64_t calc_time, bool has_sample_pages,
> -                         int64_t sample_pages, Error **errp)
> +void qmp_calc_dirty_rate(int64_t calc_time,
> +                         bool has_sample_pages,
> +                         int64_t sample_pages,
> +                         bool has_mode,
> +                         DirtyRateMeasureMode mode,
> +                         Error **errp)
>  {
>      static struct DirtyRateConfig config;
>      QemuThread thread;
> @@ -435,6 +569,15 @@ void qmp_calc_dirty_rate(int64_t calc_time, bool has_sample_pages,
>          return;
>      }
>  
> +    if (!has_mode) {
> +        mode =  DIRTY_RATE_MEASURE_MODE_PAGE_SAMPLING;
> +    }
> +
> +    if (has_sample_pages && mode == DIRTY_RATE_MEASURE_MODE_DIRTY_RING) {
> +        error_setg(errp, "either sample-pages or dirty-ring can be specified.");
> +        return;
> +    }
> +
>      if (has_sample_pages) {
>          if (!is_sample_pages_valid(sample_pages)) {
>              error_setg(errp, "sample-pages is out of range[%d, %d].",
> @@ -447,6 +590,16 @@ void qmp_calc_dirty_rate(int64_t calc_time, bool has_sample_pages,
>      }
>  
>      /*
> +     * dirty ring mode only works when kvm dirty ring is enabled.
> +     */
> +    if ((mode == DIRTY_RATE_MEASURE_MODE_DIRTY_RING) &&
> +        !kvm_dirty_ring_enabled()) {
> +        error_setg(errp, "dirty ring is disabled, use sample-pages method "
> +                         "or remeasure later.");
> +        return;
> +    }
> +
> +    /*
>       * Init calculation state as unstarted.
>       */
>      ret = dirtyrate_set_state(&CalculatingState, CalculatingState,
> @@ -458,7 +611,7 @@ 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;
> +    config.mode = mode;
>  
>      if (unlikely(dirtyrate_mode == DIRTY_RATE_MEASURE_MODE_NONE)) {
>          /* first time to calculate dirty rate */
> @@ -471,7 +624,7 @@ void qmp_calc_dirty_rate(int64_t calc_time, bool has_sample_pages,
>       * 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;
> +    dirtyrate_mode = mode;
>  
>      start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME) / 1000;
>      init_dirtyrate_stat(start_time, config);
> @@ -497,9 +650,18 @@ void hmp_info_dirty_rate(Monitor *mon, const QDict *qdict)
>                     info->sample_pages);
>      monitor_printf(mon, "Period: %"PRIi64" (sec)\n",
>                     info->calc_time);
> +    monitor_printf(mon, "Mode: %s\n",
> +                   DirtyRateMeasureMode_str(info->mode));
>      monitor_printf(mon, "Dirty rate: ");
>      if (info->has_dirty_rate) {
>          monitor_printf(mon, "%"PRIi64" (MB/s)\n", info->dirty_rate);
> +        if (info->has_vcpu_dirty_rate) {
> +            DirtyRateVcpuList *rate, *head = info->vcpu_dirty_rate;
> +            for (rate = head; rate != NULL; rate = rate->next) {
> +                monitor_printf(mon, "vcpu[%"PRIi64"], Dirty rate: %"PRIi64"\n",
> +                               rate->value->id, rate->value->dirty_rate);
> +            }
> +        }
>      } else {
>          monitor_printf(mon, "(not ready)\n");
>      }

Please be careful to not leak the list of vcpu results.. I think we need
something like qapi_free_DirtyRateVcpuList().

Thanks,

-- 
Peter Xu