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(¶ms[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,
