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

On Wed, Jun 29, 2022 at 09:03:33AM +0200, Michal Prívozník wrote: > On 6/28/22 22:25, Martin Kletzander wrote: >> 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(a)weirdnatto.in&gt; >>>>> --- >>>>>   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); >>> > > Yep, I made too much of a shortcut. > >> >> 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. > > Yes. I'm not exactly sure why those strings have to be strdup-ed(). I > mean, from the conceptual POV. They are not changed anywhere. And now > that I think about it - why they have to be strings at all? They have to > be strings at the monitor level, because that's what QEMU expects, > however, I can imagine that in our code, in its upper layers those can > be just part of enum. Or even better - a bitmask - just like > virQEMUCaps. For instance: > > qemu_monitor.h: > > typedef enum { > QEMU_MONITOR_QUERY_STATS_HALT_POLL_SUCCESS_NS, > QEMU_MONITOR_QUERY_STATS_HALT_POLL_FAIL_NS, > > QEMU_MONITOR_QUERY_STATS_LAST > } qemuMonitorQueryStatsFlags; > > > qemu_monitor.c: > > VIR_ENUM_DECL(qemuMonitorQueryStatsFlags); > VIR_ENUM_IMPL(qemuMonitorQueryStatsFlags, > QEMU_MONITOR_QUERY_STATS_LAST, > "halt_poll_success_ns", > "halt_poll_fail_ns" > ); > > and when constructing the monitor command > qemuMonitorQueryStatsFlagsTypeToString() translates those > QEMU_MONITOR_QUERY_STATS_* enum members into their corresponding string > representation. > > Now, qemuMonitorQueryStatsProviderNew() could then behave like this: > > provider = qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM); > virBitmapSet(provider->names, QEMU_MONITOR_QUERY_STATS_HALT_POLL_SUCCESS_NS); > virBitmapSet(provider->names, QEMU_MONITOR_QUERY_STATS_HALT_POLL_FAIL_NS); > > Alternatively, we can have all of that in the > qemuMonitorQueryStatsProviderNew() call with help of va_args, like this: > > > qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM, > QEMU_MONITOR_QUERY_STATS_HALT_POLL_SUCCESS_NS, > QEMU_MONITOR_QUERY_STATS_HALT_POLL_FAIL_NS, > 0); > > Then, when constructing the monitor command, virBitmapNextSetBit() could > be used in a loop to iterate over set bits in combination with > aforementioned qemuMonitorQueryStatsFlagsTypeToString() to translate bit > index into string. Sorry for not specifying this earlier but the statistics are exposed by KVM and there is no enum for the statistics at the QAPI level either. But your point still stands, I definitely wanted to use an enum for this too as it makes things much easier for everyone. I was not sure whether it was a good idea to use one for things not known at the compile time, but to be fair, using raw strings like this is also pretty much the same. Unless the user wants to query some stat, that has not been updated in the libvirt enum, using a string, doing this should be alright. Do you think we can move forward with this? The user can do the latter using qemu-monitor-command anyway.

Also note that the enums for each target would be different (VM and VCPU for now).