Re: [PATCH v2 08/11] migration: Export dirty-limit time info

[Peter Xu]

On Mon, Nov 21, 2022 at 11:26:40AM -0500, huangy81@chinatelecom.cn wrote:
> From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> 
> Export dirty limit throttle time and estimated ring full
> time, through which we can observe the process of dirty
> limit during live migration.
> 
> Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> ---
>  include/sysemu/dirtylimit.h |  2 ++
>  migration/migration.c       | 10 ++++++++++
>  monitor/hmp-cmds.c          | 10 ++++++++++
>  qapi/migration.json         | 10 +++++++++-
>  softmmu/dirtylimit.c        | 31 +++++++++++++++++++++++++++++++
>  5 files changed, 62 insertions(+), 1 deletion(-)
> 
> diff --git a/include/sysemu/dirtylimit.h b/include/sysemu/dirtylimit.h
> index 8d2c1f3..98cc4a6 100644
> --- a/include/sysemu/dirtylimit.h
> +++ b/include/sysemu/dirtylimit.h
> @@ -34,4 +34,6 @@ void dirtylimit_set_vcpu(int cpu_index,
>  void dirtylimit_set_all(uint64_t quota,
>                          bool enable);
>  void dirtylimit_vcpu_execute(CPUState *cpu);
> +int64_t dirtylimit_throttle_us_per_full(void);
> +int64_t dirtylimit_us_ring_full(void);
>  #endif
> diff --git a/migration/migration.c b/migration/migration.c
> index 096b61a..886c25d 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -62,6 +62,7 @@
>  #include "yank_functions.h"
>  #include "sysemu/qtest.h"
>  #include "sysemu/kvm.h"
> +#include "sysemu/dirtylimit.h"
>  
>  #define MAX_THROTTLE  (128 << 20)      /* Migration transfer speed 
> throttling */
>  
> @@ -1112,6 +1113,15 @@ static void populate_ram_info(MigrationInfo *info, 
> MigrationState *s)
>          info->ram->remaining = ram_bytes_remaining();
>          info->ram->dirty_pages_rate = ram_counters.dirty_pages_rate;
>      }
> +
> +    if (migrate_dirty_limit() && dirtylimit_in_service()) {
> +        info->has_dirty_limit_throttle_us_per_full = true;
> +        info->dirty_limit_throttle_us_per_full =
> +                            dirtylimit_throttle_us_per_full();
> +
> +        info->has_dirty_limit_us_ring_full = true;
> +        info->dirty_limit_us_ring_full = dirtylimit_us_ring_full();
> +    }
>  }
>  
>  static void populate_disk_info(MigrationInfo *info)
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 9ad6ee5..9d02baf 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -339,6 +339,16 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
>                         info->cpu_throttle_percentage);
>      }
>  
> +    if (info->has_dirty_limit_throttle_us_per_full) {
> +        monitor_printf(mon, "dirty-limit throttle time: %" PRIi64 " us\n",
> +                       info->dirty_limit_throttle_us_per_full);
> +    }
> +
> +    if (info->has_dirty_limit_us_ring_full) {
> +        monitor_printf(mon, "dirty-limit ring full time: %" PRIi64 " us\n",
> +                       info->dirty_limit_us_ring_full);
> +    }
> +
>      if (info->has_postcopy_blocktime) {
>          monitor_printf(mon, "postcopy blocktime: %u\n",
>                         info->postcopy_blocktime);
> diff --git a/qapi/migration.json b/qapi/migration.json
> index af6b2da..62db5cb 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -242,6 +242,12 @@
>  #                   Present and non-empty when migration is blocked.
>  #                   (since 6.0)
>  #
> +# @dirty-limit-throttle-us-per-full: Throttle time (us) during the period of
> +#                                    dirty ring full (since 7.1)
> +#
> +# @dirty-limit-us-ring-full: Estimated periodic time (us) of dirty ring full.
> +#                            (since 7.1)

s/7.1/7.3/

Could you enrich the document for the new fields?  For example, currently
you only report throttle time for vcpu0 on the 1st field, while for the
latter it's an average of all vcpus.  These need to be mentioned.

OTOH, how do you normally use these values?  Maybe that can also be added
into the documents too.

> +#
>  # Since: 0.14
>  ##
>  { 'struct': 'MigrationInfo',
> @@ -259,7 +265,9 @@
>             '*postcopy-blocktime' : 'uint32',
>             '*postcopy-vcpu-blocktime': ['uint32'],
>             '*compression': 'CompressionStats',
> -           '*socket-address': ['SocketAddress'] } }
> +           '*socket-address': ['SocketAddress'],
> +           '*dirty-limit-throttle-us-per-full': 'int64',
> +           '*dirty-limit-us-ring-full': 'int64'} }
>  
>  ##
>  # @query-migrate:
> diff --git a/softmmu/dirtylimit.c b/softmmu/dirtylimit.c
> index 3f3c405..9d1df9b 100644
> --- a/softmmu/dirtylimit.c
> +++ b/softmmu/dirtylimit.c
> @@ -573,6 +573,37 @@ static struct DirtyLimitInfo *dirtylimit_query_vcpu(int 
> cpu_index)
>      return info;
>  }
>  
> +/* Pick up first vcpu throttle time by default */
> +int64_t dirtylimit_throttle_us_per_full(void)
> +{
> +    CPUState *cpu = first_cpu;
> +    return cpu->throttle_us_per_full;

Why would vcpu0 be the standard on this sampling?

I'm wondering whether it'll make more sense to collect the MAX() of all
vcpus here, because that'll be the maximum delay for a guest write to be
postponed due to dirtylimit.  It'll provide the admin some information on
the worst impact on guest workloads.

> +}
> +
> +/*
> + * Estimate dirty ring full time under current dirty page rate.
> + * Return -1 if guest doesn't dirty memory.
> + */
> +int64_t dirtylimit_us_ring_full(void)
> +{
> +    CPUState *cpu;
> +    uint64_t curr_rate = 0;
> +    int nvcpus = 0;
> +
> +    CPU_FOREACH(cpu) {
> +        if (cpu->running) {
> +            nvcpus++;
> +            curr_rate += vcpu_dirty_rate_get(cpu->cpu_index);
> +        }
> +    }
> +
> +    if (!curr_rate || !nvcpus) {
> +        return -1;
> +    }
> +
> +    return dirtylimit_dirty_ring_full_time(curr_rate / nvcpus);
> +}
> +
>  static struct DirtyLimitInfoList *dirtylimit_query_all(void)
>  {
>      int i, index;
> -- 
> 1.8.3.1
> 
> 

-- 
Peter Xu