On Tue, Jun 28, 2022 at 10:15:28PM +0530, Amneesh Singh wrote:
On Tue, Jun 28, 2022 at 05:23:11PM +0200, Michal Prívozník wrote: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 ;-)Sure, that sounds doable :)> > - 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};Well, cannot really assign char ** with a scalar initialization, but what might work is something like const char *names[] = { success_str, fail_str, NULL }; provider = qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM); provider->names = g_strdupv((char **) names);
I think what Michal was trying to say is that since you do not change the array anywhere, there is no need for that to be a dynamically allocated array that needs to be freed. I, however, am not 100% if you are going to need this to be dynamic and whether you will be changing these arrays.
Another thought was using GStrvBuilder but it is not avaiable as per GLIB_VERSION_MAX. I too think that the current approach is not very nice. A variadic function similar to g_strv_builder_add_many that initializes a char ** would be nice.
If that is something that helps, then we can write it ourselves and only use our implementation if glib is too old.
> + > + 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(); }I see that makes sense. I shall do the necessary changes. Thanks for the detailed explanation.> + > + 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.I do understand that this is not an unfounded worry. I shall put the checks there.> + > + 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. MichalThanks for taking the time to review the patches. Amneesh
Attachment:
signature.asc
Description: PGP signature
