12:52 a.m.
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
12:52 a.m.
New subject: [PATCH 1/3] qemu_monitor: add qemuMonitorQueryStats
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(a)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
7:44 a.m.
New subject: [PATCH 1/3] qemu_monitor: add qemuMonitorQueryStats
On Thu, Jul 14, 2022 at 7:54 AM Amneesh Singh <natto(a)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(a)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
9:32 a.m.
New subject: [PATCH 1/3] qemu_monitor: add qemuMonitorQueryStats
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(a)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(a)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.
10:02 a.m.
New subject: [PATCH 1/3] qemu_monitor: add qemuMonitorQueryStats
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(a)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(a)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
12:03 a.m.
New subject: [PATCH 1/3] qemu_monitor: add qemuMonitorQueryStats
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(a)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(a)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
9:26 a.m.
New subject: [PATCH 1/3] qemu_monitor: add qemuMonitorQueryStats
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(a)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.
9:43 a.m.
New subject: [PATCH 1/3] qemu_monitor: add qemuMonitorQueryStats
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(a)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) {
...
}
12:52 a.m.
New subject: [PATCH 2/3] qemu_capabilities: add "query-stats" QMP command to the QEMU capabilities
Related: https://gitlab.com/libvirt/libvirt/-/issues/276
This patch adds "query-stats" to the QEMU capabilities.
Signed-off-by: Amneesh Singh <natto(a)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
12:52 a.m.
New subject: [PATCH 3/3] qemu_driver: use qemuMonitorQueryStats to extract halt poll time
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 | 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
10:43 a.m.
New subject: [PATCH 3/3] qemu_driver: use qemuMonitorQueryStats to extract halt poll time
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(a)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
12:09 p.m.
New subject: [PATCH 3/3] qemu_driver: use qemuMonitorQueryStats to extract halt poll time
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?
12:20 a.m.
New subject: [PATCH 3/3] qemu_driver: use qemuMonitorQueryStats to extract halt poll time
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.
5:44 a.m.
New subject: [PATCH 3/3] qemu_driver: use qemuMonitorQueryStats to extract halt poll time
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.
>
849
days inactive
861
days old
13 comments
4 participants
participants (4)
-
Amneesh Singh -
Kristina Hanicova -
Martin Kletzander -
Paolo Bonzini