[PATCH 0/3] qemu: add an API for "query-stats" QMP command
QEMU is gaining introspectable statistics which can be queried via the "query-stats" QMP command. This patchset aims to add an API for the same. The returned JSON for "query-stats" is an array of objects containing their own respective array of statistics. Patch 1 adds the API which returns the deserialized JSON in the form of a GPtrArray of GHashTables. Patch 2 adds the "query-stats" to QEMU capabilities. Patch 3 uses the API to query the halt poll success time and the halt poll failure time. v2 -> v3 ======== Sorry for the late patchset, I was under the impression I had sent it on Monday but apparently, I did not. [1/3]: - use a single enum for all the statistics. - use QEMU_MONITOR_QUERY_STATS_NAME_LAST as the sentinel value for the provider constructor. - relevant changes to enum values. [2/3]: - fix comment spacing [3/3] - better checks v1 -> v2 ======== I have been tinkering with the v1 patchset and have rewrote the v2 patches a couple of times. I believe the current patchset is still not perfect and would appreciate some reviews. I have another patch or two written but they do not make any significant changes to the current patchset. [1/3]: - use virBitmap instead of an array of strings for statistics. - add enums for the stat names and add qemuMonitorQueryStatsNameTypeToString to switch between the "ToString" functions based on the target type. - change qemuMonitorQueryStatsProviderNew to a variadic function that takes stat enum values with the sentinel value being -1. [2/3]: - No changes [3/3]: - Add relevant monitor related checks to check if the domain is active. - Acquire and release qemuMonitorObj lock before and after calling qemuMonitorQueryStats respectively. - Add the check for privileged access. Relevant QEMU patches can be found here: https://lore.kernel.org/all/20220530150714.756954-1-pbonzini@redhat.com/ This patchset is part of the 2022 GSOC contributor project. Amneesh Singh (3): qemu_monitor: add qemuMonitorQueryStats qemu_capabilities: add "query-stats" QMP command to the QEMU capabilities qemu_driver: use qemuMonitorQueryStats to extract halt poll time src/qemu/qemu_capabilities.c | 2 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_driver.c | 69 ++++++++++++++++--- src/qemu/qemu_monitor.c | 70 +++++++++++++++++++ src/qemu/qemu_monitor.h | 45 ++++++++++++ src/qemu/qemu_monitor_json.c | 130 +++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 6 ++ 7 files changed, 315 insertions(+), 8 deletions(-) -- 2.36.1
Related: https://gitlab.com/libvirt/libvirt/-/issues/276 This patch adds an API for the "query-stats" QMP command. The query returns a JSON containing the statistics based on the target, which can either be vCPU or VM, and the providers. The API deserializes the query result into an array of GHashMaps, which can later be used to extract all the query statistics. GHashMaps are used to avoid traversing the entire array to find the statistics you are looking for. This would be a singleton array if the target is a VM since the returned JSON is also a singleton array in that case. Signed-off-by: Amneesh Singh <natto@weirdnatto.in> --- src/qemu/qemu_monitor.c | 70 +++++++++++++++++++ src/qemu/qemu_monitor.h | 45 ++++++++++++ src/qemu/qemu_monitor_json.c | 130 +++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 6 ++ 4 files changed, 251 insertions(+) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index fda5d2f3..a07e6017 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4541,3 +4541,73 @@ qemuMonitorMigrateRecover(qemuMonitor *mon, return qemuMonitorJSONMigrateRecover(mon, uri); } + +VIR_ENUM_IMPL(qemuMonitorQueryStatsTarget, + QEMU_MONITOR_QUERY_STATS_TARGET_LAST, + "vm", + "vcpu", +); + +VIR_ENUM_IMPL(qemuMonitorQueryStatsName, + QEMU_MONITOR_QUERY_STATS_NAME_LAST, + "halt_poll_success_ns", + "halt_poll_fail_ns" +); + +VIR_ENUM_IMPL(qemuMonitorQueryStatsProvider, + QEMU_MONITOR_QUERY_STATS_PROVIDER_LAST, + "kvm", +); + +void +qemuMonitorQueryStatsProviderFree(qemuMonitorQueryStatsProvider *provider) +{ + virBitmapFree(provider->names); + g_free(provider); +} + +qemuMonitorQueryStatsProvider * +qemuMonitorQueryStatsProviderNew(qemuMonitorQueryStatsProviderType provider_type, + ...) +{ + g_autoptr(qemuMonitorQueryStatsProvider) provider = NULL; + qemuMonitorQueryStatsNameType stat; + va_list name_list; + size_t sentinel = QEMU_MONITOR_QUERY_STATS_NAME_LAST; + + provider = g_new0(qemuMonitorQueryStatsProvider, 1); + provider->provider = provider_type; + provider->names = NULL; + + va_start(name_list, provider_type); + stat = va_arg(name_list, qemuMonitorQueryStatsNameType); + + if (stat != sentinel) { + provider->names = virBitmapNew(QEMU_MONITOR_QUERY_STATS_NAME_LAST); + + while (stat != sentinel) { + if (virBitmapSetBit(provider->names, stat) < 0) + return NULL; + stat = va_arg(name_list, qemuMonitorQueryStatsNameType); + } + } + va_end(name_list); + + return g_steal_pointer(&provider); +} + +GPtrArray * +qemuMonitorQueryStats(qemuMonitor *mon, + qemuMonitorQueryStatsTargetType target, + char **vcpus, + GPtrArray *providers) +{ + VIR_DEBUG("target=%u vcpus=%p providers=%p", target, vcpus, providers); + + QEMU_CHECK_MONITOR_NULL(mon); + + if (target != QEMU_MONITOR_QUERY_STATS_TARGET_VCPU && vcpus) + return NULL; + + return qemuMonitorJSONQueryStats(mon, target, vcpus, providers); +} diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 95267ec6..344636e1 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1554,3 +1554,48 @@ qemuMonitorChangeMemoryRequestedSize(qemuMonitor *mon, int qemuMonitorMigrateRecover(qemuMonitor *mon, const char *uri); + +typedef enum { + QEMU_MONITOR_QUERY_STATS_TARGET_VM, + QEMU_MONITOR_QUERY_STATS_TARGET_VCPU, + QEMU_MONITOR_QUERY_STATS_TARGET_LAST, +} qemuMonitorQueryStatsTargetType; + +VIR_ENUM_DECL(qemuMonitorQueryStatsTarget); + +typedef enum { + QEMU_MONITOR_QUERY_STATS_NAME_HALT_POLL_SUCCESS_NS, + QEMU_MONITOR_QUERY_STATS_NAME_HALT_POLL_FAIL_NS, + QEMU_MONITOR_QUERY_STATS_NAME_LAST, +} qemuMonitorQueryStatsNameType; + +VIR_ENUM_DECL(qemuMonitorQueryStatsName); + +typedef enum { + QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM, + QEMU_MONITOR_QUERY_STATS_PROVIDER_LAST, +} qemuMonitorQueryStatsProviderType; + +VIR_ENUM_DECL(qemuMonitorQueryStatsProvider); + +typedef struct _qemuMonitorQueryStatsProvider qemuMonitorQueryStatsProvider; +struct _qemuMonitorQueryStatsProvider { + qemuMonitorQueryStatsProviderType provider; + virBitmap *names; +}; + +void +qemuMonitorQueryStatsProviderFree(qemuMonitorQueryStatsProvider *provider); + +qemuMonitorQueryStatsProvider * +qemuMonitorQueryStatsProviderNew(qemuMonitorQueryStatsProviderType provider_type, + ...); + +G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuMonitorQueryStatsProvider, + qemuMonitorQueryStatsProviderFree); + +GPtrArray * +qemuMonitorQueryStats(qemuMonitor *mon, + qemuMonitorQueryStatsTargetType target, + char **vcpus, + GPtrArray *providers); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 80696de7..4e228bfb 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -9031,3 +9031,133 @@ qemuMonitorJSONMigrateRecover(qemuMonitor *mon, return qemuMonitorJSONCheckError(cmd, reply); } + +static GPtrArray * +qemuMonitorJSONExtractQueryStats(virJSONValue *arr) +{ + g_autoptr(GPtrArray) queried_stats = NULL; + size_t nstats = virJSONValueArraySize(arr); + size_t i; + + /* Create a GPtrArray for GHashTables */ + queried_stats = g_ptr_array_new_full(nstats, (GDestroyNotify) g_hash_table_destroy); + + for (i = 0; i < nstats; i++) { + virJSONValue *obj = virJSONValueArrayGet(arr, i); + virJSONValue *stats = virJSONValueObjectGetArray(obj, "stats"); + size_t j; + + /* Create a GHashTable for virJSONValues */ + GHashTable *hash_table = virHashNew((GDestroyNotify) virJSONValueFree); + + for (j = 0; j < virJSONValueArraySize(stats); j++) { + virJSONValue *stat = virJSONValueArrayGet(stats, j); + + g_hash_table_insert(hash_table, + g_strdup(virJSONValueObjectGetString(stat, "name")), + virJSONValueObjectGet(stat, "value")); + } + + g_ptr_array_add(queried_stats, hash_table); + } + + return g_steal_pointer(&queried_stats); +} + + +/** + * qemuMonitorJSONQueryStats: + * @mon: monitor object + * @target: the target type for the query + * @vcpus: a list of vCPU QOM paths for filtering the statistics + * @providers: an array of providers to filter statistics + * + * @vcpus is a NULL terminated array of strings. @providers is a GPtrArray + * for qemuMonitorQueryStatsProvider. + * @vcpus and @providers are optional and can be NULL. + * + * Queries for the @target based statistics. + * Returns a GPtrArray of GHashTables containing the statistics on success or + * NULL on failure. + */ + +GPtrArray * +qemuMonitorJSONQueryStats(qemuMonitor *mon, + qemuMonitorQueryStatsTargetType target, + char **vcpus, + GPtrArray *providers) +{ + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; + g_autoptr(virJSONValue) vcpu_list = NULL; + g_autoptr(virJSONValue) provider_list = NULL; + virJSONValue *rv = NULL; + + size_t i; + + if (providers) { + provider_list = virJSONValueNewArray(); + + for (i = 0; i < providers->len; i++) { + g_autoptr(virJSONValue) provider_obj = virJSONValueNewObject(); + qemuMonitorQueryStatsProvider *provider = providers->pdata[i]; + const char *type_str = qemuMonitorQueryStatsProviderTypeToString(provider->provider); + virBitmap *names = provider->names; + int rc; + + rc = virJSONValueObjectAppendString(provider_obj, "provider", type_str); + + if (rc < 0) + return NULL; + + if (names) { + g_autoptr(virJSONValue) provider_names = virJSONValueNewArray(); + int curBit = virBitmapNextSetBit(names, -1); + + while (curBit >= 0) { + const char *name = qemuMonitorQueryStatsNameTypeToString(curBit); + + if (virJSONValueArrayAppendString(provider_names, name) < 0) + return NULL; + + curBit = virBitmapNextSetBit(names, curBit); + } + + rc = virJSONValueObjectAppend(provider_obj, "names", &provider_names); + + if (rc < 0) + return NULL; + } + + if (virJSONValueArrayAppend(provider_list, &provider_obj) < 0) + return NULL; + } + } + + if (vcpus) { + vcpu_list = virJSONValueNewArray(); + + for (i = 0; vcpus[i]; i++) + if (virJSONValueArrayAppendString(vcpu_list, vcpus[i]) < 0) + return NULL; + } + + cmd = qemuMonitorJSONMakeCommand("query-stats", + "s:target", qemuMonitorQueryStatsTargetTypeToString(target), + "A:vcpus", &vcpu_list, + "A:providers", &provider_list, + NULL); + + if (!cmd) + return NULL; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + return NULL; + + if (qemuMonitorJSONCheckError(cmd, reply) < 0) + return NULL; + + rv = virJSONValueObjectStealArray(reply, "return"); + + return qemuMonitorJSONExtractQueryStats(rv); +} diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index ad3853ae..db8dec80 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -878,3 +878,9 @@ qemuMonitorJSONChangeMemoryRequestedSize(qemuMonitor *mon, int qemuMonitorJSONMigrateRecover(qemuMonitor *mon, const char *uri); + +GPtrArray * +qemuMonitorJSONQueryStats(qemuMonitor *mon, + qemuMonitorQueryStatsTargetType target, + char **vcpus, + GPtrArray *providers); -- 2.36.1
On Thu, Jul 14, 2022 at 7:54 AM Amneesh Singh <natto@weirdnatto.in> wrote:
Related: https://gitlab.com/libvirt/libvirt/-/issues/276
This patch adds an API for the "query-stats" QMP command.
The query returns a JSON containing the statistics based on the target, which can either be vCPU or VM, and the providers. The API deserializes the query result into an array of GHashMaps, which can later be used to extract all the query statistics. GHashMaps are used to avoid traversing the entire array to find the statistics you are looking for. This would be a singleton array if the target is a VM since the returned JSON is also a singleton array in that case.
Signed-off-by: Amneesh Singh <natto@weirdnatto.in> --- src/qemu/qemu_monitor.c | 70 +++++++++++++++++++ src/qemu/qemu_monitor.h | 45 ++++++++++++ src/qemu/qemu_monitor_json.c | 130 +++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 6 ++ 4 files changed, 251 insertions(+)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index fda5d2f3..a07e6017 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4541,3 +4541,73 @@ qemuMonitorMigrateRecover(qemuMonitor *mon,
return qemuMonitorJSONMigrateRecover(mon, uri); } + +VIR_ENUM_IMPL(qemuMonitorQueryStatsTarget, + QEMU_MONITOR_QUERY_STATS_TARGET_LAST, + "vm", + "vcpu", +); + +VIR_ENUM_IMPL(qemuMonitorQueryStatsName, + QEMU_MONITOR_QUERY_STATS_NAME_LAST, + "halt_poll_success_ns", + "halt_poll_fail_ns" +); + +VIR_ENUM_IMPL(qemuMonitorQueryStatsProvider, + QEMU_MONITOR_QUERY_STATS_PROVIDER_LAST, + "kvm", +); + +void +qemuMonitorQueryStatsProviderFree(qemuMonitorQueryStatsProvider *provider) +{ + virBitmapFree(provider->names); + g_free(provider); +} + +qemuMonitorQueryStatsProvider * +qemuMonitorQueryStatsProviderNew(qemuMonitorQueryStatsProviderType provider_type, + ...) +{ + g_autoptr(qemuMonitorQueryStatsProvider) provider = NULL; + qemuMonitorQueryStatsNameType stat; + va_list name_list; + size_t sentinel = QEMU_MONITOR_QUERY_STATS_NAME_LAST; + + provider = g_new0(qemuMonitorQueryStatsProvider, 1); + provider->provider = provider_type; + provider->names = NULL; + + va_start(name_list, provider_type); + stat = va_arg(name_list, qemuMonitorQueryStatsNameType); + + if (stat != sentinel) {
It would be better to compare 'stat' with QEMU_MONITOR_QUERY_STATS_NAME_LAST directly without the additional variable 'sentinel'.
+ provider->names = virBitmapNew(QEMU_MONITOR_QUERY_STATS_NAME_LAST); + + while (stat != sentinel) {
Same here. This while cycle would be better outside the if case, such as: if (stat != sentinel) provider->names = virBitmapNew(QEMU_MONITOR_QUERY_STATS_NAME_LAST); while (stat != sentinel) { .... } Because the 'while' cycle has the same condition as the 'if' case.
+ if (virBitmapSetBit(provider->names, stat) < 0) + return NULL; + stat = va_arg(name_list, qemuMonitorQueryStatsNameType); + } + } + va_end(name_list); + + return g_steal_pointer(&provider); +} + +GPtrArray * +qemuMonitorQueryStats(qemuMonitor *mon, + qemuMonitorQueryStatsTargetType target, + char **vcpus, + GPtrArray *providers) +{ + VIR_DEBUG("target=%u vcpus=%p providers=%p", target, vcpus, providers); + + QEMU_CHECK_MONITOR_NULL(mon); + + if (target != QEMU_MONITOR_QUERY_STATS_TARGET_VCPU && vcpus) + return NULL; + + return qemuMonitorJSONQueryStats(mon, target, vcpus, providers); +} diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 95267ec6..344636e1 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1554,3 +1554,48 @@ qemuMonitorChangeMemoryRequestedSize(qemuMonitor *mon, int qemuMonitorMigrateRecover(qemuMonitor *mon, const char *uri); + +typedef enum { + QEMU_MONITOR_QUERY_STATS_TARGET_VM, + QEMU_MONITOR_QUERY_STATS_TARGET_VCPU, + QEMU_MONITOR_QUERY_STATS_TARGET_LAST, +} qemuMonitorQueryStatsTargetType; + +VIR_ENUM_DECL(qemuMonitorQueryStatsTarget); + +typedef enum { + QEMU_MONITOR_QUERY_STATS_NAME_HALT_POLL_SUCCESS_NS, + QEMU_MONITOR_QUERY_STATS_NAME_HALT_POLL_FAIL_NS, + QEMU_MONITOR_QUERY_STATS_NAME_LAST, +} qemuMonitorQueryStatsNameType; + +VIR_ENUM_DECL(qemuMonitorQueryStatsName); + +typedef enum { + QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM, + QEMU_MONITOR_QUERY_STATS_PROVIDER_LAST, +} qemuMonitorQueryStatsProviderType; + +VIR_ENUM_DECL(qemuMonitorQueryStatsProvider); + +typedef struct _qemuMonitorQueryStatsProvider qemuMonitorQueryStatsProvider; +struct _qemuMonitorQueryStatsProvider { + qemuMonitorQueryStatsProviderType provider;
It is a bit confusing for me to have a variable 'provider' in the 'provider' structure. Later you use provider->provider which is not very readable. I would prefer 'type' (maybe even 'providerType') instead.
+ virBitmap *names; +}; + +void +qemuMonitorQueryStatsProviderFree(qemuMonitorQueryStatsProvider *provider); + +qemuMonitorQueryStatsProvider * +qemuMonitorQueryStatsProviderNew(qemuMonitorQueryStatsProviderType provider_type, + ...); + +G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuMonitorQueryStatsProvider, + qemuMonitorQueryStatsProviderFree); + +GPtrArray * +qemuMonitorQueryStats(qemuMonitor *mon, + qemuMonitorQueryStatsTargetType target, + char **vcpus, + GPtrArray *providers); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 80696de7..4e228bfb 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -9031,3 +9031,133 @@ qemuMonitorJSONMigrateRecover(qemuMonitor *mon,
return qemuMonitorJSONCheckError(cmd, reply); } + +static GPtrArray * +qemuMonitorJSONExtractQueryStats(virJSONValue *arr) +{ + g_autoptr(GPtrArray) queried_stats = NULL; + size_t nstats = virJSONValueArraySize(arr); + size_t i; + + /* Create a GPtrArray for GHashTables */ + queried_stats = g_ptr_array_new_full(nstats, (GDestroyNotify) g_hash_table_destroy); + + for (i = 0; i < nstats; i++) { + virJSONValue *obj = virJSONValueArrayGet(arr, i); + virJSONValue *stats = virJSONValueObjectGetArray(obj, "stats"); + size_t j; + + /* Create a GHashTable for virJSONValues */ + GHashTable *hash_table = virHashNew((GDestroyNotify) virJSONValueFree); + + for (j = 0; j < virJSONValueArraySize(stats); j++) { + virJSONValue *stat = virJSONValueArrayGet(stats, j); + + g_hash_table_insert(hash_table, + g_strdup(virJSONValueObjectGetString(stat, "name")), + virJSONValueObjectGet(stat, "value")); + } + + g_ptr_array_add(queried_stats, hash_table); + } + + return g_steal_pointer(&queried_stats); +} + + +/** + * qemuMonitorJSONQueryStats: + * @mon: monitor object + * @target: the target type for the query + * @vcpus: a list of vCPU QOM paths for filtering the statistics + * @providers: an array of providers to filter statistics + * + * @vcpus is a NULL terminated array of strings. @providers is a GPtrArray + * for qemuMonitorQueryStatsProvider. + * @vcpus and @providers are optional and can be NULL. + * + * Queries for the @target based statistics. + * Returns a GPtrArray of GHashTables containing the statistics on success or + * NULL on failure. + */ + +GPtrArray * +qemuMonitorJSONQueryStats(qemuMonitor *mon, + qemuMonitorQueryStatsTargetType target, + char **vcpus, + GPtrArray *providers) +{ + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; + g_autoptr(virJSONValue) vcpu_list = NULL; + g_autoptr(virJSONValue) provider_list = NULL; + virJSONValue *rv = NULL; + + size_t i; + + if (providers) { + provider_list = virJSONValueNewArray(); + + for (i = 0; i < providers->len; i++) { + g_autoptr(virJSONValue) provider_obj = virJSONValueNewObject(); + qemuMonitorQueryStatsProvider *provider = providers->pdata[i]; + const char *type_str = qemuMonitorQueryStatsProviderTypeToString(provider->provider); + virBitmap *names = provider->names; + int rc; +
We usually use 'rc' (or any variable storing the result) if we need the value also after the check of its return code. When we only need to check the result, we usually compare the value directly: if (virJSONValueObjectAppendString(provider_obj, "provider", type_str) < 0) return NULL;
+ rc = virJSONValueObjectAppendString(provider_obj, "provider", type_str); + + if (rc < 0) + return NULL; + + if (names) { + g_autoptr(virJSONValue) provider_names = virJSONValueNewArray(); + int curBit = virBitmapNextSetBit(names, -1); + + while (curBit >= 0) { + const char *name = qemuMonitorQueryStatsNameTypeToString(curBit); + + if (virJSONValueArrayAppendString(provider_names, name) < 0) + return NULL; + + curBit = virBitmapNextSetBit(names, curBit); + } + + rc = virJSONValueObjectAppend(provider_obj, "names", &provider_names); +
Same here
+ if (rc < 0) + return NULL; + } + + if (virJSONValueArrayAppend(provider_list, &provider_obj) < 0) + return NULL; + } + } + + if (vcpus) { + vcpu_list = virJSONValueNewArray(); + + for (i = 0; vcpus[i]; i++) + if (virJSONValueArrayAppendString(vcpu_list, vcpus[i]) < 0) + return NULL; + } + + cmd = qemuMonitorJSONMakeCommand("query-stats", + "s:target", qemuMonitorQueryStatsTargetTypeToString(target), + "A:vcpus", &vcpu_list, + "A:providers", &provider_list, + NULL); + + if (!cmd) + return NULL; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + return NULL; + + if (qemuMonitorJSONCheckError(cmd, reply) < 0) + return NULL; + + rv = virJSONValueObjectStealArray(reply, "return"); + + return qemuMonitorJSONExtractQueryStats(rv); +} diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index ad3853ae..db8dec80 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -878,3 +878,9 @@ qemuMonitorJSONChangeMemoryRequestedSize(qemuMonitor *mon, int qemuMonitorJSONMigrateRecover(qemuMonitor *mon, const char *uri); + +GPtrArray * +qemuMonitorJSONQueryStats(qemuMonitor *mon, + qemuMonitorQueryStatsTargetType target, + char **vcpus, + GPtrArray *providers); -- 2.36.1
Kind regards, Kristina
On Fri, Jul 22, 2022 at 02:44:30PM +0200, Kristina Hanicova wrote:
On Thu, Jul 14, 2022 at 7:54 AM Amneesh Singh <natto@weirdnatto.in> wrote:
Related: https://gitlab.com/libvirt/libvirt/-/issues/276
This patch adds an API for the "query-stats" QMP command.
The query returns a JSON containing the statistics based on the target, which can either be vCPU or VM, and the providers. The API deserializes the query result into an array of GHashMaps, which can later be used to extract all the query statistics. GHashMaps are used to avoid traversing the entire array to find the statistics you are looking for. This would be a singleton array if the target is a VM since the returned JSON is also a singleton array in that case.
Signed-off-by: Amneesh Singh <natto@weirdnatto.in> --- src/qemu/qemu_monitor.c | 70 +++++++++++++++++++ src/qemu/qemu_monitor.h | 45 ++++++++++++ src/qemu/qemu_monitor_json.c | 130 +++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 6 ++ 4 files changed, 251 insertions(+)
[...]
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 95267ec6..344636e1 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1554,3 +1554,48 @@ qemuMonitorChangeMemoryRequestedSize(qemuMonitor *mon, int qemuMonitorMigrateRecover(qemuMonitor *mon, const char *uri); + +typedef enum { + QEMU_MONITOR_QUERY_STATS_TARGET_VM, + QEMU_MONITOR_QUERY_STATS_TARGET_VCPU, + QEMU_MONITOR_QUERY_STATS_TARGET_LAST, +} qemuMonitorQueryStatsTargetType; + +VIR_ENUM_DECL(qemuMonitorQueryStatsTarget); + +typedef enum { + QEMU_MONITOR_QUERY_STATS_NAME_HALT_POLL_SUCCESS_NS, + QEMU_MONITOR_QUERY_STATS_NAME_HALT_POLL_FAIL_NS, + QEMU_MONITOR_QUERY_STATS_NAME_LAST, +} qemuMonitorQueryStatsNameType; + +VIR_ENUM_DECL(qemuMonitorQueryStatsName); + +typedef enum { + QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM, + QEMU_MONITOR_QUERY_STATS_PROVIDER_LAST, +} qemuMonitorQueryStatsProviderType; + +VIR_ENUM_DECL(qemuMonitorQueryStatsProvider); + +typedef struct _qemuMonitorQueryStatsProvider qemuMonitorQueryStatsProvider; +struct _qemuMonitorQueryStatsProvider { + qemuMonitorQueryStatsProviderType provider;
It is a bit confusing for me to have a variable 'provider' in the 'provider' structure. Later you use provider->provider which is not very readable. I would prefer 'type' (maybe even 'providerType') instead.
I think I suggested `type` in previous review. I'll fix this up and the other comments (mine as well) before pushing unless there is something bigger in the last patch.
On Fri, Jul 22, 2022 at 02:44:30PM +0200, Kristina Hanicova wrote:
On Thu, Jul 14, 2022 at 7:54 AM Amneesh Singh <natto@weirdnatto.in> wrote:
Related: https://gitlab.com/libvirt/libvirt/-/issues/276
This patch adds an API for the "query-stats" QMP command.
The query returns a JSON containing the statistics based on the target, which can either be vCPU or VM, and the providers. The API deserializes the query result into an array of GHashMaps, which can later be used to extract all the query statistics. GHashMaps are used to avoid traversing the entire array to find the statistics you are looking for. This would be a singleton array if the target is a VM since the returned JSON is also a singleton array in that case.
Signed-off-by: Amneesh Singh <natto@weirdnatto.in> --- src/qemu/qemu_monitor.c | 70 +++++++++++++++++++ src/qemu/qemu_monitor.h | 45 ++++++++++++ src/qemu/qemu_monitor_json.c | 130 +++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 6 ++ 4 files changed, 251 insertions(+)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index fda5d2f3..a07e6017 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4541,3 +4541,73 @@ qemuMonitorMigrateRecover(qemuMonitor *mon,
return qemuMonitorJSONMigrateRecover(mon, uri); } + +VIR_ENUM_IMPL(qemuMonitorQueryStatsTarget, + QEMU_MONITOR_QUERY_STATS_TARGET_LAST, + "vm", + "vcpu", +); + +VIR_ENUM_IMPL(qemuMonitorQueryStatsName, + QEMU_MONITOR_QUERY_STATS_NAME_LAST, + "halt_poll_success_ns", + "halt_poll_fail_ns" +); + +VIR_ENUM_IMPL(qemuMonitorQueryStatsProvider, + QEMU_MONITOR_QUERY_STATS_PROVIDER_LAST, + "kvm", +); + +void +qemuMonitorQueryStatsProviderFree(qemuMonitorQueryStatsProvider *provider) +{ + virBitmapFree(provider->names); + g_free(provider); +} + +qemuMonitorQueryStatsProvider * +qemuMonitorQueryStatsProviderNew(qemuMonitorQueryStatsProviderType provider_type, + ...) +{ + g_autoptr(qemuMonitorQueryStatsProvider) provider = NULL; + qemuMonitorQueryStatsNameType stat; + va_list name_list; + size_t sentinel = QEMU_MONITOR_QUERY_STATS_NAME_LAST; + + provider = g_new0(qemuMonitorQueryStatsProvider, 1); + provider->provider = provider_type; + provider->names = NULL; + + va_start(name_list, provider_type); + stat = va_arg(name_list, qemuMonitorQueryStatsNameType); + + if (stat != sentinel) {
It would be better to compare 'stat' with QEMU_MONITOR_QUERY_STATS_NAME_LAST directly without the additional variable 'sentinel'.
+ provider->names = virBitmapNew(QEMU_MONITOR_QUERY_STATS_NAME_LAST); + + while (stat != sentinel) {
Same here.
This while cycle would be better outside the if case, such as:
if (stat != sentinel) provider->names = virBitmapNew(QEMU_MONITOR_QUERY_STATS_NAME_LAST);
while (stat != sentinel) { .... }
Because the 'while' cycle has the same condition as the 'if' case.
+ if (virBitmapSetBit(provider->names, stat) < 0) + return NULL; + stat = va_arg(name_list, qemuMonitorQueryStatsNameType); + } + } + va_end(name_list); + + return g_steal_pointer(&provider); +}
Sorry for so many review e-mails from me on this one patch, but while fixing few of the nitpicks I managed to rewrite the function quite a bit: qemuMonitorQueryStatsProvider *provider = g_new0(qemuMonitorQueryStatsProvider, 1); qemuMonitorQueryStatsNameType stat; va_list name_list; /* * This can be lowered later in case of the enum getting quite large, hence * the virBitmapSetExpand below. */ provider->names = virBitmapNew(QEMU_MONITOR_QUERY_STATS_NAME_LAST); provider->type = provider_type; va_start(name_list, provider_type); while ((stat = va_arg(name_list, qemuMonitorQueryStatsNameType)) != QEMU_MONITOR_QUERY_STATS_NAME_LAST) virBitmapSetBitExpand(provider->names, stat); va_end(name_list); return provider; Let me know (both of you) if you are okay with me using this version. Thanks
On Fri, Jul 22, 2022 at 05:02:06PM +0200, Martin Kletzander wrote:
On Fri, Jul 22, 2022 at 02:44:30PM +0200, Kristina Hanicova wrote:
On Thu, Jul 14, 2022 at 7:54 AM Amneesh Singh <natto@weirdnatto.in> wrote:
Related: https://gitlab.com/libvirt/libvirt/-/issues/276
This patch adds an API for the "query-stats" QMP command.
The query returns a JSON containing the statistics based on the target, which can either be vCPU or VM, and the providers. The API deserializes the query result into an array of GHashMaps, which can later be used to extract all the query statistics. GHashMaps are used to avoid traversing the entire array to find the statistics you are looking for. This would be a singleton array if the target is a VM since the returned JSON is also a singleton array in that case.
Signed-off-by: Amneesh Singh <natto@weirdnatto.in> --- src/qemu/qemu_monitor.c | 70 +++++++++++++++++++ src/qemu/qemu_monitor.h | 45 ++++++++++++ src/qemu/qemu_monitor_json.c | 130 +++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 6 ++ 4 files changed, 251 insertions(+)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index fda5d2f3..a07e6017 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4541,3 +4541,73 @@ qemuMonitorMigrateRecover(qemuMonitor *mon,
return qemuMonitorJSONMigrateRecover(mon, uri); } + +VIR_ENUM_IMPL(qemuMonitorQueryStatsTarget, + QEMU_MONITOR_QUERY_STATS_TARGET_LAST, + "vm", + "vcpu", +); + +VIR_ENUM_IMPL(qemuMonitorQueryStatsName, + QEMU_MONITOR_QUERY_STATS_NAME_LAST, + "halt_poll_success_ns", + "halt_poll_fail_ns" +); + +VIR_ENUM_IMPL(qemuMonitorQueryStatsProvider, + QEMU_MONITOR_QUERY_STATS_PROVIDER_LAST, + "kvm", +); + +void +qemuMonitorQueryStatsProviderFree(qemuMonitorQueryStatsProvider *provider) +{ + virBitmapFree(provider->names); + g_free(provider); +} + +qemuMonitorQueryStatsProvider * +qemuMonitorQueryStatsProviderNew(qemuMonitorQueryStatsProviderType provider_type, + ...) +{ + g_autoptr(qemuMonitorQueryStatsProvider) provider = NULL; + qemuMonitorQueryStatsNameType stat; + va_list name_list; + size_t sentinel = QEMU_MONITOR_QUERY_STATS_NAME_LAST; + + provider = g_new0(qemuMonitorQueryStatsProvider, 1); + provider->provider = provider_type; + provider->names = NULL; + + va_start(name_list, provider_type); + stat = va_arg(name_list, qemuMonitorQueryStatsNameType); + + if (stat != sentinel) {
It would be better to compare 'stat' with QEMU_MONITOR_QUERY_STATS_NAME_LAST directly without the additional variable 'sentinel'.
+ provider->names = virBitmapNew(QEMU_MONITOR_QUERY_STATS_NAME_LAST); + + while (stat != sentinel) {
Same here.
This while cycle would be better outside the if case, such as:
if (stat != sentinel) provider->names = virBitmapNew(QEMU_MONITOR_QUERY_STATS_NAME_LAST);
while (stat != sentinel) { .... }
Because the 'while' cycle has the same condition as the 'if' case.
+ if (virBitmapSetBit(provider->names, stat) < 0) + return NULL; + stat = va_arg(name_list, qemuMonitorQueryStatsNameType); + } + } + va_end(name_list); + + return g_steal_pointer(&provider); +}
Sorry for so many review e-mails from me on this one patch, but while fixing few of the nitpicks I managed to rewrite the function quite a bit:
qemuMonitorQueryStatsProvider *provider = g_new0(qemuMonitorQueryStatsProvider, 1); qemuMonitorQueryStatsNameType stat; va_list name_list;
/* * This can be lowered later in case of the enum getting quite large, hence * the virBitmapSetExpand below. */ provider->names = virBitmapNew(QEMU_MONITOR_QUERY_STATS_NAME_LAST); provider->type = provider_type;
va_start(name_list, provider_type);
while ((stat = va_arg(name_list, qemuMonitorQueryStatsNameType)) != QEMU_MONITOR_QUERY_STATS_NAME_LAST) virBitmapSetBitExpand(provider->names, stat);
va_end(name_list);
return provider;
Let me know (both of you) if you are okay with me using this version. Yes, that is absolutely fine, thanks for taking the time to review it. I will keep the suggestions in mind the next time I submit the patches.
Thanks
On Thu, Jul 14, 2022 at 11:22:02AM +0530, Amneesh Singh wrote:
Related: https://gitlab.com/libvirt/libvirt/-/issues/276
This patch adds an API for the "query-stats" QMP command.
The query returns a JSON containing the statistics based on the target, which can either be vCPU or VM, and the providers. The API deserializes the query result into an array of GHashMaps, which can later be used to extract all the query statistics. GHashMaps are used to avoid traversing the entire array to find the statistics you are looking for. This would be a singleton array if the target is a VM since the returned JSON is also a singleton array in that case.
Signed-off-by: Amneesh Singh <natto@weirdnatto.in> --- src/qemu/qemu_monitor.c | 70 +++++++++++++++++++ src/qemu/qemu_monitor.h | 45 ++++++++++++ src/qemu/qemu_monitor_json.c | 130 +++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 6 ++ 4 files changed, 251 insertions(+)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index fda5d2f3..a07e6017 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4541,3 +4541,73 @@ qemuMonitorMigrateRecover(qemuMonitor *mon,
return qemuMonitorJSONMigrateRecover(mon, uri); } + +VIR_ENUM_IMPL(qemuMonitorQueryStatsTarget, + QEMU_MONITOR_QUERY_STATS_TARGET_LAST, + "vm", + "vcpu", +); + +VIR_ENUM_IMPL(qemuMonitorQueryStatsName, + QEMU_MONITOR_QUERY_STATS_NAME_LAST, + "halt_poll_success_ns", + "halt_poll_fail_ns" +); + +VIR_ENUM_IMPL(qemuMonitorQueryStatsProvider, + QEMU_MONITOR_QUERY_STATS_PROVIDER_LAST, + "kvm", +); + +void +qemuMonitorQueryStatsProviderFree(qemuMonitorQueryStatsProvider *provider) +{ + virBitmapFree(provider->names); + g_free(provider); +} + +qemuMonitorQueryStatsProvider * +qemuMonitorQueryStatsProviderNew(qemuMonitorQueryStatsProviderType provider_type, + ...) +{ + g_autoptr(qemuMonitorQueryStatsProvider) provider = NULL; + qemuMonitorQueryStatsNameType stat; + va_list name_list; + size_t sentinel = QEMU_MONITOR_QUERY_STATS_NAME_LAST; + + provider = g_new0(qemuMonitorQueryStatsProvider, 1); + provider->provider = provider_type; + provider->names = NULL; + + va_start(name_list, provider_type); + stat = va_arg(name_list, qemuMonitorQueryStatsNameType); + + if (stat != sentinel) { + provider->names = virBitmapNew(QEMU_MONITOR_QUERY_STATS_NAME_LAST); +
It is very unlikely to happen to just get passed an empty set of names, and even in that case it would not cause any issues to have an empty virBitmap. I'd just allocate it in any case to make the code nicer. Also it would not crash in QueryStats when searching for the set bits.
+ while (stat != sentinel) { + if (virBitmapSetBit(provider->names, stat) < 0) + return NULL; + stat = va_arg(name_list, qemuMonitorQueryStatsNameType); + } + } + va_end(name_list); + + return g_steal_pointer(&provider); +} + diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 80696de7..4e228bfb 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -9031,3 +9031,133 @@ qemuMonitorJSONMigrateRecover(qemuMonitor *mon,
return qemuMonitorJSONCheckError(cmd, reply); } + +static GPtrArray * +qemuMonitorJSONExtractQueryStats(virJSONValue *arr) +{ + g_autoptr(GPtrArray) queried_stats = NULL; + size_t nstats = virJSONValueArraySize(arr); + size_t i; + + /* Create a GPtrArray for GHashTables */ + queried_stats = g_ptr_array_new_full(nstats, (GDestroyNotify) g_hash_table_destroy); + + for (i = 0; i < nstats; i++) { + virJSONValue *obj = virJSONValueArrayGet(arr, i); + virJSONValue *stats = virJSONValueObjectGetArray(obj, "stats"); + size_t j; + + /* Create a GHashTable for virJSONValues */ + GHashTable *hash_table = virHashNew((GDestroyNotify) virJSONValueFree); + + for (j = 0; j < virJSONValueArraySize(stats); j++) { + virJSONValue *stat = virJSONValueArrayGet(stats, j); + + g_hash_table_insert(hash_table, + g_strdup(virJSONValueObjectGetString(stat, "name")), + virJSONValueObjectGet(stat, "value"));
Similarly to the other checks we should also check these virJSONObjectGet* functions do not return NULL, just in case. The reasoning behind it is that we definitely do not want a (possibly exploited) qemu to be able to crash libvirt.
On Thu, Jul 14, 2022 at 11:22:02AM +0530, Amneesh Singh wrote:
Related: https://gitlab.com/libvirt/libvirt/-/issues/276
This patch adds an API for the "query-stats" QMP command.
The query returns a JSON containing the statistics based on the target, which can either be vCPU or VM, and the providers. The API deserializes the query result into an array of GHashMaps, which can later be used to extract all the query statistics. GHashMaps are used to avoid traversing the entire array to find the statistics you are looking for. This would be a singleton array if the target is a VM since the returned JSON is also a singleton array in that case.
Signed-off-by: Amneesh Singh <natto@weirdnatto.in> --- src/qemu/qemu_monitor.c | 70 +++++++++++++++++++ src/qemu/qemu_monitor.h | 45 ++++++++++++ src/qemu/qemu_monitor_json.c | 130 +++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 6 ++ 4 files changed, 251 insertions(+)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 80696de7..4e228bfb 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -9031,3 +9031,133 @@ qemuMonitorJSONMigrateRecover(qemuMonitor *mon,
return qemuMonitorJSONCheckError(cmd, reply); } + +static GPtrArray * +qemuMonitorJSONExtractQueryStats(virJSONValue *arr) +{ + g_autoptr(GPtrArray) queried_stats = NULL; + size_t nstats = virJSONValueArraySize(arr); + size_t i; + + /* Create a GPtrArray for GHashTables */ + queried_stats = g_ptr_array_new_full(nstats, (GDestroyNotify) g_hash_table_destroy); + + for (i = 0; i < nstats; i++) { + virJSONValue *obj = virJSONValueArrayGet(arr, i); + virJSONValue *stats = virJSONValueObjectGetArray(obj, "stats"); + size_t j; + + /* Create a GHashTable for virJSONValues */ + GHashTable *hash_table = virHashNew((GDestroyNotify) virJSONValueFree); + + for (j = 0; j < virJSONValueArraySize(stats); j++) { + virJSONValue *stat = virJSONValueArrayGet(stats, j); + + g_hash_table_insert(hash_table, + g_strdup(virJSONValueObjectGetString(stat, "name")), + virJSONValueObjectGet(stat, "value")); + } + + g_ptr_array_add(queried_stats, hash_table); + } + + return g_steal_pointer(&queried_stats); +} + + +/** + * qemuMonitorJSONQueryStats: + * @mon: monitor object + * @target: the target type for the query + * @vcpus: a list of vCPU QOM paths for filtering the statistics + * @providers: an array of providers to filter statistics + * + * @vcpus is a NULL terminated array of strings. @providers is a GPtrArray + * for qemuMonitorQueryStatsProvider. + * @vcpus and @providers are optional and can be NULL. + * + * Queries for the @target based statistics. + * Returns a GPtrArray of GHashTables containing the statistics on success or + * NULL on failure. + */ + +GPtrArray * +qemuMonitorJSONQueryStats(qemuMonitor *mon, + qemuMonitorQueryStatsTargetType target, + char **vcpus, + GPtrArray *providers) +{ + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; + g_autoptr(virJSONValue) vcpu_list = NULL; + g_autoptr(virJSONValue) provider_list = NULL; + virJSONValue *rv = NULL; + + size_t i; + + if (providers) { + provider_list = virJSONValueNewArray(); + + for (i = 0; i < providers->len; i++) { + g_autoptr(virJSONValue) provider_obj = virJSONValueNewObject(); + qemuMonitorQueryStatsProvider *provider = providers->pdata[i]; + const char *type_str = qemuMonitorQueryStatsProviderTypeToString(provider->provider); + virBitmap *names = provider->names; + int rc; + + rc = virJSONValueObjectAppendString(provider_obj, "provider", type_str); + + if (rc < 0) + return NULL; + + if (names) { + g_autoptr(virJSONValue) provider_names = virJSONValueNewArray(); + int curBit = virBitmapNextSetBit(names, -1); + + while (curBit >= 0) { + const char *name = qemuMonitorQueryStatsNameTypeToString(curBit); + + if (virJSONValueArrayAppendString(provider_names, name) < 0) + return NULL; + + curBit = virBitmapNextSetBit(names, curBit); + }
Also this can be shorter with: int curBit = -1; while ((curBit = virBitmapNextSetBit(names, curBit)) != -1) { ... }
Related: https://gitlab.com/libvirt/libvirt/-/issues/276 This patch adds "query-stats" to the QEMU capabilities. Signed-off-by: Amneesh Singh <natto@weirdnatto.in> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + 2 files changed, 3 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index fa8ffd19..9835dc82 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -672,6 +672,7 @@ VIR_ENUM_IMPL(virQEMUCaps, "display-dbus", /* QEMU_CAPS_DISPLAY_DBUS */ "iothread.thread-pool-max", /* QEMU_CAPS_IOTHREAD_THREAD_POOL_MAX */ "usb-host.guest-resets-all", /* QEMU_CAPS_USB_HOST_GUESTS_RESETS_ALL */ + "query-stats", /* QEMU_CAPS_QUERY_STATS */ ); @@ -1226,6 +1227,7 @@ struct virQEMUCapsStringFlags virQEMUCapsCommands[] = { { "query-dirty-rate", QEMU_CAPS_QUERY_DIRTY_RATE }, { "sev-inject-launch-secret", QEMU_CAPS_SEV_INJECT_LAUNCH_SECRET }, { "calc-dirty-rate", QEMU_CAPS_CALC_DIRTY_RATE }, + { "query-stats", QEMU_CAPS_QUERY_STATS }, }; struct virQEMUCapsStringFlags virQEMUCapsMigration[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index a409c127..62b28288 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -651,6 +651,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ QEMU_CAPS_DISPLAY_DBUS, /* -display dbus */ QEMU_CAPS_IOTHREAD_THREAD_POOL_MAX, /* -object iothread.thread-pool-max */ QEMU_CAPS_USB_HOST_GUESTS_RESETS_ALL, /* -device usb-host.guest-resets-all */ + QEMU_CAPS_QUERY_STATS, /* accepts query-stats */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; -- 2.36.1
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 | 69 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 61 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index da95f947..e1c595e5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18066,15 +18066,68 @@ qemuDomainGetStatsCpuCgroup(virDomainObj *dom, } static int -qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom, - virTypedParamList *params) +qemuDomainGetStatsCpuHaltPollTime(virQEMUDriver *driver, + virDomainObj *dom, + virTypedParamList *params, + unsigned int privflags) { unsigned long long haltPollSuccess = 0; unsigned long long haltPollFail = 0; - pid_t pid = dom->pid; + qemuDomainObjPrivate *priv = dom->privateData; + bool queryStatsCap = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_STATS); - if (virHostCPUGetHaltPollTime(pid, &haltPollSuccess, &haltPollFail) < 0) - return 0; + if (queryStatsCap && HAVE_JOB(privflags) && virDomainObjIsActive(dom)) { + 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; + provider = qemuMonitorQueryStatsProviderNew( + QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM, + QEMU_MONITOR_QUERY_STATS_NAME_HALT_POLL_SUCCESS_NS, + QEMU_MONITOR_QUERY_STATS_NAME_HALT_POLL_FAIL_NS, + QEMU_MONITOR_QUERY_STATS_NAME_LAST); + + providers = g_ptr_array_new_full(1, (GDestroyNotify) qemuMonitorQueryStatsProviderFree); + g_ptr_array_add(providers, provider); + + qemuDomainObjEnterMonitor(driver, dom); + queried_stats = qemuMonitorQueryStats(priv->mon, target, NULL, providers); + qemuDomainObjExitMonitor(dom); + + if (!queried_stats) + return 0; + + for (i = 0; i < queried_stats->len; i++) { + unsigned long long curHaltPollSuccess, curHaltPollFail; + GHashTable *cur_table = queried_stats->pdata[i]; + virJSONValue *success_obj, *fail_obj; + const char *success_str = qemuMonitorQueryStatsNameTypeToString( + QEMU_MONITOR_QUERY_STATS_NAME_HALT_POLL_SUCCESS_NS); + const char *fail_str = qemuMonitorQueryStatsNameTypeToString( + QEMU_MONITOR_QUERY_STATS_NAME_HALT_POLL_FAIL_NS); + + success_obj = g_hash_table_lookup(cur_table, success_str); + fail_obj = g_hash_table_lookup(cur_table, fail_str); + + if (!success_obj || !fail_obj) + return 0; + + if (virJSONValueGetNumberUlong(success_obj, &curHaltPollSuccess) < 0) + return 0; + + if (virJSONValueGetNumberUlong(fail_obj, &curHaltPollFail) < 0) + return 0; + + haltPollSuccess += curHaltPollSuccess; + haltPollFail += curHaltPollFail; + } + } else { + pid_t pid = dom->pid; + + if (virHostCPUGetHaltPollTime(pid, &haltPollSuccess, &haltPollFail) < 0) + return 0; + } if (virTypedParamListAddULLong(params, haltPollSuccess, "cpu.haltpoll.success.time") < 0 || virTypedParamListAddULLong(params, haltPollFail, "cpu.haltpoll.fail.time") < 0) @@ -18087,7 +18140,7 @@ static int qemuDomainGetStatsCpu(virQEMUDriver *driver, virDomainObj *dom, virTypedParamList *params, - unsigned int privflags G_GNUC_UNUSED) + unsigned int privflags) { if (qemuDomainGetStatsCpuCgroup(dom, params) < 0) return -1; @@ -18095,7 +18148,7 @@ qemuDomainGetStatsCpu(virQEMUDriver *driver, if (qemuDomainGetStatsCpuCache(driver, dom, params) < 0) return -1; - if (qemuDomainGetStatsCpuHaltPollTime(dom, params) < 0) + if (qemuDomainGetStatsCpuHaltPollTime(driver, dom, params, privflags) < 0) return -1; return 0; @@ -18929,7 +18982,7 @@ static virQEMUCapsFlags queryDirtyRateRequired[] = { static struct qemuDomainGetStatsWorker qemuDomainGetStatsWorkers[] = { { qemuDomainGetStatsState, VIR_DOMAIN_STATS_STATE, false, NULL }, - { qemuDomainGetStatsCpu, VIR_DOMAIN_STATS_CPU_TOTAL, false, NULL }, + { qemuDomainGetStatsCpu, VIR_DOMAIN_STATS_CPU_TOTAL, true, NULL }, { qemuDomainGetStatsBalloon, VIR_DOMAIN_STATS_BALLOON, true, NULL }, { qemuDomainGetStatsVcpu, VIR_DOMAIN_STATS_VCPU, true, NULL }, { qemuDomainGetStatsInterface, VIR_DOMAIN_STATS_INTERFACE, false, NULL }, -- 2.36.1
On Thu, Jul 14, 2022 at 11:22:04AM +0530, 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 | 69 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 61 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index da95f947..e1c595e5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18066,15 +18066,68 @@ qemuDomainGetStatsCpuCgroup(virDomainObj *dom, }
static int -qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom, - virTypedParamList *params) +qemuDomainGetStatsCpuHaltPollTime(virQEMUDriver *driver, + virDomainObj *dom, + virTypedParamList *params, + unsigned int privflags) { unsigned long long haltPollSuccess = 0; unsigned long long haltPollFail = 0; - pid_t pid = dom->pid; + qemuDomainObjPrivate *priv = dom->privateData; + bool queryStatsCap = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_STATS);
- if (virHostCPUGetHaltPollTime(pid, &haltPollSuccess, &haltPollFail) < 0) - return 0; + if (queryStatsCap && HAVE_JOB(privflags) && virDomainObjIsActive(dom)) { + 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; + provider = qemuMonitorQueryStatsProviderNew( + QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM, + QEMU_MONITOR_QUERY_STATS_NAME_HALT_POLL_SUCCESS_NS, + QEMU_MONITOR_QUERY_STATS_NAME_HALT_POLL_FAIL_NS, + QEMU_MONITOR_QUERY_STATS_NAME_LAST); + + providers = g_ptr_array_new_full(1, (GDestroyNotify) qemuMonitorQueryStatsProviderFree); + g_ptr_array_add(providers, provider); + + qemuDomainObjEnterMonitor(driver, dom); + queried_stats = qemuMonitorQueryStats(priv->mon, target, NULL, providers); + qemuDomainObjExitMonitor(dom); + + if (!queried_stats) + return 0; + + for (i = 0; i < queried_stats->len; i++) { + unsigned long long curHaltPollSuccess, curHaltPollFail; + GHashTable *cur_table = queried_stats->pdata[i]; + virJSONValue *success_obj, *fail_obj; + const char *success_str = qemuMonitorQueryStatsNameTypeToString( + QEMU_MONITOR_QUERY_STATS_NAME_HALT_POLL_SUCCESS_NS); + const char *fail_str = qemuMonitorQueryStatsNameTypeToString( + QEMU_MONITOR_QUERY_STATS_NAME_HALT_POLL_FAIL_NS); + + success_obj = g_hash_table_lookup(cur_table, success_str); + fail_obj = g_hash_table_lookup(cur_table, fail_str); + + if (!success_obj || !fail_obj) + return 0; + + if (virJSONValueGetNumberUlong(success_obj, &curHaltPollSuccess) < 0) + return 0; + + if (virJSONValueGetNumberUlong(fail_obj, &curHaltPollFail) < 0) + return 0; +
As mentioned before, all these failures do not have to exit the function, but rather fallback to the old way. You can even create two new functions for the new and old implementations and then call them from here to make the fallback easier to spot (and code). I wanted to change this before pushing as well, but I feel like I'm changing too much of your code. And I also had to rebase this (although trivially). Would you mind just changing these few last things so that we can get it in before the rc0 freeze? Thanks and have a nice weekend, Martin
+ haltPollSuccess += curHaltPollSuccess; + haltPollFail += curHaltPollFail; + } + } else { + pid_t pid = dom->pid; + + if (virHostCPUGetHaltPollTime(pid, &haltPollSuccess, &haltPollFail) < 0) + return 0; + }
if (virTypedParamListAddULLong(params, haltPollSuccess, "cpu.haltpoll.success.time") < 0 || virTypedParamListAddULLong(params, haltPollFail, "cpu.haltpoll.fail.time") < 0) @@ -18087,7 +18140,7 @@ static int qemuDomainGetStatsCpu(virQEMUDriver *driver, virDomainObj *dom, virTypedParamList *params, - unsigned int privflags G_GNUC_UNUSED) + unsigned int privflags) { if (qemuDomainGetStatsCpuCgroup(dom, params) < 0) return -1; @@ -18095,7 +18148,7 @@ qemuDomainGetStatsCpu(virQEMUDriver *driver, if (qemuDomainGetStatsCpuCache(driver, dom, params) < 0) return -1;
- if (qemuDomainGetStatsCpuHaltPollTime(dom, params) < 0) + if (qemuDomainGetStatsCpuHaltPollTime(driver, dom, params, privflags) < 0) return -1;
return 0; @@ -18929,7 +18982,7 @@ static virQEMUCapsFlags queryDirtyRateRequired[] = {
static struct qemuDomainGetStatsWorker qemuDomainGetStatsWorkers[] = { { qemuDomainGetStatsState, VIR_DOMAIN_STATS_STATE, false, NULL }, - { qemuDomainGetStatsCpu, VIR_DOMAIN_STATS_CPU_TOTAL, false, NULL }, + { qemuDomainGetStatsCpu, VIR_DOMAIN_STATS_CPU_TOTAL, true, NULL }, { qemuDomainGetStatsBalloon, VIR_DOMAIN_STATS_BALLOON, true, NULL }, { qemuDomainGetStatsVcpu, VIR_DOMAIN_STATS_VCPU, true, NULL }, { qemuDomainGetStatsInterface, VIR_DOMAIN_STATS_INTERFACE, false, NULL }, -- 2.36.1
On 7/22/22 17:43, Martin Kletzander wrote:
As mentioned before, all these failures do not have to exit the function, but rather fallback to the old way. You can even create two new functions for the new and old implementations and then call them from here to make the fallback easier to spot (and code).
More precisely, they should just "continue;" to the next iteration of the for loop, like if (!success_obj || !fail_obj) continue; found = true; and then go fall back if found is false at the end of the loop. On the other hand, here: if (virJSONValueGetNumberUlong(success_obj, &curHaltPollSuccess) < 0) return 0; if (virJSONValueGetNumberUlong(fail_obj, &curHaltPollFail) < 0) return 0; I am not sure about falling back, because it is really an unexpected situation where the statistic exist but somehow the value cannot be parsed. Paolo
I wanted to change this before pushing as well, but I feel like I'm changing too much of your code. And I also had to rebase this (although trivially). Would you mind just changing these few last things so that we can get it in before the rc0 freeze?
On Fri, Jul 22, 2022 at 07:09:57PM +0200, Paolo Bonzini wrote:
On 7/22/22 17:43, Martin Kletzander wrote:
As mentioned before, all these failures do not have to exit the function, but rather fallback to the old way. You can even create two new functions for the new and old implementations and then call them from here to make the fallback easier to spot (and code).
More precisely, they should just "continue;" to the next iteration of the for loop, like
if (!success_obj || !fail_obj) continue;
found = true;
and then go fall back if found is false at the end of the loop.
On the other hand, here:
if (virJSONValueGetNumberUlong(success_obj, &curHaltPollSuccess) < 0) return 0;
if (virJSONValueGetNumberUlong(fail_obj, &curHaltPollFail) < 0) return 0;
I am not sure about falling back, because it is really an unexpected situation where the statistic exist but somehow the value cannot be parsed. Then can we just "continue;" in case the value fails to parse as well?
Paolo
I wanted to change this before pushing as well, but I feel like I'm changing too much of your code. And I also had to rebase this (although trivially). Would you mind just changing these few last things so that we can get it in before the rc0 freeze? Alright, as soon as there is a viable check decided for the virJSONValueGet* statements above, I will push a v4 with the changes you mentioned in your reviews. Thank you both for taking the time to review.
On Sun, Jul 24, 2022 at 10:50:36AM +0530, Amneesh Singh wrote:
On Fri, Jul 22, 2022 at 07:09:57PM +0200, Paolo Bonzini wrote:
On 7/22/22 17:43, Martin Kletzander wrote:
As mentioned before, all these failures do not have to exit the function, but rather fallback to the old way. You can even create two new functions for the new and old implementations and then call them from here to make the fallback easier to spot (and code).
More precisely, they should just "continue;" to the next iteration of the for loop, like
if (!success_obj || !fail_obj) continue;
found = true;
and then go fall back if found is false at the end of the loop.
On the other hand, here:
if (virJSONValueGetNumberUlong(success_obj, &curHaltPollSuccess) < 0) return 0;
if (virJSONValueGetNumberUlong(fail_obj, &curHaltPollFail) < 0) return 0;
I am not sure about falling back, because it is really an unexpected situation where the statistic exist but somehow the value cannot be parsed.
The main thing I was worried about was libvirtd crashing on possibly expoited qemu process. Whether we fall back or report nothing in this case is not that big of a deal, since there's something wrong already anyway.
Then can we just "continue;" in case the value fails to parse as well?
That's another option as well.
Paolo
I wanted to change this before pushing as well, but I feel like I'm changing too much of your code. And I also had to rebase this (although trivially). Would you mind just changing these few last things so that we can get it in before the rc0 freeze? Alright, as soon as there is a viable check decided for the virJSONValueGet* statements above, I will push a v4 with the changes you mentioned in your reviews. Thank you both for taking the time to review.
participants (4)
-
Amneesh Singh -
Kristina Hanicova -
Martin Kletzander -
Paolo Bonzini