include/libvirt/libvirt-domain.h | 6 ++++ src/qemu/qemu_driver.c | 56 ++++++++++++++++++++++++++++++++ tools/virsh-domain.c | 3 +- 3 files changed, 64 insertions(+), 1 deletion(-)
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.
As an example: the next step is to add support to
libvirt-go and expose metrics with libvirt-exporter.
Signed-off-by: Aleksei Zakharov <zaharov@selectel.ru>
---
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++) {
+ pid_t vcpupid = qemuDomainGetVcpuPid(dom, i);
+ if (vcpupid) {
+ if (asprintf(&proc, "/proc/%d/task/%d/schedstat",
+ pid, vcpupid) < 0)
+ 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) {
+ 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)
+ {
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;
+ }
+ ret++;
+ }
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);
+ if (!err && virTypedParamListAddULLong(params, delay_time, "cpu.delay") < 0)
+ return -1;
+ return 0;
+}
static int
qemuDomainGetStatsCpuCache(virQEMUDriverPtr driver,
@@ -17950,6 +18003,9 @@ qemuDomainGetStatsCpu(virQEMUDriverPtr driver,
if (qemuDomainGetStatsCpuCache(driver, dom, params) < 0)
return -1;
+ if (qemuDomainGetStatsCpuDelay(dom, params) < 0)
+ return -1;
+
return 0;
}
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 2bb136333f..a1780da1a5 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -8085,7 +8085,8 @@ vshCPUStatsPrintField(vshControl *ctl,
if ((STREQ(param->field, VIR_DOMAIN_CPU_STATS_CPUTIME) ||
STREQ(param->field, VIR_DOMAIN_CPU_STATS_VCPUTIME) ||
STREQ(param->field, VIR_DOMAIN_CPU_STATS_USERTIME) ||
- STREQ(param->field, VIR_DOMAIN_CPU_STATS_SYSTEMTIME)) &&
+ STREQ(param->field, VIR_DOMAIN_CPU_STATS_SYSTEMTIME) ||
+ STREQ(param->field, VIR_DOMAIN_CPU_STATS_DELAYTIME)) &&
param->type == VIR_TYPED_PARAM_ULLONG) {
vshPrint(ctl, "%9lld.%09lld seconds\n",
param->value.ul / 1000000000,
--
2.17.1
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@selectel.ru>
> ---
> 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,
The previous patch introduced summarized delay time of the vm. It's worth to collect per-vcpu, so I dropped that code and added per-vcpu stats. Also, fixed coding style errors and used glib functions. Aleksei Zakharov (1): qemu: add per-vcpu delay stats src/qemu/qemu_driver.c | 39 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 36 insertions(+), 3 deletions(-) -- 2.17.1
Fixed an issue in v2: Close schedstat file after use. Fixes since v1: Collect per-vcpu stats. Coding style errors Use glib functions. Aleksei Zakharov (1): qemu: add per-vcpu delay stats src/qemu/qemu_driver.c | 40 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 37 insertions(+), 3 deletions(-) -- 2.17.1
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.
Signed-off-by: Aleksei Zakharov <zaharov@selectel.ru>
---
src/qemu/qemu_driver.c | 40 +++++++++++++++++++++++++++++++++++++---
1 file changed, 37 insertions(+), 3 deletions(-)
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;
+
+ 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;
+
+ schedstat = fopen(proc, "r");
+ if (!schedstat || fscanf(schedstat, "%llu %llu", &oncpu, cpudelay) < 2) {
+ return -1;
+ }
+
+ VIR_FORCE_FCLOSE(schedstat);
+ return 0;
+}
static int
qemuGetSchedInfo(unsigned long long *cpuWait,
@@ -1470,6 +1493,7 @@ static int
qemuDomainHelperGetVcpus(virDomainObjPtr vm,
virVcpuInfoPtr info,
unsigned long long *cpuwait,
+ unsigned long long *cpudelay,
int maxinfo,
unsigned char *cpumaps,
int maplen)
@@ -1529,6 +1553,11 @@ qemuDomainHelperGetVcpus(virDomainObjPtr vm,
return -1;
}
+ if (cpudelay) {
+ if (qemuGetSchedstatDelay(&(cpudelay[ncpuinfo]), vm->pid, vcpupid) < 0)
+ return -1;
+ }
+
ncpuinfo++;
}
@@ -4873,7 +4902,7 @@ qemuDomainGetVcpus(virDomainPtr dom,
goto cleanup;
}
- ret = qemuDomainHelperGetVcpus(vm, info, NULL, maxinfo, cpumaps, maplen);
+ ret = qemuDomainHelperGetVcpus(vm, info, NULL, NULL, maxinfo, cpumaps, maplen);
cleanup:
virDomainObjEndAPI(&vm);
@@ -17870,7 +17899,6 @@ qemuDomainGetStatsMemoryBandwidth(virQEMUDriverPtr driver,
return ret;
}
-
static int
qemuDomainGetStatsCpuCache(virQEMUDriverPtr driver,
virDomainObjPtr dom,
@@ -18060,6 +18088,7 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver,
int ret = -1;
virVcpuInfoPtr cpuinfo = NULL;
g_autofree unsigned long long *cpuwait = NULL;
+ g_autofree unsigned long long *cpudelay = NULL;
if (virTypedParamListAddUInt(params, virDomainDefGetVcpus(dom->def),
"vcpu.current") < 0)
@@ -18071,6 +18100,7 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver,
cpuinfo = g_new0(virVcpuInfo, virDomainDefGetVcpus(dom->def));
cpuwait = g_new0(unsigned long long, virDomainDefGetVcpus(dom->def));
+ cpudelay = g_new0(unsigned long long, virDomainDefGetVcpus(dom->def));
if (HAVE_JOB(privflags) && virDomainObjIsActive(dom) &&
qemuDomainRefreshVcpuHalted(driver, dom, QEMU_ASYNC_JOB_NONE) < 0) {
@@ -18079,7 +18109,7 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver,
virResetLastError();
}
- if (qemuDomainHelperGetVcpus(dom, cpuinfo, cpuwait,
+ if (qemuDomainHelperGetVcpus(dom, cpuinfo, cpuwait, cpudelay,
virDomainDefGetVcpus(dom->def),
NULL, 0) < 0) {
virResetLastError();
@@ -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;
+
/* state below is extracted from the individual vcpu structs */
if (!(vcpu = virDomainDefGetVcpu(dom->def, cpuinfo[i].number)))
continue;
--
2.17.1
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
>
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.
Signed-off-by: Aleksei Zakharov <zaharov@selectel.ru>
---
src/qemu/qemu_driver.c | 39 ++++++++++++++++++++++++++++++++++++---
1 file changed, 36 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 69fcd28666..f5f86557e0 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1332,6 +1332,28 @@ 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;
+
+ 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;
+
+ schedstat = fopen(proc, "r");
+ if (!schedstat || fscanf(schedstat, "%llu %llu", &oncpu, cpudelay) < 2) {
+ return -1;
+ }
+
+ return 0;
+}
static int
qemuGetSchedInfo(unsigned long long *cpuWait,
@@ -1470,6 +1492,7 @@ static int
qemuDomainHelperGetVcpus(virDomainObjPtr vm,
virVcpuInfoPtr info,
unsigned long long *cpuwait,
+ unsigned long long *cpudelay,
int maxinfo,
unsigned char *cpumaps,
int maplen)
@@ -1529,6 +1552,11 @@ qemuDomainHelperGetVcpus(virDomainObjPtr vm,
return -1;
}
+ if (cpudelay) {
+ if (qemuGetSchedstatDelay(&(cpudelay[ncpuinfo]), vm->pid, vcpupid) < 0)
+ return -1;
+ }
+
ncpuinfo++;
}
@@ -4873,7 +4901,7 @@ qemuDomainGetVcpus(virDomainPtr dom,
goto cleanup;
}
- ret = qemuDomainHelperGetVcpus(vm, info, NULL, maxinfo, cpumaps, maplen);
+ ret = qemuDomainHelperGetVcpus(vm, info, NULL, NULL, maxinfo, cpumaps, maplen);
cleanup:
virDomainObjEndAPI(&vm);
@@ -17868,7 +17896,6 @@ qemuDomainGetStatsMemoryBandwidth(virQEMUDriverPtr driver,
return ret;
}
-
static int
qemuDomainGetStatsCpuCache(virQEMUDriverPtr driver,
virDomainObjPtr dom,
@@ -18058,6 +18085,7 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver,
int ret = -1;
virVcpuInfoPtr cpuinfo = NULL;
g_autofree unsigned long long *cpuwait = NULL;
+ g_autofree unsigned long long *cpudelay = NULL;
if (virTypedParamListAddUInt(params, virDomainDefGetVcpus(dom->def),
"vcpu.current") < 0)
@@ -18069,6 +18097,7 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver,
cpuinfo = g_new0(virVcpuInfo, virDomainDefGetVcpus(dom->def));
cpuwait = g_new0(unsigned long long, virDomainDefGetVcpus(dom->def));
+ cpudelay = g_new0(unsigned long long, virDomainDefGetVcpus(dom->def));
if (HAVE_JOB(privflags) && virDomainObjIsActive(dom) &&
qemuDomainRefreshVcpuHalted(driver, dom, QEMU_ASYNC_JOB_NONE) < 0) {
@@ -18077,7 +18106,7 @@ qemuDomainGetStatsVcpu(virQEMUDriverPtr driver,
virResetLastError();
}
- if (qemuDomainHelperGetVcpus(dom, cpuinfo, cpuwait,
+ if (qemuDomainHelperGetVcpus(dom, cpuinfo, cpuwait, cpudelay,
virDomainDefGetVcpus(dom->def),
NULL, 0) < 0) {
virResetLastError();
@@ -18102,6 +18131,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;
+
/* state below is extracted from the individual vcpu structs */
if (!(vcpu = virDomainDefGetVcpu(dom->def, cpuinfo[i].number)))
continue;
--
2.17.1
Peter, thanks a lot for the review! I'll come back with fixes.
пт, 29 янв. 2021 г. в 18:25, Peter Krempa <pkrempa@redhat.com>:
>
> 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@selectel.ru>
> > ---
> > 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,
>
© 2016 - 2024 Red Hat, Inc.