<div dir="ltr">Peter, thanks a lot for the review! I'll come back with fixes. <div> пт, 29 янв. 2021 г. в 18:25, Peter Krempa <<a href="mailto:pkrempa@redhat.com">pkrempa@redhat.com</a>>: > > 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 <<a href="mailto:zaharov@selectel.ru">zaharov@selectel.ru</a>> > > --- > > 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, > </div></div>