Related: https://gitlab.com/libvirt/libvirt/-/issues/276
This patch uses qemuMonitorQueryStats to query "halt_poll_success_ns"
and "halt_poll_fail_ns" for every vCPU. The respective values for each
vCPU are then added together.
Signed-off-by: Amneesh Singh <natto@weirdnatto.in>
---
src/qemu/qemu_driver.c | 70 +++++++++++++++++++++++++++++++++++++-----
1 file changed, 62 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 97c6ed95..30170d5c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18052,15 +18052,69 @@ qemuDomainGetStatsCpuCgroup(virDomainObj *dom,
}
static int
-qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom,
- virTypedParamList *params)
+qemuDomainGetStatsCpuHaltPollTime(virQEMUDriver *driver,
+ virDomainObj *dom,
+ virTypedParamList *params,
+ unsigned int privflags)
{
unsigned long long haltPollSuccess = 0;
unsigned long long haltPollFail = 0;
- pid_t pid = dom->pid;
+ qemuDomainObjPrivate *priv = dom->privateData;
+ bool queryStatsCap = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_STATS);
- if (virHostCPUGetHaltPollTime(pid, &haltPollSuccess, &haltPollFail) < 0)
- return 0;
+ if (queryStatsCap && HAVE_JOB(privflags) && virDomainObjIsActive(dom) && driver->privileged) {
+ size_t i;
+ qemuMonitorQueryStatsTargetType target = QEMU_MONITOR_QUERY_STATS_TARGET_VCPU;
+ qemuMonitorQueryStatsProvider *provider = NULL;
+ g_autoptr(GPtrArray) providers = NULL;
+ g_autoptr(GPtrArray) queried_stats = NULL;
+ provider = qemuMonitorQueryStatsProviderNew(
+ target,
+ QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM,
+ QEMU_MONITOR_QUERY_STATS_VCPU_NAME_HALT_POLL_SUCCESS_NS,
+ QEMU_MONITOR_QUERY_STATS_VCPU_NAME_HALT_POLL_FAIL_NS,
+ -1);
+
+ if (!provider)
+ return 0;
+
+ providers = g_ptr_array_new_full(1, (GDestroyNotify) qemuMonitorQueryStatsProviderFree);
+ g_ptr_array_add(providers, provider);
+
+ qemuDomainObjEnterMonitor(driver, dom);
+ queried_stats = qemuMonitorQueryStats(priv->mon, target, NULL, providers);
+ qemuDomainObjExitMonitor(dom);
+
+ if (!queried_stats)
+ return 0;
+
+ for (i = 0; i < queried_stats->len; i++) {
+ unsigned long long curHaltPollSuccess, curHaltPollFail;
+ GHashTable *cur_table = queried_stats->pdata[i];
+ virJSONValue *success_obj, *fail_obj;
+ const char *success_str = qemuMonitorQueryStatsVcpuNameTypeToString(
+ QEMU_MONITOR_QUERY_STATS_VCPU_NAME_HALT_POLL_SUCCESS_NS);
+ const char *fail_str = qemuMonitorQueryStatsVcpuNameTypeToString(
+ QEMU_MONITOR_QUERY_STATS_VCPU_NAME_HALT_POLL_FAIL_NS);
+
+ success_obj = g_hash_table_lookup(cur_table, success_str);
+ fail_obj = g_hash_table_lookup(cur_table, fail_str);
+
+ if (!(success_obj && fail_obj))
+ return 0;
+
+ ignore_value(virJSONValueGetNumberUlong(success_obj, &curHaltPollSuccess));
+ ignore_value(virJSONValueGetNumberUlong(fail_obj, &curHaltPollFail));
+
+ haltPollSuccess += curHaltPollSuccess;
+ haltPollFail += curHaltPollFail;
+ }
+ } else {
+ pid_t pid = dom->pid;
+
+ if (virHostCPUGetHaltPollTime(pid, &haltPollSuccess, &haltPollFail) < 0)
+ return 0;
+ }
if (virTypedParamListAddULLong(params, haltPollSuccess, "cpu.haltpoll.success.time") < 0 ||
virTypedParamListAddULLong(params, haltPollFail, "cpu.haltpoll.fail.time") < 0)
@@ -18073,7 +18127,7 @@ static int
qemuDomainGetStatsCpu(virQEMUDriver *driver,
virDomainObj *dom,
virTypedParamList *params,
- unsigned int privflags G_GNUC_UNUSED)
+ unsigned int privflags)
{
if (qemuDomainGetStatsCpuCgroup(dom, params) < 0)
return -1;
@@ -18081,7 +18135,7 @@ qemuDomainGetStatsCpu(virQEMUDriver *driver,
if (qemuDomainGetStatsCpuCache(driver, dom, params) < 0)
return -1;
- if (qemuDomainGetStatsCpuHaltPollTime(dom, params) < 0)
+ if (qemuDomainGetStatsCpuHaltPollTime(driver, dom, params, privflags) < 0)
return -1;
return 0;
@@ -18915,7 +18969,7 @@ static virQEMUCapsFlags queryDirtyRateRequired[] = {
static struct qemuDomainGetStatsWorker qemuDomainGetStatsWorkers[] = {
{ qemuDomainGetStatsState, VIR_DOMAIN_STATS_STATE, false, NULL },
- { qemuDomainGetStatsCpu, VIR_DOMAIN_STATS_CPU_TOTAL, false, NULL },
+ { qemuDomainGetStatsCpu, VIR_DOMAIN_STATS_CPU_TOTAL, true, NULL },
{ qemuDomainGetStatsBalloon, VIR_DOMAIN_STATS_BALLOON, true, NULL },
{ qemuDomainGetStatsVcpu, VIR_DOMAIN_STATS_VCPU, true, NULL },
{ qemuDomainGetStatsInterface, VIR_DOMAIN_STATS_INTERFACE, false, NULL },
--
2.36.1On Wed, Jul 06, 2022 at 04:56:28AM +0530, Amneesh Singh wrote:
>Related: https://gitlab.com/libvirt/libvirt/-/issues/276
>
>This patch uses qemuMonitorQueryStats to query "halt_poll_success_ns"
>and "halt_poll_fail_ns" for every vCPU. The respective values for each
>vCPU are then added together.
>
>Signed-off-by: Amneesh Singh <natto@weirdnatto.in>
>---
> src/qemu/qemu_driver.c | 70 +++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 62 insertions(+), 8 deletions(-)
>
>diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>index 97c6ed95..30170d5c 100644
>--- a/src/qemu/qemu_driver.c
>+++ b/src/qemu/qemu_driver.c
>@@ -18052,15 +18052,69 @@ qemuDomainGetStatsCpuCgroup(virDomainObj *dom,
> }
>
> static int
>-qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom,
>- virTypedParamList *params)
>+qemuDomainGetStatsCpuHaltPollTime(virQEMUDriver *driver,
>+ virDomainObj *dom,
>+ virTypedParamList *params,
>+ unsigned int privflags)
> {
> unsigned long long haltPollSuccess = 0;
> unsigned long long haltPollFail = 0;
>- pid_t pid = dom->pid;
>+ qemuDomainObjPrivate *priv = dom->privateData;
>+ bool queryStatsCap = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_STATS);
>
>- if (virHostCPUGetHaltPollTime(pid, &haltPollSuccess, &haltPollFail) < 0)
>- return 0;
>+ if (queryStatsCap && HAVE_JOB(privflags) && virDomainObjIsActive(dom) && driver->privileged) {
Why is there a check for whether the driver is privileged? I thought
this can also work with a session mode.
>+ size_t i;
>+ qemuMonitorQueryStatsTargetType target = QEMU_MONITOR_QUERY_STATS_TARGET_VCPU;
>+ qemuMonitorQueryStatsProvider *provider = NULL;
>+ g_autoptr(GPtrArray) providers = NULL;
>+ g_autoptr(GPtrArray) queried_stats = NULL;
>+ provider = qemuMonitorQueryStatsProviderNew(
>+ target,
>+ QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM,
>+ QEMU_MONITOR_QUERY_STATS_VCPU_NAME_HALT_POLL_SUCCESS_NS,
>+ QEMU_MONITOR_QUERY_STATS_VCPU_NAME_HALT_POLL_FAIL_NS,
>+ -1);
>+
>+ if (!provider)
>+ return 0;
In this case you can still do the callback. Basically don't put it in
an "else" branch, just get a new condition checking if the stats were
gathered or not (if need be you can get a new variable for that).
>+
>+ providers = g_ptr_array_new_full(1, (GDestroyNotify) qemuMonitorQueryStatsProviderFree);
>+ g_ptr_array_add(providers, provider);
>+
>+ qemuDomainObjEnterMonitor(driver, dom);
>+ queried_stats = qemuMonitorQueryStats(priv->mon, target, NULL, providers);
>+ qemuDomainObjExitMonitor(dom);
>+
>+ if (!queried_stats)
>+ return 0;
>+
>+ for (i = 0; i < queried_stats->len; i++) {
>+ unsigned long long curHaltPollSuccess, curHaltPollFail;
>+ GHashTable *cur_table = queried_stats->pdata[i];
>+ virJSONValue *success_obj, *fail_obj;
>+ const char *success_str = qemuMonitorQueryStatsVcpuNameTypeToString(
>+ QEMU_MONITOR_QUERY_STATS_VCPU_NAME_HALT_POLL_SUCCESS_NS);
>+ const char *fail_str = qemuMonitorQueryStatsVcpuNameTypeToString(
>+ QEMU_MONITOR_QUERY_STATS_VCPU_NAME_HALT_POLL_FAIL_NS);
>+
>+ success_obj = g_hash_table_lookup(cur_table, success_str);
>+ fail_obj = g_hash_table_lookup(cur_table, fail_str);
>+
If you want it could be nicer to have an extra function for this since
it looks like it'll be repeated. Also have a look if the enums can't be
defined elsewhere/differently so that you can end up with something like
this:
virJSONValue *success_obj = qemuStatsGet(QEMU_STATS_NAME_HALT_POLL_SUCCESS_NS);
>+ if (!(success_obj && fail_obj))
>+ return 0;
>+
This checks if they both exist (although I would prefer (!success_obj ||
!fail_obj) as it is more readable and for me it is similar to how I
would say that in a sentence) but does not check that they really are a
number.
>+ ignore_value(virJSONValueGetNumberUlong(success_obj, &curHaltPollSuccess));
>+ ignore_value(virJSONValueGetNumberUlong(fail_obj, &curHaltPollFail));
>+
That's why you should also check that the virJSONValueGetNumberUlong
does not return -1. That's why it makes you use the return value.
Also these are another cases where you can execute the fallback instead.
Thanks for taking the time to polish this.
© 2016 - 2026 Red Hat, Inc.