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

[Hyman Huang]

在 2023/1/11 22:38, Markus Armbruster 写道:
> 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?
No, not migration round.

When dirty ring is full and return to userspace due to the exception 
"KVM_EXIT_DIRTY_RING_FULL"。 The "dirty-limit" logic would make vcpu 
sleep some time, which is adjusted dynamically every exception, and 
which is represented by the "dirty-limit-throttle-time-per-full".

As to "dirty ring full round" (sorry about that i failed to make it 
clear), It represents one round that dirty ring of vCPU is full.
> > +#                                      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"?
It's better than what i described.

> 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.
It's ok

> > +#                                      (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?
Yes. Since we could not monitor the actual time, the value is calculated 
by the formula:
dirty ring full time = dirty ring size / dirty page rate.

> 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:
>
> [...]

--
Best regard

Hyman Huang(黄勇)