On 6/24/22 10:14, 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 | 48 ++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 44 insertions(+), 4 deletions(-) > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 3b5c3db6..0a2be6d3 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -18057,10 +18057,50 @@ qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom, > { > unsigned long long haltPollSuccess = 0; > unsigned long long haltPollFail = 0; > - pid_t pid = dom->pid; > + qemuDomainObjPrivate *priv = dom->privateData; > + bool canQueryStats = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_STATS); Is this variable really needed? I mean, we can just: if (virQEMUCapsGet(...) { } else { } But if you want to avoid long lines, then perhaps rename the variable to queryStatsCap? This way it's more obvious what the variable reflects. Stats can be queried in both cases ;-) > > - if (virHostCPUGetHaltPollTime(pid, &haltPollSuccess, &haltPollFail) < 0) > - return 0; > + if (!canQueryStats) { > + pid_t pid = dom->pid; > + > + if (virHostCPUGetHaltPollTime(pid, &haltPollSuccess, &haltPollFail) < 0) > + return 0; > + } else { > + 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; > + const char *success_str = "halt_poll_success_ns"; > + const char *fail_str = "halt_poll_fail_ns"; > + > + provider = qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM); > + provider->names = g_new0(char *, 3); > + provider->names[0] = g_strdup(success_str), provider->names[1] = g_strdup(fail_str); I'm starting to wonder whether this is a good interface. These ->names[] array is never changed, so maybe we can have it as a NULL terminated list of constant strings? For instance: provider = qemuMonitorQueryStatsProviderNew(); provider->names = {"halt_poll_success_ns", "halt_poll_fail_ns", NULL}; > + > + providers = g_ptr_array_new_full(1, (GDestroyNotify) qemuMonitorQueryStatsProviderFree); > + g_ptr_array_add(providers, provider); > + > + queried_stats = qemuMonitorQueryStats(priv->mon, target, NULL, providers); This issues monitor command without proper checks. Firstly, you need to check whether job acquiring (that s/false/true/ change done in the last hunk) was successful and the domain is still running: if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom)) { /* code here */ } Then, you need to so called "enter the monitor" and "exit the monitor". So what happens here is that @vm is when this function is called. Now, issuing a monitor command with the domain object lock held is potentially very dangerous because if QEMU takes long time to reply (or it doesn't reply at all) the domain object is locked an can't be destroyed (virsh destroy) - because the first thing that qemuDomainDestroyFlags() does is locking the domain object. Also, you want to make sure no other thread is currently talking on the monitor - so the monitor has to be locked too. We have two convenient functions for these operations: qemuDomainObjEnterMonitor() qemuDomainObjExitMonitor() This is all described in more detail in docs/kbase/internals/qemu-threads.rst. Having said all of this, I think we can fallback to the current behavior if job wasn't acquired successfully. Therefore the condition at the very beginning might look like this: if (queryStatsCap && HAVE_JOB() && virDomainObjIsActive()) { ... qemuDomainEnterMonitor(); qemuMonitorQueryStats(); qemuDomainExitMonitor(); } else { virHostCPUGetHaltPollTime(); } > + > + 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, *fail; > + > + success = g_hash_table_lookup(cur_table, success_str); > + fail = g_hash_table_lookup(cur_table, fail_str); > + > + ignore_value(virJSONValueGetNumberUlong(success, &curHaltPollSuccess)); > + ignore_value(virJSONValueGetNumberUlong(fail, &curHaltPollFail)); I don't think this is a safe thing to do. What if either of @success or @fail is NULL? That can happen when QEMU didn't return requested member. True, at that point the 'query-stats' command should have failed, but I think it's more defensive if we checked these pointers. My worry is that virJSONValueGetNumberUlong() dereferences the pointer right away. > + > + haltPollSuccess += curHaltPollSuccess; > + haltPollFail += curHaltPollFail; > + } > + } > > if (virTypedParamListAddULLong(params, haltPollSuccess, "cpu.haltpoll.success.time") < 0 || > virTypedParamListAddULLong(params, haltPollFail, "cpu.haltpoll.fail.time") < 0) > @@ -18915,7 +18955,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 }, Please feel free to object to anything I wrote. Maybe you have more patches written on a local branch that justify your choices. I don't know. Michal
