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

[Michal Prívozník]

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 at weirdnatto.in>
> ---
>  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