5:47 a.m.
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>
>>> > ---
>>> > 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).
Thanks
>
>> 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
Amneesh