[PATCH v6 5/5] migration/dirtyrate: Introduce command 'virsh domstats --dirtyrate'

Michal Privoznik mprivozn at redhat.com
Wed Mar 3 11:32:03 UTC 2021


On 2/26/21 9:35 AM, Hao Wang wrote:
> Introduce command 'virsh domstats --dirtyrate' for reporting memory
> dirty rate infomation. The info is listed as:
> 
> Domain: 'vm0'
>    dirtyrate.calc_status=measured
>    dirtyrate.calc_start_time=502814
>    dirtyrate.calc_period=1
>    dirtyrate.megabytes_per_second=2
> 
> Signed-off-by: Hao Wang <wanghao232 at huawei.com>
> ---
>   docs/manpages/virsh.rst          | 16 +++++++--
>   include/libvirt/libvirt-domain.h |  1 +
>   src/libvirt-domain.c             | 13 ++++++++
>   src/qemu/qemu_driver.c           | 56 ++++++++++++++++++++++++++++++++
>   tools/virsh-domain-monitor.c     |  7 ++++
>   5 files changed, 91 insertions(+), 2 deletions(-)

There are two things happening in this patch. You're extending virsh and 
at the same time introducing new domain statistics. I think they should 
go separate (it'd be easier to backport for instance).

> 
> diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
> index 417ea444f4..b0fab3e781 100644
> --- a/docs/manpages/virsh.rst
> +++ b/docs/manpages/virsh.rst
> @@ -2219,7 +2219,7 @@ domstats
>   
>      domstats [--raw] [--enforce] [--backing] [--nowait] [--state]
>         [--cpu-total] [--balloon] [--vcpu] [--interface]
> -      [--block] [--perf] [--iothread] [--memory]
> +      [--block] [--perf] [--iothread] [--memory] [--dirtyrate]
>         [[--list-active] [--list-inactive]
>          [--list-persistent] [--list-transient] [--list-running]y
>          [--list-paused] [--list-shutoff] [--list-other]] | [domain ...]
> @@ -2238,7 +2238,8 @@ behavior use the *--raw* flag.
>   The individual statistics groups are selectable via specific flags. By
>   default all supported statistics groups are returned. Supported
>   statistics groups flags are: *--state*, *--cpu-total*, *--balloon*,
> -*--vcpu*, *--interface*, *--block*, *--perf*, *--iothread*, *--memory*.
> +*--vcpu*, *--interface*, *--block*, *--perf*, *--iothread*, *--memory*,
> +*--dirtyrate*.
>   
>   Note that - depending on the hypervisor type and version or the domain state
>   - not all of the following statistics may be returned.
> @@ -2431,6 +2432,17 @@ not available for statistical purposes.
>     bytes consumed by @vcpus that passing through all memory controllers, either
>     local or remote controller.
>   
> +*--dirtyrate* returns:
> +
> +* ``dirtyrate.calc_status`` - the status of last memory dirty rate
> +  calculation
> +* ``dirtyrate.calc_start_time`` - the start time of last memory dirty
> +  rate calculation
> +* ``dirtyrate.calc_period`` - the period of last memory dirty rate
> +  calculation
> +* ``dirtyrate.megabytes_per_second`` - the calculated memory dirty
> +  rate in MiB/s
> +
>   
>   Selecting a specific statistics groups doesn't guarantee that the
>   daemon supports the selected group of stats. Flag *--enforce*
> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> index 7aa5ef4543..81371f54b7 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -2185,6 +2185,7 @@ typedef enum {
>       VIR_DOMAIN_STATS_PERF = (1 << 6), /* return domain perf event info */
>       VIR_DOMAIN_STATS_IOTHREAD = (1 << 7), /* return iothread poll info */
>       VIR_DOMAIN_STATS_MEMORY = (1 << 8), /* return domain memory info */
> +    VIR_DOMAIN_STATS_DIRTYRATE = (1 << 9), /* return domain dirty rate info */
>   } virDomainStatsTypes;

I think this hunk doesn't belong here.

>   
>   typedef enum {
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index b1cc2ebbf3..0a03b2ca90 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -11883,6 +11883,19 @@ virConnectGetDomainCapabilities(virConnectPtr conn,
>    *                       bytes consumed by @vcpus that passing through all
>    *                       memory controllers, either local or remote controller.
>    *
> + * VIR_DOMAIN_STATS_DIRTYRATE:
> + *     Return memory dirty rate information. The typed parameter keys are in
> + *     this format:
> + *
> + *     "dirtyrate.calc_status" - the status of last memory dirty rate
> + *                               calculation

I'm not sure we want to expose this as string. At the same time, we 
don't have any enum defined, yet. What do others think?

> + *     "dirtyrate.calc_start_time" - the start time of last memory dirty
> + *                                   rate calculation
> + *     "dirtyrate.calc_period" - the period of last memory dirty rate
> + *                               calculation
> + *     "dirtyrate.megabytes_per_second" - the calculated memory dirty
> + *                                        rate in MiB/s

Notice how the other descriptions have "as unsigned int" or "as long 
long"? That is so that a developer reading this description knows which 
one of virTypedParamsGet*() family to call. For instance, if I'd be 
interested in calc_start_time, then I can do:

long long dirtyrate;

virDomainListGetStats(domlist, VIR_DOMAIN_STATS_DIRTYRATE, &records, 0);

virTypedParamsGetLLong(records->params, records->nparams, 
"dirtyrate.megabytes_per_second", &dirtyrate);

> + *
>    * Note that entire stats groups or individual stat fields may be missing from
>    * the output in case they are not supported by the given hypervisor, are not
>    * applicable for the current state of the guest domain, or their retrieval
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c

I think this should be in the other patch too.

> index 7420937cf7..f847650e22 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -18535,6 +18535,61 @@ qemuDomainGetStatsPerf(virQEMUDriverPtr driver G_GNUC_UNUSED,
>       return 0;
>   }
>   
> +static int
> +qemuDomainGetStatsDirtyRateMon(virQEMUDriverPtr driver,
> +                               virDomainObjPtr vm,
> +                               qemuMonitorDirtyRateInfoPtr info)
> +{
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    int ret;
> +
> +    qemuDomainObjEnterMonitor(driver, vm);
> +    ret = qemuMonitorQueryDirtyRate(priv->mon, info);
> +    if (qemuDomainObjExitMonitor(driver, vm) < 0)
> +        ret = -1;
> +
> +    return ret;
> +}
> +
> +static int
> +qemuDomainGetStatsDirtyRate(virQEMUDriverPtr driver,
> +                           virDomainObjPtr dom,
> +                           virTypedParamListPtr params,
> +                           unsigned int privflags)
> +{
> +    qemuMonitorDirtyRateInfo info;
> +    int ret = -1;
> +
> +    if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom))
> +        return 0;
> +
> +    if (qemuDomainGetStatsDirtyRateMon(driver, dom, &info) < 0)
> +        goto cleanup;
> +
> +    if (virTypedParamListAddString(params, info.status,
> +                                   "dirtyrate.calc_status") < 0)
> +        goto cleanup;
> +
> +    if (virTypedParamListAddLLong(params, info.startTime,
> +                                  "dirtyrate.calc_start_time") < 0)
> +        goto cleanup;
> +
> +    if (virTypedParamListAddInt(params, info.calcTime,
> +                                "dirtyrate.calc_period") < 0)
> +        goto cleanup;
> +
> +    if (STREQ(info.status, "measured") &&
> +        virTypedParamListAddLLong(params, info.dirtyRate,
> +                                  "dirtyrate.megabytes_per_second") < 0)
> +        goto cleanup;
> +
> +    ret = 0;
> +
> + cleanup:
> +    VIR_FREE(info.status);
> +    return ret;
> +}
> +
>   typedef int
>   (*qemuDomainGetStatsFunc)(virQEMUDriverPtr driver,
>                             virDomainObjPtr dom,
> @@ -18557,6 +18612,7 @@ static struct qemuDomainGetStatsWorker qemuDomainGetStatsWorkers[] = {
>       { qemuDomainGetStatsPerf, VIR_DOMAIN_STATS_PERF, false },
>       { qemuDomainGetStatsIOThread, VIR_DOMAIN_STATS_IOTHREAD, true },
>       { qemuDomainGetStatsMemory, VIR_DOMAIN_STATS_MEMORY, false },
> +    { qemuDomainGetStatsDirtyRate, VIR_DOMAIN_STATS_DIRTYRATE, true },
>       { NULL, 0, false }
>   };
>   

Theset two hunks should be in the other patch.

> diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
> index 897339b6f9..c4d7464695 100644
> --- a/tools/virsh-domain-monitor.c
> +++ b/tools/virsh-domain-monitor.c
> @@ -2148,6 +2148,10 @@ static const vshCmdOptDef opts_domstats[] = {
>        .type = VSH_OT_BOOL,
>        .help = N_("report domain memory usage"),
>       },
> +    {.name = "dirtyrate",
> +     .type = VSH_OT_BOOL,
> +     .help = N_("report domain dirty rate information"),
> +    },
>       {.name = "list-active",
>        .type = VSH_OT_BOOL,
>        .help = N_("list only active domains"),
> @@ -2267,6 +2271,9 @@ cmdDomstats(vshControl *ctl, const vshCmd *cmd)
>       if (vshCommandOptBool(cmd, "memory"))
>           stats |= VIR_DOMAIN_STATS_MEMORY;
>   
> +    if (vshCommandOptBool(cmd, "dirtyrate"))
> +        stats |= VIR_DOMAIN_STATS_DIRTYRATE;
> +
>       if (vshCommandOptBool(cmd, "list-active"))
>           flags |= VIR_CONNECT_GET_ALL_DOMAINS_STATS_ACTIVE;
>   
> 

Michal




More information about the libvir-list mailing list