[PATCH RFC v2 3/3] qemu_driver: use qemuMonitorQueryStats to extract halt poll time

[Martin Kletzander]

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 at 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20220707/fb7efb22/attachment.sig>