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@xxxxxxxxxxx>
> ---
> 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
>
