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

[Hyman]

在 2022/11/30 8:09, 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.
Of course yes. I'll do that next version
> > +#
> >   # 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.
Good idea.
> > +}
> > +
> > +/*
> > + * 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