* [PATCH 2/8] kvm: Support for querying fd-based stats
2022-05-23 15:07 ` [PATCH 1/8] qmp: Support for querying stats Paolo Bonzini
@ 2022-05-23 15:07 ` Paolo Bonzini
2022-05-23 15:07 ` [PATCH 3/8] qmp: add filtering of statistics by target vCPU Paolo Bonzini
` (7 subsequent siblings)
8 siblings, 0 replies; 20+ messages in thread
From: Paolo Bonzini @ 2022-05-23 15:07 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Mark Kanda
From: Mark Kanda <mark.kanda@oracle.com>
Add support for querying fd-based KVM stats - as introduced by Linux kernel
commit:
cb082bfab59a ("KVM: stats: Add fd-based API to read binary stats data")
This allows the user to analyze the behavior of the VM without access
to debugfs.
Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
accel/kvm/kvm-all.c | 403 ++++++++++++++++++++++++++++++++++++++++++++
qapi/stats.json | 2 +-
2 files changed, 404 insertions(+), 1 deletion(-)
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 32e177bd26..6a6bbe2994 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -47,6 +47,7 @@
#include "kvm-cpus.h"
#include "hw/boards.h"
+#include "monitor/stats.h"
/* This check must be after config-host.h is included */
#ifdef CONFIG_EVENTFD
@@ -2310,6 +2311,9 @@ bool kvm_dirty_ring_enabled(void)
return kvm_state->kvm_dirty_ring_size ? true : false;
}
+static void query_stats_cb(StatsResultList **result, StatsTarget target, Error **errp);
+static void query_stats_schemas_cb(StatsSchemaList **result, Error **errp);
+
static int kvm_init(MachineState *ms)
{
MachineClass *mc = MACHINE_GET_CLASS(ms);
@@ -2638,6 +2642,10 @@ static int kvm_init(MachineState *ms)
}
}
+ if (kvm_check_extension(kvm_state, KVM_CAP_BINARY_STATS_FD)) {
+ add_stats_callbacks(query_stats_cb, query_stats_schemas_cb);
+ }
+
return 0;
err:
@@ -3697,3 +3705,398 @@ static void kvm_type_init(void)
}
type_init(kvm_type_init);
+
+typedef struct StatsArgs {
+ union StatsResultsType {
+ StatsResultList **stats;
+ StatsSchemaList **schema;
+ } result;
+ Error **errp;
+} StatsArgs;
+
+static StatsList *add_kvmstat_entry(struct kvm_stats_desc *pdesc,
+ uint64_t *stats_data,
+ StatsList *stats_list,
+ Error **errp)
+{
+
+ StatsList *stats_entry;
+ Stats *stats;
+ uint64List *val_list = NULL;
+
+ switch (pdesc->flags & KVM_STATS_TYPE_MASK) {
+ case KVM_STATS_TYPE_CUMULATIVE:
+ case KVM_STATS_TYPE_INSTANT:
+ case KVM_STATS_TYPE_PEAK:
+ case KVM_STATS_TYPE_LINEAR_HIST:
+ case KVM_STATS_TYPE_LOG_HIST:
+ break;
+ default:
+ return stats_list;
+ }
+
+ switch (pdesc->flags & KVM_STATS_UNIT_MASK) {
+ case KVM_STATS_UNIT_NONE:
+ case KVM_STATS_UNIT_BYTES:
+ case KVM_STATS_UNIT_CYCLES:
+ case KVM_STATS_UNIT_SECONDS:
+ break;
+ default:
+ return stats_list;
+ }
+
+ switch (pdesc->flags & KVM_STATS_BASE_MASK) {
+ case KVM_STATS_BASE_POW10:
+ case KVM_STATS_BASE_POW2:
+ break;
+ default:
+ return stats_list;
+ }
+
+ /* Alloc and populate data list */
+ stats_entry = g_new0(StatsList, 1);
+ stats = g_new0(Stats, 1);
+ stats->name = g_strdup(pdesc->name);
+ stats->value = g_new0(StatsValue, 1);;
+
+ if (pdesc->size == 1) {
+ stats->value->u.scalar = *stats_data;
+ stats->value->type = QTYPE_QNUM;
+ } else {
+ int i;
+ for (i = 0; i < pdesc->size; i++) {
+ uint64List *val_entry = g_new0(uint64List, 1);
+ val_entry->value = stats_data[i];
+ val_entry->next = val_list;
+ val_list = val_entry;
+ }
+ stats->value->u.list = val_list;
+ stats->value->type = QTYPE_QLIST;
+ }
+
+ stats_entry->value = stats;
+ stats_entry->next = stats_list;
+
+ return stats_entry;
+}
+
+static StatsSchemaValueList *add_kvmschema_entry(struct kvm_stats_desc *pdesc,
+ StatsSchemaValueList *list,
+ Error **errp)
+{
+ StatsSchemaValueList *schema_entry = g_new0(StatsSchemaValueList, 1);
+ schema_entry->value = g_new0(StatsSchemaValue, 1);
+
+ switch (pdesc->flags & KVM_STATS_TYPE_MASK) {
+ case KVM_STATS_TYPE_CUMULATIVE:
+ schema_entry->value->type = STATS_TYPE_CUMULATIVE;
+ break;
+ case KVM_STATS_TYPE_INSTANT:
+ schema_entry->value->type = STATS_TYPE_INSTANT;
+ break;
+ case KVM_STATS_TYPE_PEAK:
+ schema_entry->value->type = STATS_TYPE_PEAK;
+ break;
+ case KVM_STATS_TYPE_LINEAR_HIST:
+ schema_entry->value->type = STATS_TYPE_LINEAR_HISTOGRAM;
+ schema_entry->value->bucket_size = pdesc->bucket_size;
+ schema_entry->value->has_bucket_size = true;
+ break;
+ case KVM_STATS_TYPE_LOG_HIST:
+ schema_entry->value->type = STATS_TYPE_LOG2_HISTOGRAM;
+ break;
+ default:
+ goto exit;
+ }
+
+ switch (pdesc->flags & KVM_STATS_UNIT_MASK) {
+ case KVM_STATS_UNIT_NONE:
+ break;
+ case KVM_STATS_UNIT_BYTES:
+ schema_entry->value->has_unit = true;
+ schema_entry->value->unit = STATS_UNIT_BYTES;
+ break;
+ case KVM_STATS_UNIT_CYCLES:
+ schema_entry->value->has_unit = true;
+ schema_entry->value->unit = STATS_UNIT_CYCLES;
+ break;
+ case KVM_STATS_UNIT_SECONDS:
+ schema_entry->value->has_unit = true;
+ schema_entry->value->unit = STATS_UNIT_SECONDS;
+ break;
+ default:
+ goto exit;
+ }
+
+ schema_entry->value->exponent = pdesc->exponent;
+ if (pdesc->exponent) {
+ switch (pdesc->flags & KVM_STATS_BASE_MASK) {
+ case KVM_STATS_BASE_POW10:
+ schema_entry->value->has_base = true;
+ schema_entry->value->base = 10;
+ break;
+ case KVM_STATS_BASE_POW2:
+ schema_entry->value->has_base = true;
+ schema_entry->value->base = 2;
+ break;
+ default:
+ goto exit;
+ }
+ }
+
+ schema_entry->value->name = g_strdup(pdesc->name);
+ schema_entry->next = list;
+ return schema_entry;
+exit:
+ g_free(schema_entry->value);
+ g_free(schema_entry);
+ return list;
+}
+
+/* Cached stats descriptors */
+typedef struct StatsDescriptors {
+ char *ident; /* 'vm' or vCPU qom path */
+ struct kvm_stats_desc *kvm_stats_desc;
+ struct kvm_stats_header *kvm_stats_header;
+ QTAILQ_ENTRY(StatsDescriptors) next;
+} StatsDescriptors;
+
+static QTAILQ_HEAD(, StatsDescriptors) stats_descriptors =
+ QTAILQ_HEAD_INITIALIZER(stats_descriptors);
+
+static StatsDescriptors *find_stats_descriptors(StatsTarget target, int stats_fd,
+ Error **errp)
+{
+ StatsDescriptors *descriptors;
+ const char *ident;
+ struct kvm_stats_desc *kvm_stats_desc;
+ struct kvm_stats_header *kvm_stats_header;
+ size_t size_desc;
+ ssize_t ret;
+
+ switch (target) {
+ case STATS_TARGET_VM:
+ ident = StatsTarget_str(STATS_TARGET_VM);
+ break;
+ case STATS_TARGET_VCPU:
+ ident = current_cpu->parent_obj.canonical_path;
+ break;
+ default:
+ abort();
+ }
+
+ QTAILQ_FOREACH(descriptors, &stats_descriptors, next) {
+ if (g_str_equal(descriptors->ident, ident)) {
+ return descriptors;
+ }
+ }
+
+ descriptors = g_new0(StatsDescriptors, 1);
+
+ /* Read stats header */
+ kvm_stats_header = g_malloc(sizeof(*kvm_stats_header));
+ ret = read(stats_fd, kvm_stats_header, sizeof(*kvm_stats_header));
+ if (ret != sizeof(*kvm_stats_header)) {
+ error_setg(errp, "KVM stats: failed to read stats header: "
+ "expected %zu actual %zu",
+ sizeof(*kvm_stats_header), ret);
+ return NULL;
+ }
+ size_desc = sizeof(*kvm_stats_desc) + kvm_stats_header->name_size;
+
+ /* Read stats descriptors */
+ kvm_stats_desc = g_malloc0_n(kvm_stats_header->num_desc, size_desc);
+ ret = pread(stats_fd, kvm_stats_desc,
+ size_desc * kvm_stats_header->num_desc,
+ kvm_stats_header->desc_offset);
+
+ if (ret != size_desc * kvm_stats_header->num_desc) {
+ error_setg(errp, "KVM stats: failed to read stats descriptors: "
+ "expected %zu actual %zu",
+ size_desc * kvm_stats_header->num_desc, ret);
+ g_free(descriptors);
+ return NULL;
+ }
+ descriptors->kvm_stats_header = kvm_stats_header;
+ descriptors->kvm_stats_desc = kvm_stats_desc;
+ descriptors->ident = g_strdup(ident);
+ QTAILQ_INSERT_TAIL(&stats_descriptors, descriptors, next);
+ return descriptors;
+}
+
+static void query_stats(StatsResultList **result, StatsTarget target,
+ int stats_fd, Error **errp)
+{
+ struct kvm_stats_desc *kvm_stats_desc;
+ struct kvm_stats_header *kvm_stats_header;
+ StatsDescriptors *descriptors;
+ g_autofree uint64_t *stats_data = NULL;
+ struct kvm_stats_desc *pdesc;
+ StatsList *stats_list = NULL;
+ size_t size_desc, size_data = 0;
+ ssize_t ret;
+ int i;
+
+ descriptors = find_stats_descriptors(target, stats_fd, errp);
+ if (!descriptors) {
+ return;
+ }
+
+ kvm_stats_header = descriptors->kvm_stats_header;
+ kvm_stats_desc = descriptors->kvm_stats_desc;
+ size_desc = sizeof(*kvm_stats_desc) + kvm_stats_header->name_size;
+
+ /* Tally the total data size; read schema data */
+ for (i = 0; i < kvm_stats_header->num_desc; ++i) {
+ pdesc = (void *)kvm_stats_desc + i * size_desc;
+ size_data += pdesc->size * sizeof(*stats_data);
+ }
+
+ stats_data = g_malloc0(size_data);
+ ret = pread(stats_fd, stats_data, size_data, kvm_stats_header->data_offset);
+
+ if (ret != size_data) {
+ error_setg(errp, "KVM stats: failed to read data: "
+ "expected %zu actual %zu", size_data, ret);
+ return;
+ }
+
+ for (i = 0; i < kvm_stats_header->num_desc; ++i) {
+ uint64_t *stats;
+ pdesc = (void *)kvm_stats_desc + i * size_desc;
+
+ /* Add entry to the list */
+ stats = (void *)stats_data + pdesc->offset;
+ stats_list = add_kvmstat_entry(pdesc, stats, stats_list, errp);
+ }
+
+ if (!stats_list) {
+ return;
+ }
+
+ switch (target) {
+ case STATS_TARGET_VM:
+ add_stats_entry(result, STATS_PROVIDER_KVM, NULL, stats_list);
+ break;
+ case STATS_TARGET_VCPU:
+ add_stats_entry(result, STATS_PROVIDER_KVM,
+ current_cpu->parent_obj.canonical_path,
+ stats_list);
+ break;
+ default:
+ break;
+ }
+}
+
+static void query_stats_schema(StatsSchemaList **result, StatsTarget target,
+ int stats_fd, Error **errp)
+{
+ struct kvm_stats_desc *kvm_stats_desc;
+ struct kvm_stats_header *kvm_stats_header;
+ StatsDescriptors *descriptors;
+ struct kvm_stats_desc *pdesc;
+ StatsSchemaValueList *stats_list = NULL;
+ size_t size_desc;
+ int i;
+
+ descriptors = find_stats_descriptors(target, stats_fd, errp);
+ if (!descriptors) {
+ return;
+ }
+
+ kvm_stats_header = descriptors->kvm_stats_header;
+ kvm_stats_desc = descriptors->kvm_stats_desc;
+ size_desc = sizeof(*kvm_stats_desc) + kvm_stats_header->name_size;
+
+ /* Tally the total data size; read schema data */
+ for (i = 0; i < kvm_stats_header->num_desc; ++i) {
+ pdesc = (void *)kvm_stats_desc + i * size_desc;
+ stats_list = add_kvmschema_entry(pdesc, stats_list, errp);
+ }
+
+ add_stats_schema(result, STATS_PROVIDER_KVM, target, stats_list);
+}
+
+static void query_stats_vcpu(CPUState *cpu, run_on_cpu_data data)
+{
+ StatsArgs *kvm_stats_args = (StatsArgs *) data.host_ptr;
+ int stats_fd = kvm_vcpu_ioctl(cpu, KVM_GET_STATS_FD, NULL);
+ Error *local_err = NULL;
+
+ if (stats_fd == -1) {
+ error_setg(&local_err, "KVM stats: ioctl failed");
+ error_propagate(kvm_stats_args->errp, local_err);
+ return;
+ }
+ query_stats(kvm_stats_args->result.stats, STATS_TARGET_VCPU, stats_fd,
+ kvm_stats_args->errp);
+ close(stats_fd);
+}
+
+static void query_stats_schema_vcpu(CPUState *cpu, run_on_cpu_data data)
+{
+ StatsArgs *kvm_stats_args = (StatsArgs *) data.host_ptr;
+ int stats_fd = kvm_vcpu_ioctl(cpu, KVM_GET_STATS_FD, NULL);
+ Error *local_err = NULL;
+
+ if (stats_fd == -1) {
+ error_setg(&local_err, "KVM stats: ioctl failed");
+ error_propagate(kvm_stats_args->errp, local_err);
+ return;
+ }
+ query_stats_schema(kvm_stats_args->result.schema, STATS_TARGET_VCPU, stats_fd,
+ kvm_stats_args->errp);
+ close(stats_fd);
+}
+
+static void query_stats_cb(StatsResultList **result, StatsTarget target, Error **errp)
+{
+ KVMState *s = kvm_state;
+ CPUState *cpu;
+ int stats_fd;
+
+ switch (target) {
+ case STATS_TARGET_VM:
+ {
+ stats_fd = kvm_vm_ioctl(s, KVM_GET_STATS_FD, NULL);
+ if (stats_fd == -1) {
+ error_setg(errp, "KVM stats: ioctl failed");
+ return;
+ }
+ query_stats(result, target, stats_fd, errp);
+ close(stats_fd);
+ break;
+ }
+ case STATS_TARGET_VCPU:
+ {
+ StatsArgs stats_args;
+ stats_args.result.stats = result;
+ stats_args.errp = errp;
+ CPU_FOREACH(cpu) {
+ run_on_cpu(cpu, query_stats_vcpu, RUN_ON_CPU_HOST_PTR(&stats_args));
+ }
+ break;
+ }
+ default:
+ break;
+ }
+}
+
+void query_stats_schemas_cb(StatsSchemaList **result, Error **errp)
+{
+ StatsArgs stats_args;
+ KVMState *s = kvm_state;
+ int stats_fd;
+
+ stats_fd = kvm_vm_ioctl(s, KVM_GET_STATS_FD, NULL);
+ if (stats_fd == -1) {
+ error_setg(errp, "KVM stats: ioctl failed");
+ return;
+ }
+ query_stats_schema(result, STATS_TARGET_VM, stats_fd, errp);
+ close(stats_fd);
+
+ stats_args.result.schema = result;
+ stats_args.errp = errp;
+ run_on_cpu(first_cpu, query_stats_schema_vcpu, RUN_ON_CPU_HOST_PTR(&stats_args));
+}
diff --git a/qapi/stats.json b/qapi/stats.json
index 650d883297..1129ba49bc 100644
--- a/qapi/stats.json
+++ b/qapi/stats.json
@@ -51,7 +51,7 @@
# Since: 7.1
##
{ 'enum': 'StatsProvider',
- 'data': [ ] }
+ 'data': [ 'kvm' ] }
##
# @StatsTarget:
--
2.36.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 3/8] qmp: add filtering of statistics by target vCPU
2022-05-23 15:07 ` [PATCH 1/8] qmp: Support for querying stats Paolo Bonzini
2022-05-23 15:07 ` [PATCH 2/8] kvm: Support for querying fd-based stats Paolo Bonzini
@ 2022-05-23 15:07 ` Paolo Bonzini
2022-05-24 11:20 ` Markus Armbruster
2022-05-23 15:07 ` [PATCH 4/8] hmp: add basic "info stats" implementation Paolo Bonzini
` (6 subsequent siblings)
8 siblings, 1 reply; 20+ messages in thread
From: Paolo Bonzini @ 2022-05-23 15:07 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Mark Kanda
Introduce a simple filtering of statistics, that allows to retrieve
statistics for a subset of the guest vCPUs. This will be used for
example by the HMP monitor, in order to retrieve the statistics
for the currently selected CPU.
Example:
{ "execute": "query-stats",
"arguments": {
"target": "vcpu",
"vcpus": [ "/machine/unattached/device[2]",
"/machine/unattached/device[4]" ] } }
Extracted from a patch by Mark Kanda.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
v3->v4: renamed str_in_list to apply_str_list_filter
handle empty filter early by avoiding the query altogether
accel/kvm/kvm-all.c | 9 +++++++--
include/monitor/stats.h | 11 ++++++++++-
monitor/qmp-cmds.c | 34 +++++++++++++++++++++++++++++++++-
qapi/stats.json | 24 +++++++++++++++++++-----
4 files changed, 69 insertions(+), 9 deletions(-)
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 6a6bbe2994..3ee431a431 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2311,7 +2311,8 @@ bool kvm_dirty_ring_enabled(void)
return kvm_state->kvm_dirty_ring_size ? true : false;
}
-static void query_stats_cb(StatsResultList **result, StatsTarget target, Error **errp);
+static void query_stats_cb(StatsResultList **result, StatsTarget target,
+ strList *targets, Error **errp);
static void query_stats_schemas_cb(StatsSchemaList **result, Error **errp);
static int kvm_init(MachineState *ms)
@@ -4049,7 +4050,8 @@ static void query_stats_schema_vcpu(CPUState *cpu, run_on_cpu_data data)
close(stats_fd);
}
-static void query_stats_cb(StatsResultList **result, StatsTarget target, Error **errp)
+static void query_stats_cb(StatsResultList **result, StatsTarget target,
+ strList *targets, Error **errp)
{
KVMState *s = kvm_state;
CPUState *cpu;
@@ -4073,6 +4075,9 @@ static void query_stats_cb(StatsResultList **result, StatsTarget target, Error *
stats_args.result.stats = result;
stats_args.errp = errp;
CPU_FOREACH(cpu) {
+ if (!apply_str_list_filter(cpu->parent_obj.canonical_path, targets)) {
+ continue;
+ }
run_on_cpu(cpu, query_stats_vcpu, RUN_ON_CPU_HOST_PTR(&stats_args));
}
break;
diff --git a/include/monitor/stats.h b/include/monitor/stats.h
index 912eeadb2f..8c50feeaa9 100644
--- a/include/monitor/stats.h
+++ b/include/monitor/stats.h
@@ -11,7 +11,7 @@
#include "qapi/qapi-types-stats.h"
typedef void StatRetrieveFunc(StatsResultList **result, StatsTarget target,
- Error **errp);
+ strList *targets, Error **errp);
typedef void SchemaRetrieveFunc(StatsSchemaList **result, Error **errp);
/*
@@ -31,4 +31,13 @@ void add_stats_entry(StatsResultList **, StatsProvider, const char *id,
void add_stats_schema(StatsSchemaList **, StatsProvider, StatsTarget,
StatsSchemaValueList *);
+/*
+ * True if a string matches the filter passed to the stats_fn callabck,
+ * false otherwise.
+ *
+ * Note that an empty list means no filtering, i.e. all strings will
+ * return true.
+ */
+bool apply_str_list_filter(const char *string, strList *list);
+
#endif /* STATS_H */
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index d65c5f0257..ebf6f49446 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -466,11 +466,28 @@ void add_stats_callbacks(StatRetrieveFunc *stats_fn,
StatsResultList *qmp_query_stats(StatsFilter *filter, Error **errp)
{
StatsResultList *stats_results = NULL;
+ strList *targets = NULL;
StatsCallbacks *entry;
ERRP_GUARD();
+ switch (filter->target) {
+ case STATS_TARGET_VM:
+ break;
+ case STATS_TARGET_VCPU:
+ if (filter->u.vcpu.has_vcpus) {
+ if (!filter->u.vcpu.vcpus) {
+ /* No targets allowed? Return no statistics. */
+ return NULL;
+ }
+ targets = filter->u.vcpu.vcpus;
+ }
+ break;
+ default:
+ abort();
+ }
+
QTAILQ_FOREACH(entry, &stats_callbacks, next) {
- entry->stats_cb(&stats_results, filter->target, errp);
+ entry->stats_cb(&stats_results, filter->target, targets, errp);
if (*errp) {
qapi_free_StatsResultList(stats_results);
return NULL;
@@ -523,3 +540,18 @@ void add_stats_schema(StatsSchemaList **schema_results,
entry->stats = stats_list;
QAPI_LIST_PREPEND(*schema_results, entry);
}
+
+bool apply_str_list_filter(const char *string, strList *list)
+{
+ strList *str_list = NULL;
+
+ if (!list) {
+ return true;
+ }
+ for (str_list = list; str_list; str_list = str_list->next) {
+ if (g_str_equal(string, str_list->value)) {
+ return true;
+ }
+ }
+ return false;
+}
diff --git a/qapi/stats.json b/qapi/stats.json
index 1129ba49bc..e8ed907793 100644
--- a/qapi/stats.json
+++ b/qapi/stats.json
@@ -69,15 +69,29 @@
'data': [ 'vm', 'vcpu' ] }
##
-# @StatsFilter:
+# @StatsVCPUFilter:
#
-# The arguments to the query-stats command; specifies a target for which to
-# request statistics.
+# @vcpus: list of QOM paths for the desired vCPU objects.
#
# Since: 7.1
##
-{ 'struct': 'StatsFilter',
- 'data': { 'target': 'StatsTarget' } }
+{ 'struct': 'StatsVCPUFilter',
+ 'data': { '*vcpus': [ 'str' ] } }
+
+##
+# @StatsFilter:
+#
+# The arguments to the query-stats command; specifies a target for which to
+# request statistics and optionally the required subset of information for
+# that target:
+# - which vCPUs to request statistics for
+#
+# Since: 7.1
+##
+{ 'union': 'StatsFilter',
+ 'base': { 'target': 'StatsTarget' },
+ 'discriminator': 'target',
+ 'data': { 'vcpu': 'StatsVCPUFilter' } }
##
# @StatsValue:
--
2.36.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 3/8] qmp: add filtering of statistics by target vCPU
2022-05-23 15:07 ` [PATCH 3/8] qmp: add filtering of statistics by target vCPU Paolo Bonzini
@ 2022-05-24 11:20 ` Markus Armbruster
0 siblings, 0 replies; 20+ messages in thread
From: Markus Armbruster @ 2022-05-24 11:20 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, Mark Kanda
Paolo Bonzini <pbonzini@redhat.com> writes:
> Introduce a simple filtering of statistics, that allows to retrieve
> statistics for a subset of the guest vCPUs. This will be used for
> example by the HMP monitor, in order to retrieve the statistics
> for the currently selected CPU.
>
> Example:
> { "execute": "query-stats",
> "arguments": {
> "target": "vcpu",
> "vcpus": [ "/machine/unattached/device[2]",
> "/machine/unattached/device[4]" ] } }
>
> Extracted from a patch by Mark Kanda.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 4/8] hmp: add basic "info stats" implementation
2022-05-23 15:07 ` [PATCH 1/8] qmp: Support for querying stats Paolo Bonzini
2022-05-23 15:07 ` [PATCH 2/8] kvm: Support for querying fd-based stats Paolo Bonzini
2022-05-23 15:07 ` [PATCH 3/8] qmp: add filtering of statistics by target vCPU Paolo Bonzini
@ 2022-05-23 15:07 ` Paolo Bonzini
2022-05-24 17:22 ` Mark Kanda
2022-05-23 15:07 ` [PATCH 5/8] qmp: add filtering of statistics by provider Paolo Bonzini
` (5 subsequent siblings)
8 siblings, 1 reply; 20+ messages in thread
From: Paolo Bonzini @ 2022-05-23 15:07 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Mark Kanda
From: Mark Kanda <mark.kanda@oracle.com>
Add an HMP command to retrieve statistics collected at run-time.
The command will retrieve and print either all VM-level statistics,
or all vCPU-level statistics for the currently selected CPU.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hmp-commands-info.hx | 13 +++
include/monitor/hmp.h | 1 +
monitor/hmp-cmds.c | 187 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 201 insertions(+)
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index adfa085a9b..221feab8c0 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -894,3 +894,16 @@ SRST
``info via``
Show guest mos6522 VIA devices.
ERST
+
+ {
+ .name = "stats",
+ .args_type = "target:s",
+ .params = "target",
+ .help = "show statistics; target is either vm or vcpu",
+ .cmd = hmp_info_stats,
+ },
+
+SRST
+ ``stats``
+ Show runtime-collected statistics
+ERST
diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index 96d014826a..2e89a97bd6 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -133,5 +133,6 @@ void hmp_info_dirty_rate(Monitor *mon, const QDict *qdict);
void hmp_calc_dirty_rate(Monitor *mon, const QDict *qdict);
void hmp_human_readable_text_helper(Monitor *mon,
HumanReadableText *(*qmp_handler)(Error **));
+void hmp_info_stats(Monitor *mon, const QDict *qdict);
#endif
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 622c783c32..c0cb440902 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -40,6 +40,7 @@
#include "qapi/qapi-commands-pci.h"
#include "qapi/qapi-commands-rocker.h"
#include "qapi/qapi-commands-run-state.h"
+#include "qapi/qapi-commands-stats.h"
#include "qapi/qapi-commands-tpm.h"
#include "qapi/qapi-commands-ui.h"
#include "qapi/qapi-visit-net.h"
@@ -52,6 +53,7 @@
#include "ui/console.h"
#include "qemu/cutils.h"
#include "qemu/error-report.h"
+#include "hw/core/cpu.h"
#include "hw/intc/intc.h"
#include "migration/snapshot.h"
#include "migration/misc.h"
@@ -2239,3 +2241,188 @@ void hmp_info_memory_size_summary(Monitor *mon, const QDict *qdict)
}
hmp_handle_error(mon, err);
}
+
+static void print_stats_schema_value(Monitor *mon, StatsSchemaValue *value)
+{
+ const char *prefix = "";
+ monitor_printf(mon, " %s (%s", value->name, StatsType_str(value->type));
+
+ if (value->has_unit && value->unit == STATS_UNIT_SECONDS &&
+ (value->exponent == 0 || value->base == 10) &&
+ value->exponent >= -9 && value->exponent <= 0 &&
+ value->exponent % 3 == 0) {
+
+ static const char *si_prefix[] = { "", "milli", "micro", "nano" };
+ prefix = si_prefix[value->exponent / -3];
+
+ } else if (value->has_unit && value->unit == STATS_UNIT_BYTES &&
+ (value->exponent == 0 || value->base == 2) &&
+ value->exponent >= 0 && value->exponent <= 40 &&
+ value->exponent % 10 == 0) {
+
+ static const char *si_prefix[] = {
+ "", "kilo", "mega", "giga", "tera" };
+ prefix = si_prefix[value->exponent / 10];
+
+ } else if (value->exponent) {
+ /* Print the base and exponent as "x <base>^<exp>" */
+ monitor_printf(mon, ", * %d^%d", value->base,
+ value->exponent);
+ }
+
+ if (value->has_unit) {
+ monitor_printf(mon, " %s%s", prefix, StatsUnit_str(value->unit));
+ }
+
+ /* Print bucket size for linear histograms */
+ if (value->type == STATS_TYPE_LINEAR_HISTOGRAM && value->has_bucket_size) {
+ monitor_printf(mon, ", bucket size=%d", value->bucket_size);
+ }
+ monitor_printf(mon, ")");
+}
+
+static StatsSchemaValueList *find_schema_value_list(
+ StatsSchemaList *list, StatsProvider provider,
+ StatsTarget target)
+{
+ StatsSchemaList *node;
+
+ for (node = list; node; node = node->next) {
+ if (node->value->provider == provider &&
+ node->value->target == target) {
+ return node->value->stats;
+ }
+ }
+ return NULL;
+}
+
+static void print_stats_results(Monitor *mon, StatsTarget target,
+ StatsResult *result,
+ StatsSchemaList *schema)
+{
+ /* Find provider schema */
+ StatsSchemaValueList *schema_value_list =
+ find_schema_value_list(schema, result->provider, target);
+ StatsList *stats_list;
+
+ if (!schema_value_list) {
+ monitor_printf(mon, "failed to find schema list for %s\n",
+ StatsProvider_str(result->provider));
+ return;
+ }
+
+ monitor_printf(mon, "provider: %s\n",
+ StatsProvider_str(result->provider));
+
+ for (stats_list = result->stats; stats_list;
+ stats_list = stats_list->next,
+ schema_value_list = schema_value_list->next) {
+
+ Stats *stats = stats_list->value;
+ StatsValue *stats_value = stats->value;
+ StatsSchemaValue *schema_value = schema_value_list->value;
+
+ /* Find schema entry */
+ while (!g_str_equal(stats->name, schema_value->name)) {
+ if (!schema_value_list->next) {
+ monitor_printf(mon, "failed to find schema entry for %s\n",
+ stats->name);
+ return;
+ }
+ schema_value_list = schema_value_list->next;
+ schema_value = schema_value_list->value;
+ }
+
+ print_stats_schema_value(mon, schema_value);
+
+ if (stats_value->type == QTYPE_QNUM) {
+ monitor_printf(mon, ": %" PRId64 "\n", stats_value->u.scalar);
+ } else if (stats_value->type == QTYPE_QLIST) {
+ uint64List *list;
+ int i;
+
+ monitor_printf(mon, ": ");
+ for (list = stats_value->u.list, i = 1;
+ list;
+ list = list->next, i++) {
+ monitor_printf(mon, "[%d]=%" PRId64 " ", i, list->value);
+ }
+ monitor_printf(mon, "\n");
+ }
+ }
+}
+
+/* Create the StatsFilter that is needed for an "info stats" invocation. */
+static StatsFilter *stats_filter(StatsTarget target, int cpu_index)
+{
+ StatsFilter *filter = g_malloc0(sizeof(*filter));
+
+ filter->target = target;
+ switch (target) {
+ case STATS_TARGET_VM:
+ break;
+ case STATS_TARGET_VCPU:
+ {
+ strList *vcpu_list = NULL;
+ CPUState *cpu = qemu_get_cpu(cpu_index);
+ char *canonical_path = object_get_canonical_path(OBJECT(cpu));
+
+ QAPI_LIST_PREPEND(vcpu_list, canonical_path);
+ filter->u.vcpu.has_vcpus = true;
+ filter->u.vcpu.vcpus = vcpu_list;
+ break;
+ }
+ default:
+ break;
+ }
+ return filter;
+}
+
+void hmp_info_stats(Monitor *mon, const QDict *qdict)
+{
+ const char *target_str = qdict_get_str(qdict, "target");
+ StatsTarget target;
+ Error *err = NULL;
+ g_autoptr(StatsSchemaList) schema = NULL;
+ g_autoptr(StatsResultList) stats = NULL;
+ g_autoptr(StatsFilter) filter = NULL;
+ StatsResultList *entry;
+
+ target = qapi_enum_parse(&StatsTarget_lookup, target_str, -1, &err);
+ if (err) {
+ monitor_printf(mon, "invalid stats target %s\n", target_str);
+ goto exit_no_print;
+ }
+
+ schema = qmp_query_stats_schemas(&err);
+ if (err) {
+ goto exit;
+ }
+
+ switch (target) {
+ case STATS_TARGET_VM:
+ filter = stats_filter(target, -1);
+ break;
+ case STATS_TARGET_VCPU: {}
+ int cpu_index = monitor_get_cpu_index(mon);
+ filter = stats_filter(target, cpu_index);
+ break;
+ default:
+ abort();
+ }
+
+ stats = qmp_query_stats(filter, &err);
+ if (err) {
+ goto exit;
+ }
+ for (entry = stats; entry; entry = entry->next) {
+ print_stats_results(mon, target, entry->value, schema);
+ }
+
+exit:
+ if (err) {
+ monitor_printf(mon, "%s\n", error_get_pretty(err));
+ }
+exit_no_print:
+ error_free(err);
+}
--
2.36.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 4/8] hmp: add basic "info stats" implementation
2022-05-23 15:07 ` [PATCH 4/8] hmp: add basic "info stats" implementation Paolo Bonzini
@ 2022-05-24 17:22 ` Mark Kanda
0 siblings, 0 replies; 20+ messages in thread
From: Mark Kanda @ 2022-05-24 17:22 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: armbru
On 5/23/2022 10:07 AM, Paolo Bonzini wrote:
> From: Mark Kanda <mark.kanda@oracle.com>
>
> Add an HMP command to retrieve statistics collected at run-time.
> The command will retrieve and print either all VM-level statistics,
> or all vCPU-level statistics for the currently selected CPU.
>
As I'm credited as the 'poster' (thank you Paolo), I assume this should be added:
Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
Thanks/regards,
-Mark
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hmp-commands-info.hx | 13 +++
> include/monitor/hmp.h | 1 +
> monitor/hmp-cmds.c | 187 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 201 insertions(+)
>
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index adfa085a9b..221feab8c0 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -894,3 +894,16 @@ SRST
> ``info via``
> Show guest mos6522 VIA devices.
> ERST
> +
> + {
> + .name = "stats",
> + .args_type = "target:s",
> + .params = "target",
> + .help = "show statistics; target is either vm or vcpu",
> + .cmd = hmp_info_stats,
> + },
> +
> +SRST
> + ``stats``
> + Show runtime-collected statistics
> +ERST
> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
> index 96d014826a..2e89a97bd6 100644
> --- a/include/monitor/hmp.h
> +++ b/include/monitor/hmp.h
> @@ -133,5 +133,6 @@ void hmp_info_dirty_rate(Monitor *mon, const QDict *qdict);
> void hmp_calc_dirty_rate(Monitor *mon, const QDict *qdict);
> void hmp_human_readable_text_helper(Monitor *mon,
> HumanReadableText *(*qmp_handler)(Error **));
> +void hmp_info_stats(Monitor *mon, const QDict *qdict);
>
> #endif
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 622c783c32..c0cb440902 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -40,6 +40,7 @@
> #include "qapi/qapi-commands-pci.h"
> #include "qapi/qapi-commands-rocker.h"
> #include "qapi/qapi-commands-run-state.h"
> +#include "qapi/qapi-commands-stats.h"
> #include "qapi/qapi-commands-tpm.h"
> #include "qapi/qapi-commands-ui.h"
> #include "qapi/qapi-visit-net.h"
> @@ -52,6 +53,7 @@
> #include "ui/console.h"
> #include "qemu/cutils.h"
> #include "qemu/error-report.h"
> +#include "hw/core/cpu.h"
> #include "hw/intc/intc.h"
> #include "migration/snapshot.h"
> #include "migration/misc.h"
> @@ -2239,3 +2241,188 @@ void hmp_info_memory_size_summary(Monitor *mon, const QDict *qdict)
> }
> hmp_handle_error(mon, err);
> }
> +
> +static void print_stats_schema_value(Monitor *mon, StatsSchemaValue *value)
> +{
> + const char *prefix = "";
> + monitor_printf(mon, " %s (%s", value->name, StatsType_str(value->type));
> +
> + if (value->has_unit && value->unit == STATS_UNIT_SECONDS &&
> + (value->exponent == 0 || value->base == 10) &&
> + value->exponent >= -9 && value->exponent <= 0 &&
> + value->exponent % 3 == 0) {
> +
> + static const char *si_prefix[] = { "", "milli", "micro", "nano" };
> + prefix = si_prefix[value->exponent / -3];
> +
> + } else if (value->has_unit && value->unit == STATS_UNIT_BYTES &&
> + (value->exponent == 0 || value->base == 2) &&
> + value->exponent >= 0 && value->exponent <= 40 &&
> + value->exponent % 10 == 0) {
> +
> + static const char *si_prefix[] = {
> + "", "kilo", "mega", "giga", "tera" };
> + prefix = si_prefix[value->exponent / 10];
> +
> + } else if (value->exponent) {
> + /* Print the base and exponent as "x <base>^<exp>" */
> + monitor_printf(mon, ", * %d^%d", value->base,
> + value->exponent);
> + }
> +
> + if (value->has_unit) {
> + monitor_printf(mon, " %s%s", prefix, StatsUnit_str(value->unit));
> + }
> +
> + /* Print bucket size for linear histograms */
> + if (value->type == STATS_TYPE_LINEAR_HISTOGRAM && value->has_bucket_size) {
> + monitor_printf(mon, ", bucket size=%d", value->bucket_size);
> + }
> + monitor_printf(mon, ")");
> +}
> +
> +static StatsSchemaValueList *find_schema_value_list(
> + StatsSchemaList *list, StatsProvider provider,
> + StatsTarget target)
> +{
> + StatsSchemaList *node;
> +
> + for (node = list; node; node = node->next) {
> + if (node->value->provider == provider &&
> + node->value->target == target) {
> + return node->value->stats;
> + }
> + }
> + return NULL;
> +}
> +
> +static void print_stats_results(Monitor *mon, StatsTarget target,
> + StatsResult *result,
> + StatsSchemaList *schema)
> +{
> + /* Find provider schema */
> + StatsSchemaValueList *schema_value_list =
> + find_schema_value_list(schema, result->provider, target);
> + StatsList *stats_list;
> +
> + if (!schema_value_list) {
> + monitor_printf(mon, "failed to find schema list for %s\n",
> + StatsProvider_str(result->provider));
> + return;
> + }
> +
> + monitor_printf(mon, "provider: %s\n",
> + StatsProvider_str(result->provider));
> +
> + for (stats_list = result->stats; stats_list;
> + stats_list = stats_list->next,
> + schema_value_list = schema_value_list->next) {
> +
> + Stats *stats = stats_list->value;
> + StatsValue *stats_value = stats->value;
> + StatsSchemaValue *schema_value = schema_value_list->value;
> +
> + /* Find schema entry */
> + while (!g_str_equal(stats->name, schema_value->name)) {
> + if (!schema_value_list->next) {
> + monitor_printf(mon, "failed to find schema entry for %s\n",
> + stats->name);
> + return;
> + }
> + schema_value_list = schema_value_list->next;
> + schema_value = schema_value_list->value;
> + }
> +
> + print_stats_schema_value(mon, schema_value);
> +
> + if (stats_value->type == QTYPE_QNUM) {
> + monitor_printf(mon, ": %" PRId64 "\n", stats_value->u.scalar);
> + } else if (stats_value->type == QTYPE_QLIST) {
> + uint64List *list;
> + int i;
> +
> + monitor_printf(mon, ": ");
> + for (list = stats_value->u.list, i = 1;
> + list;
> + list = list->next, i++) {
> + monitor_printf(mon, "[%d]=%" PRId64 " ", i, list->value);
> + }
> + monitor_printf(mon, "\n");
> + }
> + }
> +}
> +
> +/* Create the StatsFilter that is needed for an "info stats" invocation. */
> +static StatsFilter *stats_filter(StatsTarget target, int cpu_index)
> +{
> + StatsFilter *filter = g_malloc0(sizeof(*filter));
> +
> + filter->target = target;
> + switch (target) {
> + case STATS_TARGET_VM:
> + break;
> + case STATS_TARGET_VCPU:
> + {
> + strList *vcpu_list = NULL;
> + CPUState *cpu = qemu_get_cpu(cpu_index);
> + char *canonical_path = object_get_canonical_path(OBJECT(cpu));
> +
> + QAPI_LIST_PREPEND(vcpu_list, canonical_path);
> + filter->u.vcpu.has_vcpus = true;
> + filter->u.vcpu.vcpus = vcpu_list;
> + break;
> + }
> + default:
> + break;
> + }
> + return filter;
> +}
> +
> +void hmp_info_stats(Monitor *mon, const QDict *qdict)
> +{
> + const char *target_str = qdict_get_str(qdict, "target");
> + StatsTarget target;
> + Error *err = NULL;
> + g_autoptr(StatsSchemaList) schema = NULL;
> + g_autoptr(StatsResultList) stats = NULL;
> + g_autoptr(StatsFilter) filter = NULL;
> + StatsResultList *entry;
> +
> + target = qapi_enum_parse(&StatsTarget_lookup, target_str, -1, &err);
> + if (err) {
> + monitor_printf(mon, "invalid stats target %s\n", target_str);
> + goto exit_no_print;
> + }
> +
> + schema = qmp_query_stats_schemas(&err);
> + if (err) {
> + goto exit;
> + }
> +
> + switch (target) {
> + case STATS_TARGET_VM:
> + filter = stats_filter(target, -1);
> + break;
> + case STATS_TARGET_VCPU: {}
> + int cpu_index = monitor_get_cpu_index(mon);
> + filter = stats_filter(target, cpu_index);
> + break;
> + default:
> + abort();
> + }
> +
> + stats = qmp_query_stats(filter, &err);
> + if (err) {
> + goto exit;
> + }
> + for (entry = stats; entry; entry = entry->next) {
> + print_stats_results(mon, target, entry->value, schema);
> + }
> +
> +exit:
> + if (err) {
> + monitor_printf(mon, "%s\n", error_get_pretty(err));
> + }
> +exit_no_print:
> + error_free(err);
> +}
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 5/8] qmp: add filtering of statistics by provider
2022-05-23 15:07 ` [PATCH 1/8] qmp: Support for querying stats Paolo Bonzini
` (2 preceding siblings ...)
2022-05-23 15:07 ` [PATCH 4/8] hmp: add basic "info stats" implementation Paolo Bonzini
@ 2022-05-23 15:07 ` Paolo Bonzini
2022-05-24 12:32 ` Markus Armbruster
2022-05-23 15:07 ` [PATCH 6/8] hmp: " Paolo Bonzini
` (4 subsequent siblings)
8 siblings, 1 reply; 20+ messages in thread
From: Paolo Bonzini @ 2022-05-23 15:07 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Mark Kanda
Allow retrieving the statistics from a specific provider only.
This can be used in the future by HMP commands such as "info
sync-profile" or "info profile". The next patch also adds
filter-by-provider capabilities to the HMP equivalent of
query-stats, "info stats".
Example:
{ "execute": "query-stats",
"arguments": {
"target": "vm",
"providers": [
{ "provider": "kvm" } ] } }
The QAPI is a bit more verbose than just a list of StatsProvider,
so that it can be subsequently extended with filtering of statistics
by name.
Extracted from a patch by Mark Kanda.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
accel/kvm/kvm-all.c | 3 ++-
include/monitor/stats.h | 4 +++-
monitor/hmp-cmds.c | 2 +-
monitor/qmp-cmds.c | 45 ++++++++++++++++++++++++++++++++---------
qapi/stats.json | 17 ++++++++++++++--
5 files changed, 56 insertions(+), 15 deletions(-)
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 3ee431a431..461b6cf927 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2644,7 +2644,8 @@ static int kvm_init(MachineState *ms)
}
if (kvm_check_extension(kvm_state, KVM_CAP_BINARY_STATS_FD)) {
- add_stats_callbacks(query_stats_cb, query_stats_schemas_cb);
+ add_stats_callbacks(STATS_PROVIDER_KVM, query_stats_cb,
+ query_stats_schemas_cb);
}
return 0;
diff --git a/include/monitor/stats.h b/include/monitor/stats.h
index 8c50feeaa9..840c4ed08e 100644
--- a/include/monitor/stats.h
+++ b/include/monitor/stats.h
@@ -17,10 +17,12 @@ typedef void SchemaRetrieveFunc(StatsSchemaList **result, Error **errp);
/*
* Register callbacks for the QMP query-stats command.
*
+ * @provider: stats provider
* @stats_fn: routine to query stats:
* @schema_fn: routine to query stat schemas:
*/
-void add_stats_callbacks(StatRetrieveFunc *stats_fn,
+void add_stats_callbacks(StatsProvider provider,
+ StatRetrieveFunc *stats_fn,
SchemaRetrieveFunc *schemas_fn);
/*
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index c0cb440902..8d2c4792d5 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -2394,7 +2394,7 @@ void hmp_info_stats(Monitor *mon, const QDict *qdict)
goto exit_no_print;
}
- schema = qmp_query_stats_schemas(&err);
+ schema = qmp_query_stats_schemas(false, 0, &err);
if (err) {
goto exit;
}
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index ebf6f49446..7be0e7414a 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -445,6 +445,7 @@ HumanReadableText *qmp_x_query_irq(Error **errp)
}
typedef struct StatsCallbacks {
+ StatsProvider provider;
StatRetrieveFunc *stats_cb;
SchemaRetrieveFunc *schemas_cb;
QTAILQ_ENTRY(StatsCallbacks) next;
@@ -453,16 +454,34 @@ typedef struct StatsCallbacks {
static QTAILQ_HEAD(, StatsCallbacks) stats_callbacks =
QTAILQ_HEAD_INITIALIZER(stats_callbacks);
-void add_stats_callbacks(StatRetrieveFunc *stats_fn,
+void add_stats_callbacks(StatsProvider provider,
+ StatRetrieveFunc *stats_fn,
SchemaRetrieveFunc *schemas_fn)
{
StatsCallbacks *entry = g_new(StatsCallbacks, 1);
+ entry->provider = provider;
entry->stats_cb = stats_fn;
entry->schemas_cb = schemas_fn;
QTAILQ_INSERT_TAIL(&stats_callbacks, entry, next);
}
+static bool stats_provider_requested(StatsProvider provider,
+ StatsFilter *filter)
+{
+ StatsRequestList *request;
+
+ if (!filter->has_providers) {
+ return true;
+ }
+ for (request = filter->providers; request; request = request->next) {
+ if (request->value->provider == provider) {
+ return true;
+ }
+ }
+ return false;
+}
+
StatsResultList *qmp_query_stats(StatsFilter *filter, Error **errp)
{
StatsResultList *stats_results = NULL;
@@ -487,27 +506,33 @@ StatsResultList *qmp_query_stats(StatsFilter *filter, Error **errp)
}
QTAILQ_FOREACH(entry, &stats_callbacks, next) {
- entry->stats_cb(&stats_results, filter->target, targets, errp);
- if (*errp) {
- qapi_free_StatsResultList(stats_results);
- return NULL;
+ if (stats_provider_requested(entry->provider, filter)) {
+ entry->stats_cb(&stats_results, filter->target, targets, errp);
+ if (*errp) {
+ qapi_free_StatsResultList(stats_results);
+ return NULL;
+ }
}
}
return stats_results;
}
-StatsSchemaList *qmp_query_stats_schemas(Error **errp)
+StatsSchemaList *qmp_query_stats_schemas(bool has_provider,
+ StatsProvider provider,
+ Error **errp)
{
StatsSchemaList *stats_results = NULL;
StatsCallbacks *entry;
ERRP_GUARD();
QTAILQ_FOREACH(entry, &stats_callbacks, next) {
- entry->schemas_cb(&stats_results, errp);
- if (*errp) {
- qapi_free_StatsSchemaList(stats_results);
- return NULL;
+ if (!has_provider || provider == entry->provider) {
+ entry->schemas_cb(&stats_results, errp);
+ if (*errp) {
+ qapi_free_StatsSchemaList(stats_results);
+ return NULL;
+ }
}
}
diff --git a/qapi/stats.json b/qapi/stats.json
index e8ed907793..b75bb86124 100644
--- a/qapi/stats.json
+++ b/qapi/stats.json
@@ -68,6 +68,18 @@
{ 'enum': 'StatsTarget',
'data': [ 'vm', 'vcpu' ] }
+##
+# @StatsRequest:
+#
+# Indicates a set of statistics that should be returned by query-stats.
+#
+# @provider: provider for which to return statistics.
+#
+# Since: 7.1
+##
+{ 'struct': 'StatsRequest',
+ 'data': { 'provider': 'StatsProvider' } }
+
##
# @StatsVCPUFilter:
#
@@ -85,11 +97,12 @@
# request statistics and optionally the required subset of information for
# that target:
# - which vCPUs to request statistics for
+# - which provider to request statistics from
#
# Since: 7.1
##
{ 'union': 'StatsFilter',
- 'base': { 'target': 'StatsTarget' },
+ 'base': { 'target': 'StatsTarget', '*providers': [ 'StatsRequest' ] },
'discriminator': 'target',
'data': { 'vcpu': 'StatsVCPUFilter' } }
@@ -225,5 +238,5 @@
# Since: 7.1
##
{ 'command': 'query-stats-schemas',
- 'data': { },
+ 'data': { '*provider': 'StatsProvider' },
'returns': [ 'StatsSchema' ] }
--
2.36.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 5/8] qmp: add filtering of statistics by provider
2022-05-23 15:07 ` [PATCH 5/8] qmp: add filtering of statistics by provider Paolo Bonzini
@ 2022-05-24 12:32 ` Markus Armbruster
2022-05-24 16:42 ` Paolo Bonzini
0 siblings, 1 reply; 20+ messages in thread
From: Markus Armbruster @ 2022-05-24 12:32 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, armbru, Mark Kanda
Paolo Bonzini <pbonzini@redhat.com> writes:
> Allow retrieving the statistics from a specific provider only.
> This can be used in the future by HMP commands such as "info
> sync-profile" or "info profile". The next patch also adds
> filter-by-provider capabilities to the HMP equivalent of
> query-stats, "info stats".
>
> Example:
>
> { "execute": "query-stats",
> "arguments": {
> "target": "vm",
> "providers": [
> { "provider": "kvm" } ] } }
>
> The QAPI is a bit more verbose than just a list of StatsProvider,
> so that it can be subsequently extended with filtering of statistics
> by name.
>
> Extracted from a patch by Mark Kanda.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> accel/kvm/kvm-all.c | 3 ++-
> include/monitor/stats.h | 4 +++-
> monitor/hmp-cmds.c | 2 +-
> monitor/qmp-cmds.c | 45 ++++++++++++++++++++++++++++++++---------
> qapi/stats.json | 17 ++++++++++++++--
> 5 files changed, 56 insertions(+), 15 deletions(-)
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 3ee431a431..461b6cf927 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2644,7 +2644,8 @@ static int kvm_init(MachineState *ms)
> }
>
> if (kvm_check_extension(kvm_state, KVM_CAP_BINARY_STATS_FD)) {
> - add_stats_callbacks(query_stats_cb, query_stats_schemas_cb);
> + add_stats_callbacks(STATS_PROVIDER_KVM, query_stats_cb,
> + query_stats_schemas_cb);
> }
>
> return 0;
> diff --git a/include/monitor/stats.h b/include/monitor/stats.h
> index 8c50feeaa9..840c4ed08e 100644
> --- a/include/monitor/stats.h
> +++ b/include/monitor/stats.h
> @@ -17,10 +17,12 @@ typedef void SchemaRetrieveFunc(StatsSchemaList **result, Error **errp);
> /*
> * Register callbacks for the QMP query-stats command.
> *
> + * @provider: stats provider
Argument documentation that merely paraphrases the type is redundant.
May I have a proper contract?
> * @stats_fn: routine to query stats:
> * @schema_fn: routine to query stat schemas:
> */
> -void add_stats_callbacks(StatRetrieveFunc *stats_fn,
> +void add_stats_callbacks(StatsProvider provider,
> + StatRetrieveFunc *stats_fn,
> SchemaRetrieveFunc *schemas_fn);
>
> /*
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index c0cb440902..8d2c4792d5 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -2394,7 +2394,7 @@ void hmp_info_stats(Monitor *mon, const QDict *qdict)
> goto exit_no_print;
> }
>
> - schema = qmp_query_stats_schemas(&err);
> + schema = qmp_query_stats_schemas(false, 0, &err);
NULL instead of 0 please.
> if (err) {
> goto exit;
> }
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index ebf6f49446..7be0e7414a 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -445,6 +445,7 @@ HumanReadableText *qmp_x_query_irq(Error **errp)
> }
>
> typedef struct StatsCallbacks {
> + StatsProvider provider;
> StatRetrieveFunc *stats_cb;
> SchemaRetrieveFunc *schemas_cb;
> QTAILQ_ENTRY(StatsCallbacks) next;
> @@ -453,16 +454,34 @@ typedef struct StatsCallbacks {
> static QTAILQ_HEAD(, StatsCallbacks) stats_callbacks =
> QTAILQ_HEAD_INITIALIZER(stats_callbacks);
>
> -void add_stats_callbacks(StatRetrieveFunc *stats_fn,
> +void add_stats_callbacks(StatsProvider provider,
> + StatRetrieveFunc *stats_fn,
> SchemaRetrieveFunc *schemas_fn)
> {
> StatsCallbacks *entry = g_new(StatsCallbacks, 1);
> + entry->provider = provider;
> entry->stats_cb = stats_fn;
> entry->schemas_cb = schemas_fn;
>
> QTAILQ_INSERT_TAIL(&stats_callbacks, entry, next);
> }
>
> +static bool stats_provider_requested(StatsProvider provider,
> + StatsFilter *filter)
> +{
> + StatsRequestList *request;
> +
> + if (!filter->has_providers) {
> + return true;
> + }
> + for (request = filter->providers; request; request = request->next) {
> + if (request->value->provider == provider) {
> + return true;
> + }
> + }
> + return false;
> +}
> +
This is just like apply_str_list_filter(). Good! Could we make the two
names similar, too?
> StatsResultList *qmp_query_stats(StatsFilter *filter, Error **errp)
> {
> StatsResultList *stats_results = NULL;
> @@ -487,27 +506,33 @@ StatsResultList *qmp_query_stats(StatsFilter *filter, Error **errp)
> }
>
> QTAILQ_FOREACH(entry, &stats_callbacks, next) {
> - entry->stats_cb(&stats_results, filter->target, targets, errp);
> - if (*errp) {
> - qapi_free_StatsResultList(stats_results);
> - return NULL;
> + if (stats_provider_requested(entry->provider, filter)) {
> + entry->stats_cb(&stats_results, filter->target, targets, errp);
> + if (*errp) {
> + qapi_free_StatsResultList(stats_results);
> + return NULL;
> + }
I like if (!wanted) continue in such loops for less nesting. Clearly a
matter of taste.
> }
> }
>
> return stats_results;
> }
>
> -StatsSchemaList *qmp_query_stats_schemas(Error **errp)
> +StatsSchemaList *qmp_query_stats_schemas(bool has_provider,
> + StatsProvider provider,
> + Error **errp)
> {
> StatsSchemaList *stats_results = NULL;
> StatsCallbacks *entry;
> ERRP_GUARD();
>
> QTAILQ_FOREACH(entry, &stats_callbacks, next) {
> - entry->schemas_cb(&stats_results, errp);
> - if (*errp) {
> - qapi_free_StatsSchemaList(stats_results);
> - return NULL;
> + if (!has_provider || provider == entry->provider) {
> + entry->schemas_cb(&stats_results, errp);
> + if (*errp) {
> + qapi_free_StatsSchemaList(stats_results);
> + return NULL;
> + }
Likewise.
> }
> }
>
> diff --git a/qapi/stats.json b/qapi/stats.json
> index e8ed907793..b75bb86124 100644
> --- a/qapi/stats.json
> +++ b/qapi/stats.json
> @@ -68,6 +68,18 @@
> { 'enum': 'StatsTarget',
> 'data': [ 'vm', 'vcpu' ] }
>
> +##
> +# @StatsRequest:
> +#
> +# Indicates a set of statistics that should be returned by query-stats.
> +#
> +# @provider: provider for which to return statistics.
> +#
> +# Since: 7.1
> +##
> +{ 'struct': 'StatsRequest',
> + 'data': { 'provider': 'StatsProvider' } }
> +
> ##
> # @StatsVCPUFilter:
> #
> @@ -85,11 +97,12 @@
> # request statistics and optionally the required subset of information for
> # that target:
> # - which vCPUs to request statistics for
> +# - which provider to request statistics from
> #
> # Since: 7.1
> ##
> { 'union': 'StatsFilter',
> - 'base': { 'target': 'StatsTarget' },
> + 'base': { 'target': 'StatsTarget', '*providers': [ 'StatsRequest' ] },
Easier to read, I think:
'base': {
'target': 'StatsTarget',
'*providers': [ 'StatsRequest' ] },
> 'discriminator': 'target',
> 'data': { 'vcpu': 'StatsVCPUFilter' } }
>
> @@ -225,5 +238,5 @@
> # Since: 7.1
> ##
> { 'command': 'query-stats-schemas',
> - 'data': { },
> + 'data': { '*provider': 'StatsProvider' },
> 'returns': [ 'StatsSchema' ] }
Only the "NULL instead of 0" is a request, the remainder suggestions.
So, with that request addressed
Reviewed-by: Markus Armbruster <armbru@redhat.com>
PS: Consider adding
[diff]
orderFile = scripts/git.orderfile
to your .git/config.
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 5/8] qmp: add filtering of statistics by provider
2022-05-24 12:32 ` Markus Armbruster
@ 2022-05-24 16:42 ` Paolo Bonzini
0 siblings, 0 replies; 20+ messages in thread
From: Paolo Bonzini @ 2022-05-24 16:42 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Mark Kanda
On 5/24/22 14:32, Markus Armbruster wrote:
>> + * @provider: stats provider
> Argument documentation that merely paraphrases the type is redundant.
> May I have a proper contract?
>
"stats provider matched against QMP command arguments"?
>>
>> +static bool stats_provider_requested(StatsProvider provider,
>> + StatsFilter *filter)
>> +{
>> + StatsRequestList *request;
>> +
>> + if (!filter->has_providers) {
>> + return true;
>> + }
>> + for (request = filter->providers; request; request = request->next) {
>> + if (request->value->provider == provider) {
>> + return true;
>> + }
>> + }
>> + return false;
>> +}
>> +
>
> This is just like apply_str_list_filter(). Good! Could we make the two
> names similar, too?
It looks similar but there is a difference in patch 7, in that it also
returns the "names" filter. I can rename it to
find_stats_provider_filter() if you prefer.
All other suggestions applied, thanks.
Paolo
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 6/8] hmp: add filtering of statistics by provider
2022-05-23 15:07 ` [PATCH 1/8] qmp: Support for querying stats Paolo Bonzini
` (3 preceding siblings ...)
2022-05-23 15:07 ` [PATCH 5/8] qmp: add filtering of statistics by provider Paolo Bonzini
@ 2022-05-23 15:07 ` Paolo Bonzini
2022-05-23 15:07 ` [PATCH 7/8] qmp: add filtering of statistics by name Paolo Bonzini
` (3 subsequent siblings)
8 siblings, 0 replies; 20+ messages in thread
From: Paolo Bonzini @ 2022-05-23 15:07 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Mark Kanda
Allow the user to request statistics for a single provider of interest.
Extracted from a patch by Mark Kanda.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hmp-commands-info.hx | 7 ++++---
monitor/hmp-cmds.c | 41 ++++++++++++++++++++++++++++++++++-------
2 files changed, 38 insertions(+), 10 deletions(-)
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index 221feab8c0..d4d8a1e618 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -897,9 +897,10 @@ ERST
{
.name = "stats",
- .args_type = "target:s",
- .params = "target",
- .help = "show statistics; target is either vm or vcpu",
+ .args_type = "target:s,provider:s?",
+ .params = "target [provider]",
+ .help = "show statistics for the given target (vm or vcpu); optionally filter by "
+ "provider",
.cmd = hmp_info_stats,
},
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 8d2c4792d5..02a455090d 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -2297,6 +2297,7 @@ static StatsSchemaValueList *find_schema_value_list(
}
static void print_stats_results(Monitor *mon, StatsTarget target,
+ bool show_provider,
StatsResult *result,
StatsSchemaList *schema)
{
@@ -2311,8 +2312,10 @@ static void print_stats_results(Monitor *mon, StatsTarget target,
return;
}
- monitor_printf(mon, "provider: %s\n",
- StatsProvider_str(result->provider));
+ if (show_provider) {
+ monitor_printf(mon, "provider: %s\n",
+ StatsProvider_str(result->provider));
+ }
for (stats_list = result->stats; stats_list;
stats_list = stats_list->next,
@@ -2353,7 +2356,8 @@ static void print_stats_results(Monitor *mon, StatsTarget target,
}
/* Create the StatsFilter that is needed for an "info stats" invocation. */
-static StatsFilter *stats_filter(StatsTarget target, int cpu_index)
+static StatsFilter *stats_filter(StatsTarget target, int cpu_index,
+ StatsProvider provider)
{
StatsFilter *filter = g_malloc0(sizeof(*filter));
@@ -2375,12 +2379,27 @@ static StatsFilter *stats_filter(StatsTarget target, int cpu_index)
default:
break;
}
+
+ if (provider == STATS_PROVIDER__MAX) {
+ return filter;
+ }
+
+ /* "info stats" can only query either one or all the providers. */
+ StatsRequest *request = g_new0(StatsRequest, 1);
+ request->provider = provider;
+ StatsRequestList *request_list = g_new0(StatsRequestList, 1);
+ QAPI_LIST_PREPEND(request_list, request);
+ filter->has_providers = true;
+ filter->providers = request_list;
return filter;
}
void hmp_info_stats(Monitor *mon, const QDict *qdict)
{
const char *target_str = qdict_get_str(qdict, "target");
+ const char *provider_str = qdict_get_try_str(qdict, "provider");
+
+ StatsProvider provider = STATS_PROVIDER__MAX;
StatsTarget target;
Error *err = NULL;
g_autoptr(StatsSchemaList) schema = NULL;
@@ -2393,19 +2412,27 @@ void hmp_info_stats(Monitor *mon, const QDict *qdict)
monitor_printf(mon, "invalid stats target %s\n", target_str);
goto exit_no_print;
}
+ if (provider_str) {
+ provider = qapi_enum_parse(&StatsProvider_lookup, provider_str, -1, &err);
+ if (err) {
+ monitor_printf(mon, "invalid stats provider %s\n", provider_str);
+ goto exit_no_print;
+ }
+ }
- schema = qmp_query_stats_schemas(false, 0, &err);
+ schema = qmp_query_stats_schemas(provider_str ? true : false,
+ provider, &err);
if (err) {
goto exit;
}
switch (target) {
case STATS_TARGET_VM:
- filter = stats_filter(target, -1);
+ filter = stats_filter(target, -1, provider);
break;
case STATS_TARGET_VCPU: {}
int cpu_index = monitor_get_cpu_index(mon);
- filter = stats_filter(target, cpu_index);
+ filter = stats_filter(target, cpu_index, provider);
break;
default:
abort();
@@ -2416,7 +2443,7 @@ void hmp_info_stats(Monitor *mon, const QDict *qdict)
goto exit;
}
for (entry = stats; entry; entry = entry->next) {
- print_stats_results(mon, target, entry->value, schema);
+ print_stats_results(mon, target, provider_str == NULL, entry->value, schema);
}
exit:
--
2.36.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH 7/8] qmp: add filtering of statistics by name
2022-05-23 15:07 ` [PATCH 1/8] qmp: Support for querying stats Paolo Bonzini
` (4 preceding siblings ...)
2022-05-23 15:07 ` [PATCH 6/8] hmp: " Paolo Bonzini
@ 2022-05-23 15:07 ` Paolo Bonzini
2022-05-24 13:08 ` Markus Armbruster
2022-05-23 15:07 ` [PATCH 8/8] hmp: " Paolo Bonzini
` (2 subsequent siblings)
8 siblings, 1 reply; 20+ messages in thread
From: Paolo Bonzini @ 2022-05-23 15:07 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Mark Kanda
Allow retrieving only a subset of statistics. This can be useful
for example in order to plot a subset of the statistics many times
a second.
KVM publishes ~40 statistics for each vCPU on x86; retrieving and
serializing all of them would be useless
Another use will be in HMP in the following patch; implementing the
filter in the backend is easy enough that it was deemed okay to make
this a public interface.
Example:
{ "execute": "query-stats",
"arguments": {
"target": "vcpu",
"vcpus": [ "/machine/unattached/device[2]",
"/machine/unattached/device[4]" ],
"providers": [
{ "provider": "kvm",
"names": [ "l1d_flush", "exits" ] } } }
{ "return": {
"vcpus": [
{ "path": "/machine/unattached/device[2]"
"providers": [
{ "provider": "kvm",
"stats": [ { "name": "l1d_flush", "value": 41213 },
{ "name": "exits", "value": 74291 } ] } ] },
{ "path": "/machine/unattached/device[4]"
"providers": [
{ "provider": "kvm",
"stats": [ { "name": "l1d_flush", "value": 16132 },
{ "name": "exits", "value": 57922 } ] } ] } ] } }
Extracted from a patch by Mark Kanda.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
v3->v4: handle empty filter early by avoiding the query altogether
accel/kvm/kvm-all.c | 17 +++++++++++------
include/monitor/stats.h | 4 ++--
monitor/qmp-cmds.c | 16 +++++++++++++---
qapi/stats.json | 6 +++++-
4 files changed, 31 insertions(+), 12 deletions(-)
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 461b6cf927..a61d17719e 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2311,7 +2311,7 @@ bool kvm_dirty_ring_enabled(void)
return kvm_state->kvm_dirty_ring_size ? true : false;
}
-static void query_stats_cb(StatsResultList **result, StatsTarget target,
+static void query_stats_cb(StatsResultList **result, StatsTarget target, strList *names,
strList *targets, Error **errp);
static void query_stats_schemas_cb(StatsSchemaList **result, Error **errp);
@@ -3713,6 +3713,7 @@ typedef struct StatsArgs {
StatsResultList **stats;
StatsSchemaList **schema;
} result;
+ strList *names;
Error **errp;
} StatsArgs;
@@ -3926,7 +3927,7 @@ static StatsDescriptors *find_stats_descriptors(StatsTarget target, int stats_fd
return descriptors;
}
-static void query_stats(StatsResultList **result, StatsTarget target,
+static void query_stats(StatsResultList **result, StatsTarget target, strList *names,
int stats_fd, Error **errp)
{
struct kvm_stats_desc *kvm_stats_desc;
@@ -3969,6 +3970,9 @@ static void query_stats(StatsResultList **result, StatsTarget target,
/* Add entry to the list */
stats = (void *)stats_data + pdesc->offset;
+ if (!apply_str_list_filter(pdesc->name, names)) {
+ continue;
+ }
stats_list = add_kvmstat_entry(pdesc, stats, stats_list, errp);
}
@@ -4030,8 +4034,8 @@ static void query_stats_vcpu(CPUState *cpu, run_on_cpu_data data)
error_propagate(kvm_stats_args->errp, local_err);
return;
}
- query_stats(kvm_stats_args->result.stats, STATS_TARGET_VCPU, stats_fd,
- kvm_stats_args->errp);
+ query_stats(kvm_stats_args->result.stats, STATS_TARGET_VCPU,
+ kvm_stats_args->names, stats_fd, kvm_stats_args->errp);
close(stats_fd);
}
@@ -4052,7 +4056,7 @@ static void query_stats_schema_vcpu(CPUState *cpu, run_on_cpu_data data)
}
static void query_stats_cb(StatsResultList **result, StatsTarget target,
- strList *targets, Error **errp)
+ strList *names, strList *targets, Error **errp)
{
KVMState *s = kvm_state;
CPUState *cpu;
@@ -4066,7 +4070,7 @@ static void query_stats_cb(StatsResultList **result, StatsTarget target,
error_setg(errp, "KVM stats: ioctl failed");
return;
}
- query_stats(result, target, stats_fd, errp);
+ query_stats(result, target, names, stats_fd, errp);
close(stats_fd);
break;
}
@@ -4074,6 +4078,7 @@ static void query_stats_cb(StatsResultList **result, StatsTarget target,
{
StatsArgs stats_args;
stats_args.result.stats = result;
+ stats_args.names = names;
stats_args.errp = errp;
CPU_FOREACH(cpu) {
if (!apply_str_list_filter(cpu->parent_obj.canonical_path, targets)) {
diff --git a/include/monitor/stats.h b/include/monitor/stats.h
index 840c4ed08e..88f046f568 100644
--- a/include/monitor/stats.h
+++ b/include/monitor/stats.h
@@ -11,8 +11,8 @@
#include "qapi/qapi-types-stats.h"
typedef void StatRetrieveFunc(StatsResultList **result, StatsTarget target,
- strList *targets, Error **errp);
-typedef void SchemaRetrieveFunc(StatsSchemaList **result, Error **errp);
+ strList *names, strList *targets, Error **errp);
+typedef void SchemaRetrieveFunc(StatsSchemaList **, Error **);
/*
* Register callbacks for the QMP query-stats command.
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 7be0e7414a..e822f1b0a9 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -467,15 +467,24 @@ void add_stats_callbacks(StatsProvider provider,
}
static bool stats_provider_requested(StatsProvider provider,
- StatsFilter *filter)
+ StatsFilter *filter,
+ strList **p_names)
{
StatsRequestList *request;
+ *p_names = NULL;
if (!filter->has_providers) {
return true;
}
for (request = filter->providers; request; request = request->next) {
if (request->value->provider == provider) {
+ if (request->value->has_names) {
+ if (!request->value->names) {
+ /* No names allowed is the same as skipping the provider. */
+ return false;
+ }
+ *p_names = request->value->names;
+ }
return true;
}
}
@@ -506,8 +515,9 @@ StatsResultList *qmp_query_stats(StatsFilter *filter, Error **errp)
}
QTAILQ_FOREACH(entry, &stats_callbacks, next) {
- if (stats_provider_requested(entry->provider, filter)) {
- entry->stats_cb(&stats_results, filter->target, targets, errp);
+ strList *names = NULL;
+ if (stats_provider_requested(entry->provider, filter, &names)) {
+ entry->stats_cb(&stats_results, filter->target, names, targets, errp);
if (*errp) {
qapi_free_StatsResultList(stats_results);
return NULL;
diff --git a/qapi/stats.json b/qapi/stats.json
index b75bb86124..2dbf188d36 100644
--- a/qapi/stats.json
+++ b/qapi/stats.json
@@ -74,11 +74,14 @@
# Indicates a set of statistics that should be returned by query-stats.
#
# @provider: provider for which to return statistics.
+
+# @names: statistics to be returned (all if omitted).
#
# Since: 7.1
##
{ 'struct': 'StatsRequest',
- 'data': { 'provider': 'StatsProvider' } }
+ 'data': { 'provider': 'StatsProvider',
+ '*names': [ 'str' ] } }
##
# @StatsVCPUFilter:
@@ -98,6 +101,7 @@
# that target:
# - which vCPUs to request statistics for
# - which provider to request statistics from
+# - which values to return within each provider
#
# Since: 7.1
##
--
2.36.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 7/8] qmp: add filtering of statistics by name
2022-05-23 15:07 ` [PATCH 7/8] qmp: add filtering of statistics by name Paolo Bonzini
@ 2022-05-24 13:08 ` Markus Armbruster
2022-05-24 16:49 ` Paolo Bonzini
0 siblings, 1 reply; 20+ messages in thread
From: Markus Armbruster @ 2022-05-24 13:08 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, Mark Kanda
Paolo Bonzini <pbonzini@redhat.com> writes:
> Allow retrieving only a subset of statistics. This can be useful
> for example in order to plot a subset of the statistics many times
> a second.
>
> KVM publishes ~40 statistics for each vCPU on x86; retrieving and
> serializing all of them would be useless
>
> Another use will be in HMP in the following patch; implementing the
> filter in the backend is easy enough that it was deemed okay to make
> this a public interface.
>
> Example:
>
> { "execute": "query-stats",
> "arguments": {
> "target": "vcpu",
> "vcpus": [ "/machine/unattached/device[2]",
> "/machine/unattached/device[4]" ],
> "providers": [
> { "provider": "kvm",
> "names": [ "l1d_flush", "exits" ] } } }
>
> { "return": {
> "vcpus": [
> { "path": "/machine/unattached/device[2]"
> "providers": [
> { "provider": "kvm",
> "stats": [ { "name": "l1d_flush", "value": 41213 },
> { "name": "exits", "value": 74291 } ] } ] },
> { "path": "/machine/unattached/device[4]"
> "providers": [
> { "provider": "kvm",
> "stats": [ { "name": "l1d_flush", "value": 16132 },
> { "name": "exits", "value": 57922 } ] } ] } ] } }
>
> Extracted from a patch by Mark Kanda.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> v3->v4: handle empty filter early by avoiding the query altogether
>
> accel/kvm/kvm-all.c | 17 +++++++++++------
> include/monitor/stats.h | 4 ++--
> monitor/qmp-cmds.c | 16 +++++++++++++---
> qapi/stats.json | 6 +++++-
> 4 files changed, 31 insertions(+), 12 deletions(-)
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 461b6cf927..a61d17719e 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2311,7 +2311,7 @@ bool kvm_dirty_ring_enabled(void)
> return kvm_state->kvm_dirty_ring_size ? true : false;
> }
>
> -static void query_stats_cb(StatsResultList **result, StatsTarget target,
> +static void query_stats_cb(StatsResultList **result, StatsTarget target, strList *names,
Long line.
> strList *targets, Error **errp);
> static void query_stats_schemas_cb(StatsSchemaList **result, Error **errp);
>
> @@ -3713,6 +3713,7 @@ typedef struct StatsArgs {
> StatsResultList **stats;
> StatsSchemaList **schema;
> } result;
> + strList *names;
> Error **errp;
> } StatsArgs;
>
> @@ -3926,7 +3927,7 @@ static StatsDescriptors *find_stats_descriptors(StatsTarget target, int stats_fd
> return descriptors;
> }
>
> -static void query_stats(StatsResultList **result, StatsTarget target,
> +static void query_stats(StatsResultList **result, StatsTarget target, strList *names,
Long line.
> int stats_fd, Error **errp)
> {
> struct kvm_stats_desc *kvm_stats_desc;
> @@ -3969,6 +3970,9 @@ static void query_stats(StatsResultList **result, StatsTarget target,
>
> /* Add entry to the list */
> stats = (void *)stats_data + pdesc->offset;
> + if (!apply_str_list_filter(pdesc->name, names)) {
> + continue;
> + }
> stats_list = add_kvmstat_entry(pdesc, stats, stats_list, errp);
> }
>
> @@ -4030,8 +4034,8 @@ static void query_stats_vcpu(CPUState *cpu, run_on_cpu_data data)
> error_propagate(kvm_stats_args->errp, local_err);
> return;
> }
> - query_stats(kvm_stats_args->result.stats, STATS_TARGET_VCPU, stats_fd,
> - kvm_stats_args->errp);
> + query_stats(kvm_stats_args->result.stats, STATS_TARGET_VCPU,
> + kvm_stats_args->names, stats_fd, kvm_stats_args->errp);
> close(stats_fd);
> }
>
> @@ -4052,7 +4056,7 @@ static void query_stats_schema_vcpu(CPUState *cpu, run_on_cpu_data data)
> }
>
> static void query_stats_cb(StatsResultList **result, StatsTarget target,
> - strList *targets, Error **errp)
> + strList *names, strList *targets, Error **errp)
> {
> KVMState *s = kvm_state;
> CPUState *cpu;
> @@ -4066,7 +4070,7 @@ static void query_stats_cb(StatsResultList **result, StatsTarget target,
> error_setg(errp, "KVM stats: ioctl failed");
> return;
> }
> - query_stats(result, target, stats_fd, errp);
> + query_stats(result, target, names, stats_fd, errp);
> close(stats_fd);
> break;
> }
> @@ -4074,6 +4078,7 @@ static void query_stats_cb(StatsResultList **result, StatsTarget target,
> {
> StatsArgs stats_args;
> stats_args.result.stats = result;
> + stats_args.names = names;
> stats_args.errp = errp;
> CPU_FOREACH(cpu) {
> if (!apply_str_list_filter(cpu->parent_obj.canonical_path, targets)) {
> diff --git a/include/monitor/stats.h b/include/monitor/stats.h
> index 840c4ed08e..88f046f568 100644
> --- a/include/monitor/stats.h
> +++ b/include/monitor/stats.h
> @@ -11,8 +11,8 @@
> #include "qapi/qapi-types-stats.h"
>
> typedef void StatRetrieveFunc(StatsResultList **result, StatsTarget target,
> - strList *targets, Error **errp);
> -typedef void SchemaRetrieveFunc(StatsSchemaList **result, Error **errp);
> + strList *names, strList *targets, Error **errp);
> +typedef void SchemaRetrieveFunc(StatsSchemaList **, Error **);
Did you drop the parameter names intentionally?
>
> /*
> * Register callbacks for the QMP query-stats command.
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 7be0e7414a..e822f1b0a9 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -467,15 +467,24 @@ void add_stats_callbacks(StatsProvider provider,
> }
>
> static bool stats_provider_requested(StatsProvider provider,
> - StatsFilter *filter)
> + StatsFilter *filter,
> + strList **p_names)
> {
> StatsRequestList *request;
>
> + *p_names = NULL;
> if (!filter->has_providers) {
> return true;
> }
> for (request = filter->providers; request; request = request->next) {
> if (request->value->provider == provider) {
> + if (request->value->has_names) {
> + if (!request->value->names) {
> + /* No names allowed is the same as skipping the provider. */
Long line.
> + return false;
Any other elements of filter->providers that match @provider will be
silently ignored. Is this what you want?
Uh, do we leak @p_names if earlier elements matched?
> + }
> + *p_names = request->value->names;
> + }
> return true;
> }
> }
> @@ -506,8 +515,9 @@ StatsResultList *qmp_query_stats(StatsFilter *filter, Error **errp)
> }
>
> QTAILQ_FOREACH(entry, &stats_callbacks, next) {
> - if (stats_provider_requested(entry->provider, filter)) {
> - entry->stats_cb(&stats_results, filter->target, targets, errp);
> + strList *names = NULL;
> + if (stats_provider_requested(entry->provider, filter, &names)) {
> + entry->stats_cb(&stats_results, filter->target, names, targets, errp);
Brain cramp (broken night)... where is @names freed?
Long line.
> if (*errp) {
> qapi_free_StatsResultList(stats_results);
> return NULL;
> diff --git a/qapi/stats.json b/qapi/stats.json
> index b75bb86124..2dbf188d36 100644
> --- a/qapi/stats.json
> +++ b/qapi/stats.json
> @@ -74,11 +74,14 @@
> # Indicates a set of statistics that should be returned by query-stats.
> #
> # @provider: provider for which to return statistics.
> +
> +# @names: statistics to be returned (all if omitted).
> #
> # Since: 7.1
> ##
> { 'struct': 'StatsRequest',
> - 'data': { 'provider': 'StatsProvider' } }
> + 'data': { 'provider': 'StatsProvider',
> + '*names': [ 'str' ] } }
>
> ##
> # @StatsVCPUFilter:
> @@ -98,6 +101,7 @@
> # that target:
> # - which vCPUs to request statistics for
> # - which provider to request statistics from
Should this be "which providers"?
> +# - which values to return within each provider
This is member @provider sub-member @names. Hmm. "which named values
to return"?
> #
> # Since: 7.1
> ##
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 7/8] qmp: add filtering of statistics by name
2022-05-24 13:08 ` Markus Armbruster
@ 2022-05-24 16:49 ` Paolo Bonzini
2022-05-25 7:49 ` Markus Armbruster
0 siblings, 1 reply; 20+ messages in thread
From: Paolo Bonzini @ 2022-05-24 16:49 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Mark Kanda
On 5/24/22 15:08, Markus Armbruster wrote:
>> -typedef void SchemaRetrieveFunc(StatsSchemaList **result, Error **errp);
>> + strList *names, strList *targets, Error **errp);
>> +typedef void SchemaRetrieveFunc(StatsSchemaList **, Error **);
> Did you drop the parameter names intentionally?
No, I didn't.
>> + /* No names allowed is the same as skipping the provider. */
>
> Long line.
>
>> + return false;
>
> Any other elements of filter->providers that match @provider will be
> silently ignored. Is this what you want?
Hmm, key/value pairs are ugly in QMP.
I'll see if I can make it work nicely without inlining
stats_provider_requested() in the caller.
> Uh, do we leak @p_names if earlier elements matched?
No, it's not copied so there are no leaks.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 7/8] qmp: add filtering of statistics by name
2022-05-24 16:49 ` Paolo Bonzini
@ 2022-05-25 7:49 ` Markus Armbruster
0 siblings, 0 replies; 20+ messages in thread
From: Markus Armbruster @ 2022-05-25 7:49 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, Mark Kanda
Paolo Bonzini <pbonzini@redhat.com> writes:
> On 5/24/22 15:08, Markus Armbruster wrote:
>>> -typedef void SchemaRetrieveFunc(StatsSchemaList **result, Error **errp);
>>> + strList *names, strList *targets, Error **errp);
>>> +typedef void SchemaRetrieveFunc(StatsSchemaList **, Error **);
>>
>> Did you drop the parameter names intentionally?
>
> No, I didn't.
Easy enough to revert :)
>>> + /* No names allowed is the same as skipping the provider. */
>>
>> Long line.
>>
>>> + return false;
>>
>> Any other elements of filter->providers that match @provider will be
>> silently ignored. Is this what you want?
>
> Hmm, key/value pairs are ugly in QMP.
Funny, considering what JSON objects are, isn't it?
Ways to do maps in QMP:
1. You can always use a JSON array of objects. Any combination of
members can be a key. Any semantic constaint "keys are unique" you get
to enforce manually.
2. If the key is a string, you can use a JSON object.
In either case, you may or may not be able to define a compile-time
static schema. If you are, then 1.'s schema can be ['UnionType'], where
the key is in the UnionType's base, and 2.'s can be a struct with
optional members. Else, you get to play with 'any', I guess.
> I'll see if I can make it work nicely without inlining stats_provider_requested() in the caller.
>
>> Uh, do we leak @p_names if earlier elements matched?
>
> No, it's not copied so there are no leaks.
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 8/8] hmp: add filtering of statistics by name
2022-05-23 15:07 ` [PATCH 1/8] qmp: Support for querying stats Paolo Bonzini
` (5 preceding siblings ...)
2022-05-23 15:07 ` [PATCH 7/8] qmp: add filtering of statistics by name Paolo Bonzini
@ 2022-05-23 15:07 ` Paolo Bonzini
2022-05-24 10:41 ` [PATCH 1/8] qmp: Support for querying stats Markus Armbruster
2022-05-24 12:20 ` Markus Armbruster
8 siblings, 0 replies; 20+ messages in thread
From: Paolo Bonzini @ 2022-05-23 15:07 UTC (permalink / raw)
To: qemu-devel; +Cc: armbru, Mark Kanda
Allow the user to request only a specific subset of statistics.
This can be useful when working on a feature or optimization that is
known to affect that statistic.
Extracted from a patch by Mark Kanda.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hmp-commands-info.hx | 8 ++++----
monitor/hmp-cmds.c | 35 +++++++++++++++++++++++++----------
2 files changed, 29 insertions(+), 14 deletions(-)
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index d4d8a1e618..767aafd1ea 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -897,10 +897,10 @@ ERST
{
.name = "stats",
- .args_type = "target:s,provider:s?",
- .params = "target [provider]",
- .help = "show statistics for the given target (vm or vcpu); optionally filter by "
- "provider",
+ .args_type = "target:s,names:s?,provider:s?",
+ .params = "target [names] [provider]",
+ .help = "show statistics for the given target (vm or vcpu); optionally filter by"
+ "name (comma-separated list, or * for all) and provider",
.cmd = hmp_info_stats,
},
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 02a455090d..7fe8a9cdc8 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -2356,10 +2356,12 @@ static void print_stats_results(Monitor *mon, StatsTarget target,
}
/* Create the StatsFilter that is needed for an "info stats" invocation. */
-static StatsFilter *stats_filter(StatsTarget target, int cpu_index,
- StatsProvider provider)
+static StatsFilter *stats_filter(StatsTarget target, const char *names,
+ int cpu_index, StatsProvider provider)
{
StatsFilter *filter = g_malloc0(sizeof(*filter));
+ StatsProvider provider_idx;
+ StatsRequestList *request_list = NULL;
filter->target = target;
switch (target) {
@@ -2380,15 +2382,27 @@ static StatsFilter *stats_filter(StatsTarget target, int cpu_index,
break;
}
- if (provider == STATS_PROVIDER__MAX) {
+ if (!names && provider == STATS_PROVIDER__MAX) {
return filter;
}
- /* "info stats" can only query either one or all the providers. */
- StatsRequest *request = g_new0(StatsRequest, 1);
- request->provider = provider;
- StatsRequestList *request_list = g_new0(StatsRequestList, 1);
- QAPI_LIST_PREPEND(request_list, request);
+ /*
+ * "info stats" can only query either one or all the providers. Querying
+ * by name, but not by provider, requires the creation of one filter per
+ * provider.
+ */
+ for (provider_idx = 0; provider_idx < STATS_PROVIDER__MAX; provider_idx++) {
+ if (provider == STATS_PROVIDER__MAX || provider == provider_idx) {
+ StatsRequest *request = g_new0(StatsRequest, 1);
+ request->provider = provider_idx;
+ if (names && !g_str_equal(names, "*")) {
+ request->has_names = true;
+ request->names = strList_from_comma_list(names);
+ }
+ QAPI_LIST_PREPEND(request_list, request);
+ }
+ }
+
filter->has_providers = true;
filter->providers = request_list;
return filter;
@@ -2398,6 +2412,7 @@ void hmp_info_stats(Monitor *mon, const QDict *qdict)
{
const char *target_str = qdict_get_str(qdict, "target");
const char *provider_str = qdict_get_try_str(qdict, "provider");
+ const char *names = qdict_get_try_str(qdict, "names");
StatsProvider provider = STATS_PROVIDER__MAX;
StatsTarget target;
@@ -2428,11 +2443,11 @@ void hmp_info_stats(Monitor *mon, const QDict *qdict)
switch (target) {
case STATS_TARGET_VM:
- filter = stats_filter(target, -1, provider);
+ filter = stats_filter(target, names, -1, provider);
break;
case STATS_TARGET_VCPU: {}
int cpu_index = monitor_get_cpu_index(mon);
- filter = stats_filter(target, cpu_index, provider);
+ filter = stats_filter(target, names, cpu_index, provider);
break;
default:
abort();
--
2.36.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 1/8] qmp: Support for querying stats
2022-05-23 15:07 ` [PATCH 1/8] qmp: Support for querying stats Paolo Bonzini
` (6 preceding siblings ...)
2022-05-23 15:07 ` [PATCH 8/8] hmp: " Paolo Bonzini
@ 2022-05-24 10:41 ` Markus Armbruster
2022-05-24 16:47 ` Paolo Bonzini
2022-05-24 12:20 ` Markus Armbruster
8 siblings, 1 reply; 20+ messages in thread
From: Markus Armbruster @ 2022-05-24 10:41 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, Mark Kanda
Paolo Bonzini <pbonzini@redhat.com> writes:
> From: Mark Kanda <mark.kanda@oracle.com>
>
> Gathering statistics is important for development, for monitoring and
> for performance measurement. There are tools such as kvm_stat that do
> this and they rely on the _user_ knowing the interesting data points
> rather than the tool (which can treat them as opaque).
>
> The commands introduced in this commit introduce QMP support for
> querying stats; the goal is to take the capabilities of these tools
> and making them available throughout the whole virtualization stack,
> so that one can observe, monitor and measure virtual machines without
> having shell access + root on the host that runs them.
>
> query-stats returns a list of all stats per target type (only VM
> and vCPU to start); future commits add extra options for specifying
> stat names, vCPU qom paths, and providers. All these are used by the
> HMP command "info stats". Because of the development usecases around
> statistics, a good HMP interface is important.
>
> query-stats-schemas returns a list of stats included in each target
> type, with an option for specifying the provider. The concepts in the
> schema are based on the KVM binary stats' own introspection data, just
> translated to QAPI.
>
> There are two reasons to have a separate schema that is not tied to
> the QAPI schema. The first is the contents of the schemas: the new
> introspection data provides different information than the QAPI data,
> namely unit of measurement, how the numbers are gathered and change
> (peak/instant/cumulative/histogram), and histogram bucket sizes.
> There's really no reason to have this kind of metadata in the QAPI
> introspection schema (except possibly for the unit of measure, but
> there's a very weak justification).
>
> Another reason is the dynamicity of the schema. The QAPI introspection
> data is very much static; and while QOM is somewhat more dynamic,
> generally we consider that to be a bug rather than a feature these days.
> On the other hand, the statistics that are exposed by QEMU might be
> passed through from another source, such as KVM, and the disadvantages of
> manually updating the QAPI schema for outweight the benefits from vetting
> the statistics and filtering out anything that seems "too unstable".
> Running old QEMU with new kernel is a supported usecase; if old QEMU
> cannot expose statistics from a new kernel, or if a kernel developer
> needs to change QEMU before gathering new info from the new kernel,
> then that is a poor user interface.
>
> The framework provides a method to register callbacks for these QMP
> commands. Most of the work in fact is done by the callbacks, and a
> large majority of this patch is new QAPI structs and commands.
>
> Examples (with KVM stats):
>
> - Query all VM stats:
>
> { "execute": "query-stats", "arguments" : { "target": "vm" } }
>
> { "return": [
> { "provider": "kvm",
> "stats": [
> { "name": "max_mmu_page_hash_collisions", "value": 0 },
> { "name": "max_mmu_rmap_size", "value": 0 },
> { "name": "nx_lpage_splits", "value": 148 },
> ... ] },
> { "provider": "xyz",
> "stats": [ ... ] }
> ] }
>
> - Query all vCPU stats:
>
> { "execute": "query-stats", "arguments" : { "target": "vcpu" } }
>
> { "return": [
> { "provider": "kvm",
> "qom_path": "/machine/unattached/device[0]"
> "stats": [
> { "name": "guest_mode", "value": 0 },
> { "name": "directed_yield_successful", "value": 0 },
> { "name": "directed_yield_attempted", "value": 106 },
> ... ] },
> { "provider": "kvm",
> "qom_path": "/machine/unattached/device[1]"
> "stats": [
> { "name": "guest_mode", "value": 0 },
> { "name": "directed_yield_successful", "value": 0 },
> { "name": "directed_yield_attempted", "value": 106 },
> ... ] },
> ] }
>
> - Retrieve the schemas:
>
> { "execute": "query-stats-schemas" }
>
> { "return": [
> { "provider": "kvm",
> "target": "vcpu",
> "stats": [
> { "name": "guest_mode",
> "unit": "none",
> "base": 10,
> "exponent": 0,
> "type": "instant" },
> { "name": "directed_yield_successful",
> "unit": "none",
> "base": 10,
> "exponent": 0,
> "type": "cumulative" },
> ... ]
> },
> { "provider": "kvm",
> "target": "vm",
> "stats": [
> { "name": "max_mmu_page_hash_collisions",
> "unit": "none",
> "base": 10,
> "exponent": 0,
> "type": "peak" },
> ... ]
> },
> { "provider": "xyz",
> "target": "vm",
> "stats": [ ... ]
> }
> ] }
>
> Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> v3->v4: correctly handle errors from the callbacks
>
> include/monitor/stats.h | 34 +++++++
> monitor/qmp-cmds.c | 82 +++++++++++++++
> qapi/meson.build | 1 +
> qapi/qapi-schema.json | 1 +
> qapi/stats.json | 215 ++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 333 insertions(+)
> create mode 100644 include/monitor/stats.h
> create mode 100644 qapi/stats.json
>
> diff --git a/include/monitor/stats.h b/include/monitor/stats.h
> new file mode 100644
> index 0000000000..912eeadb2f
> --- /dev/null
> +++ b/include/monitor/stats.h
> @@ -0,0 +1,34 @@
> +/*
> + * Copyright (c) 2022 Oracle and/or its affiliates.
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef STATS_H
> +#define STATS_H
> +
> +#include "qapi/qapi-types-stats.h"
> +
> +typedef void StatRetrieveFunc(StatsResultList **result, StatsTarget target,
> + Error **errp);
> +typedef void SchemaRetrieveFunc(StatsSchemaList **result, Error **errp);
> +
> +/*
> + * Register callbacks for the QMP query-stats command.
> + *
> + * @stats_fn: routine to query stats:
> + * @schema_fn: routine to query stat schemas:
Contracts would be nice.
> + */
> +void add_stats_callbacks(StatRetrieveFunc *stats_fn,
> + SchemaRetrieveFunc *schemas_fn);
> +
> +/*
> + * Helper routines for adding stats entries to the results lists.
> + */
> +void add_stats_entry(StatsResultList **, StatsProvider, const char *id,
> + StatsList *stats_list);
> +void add_stats_schema(StatsSchemaList **, StatsProvider, StatsTarget,
> + StatsSchemaValueList *);
> +
> +#endif /* STATS_H */
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 1ebb89f46c..d65c5f0257 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -35,6 +35,7 @@
> #include "qapi/qapi-commands-control.h"
> #include "qapi/qapi-commands-machine.h"
> #include "qapi/qapi-commands-misc.h"
> +#include "qapi/qapi-commands-stats.h"
> #include "qapi/qapi-commands-ui.h"
> #include "qapi/type-helpers.h"
> #include "qapi/qmp/qerror.h"
> @@ -43,6 +44,7 @@
> #include "hw/acpi/acpi_dev_interface.h"
> #include "hw/intc/intc.h"
> #include "hw/rdma/rdma.h"
> +#include "monitor/stats.h"
>
> NameInfo *qmp_query_name(Error **errp)
> {
> @@ -441,3 +443,83 @@ HumanReadableText *qmp_x_query_irq(Error **errp)
>
> return human_readable_text_from_str(buf);
> }
> +
> +typedef struct StatsCallbacks {
> + StatRetrieveFunc *stats_cb;
> + SchemaRetrieveFunc *schemas_cb;
> + QTAILQ_ENTRY(StatsCallbacks) next;
> +} StatsCallbacks;
> +
> +static QTAILQ_HEAD(, StatsCallbacks) stats_callbacks =
> + QTAILQ_HEAD_INITIALIZER(stats_callbacks);
> +
> +void add_stats_callbacks(StatRetrieveFunc *stats_fn,
> + SchemaRetrieveFunc *schemas_fn)
> +{
> + StatsCallbacks *entry = g_new(StatsCallbacks, 1);
> + entry->stats_cb = stats_fn;
> + entry->schemas_cb = schemas_fn;
> +
> + QTAILQ_INSERT_TAIL(&stats_callbacks, entry, next);
> +}
> +
> +StatsResultList *qmp_query_stats(StatsFilter *filter, Error **errp)
> +{
> + StatsResultList *stats_results = NULL;
> + StatsCallbacks *entry;
> + ERRP_GUARD();
> +
> + QTAILQ_FOREACH(entry, &stats_callbacks, next) {
> + entry->stats_cb(&stats_results, filter->target, errp);
> + if (*errp) {
> + qapi_free_StatsResultList(stats_results);
> + return NULL;
> + }
> + }
> +
> + return stats_results;
> +}
> +
> +StatsSchemaList *qmp_query_stats_schemas(Error **errp)
> +{
> + StatsSchemaList *stats_results = NULL;
> + StatsCallbacks *entry;
> + ERRP_GUARD();
> +
> + QTAILQ_FOREACH(entry, &stats_callbacks, next) {
> + entry->schemas_cb(&stats_results, errp);
> + if (*errp) {
> + qapi_free_StatsSchemaList(stats_results);
> + return NULL;
> + }
> + }
> +
> + return stats_results;
> +}
Have you considered making the callbacks return bool, following
error.h's "Whenever practical, also return a value that indicates
success / failure" recommendation?
> +
> +void add_stats_entry(StatsResultList **stats_results, StatsProvider provider,
> + const char *qom_path, StatsList *stats_list)
> +{
> + StatsResult *entry = g_new0(StatsResult, 1);
> +
> + entry->provider = provider;
> + if (qom_path) {
> + entry->has_qom_path = true;
> + entry->qom_path = g_strdup(qom_path);
> + }
> + entry->stats = stats_list;
> +
> + QAPI_LIST_PREPEND(*stats_results, entry);
> +}
> +
> +void add_stats_schema(StatsSchemaList **schema_results,
> + StatsProvider provider, StatsTarget target,
> + StatsSchemaValueList *stats_list)
> +{
> + StatsSchema *entry = g_new0(StatsSchema, 1);
> +
> + entry->provider = provider;
> + entry->target = target;
> + entry->stats = stats_list;
> + QAPI_LIST_PREPEND(*schema_results, entry);
> +}
> diff --git a/qapi/meson.build b/qapi/meson.build
> index 656ef0e039..fd5c93d643 100644
> --- a/qapi/meson.build
> +++ b/qapi/meson.build
> @@ -46,6 +46,7 @@ qapi_all_modules = [
> 'replay',
> 'run-state',
> 'sockets',
> + 'stats',
> 'trace',
> 'transaction',
> 'yank',
> diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
> index 4912b9744e..92d7ecc52c 100644
> --- a/qapi/qapi-schema.json
> +++ b/qapi/qapi-schema.json
> @@ -93,3 +93,4 @@
> { 'include': 'audio.json' }
> { 'include': 'acpi.json' }
> { 'include': 'pci.json' }
> +{ 'include': 'stats.json' }
> diff --git a/qapi/stats.json b/qapi/stats.json
> new file mode 100644
> index 0000000000..650d883297
> --- /dev/null
> +++ b/qapi/stats.json
> @@ -0,0 +1,215 @@
> +# -*- Mode: Python -*-
> +# vim: filetype=python
> +#
> +# Copyright (c) 2022 Oracle and/or its affiliates.
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or later.
> +# See the COPYING file in the top-level directory.
> +#
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +##
> +# = Statistics
> +##
> +
> +##
> +# @StatsType:
> +#
> +# Enumeration of statistics types
> +#
> +# @cumulative: stat is cumulative; value can only increase.
> +# @instant: stat is instantaneous; value can increase or decrease.
> +# @peak: stat is the peak value; value can only increase.
> +# @linear-histogram: stat is a linear histogram.
> +# @log2-histogram: stat is a logarithmic histogram, with one bucket
> +# for each power of two.
> +#
> +# Since: 7.1
> +##
> +{ 'enum' : 'StatsType',
> + 'data' : [ 'cumulative', 'instant', 'peak', 'linear-histogram', 'log2-histogram' ] }
Long line.
> +
> +##
> +# @StatsUnit:
> +#
> +# Enumeration of unit of measurement for statistics
> +#
> +# @bytes: stat reported in bytes.
> +# @seconds: stat reported in seconds.
> +# @cycles: stat reported in clock cycles.
> +#
> +# Since: 7.1
> +##
> +{ 'enum' : 'StatsUnit',
> + 'data' : [ 'bytes', 'seconds', 'cycles' ] }
> +
> +##
> +# @StatsProvider:
> +#
> +# Enumeration of statistics providers.
> +#
> +# Since: 7.1
> +##
> +{ 'enum': 'StatsProvider',
> + 'data': [ ] }
> +
> +##
> +# @StatsTarget:
> +#
> +# The kinds of objects on which one can request statistics.
> +#
> +# @vm: statistics that apply to the entire virtual machine or
> +# the entire QEMU process.
> +#
> +# @vcpu: statistics that apply to a single virtual CPU.
> +#
> +# Since: 7.1
> +##
> +{ 'enum': 'StatsTarget',
> + 'data': [ 'vm', 'vcpu' ] }
> +
> +##
> +# @StatsFilter:
> +#
> +# The arguments to the query-stats command; specifies a target for which to
> +# request statistics.
> +#
> +# Since: 7.1
> +##
> +{ 'struct': 'StatsFilter',
> + 'data': { 'target': 'StatsTarget' } }
> +
> +##
> +# @StatsValue:
> +#
> +# @scalar: single unsigned 64-bit integers.
> +# @list: list of unsigned 64-bit integers (used for histograms).
> +#
> +# Since: 7.1
> +##
> +{ 'alternate': 'StatsValue',
> + 'data': { 'scalar': 'uint64',
> + 'list': [ 'uint64' ] } }
> +
> +##
> +# @Stats:
> +#
> +# @name: name of stat.
> +# @value: stat value.
> +#
> +# Since: 7.1
> +##
> +{ 'struct': 'Stats',
> + 'data': { 'name': 'str',
> + 'value' : 'StatsValue' } }
> +
> +##
> +# @StatsResult:
> +#
> +# @provider: provider for this set of statistics.
> +#
> +# @qom-path: Path to the object for which the statistics are returned,
> +# if the object is exposed in the QOM tree
> +#
> +# @stats: list of statistics.
> +#
> +# Since: 7.1
> +##
> +{ 'struct': 'StatsResult',
> + 'data': { 'provider': 'StatsProvider',
> + '*qom-path': 'str',
> + 'stats': [ 'Stats' ] } }
> +
> +##
> +# @query-stats:
> +#
> +# Return runtime-collected statistics for objects such as the
> +# VM or its vCPUs.
> +#
> +# The arguments are a StatsFilter and specify the provider and objects
> +# to return statistics about.
> +#
> +# Returns: a list of StatsResult, one for each provider and object
> +# (e.g., for each vCPU).
> +#
> +# Since: 7.1
> +##
> +{ 'command': 'query-stats',
> + 'data': 'StatsFilter',
> + 'boxed': true,
> + 'returns': [ 'StatsResult' ] }
> +
> +##
> +# @StatsSchemaValue:
> +#
> +# Schema for a single statistic.
> +#
> +# @name: name of the statistic; each element of the schema is uniquely
> +# identified by a target, a provider (both available in @StatsSchema)
> +# and the name.
> +#
> +# @type: kind of statistic.
> +#
> +# @unit: basic unit of measure for the statistic; if missing, the statistic
> +# is a simple number or counter.
> +#
> +# @base: base for the multiple of @unit in which the statistic is measured.
> +# Only present if @exponent is non-zero; @base and @exponent together
> +# form a SI prefix (e.g., _nano-_ for ``base=10`` and ``exponent=-9``)
> +# or IEC binary prefix (e.g. _kibi-_ for ``base=2`` and ``exponent=10``)
> +#
> +# @exponent: exponent for the multiple of @unit in which the statistic is
> +# expressed, or 0 for the basic unit
> +#
> +# @bucket-size: Present when @type is "linear-histogram", contains the width
> +# of each bucket of the histogram.
> +#
> +# Since: 7.1
> +##
> +{ 'struct': 'StatsSchemaValue',
> + 'data': { 'name': 'str',
> + 'type': 'StatsType',
> + '*unit': 'StatsUnit',
> + '*base': 'int8',
> + 'exponent': 'int16',
> + '*bucket-size': 'uint32' } }
> +
> +##
> +# @StatsSchema:
> +#
> +# Schema for all available statistics for a provider and target.
> +#
> +# @provider: provider for this set of statistics.
> +#
> +# @target: the kind of object that can be queried through the provider.
> +#
> +# @stats: list of statistics.
> +#
> +# Since: 7.1
> +##
> +{ 'struct': 'StatsSchema',
> + 'data': { 'provider': 'StatsProvider',
> + 'target': 'StatsTarget',
> + 'stats': [ 'StatsSchemaValue' ] } }
> +
> +##
> +# @query-stats-schemas:
> +#
> +# Return the schema for all available runtime-collected statistics.
> +#
> +# Note: runtime-collected statistics and their names fall outside QEMU's
> +# usual deprecation policies. QEMU will try to keep the set of available
> +# data stable, together with their names, but will not guarantee stability
> +# at all costs; the same is true of providers that source statistics
> +# externally, e.g. from Linux. For example, if the same value is being
> +# tracked with different names on different architectures or by different
> +# providers, one of them might be renamed. A statistic might go away if
> +# an algorithm is changed or some code is removed; changing a default might
> +# cause previously useful statistics to always report 0. Such changes,
> +# however, are expected to be rare.
> +#
> +# Since: 7.1
> +##
> +{ 'command': 'query-stats-schemas',
> + 'data': { },
> + 'returns': [ 'StatsSchema' ] }
With the long line wrapped:
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 1/8] qmp: Support for querying stats
2022-05-24 10:41 ` [PATCH 1/8] qmp: Support for querying stats Markus Armbruster
@ 2022-05-24 16:47 ` Paolo Bonzini
0 siblings, 0 replies; 20+ messages in thread
From: Paolo Bonzini @ 2022-05-24 16:47 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Mark Kanda
On 5/24/22 12:41, Markus Armbruster wrote:
>> + return stats_results;
>> +}
> Have you considered making the callbacks return bool, following
> error.h's "Whenever practical, also return a value that indicates
> success / failure" recommendation?
>
No, because I generally do not consider that recommendation to apply to
callbacks.
Callbacks are written many times and typically invoked just once, so I
prefer an O(1) loss in convenience to an O(n) chance of making a mistake
in the return value.
Paolo
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/8] qmp: Support for querying stats
2022-05-23 15:07 ` [PATCH 1/8] qmp: Support for querying stats Paolo Bonzini
` (7 preceding siblings ...)
2022-05-24 10:41 ` [PATCH 1/8] qmp: Support for querying stats Markus Armbruster
@ 2022-05-24 12:20 ` Markus Armbruster
8 siblings, 0 replies; 20+ messages in thread
From: Markus Armbruster @ 2022-05-24 12:20 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, Mark Kanda
[...]
> diff --git a/qapi/stats.json b/qapi/stats.json
> new file mode 100644
> index 0000000000..650d883297
> --- /dev/null
> +++ b/qapi/stats.json
[...]
> +##
> +# @query-stats-schemas:
> +#
> +# Return the schema for all available runtime-collected statistics.
> +#
> +# Note: runtime-collected statistics and their names fall outside QEMU's
> +# usual deprecation policies. QEMU will try to keep the set of available
/work/armbru/qemu/scripts/qapi-gen.py: In file included from ../qapi/qapi-schema.json:96:
../qapi/stats.json:201:1: unexpected de-indent (expected at least 6 spaces)
> +# data stable, together with their names, but will not guarantee stability
> +# at all costs; the same is true of providers that source statistics
> +# externally, e.g. from Linux. For example, if the same value is being
> +# tracked with different names on different architectures or by different
> +# providers, one of them might be renamed. A statistic might go away if
> +# an algorithm is changed or some code is removed; changing a default might
> +# cause previously useful statistics to always report 0. Such changes,
> +# however, are expected to be rare.
> +#
> +# Since: 7.1
> +##
> +{ 'command': 'query-stats-schemas',
> + 'data': { },
> + 'returns': [ 'StatsSchema' ] }
^ permalink raw reply [flat|nested] 20+ messages in thread