* [PATCH v2 0/3] Support fd-based KVM stats
@ 2021-11-19 19:51 Mark Kanda
2021-11-19 19:51 ` [PATCH v2 1/3] qmp: Support for querying stats Mark Kanda
` (3 more replies)
0 siblings, 4 replies; 16+ messages in thread
From: Mark Kanda @ 2021-11-19 19:51 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini
v2: [Paolo]
- generalize the interface
- add support for querying stat schema and instances
- add additional HMP semantic processing for a few exponent/unit
combinations (related to seconds and bytes)
This patchset adds QEMU support for querying fd-based KVM stats. The
kernel support was introduced by:
cb082bfab59a ("KVM: stats: Add fd-based API to read binary stats data")
Mark Kanda (3):
qmp: Support for querying stats
hmp: Support for querying stats
kvm: Support for querying fd-based stats
accel/kvm/kvm-all.c | 399 ++++++++++++++++++++++++++++++++++++++
hmp-commands-info.hx | 40 ++++
include/monitor/hmp.h | 3 +
include/monitor/monitor.h | 27 +++
monitor/hmp-cmds.c | 125 ++++++++++++
monitor/qmp-cmds.c | 71 +++++++
qapi/misc.json | 142 ++++++++++++++
7 files changed, 807 insertions(+)
--
2.26.2
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 1/3] qmp: Support for querying stats
2021-11-19 19:51 [PATCH v2 0/3] Support fd-based KVM stats Mark Kanda
@ 2021-11-19 19:51 ` Mark Kanda
2021-12-07 18:42 ` Daniel P. Berrangé
2021-11-19 19:51 ` [PATCH v2 2/3] hmp: " Mark Kanda
` (2 subsequent siblings)
3 siblings, 1 reply; 16+ messages in thread
From: Mark Kanda @ 2021-11-19 19:51 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini
Introduce qmp support for querying stats. Provide a framework for
adding new stats and support for the following commands:
- query-stats
Returns a list of all stats, with options for specifying a stat
name and schema type. A schema type is the set of stats associated
with a given component (e.g. vm or vcpu).
- query-stats-schemas
Returns a list of stats included in each schema type, with an
option for specifying the schema name.
- query-stats-instances
Returns a list of stat instances and their associated schema type.
The framework provides a method to register callbacks for these qmp
commands.
The first usecase will be for fd-based KVM stats (in an upcoming
patch).
Examples (with fd-based KVM stats):
{ "execute": "query-stats" }
{ "return": [
{ "name": "vcpu_1",
"type": "kvm-vcpu",
"stats": [
{ "name": "guest_mode",
"unit": "none",
"base": 10,
"val": [ 0 ],
"exponent": 0,
"type": "instant" },
{ "name": "directed_yield_successful",
"unit": "none",
"base": 10,
"val": [ 0 ],
"exponent": 0,
"type": "cumulative" },
...
},
{ "name": "vcpu_0",
"type": "kvm-vcpu",
"stats": ...
...
},
{ "name": "vm",
"type": "kvm-vm",
"stats": [
{ "name": "max_mmu_page_hash_collisions",
"unit": "none",
"base": 10,
"val": [ 0 ],
"exponent": 0,
"type": "peak" },
...
{ "execute": "query-stats-schemas" }
{ "return": [
{ "type": "kvm-vcpu",
"stats": [
{ "name": "guest_mode" },
{ "name": "directed_yield_successful" },
...
},
{ "type": "kvm-vm",
"stats": [
{ "name": "max_mmu_page_hash_collisions" },
{ "name": "max_mmu_rmap_size" },
...
{ "execute": "query-stats-instances" }
{ "return": [
{ "name": "vcpu_1",
"type": "kvm-vcpu" },
{ "name": "vcpu_0",
"type": "kvm-vcpu" },
{ "name": "vm",
"type": "kvm-vm" } ]
}
Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
---
include/monitor/monitor.h | 27 ++++++++
monitor/qmp-cmds.c | 71 +++++++++++++++++++
qapi/misc.json | 142 ++++++++++++++++++++++++++++++++++++++
3 files changed, 240 insertions(+)
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 12d395d62d..14d3432ade 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -56,4 +56,31 @@ void monitor_register_hmp(const char *name, bool info,
void monitor_register_hmp_info_hrt(const char *name,
HumanReadableText *(*handler)(Error **errp));
+/*
+ * Add qmp stats callbacks to the stats_callbacks list.
+ *
+ * @name: name of stats callbacks
+ * @stats_fn: routine to query stats - with options for name and type:
+ * StatsList *(*stats_fn)(StatsList *list_tail, bool has_name,
+ * const char *name, bool has_type, const char *type, Error **errp)
+ *
+ * @schema_fn: routine to query stat schemas - with an option for type:
+ * StatsSchemaList *(*schemas_fn)(StatsSchemaList *list tail, bool has_type,
+ * const char *type, Error **errp)
+ *
+ * @instance_fn: routine to query stat instances:
+ * StatsInstanceList *(*instances_fn)(StatsInstanceList *list_tail,
+ * Error **errp)
+ */
+void add_stats_callbacks(const char *name,
+ StatsList *(*stats_fn)(StatsList *,
+ bool, const char *,
+ bool, const char *,
+ Error **),
+ StatsSchemaList *(*schemas_fn)(StatsSchemaList *,
+ bool, const char *,
+ Error **),
+ StatsInstanceList *(*instances_fn)(StatsInstanceList *,
+ Error **));
+
#endif /* MONITOR_H */
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 343353e27a..c7bdff1e1c 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -466,3 +466,74 @@ HumanReadableText *qmp_x_query_irq(Error **errp)
return human_readable_text_from_str(buf);
}
+
+typedef struct StatsCallbacks {
+ char *name;
+ StatsList *(*stats_cb)(StatsList *, bool, const char *, bool,
+ const char *, Error **);
+ StatsSchemaList *(*schemas_cb)(StatsSchemaList *, bool, const char *,
+ Error **);
+ StatsInstanceList *(*instances_cb)(StatsInstanceList *, Error **);
+ QTAILQ_ENTRY(StatsCallbacks) next;
+} StatsCallbacks;
+
+static QTAILQ_HEAD(, StatsCallbacks) stats_callbacks =
+ QTAILQ_HEAD_INITIALIZER(stats_callbacks);
+
+void add_stats_callbacks(const char *name,
+ StatsList *(*stats_fn)(StatsList *,
+ bool, const char *,
+ bool, const char *,
+ Error **),
+ StatsSchemaList *(*schemas_fn)(StatsSchemaList *,
+ bool, const char *,
+ Error **),
+ StatsInstanceList *(*instances_fn)(StatsInstanceList *,
+ Error **))
+{
+ StatsCallbacks *entry = g_malloc0(sizeof(*entry));
+ entry->name = strdup(name);
+ entry->stats_cb = stats_fn;
+ entry->schemas_cb = schemas_fn;
+ entry->instances_cb = instances_fn;
+
+ QTAILQ_INSERT_TAIL(&stats_callbacks, entry, next);
+}
+
+StatsList *qmp_query_stats(bool has_name, const char *name, bool has_type,
+ const char *type, Error **errp) {
+ StatsList *list_tail = NULL;
+ StatsCallbacks *entry;
+
+ QTAILQ_FOREACH(entry, &stats_callbacks, next) {
+ list_tail = entry->stats_cb(list_tail, has_name, name,
+ has_type, type, errp);
+ }
+
+ return list_tail;
+}
+
+StatsSchemaList *qmp_query_stats_schemas(bool has_type, const char *type,
+ Error **errp)
+{
+ StatsSchemaList *list_tail = NULL;
+ StatsCallbacks *entry;
+
+ QTAILQ_FOREACH(entry, &stats_callbacks, next) {
+ list_tail = entry->schemas_cb(list_tail, has_type, type, errp);
+ }
+
+ return list_tail;
+}
+
+StatsInstanceList *qmp_query_stats_instances(Error **errp)
+{
+ StatsInstanceList *list_tail = NULL;
+ StatsCallbacks *entry;
+
+ QTAILQ_FOREACH(entry, &stats_callbacks, next) {
+ list_tail = entry->instances_cb(list_tail, errp);
+ }
+
+ return list_tail;
+}
diff --git a/qapi/misc.json b/qapi/misc.json
index 358548abe1..a0a07ef0b1 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -527,3 +527,145 @@
'data': { '*option': 'str' },
'returns': ['CommandLineOptionInfo'],
'allow-preconfig': true }
+
+##
+# @StatType:
+#
+# Enumeration of stat 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.
+#
+# Since: 7.0
+##
+{ 'enum' : 'StatType',
+ 'data' : [ 'cumulative', 'instant', 'peak' ] }
+
+##
+# @StatUnit:
+#
+# Enumeration of stat units
+# @bytes: stat reported in bytes.
+# @seconds: stat reported in seconds.
+# @cycles: stat reported in clock cycles.
+# @none: no unit for this stat.
+#
+# Since: 7.0
+##
+{ 'enum' : 'StatUnit',
+ 'data' : [ 'bytes', 'seconds', 'cycles', 'none' ] }
+
+##
+# @StatData:
+#
+# Individual stat
+# @name: Stat name
+# @type: @StatType
+# @unit: @StatUnit
+# @base: Exponent base (2 or 10)
+# @exponent: Used together with @base
+# @val: List of uint64 values
+#
+# Since: 7.0
+##
+{ 'struct': 'StatData',
+ 'data': { 'name': 'str',
+ 'type': 'StatType',
+ 'unit': 'StatUnit',
+ 'base': 'uint8',
+ 'exponent': 'int16',
+ 'val': [ 'uint64' ] } }
+
+##
+# @Stats:
+#
+# Stats per resource (e.g. vm or vcpu)
+# @name: Resource name
+# @stats: List of @StatData
+#
+# Since: 7.0
+##
+{ 'struct': 'Stats',
+ 'data': {'name': 'str',
+ 'type': 'StatSchemaType',
+ 'stats': [ 'StatData' ] } }
+
+##
+# @query-stats:
+#
+# @name: Stat name (optional)
+# @type: Type name (optional)
+# Returns: List of @Stats
+#
+# Since: 7.0
+##
+{ 'command': 'query-stats',
+ 'data': { '*name': 'str', '*type': 'str' },
+ 'returns': [ 'Stats' ] }
+
+##
+# @StatSchemaType:
+#
+# Enumeration of stats schema types
+#
+# Since: 7.0
+##
+{ 'enum' : 'StatSchemaType',
+ 'data' : [ ] }
+
+##
+# @StatSchemaEntry:
+#
+# Individual stat in a schema type
+#
+# Since: 7.0
+##
+{ 'struct': 'StatSchemaEntry',
+ 'data': { 'name': 'str' } }
+
+##
+# @StatsSchema:
+#
+# Stats per @StatSchemaType
+# @type: @StatSchemaType
+# @stats: @StatCchemaName
+#
+# Since: 7.0
+##
+{ 'struct': 'StatsSchema',
+ 'data': { 'type': 'StatSchemaType',
+ 'stats': [ 'StatSchemaEntry' ] } }
+
+##
+# @query-stats-schemas:
+#
+# @type: type name (optional)
+# Returns: List of @StatsSchema
+#
+# Since: 7.0
+##
+{ 'command': 'query-stats-schemas',
+ 'data': { '*type': 'str' },
+ 'returns': [ 'StatsSchema' ] }
+
+##
+# @StatsInstance:
+#
+# @name: resource name
+# @type: @StatSchemaType
+#
+# Since: 7.0
+##
+{ 'struct': 'StatsInstance',
+ 'data': { 'name': 'str',
+ 'type': 'StatSchemaType' } }
+
+##
+# @query-stats-instances:
+#
+# Returns list of @StatsInstance
+#
+# Since: 7.0
+##
+{ 'command': 'query-stats-instances',
+ 'returns': [ 'StatsInstance' ] }
--
2.26.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 2/3] hmp: Support for querying stats
2021-11-19 19:51 [PATCH v2 0/3] Support fd-based KVM stats Mark Kanda
2021-11-19 19:51 ` [PATCH v2 1/3] qmp: Support for querying stats Mark Kanda
@ 2021-11-19 19:51 ` Mark Kanda
2021-11-19 19:51 ` [PATCH v2 3/3] kvm: Support for querying fd-based stats Mark Kanda
2022-01-15 16:22 ` [PATCH v2 0/3] Support fd-based KVM stats Paolo Bonzini
3 siblings, 0 replies; 16+ messages in thread
From: Mark Kanda @ 2021-11-19 19:51 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini
Leverage the QMP support for querying stats. The interface supports
the same arguments as the QMP interface.
Examples (with fd-based KVM stats):
(qemu) info stats
vcpu_1 (kvm-vcpu):
guest_mode (instant): 0
directed_yield_successful (cumulative): 0
...
vcpu_0 (kvm-vcpu):
guest_mode (instant): 0
...
vm (kvm-vm):
max_mmu_page_hash_collisions (peak): 0
max_mmu_rmap_size (peak): 0
...
(qemu) info stats-schemas
kvm-vcpu:
guest_mode
directed_yield_successful
...
kvm-vm:
max_mmu_page_hash_collisions
max_mmu_rmap_size
...
(qemu) info stats-instances
name type
---- ----
vcpu_1 kvm-vcpu
vcpu_0 kvm-vcpu
vm kvm-vm
Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
---
hmp-commands-info.hx | 40 ++++++++++++++
include/monitor/hmp.h | 3 +
monitor/hmp-cmds.c | 125 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 168 insertions(+)
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index 407a1da800..0d5f025adb 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -877,3 +877,43 @@ SRST
``info sgx``
Show intel SGX information.
ERST
+
+ {
+ .name = "stats",
+ .args_type = "name:s?,type:s?",
+ .params = "[name] [type]",
+ .help = "show statistics; optional name and type",
+ .cmd = hmp_info_stats,
+ },
+
+SRST
+ ``stats``
+ Show stats
+ERST
+
+ {
+ .name = "stats-schemas",
+ .args_type = "type:s?",
+ .params = "[type]",
+ .help = "show stats for schema type; optional type",
+ .cmd = hmp_info_stats_schemas,
+ },
+
+SRST
+ ``stats-schemas``
+ Show stats for schema type
+ERST
+
+
+ {
+ .name = "stats-instances",
+ .args_type = "",
+ .params = "",
+ .help = "show current stat instances",
+ .cmd = hmp_info_stats_instances,
+ },
+
+SRST
+ ``stats-instances``
+ Show current stat instances
+ERST
diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index 96d014826a..3670280a6b 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -133,5 +133,8 @@ 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);
+void hmp_info_stats_schemas(Monitor *mon, const QDict *qdict);
+void hmp_info_stats_instances(Monitor *mon, const QDict *qdict);
#endif
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 9c91bf93e9..9752ca6aa8 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -2165,3 +2165,128 @@ void hmp_info_memory_size_summary(Monitor *mon, const QDict *qdict)
}
hmp_handle_error(mon, err);
}
+
+void hmp_info_stats(Monitor *mon, const QDict *qdict)
+{
+ StatsList *stats_list, *stats_list_entry;
+ Stats *stats_entry;
+ StatDataList *data_entry;
+ StatData *stat;
+ uint64List *val;
+ const char *name, *type;
+ Error *err = NULL;
+
+ name = qdict_get_try_str(qdict, "name");
+ type = qdict_get_try_str(qdict, "type");
+
+ stats_list = qmp_query_stats(name ? TRUE : FALSE, name,
+ type ? TRUE : FALSE, type, &err);
+
+ if (err) {
+ monitor_printf(mon, "%s\n", error_get_pretty(err));
+ error_free(err);
+ return;
+ }
+
+ for (stats_list_entry = stats_list; stats_list_entry;
+ stats_list_entry = stats_list_entry->next) {
+ stats_entry = stats_list_entry->value;
+ monitor_printf(mon, "\n%s (%s):\n", stats_entry->name,
+ StatSchemaType_str(stats_entry->type));
+
+ for (data_entry = stats_entry->stats; data_entry;
+ data_entry = data_entry->next) {
+ stat = data_entry->value;
+ monitor_printf(mon, " %s (%s):", stat->name,
+ StatType_str(stat->type));
+
+ for (val = stat->val; val; val = val->next) {
+ if (stat->unit == STAT_UNIT_SECONDS &&
+ stat->exponent >= -9 && stat->exponent <= 0 &&
+ stat->exponent % 3 == 0 && stat->base == 10) {
+ const char *si_prefix[] = { "", "milli", "micro", "nano" };
+ monitor_printf(mon, " %lu %s", val->value,
+ si_prefix[stat->exponent / -3]);
+ } else if (
+ stat->unit == STAT_UNIT_BYTES &&
+ stat->exponent >= 0 && stat->exponent <= 40 &&
+ stat->exponent % 10 == 0 && stat->base == 2) {
+ const char *si_prefix[] = {
+ "", "kilo", "mega", "giga", "tera" };
+ monitor_printf(mon, " %lu %s", val->value,
+ si_prefix[stat->exponent / 10]);
+ } else if (stat->exponent) {
+ /* Print the base and exponent as "*<base>^<exp>" */
+ monitor_printf(mon, " %lu*%d^%d", val->value,
+ stat->base, stat->exponent);
+ } else {
+ monitor_printf(mon, " %lu", val->value);
+ }
+
+ /* Don't print "none" unit type */
+ monitor_printf(mon, "%s\n", stat->unit == STAT_UNIT_NONE ?
+ "" : StatUnit_str(stat->unit));
+ }
+ }
+ }
+ qapi_free_StatsList(stats_list);
+}
+
+void hmp_info_stats_schemas(Monitor *mon, const QDict *qdict)
+{
+ StatsSchemaList *stats_list, *stats_list_entry;
+ StatsSchema *stats_entry;
+ StatSchemaEntryList *data_entry;
+ StatSchemaEntry *stat;
+ const char *type;
+ Error *err = NULL;
+
+ type = qdict_get_try_str(qdict, "type");
+
+ stats_list = qmp_query_stats_schemas(type ? TRUE : FALSE, type, &err);
+
+ if (err) {
+ monitor_printf(mon, "%s\n", error_get_pretty(err));
+ error_free(err);
+ return;
+ }
+
+ for (stats_list_entry = stats_list; stats_list_entry;
+ stats_list_entry = stats_list_entry->next) {
+ stats_entry = stats_list_entry->value;
+ monitor_printf(mon, "\n%s:\n", StatSchemaType_str(stats_entry->type));
+
+ for (data_entry = stats_entry->stats; data_entry;
+ data_entry = data_entry->next) {
+ stat = data_entry->value;
+ monitor_printf(mon, " %s\n", stat->name);
+ }
+ }
+ qapi_free_StatsSchemaList(stats_list);
+}
+
+void hmp_info_stats_instances(Monitor *mon, const QDict *qdict)
+{
+ StatsInstanceList *stats_list, *stats_list_entry;
+ StatsInstance *stats_entry;
+ Error *err = NULL;
+
+ stats_list = qmp_query_stats_instances(&err);
+
+ if (err) {
+ monitor_printf(mon, "%s\n", error_get_pretty(err));
+ error_free(err);
+ return;
+ }
+
+ monitor_printf(mon, "%-15s %s\n", "name", "type");
+ monitor_printf(mon, "%-15s %s\n", "----", "----");
+
+ for (stats_list_entry = stats_list; stats_list_entry;
+ stats_list_entry = stats_list_entry->next) {
+ stats_entry = stats_list_entry->value;
+ monitor_printf(mon, "%-15s %s\n", stats_entry->name,
+ StatSchemaType_str(stats_entry->type));
+ }
+ qapi_free_StatsInstanceList(stats_list);
+}
--
2.26.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 3/3] kvm: Support for querying fd-based stats
2021-11-19 19:51 [PATCH v2 0/3] Support fd-based KVM stats Mark Kanda
2021-11-19 19:51 ` [PATCH v2 1/3] qmp: Support for querying stats Mark Kanda
2021-11-19 19:51 ` [PATCH v2 2/3] hmp: " Mark Kanda
@ 2021-11-19 19:51 ` Mark Kanda
2022-01-15 16:22 ` [PATCH v2 0/3] Support fd-based KVM stats Paolo Bonzini
3 siblings, 0 replies; 16+ messages in thread
From: Mark Kanda @ 2021-11-19 19:51 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini
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")
Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
---
accel/kvm/kvm-all.c | 399 ++++++++++++++++++++++++++++++++++++++++++++
qapi/misc.json | 2 +-
2 files changed, 400 insertions(+), 1 deletion(-)
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index eecd8031cf..10e8b8ed5c 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -47,6 +47,8 @@
#include "kvm-cpus.h"
#include "hw/boards.h"
+#include "qapi/qapi-commands-misc.h"
+#include "monitor/monitor.h"
/* This check must be after config-host.h is included */
#ifdef CONFIG_EVENTFD
@@ -2303,6 +2305,15 @@ bool kvm_dirty_ring_enabled(void)
return kvm_state->kvm_dirty_ring_size ? true : false;
}
+static StatsList *query_stats_cb(StatsList *, bool, const char *, bool,
+ const char *, Error **);
+
+static StatsSchemaList *query_stats_schemas_cb(StatsSchemaList *, bool,
+ const char *, Error **);
+
+static StatsInstanceList *query_stats_instances_cb(StatsInstanceList *,
+ Error **);
+
static int kvm_init(MachineState *ms)
{
MachineClass *mc = MACHINE_GET_CLASS(ms);
@@ -2612,6 +2623,11 @@ static int kvm_init(MachineState *ms)
}
}
+ if (kvm_check_extension(kvm_state, KVM_CAP_BINARY_STATS_FD)) {
+ add_stats_callbacks("kvm", &query_stats_cb, &query_stats_schemas_cb,
+ &query_stats_instances_cb);
+ }
+
return 0;
err:
@@ -3667,3 +3683,386 @@ static void kvm_type_init(void)
}
type_init(kvm_type_init);
+
+typedef struct StatsArgs {
+ void *kvm_stat;
+ char *name;
+ bool query_schema;
+ Error **errp;
+} StatsArgs;
+
+static StatDataList *add_kvmstat_entry(struct kvm_stats_desc *pdesc,
+ uint64_t *stats_data,
+ StatDataList *data_list,
+ Error **errp)
+{
+ StatDataList *data_entry;
+ uint64List *val_list = NULL;
+ int i;
+
+ data_entry = g_malloc0(sizeof(*data_entry));
+ data_entry->value = g_malloc0(sizeof(*data_entry->value));
+ data_entry->value->name = g_strdup(pdesc->name);
+
+ /* Convert flags to type, unit and base (QAPI auto-generated enums) */
+ switch (pdesc->flags & KVM_STATS_TYPE_MASK) {
+ case KVM_STATS_TYPE_CUMULATIVE:
+ data_entry->value->type = STAT_TYPE_CUMULATIVE;
+ break;
+ case KVM_STATS_TYPE_INSTANT:
+ data_entry->value->type = STAT_TYPE_INSTANT;
+ break;
+ case KVM_STATS_TYPE_PEAK:
+ data_entry->value->type = STAT_TYPE_PEAK;
+ break;
+ default:
+ /* Unknown type - skip */
+ goto exit;
+ }
+
+ switch (pdesc->flags & KVM_STATS_UNIT_MASK) {
+ case KVM_STATS_UNIT_NONE:
+ data_entry->value->unit = STAT_UNIT_NONE;
+ break;
+ case KVM_STATS_UNIT_BYTES:
+ data_entry->value->unit = STAT_UNIT_BYTES;
+ break;
+ case KVM_STATS_UNIT_CYCLES:
+ data_entry->value->unit = STAT_UNIT_CYCLES;
+ break;
+ case KVM_STATS_UNIT_SECONDS:
+ data_entry->value->unit = STAT_UNIT_SECONDS;
+ break;
+ default:
+ /* Unknown unit - skip */
+ goto exit;
+ }
+
+ switch (pdesc->flags & KVM_STATS_BASE_MASK) {
+ case KVM_STATS_BASE_POW10:
+ data_entry->value->base = 10;
+ break;
+ case KVM_STATS_BASE_POW2:
+ data_entry->value->base = 2;
+ break;
+ default:
+ /* Unknown base - skip */
+ goto exit;
+ }
+
+ data_entry->value->exponent = pdesc->exponent;
+
+ /* Alloc and populate data list */
+ for (i = 0; i < pdesc->size; i++) {
+ uint64List *val_entry = g_malloc0(sizeof(*val_entry));
+ val_entry->value = stats_data[i];
+ val_entry->next = val_list;
+ val_list = val_entry;
+ }
+ data_entry->value->val = val_list;
+ data_entry->next = data_list;
+ data_list = data_entry;
+
+ return data_list;
+
+exit:
+ g_free(data_entry->value->name);
+ g_free(data_entry->value);
+ g_free(data_entry);
+
+ return data_list;
+}
+
+static StatSchemaEntryList *add_kvmschema_entry(struct kvm_stats_desc *pdesc,
+ StatSchemaEntryList *data_list,
+ Error **errp)
+{
+ StatSchemaEntryList *data_entry;
+
+ data_entry = g_malloc0(sizeof(*data_entry));
+ data_entry->value = g_malloc0(sizeof(*data_entry->value));
+ data_entry->value->name = g_strdup(pdesc->name);
+
+ data_entry->next = data_list;
+ data_list = data_entry;
+
+ return data_list;
+}
+
+static void query_stats(StatsArgs *kvm_stat_args, int stats_fd)
+{
+ size_t size_desc, size_data;
+ struct kvm_stats_header *header;
+ struct kvm_stats_desc *stats_desc = NULL;
+ Error *local_err = NULL;
+ void *data_list = NULL;
+ char *id = NULL;
+ ssize_t ret;
+ int i;
+
+ /* Read kvm stats header */
+ header = g_malloc(sizeof(*header));
+ ret = read(stats_fd, header, sizeof(*header));
+ if (ret != sizeof(*header)) {
+ error_setg(&local_err, "KVM stats: failed to read stats header: "
+ "expected %zu actual %zu", sizeof(*header), ret);
+ goto exit;
+ }
+ size_desc = sizeof(*stats_desc) + header->name_size;
+
+ /* Read kvm stats id string */
+ id = g_malloc(header->name_size);
+ ret = read(stats_fd, id, header->name_size);
+ if (ret != header->name_size) {
+ error_setg(&local_err, "KVM stats: failed to read id string: "
+ "expected %zu actual %zu", (size_t) header->name_size, ret);
+ goto exit;
+ }
+
+ /* Read kvm stats descriptors */
+ stats_desc = g_malloc0(header->num_desc * size_desc);
+ ret = pread(stats_fd, stats_desc,
+ size_desc * header->num_desc, header->desc_offset);
+
+ if (ret != size_desc * header->num_desc) {
+ error_setg(&local_err, "KVM stats: failed to read stats descriptors: "
+ "expected %zu actual %zu",
+ size_desc * header->num_desc, ret);
+ goto exit;
+ }
+
+ for (i = 0; i < header->num_desc; ++i) {
+ struct kvm_stats_desc *pdesc = (void *)stats_desc + i * size_desc;
+ size_data = pdesc->size * sizeof(uint64_t);
+ uint64_t *stats_data = g_malloc(size_data);
+
+ ret = pread(stats_fd, stats_data, size_data,
+ header->data_offset + pdesc->offset);
+
+ if (ret != pdesc->size * sizeof(*stats_data)) {
+ error_setg(&local_err, "KVM stats: failed to read data: "
+ "expected %zu actual %zu",
+ pdesc->size * sizeof(*stats_data), ret);
+ g_free(stats_data);
+ goto exit;
+ }
+
+ if (kvm_stat_args->name) {
+ if (g_strcmp0(kvm_stat_args->name, pdesc->name)) {
+ g_free(stats_data);
+ continue;
+ }
+ }
+
+ /* Add entry to the list */
+ if (kvm_stat_args->query_schema) {
+ data_list = add_kvmschema_entry(pdesc, (StatSchemaEntryList *)
+ data_list, &local_err);
+ } else {
+ data_list = add_kvmstat_entry(pdesc, stats_data, (StatDataList *)
+ data_list, &local_err);
+ }
+ g_free(stats_data);
+ }
+
+ if (kvm_stat_args->query_schema) {
+ struct StatsSchema *kvm_stat =
+ (struct StatsSchema *) kvm_stat_args->kvm_stat;
+
+ kvm_stat->stats = (StatSchemaEntryList *)data_list;
+ } else {
+ struct Stats *kvm_stat = (struct Stats *) kvm_stat_args->kvm_stat;
+
+ kvm_stat->stats = (StatDataList *)data_list;
+ }
+exit:
+ error_propagate(kvm_stat_args->errp, local_err);
+ g_free(stats_desc);
+ g_free(id);
+ g_free(header);
+}
+
+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, stats_fd);
+ close(stats_fd);
+}
+
+static StatsSchemaList *add_stats_schema(StatsSchemaList *list_tail,
+ StatsArgs *stats_args,
+ const char *type)
+{
+ StatsSchema *value = g_malloc0(sizeof(*value));
+
+ for (int i = 0; i < STAT_SCHEMA_TYPE__MAX; i++) {
+ if (!g_strcmp0(type, StatSchemaType_str(i))) {
+ value->type = i;
+ }
+ }
+
+ QAPI_LIST_PREPEND(list_tail, value);
+ stats_args->kvm_stat = value;
+ stats_args->query_schema = TRUE;
+
+ return list_tail;
+}
+
+static StatsList *add_stats(StatsList *list_tail,
+ StatsArgs *stats_args,
+ const char *name,
+ const char *type)
+{
+ Stats *value = g_malloc0(sizeof(*value));
+ value->name = g_strdup(name);
+
+ for (int i = 0; i < STAT_SCHEMA_TYPE__MAX; i++) {
+ if (!g_strcmp0(type, StatSchemaType_str(i))) {
+ value->type = i;
+ }
+ }
+
+ QAPI_LIST_PREPEND(list_tail, value);
+ stats_args->kvm_stat = value;
+
+ return list_tail;
+}
+
+static StatsArgs *prepare_stats_args(const char *name, Error **errp)
+{
+ StatsArgs *stats_args;
+ KVMState *s = kvm_state;
+ Error *local_err = NULL;
+
+ if (!kvm_enabled()) {
+ error_setg(&local_err, "KVM stats: KVM not enabled");
+ error_propagate(errp, local_err);
+ return NULL;
+ }
+
+ if (!kvm_check_extension(s, KVM_CAP_BINARY_STATS_FD)) {
+ error_setg(&local_err, "KVM stats: not supported");
+ error_propagate(errp, local_err);
+ return NULL;
+ }
+
+ stats_args = g_malloc0(sizeof(*stats_args));
+ stats_args->errp = errp;
+ stats_args->name = g_strdup(name);
+
+ return stats_args;
+}
+
+static StatsList *query_stats_cb(StatsList *list_tail,
+ bool has_name, const char *name,
+ bool has_type, const char *type,
+ Error **errp)
+{
+ KVMState *s = kvm_state;
+ CPUState *cpu;
+ int stats_fd;
+ StatsArgs *stats_args = NULL;
+ StatSchemaType schema_type;
+
+ /* Query vm stats */
+ stats_fd = kvm_vm_ioctl(s, KVM_GET_STATS_FD, NULL);
+ if (stats_fd == -1) {
+ error_setg(errp, "KVM stats: ioctl failed");
+ return NULL;
+ }
+
+ stats_args = prepare_stats_args(name, errp);
+ if (!stats_args) {
+ return NULL;
+ }
+
+ schema_type = STAT_SCHEMA_TYPE_KVM_VM;
+ if (!type || !g_strcmp0(StatSchemaType_str(schema_type), type)) {
+ list_tail = add_stats(list_tail, stats_args, "vm",
+ StatSchemaType_str(schema_type));
+ query_stats(stats_args, stats_fd);
+ }
+
+ /* Query vcpu stats */
+ CPU_FOREACH(cpu) {
+ schema_type = STAT_SCHEMA_TYPE_KVM_VCPU;
+ if (type && g_strcmp0(StatSchemaType_str(schema_type), type)) {
+ continue;
+ }
+ char *resname = g_strdup_printf("vcpu_%d", cpu->cpu_index);
+ list_tail = add_stats(list_tail, stats_args, resname,
+ StatSchemaType_str(schema_type));
+ run_on_cpu(cpu, query_stats_vcpu, RUN_ON_CPU_HOST_PTR(stats_args));
+ }
+
+ g_free(stats_args);
+ return list_tail;
+}
+
+static StatsSchemaList *query_stats_schemas_cb(StatsSchemaList *list_tail,
+ bool has_type, const char *type,
+ Error **errp)
+{
+ KVMState *s = kvm_state;
+ int stats_fd;
+ StatsArgs *stats_args = NULL;
+ StatSchemaType schema_type;
+
+ stats_fd = kvm_vm_ioctl(s, KVM_GET_STATS_FD, NULL);
+ if (stats_fd == -1) {
+ error_setg(errp, "KVM stats: ioctl failed");
+ return NULL;
+ }
+
+ stats_args = prepare_stats_args(NULL, errp);
+ if (!stats_args) {
+ return NULL;
+ }
+
+ /* Query vm */
+ schema_type = STAT_SCHEMA_TYPE_KVM_VM;
+ if (!type || !g_strcmp0(StatSchemaType_str(schema_type), type)) {
+ list_tail = add_stats_schema(list_tail, stats_args,
+ StatSchemaType_str(schema_type));
+ query_stats(stats_args, stats_fd);
+ }
+
+ /* Query vcpu */
+ schema_type = STAT_SCHEMA_TYPE_KVM_VCPU;
+ if (type && g_strcmp0(StatSchemaType_str(schema_type), type)) {
+ return list_tail;
+ }
+ list_tail = add_stats_schema(list_tail, stats_args,
+ StatSchemaType_str(schema_type));
+ run_on_cpu(first_cpu, query_stats_vcpu, RUN_ON_CPU_HOST_PTR(stats_args));
+
+ g_free(stats_args);
+ return list_tail;
+}
+
+static StatsInstanceList *query_stats_instances_cb(StatsInstanceList *list_tail,
+ Error **errp)
+{
+ CPUState *cpu;
+
+ StatsInstance *value = g_malloc0(sizeof(*value));
+ value->name = g_strdup("vm");
+ value->type = STAT_SCHEMA_TYPE_KVM_VM;
+ QAPI_LIST_PREPEND(list_tail, value);
+
+ CPU_FOREACH(cpu) {
+ value = g_malloc0(sizeof(*value));
+ value->name = g_strdup_printf("vcpu_%d", cpu->cpu_index);
+ value->type = STAT_SCHEMA_TYPE_KVM_VCPU;
+ QAPI_LIST_PREPEND(list_tail, value);
+ }
+ return list_tail;
+}
diff --git a/qapi/misc.json b/qapi/misc.json
index a0a07ef0b1..77e3962a1d 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -611,7 +611,7 @@
# Since: 7.0
##
{ 'enum' : 'StatSchemaType',
- 'data' : [ ] }
+ 'data' : [ 'kvm-vm', 'kvm-vcpu' ] }
##
# @StatSchemaEntry:
--
2.26.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/3] qmp: Support for querying stats
2021-11-19 19:51 ` [PATCH v2 1/3] qmp: Support for querying stats Mark Kanda
@ 2021-12-07 18:42 ` Daniel P. Berrangé
2022-01-17 12:05 ` Paolo Bonzini
0 siblings, 1 reply; 16+ messages in thread
From: Daniel P. Berrangé @ 2021-12-07 18:42 UTC (permalink / raw)
To: Mark Kanda; +Cc: pbonzini, qemu-devel, armbru
Copying in Markus as QAPI maintainer, since I feel this proposed
design is a little oddball from typical QAPI design approach....
On Fri, Nov 19, 2021 at 01:51:51PM -0600, Mark Kanda wrote:
> Introduce qmp support for querying stats. Provide a framework for
> adding new stats and support for the following commands:
>
> - query-stats
> Returns a list of all stats, with options for specifying a stat
> name and schema type. A schema type is the set of stats associated
> with a given component (e.g. vm or vcpu).
>
> - query-stats-schemas
> Returns a list of stats included in each schema type, with an
> option for specifying the schema name.
>
> - query-stats-instances
> Returns a list of stat instances and their associated schema type.
>
> The framework provides a method to register callbacks for these qmp
> commands.
>
> The first usecase will be for fd-based KVM stats (in an upcoming
> patch).
>
> Examples (with fd-based KVM stats):
>
> { "execute": "query-stats" }
> { "return": [
> { "name": "vcpu_1",
> "type": "kvm-vcpu",
> "stats": [
> { "name": "guest_mode",
> "unit": "none",
> "base": 10,
> "val": [ 0 ],
> "exponent": 0,
> "type": "instant" },
> { "name": "directed_yield_successful",
> "unit": "none",
> "base": 10,
> "val": [ 0 ],
> "exponent": 0,
> "type": "cumulative" },
> ...
> },
> { "name": "vcpu_0",
> "type": "kvm-vcpu",
> "stats": ...
> ...
> },
> { "name": "vm",
> "type": "kvm-vm",
> "stats": [
> { "name": "max_mmu_page_hash_collisions",
> "unit": "none",
> "base": 10,
> "val": [ 0 ],
> "exponent": 0,
> "type": "peak" },
> ...
So this is essentially exposing the low level kernel data structure
'struct kvm_stats_desc' mapped 1-to-1 into QAPI.
There are pros/cons to doing that should be explored to see whether
this actually makes sense for the QMP design.
I understand this design is intended to be fully self-describing
such that we can add arbitrarily more fields without ever
changing QEMU code, and with a simple mapping from the kernel
kvm_stats_desc.
Taking the first level of data returned, we see the natural
structure of the data wrt vCPUs is flattened:
{ "return": [
{ "name": "vcpu_0",
"type": "kvm-vcpu",
"stats": [...], // stats for vcpu 0
},
{ "name": "vcpu_1",
"type": "kvm-vcpu",
"stats": [...], // stats for vcpu 0
},
...other vCPUs...
{ "name": "vm",
"type": "kvm-vm",
"stats": [...], // stats for the VM
},
}
This name+type stuff is all unnecessarily indirect. If we ever
have to add more data unrelated to the kvm stats, we're going
to need QEMU changes no matter what, so this indirect structure
isn't future proofing it.
I'd rather expect us to have named struct fields for each
different provider of data, respecting the natural hierachy.
ie use an array for vCPU data.
I understand this is future proofed to be able to support
non-KVM stats. If we have KVM per-vCPU stat and non-KVM
per-VCPU stats at the same time, I'd expect them all to
appear in the same place. IOW, overall I'd expect to
see grouping more like
{ "return": {
"vcpus": [
[ // stats for vcpu 0
{ "provider": 'kvm',
"stats": [...] },
{ "provider": 'qemu',
"stats"; [...] }
],
[ // stats for vcpu 1
{ "provider": 'kvm',
"stats": [...] },
{ "provider": 'qemu',
"stats"; [...] }
],
...other vCPUs...
]
"vm": [
{ "provider": 'kvm',
"stats": [...] },
{ "provider": 'qemu',
"stats"; [...] } ],
],
}
Now onto the values being reported. AFAICT from the kernel
docs, for all the types of data it currently reports
(cumulative, instant, peak, none), there is only ever going
to be a single value. I assume the ability to report multiple
values is future proofing for a later requirement. This is
fine from a kerenl POV since they're trying to fit this into
a flat struct. QAPI is way more flexible. It can switch
between reporting an scale or array or scalars for the
same field. So if we know the stat will only ever have
1 value, we should be reporting a scalar, not an array
which will only ever have one value.
Second, for a given named statistic, AFAICT, the data type,
unit, base and exponent are all fixed. I don't see a reason
for us to be reporting that information every time we call
'query-stats'. Just report the name + value(s). Apps that
want a specific named stat won't care about the dimensions,
because they'll already know what the value means.
Apps that want to be metadata driven to handle arbitrary
stats, can just call 'query-stats-schemas' to learn about
the dimensions one time.
This will give waaay lower data transfer for querying
values repeatedly.
>
> { "execute": "query-stats-schemas" }
> { "return": [
> { "type": "kvm-vcpu",
> "stats": [
> { "name": "guest_mode" },
> { "name": "directed_yield_successful" },
> ...
> },
> { "type": "kvm-vm",
> "stats": [
> { "name": "max_mmu_page_hash_collisions" },
> { "name": "max_mmu_rmap_size" },
> ...
...this can be used to introspect the data type, unit,
base, exponent as a one time task, if needed.
Again, I'd expect the first level of nested to be
removed to mirror 'query-stats',
{ "return": {
"vcpu": [
{
"provider": "kvm",
"stats": [
{ "name": "guest_mode",
"unit": "none",
"base": 10,
"exponent": 0 },
{ "name": "directed_yield_successful"
"unit": "none",
"base": 10,
"exponent": 0 },
},
],
},
{
"provider": "qemu"
"stats": [
{
"name": "something_blah_blah",
"unit": "bytes",
"base": 2,
"exponent": 20,
},
]
},
],
"vm": [
{
"provider": "kvm",
"stats": [
{ "name": "max_mmu_page_hash_collisions", ... }
{ "name": "max_mmu_rmap_size", ... }
]
},
{
"provider": "qemu",
"stats": [
{ "name": "blah", ... }
]
},
}
}
>
> { "execute": "query-stats-instances" }
> { "return": [
> { "name": "vcpu_1",
> "type": "kvm-vcpu" },
> { "name": "vcpu_0",
> "type": "kvm-vcpu" },
> { "name": "vm",
> "type": "kvm-vm" } ]
> }
I don't see a need for this command at all. It doesn't tell
apps anything they can't already learn from "query-stats-schemas"
New 'type' values will involve QEMU code changes no matter what.
So in the even we need something other than 'vcpu' and 'vm' stats
reported by 'query-stats', apps can just query the QAPI schema
in the normal manner to learn about struct field names that exist
in this QEMU version.
IOW, this really just re-invented QAPI introspection for no
benefit IMHO.
> diff --git a/qapi/misc.json b/qapi/misc.json
> index 358548abe1..a0a07ef0b1 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -527,3 +527,145 @@
> 'data': { '*option': 'str' },
> 'returns': ['CommandLineOptionInfo'],
> 'allow-preconfig': true }
> +
> +##
> +# @StatType:
> +#
> +# Enumeration of stat 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.
> +#
> +# Since: 7.0
> +##
> +{ 'enum' : 'StatType',
> + 'data' : [ 'cumulative', 'instant', 'peak' ] }
> +
> +##
> +# @StatUnit:
> +#
> +# Enumeration of stat units
> +# @bytes: stat reported in bytes.
> +# @seconds: stat reported in seconds.
> +# @cycles: stat reported in clock cycles.
> +# @none: no unit for this stat.
> +#
> +# Since: 7.0
> +##
> +{ 'enum' : 'StatUnit',
> + 'data' : [ 'bytes', 'seconds', 'cycles', 'none' ] }
> +
> +##
> +# @StatData:
> +#
> +# Individual stat
> +# @name: Stat name
> +# @type: @StatType
> +# @unit: @StatUnit
> +# @base: Exponent base (2 or 10)
> +# @exponent: Used together with @base
> +# @val: List of uint64 values
> +#
> +# Since: 7.0
> +##
> +{ 'struct': 'StatData',
> + 'data': { 'name': 'str',
> + 'type': 'StatType',
> + 'unit': 'StatUnit',
> + 'base': 'uint8',
> + 'exponent': 'int16',
> + 'val': [ 'uint64' ] } }
> +
> +##
> +# @Stats:
> +#
> +# Stats per resource (e.g. vm or vcpu)
> +# @name: Resource name
> +# @stats: List of @StatData
> +#
> +# Since: 7.0
> +##
> +{ 'struct': 'Stats',
> + 'data': {'name': 'str',
> + 'type': 'StatSchemaType',
> + 'stats': [ 'StatData' ] } }
> +
> +##
> +# @query-stats:
> +#
> +# @name: Stat name (optional)
> +# @type: Type name (optional)
> +# Returns: List of @Stats
> +#
> +# Since: 7.0
> +##
> +{ 'command': 'query-stats',
> + 'data': { '*name': 'str', '*type': 'str' },
> + 'returns': [ 'Stats' ] }
The 'name' and 'type' are used for filtering I presume. Only allowing
a single value for each feels pretty inflexible. I'd say we want to
allow mutliple requests at a time for efficiency.
Bearing in mind my other suggestions above, I'd think we should have
something more like
{ 'enum': 'StatsProvider',
'data': ["kvm", "qemu", "tcg", ....],
}
{ 'struct': 'StatsRequest',
'data': {
'provider': 'StatsProvider',
// If omitted, report everything for this provider
'*fields': [ 'str' ]
}
}
{ 'struct': 'StatsVCPURequest',
'base': 'StatsRequest',
'data': {
// To request subset of vCPUs e.g.
// "cpu_set": "0-3"
// Could use ['int'] instead if desired
'*cpu_set': str,
},
}
{ 'struct': 'StatsFilter',
'data': {
// If omitted means don't report that group of data
'*vcpu': 'StatsVCPURequest',
'*vm': 'StatsRequest',
},
}
{ 'alternate': 'StatsValue',
'data': { 'scalar': 'int64',
'list': [ 'int64 ' ] }
}
{ 'struct': 'StatsResultsEntry',
'data': {
'provider': 'StatsProvider',
'stats': [ 'StatsValue' ]
}
}
{ 'struct': 'StatsResults':
'data': {
'*vcpu': [ [ 'StatsResultsEntry' ] ],
'*vm': [ 'StatsResultsEntry' ]
}
}
{ 'command': 'query-stats',
'data': { 'filter': '*StatsFilter' },
'returns': 'StatsResults' }
> +
> +##
> +# @StatSchemaType:
> +#
> +# Enumeration of stats schema types
> +#
> +# Since: 7.0
> +##
> +{ 'enum' : 'StatSchemaType',
> + 'data' : [ ] }
> +
> +##
> +# @StatSchemaEntry:
> +#
> +# Individual stat in a schema type
> +#
> +# Since: 7.0
> +##
> +{ 'struct': 'StatSchemaEntry',
> + 'data': { 'name': 'str' } }
> +
> +##
> +# @StatsSchema:
> +#
> +# Stats per @StatSchemaType
> +# @type: @StatSchemaType
> +# @stats: @StatCchemaName
> +#
> +# Since: 7.0
> +##
> +{ 'struct': 'StatsSchema',
> + 'data': { 'type': 'StatSchemaType',
> + 'stats': [ 'StatSchemaEntry' ] } }
> +
> +##
> +# @query-stats-schemas:
> +#
> +# @type: type name (optional)
> +# Returns: List of @StatsSchema
> +#
> +# Since: 7.0
> +##
> +{ 'command': 'query-stats-schemas',
> + 'data': { '*type': 'str' },
> + 'returns': [ 'StatsSchema' ] }
I'd think this is more like
{ 'struct': 'StatsSchemaValue',
'data': {
'name': 'str',
'type': 'StatType',
'unit': 'StatUnit',
'base': 'uint8',
'exponent': 'int16',
},
}
{ 'struct': 'StatsSchemaProvider',
'data': {
'provider': 'StatsProvider',
'stats': [ 'StatsSchemaValue'],
}
}
{ 'struct': 'StatsSchemaResult',
'data': {
'vcpu': ['StatsSchemaProvider'],
'vm': ['StatsSchemaProvider'],
}
}
{ 'command': 'query-stats-schemas',
'returns': [ 'StatsSchemaResult' ] }
> +
> +##
> +# @StatsInstance:
> +#
> +# @name: resource name
> +# @type: @StatSchemaType
> +#
> +# Since: 7.0
> +##
> +{ 'struct': 'StatsInstance',
> + 'data': { 'name': 'str',
> + 'type': 'StatSchemaType' } }
> +
> +##
> +# @query-stats-instances:
> +#
> +# Returns list of @StatsInstance
> +#
> +# Since: 7.0
> +##
> +{ 'command': 'query-stats-instances',
> + 'returns': [ 'StatsInstance' ] }
As mentioned earlier, IMHO this doesn't need to exist.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/3] Support fd-based KVM stats
2021-11-19 19:51 [PATCH v2 0/3] Support fd-based KVM stats Mark Kanda
` (2 preceding siblings ...)
2021-11-19 19:51 ` [PATCH v2 3/3] kvm: Support for querying fd-based stats Mark Kanda
@ 2022-01-15 16:22 ` Paolo Bonzini
2022-01-16 23:17 ` Mark Kanda
3 siblings, 1 reply; 16+ messages in thread
From: Paolo Bonzini @ 2022-01-15 16:22 UTC (permalink / raw)
To: Mark Kanda, qemu-devel
On 11/19/21 20:51, Mark Kanda wrote:
> v2: [Paolo]
> - generalize the interface
> - add support for querying stat schema and instances
> - add additional HMP semantic processing for a few exponent/unit
> combinations (related to seconds and bytes)
>
> This patchset adds QEMU support for querying fd-based KVM stats. The
> kernel support was introduced by:
>
> cb082bfab59a ("KVM: stats: Add fd-based API to read binary stats data")
Hi Mark,
sorry for the late review. Fortunately there's very little that I'd change.
In particular:
* please change the callbacks to accept a NULL name and type, instead of
having the "bool"/"const char *" pair. You can probably benefit from a
function to cutils.c like
bool qemu_match_string(const char *value, const char *request) {
return !request || g_str_equal(value, request);
}
* please pass a single const struct to add_stats_callbacks, using GList
so that the struct can be const.
Putting both together it would be something like:
typedef struct StatsCallbacks {
char *name;
StatsList *(*get_values)(StatsList *list, const char *name,
const char *type, Error **errp);
StatsSchemaList *(*get_schemas)(StatsSchemaList *list,
const char *name, Error **errp);
StatsInstanceList *(*get_instances)(StatsInstanceList *list,
Error **errp);
} StatsCallbacks;
Finally, please put everything in a new header include/monitor/stats.h,
so that we can document everything and please it in docs/devel. I can
take care of that though.
Thanks,
Paolo
>
> Mark Kanda (3):
> qmp: Support for querying stats
> hmp: Support for querying stats
> kvm: Support for querying fd-based stats
>
> accel/kvm/kvm-all.c | 399 ++++++++++++++++++++++++++++++++++++++
> hmp-commands-info.hx | 40 ++++
> include/monitor/hmp.h | 3 +
> include/monitor/monitor.h | 27 +++
> monitor/hmp-cmds.c | 125 ++++++++++++
> monitor/qmp-cmds.c | 71 +++++++
> qapi/misc.json | 142 ++++++++++++++
> 7 files changed, 807 insertions(+)
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/3] Support fd-based KVM stats
2022-01-15 16:22 ` [PATCH v2 0/3] Support fd-based KVM stats Paolo Bonzini
@ 2022-01-16 23:17 ` Mark Kanda
0 siblings, 0 replies; 16+ messages in thread
From: Mark Kanda @ 2022-01-16 23:17 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: berrange, armbru
[-- Attachment #1: Type: text/plain, Size: 3008 bytes --]
(+ Daniel and Markus)
Thank you Paolo,
I'm in the midst of implementing various API changes as requested by Daniel [1]
and was planning to send out v3 this week. Could you please take a look at his
response and comment on the proposal? Or, perhaps I should publish v3 (based on
Daniel's proposal) and you could review that? Please let me know your preference.
Thanks/regards,
-Mark
[1] https://lore.kernel.org/qemu-devel/Ya+rLex1djU%2F1Wc1@redhat.com/
<https://lore.kernel.org/qemu-devel/Ya+rLex1djU%2F1Wc1@redhat.com/>
On 1/15/2022 10:22 AM, Paolo Bonzini wrote:
> On 11/19/21 20:51, Mark Kanda wrote:
>> v2: [Paolo]
>> - generalize the interface
>> - add support for querying stat schema and instances
>> - add additional HMP semantic processing for a few exponent/unit
>> combinations (related to seconds and bytes)
>>
>> This patchset adds QEMU support for querying fd-based KVM stats. The
>> kernel support was introduced by:
>>
>> cb082bfab59a ("KVM: stats: Add fd-based API to read binary stats data")
>
> Hi Mark,
>
> sorry for the late review. Fortunately there's very little that I'd change.
>
> In particular:
>
> * please change the callbacks to accept a NULL name and type, instead of
> having the "bool"/"const char *" pair. You can probably benefit from a
> function to cutils.c like
>
> bool qemu_match_string(const char *value, const char *request) {
> return !request || g_str_equal(value, request);
> }
>
> * please pass a single const struct to add_stats_callbacks, using GList so
> that the struct can be const.
>
> Putting both together it would be something like:
>
> typedef struct StatsCallbacks {
> char *name;
> StatsList *(*get_values)(StatsList *list, const char *name,
> const char *type, Error **errp);
> StatsSchemaList *(*get_schemas)(StatsSchemaList *list,
> const char *name, Error **errp);
> StatsInstanceList *(*get_instances)(StatsInstanceList *list,
> Error **errp);
> } StatsCallbacks;
>
> Finally, please put everything in a new header include/monitor/stats.h, so
> that we can document everything and please it in docs/devel. I can take care
> of that though.
>
> Thanks,
>
> Paolo
>
>>
>> Mark Kanda (3):
>> qmp: Support for querying stats
>> hmp: Support for querying stats
>> kvm: Support for querying fd-based stats
>>
>> accel/kvm/kvm-all.c | 399 ++++++++++++++++++++++++++++++++++++++
>> hmp-commands-info.hx | 40 ++++
>> include/monitor/hmp.h | 3 +
>> include/monitor/monitor.h | 27 +++
>> monitor/hmp-cmds.c | 125 ++++++++++++
>> monitor/qmp-cmds.c | 71 +++++++
>> qapi/misc.json | 142 ++++++++++++++
>> 7 files changed, 807 insertions(+)
>>
>
[-- Attachment #2: Type: text/html, Size: 9757 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/3] qmp: Support for querying stats
2021-12-07 18:42 ` Daniel P. Berrangé
@ 2022-01-17 12:05 ` Paolo Bonzini
2022-01-17 15:17 ` Mark Kanda
0 siblings, 1 reply; 16+ messages in thread
From: Paolo Bonzini @ 2022-01-17 12:05 UTC (permalink / raw)
To: Daniel P. Berrangé, Mark Kanda; +Cc: qemu-devel, armbru
On 12/7/21 19:42, Daniel P. Berrangé wrote:
> Now onto the values being reported. AFAICT from the kernel
> docs, for all the types of data it currently reports
> (cumulative, instant, peak, none), there is only ever going
> to be a single value. I assume the ability to report multiple
> values is future proofing for a later requirement.
Yes, in fact histogram values have since been added.
> Second, for a given named statistic, AFAICT, the data type,
> unit, base and exponent are all fixed. I don't see a reason
> for us to be reporting that information every time we call
> 'query-stats'. Just report the name + value(s). Apps that
> want a specific named stat won't care about the dimensions,
> because they'll already know what the value means.
I agree on this.
> The 'name' and 'type' are used for filtering I presume. Only allowing
> a single value for each feels pretty inflexible. I'd say we want to
> allow mutliple requests at a time for efficiency.
>
> Bearing in mind my other suggestions above, I'd think we should have
> something more like
>
> { 'enum': 'StatsProvider',
> 'data': ["kvm", "qemu", "tcg", ....],
> }
>
> { 'struct': 'StatsRequest',
> 'data': {
> 'provider': 'StatsProvider',
> // If omitted, report everything for this provider
> '*fields': [ 'str' ]
I think provider should be optional as well. See below.
> }
> }
>
> { 'struct': 'StatsVCPURequest',
> 'base': 'StatsRequest',
> 'data': {
> // To request subset of vCPUs e.g.
> // "cpu_set": "0-3"
> // Could use ['int'] instead if desired
> '*cpu_set': str,
Yes, ['int'] is preferrable.
> },
> }
>
> { 'struct': 'StatsFilter',
> 'data': {
> // If omitted means don't report that group of data
> '*vcpu': 'StatsVCPURequest',
> '*vm': 'StatsRequest',
> },
> }
I agree except that I think this and StatsResults should be unions, even
if it means running multiple query-stats commands. There should also be
an enum ['vcpu', 'vm'] that acts as the discriminator for both
StatsFilter and StatsResults:
{ 'enum': 'StatsTarget',
'data': [ 'vcpu', 'vm' ] }
{ 'union': 'StatsFilter',
'base': { 'target': 'StatsTarget', '*providers': ['StatsProvider'] },
'discriminator': 'target',
'data': { 'vcpu': ['*cpu-set': ['int']] }
}
{ 'union': 'StatsResults',
'base': { 'target': 'StatsTarget', stats: ['StatsResultsEntry'] },
'discriminator': 'target',
'data': { 'vcpu': ['id': 'int'] }
}
Alternatively, the providers should simply be keys in the dictionary
Paolo
>
> { 'alternate': 'StatsValue',
> 'data': { 'scalar': 'int64',
> 'list': [ 'int64 ' ] }
> }
>
> { 'struct': 'StatsResultsEntry',
> 'data': {
> 'provider': 'StatsProvider',
> 'stats': [ 'StatsValue' ]
> }
> }
>
> { 'struct': 'StatsResults':
> 'data': {
> '*vcpu': [ [ 'StatsResultsEntry' ] ],
> '*vm': [ 'StatsResultsEntry' ]
> }
> }
>
> { 'command': 'query-stats',
> 'data': { 'filter': '*StatsFilter' },
> 'returns': 'StatsResults' }
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/3] qmp: Support for querying stats
2022-01-17 12:05 ` Paolo Bonzini
@ 2022-01-17 15:17 ` Mark Kanda
2022-01-18 12:26 ` Paolo Bonzini
0 siblings, 1 reply; 16+ messages in thread
From: Mark Kanda @ 2022-01-17 15:17 UTC (permalink / raw)
To: Paolo Bonzini, Daniel P. Berrangé; +Cc: qemu-devel, armbru
On 1/17/2022 6:05 AM, Paolo Bonzini wrote:
> On 12/7/21 19:42, Daniel P. Berrangé wrote:
>> Now onto the values being reported. AFAICT from the kernel
>> docs, for all the types of data it currently reports
>> (cumulative, instant, peak, none), there is only ever going
>> to be a single value. I assume the ability to report multiple
>> values is future proofing for a later requirement.
>
> Yes, in fact histogram values have since been added.
>
>> Second, for a given named statistic, AFAICT, the data type,
>> unit, base and exponent are all fixed. I don't see a reason
>> for us to be reporting that information every time we call
>> 'query-stats'. Just report the name + value(s). Apps that
>> want a specific named stat won't care about the dimensions,
>> because they'll already know what the value means.
>
> I agree on this.
>
>> The 'name' and 'type' are used for filtering I presume. Only allowing
>> a single value for each feels pretty inflexible. I'd say we want to
>> allow mutliple requests at a time for efficiency.
>>
>> Bearing in mind my other suggestions above, I'd think we should have
>> something more like
>>
>> { 'enum': 'StatsProvider',
>> 'data': ["kvm", "qemu", "tcg", ....],
>> }
>>
>> { 'struct': 'StatsRequest',
>> 'data': {
>> 'provider': 'StatsProvider',
>> // If omitted, report everything for this provider
>> '*fields': [ 'str' ]
>
> I think provider should be optional as well. See below.
>
>> }
>> }
>>
>> { 'struct': 'StatsVCPURequest',
>> 'base': 'StatsRequest',
>> 'data': {
>> // To request subset of vCPUs e.g.
>> // "cpu_set": "0-3"
>> // Could use ['int'] instead if desired
>> '*cpu_set': str,
>
> Yes, ['int'] is preferrable.
>
>> },
>> }
>>
>> { 'struct': 'StatsFilter',
>> 'data': {
>> // If omitted means don't report that group of data
>> '*vcpu': 'StatsVCPURequest',
>> '*vm': 'StatsRequest',
>> },
>> }
>
> I agree except that I think this and StatsResults should be unions, even if it
> means running multiple query-stats commands.
IIUC, making StatsResults a union implies the filter is a required argument
(currently it is optional - omitting it dumps all VM and VCPU stats). Just to
confirm - we want the filter to be required?
Thanks/regards,
-Mark
> There should also be an enum ['vcpu', 'vm'] that acts as the discriminator for
> both StatsFilter and StatsResults:
>
> { 'enum': 'StatsTarget',
> 'data': [ 'vcpu', 'vm' ] }
>
> { 'union': 'StatsFilter',
> 'base': { 'target': 'StatsTarget', '*providers': ['StatsProvider'] },
> 'discriminator': 'target',
> 'data': { 'vcpu': ['*cpu-set': ['int']] }
> }
>
> { 'union': 'StatsResults',
> 'base': { 'target': 'StatsTarget', stats: ['StatsResultsEntry'] },
> 'discriminator': 'target',
> 'data': { 'vcpu': ['id': 'int'] }
> }
>
> Alternatively, the providers should simply be keys in the dictionary
>
> Paolo
>
>>
>> { 'alternate': 'StatsValue',
>> 'data': { 'scalar': 'int64',
>> 'list': [ 'int64 ' ] }
>> }
>>
>> { 'struct': 'StatsResultsEntry',
>> 'data': {
>> 'provider': 'StatsProvider',
>> 'stats': [ 'StatsValue' ]
>> }
>> }
>>
>> { 'struct': 'StatsResults':
>> 'data': {
>> '*vcpu': [ [ 'StatsResultsEntry' ] ],
>> '*vm': [ 'StatsResultsEntry' ]
>> }
>> }
>>
>> { 'command': 'query-stats',
>> 'data': { 'filter': '*StatsFilter' },
>> 'returns': 'StatsResults' }
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/3] qmp: Support for querying stats
2022-01-17 15:17 ` Mark Kanda
@ 2022-01-18 12:26 ` Paolo Bonzini
2022-01-18 12:52 ` Daniel P. Berrangé
0 siblings, 1 reply; 16+ messages in thread
From: Paolo Bonzini @ 2022-01-18 12:26 UTC (permalink / raw)
To: Mark Kanda, Daniel P. Berrangé; +Cc: qemu-devel, armbru
On 1/17/22 16:17, Mark Kanda wrote:
>>
>> I agree except that I think this and StatsResults should be unions,
>> even if it means running multiple query-stats commands.
>
> IIUC, making StatsResults a union implies the filter is a required
> argument (currently it is optional - omitting it dumps all VM and VCPU
> stats). Just to confirm - we want the filter to be required?
Yeah, I think at least the "kind" (vcpu, vm, perhaps in the future block
or net) should be mandatory. If the caller doesn't know of a "kind",
chances are it won't be able to understand what object the stats refer
to, for example the vcpu "id" here:
{ 'union': 'StatsResults',
'base': { 'target': 'StatsTarget', stats: ['StatsResultsEntry'] },
'discriminator': 'target',
'data': { 'vcpu': ['id': 'int'] }
}
(which is another different between Daniel's proposal and mine; his just
placed all vcpus into an array with no explicit id, if I understand
correctly).
Paolo
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/3] qmp: Support for querying stats
2022-01-18 12:26 ` Paolo Bonzini
@ 2022-01-18 12:52 ` Daniel P. Berrangé
2022-01-18 13:59 ` Mark Kanda
2022-01-18 14:47 ` Igor Mammedov
0 siblings, 2 replies; 16+ messages in thread
From: Daniel P. Berrangé @ 2022-01-18 12:52 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, armbru
On Tue, Jan 18, 2022 at 01:26:32PM +0100, Paolo Bonzini wrote:
> On 1/17/22 16:17, Mark Kanda wrote:
> > >
> > > I agree except that I think this and StatsResults should be unions,
> > > even if it means running multiple query-stats commands.
> >
> > IIUC, making StatsResults a union implies the filter is a required
> > argument (currently it is optional - omitting it dumps all VM and VCPU
> > stats). Just to confirm - we want the filter to be required?
>
> Yeah, I think at least the "kind" (vcpu, vm, perhaps in the future block or
> net) should be mandatory. If the caller doesn't know of a "kind", chances
> are it won't be able to understand what object the stats refer to, for
> example the vcpu "id" here:
>
> { 'union': 'StatsResults',
> 'base': { 'target': 'StatsTarget', stats: ['StatsResultsEntry'] },
> 'discriminator': 'target',
> 'data': { 'vcpu': ['id': 'int'] }
> }
>
> (which is another different between Daniel's proposal and mine; his just
> placed all vcpus into an array with no explicit id, if I understand
> correctly).
An explicit ID isn't strictly required, since the caller can assume
the results are ordered on CPU ID, so even if they gave a request
for a sparse subset of CPUs, the results can be interpreted. None
the less having a vCPU id included is more friendly, so worth
having.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/3] qmp: Support for querying stats
2022-01-18 12:52 ` Daniel P. Berrangé
@ 2022-01-18 13:59 ` Mark Kanda
2022-01-19 8:48 ` Paolo Bonzini
2022-01-18 14:47 ` Igor Mammedov
1 sibling, 1 reply; 16+ messages in thread
From: Mark Kanda @ 2022-01-18 13:59 UTC (permalink / raw)
To: Daniel P. Berrangé, Paolo Bonzini; +Cc: qemu-devel, armbru
On 1/18/2022 6:52 AM, Daniel P. Berrangé wrote:
> On Tue, Jan 18, 2022 at 01:26:32PM +0100, Paolo Bonzini wrote:
>> On 1/17/22 16:17, Mark Kanda wrote:
>>>> I agree except that I think this and StatsResults should be unions,
>>>> even if it means running multiple query-stats commands.
>>> IIUC, making StatsResults a union implies the filter is a required
>>> argument (currently it is optional - omitting it dumps all VM and VCPU
>>> stats). Just to confirm - we want the filter to be required?
>> Yeah, I think at least the "kind" (vcpu, vm, perhaps in the future block or
>> net) should be mandatory. If the caller doesn't know of a "kind", chances
>> are it won't be able to understand what object the stats refer to, for
>> example the vcpu "id" here:
>>
>> { 'union': 'StatsResults',
>> 'base': { 'target': 'StatsTarget', stats: ['StatsResultsEntry'] },
>> 'discriminator': 'target',
>> 'data': { 'vcpu': ['id': 'int'] }
>> }
>>
>> (which is another different between Daniel's proposal and mine; his just
>> placed all vcpus into an array with no explicit id, if I understand
>> correctly).
> An explicit ID isn't strictly required, since the caller can assume
> the results are ordered on CPU ID, so even if they gave a request
> for a sparse subset of CPUs, the results can be interpreted. None
> the less having a vCPU id included is more friendly, so worth
> having.
>
>
> Regards,
> Daniel
OK. Thank you Daniel and Paolo. I'll implement these changes for v3.
Best regards,
-Mark
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/3] qmp: Support for querying stats
2022-01-18 12:52 ` Daniel P. Berrangé
2022-01-18 13:59 ` Mark Kanda
@ 2022-01-18 14:47 ` Igor Mammedov
2022-01-19 8:43 ` Paolo Bonzini
1 sibling, 1 reply; 16+ messages in thread
From: Igor Mammedov @ 2022-01-18 14:47 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: Paolo Bonzini, qemu-devel, armbru
On Tue, 18 Jan 2022 12:52:10 +0000
Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Tue, Jan 18, 2022 at 01:26:32PM +0100, Paolo Bonzini wrote:
> > On 1/17/22 16:17, Mark Kanda wrote:
> > > >
> > > > I agree except that I think this and StatsResults should be unions,
> > > > even if it means running multiple query-stats commands.
> > >
> > > IIUC, making StatsResults a union implies the filter is a required
> > > argument (currently it is optional - omitting it dumps all VM and VCPU
> > > stats). Just to confirm - we want the filter to be required?
> >
> > Yeah, I think at least the "kind" (vcpu, vm, perhaps in the future block or
> > net) should be mandatory. If the caller doesn't know of a "kind", chances
> > are it won't be able to understand what object the stats refer to, for
> > example the vcpu "id" here:
> >
> > { 'union': 'StatsResults',
> > 'base': { 'target': 'StatsTarget', stats: ['StatsResultsEntry'] },
> > 'discriminator': 'target',
> > 'data': { 'vcpu': ['id': 'int'] }
> > }
> >
> > (which is another different between Daniel's proposal and mine; his just
> > placed all vcpus into an array with no explicit id, if I understand
> > correctly).
>
> An explicit ID isn't strictly required, since the caller can assume
> the results are ordered on CPU ID, so even if they gave a request
> for a sparse subset of CPUs, the results can be interpreted. None
> the less having a vCPU id included is more friendly, so worth
> having.
and what exactly this CPU ID is,
may QOM path pointing to VCPU instance would be better?
>
> Regards,
> Daniel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/3] qmp: Support for querying stats
2022-01-18 14:47 ` Igor Mammedov
@ 2022-01-19 8:43 ` Paolo Bonzini
2022-01-19 9:11 ` Daniel P. Berrangé
0 siblings, 1 reply; 16+ messages in thread
From: Paolo Bonzini @ 2022-01-19 8:43 UTC (permalink / raw)
To: Igor Mammedov, Daniel P. Berrangé; +Cc: qemu-devel, armbru
On 1/18/22 15:47, Igor Mammedov wrote:
> and what exactly this CPU ID is,
> may QOM path pointing to VCPU instance would be better?
For x86 it would be the APIC ID but yes, having a QOM path is more
future proof. Thanks Igor for noting this.
Paolo
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/3] qmp: Support for querying stats
2022-01-18 13:59 ` Mark Kanda
@ 2022-01-19 8:48 ` Paolo Bonzini
0 siblings, 0 replies; 16+ messages in thread
From: Paolo Bonzini @ 2022-01-19 8:48 UTC (permalink / raw)
To: Mark Kanda, Daniel P. Berrangé; +Cc: qemu-devel, armbru
On 1/18/22 14:59, Mark Kanda wrote:
> OK. Thank you Daniel and Paolo. I'll implement these changes for v3.
To clarify, the command should be
{ 'command': 'query-stats',
'data': 'StatsFilter',
'returns': 'StatsResults' }
so the "target" would be mandatory and there's one level less of nesting
in the arguments.
Paolo
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/3] qmp: Support for querying stats
2022-01-19 8:43 ` Paolo Bonzini
@ 2022-01-19 9:11 ` Daniel P. Berrangé
0 siblings, 0 replies; 16+ messages in thread
From: Daniel P. Berrangé @ 2022-01-19 9:11 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Igor Mammedov, qemu-devel, armbru
On Wed, Jan 19, 2022 at 09:43:23AM +0100, Paolo Bonzini wrote:
> On 1/18/22 15:47, Igor Mammedov wrote:
> > and what exactly this CPU ID is,
> > may QOM path pointing to VCPU instance would be better?
>
> For x86 it would be the APIC ID but yes, having a QOM path is more future
> proof. Thanks Igor for noting this.
Whatever format we use to describe a CPU in the results, should be the
same as the format uses in the input parameters. I had suggested using
a bitmap of CPU IDs, but if we're going to use QOM paths for results,
we must use QOM paths to select CPUs too.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2022-01-19 9:37 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-19 19:51 [PATCH v2 0/3] Support fd-based KVM stats Mark Kanda
2021-11-19 19:51 ` [PATCH v2 1/3] qmp: Support for querying stats Mark Kanda
2021-12-07 18:42 ` Daniel P. Berrangé
2022-01-17 12:05 ` Paolo Bonzini
2022-01-17 15:17 ` Mark Kanda
2022-01-18 12:26 ` Paolo Bonzini
2022-01-18 12:52 ` Daniel P. Berrangé
2022-01-18 13:59 ` Mark Kanda
2022-01-19 8:48 ` Paolo Bonzini
2022-01-18 14:47 ` Igor Mammedov
2022-01-19 8:43 ` Paolo Bonzini
2022-01-19 9:11 ` Daniel P. Berrangé
2021-11-19 19:51 ` [PATCH v2 2/3] hmp: " Mark Kanda
2021-11-19 19:51 ` [PATCH v2 3/3] kvm: Support for querying fd-based stats Mark Kanda
2022-01-15 16:22 ` [PATCH v2 0/3] Support fd-based KVM stats Paolo Bonzini
2022-01-16 23:17 ` Mark Kanda
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.