Re: [PATCH] qemu: add delay time cpu stats

[Aleksei Zakharov <zaharov@xxxxxxxxxxx>]

Peter, thanks a lot for the review! I'll come back with fixes.

пт, 29 янв. 2021 г. в 18:25, Peter Krempa <pkrempa@xxxxxxxxxx>:
>
> On Fri, Jan 29, 2021 at 17:34:02 +0300, Aleksei Zakharov wrote:
> > This commit adds delay time (steal time inside guest) to libvirt
> > domain CPU stats. It's more convenient to work with this statistic
> > in a context of libvirt domain. Any monitoring software may use
> > this information.
>
> Please primarily describe what the value is used for.
>
> >
> > As an example: the next step is to add support to
> > libvirt-go and expose metrics with libvirt-exporter.
>
> This doesn't belong to a commit message.
>
> >
> > Signed-off-by: Aleksei Zakharov <zaharov@xxxxxxxxxxx>
> > ---
> >  include/libvirt/libvirt-domain.h |  6 ++++
> >  src/qemu/qemu_driver.c           | 56 ++++++++++++++++++++++++++++++++
> >  tools/virsh-domain.c             |  3 +-
> >  3 files changed, 64 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> > index de2456812c..b3f9f375a5 100644
> > --- a/include/libvirt/libvirt-domain.h
> > +++ b/include/libvirt/libvirt-domain.h
> > @@ -1350,6 +1350,12 @@ int                     virDomainGetState       (virDomainPtr domain,
> >   */
> >  # define VIR_DOMAIN_CPU_STATS_SYSTEMTIME "system_time"
> >
> > +/**
> > + * VIR_DOMAIN_CPU_STATS_DELAYTIME:
> > + * cpu time waiting on runqueue in nanoseconds, as a ullong
> > + */
> > +# define VIR_DOMAIN_CPU_STATS_DELAYTIME "delay_time"
> > +
> >  /**
> >   * VIR_DOMAIN_CPU_STATS_VCPUTIME:
> >   * vcpu usage in nanoseconds (cpu_time excluding hypervisor time),
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 6193376544..41839a0239 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -16674,6 +16674,36 @@ qemuDomainGetMetadata(virDomainPtr dom,
> >      return ret;
> >  }
> >
> > +static int
> > +virSchedstatGetDelay(virDomainObjPtr dom, unsigned long long *delay)
> > +{
> > +    char *proc = NULL;
> > +    FILE* schedstat;
> > +    unsigned long long curr_delay, _oncpu_ = 0;
> > +    pid_t pid = dom->pid;
> > +    for (size_t i = 0; i < virDomainDefGetVcpusMax(dom->def); i++) {
>
> Wouldn't it be worth co collect the stats per-vcpu?
>
> Also we don't allow variable declaration inside loops.
>
> Additionally virDomainDefGetVcpusMax returns the total number of cpus,
> so you'll attempt to gather stats for offline vcpus too, which will fail
> because qemu doesn't create cpu threads for them.
>
> > +        pid_t vcpupid = qemuDomainGetVcpuPid(dom, i);
> > +        if (vcpupid) {
> > +            if (asprintf(&proc, "/proc/%d/task/%d/schedstat",
> > +                                    pid, vcpupid) < 0)
>
> Note that we use the glib versions of formatters thus g_strdup_printf
> here.
>
> > +                return -1;
> > +        } else {
> > +            if (asprintf(&proc, "/proc/%d/schedstat", pid) < 0)
> > +                return -1;
> > +        }
> > +        schedstat = fopen(proc, "r");
> > +        VIR_FREE(proc);
> > +        if (!schedstat ||
> > +            fscanf(schedstat, "%llu %llu",
> > +                &oncpu, &curr_delay) < 2) {
>
> The alignment here is off and doesn't conform to our coding style
>
> > +                return -1;
> > +        }
> > +
> > +        *delay += curr_delay;
> > +    }
> > +    return 0;
> > +}
> > +
> >
> >  static int
> >  qemuDomainGetCPUStats(virDomainPtr domain,
> > @@ -16687,6 +16717,7 @@ qemuDomainGetCPUStats(virDomainPtr domain,
> >      int ret = -1;
> >      qemuDomainObjPrivatePtr priv;
> >      virBitmapPtr guestvcpus = NULL;
> > +    unsigned long long delay = 0;
> >
> >      virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1);
> >
> > @@ -16712,8 +16743,19 @@ qemuDomainGetCPUStats(virDomainPtr domain,
> >          goto cleanup;
> >
> >      if (start_cpu == -1)
> > +    {
>
> This doesn't conform to our coding style
>
> >          ret = virCgroupGetDomainTotalCpuStats(priv->cgroup,
> >                                                params, nparams);
> > +        if (nparams > 3) {
> > +            if (virSchedstatGetDelay(vm,&delay) < 0)
> > +                return -1;
> > +            if (virTypedParameterAssign(&params[3],
> > +                            VIR_DOMAIN_CPU_STATS_DELAYTIME,
> > +                            VIR_TYPED_PARAM_ULLONG, delay) < 0)
> > +             return -1;
>
> The alignment is totally off here.
>
> > +        }
> > +        ret++;
> > +    }
>
> Many of those problems above actually trip our test suite.
>
> Our contributor guidelines specifically ask contributors to run the test
> suite before posting patches. Please fix all of the problems and
> re-send.
>
>
> >      else
> >          ret = virCgroupGetPercpuStats(priv->cgroup, params, nparams,
> >                                        start_cpu, ncpus, guestvcpus);
> > @@ -17845,6 +17887,17 @@ qemuDomainGetStatsMemoryBandwidth(virQEMUDriverPtr driver,
> >      return ret;
> >  }
> >
> > +static int
> > +qemuDomainGetStatsCpuDelay(virDomainObjPtr dom,
> > +                           virTypedParamListPtr params)
> > +{
> > +    unsigned long long delay_time = 0;
> > +    int err = 0;
> > +    err = virSchedstatGetDelay(dom, &delay_time);
>
> You can return here directly without the need for 'err' variable.
>
> > +    if (!err && virTypedParamListAddULLong(params, delay_time, "cpu.delay") < 0)
> > +        return -1;
> > +    return 0;
> > +}
> >
> >  static int
> >  qemuDomainGetStatsCpuCache(virQEMUDriverPtr driver,
>