Re: [PATCH RESEND v3 09/10] migration: Export dirty-limit time info for observation

[Markus Armbruster <armbru@redhat.com>]

huangy81@chinatelecom.cn writes:

> From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
>
> Export dirty limit throttle time and estimated ring full
> time, through which we can observe if dirty limit take
> effect during live migration.

Suggest something like "Extend query-migrate to provide ..." both here
and in subject.

> 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         | 15 ++++++++++++++-
>  softmmu/dirtylimit.c        | 39 +++++++++++++++++++++++++++++++++++++++
>  5 files changed, 75 insertions(+), 1 deletion(-)
>
> diff --git a/include/sysemu/dirtylimit.h b/include/sysemu/dirtylimit.h
> index 8d2c1f3..f15e01d 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_time_per_full(void);
> +int64_t dirtylimit_ring_full_time(void);
>  #endif
> diff --git a/migration/migration.c b/migration/migration.c
> index 127d0fe..3f92389 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 */
>  
> @@ -1114,6 +1115,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_time_per_full = true;
> +        info->dirty_limit_throttle_time_per_full =
> +                            dirtylimit_throttle_time_per_full();
> +
> +        info->has_dirty_limit_ring_full_time = true;
> +        info->dirty_limit_ring_full_time = 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..c3aaba3 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_time_per_full) {
> +        monitor_printf(mon, "dirty-limit throttle time: %" PRIi64 " us\n",
> +                       info->dirty_limit_throttle_time_per_full);
> +    }
> +
> +    if (info->has_dirty_limit_ring_full_time) {
> +        monitor_printf(mon, "dirty-limit ring full time: %" PRIi64 " us\n",
> +                       info->dirty_limit_ring_full_time);
> +    }

I discuss naming below.  If we change the names, we probably want to
change the string literals here, too.

> +
>      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 6055fdc..ae7d22d 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -242,6 +242,17 @@
>  #                   Present and non-empty when migration is blocked.
>  #                   (since 6.0)
>  #
> +# @dirty-limit-throttle-time-per-full: Maximum throttle time (in microseconds) of virtual
> +#                                      CPUs each dirty ring full round, used to observe

What's a dirty "ring full round"?  Is it a migration round?  Something
else?

> +#                                      if dirty-limit take effect during live migration.

takes effect

I think "if dirty-limit takes effect" isn't quite right.  We can use
this to observe how MigrationCapability dirty-limit affects the guest.
What about "shows how MigrationCapability dirty-limit affects the
guest"?

Even though dirty-limit-throttle-time-per-full is quite long, it still
feels abbreviated.  Full what?  What about
dirty-limit-throttle-time-per-round?  Naming is hard.

> +#                                      (since 7.3)
> +#
> +# @dirty-limit-ring-full-time: Estimated average dirty ring full time (in microseconds)
> +#                              each dirty ring full round, note that the value equals
> +#                              dirty ring memory size divided by average dirty page rate
> +#                              of virtual CPU, which can be used to observe the average
> +#                              memory load of virtual CPU indirectly. (since 7.3)
> +#

Uff.

What is estimated?  The average amount of time the dirty ring (whatever
that is) is full in each migration round?

Aside: our doc comment syntax can push text blocks far to the right.
Not good.  Also not your fault, and not your problem to fix.

>  # Since: 0.14
>  ##
>  { 'struct': 'MigrationInfo',
> @@ -259,7 +270,9 @@
>             '*postcopy-blocktime' : 'uint32',
>             '*postcopy-vcpu-blocktime': ['uint32'],
>             '*compression': 'CompressionStats',
> -           '*socket-address': ['SocketAddress'] } }
> +           '*socket-address': ['SocketAddress'],
> +           '*dirty-limit-throttle-time-per-full': 'int64',
> +           '*dirty-limit-ring-full-time': 'int64'} }
>  
>  ##
>  # @query-migrate:

[...]