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@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,
- 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),
&haltPollFail) < 0) 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