Re: [PATCH v3 1/1] qemu: add per-vcpu delay stats
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(a)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