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 <na...@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 ;-)
>> 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.
>
>> 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.
Yeah, in general we could just copy glib's implementation into
src/util/glibcompat.c and drop it later on, but I think we can do
without strings (at least dynamically allocated ones).
One of the reasons I want to avoid working with strings at this level is
(besides the obvious memory complexity): compiler can't check for
correctness. If I made a typo in either of strings
("halt_poll_sucess_ns") then nothing warns me.
Michal
