Re: [PATCH v2 3/3] qemu: Provide virDomainGetCPUStats() implementation for session connection

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

On Wed, Jan 18, 2023 at 10:58:19AM +0100, Michal Privoznik wrote:
> We have virDomainGetCPUStats() API which offers querying
> statistics on host CPU usage by given guest. And it works in two
> modes: getting overall stats (@start_cpu == -1, @ncpus == 1) or
> getting per host CPU usage.
>
> For the QEMU driver it is implemented by looking into values
> stored in corresponding cpuacct CGroup controller. Well, this
> works for system instances, where libvirt has permissions to
> create CGroups and place QEMU process into them. But it does not
> fly for session connection, where no CGroups are set up.
>
> Fortunately, we can do something similar to v8.8.0-rc1~95 and use
> virProcessGetStatInfo() to fill the overall stats. Unfortunately,
> I haven't found any source of per host CPU usage, so we just
> continue throwing an error in that case.
>
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
> src/qemu/qemu_driver.c | 52 ++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 50 insertions(+), 2 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 25c681bfd2..cc10dd87e2 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -16006,6 +16006,50 @@ qemuDomainGetMetadata(virDomainPtr dom,
>     return ret;
> }
>
> +#define QEMU_CPU_STATS_PROC_TOTAL 3
> +
> +static int
> +qemuDomainGetCPUStatsProc(virDomainObj *vm,
> +                          virTypedParameterPtr params,
> +                          unsigned int nparams)

The only thing I do not like about this patch is that we would now have
both:

    static int
    qemuDomainGetCPUStatsProc(virDomainObj *vm,
                              virTypedParameterPtr params,
                              unsigned int nparams)

and:

    static int
    qemuDomainGetStatsCpuProc(virDomainObj *vm,
                              virTypedParamList *params)

doing almost, but not completely, the same thing.  And there is no nice
way to avoid it.  I just haven't noticed in v1, sorry.  Or is there a
way to converge at least some of these?

> +{
> +    unsigned long long cpuTime = 0;
> +    unsigned long long userTime = 0;
> +    unsigned long long sysTime = 0;
> +
> +    if (nparams == 0) {
> +        /* return supported number of params */
> +        return QEMU_CPU_STATS_PROC_TOTAL;
> +    }
> +
> +    if (virProcessGetStatInfo(&cpuTime, &userTime, &sysTime,
> +                              NULL, NULL, vm->pid, 0) < 0) {
> +        virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> +                       _("cannot read cputime for domain"));
> +        return -1;
> +    }
> +
> +    if (virTypedParameterAssign(&params[0], VIR_DOMAIN_CPU_STATS_CPUTIME,
> +                                VIR_TYPED_PARAM_ULLONG, cpuTime) < 0)
> +        return -1;
> +
> +    if (nparams > 1 &&
> +        virTypedParameterAssign(&params[1], VIR_DOMAIN_CPU_STATS_USERTIME,
> +                                VIR_TYPED_PARAM_ULLONG, userTime) < 0)
> +        return -1;
> +
> +    if (nparams > 2 &&
> +        virTypedParameterAssign(&params[2], VIR_DOMAIN_CPU_STATS_SYSTEMTIME,
> +                                VIR_TYPED_PARAM_ULLONG, sysTime) < 0)
> +        return -1;
> +
> +    if (nparams > 3)
> +        nparams = 3;
> +
> +    return nparams;
> +}
> +
> +#undef QEMU_CPU_STATS_PROC_TOTAL
>
> static int
> qemuDomainGetCPUStats(virDomainPtr domain,
> @@ -16034,8 +16078,12 @@ qemuDomainGetCPUStats(virDomainPtr domain,
>         goto cleanup;
>
>     if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUACCT)) {
> -        virReportError(VIR_ERR_OPERATION_INVALID,
> -                       "%s", _("cgroup CPUACCT controller is not mounted"));
> +        if (start_cpu == -1) {
> +            ret = qemuDomainGetCPUStatsProc(vm, params, nparams);
> +        } else {
> +            virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> +                           _("cgroup CPUACCT controller is not mounted"));
> +        }
>         goto cleanup;
>     }
>
> --
> 2.38.2

Attachment: signature.asc
Description: PGP signature