3:14 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.
Relevant QEMU patches can be found here:
https://lore.kernel.org/all/20220523150509.349412-1-pbonzini@redhat.com/
This patchset is part of the 2022 GSOC contributor project. All reviews
and comments are appreciated :).
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 | 48 +++++++++++--
src/qemu/qemu_monitor.c | 46 +++++++++++++
src/qemu/qemu_monitor.h | 36 ++++++++++
src/qemu/qemu_monitor_json.c | 128 +++++++++++++++++++++++++++++++++++
src/qemu/qemu_monitor_json.h | 6 ++
7 files changed, 263 insertions(+), 4 deletions(-)
--
2.36.1
3:14 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 | 46 +++++++++++++
src/qemu/qemu_monitor.h | 36 ++++++++++
src/qemu/qemu_monitor_json.c | 128 +++++++++++++++++++++++++++++++++++
src/qemu/qemu_monitor_json.h | 6 ++
4 files changed, 216 insertions(+)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index fda5d2f3..bedb5308 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -4541,3 +4541,49 @@ qemuMonitorMigrateRecover(qemuMonitor *mon,
return qemuMonitorJSONMigrateRecover(mon, uri);
}
+
+VIR_ENUM_IMPL(qemuMonitorQueryStatsTarget,
+ QEMU_MONITOR_QUERY_STATS_TARGET_LAST,
+ "vm",
+ "vcpu",
+);
+
+VIR_ENUM_IMPL(qemuMonitorQueryStatsProvider,
+ QEMU_MONITOR_QUERY_STATS_PROVIDER_LAST,
+ "kvm",
+);
+
+void
+qemuMonitorQueryStatsProviderFree(qemuMonitorQueryStatsProvider *provider)
+{
+ g_strfreev(provider->names);
+ g_free(provider);
+}
+
+qemuMonitorQueryStatsProvider *
+qemuMonitorQueryStatsProviderNew(qemuMonitorQueryStatsProviderType provider_type)
+{
+ g_autoptr(qemuMonitorQueryStatsProvider) provider = NULL;
+
+ provider = g_new0(qemuMonitorQueryStatsProvider, 1);
+ provider->provider = provider_type;
+ provider->names = NULL;
+
+ 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..ddfdb526 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -1554,3 +1554,39 @@ 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_PROVIDER_KVM,
+ QEMU_MONITOR_QUERY_STATS_PROVIDER_LAST,
+} qemuMonitorQueryStatsProviderType;
+
+VIR_ENUM_DECL(qemuMonitorQueryStatsProvider);
+
+typedef struct _qemuMonitorQueryStatsProvider qemuMonitorQueryStatsProvider;
+struct _qemuMonitorQueryStatsProvider {
+ qemuMonitorQueryStatsProviderType provider;
+ GStrv 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 3aad2ab2..d6757042 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -9006,3 +9006,131 @@ 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++) {
+ int rc;
+ g_autoptr(virJSONValue) provider_obj = virJSONValueNewObject();
+ qemuMonitorQueryStatsProvider *provider = providers->pdata[i];
+ const char *type_str =
qemuMonitorQueryStatsProviderTypeToString(provider->provider);
+
+ rc = virJSONValueObjectAppendString(provider_obj, "provider",
type_str);
+
+ if (rc < 0)
+ return NULL;
+
+ if (provider->names) {
+ g_autoptr(virJSONValue) providerNames = virJSONValueNewArray();
+ size_t j = 0;
+ const char *name = provider->names[j];
+
+ while (name) {
+ if (virJSONValueArrayAppendString(providerNames, name) < 0)
+ return NULL;
+
+ name = provider->names[++j];
+ }
+
+ rc = virJSONValueObjectAppend(provider_obj, "names",
&providerNames);
+
+ 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
10:23 a.m.
New subject: [PATCH 1/3] qemu_monitor: add qemuMonitorQueryStats
On 6/24/22 10:14, 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 | 46 +++++++++++++
src/qemu/qemu_monitor.h | 36 ++++++++++
src/qemu/qemu_monitor_json.c | 128 +++++++++++++++++++++++++++++++++++
src/qemu/qemu_monitor_json.h | 6 ++
4 files changed, 216 insertions(+)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index fda5d2f3..bedb5308 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -4541,3 +4541,49 @@ qemuMonitorMigrateRecover(qemuMonitor *mon,
return qemuMonitorJSONMigrateRecover(mon, uri);
}
+
+VIR_ENUM_IMPL(qemuMonitorQueryStatsTarget,
+ QEMU_MONITOR_QUERY_STATS_TARGET_LAST,
+ "vm",
+ "vcpu",
+);
+
+VIR_ENUM_IMPL(qemuMonitorQueryStatsProvider,
+ QEMU_MONITOR_QUERY_STATS_PROVIDER_LAST,
+ "kvm",
+);
+
+void
+qemuMonitorQueryStatsProviderFree(qemuMonitorQueryStatsProvider *provider)
+{
+ g_strfreev(provider->names);
+ g_free(provider);
+}
+
+qemuMonitorQueryStatsProvider *
+qemuMonitorQueryStatsProviderNew(qemuMonitorQueryStatsProviderType provider_type)
+{
+ g_autoptr(qemuMonitorQueryStatsProvider) provider = NULL;
+
+ provider = g_new0(qemuMonitorQueryStatsProvider, 1);
+ provider->provider = provider_type;
+ provider->names = NULL;
+
+ 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..ddfdb526 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -1554,3 +1554,39 @@ 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_PROVIDER_KVM,
+ QEMU_MONITOR_QUERY_STATS_PROVIDER_LAST,
+} qemuMonitorQueryStatsProviderType;
+
+VIR_ENUM_DECL(qemuMonitorQueryStatsProvider);
+
+typedef struct _qemuMonitorQueryStatsProvider qemuMonitorQueryStatsProvider;
+struct _qemuMonitorQueryStatsProvider {
+ qemuMonitorQueryStatsProviderType provider;
+ GStrv 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 3aad2ab2..d6757042 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -9006,3 +9006,131 @@ 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++) {
+ int rc;
+ g_autoptr(virJSONValue) provider_obj = virJSONValueNewObject();
+ qemuMonitorQueryStatsProvider *provider = providers->pdata[i];
+ const char *type_str =
qemuMonitorQueryStatsProviderTypeToString(provider->provider);
Nit pick, I think this block would look a bit better if @rc was declared
at its end.
+
+ rc = virJSONValueObjectAppendString(provider_obj, "provider",
type_str);
+
+ if (rc < 0)
+ return NULL;
+
+ if (provider->names) {
+ g_autoptr(virJSONValue) providerNames = virJSONValueNewArray();
+ size_t j = 0;
+ const char *name = provider->names[j];
Another nitpick: here I'd prefer names[0] instead if names[j]. While it
may not matter now, @j and @name are declared next to each other, that
may not be always the case and I have to remember one thing less when
reading the code.
+
+ while (name) {
+ if (virJSONValueArrayAppendString(providerNames, name) < 0)
+ return NULL;
+
+ name = provider->names[++j];
+ }
+
+ rc = virJSONValueObjectAppend(provider_obj, "names",
&providerNames);
+
+ if (rc < 0)
+ return NULL;
+ }
Misleading alignment.
+
+ 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);
+}
Michal
3:14 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 2c3be3ec..81fc597a 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -671,6 +671,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
"chardev.qemu-vdagent", /* QEMU_CAPS_CHARDEV_QEMU_VDAGENT */
"display-dbus", /* QEMU_CAPS_DISPLAY_DBUS */
"iothread.thread-pool-max", /* QEMU_CAPS_IOTHREAD_THREAD_POOL_MAX
*/
+ "query-stats", /* QEMU_CAPS_QUERY_STATS */
);
@@ -1225,6 +1226,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 6f35ba14..29e55b3a 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -650,6 +650,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check
*/
QEMU_CAPS_CHARDEV_QEMU_VDAGENT, /* -chardev qemu-vdagent */
QEMU_CAPS_DISPLAY_DBUS, /* -display dbus */
QEMU_CAPS_IOTHREAD_THREAD_POOL_MAX, /* -object iothread.thread-pool-max */
+ QEMU_CAPS_QUERY_STATS, /*accepts query-stats */
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
--
2.36.1
3:14 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 | 48 ++++++++++++++++++++++++++++++++++++++----
1 file changed, 44 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 3b5c3db6..0a2be6d3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18057,10 +18057,50 @@ qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom,
{
unsigned long long haltPollSuccess = 0;
unsigned long long haltPollFail = 0;
- pid_t pid = dom->pid;
+ qemuDomainObjPrivate *priv = dom->privateData;
+ bool canQueryStats = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_STATS);
- if (virHostCPUGetHaltPollTime(pid, &haltPollSuccess, &haltPollFail) < 0)
- return 0;
+ if (!canQueryStats) {
+ pid_t pid = dom->pid;
+
+ if (virHostCPUGetHaltPollTime(pid, &haltPollSuccess, &haltPollFail) <
0)
+ return 0;
+ } else {
+ size_t i;
+ qemuMonitorQueryStatsTargetType target = QEMU_MONITOR_QUERY_STATS_TARGET_VCPU;
+ qemuMonitorQueryStatsProvider *provider = NULL;
+ g_autoptr(GPtrArray) providers = NULL;
+ g_autoptr(GPtrArray) queried_stats = NULL;
+ const char *success_str = "halt_poll_success_ns";
+ const char *fail_str = "halt_poll_fail_ns";
+
+ provider =
qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM);
+ provider->names = g_new0(char *, 3);
+ provider->names[0] = g_strdup(success_str), provider->names[1] =
g_strdup(fail_str);
+
+ providers = g_ptr_array_new_full(1, (GDestroyNotify)
qemuMonitorQueryStatsProviderFree);
+ g_ptr_array_add(providers, provider);
+
+ queried_stats = qemuMonitorQueryStats(priv->mon, target, NULL, providers);
+
+ 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, *fail;
+
+ success = g_hash_table_lookup(cur_table, success_str);
+ fail = g_hash_table_lookup(cur_table, fail_str);
+
+ ignore_value(virJSONValueGetNumberUlong(success, &curHaltPollSuccess));
+ ignore_value(virJSONValueGetNumberUlong(fail, &curHaltPollFail));
+
+ haltPollSuccess += curHaltPollSuccess;
+ haltPollFail += curHaltPollFail;
+ }
+ }
if (virTypedParamListAddULLong(params, haltPollSuccess,
"cpu.haltpoll.success.time") < 0 ||
virTypedParamListAddULLong(params, haltPollFail,
"cpu.haltpoll.fail.time") < 0)
@@ -18915,7 +18955,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:23 a.m.
New subject: [PATCH 3/3] qemu_driver: use qemuMonitorQueryStats to extract halt poll time
On 6/24/22 10:14, Amneesh Singh wrote:
Related: https://gitlab.com/libvirt/libvirt/-/issues/276
This patch uses qemuMonitorQueryStats to query "halt_poll_success_ns"
and "halt_poll_fail_ns" for every vCPU. The respective values for each
vCPU are then added together.
Signed-off-by: Amneesh Singh <natto(a)weirdnatto.in>
---
src/qemu/qemu_driver.c | 48 ++++++++++++++++++++++++++++++++++++++----
1 file changed, 44 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 3b5c3db6..0a2be6d3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18057,10 +18057,50 @@ qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom,
{
unsigned long long haltPollSuccess = 0;
unsigned long long haltPollFail = 0;
- pid_t pid = dom->pid;
+ qemuDomainObjPrivate *priv = dom->privateData;
+ bool canQueryStats = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_STATS);
Is this variable really needed? I mean, we can just:
if (virQEMUCapsGet(...) {
} else {
}
But if you want to avoid long lines, then perhaps rename the variable to
queryStatsCap? This way it's more obvious what the variable reflects.
Stats can be queried in both cases ;-)
- if (virHostCPUGetHaltPollTime(pid, &haltPollSuccess, &haltPollFail) < 0)
- return 0;
+ if (!canQueryStats) {
+ pid_t pid = dom->pid;
+
+ if (virHostCPUGetHaltPollTime(pid, &haltPollSuccess, &haltPollFail) <
0)
+ return 0;
+ } else {
+ size_t i;
+ qemuMonitorQueryStatsTargetType target = QEMU_MONITOR_QUERY_STATS_TARGET_VCPU;
+ qemuMonitorQueryStatsProvider *provider = NULL;
+ g_autoptr(GPtrArray) providers = NULL;
+ g_autoptr(GPtrArray) queried_stats = NULL;
+ const char *success_str = "halt_poll_success_ns";
+ const char *fail_str = "halt_poll_fail_ns";
+
+ provider =
qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM);
+ provider->names = g_new0(char *, 3);
+ provider->names[0] = g_strdup(success_str), provider->names[1] =
g_strdup(fail_str);
I'm starting to wonder whether this is a good interface. These ->names[]
array is never changed, so maybe we can have it as a NULL terminated
list of constant strings? For instance:
provider = qemuMonitorQueryStatsProviderNew();
provider->names = {"halt_poll_success_ns", "halt_poll_fail_ns",
NULL};
+
+ providers = g_ptr_array_new_full(1, (GDestroyNotify)
qemuMonitorQueryStatsProviderFree);
+ g_ptr_array_add(providers, provider);
+
+ queried_stats = qemuMonitorQueryStats(priv->mon, target, NULL, providers);
This issues monitor command without proper checks. Firstly, you need to
check whether job acquiring (that s/false/true/ change done in the last
hunk) was successful and the domain is still running:
if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom)) {
/* code here */
}
Then, you need to so called "enter the monitor" and "exit the
monitor".
So what happens here is that @vm is when this function is called. Now,
issuing a monitor command with the domain object lock held is
potentially very dangerous because if QEMU takes long time to reply (or
it doesn't reply at all) the domain object is locked an can't be
destroyed (virsh destroy) - because the first thing that
qemuDomainDestroyFlags() does is locking the domain object. Also, you
want to make sure no other thread is currently talking on the monitor -
so the monitor has to be locked too. We have two convenient functions
for these operations:
qemuDomainObjEnterMonitor()
qemuDomainObjExitMonitor()
This is all described in more detail in
docs/kbase/internals/qemu-threads.rst.
Having said all of this, I think we can fallback to the current behavior
if job wasn't acquired successfully. Therefore the condition at the very
beginning might look like this:
if (queryStatsCap && HAVE_JOB() && virDomainObjIsActive()) {
...
qemuDomainEnterMonitor();
qemuMonitorQueryStats();
qemuDomainExitMonitor();
} else {
virHostCPUGetHaltPollTime();
}
+
+ 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, *fail;
+
+ success = g_hash_table_lookup(cur_table, success_str);
+ fail = g_hash_table_lookup(cur_table, fail_str);
+
+ ignore_value(virJSONValueGetNumberUlong(success, &curHaltPollSuccess));
+ ignore_value(virJSONValueGetNumberUlong(fail, &curHaltPollFail));
I don't think this is a safe thing to do. What if either of @success or
@fail is NULL? That can happen when QEMU didn't return requested member.
True, at that point the 'query-stats' command should have failed, but I
think it's more defensive if we checked these pointers. My worry is that
virJSONValueGetNumberUlong() dereferences the pointer right away.
+
+ haltPollSuccess += curHaltPollSuccess;
+ haltPollFail += curHaltPollFail;
+ }
+ }
if (virTypedParamListAddULLong(params, haltPollSuccess,
"cpu.haltpoll.success.time") < 0 ||
virTypedParamListAddULLong(params, haltPollFail,
"cpu.haltpoll.fail.time") < 0)
@@ -18915,7 +18955,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 },
Please feel free to object to anything I wrote. Maybe you have more
patches written on a local branch that justify your choices. I don't know.
Michal
11:45 a.m.
New subject: [PATCH 3/3] qemu_driver: use qemuMonitorQueryStats to extract halt poll time
On Tue, Jun 28, 2022 at 05:23:11PM +0200, Michal Prívozník wrote:
On 6/24/22 10:14, Amneesh Singh wrote:
> Related: https://gitlab.com/libvirt/libvirt/-/issues/276
>
> This patch uses qemuMonitorQueryStats to query "halt_poll_success_ns"
> and "halt_poll_fail_ns" for every vCPU. The respective values for each
> vCPU are then added together.
>
> Signed-off-by: Amneesh Singh <natto(a)weirdnatto.in>
> ---
> src/qemu/qemu_driver.c | 48 ++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 44 insertions(+), 4 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 3b5c3db6..0a2be6d3 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -18057,10 +18057,50 @@ qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom,
> {
> unsigned long long haltPollSuccess = 0;
> unsigned long long haltPollFail = 0;
> - pid_t pid = dom->pid;
> + qemuDomainObjPrivate *priv = dom->privateData;
> + bool canQueryStats = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_STATS);
Is this variable really needed? I mean, we can just:
if (virQEMUCapsGet(...) {
} else {
}
But if you want to avoid long lines, then perhaps rename the variable to
queryStatsCap? This way it's more obvious what the variable reflects.
Stats can be queried in both cases ;-)
Sure, that sounds doable :)
>
> - if (virHostCPUGetHaltPollTime(pid, &haltPollSuccess, &haltPollFail)
< 0)
> - return 0;
> + if (!canQueryStats) {
> + pid_t pid = dom->pid;
> +
> + if (virHostCPUGetHaltPollTime(pid, &haltPollSuccess, &haltPollFail)
< 0)
> + return 0;
> + } else {
> + size_t i;
> + qemuMonitorQueryStatsTargetType target =
QEMU_MONITOR_QUERY_STATS_TARGET_VCPU;
> + qemuMonitorQueryStatsProvider *provider = NULL;
> + g_autoptr(GPtrArray) providers = NULL;
> + g_autoptr(GPtrArray) queried_stats = NULL;
> + const char *success_str = "halt_poll_success_ns";
> + const char *fail_str = "halt_poll_fail_ns";
> +
> + provider =
qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM);
> + provider->names = g_new0(char *, 3);
> + provider->names[0] = g_strdup(success_str), provider->names[1] =
g_strdup(fail_str);
I'm starting to wonder whether this is a good interface. These ->names[]
array is never changed, so maybe we can have it as a NULL terminated
list of constant strings? For instance:
provider = qemuMonitorQueryStatsProviderNew();
provider->names = {"halt_poll_success_ns", "halt_poll_fail_ns",
NULL};
Well, cannot really assign char ** with a scalar initialization, but
what might work is something like
const char *names[] = { success_str, fail_str, NULL };
provider = qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM);
provider->names = g_strdupv((char **) names);
Another thought was using GStrvBuilder but it is not avaiable as per
GLIB_VERSION_MAX.
I too think that the current approach is not very nice. A variadic
function similar to g_strv_builder_add_many that initializes a
char ** would be nice.
> +
> + providers = g_ptr_array_new_full(1, (GDestroyNotify)
qemuMonitorQueryStatsProviderFree);
> + g_ptr_array_add(providers, provider);
> +
> + queried_stats = qemuMonitorQueryStats(priv->mon, target, NULL,
providers);
This issues monitor command without proper checks. Firstly, you need to
check whether job acquiring (that s/false/true/ change done in the last
hunk) was successful and the domain is still running:
if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom)) {
/* code here */
}
Then, you need to so called "enter the monitor" and "exit the
monitor".
So what happens here is that @vm is when this function is called. Now,
issuing a monitor command with the domain object lock held is
potentially very dangerous because if QEMU takes long time to reply (or
it doesn't reply at all) the domain object is locked an can't be
destroyed (virsh destroy) - because the first thing that
qemuDomainDestroyFlags() does is locking the domain object. Also, you
want to make sure no other thread is currently talking on the monitor -
so the monitor has to be locked too. We have two convenient functions
for these operations:
qemuDomainObjEnterMonitor()
qemuDomainObjExitMonitor()
This is all described in more detail in
docs/kbase/internals/qemu-threads.rst.
Having said all of this, I think we can fallback to the current behavior
if job wasn't acquired successfully. Therefore the condition at the very
beginning might look like this:
if (queryStatsCap && HAVE_JOB() && virDomainObjIsActive()) {
...
qemuDomainEnterMonitor();
qemuMonitorQueryStats();
qemuDomainExitMonitor();
} else {
virHostCPUGetHaltPollTime();
}
I see that makes sense. I shall do the necessary changes. Thanks for the
detailed explanation.
> +
> + 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, *fail;
> +
> + success = g_hash_table_lookup(cur_table, success_str);
> + fail = g_hash_table_lookup(cur_table, fail_str);
> +
> + ignore_value(virJSONValueGetNumberUlong(success,
&curHaltPollSuccess));
> + ignore_value(virJSONValueGetNumberUlong(fail, &curHaltPollFail));
I don't think this is a safe thing to do. What if either of @success or
@fail is NULL? That can happen when QEMU didn't return requested member.
True, at that point the 'query-stats' command should have failed, but I
think it's more defensive if we checked these pointers. My worry is that
virJSONValueGetNumberUlong() dereferences the pointer right away.
I do understand that this is not an unfounded worry. I shall put the
checks there.
> +
> + haltPollSuccess += curHaltPollSuccess;
> + haltPollFail += curHaltPollFail;
> + }
> + }
>
> if (virTypedParamListAddULLong(params, haltPollSuccess,
"cpu.haltpoll.success.time") < 0 ||
> virTypedParamListAddULLong(params, haltPollFail,
"cpu.haltpoll.fail.time") < 0)
> @@ -18915,7 +18955,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 },
Please feel free to object to anything I wrote. Maybe you have more
patches written on a local branch that justify your choices. I don't know.
Michal
Thanks for taking the time to review the patches.
Amneesh
3:25 p.m.
New subject: [PATCH 3/3] qemu_driver: use qemuMonitorQueryStats to extract halt poll time
On Tue, Jun 28, 2022 at 10:15:28PM +0530, Amneesh Singh wrote:
On Tue, Jun 28, 2022 at 05:23:11PM +0200, Michal Prívozník wrote:
> On 6/24/22 10:14, Amneesh Singh wrote:
> > Related: https://gitlab.com/libvirt/libvirt/-/issues/276
> >
> > This patch uses qemuMonitorQueryStats to query "halt_poll_success_ns"
> > and "halt_poll_fail_ns" for every vCPU. The respective values for
each
> > vCPU are then added together.
> >
> > Signed-off-by: Amneesh Singh <natto(a)weirdnatto.in>
> > ---
> > src/qemu/qemu_driver.c | 48 ++++++++++++++++++++++++++++++++++++++----
> > 1 file changed, 44 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 3b5c3db6..0a2be6d3 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -18057,10 +18057,50 @@ qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom,
> > {
> > unsigned long long haltPollSuccess = 0;
> > unsigned long long haltPollFail = 0;
> > - pid_t pid = dom->pid;
> > + qemuDomainObjPrivate *priv = dom->privateData;
> > + bool canQueryStats = virQEMUCapsGet(priv->qemuCaps,
QEMU_CAPS_QUERY_STATS);
>
> Is this variable really needed? I mean, we can just:
>
> if (virQEMUCapsGet(...) {
>
> } else {
>
> }
>
> But if you want to avoid long lines, then perhaps rename the variable to
> queryStatsCap? This way it's more obvious what the variable reflects.
> Stats can be queried in both cases ;-)
Sure, that sounds doable :)
>
> >
> > - if (virHostCPUGetHaltPollTime(pid, &haltPollSuccess, &haltPollFail)
< 0)
> > - return 0;
> > + if (!canQueryStats) {
> > + pid_t pid = dom->pid;
> > +
> > + if (virHostCPUGetHaltPollTime(pid, &haltPollSuccess,
&haltPollFail) < 0)
> > + return 0;
> > + } else {
> > + size_t i;
> > + qemuMonitorQueryStatsTargetType target =
QEMU_MONITOR_QUERY_STATS_TARGET_VCPU;
> > + qemuMonitorQueryStatsProvider *provider = NULL;
> > + g_autoptr(GPtrArray) providers = NULL;
> > + g_autoptr(GPtrArray) queried_stats = NULL;
> > + const char *success_str = "halt_poll_success_ns";
> > + const char *fail_str = "halt_poll_fail_ns";
> > +
> > + provider =
qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM);
> > + provider->names = g_new0(char *, 3);
> > + provider->names[0] = g_strdup(success_str), provider->names[1] =
g_strdup(fail_str);
>
> I'm starting to wonder whether this is a good interface. These ->names[]
> array is never changed, so maybe we can have it as a NULL terminated
> list of constant strings? For instance:
>
> provider = qemuMonitorQueryStatsProviderNew();
> provider->names = {"halt_poll_success_ns",
"halt_poll_fail_ns", NULL};
Well, cannot really assign char ** with a scalar initialization, but
what might work is something like
const char *names[] = { success_str, fail_str, NULL };
provider = qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM);
provider->names = g_strdupv((char **) names);
I think what Michal was trying to say is that since you do not change
the array anywhere, there is no need for that to be a dynamically
allocated array that needs to be freed. I, however, am not 100% if you
are going to need this to be dynamic and whether you will be changing
these arrays.
Another thought was using GStrvBuilder but it is not avaiable as per
GLIB_VERSION_MAX.
I too think that the current approach is not very nice. A variadic
function similar to g_strv_builder_add_many that initializes a
char ** would be nice.
>
If that is something that helps, then we can write it ourselves and only
use our implementation if glib is too old.
> > +
> > + providers = g_ptr_array_new_full(1, (GDestroyNotify)
qemuMonitorQueryStatsProviderFree);
> > + g_ptr_array_add(providers, provider);
> > +
> > + queried_stats = qemuMonitorQueryStats(priv->mon, target, NULL,
providers);
>
> This issues monitor command without proper checks. Firstly, you need to
> check whether job acquiring (that s/false/true/ change done in the last
> hunk) was successful and the domain is still running:
>
> if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom)) {
> /* code here */
> }
>
> Then, you need to so called "enter the monitor" and "exit the
monitor".
> So what happens here is that @vm is when this function is called. Now,
> issuing a monitor command with the domain object lock held is
> potentially very dangerous because if QEMU takes long time to reply (or
> it doesn't reply at all) the domain object is locked an can't be
> destroyed (virsh destroy) - because the first thing that
> qemuDomainDestroyFlags() does is locking the domain object. Also, you
> want to make sure no other thread is currently talking on the monitor -
> so the monitor has to be locked too. We have two convenient functions
> for these operations:
>
> qemuDomainObjEnterMonitor()
> qemuDomainObjExitMonitor()
>
> This is all described in more detail in
> docs/kbase/internals/qemu-threads.rst.
>
> Having said all of this, I think we can fallback to the current behavior
> if job wasn't acquired successfully. Therefore the condition at the very
> beginning might look like this:
>
> if (queryStatsCap && HAVE_JOB() && virDomainObjIsActive()) {
> ...
> qemuDomainEnterMonitor();
> qemuMonitorQueryStats();
> qemuDomainExitMonitor();
> } else {
> virHostCPUGetHaltPollTime();
> }
>
I see that makes sense. I shall do the necessary changes. Thanks for the
detailed explanation.
> > +
> > + 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, *fail;
> > +
> > + success = g_hash_table_lookup(cur_table, success_str);
> > + fail = g_hash_table_lookup(cur_table, fail_str);
> > +
> > + ignore_value(virJSONValueGetNumberUlong(success,
&curHaltPollSuccess));
> > + ignore_value(virJSONValueGetNumberUlong(fail,
&curHaltPollFail));
>
> I don't think this is a safe thing to do. What if either of @success or
> @fail is NULL? That can happen when QEMU didn't return requested member.
> True, at that point the 'query-stats' command should have failed, but I
> think it's more defensive if we checked these pointers. My worry is that
> virJSONValueGetNumberUlong() dereferences the pointer right away.
>
I do understand that this is not an unfounded worry. I shall put the
checks there.
> > +
> > + haltPollSuccess += curHaltPollSuccess;
> > + haltPollFail += curHaltPollFail;
> > + }
> > + }
> >
> > if (virTypedParamListAddULLong(params, haltPollSuccess,
"cpu.haltpoll.success.time") < 0 ||
> > virTypedParamListAddULLong(params, haltPollFail,
"cpu.haltpoll.fail.time") < 0)
> > @@ -18915,7 +18955,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 },
>
> Please feel free to object to anything I wrote. Maybe you have more
> patches written on a local branch that justify your choices. I don't know.
>
> Michal
>
Thanks for taking the time to review the patches.
Amneesh
9:12 p.m.
New subject: [PATCH 3/3] qemu_driver: use qemuMonitorQueryStats to extract halt poll time
On Tue, Jun 28, 2022 at 10:25:54PM +0200, Martin Kletzander wrote:
On Tue, Jun 28, 2022 at 10:15:28PM +0530, Amneesh Singh wrote:
> On Tue, Jun 28, 2022 at 05:23:11PM +0200, Michal Prívozník wrote:
> > On 6/24/22 10:14, Amneesh Singh wrote:
> > > Related: https://gitlab.com/libvirt/libvirt/-/issues/276
> > >
> > > This patch uses qemuMonitorQueryStats to query
"halt_poll_success_ns"
> > > and "halt_poll_fail_ns" for every vCPU. The respective values
for each
> > > vCPU are then added together.
> > >
> > > Signed-off-by: Amneesh Singh <natto(a)weirdnatto.in>
> > > ---
> > > src/qemu/qemu_driver.c | 48 ++++++++++++++++++++++++++++++++++++++----
> > > 1 file changed, 44 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > > index 3b5c3db6..0a2be6d3 100644
> > > --- a/src/qemu/qemu_driver.c
> > > +++ b/src/qemu/qemu_driver.c
> > > @@ -18057,10 +18057,50 @@ qemuDomainGetStatsCpuHaltPollTime(virDomainObj
*dom,
> > > {
> > > unsigned long long haltPollSuccess = 0;
> > > unsigned long long haltPollFail = 0;
> > > - pid_t pid = dom->pid;
> > > + qemuDomainObjPrivate *priv = dom->privateData;
> > > + bool canQueryStats = virQEMUCapsGet(priv->qemuCaps,
QEMU_CAPS_QUERY_STATS);
> >
> > Is this variable really needed? I mean, we can just:
> >
> > if (virQEMUCapsGet(...) {
> >
> > } else {
> >
> > }
> >
> > But if you want to avoid long lines, then perhaps rename the variable to
> > queryStatsCap? This way it's more obvious what the variable reflects.
> > Stats can be queried in both cases ;-)
> Sure, that sounds doable :)
> >
> > >
> > > - if (virHostCPUGetHaltPollTime(pid, &haltPollSuccess,
&haltPollFail) < 0)
> > > - return 0;
> > > + if (!canQueryStats) {
> > > + pid_t pid = dom->pid;
> > > +
> > > + if (virHostCPUGetHaltPollTime(pid, &haltPollSuccess,
&haltPollFail) < 0)
> > > + return 0;
> > > + } else {
> > > + size_t i;
> > > + qemuMonitorQueryStatsTargetType target =
QEMU_MONITOR_QUERY_STATS_TARGET_VCPU;
> > > + qemuMonitorQueryStatsProvider *provider = NULL;
> > > + g_autoptr(GPtrArray) providers = NULL;
> > > + g_autoptr(GPtrArray) queried_stats = NULL;
> > > + const char *success_str = "halt_poll_success_ns";
> > > + const char *fail_str = "halt_poll_fail_ns";
> > > +
> > > + provider =
qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM);
> > > + provider->names = g_new0(char *, 3);
> > > + provider->names[0] = g_strdup(success_str),
provider->names[1] = g_strdup(fail_str);
> >
> > I'm starting to wonder whether this is a good interface. These
->names[]
> > array is never changed, so maybe we can have it as a NULL terminated
> > list of constant strings? For instance:
> >
> > provider = qemuMonitorQueryStatsProviderNew();
> > provider->names = {"halt_poll_success_ns",
"halt_poll_fail_ns", NULL};
> Well, cannot really assign char ** with a scalar initialization, but
> what might work is something like
>
> const char *names[] = { success_str, fail_str, NULL };
>
> provider = qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM);
> provider->names = g_strdupv((char **) names);
>
I think what Michal was trying to say is that since you do not change
the array anywhere, there is no need for that to be a dynamically
allocated array that needs to be freed. I, however, am not 100% if you
are going to need this to be dynamic and whether you will be changing
these arrays.
I don't think there will be any need to have the array mutable,
in
which case, maybe something like this should work
provider->names = (const char *[]){ success_str, fail_str, NULL };
provided that provider->names is changed to const char ** or the same
thing can be done with string literals (or non const string variables)
provided that the cast is (char *[]) and names is still char **.
Compound literals are part of C99 though.
> Another thought was using GStrvBuilder but it is not avaiable as per
> GLIB_VERSION_MAX.
>
> I too think that the current approach is not very nice. A variadic
> function similar to g_strv_builder_add_many that initializes a
> char ** would be nice.
> >
If that is something that helps, then we can write it ourselves and only
use our implementation if glib is too old.
> > > +
> > > + providers = g_ptr_array_new_full(1, (GDestroyNotify)
qemuMonitorQueryStatsProviderFree);
> > > + g_ptr_array_add(providers, provider);
> > > +
> > > + queried_stats = qemuMonitorQueryStats(priv->mon, target, NULL,
providers);
> >
> > This issues monitor command without proper checks. Firstly, you need to
> > check whether job acquiring (that s/false/true/ change done in the last
> > hunk) was successful and the domain is still running:
> >
> > if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom)) {
> > /* code here */
> > }
> >
> > Then, you need to so called "enter the monitor" and "exit the
monitor".
> > So what happens here is that @vm is when this function is called. Now,
> > issuing a monitor command with the domain object lock held is
> > potentially very dangerous because if QEMU takes long time to reply (or
> > it doesn't reply at all) the domain object is locked an can't be
> > destroyed (virsh destroy) - because the first thing that
> > qemuDomainDestroyFlags() does is locking the domain object. Also, you
> > want to make sure no other thread is currently talking on the monitor -
> > so the monitor has to be locked too. We have two convenient functions
> > for these operations:
> >
> > qemuDomainObjEnterMonitor()
> > qemuDomainObjExitMonitor()
> >
> > This is all described in more detail in
> > docs/kbase/internals/qemu-threads.rst.
> >
> > Having said all of this, I think we can fallback to the current behavior
> > if job wasn't acquired successfully. Therefore the condition at the very
> > beginning might look like this:
> >
> > if (queryStatsCap && HAVE_JOB() && virDomainObjIsActive()) {
> > ...
> > qemuDomainEnterMonitor();
> > qemuMonitorQueryStats();
> > qemuDomainExitMonitor();
> > } else {
> > virHostCPUGetHaltPollTime();
> > }
> >
> I see that makes sense. I shall do the necessary changes. Thanks for the
> detailed explanation.
> > > +
> > > + 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, *fail;
> > > +
> > > + success = g_hash_table_lookup(cur_table, success_str);
> > > + fail = g_hash_table_lookup(cur_table, fail_str);
> > > +
> > > + ignore_value(virJSONValueGetNumberUlong(success,
&curHaltPollSuccess));
> > > + ignore_value(virJSONValueGetNumberUlong(fail,
&curHaltPollFail));
> >
> > I don't think this is a safe thing to do. What if either of @success or
> > @fail is NULL? That can happen when QEMU didn't return requested member.
> > True, at that point the 'query-stats' command should have failed, but
I
> > think it's more defensive if we checked these pointers. My worry is that
> > virJSONValueGetNumberUlong() dereferences the pointer right away.
> >
> I do understand that this is not an unfounded worry. I shall put the
> checks there.
> > > +
> > > + haltPollSuccess += curHaltPollSuccess;
> > > + haltPollFail += curHaltPollFail;
> > > + }
> > > + }
> > >
> > > if (virTypedParamListAddULLong(params, haltPollSuccess,
"cpu.haltpoll.success.time") < 0 ||
> > > virTypedParamListAddULLong(params, haltPollFail,
"cpu.haltpoll.fail.time") < 0)
> > > @@ -18915,7 +18955,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 },
> >
> > Please feel free to object to anything I wrote. Maybe you have more
> > patches written on a local branch that justify your choices. I don't know.
> >
> > Michal
> >
> Thanks for taking the time to review the patches.
> Amneesh
Thanks
Amneesh
2:03 a.m.
New subject: [PATCH 3/3] qemu_driver: use qemuMonitorQueryStats to extract halt poll time
On 6/28/22 22:25, Martin Kletzander wrote:
On Tue, Jun 28, 2022 at 10:15:28PM +0530, Amneesh Singh wrote:
> On Tue, Jun 28, 2022 at 05:23:11PM +0200, Michal Prívozník wrote:
>> On 6/24/22 10:14, Amneesh Singh wrote:
>> > Related: https://gitlab.com/libvirt/libvirt/-/issues/276
>> >
>> > This patch uses qemuMonitorQueryStats to query
"halt_poll_success_ns"
>> > and "halt_poll_fail_ns" for every vCPU. The respective values for
each
>> > vCPU are then added together.
>> >
>> > Signed-off-by: Amneesh Singh <natto(a)weirdnatto.in>
>> > ---
>> > src/qemu/qemu_driver.c | 48
>> ++++++++++++++++++++++++++++++++++++++----
>> > 1 file changed, 44 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> > index 3b5c3db6..0a2be6d3 100644
>> > --- a/src/qemu/qemu_driver.c
>> > +++ b/src/qemu/qemu_driver.c
>> > @@ -18057,10 +18057,50 @@
>> qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom,
>> > {
>> > unsigned long long haltPollSuccess = 0;
>> > unsigned long long haltPollFail = 0;
>> > - pid_t pid = dom->pid;
>> > + qemuDomainObjPrivate *priv = dom->privateData;
>> > + bool canQueryStats = virQEMUCapsGet(priv->qemuCaps,
>> QEMU_CAPS_QUERY_STATS);
>>
>> Is this variable really needed? I mean, we can just:
>>
>> if (virQEMUCapsGet(...) {
>>
>> } else {
>>
>> }
>>
>> But if you want to avoid long lines, then perhaps rename the variable to
>> queryStatsCap? This way it's more obvious what the variable reflects.
>> Stats can be queried in both cases ;-)
> Sure, that sounds doable :)
>>
>> >
>> > - if (virHostCPUGetHaltPollTime(pid, &haltPollSuccess,
>> &haltPollFail) < 0)
>> > - return 0;
>> > + if (!canQueryStats) {
>> > + pid_t pid = dom->pid;
>> > +
>> > + if (virHostCPUGetHaltPollTime(pid, &haltPollSuccess,
>> &haltPollFail) < 0)
>> > + return 0;
>> > + } else {
>> > + size_t i;
>> > + qemuMonitorQueryStatsTargetType target =
>> QEMU_MONITOR_QUERY_STATS_TARGET_VCPU;
>> > + qemuMonitorQueryStatsProvider *provider = NULL;
>> > + g_autoptr(GPtrArray) providers = NULL;
>> > + g_autoptr(GPtrArray) queried_stats = NULL;
>> > + const char *success_str = "halt_poll_success_ns";
>> > + const char *fail_str = "halt_poll_fail_ns";
>> > +
>> > + provider =
>> qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM);
>> > + provider->names = g_new0(char *, 3);
>> > + provider->names[0] = g_strdup(success_str),
>> provider->names[1] = g_strdup(fail_str);
>>
>> I'm starting to wonder whether this is a good interface. These ->names[]
>> array is never changed, so maybe we can have it as a NULL terminated
>> list of constant strings? For instance:
>>
>> provider = qemuMonitorQueryStatsProviderNew();
>> provider->names = {"halt_poll_success_ns",
"halt_poll_fail_ns", NULL};
> Well, cannot really assign char ** with a scalar initialization, but
> what might work is something like
>
> const char *names[] = { success_str, fail_str, NULL };
>
> provider =
> qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM);
> provider->names = g_strdupv((char **) names);
>
Yep, I made too much of a shortcut.
I think what Michal was trying to say is that since you do not change
the array anywhere, there is no need for that to be a dynamically
allocated array that needs to be freed. I, however, am not 100% if you
are going to need this to be dynamic and whether you will be changing
these arrays.
Yes. I'm not exactly sure why those strings have to be strdup-ed(). I
mean, from the conceptual POV. They are not changed anywhere. And now
that I think about it - why they have to be strings at all? They have to
be strings at the monitor level, because that's what QEMU expects,
however, I can imagine that in our code, in its upper layers those can
be just part of enum. Or even better - a bitmask - just like
virQEMUCaps. For instance:
qemu_monitor.h:
typedef enum {
QEMU_MONITOR_QUERY_STATS_HALT_POLL_SUCCESS_NS,
QEMU_MONITOR_QUERY_STATS_HALT_POLL_FAIL_NS,
QEMU_MONITOR_QUERY_STATS_LAST
} qemuMonitorQueryStatsFlags;
qemu_monitor.c:
VIR_ENUM_DECL(qemuMonitorQueryStatsFlags);
VIR_ENUM_IMPL(qemuMonitorQueryStatsFlags,
QEMU_MONITOR_QUERY_STATS_LAST,
"halt_poll_success_ns",
"halt_poll_fail_ns"
);
and when constructing the monitor command
qemuMonitorQueryStatsFlagsTypeToString() translates those
QEMU_MONITOR_QUERY_STATS_* enum members into their corresponding string
representation.
Now, qemuMonitorQueryStatsProviderNew() could then behave like this:
provider = qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM);
virBitmapSet(provider->names, QEMU_MONITOR_QUERY_STATS_HALT_POLL_SUCCESS_NS);
virBitmapSet(provider->names, QEMU_MONITOR_QUERY_STATS_HALT_POLL_FAIL_NS);
Alternatively, we can have all of that in the
qemuMonitorQueryStatsProviderNew() call with help of va_args, like this:
qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM,
QEMU_MONITOR_QUERY_STATS_HALT_POLL_SUCCESS_NS,
QEMU_MONITOR_QUERY_STATS_HALT_POLL_FAIL_NS,
0);
Then, when constructing the monitor command, virBitmapNextSetBit() could
be used in a loop to iterate over set bits in combination with
aforementioned qemuMonitorQueryStatsFlagsTypeToString() to translate bit
index into string.
> Another thought was using GStrvBuilder but it is not avaiable as per
> GLIB_VERSION_MAX.
>
> I too think that the current approach is not very nice. A variadic
> function similar to g_strv_builder_add_many that initializes a
> char ** would be nice.
>>
If that is something that helps, then we can write it ourselves and only
use our implementation if glib is too old.
Yeah, in general we could just copy glib's implementation into
src/util/glibcompat.c and drop it later on, but I think we can do
without strings (at least dynamically allocated ones).
One of the reasons I want to avoid working with strings at this level is
(besides the obvious memory complexity): compiler can't check for
correctness. If I made a typo in either of strings
("halt_poll_sucess_ns") then nothing warns me.
Michal
5:47 a.m.
New subject: [PATCH 3/3] qemu_driver: use qemuMonitorQueryStats to extract halt poll time
On Wed, Jun 29, 2022 at 09:03:33AM +0200, Michal Prívozník wrote:
On 6/28/22 22:25, Martin Kletzander wrote:
> On Tue, Jun 28, 2022 at 10:15:28PM +0530, Amneesh Singh wrote:
>> On Tue, Jun 28, 2022 at 05:23:11PM +0200, Michal Prívozník wrote:
>>> On 6/24/22 10:14, Amneesh Singh wrote:
>>> > Related: https://gitlab.com/libvirt/libvirt/-/issues/276
>>> >
>>> > This patch uses qemuMonitorQueryStats to query
"halt_poll_success_ns"
>>> > and "halt_poll_fail_ns" for every vCPU. The respective values
for each
>>> > vCPU are then added together.
>>> >
>>> > Signed-off-by: Amneesh Singh <natto(a)weirdnatto.in>
>>> > ---
>>> > src/qemu/qemu_driver.c | 48
>>> ++++++++++++++++++++++++++++++++++++++----
>>> > 1 file changed, 44 insertions(+), 4 deletions(-)
>>> >
>>> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>> > index 3b5c3db6..0a2be6d3 100644
>>> > --- a/src/qemu/qemu_driver.c
>>> > +++ b/src/qemu/qemu_driver.c
>>> > @@ -18057,10 +18057,50 @@
>>> qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom,
>>> > {
>>> > unsigned long long haltPollSuccess = 0;
>>> > unsigned long long haltPollFail = 0;
>>> > - pid_t pid = dom->pid;
>>> > + qemuDomainObjPrivate *priv = dom->privateData;
>>> > + bool canQueryStats = virQEMUCapsGet(priv->qemuCaps,
>>> QEMU_CAPS_QUERY_STATS);
>>>
>>> Is this variable really needed? I mean, we can just:
>>>
>>> if (virQEMUCapsGet(...) {
>>>
>>> } else {
>>>
>>> }
>>>
>>> But if you want to avoid long lines, then perhaps rename the variable to
>>> queryStatsCap? This way it's more obvious what the variable reflects.
>>> Stats can be queried in both cases ;-)
>> Sure, that sounds doable :)
>>>
>>> >
>>> > - if (virHostCPUGetHaltPollTime(pid, &haltPollSuccess,
>>> &haltPollFail) < 0)
>>> > - return 0;
>>> > + if (!canQueryStats) {
>>> > + pid_t pid = dom->pid;
>>> > +
>>> > + if (virHostCPUGetHaltPollTime(pid, &haltPollSuccess,
>>> &haltPollFail) < 0)
>>> > + return 0;
>>> > + } else {
>>> > + size_t i;
>>> > + qemuMonitorQueryStatsTargetType target =
>>> QEMU_MONITOR_QUERY_STATS_TARGET_VCPU;
>>> > + qemuMonitorQueryStatsProvider *provider = NULL;
>>> > + g_autoptr(GPtrArray) providers = NULL;
>>> > + g_autoptr(GPtrArray) queried_stats = NULL;
>>> > + const char *success_str = "halt_poll_success_ns";
>>> > + const char *fail_str = "halt_poll_fail_ns";
>>> > +
>>> > + provider =
>>> qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM);
>>> > + provider->names = g_new0(char *, 3);
>>> > + provider->names[0] = g_strdup(success_str),
>>> provider->names[1] = g_strdup(fail_str);
>>>
>>> I'm starting to wonder whether this is a good interface. These
->names[]
>>> array is never changed, so maybe we can have it as a NULL terminated
>>> list of constant strings? For instance:
>>>
>>> provider = qemuMonitorQueryStatsProviderNew();
>>> provider->names = {"halt_poll_success_ns",
"halt_poll_fail_ns", NULL};
>> Well, cannot really assign char ** with a scalar initialization, but
>> what might work is something like
>>
>> const char *names[] = { success_str, fail_str, NULL };
>>
>> provider =
>> qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM);
>> provider->names = g_strdupv((char **) names);
>>
Yep, I made too much of a shortcut.
>
> I think what Michal was trying to say is that since you do not change
> the array anywhere, there is no need for that to be a dynamically
> allocated array that needs to be freed. I, however, am not 100% if you
> are going to need this to be dynamic and whether you will be changing
> these arrays.
Yes. I'm not exactly sure why those strings have to be strdup-ed(). I
mean, from the conceptual POV. They are not changed anywhere. And now
that I think about it - why they have to be strings at all? They have to
be strings at the monitor level, because that's what QEMU expects,
however, I can imagine that in our code, in its upper layers those can
be just part of enum. Or even better - a bitmask - just like
virQEMUCaps. For instance:
qemu_monitor.h:
typedef enum {
QEMU_MONITOR_QUERY_STATS_HALT_POLL_SUCCESS_NS,
QEMU_MONITOR_QUERY_STATS_HALT_POLL_FAIL_NS,
QEMU_MONITOR_QUERY_STATS_LAST
} qemuMonitorQueryStatsFlags;
qemu_monitor.c:
VIR_ENUM_DECL(qemuMonitorQueryStatsFlags);
VIR_ENUM_IMPL(qemuMonitorQueryStatsFlags,
QEMU_MONITOR_QUERY_STATS_LAST,
"halt_poll_success_ns",
"halt_poll_fail_ns"
);
and when constructing the monitor command
qemuMonitorQueryStatsFlagsTypeToString() translates those
QEMU_MONITOR_QUERY_STATS_* enum members into their corresponding string
representation.
Now, qemuMonitorQueryStatsProviderNew() could then behave like this:
provider = qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM);
virBitmapSet(provider->names, QEMU_MONITOR_QUERY_STATS_HALT_POLL_SUCCESS_NS);
virBitmapSet(provider->names, QEMU_MONITOR_QUERY_STATS_HALT_POLL_FAIL_NS);
Alternatively, we can have all of that in the
qemuMonitorQueryStatsProviderNew() call with help of va_args, like this:
qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM,
QEMU_MONITOR_QUERY_STATS_HALT_POLL_SUCCESS_NS,
QEMU_MONITOR_QUERY_STATS_HALT_POLL_FAIL_NS,
0);
Then, when constructing the monitor command, virBitmapNextSetBit() could
be used in a loop to iterate over set bits in combination with
aforementioned qemuMonitorQueryStatsFlagsTypeToString() to translate bit
index into string.
Sorry for not specifying this earlier but the statistics are
exposed by
KVM and there is no enum for the statistics at the QAPI level either.
But your point still stands, I definitely wanted to use an enum for this
too as it makes things much easier for everyone. I was not sure whether
it was a good idea to use one for things not known at the compile time,
but to be fair, using raw strings like this is also pretty much the same.
Unless the user wants to query some stat, that has not been updated in
the libvirt enum, using a string, doing this should be alright. Do you
think we can move forward with this? The user can do the latter using
qemu-monitor-command anyway.
Also note that the enums for each target would be different (VM and
VCPU for now).
Thanks
>
>> Another thought was using GStrvBuilder but it is not avaiable as per
>> GLIB_VERSION_MAX.
>>
>> I too think that the current approach is not very nice. A variadic
>> function similar to g_strv_builder_add_many that initializes a
>> char ** would be nice.
>>>
>
> If that is something that helps, then we can write it ourselves and only
> use our implementation if glib is too old.
Yeah, in general we could just copy glib's implementation into
src/util/glibcompat.c and drop it later on, but I think we can do
without strings (at least dynamically allocated ones).
One of the reasons I want to avoid working with strings at this level is
(besides the obvious memory complexity): compiler can't check for
correctness. If I made a typo in either of strings
("halt_poll_sucess_ns") then nothing warns me.
Michal
Amneesh
9:15 a.m.
New subject: [PATCH 3/3] qemu_driver: use qemuMonitorQueryStats to extract halt poll time
On 6/29/22 12:47, Amneesh Singh wrote:
On Wed, Jun 29, 2022 at 09:03:33AM +0200, Michal Prívozník wrote:
> On 6/28/22 22:25, Martin Kletzander wrote:
>> On Tue, Jun 28, 2022 at 10:15:28PM +0530, Amneesh Singh wrote:
>>> On Tue, Jun 28, 2022 at 05:23:11PM +0200, Michal Prívozník wrote:
>>>> On 6/24/22 10:14, Amneesh Singh wrote:
>>>>> Related: https://gitlab.com/libvirt/libvirt/-/issues/276
>>>>>
>>>>> This patch uses qemuMonitorQueryStats to query
"halt_poll_success_ns"
>>>>> and "halt_poll_fail_ns" for every vCPU. The respective
values for each
>>>>> vCPU are then added together.
>>>>>
>>>>> Signed-off-by: Amneesh Singh <natto(a)weirdnatto.in>
>>>>> ---
>>>>> src/qemu/qemu_driver.c | 48
>>>> ++++++++++++++++++++++++++++++++++++++----
>>>>> 1 file changed, 44 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>>>> index 3b5c3db6..0a2be6d3 100644
>>>>> --- a/src/qemu/qemu_driver.c
>>>>> +++ b/src/qemu/qemu_driver.c
>>>>> @@ -18057,10 +18057,50 @@
>>>> qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom,
>>>>> {
>>>>> unsigned long long haltPollSuccess = 0;
>>>>> unsigned long long haltPollFail = 0;
>>>>> - pid_t pid = dom->pid;
>>>>> + qemuDomainObjPrivate *priv = dom->privateData;
>>>>> + bool canQueryStats = virQEMUCapsGet(priv->qemuCaps,
>>>> QEMU_CAPS_QUERY_STATS);
>>>>
>>>> Is this variable really needed? I mean, we can just:
>>>>
>>>> if (virQEMUCapsGet(...) {
>>>>
>>>> } else {
>>>>
>>>> }
>>>>
>>>> But if you want to avoid long lines, then perhaps rename the variable to
>>>> queryStatsCap? This way it's more obvious what the variable
reflects.
>>>> Stats can be queried in both cases ;-)
>>> Sure, that sounds doable :)
>>>>
>>>>>
>>>>> - if (virHostCPUGetHaltPollTime(pid, &haltPollSuccess,
>>>> &haltPollFail) < 0)
>>>>> - return 0;
>>>>> + if (!canQueryStats) {
>>>>> + pid_t pid = dom->pid;
>>>>> +
>>>>> + if (virHostCPUGetHaltPollTime(pid, &haltPollSuccess,
>>>> &haltPollFail) < 0)
>>>>> + return 0;
>>>>> + } else {
>>>>> + size_t i;
>>>>> + qemuMonitorQueryStatsTargetType target =
>>>> QEMU_MONITOR_QUERY_STATS_TARGET_VCPU;
>>>>> + qemuMonitorQueryStatsProvider *provider = NULL;
>>>>> + g_autoptr(GPtrArray) providers = NULL;
>>>>> + g_autoptr(GPtrArray) queried_stats = NULL;
>>>>> + const char *success_str = "halt_poll_success_ns";
>>>>> + const char *fail_str = "halt_poll_fail_ns";
>>>>> +
>>>>> + provider =
>>>> qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM);
>>>>> + provider->names = g_new0(char *, 3);
>>>>> + provider->names[0] = g_strdup(success_str),
>>>> provider->names[1] = g_strdup(fail_str);
>>>>
>>>> I'm starting to wonder whether this is a good interface. These
->names[]
>>>> array is never changed, so maybe we can have it as a NULL terminated
>>>> list of constant strings? For instance:
>>>>
>>>> provider = qemuMonitorQueryStatsProviderNew();
>>>> provider->names = {"halt_poll_success_ns",
"halt_poll_fail_ns", NULL};
>>> Well, cannot really assign char ** with a scalar initialization, but
>>> what might work is something like
>>>
>>> const char *names[] = { success_str, fail_str, NULL };
>>>
>>> provider =
>>> qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM);
>>> provider->names = g_strdupv((char **) names);
>>>
>
> Yep, I made too much of a shortcut.
>
>>
>> I think what Michal was trying to say is that since you do not change
>> the array anywhere, there is no need for that to be a dynamically
>> allocated array that needs to be freed. I, however, am not 100% if you
>> are going to need this to be dynamic and whether you will be changing
>> these arrays.
>
> Yes. I'm not exactly sure why those strings have to be strdup-ed(). I
> mean, from the conceptual POV. They are not changed anywhere. And now
> that I think about it - why they have to be strings at all? They have to
> be strings at the monitor level, because that's what QEMU expects,
> however, I can imagine that in our code, in its upper layers those can
> be just part of enum. Or even better - a bitmask - just like
> virQEMUCaps. For instance:
>
> qemu_monitor.h:
>
> typedef enum {
> QEMU_MONITOR_QUERY_STATS_HALT_POLL_SUCCESS_NS,
> QEMU_MONITOR_QUERY_STATS_HALT_POLL_FAIL_NS,
>
> QEMU_MONITOR_QUERY_STATS_LAST
> } qemuMonitorQueryStatsFlags;
>
>
> qemu_monitor.c:
>
> VIR_ENUM_DECL(qemuMonitorQueryStatsFlags);
> VIR_ENUM_IMPL(qemuMonitorQueryStatsFlags,
> QEMU_MONITOR_QUERY_STATS_LAST,
> "halt_poll_success_ns",
> "halt_poll_fail_ns"
> );
>
> and when constructing the monitor command
> qemuMonitorQueryStatsFlagsTypeToString() translates those
> QEMU_MONITOR_QUERY_STATS_* enum members into their corresponding string
> representation.
>
> Now, qemuMonitorQueryStatsProviderNew() could then behave like this:
>
> provider = qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM);
> virBitmapSet(provider->names, QEMU_MONITOR_QUERY_STATS_HALT_POLL_SUCCESS_NS);
> virBitmapSet(provider->names, QEMU_MONITOR_QUERY_STATS_HALT_POLL_FAIL_NS);
>
> Alternatively, we can have all of that in the
> qemuMonitorQueryStatsProviderNew() call with help of va_args, like this:
>
>
> qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM,
> QEMU_MONITOR_QUERY_STATS_HALT_POLL_SUCCESS_NS,
> QEMU_MONITOR_QUERY_STATS_HALT_POLL_FAIL_NS,
> 0);
>
> Then, when constructing the monitor command, virBitmapNextSetBit() could
> be used in a loop to iterate over set bits in combination with
> aforementioned qemuMonitorQueryStatsFlagsTypeToString() to translate bit
> index into string.
Sorry for not specifying this earlier but the statistics are exposed by
KVM and there is no enum for the statistics at the QAPI level either.
But your point still stands, I definitely wanted to use an enum for this
too as it makes things much easier for everyone. I was not sure whether
it was a good idea to use one for things not known at the compile time,
but to be fair, using raw strings like this is also pretty much the same.
Unless the user wants to query some stat, that has not been updated in
the libvirt enum, using a string, doing this should be alright. Do you
think we can move forward with this? The user can do the latter using
qemu-monitor-command anyway.
Yeah, this is not different to other areas of the code. We usually have
to add something to an enum when QEMU gains new functionality. That's
okay. We much rather have control over QEMU than allow user specifying
"random" values.
Also note that the enums for each target would be different (VM and
VCPU for now).
Yes, I suspected that :-)
Michal
906
days inactive
912
days old
11 comments
3 participants
participants (3)
-
Amneesh Singh -
Martin Kletzander -
Michal Prívozník