On Mon, Feb 15, 2021 at 14:21:45 +0300, Aleksei Zakharov wrote:
This patch adds delay time (steal time inside guest) to libvirt domain per-vcpu stats. Delay time is a consequence of the overloaded CPU. Knowledge of the delay time of a virtual machine helps to react exactly when needed and rebalance the load between hosts. This is used by cloud providers to provide quality of service, especially when the CPU is oversubscripted.
It's more convenient to work with this metric in a context of a libvirt domain. Any monitoring software may use this information.
This is a code review, I didn't have the chance to look up more on whether it makes actually sense to expose this.
Signed-off-by: Aleksei Zakharov <zaharov@selectel.ru> --- src/qemu/qemu_driver.c | 40 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 37 insertions(+), 3 deletions(-)
Also always post new versions of the patches as a new thread, don't reply to existing threads.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3d54653217..319bc60632 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1332,6 +1332,29 @@ static char *qemuConnectGetCapabilities(virConnectPtr conn) { return virCapabilitiesFormatXML(caps); }
+static int +qemuGetSchedstatDelay(unsigned long long *cpudelay, + pid_t pid, pid_t tid) +{ + g_autofree char *proc = NULL; + unsigned long long oncpu = 0; + g_autofree FILE *schedstat = NULL;
This will just g_free() the pointer [1]...
+ + if (tid) + proc = g_strdup_printf("/proc/%d/task/%d/schedstat", (int)pid, (int)tid); + else + proc = g_strdup_printf("/proc/%d/schedstat", (int)pid); + if (!proc) + return -1;
proc can't be NULL here g_strdup_printf either returns a pointer or aborts.
+ + schedstat = fopen(proc, "r"); + if (!schedstat || fscanf(schedstat, "%llu %llu", &oncpu, cpudelay) < 2) { + return -1;
Alignment is off. ...[1] so if fscanf fails the file will not be closed. The caller also expects that a libvirt error is reported on failure since it doesn't report own errors, so please make sure you do so. Additionally similarly to qemuGetSchedInfo I don't think we should make the failure to open the file fatal since it's just stats.
+ } + + VIR_FORCE_FCLOSE(schedstat); + return 0; +}
static int qemuGetSchedInfo(unsigned long long *cpuWait,
[...]
@@ -17870,7 +17899,6 @@ qemuDomainGetStatsMemoryBandwidth(virQEMUDriverPtr driver, return ret; }
-
Don't delete unrelated whitespace.
static int qemuDomainGetStatsCpuCache(virQEMUDriverPtr driver, virDomainObjPtr dom,
[...]
@@ -18104,6 +18134,10 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver, "vcpu.%u.wait", cpuinfo[i].number) < 0) goto cleanup;
+ if (virTypedParamListAddULLong(params, cpudelay[i], + "vcpu.%u.delay", cpuinfo[i].number) < 0) + goto cleanup;
Addition of a new stats field must be documented both in the public API docs and the virsh docs. Just look for any of the existing docs in: src/libvirt-domain.c and docs/manpages/virsh.rst
+ /* state below is extracted from the individual vcpu structs */ if (!(vcpu = virDomainDefGetVcpu(dom->def, cpuinfo[i].number))) continue; -- 2.17.1