On 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@xxxxxxxxxxxxx> --- 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.
Attachment:
signature.asc
Description: PGP signature
