[PATCH 0/3] qemu: add an API for "query-stats" QMP command
QEMU is gaining introspectable statistics which can be queried via the "query-stats" QMP command. This patchset aims to add an API for the same. The returned JSON for "query-stats" is an array of objects containing their own respective array of statistics. Patch 1 adds the API which returns the deserialized JSON in the form of a GPtrArray of GHashTables. Patch 2 adds the "query-stats" to QEMU capabilities. Patch 3 uses the API to query the halt poll success time and the halt poll failure time. 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
Related: https://gitlab.com/libvirt/libvirt/-/issues/276 This patch adds an API for the "query-stats" QMP command. The query returns a JSON containing the statistics based on the target, which can either be vCPU or VM, and the providers. The API deserializes the query result into an array of GHashMaps, which can later be used to extract all the query statistics. GHashMaps are used to avoid traversing the entire array to find the statistics you are looking for. This would be a singleton array if the target is a VM since the returned JSON is also a singleton array in that case. Signed-off-by: Amneesh Singh <natto@weirdnatto.in> --- src/qemu/qemu_monitor.c | 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
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@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
Related: https://gitlab.com/libvirt/libvirt/-/issues/276 This patch adds "query-stats" to the QEMU capabilities. Signed-off-by: Amneesh Singh <natto@weirdnatto.in> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + 2 files changed, 3 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 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
Related: https://gitlab.com/libvirt/libvirt/-/issues/276 This patch uses qemuMonitorQueryStats to query "halt_poll_success_ns" and "halt_poll_fail_ns" for every vCPU. The respective values for each vCPU are then added together. Signed-off-by: Amneesh Singh <natto@weirdnatto.in> --- src/qemu/qemu_driver.c | 48 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 44 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3b5c3db6..0a2be6d3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18057,10 +18057,50 @@ qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom, { unsigned long long haltPollSuccess = 0; unsigned long long haltPollFail = 0; - pid_t pid = dom->pid; + qemuDomainObjPrivate *priv = dom->privateData; + bool canQueryStats = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_STATS); - 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
On 6/24/22 10:14, Amneesh Singh wrote:
Related: https://gitlab.com/libvirt/libvirt/-/issues/276
This patch uses qemuMonitorQueryStats to query "halt_poll_success_ns" and "halt_poll_fail_ns" for every vCPU. The respective values for each vCPU are then added together.
Signed-off-by: Amneesh Singh <natto@weirdnatto.in> --- src/qemu/qemu_driver.c | 48 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 44 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3b5c3db6..0a2be6d3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18057,10 +18057,50 @@ qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom, { unsigned long long haltPollSuccess = 0; unsigned long long haltPollFail = 0; - pid_t pid = dom->pid; + qemuDomainObjPrivate *priv = dom->privateData; + bool canQueryStats = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_STATS);
Is this variable really needed? I mean, we can just: if (virQEMUCapsGet(...) { } else { } But if you want to avoid long lines, then perhaps rename the variable to queryStatsCap? This way it's more obvious what the variable reflects. Stats can be queried in both cases ;-)
- 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
On Tue, Jun 28, 2022 at 05:23:11PM +0200, Michal Prívozník wrote:
On 6/24/22 10:14, Amneesh Singh wrote:
Related: https://gitlab.com/libvirt/libvirt/-/issues/276
This patch uses qemuMonitorQueryStats to query "halt_poll_success_ns" and "halt_poll_fail_ns" for every vCPU. The respective values for each vCPU are then added together.
Signed-off-by: Amneesh Singh <natto@weirdnatto.in> --- src/qemu/qemu_driver.c | 48 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 44 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3b5c3db6..0a2be6d3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18057,10 +18057,50 @@ qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom, { unsigned long long haltPollSuccess = 0; unsigned long long haltPollFail = 0; - pid_t pid = dom->pid; + qemuDomainObjPrivate *priv = dom->privateData; + bool canQueryStats = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_STATS);
Is this variable really needed? I mean, we can just:
if (virQEMUCapsGet(...) {
} else {
}
But if you want to avoid long lines, then perhaps rename the variable to queryStatsCap? This way it's more obvious what the variable reflects. Stats can be queried in both cases ;-) Sure, that sounds doable :)
- if (virHostCPUGetHaltPollTime(pid, &haltPollSuccess, &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
On Tue, Jun 28, 2022 at 10:15:28PM +0530, Amneesh Singh wrote:
On Tue, Jun 28, 2022 at 05:23:11PM +0200, Michal Prívozník wrote:
On 6/24/22 10:14, Amneesh Singh wrote:
Related: https://gitlab.com/libvirt/libvirt/-/issues/276
This patch uses qemuMonitorQueryStats to query "halt_poll_success_ns" and "halt_poll_fail_ns" for every vCPU. The respective values for each vCPU are then added together.
Signed-off-by: Amneesh Singh <natto@weirdnatto.in> --- src/qemu/qemu_driver.c | 48 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 44 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3b5c3db6..0a2be6d3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18057,10 +18057,50 @@ qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom, { unsigned long long haltPollSuccess = 0; unsigned long long haltPollFail = 0; - pid_t pid = dom->pid; + qemuDomainObjPrivate *priv = dom->privateData; + bool canQueryStats = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_STATS);
Is this variable really needed? I mean, we can just:
if (virQEMUCapsGet(...) {
} else {
}
But if you want to avoid long lines, then perhaps rename the variable to queryStatsCap? This way it's more obvious what the variable reflects. Stats can be queried in both cases ;-) Sure, that sounds doable :)
- if (virHostCPUGetHaltPollTime(pid, &haltPollSuccess, &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
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@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
On 6/28/22 22:25, Martin Kletzander wrote:
On Tue, Jun 28, 2022 at 10:15:28PM +0530, Amneesh Singh wrote:
On Tue, Jun 28, 2022 at 05:23:11PM +0200, Michal Prívozník wrote:
On 6/24/22 10:14, Amneesh Singh wrote:
Related: https://gitlab.com/libvirt/libvirt/-/issues/276
This patch uses qemuMonitorQueryStats to query "halt_poll_success_ns" and "halt_poll_fail_ns" for every vCPU. The respective values for each vCPU are then added together.
Signed-off-by: Amneesh Singh <natto@weirdnatto.in> --- src/qemu/qemu_driver.c | 48 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 44 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3b5c3db6..0a2be6d3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18057,10 +18057,50 @@ qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom, { unsigned long long haltPollSuccess = 0; unsigned long long haltPollFail = 0; - pid_t pid = dom->pid; + qemuDomainObjPrivate *priv = dom->privateData; + bool canQueryStats = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_STATS);
Is this variable really needed? I mean, we can just:
if (virQEMUCapsGet(...) {
} else {
}
But if you want to avoid long lines, then perhaps rename the variable to queryStatsCap? This way it's more obvious what the variable reflects. Stats can be queried in both cases ;-) Sure, that sounds doable :)
- if (virHostCPUGetHaltPollTime(pid, &haltPollSuccess,
- return 0; + if (!canQueryStats) { + pid_t pid = dom->pid; + + if (virHostCPUGetHaltPollTime(pid, &haltPollSuccess, &haltPollFail) < 0) + return 0; + } else { + size_t i; + qemuMonitorQueryStatsTargetType target = QEMU_MONITOR_QUERY_STATS_TARGET_VCPU; + qemuMonitorQueryStatsProvider *provider = NULL; + g_autoptr(GPtrArray) providers = NULL; + g_autoptr(GPtrArray) queried_stats = NULL; + const char *success_str = "halt_poll_success_ns"; + const char *fail_str = "halt_poll_fail_ns"; + + provider = qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM); + provider->names = g_new0(char *, 3); + provider->names[0] = g_strdup(success_str),
&haltPollFail) < 0) provider->names[1] = g_strdup(fail_str);
I'm starting to wonder whether this is a good interface. These ->names[] array is never changed, so maybe we can have it as a NULL terminated list of constant strings? For instance:
provider = qemuMonitorQueryStatsProviderNew(); provider->names = {"halt_poll_success_ns", "halt_poll_fail_ns", NULL}; Well, cannot really assign char ** with a scalar initialization, but what might work is something like
const char *names[] = { success_str, fail_str, NULL };
provider = qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM); provider->names = g_strdupv((char **) names);
Yep, I made too much of a shortcut.
I think what Michal was trying to say is that since you do not change the array anywhere, there is no need for that to be a dynamically allocated array that needs to be freed. I, however, am not 100% if you are going to need this to be dynamic and whether you will be changing these arrays.
Yes. I'm not exactly sure why those strings have to be strdup-ed(). I mean, from the conceptual POV. They are not changed anywhere. And now that I think about it - why they have to be strings at all? They have to be strings at the monitor level, because that's what QEMU expects, however, I can imagine that in our code, in its upper layers those can be just part of enum. Or even better - a bitmask - just like virQEMUCaps. For instance: qemu_monitor.h: typedef enum { QEMU_MONITOR_QUERY_STATS_HALT_POLL_SUCCESS_NS, QEMU_MONITOR_QUERY_STATS_HALT_POLL_FAIL_NS, QEMU_MONITOR_QUERY_STATS_LAST } qemuMonitorQueryStatsFlags; qemu_monitor.c: VIR_ENUM_DECL(qemuMonitorQueryStatsFlags); VIR_ENUM_IMPL(qemuMonitorQueryStatsFlags, QEMU_MONITOR_QUERY_STATS_LAST, "halt_poll_success_ns", "halt_poll_fail_ns" ); and when constructing the monitor command qemuMonitorQueryStatsFlagsTypeToString() translates those QEMU_MONITOR_QUERY_STATS_* enum members into their corresponding string representation. Now, qemuMonitorQueryStatsProviderNew() could then behave like this: provider = qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM); virBitmapSet(provider->names, QEMU_MONITOR_QUERY_STATS_HALT_POLL_SUCCESS_NS); virBitmapSet(provider->names, QEMU_MONITOR_QUERY_STATS_HALT_POLL_FAIL_NS); Alternatively, we can have all of that in the qemuMonitorQueryStatsProviderNew() call with help of va_args, like this: qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM, QEMU_MONITOR_QUERY_STATS_HALT_POLL_SUCCESS_NS, QEMU_MONITOR_QUERY_STATS_HALT_POLL_FAIL_NS, 0); Then, when constructing the monitor command, virBitmapNextSetBit() could be used in a loop to iterate over set bits in combination with aforementioned qemuMonitorQueryStatsFlagsTypeToString() to translate bit index into string.
Another thought was using GStrvBuilder but it is not avaiable as per GLIB_VERSION_MAX.
I too think that the current approach is not very nice. A variadic function similar to g_strv_builder_add_many that initializes a char ** would be nice.
If that is something that helps, then we can write it ourselves and only use our implementation if glib is too old.
Yeah, in general we could just copy glib's implementation into src/util/glibcompat.c and drop it later on, but I think we can do without strings (at least dynamically allocated ones). One of the reasons I want to avoid working with strings at this level is (besides the obvious memory complexity): compiler can't check for correctness. If I made a typo in either of strings ("halt_poll_sucess_ns") then nothing warns me. Michal
On 6/28/22 22:25, Martin Kletzander wrote:
On Tue, Jun 28, 2022 at 10:15:28PM +0530, Amneesh Singh wrote:
On Tue, Jun 28, 2022 at 05:23:11PM +0200, Michal Prívozník wrote:
On 6/24/22 10:14, Amneesh Singh wrote:
Related: https://gitlab.com/libvirt/libvirt/-/issues/276
This patch uses qemuMonitorQueryStats to query "halt_poll_success_ns" and "halt_poll_fail_ns" for every vCPU. The respective values for each vCPU are then added together.
Signed-off-by: Amneesh Singh <natto@weirdnatto.in> --- src/qemu/qemu_driver.c | 48 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 44 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3b5c3db6..0a2be6d3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18057,10 +18057,50 @@ qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom, { unsigned long long haltPollSuccess = 0; unsigned long long haltPollFail = 0; - pid_t pid = dom->pid; + qemuDomainObjPrivate *priv = dom->privateData; + bool canQueryStats = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_STATS);
Is this variable really needed? I mean, we can just:
if (virQEMUCapsGet(...) {
} else {
}
But if you want to avoid long lines, then perhaps rename the variable to queryStatsCap? This way it's more obvious what the variable reflects. Stats can be queried in both cases ;-) Sure, that sounds doable :)
- if (virHostCPUGetHaltPollTime(pid, &haltPollSuccess,
- return 0; + if (!canQueryStats) { + pid_t pid = dom->pid; + + if (virHostCPUGetHaltPollTime(pid, &haltPollSuccess, &haltPollFail) < 0) + return 0; + } else { + size_t i; + qemuMonitorQueryStatsTargetType target = QEMU_MONITOR_QUERY_STATS_TARGET_VCPU; + qemuMonitorQueryStatsProvider *provider = NULL; + g_autoptr(GPtrArray) providers = NULL; + g_autoptr(GPtrArray) queried_stats = NULL; + const char *success_str = "halt_poll_success_ns"; + const char *fail_str = "halt_poll_fail_ns"; + + provider = qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM); + provider->names = g_new0(char *, 3); + provider->names[0] = g_strdup(success_str),
&haltPollFail) < 0) provider->names[1] = g_strdup(fail_str);
I'm starting to wonder whether this is a good interface. These ->names[] array is never changed, so maybe we can have it as a NULL terminated list of constant strings? For instance:
provider = qemuMonitorQueryStatsProviderNew(); provider->names = {"halt_poll_success_ns", "halt_poll_fail_ns", NULL}; Well, cannot really assign char ** with a scalar initialization, but what might work is something like
const char *names[] = { success_str, fail_str, NULL };
provider = qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM); provider->names = g_strdupv((char **) names);
Yep, I made too much of a shortcut.
I think what Michal was trying to say is that since you do not change the array anywhere, there is no need for that to be a dynamically allocated array that needs to be freed. I, however, am not 100% if you are going to need this to be dynamic and whether you will be changing these arrays.
Yes. I'm not exactly sure why those strings have to be strdup-ed(). I mean, from the conceptual POV. They are not changed anywhere. And now that I think about it - why they have to be strings at all? They have to be strings at the monitor level, because that's what QEMU expects, however, I can imagine that in our code, in its upper layers those can be just part of enum. Or even better - a bitmask - just like virQEMUCaps. For instance:
qemu_monitor.h:
typedef enum { QEMU_MONITOR_QUERY_STATS_HALT_POLL_SUCCESS_NS, QEMU_MONITOR_QUERY_STATS_HALT_POLL_FAIL_NS,
QEMU_MONITOR_QUERY_STATS_LAST } qemuMonitorQueryStatsFlags;
qemu_monitor.c:
VIR_ENUM_DECL(qemuMonitorQueryStatsFlags); VIR_ENUM_IMPL(qemuMonitorQueryStatsFlags, QEMU_MONITOR_QUERY_STATS_LAST, "halt_poll_success_ns", "halt_poll_fail_ns" );
and when constructing the monitor command qemuMonitorQueryStatsFlagsTypeToString() translates those QEMU_MONITOR_QUERY_STATS_* enum members into their corresponding string representation.
Now, qemuMonitorQueryStatsProviderNew() could then behave like this:
provider = qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM); virBitmapSet(provider->names, QEMU_MONITOR_QUERY_STATS_HALT_POLL_SUCCESS_NS); virBitmapSet(provider->names, QEMU_MONITOR_QUERY_STATS_HALT_POLL_FAIL_NS);
Alternatively, we can have all of that in the qemuMonitorQueryStatsProviderNew() call with help of va_args, like this:
qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM, QEMU_MONITOR_QUERY_STATS_HALT_POLL_SUCCESS_NS, QEMU_MONITOR_QUERY_STATS_HALT_POLL_FAIL_NS, 0);
Then, when constructing the monitor command, virBitmapNextSetBit() could be used in a loop to iterate over set bits in combination with aforementioned qemuMonitorQueryStatsFlagsTypeToString() to translate bit index into string. 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
On Wed, Jun 29, 2022 at 09:03:33AM +0200, Michal Prívozník wrote: 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
On 6/29/22 12:47, Amneesh Singh 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@weirdnatto.in> --- src/qemu/qemu_driver.c | 48 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 44 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3b5c3db6..0a2be6d3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18057,10 +18057,50 @@ qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom, { unsigned long long haltPollSuccess = 0; unsigned long long haltPollFail = 0; - pid_t pid = dom->pid; + qemuDomainObjPrivate *priv = dom->privateData; + bool canQueryStats = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_QUERY_STATS);
Is this variable really needed? I mean, we can just:
if (virQEMUCapsGet(...) {
} else {
}
But if you want to avoid long lines, then perhaps rename the variable to queryStatsCap? This way it's more obvious what the variable reflects. Stats can be queried in both cases ;-) Sure, that sounds doable :)
- if (virHostCPUGetHaltPollTime(pid, &haltPollSuccess,
- return 0; + if (!canQueryStats) { + pid_t pid = dom->pid; + + if (virHostCPUGetHaltPollTime(pid, &haltPollSuccess, &haltPollFail) < 0) + return 0; + } else { + size_t i; + qemuMonitorQueryStatsTargetType target = QEMU_MONITOR_QUERY_STATS_TARGET_VCPU; + qemuMonitorQueryStatsProvider *provider = NULL; + g_autoptr(GPtrArray) providers = NULL; + g_autoptr(GPtrArray) queried_stats = NULL; + const char *success_str = "halt_poll_success_ns"; + const char *fail_str = "halt_poll_fail_ns"; + + provider = qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM); + provider->names = g_new0(char *, 3); + provider->names[0] = g_strdup(success_str),
&haltPollFail) < 0) provider->names[1] = g_strdup(fail_str);
I'm starting to wonder whether this is a good interface. These ->names[] array is never changed, so maybe we can have it as a NULL terminated list of constant strings? For instance:
provider = qemuMonitorQueryStatsProviderNew(); provider->names = {"halt_poll_success_ns", "halt_poll_fail_ns", NULL}; Well, cannot really assign char ** with a scalar initialization, but what might work is something like
const char *names[] = { success_str, fail_str, NULL };
provider = qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM); provider->names = g_strdupv((char **) names);
Yep, I made too much of a shortcut.
I think what Michal was trying to say is that since you do not change the array anywhere, there is no need for that to be a dynamically allocated array that needs to be freed. I, however, am not 100% if you are going to need this to be dynamic and whether you will be changing these arrays.
Yes. I'm not exactly sure why those strings have to be strdup-ed(). I mean, from the conceptual POV. They are not changed anywhere. And now that I think about it - why they have to be strings at all? They have to be strings at the monitor level, because that's what QEMU expects, however, I can imagine that in our code, in its upper layers those can be just part of enum. Or even better - a bitmask - just like virQEMUCaps. For instance:
qemu_monitor.h:
typedef enum { QEMU_MONITOR_QUERY_STATS_HALT_POLL_SUCCESS_NS, QEMU_MONITOR_QUERY_STATS_HALT_POLL_FAIL_NS,
QEMU_MONITOR_QUERY_STATS_LAST } qemuMonitorQueryStatsFlags;
qemu_monitor.c:
VIR_ENUM_DECL(qemuMonitorQueryStatsFlags); VIR_ENUM_IMPL(qemuMonitorQueryStatsFlags, QEMU_MONITOR_QUERY_STATS_LAST, "halt_poll_success_ns", "halt_poll_fail_ns" );
and when constructing the monitor command qemuMonitorQueryStatsFlagsTypeToString() translates those QEMU_MONITOR_QUERY_STATS_* enum members into their corresponding string representation.
Now, qemuMonitorQueryStatsProviderNew() could then behave like this:
provider = qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM); virBitmapSet(provider->names, QEMU_MONITOR_QUERY_STATS_HALT_POLL_SUCCESS_NS); virBitmapSet(provider->names, QEMU_MONITOR_QUERY_STATS_HALT_POLL_FAIL_NS);
Alternatively, we can have all of that in the qemuMonitorQueryStatsProviderNew() call with help of va_args, like this:
qemuMonitorQueryStatsProviderNew(QEMU_MONITOR_QUERY_STATS_PROVIDER_KVM, QEMU_MONITOR_QUERY_STATS_HALT_POLL_SUCCESS_NS, QEMU_MONITOR_QUERY_STATS_HALT_POLL_FAIL_NS, 0);
Then, when constructing the monitor command, virBitmapNextSetBit() could be used in a loop to iterate over set bits in combination with aforementioned qemuMonitorQueryStatsFlagsTypeToString() to translate bit index into string. 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
On Wed, Jun 29, 2022 at 09:03:33AM +0200, Michal Prívozník wrote: 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
participants (3)
-
Amneesh Singh -
Martin Kletzander -
Michal Prívozník