2:03 a.m.
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>
>> > ---
>> > 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