* [PATCH v3 1/3] qmp: Support for querying stats
2022-01-31 19:43 [PATCH v3 0/3] Support fd-based KVM stats Mark Kanda
@ 2022-01-31 19:43 ` Mark Kanda
2022-02-01 10:51 ` Paolo Bonzini
2022-02-01 12:08 ` Daniel P. Berrangé
2022-01-31 19:43 ` [PATCH v3 2/3] hmp: " Mark Kanda
2022-01-31 19:43 ` [PATCH v3 3/3] kvm: Support for querying fd-based stats Mark Kanda
2 siblings, 2 replies; 17+ messages in thread
From: Mark Kanda @ 2022-01-31 19:43 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, berrange, armbru
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 per target type (only VM and VCPU for now), with
additional options for specifying stat names, VCPU qom paths, and stat provider.
- query-stats-schemas
Returns a list of stats included in each schema type, with an option for
specifying the stat provider.
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):
- Display all VM stats:
{ "execute": "query-stats", "arguments" : { "target": "vm" } }
{ "return": {
"list": [
{ "provider": "kvm",
"stats": [
{ "name": "max_mmu_page_hash_collisions", "value": 0 },
{ "name": "max_mmu_rmap_size", "value": 0 },
{ "name": "nx_lpage_splits", "value": 131 },
...
] }
{ "provider": "provider XYZ",
...
],
"target": "vm"
}
}
- Display all VCPU stats:
{ "execute": "query-stats", "arguments" : { "target": "vcpu" } }
{ "return": {
"list": [
{ "list": [
{ "provider": "kvm",
"stats": [
{ "name": "guest_mode", "value": 0 },
{ "name": "directed_yield_successful", "value": 0 },
{ "name": "directed_yield_attempted", "value": 76 },
...
] }
{ "provider": "provider XYZ",
...
],
"path": "/machine/unattached/device[0]"
},
{ "list": [
{ "provider": "kvm",
"stats": [
{ "name": "guest_mode", "value": 0 },
{ "name": "directed_yield_successful", "value": 0 },
{ "name": "directed_yield_attempted", "value": 51 },
...
}
],
"target": "vcpu"
}
}
- Display 'exits' and 'l1d_flush' KVM stats for VCPUs at '/machine/unattached/device[2]'
and '/machine/unattached/device[4]':
{ "execute": "query-stats",
"arguments" : { "target": "vcpu",
"fields": [ "exits", "l1d_flush" ],
"paths": [ "/machine/unattached/device[2]",
"/machine/unattached/device[4]" ]
"provider": "kvm" } }
{ "return": {
"list": [
{ "list": [
{ "provider": "kvm",
"stats": [
{ "name": "l1d_flush", "value": 14690 },
{ "name": "exits", "value": 50898 }
] }
],
"path": "/machine/unattached/device[2]"
},
{ "list": [
{ "provider": "kvm",
"stats": [
{ "name": "l1d_flush", "value": 24902 },
{ "name": "exits", "value": 74374 }
] }
],
"path": "/machine/unattached/device[4]"
}
],
"target": "vcpu"
}
}
- Query stats schemas:
{ "execute": "query-stats-schemas" }
{ "return": {
"vcpu": [
{ "provider": "kvm",
"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": "provider XYZ",
...
"vm": [
{ "provider": "kvm",
"stats": [
{ "name": "max_mmu_page_hash_collisions",
"unit": "none",
"base": 10,
"exponent": 0,
"type": "peak" },
"provider": "provider XYZ",
...
Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
---
include/monitor/stats.h | 36 ++++++
monitor/qmp-cmds.c | 183 +++++++++++++++++++++++++++++
qapi/misc.json | 253 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 472 insertions(+)
create mode 100644 include/monitor/stats.h
diff --git a/include/monitor/stats.h b/include/monitor/stats.h
new file mode 100644
index 0000000000..d4b57322eb
--- /dev/null
+++ b/include/monitor/stats.h
@@ -0,0 +1,36 @@
+/*
+ * 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
+
+/*
+ * Add qmp stats callbacks to the stats_callbacks list.
+ *
+ * @provider: stats provider
+ *
+ * @stats_fn: routine to query stats:
+ * void (*stats_fn)(StatsResults *results, StatsFilter *filter, Error **errp)
+ *
+ * @schema_fn: routine to query stat schemas:
+ * void (*schemas_fn)(StatsSchemaResult *results, Error **errp)
+ */
+void add_stats_callbacks(StatsProvider provider,
+ void (*stats_fn)(StatsResults *, StatsFilter *,
+ Error **),
+ void (*schemas_fn)(StatsSchemaResult *, Error **));
+
+/* Stats helpers routines */
+StatsResultsEntry *add_vm_stats_entry(StatsResults *, StatsProvider);
+StatsResultsEntry *add_vcpu_stats_entry(StatsResults *, StatsProvider, char *);
+StatsSchemaProvider *add_vm_stats_schema(StatsSchemaResult *, StatsProvider);
+StatsSchemaProvider *add_vcpu_stats_schema(StatsSchemaResult *, StatsProvider);
+
+bool stat_name_filter(StatsFilter *, StatsTarget, char *);
+bool stat_cpu_filter(StatsFilter *, char *);
+
+#endif /* STATS_H */
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index db4d186448..bd562edfb8 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -18,6 +18,7 @@
#include "qemu/cutils.h"
#include "qemu/option.h"
#include "monitor/monitor.h"
+#include "monitor/stats.h"
#include "sysemu/sysemu.h"
#include "qemu/config-file.h"
#include "qemu/uuid.h"
@@ -448,3 +449,185 @@ HumanReadableText *qmp_x_query_irq(Error **errp)
return human_readable_text_from_str(buf);
}
+
+typedef struct StatsCallbacks {
+ StatsProvider provider;
+ void (*stats_cb)(StatsResults *, StatsFilter *, Error **);
+ void (*schemas_cb)(StatsSchemaResult *, Error **);
+ QTAILQ_ENTRY(StatsCallbacks) next;
+} StatsCallbacks;
+
+static QTAILQ_HEAD(, StatsCallbacks) stats_callbacks =
+ QTAILQ_HEAD_INITIALIZER(stats_callbacks);
+
+void add_stats_callbacks(StatsProvider provider,
+ void (*stats_fn)(StatsResults *, StatsFilter*,
+ Error **),
+ void (*schemas_fn)(StatsSchemaResult *, Error **))
+{
+ StatsCallbacks *entry = g_malloc0(sizeof(*entry));
+ entry->provider = provider;
+ entry->stats_cb = stats_fn;
+ entry->schemas_cb = schemas_fn;
+
+ QTAILQ_INSERT_TAIL(&stats_callbacks, entry, next);
+}
+
+StatsResults *qmp_query_stats(StatsFilter *filter, Error **errp)
+{
+ StatsResults *stats_results = g_malloc0(sizeof(*stats_results));
+ StatsCallbacks *entry;
+ StatsProvider provider = STATS_PROVIDER__MAX;
+
+ if (filter->target == STATS_TARGET_VM &&
+ filter->u.vm.has_provider) {
+ provider = filter->u.vm.provider;
+ } else if (filter->target == STATS_TARGET_VCPU &&
+ filter->u.vcpu.has_provider) {
+ provider = filter->u.vcpu.provider;
+ }
+
+ QTAILQ_FOREACH(entry, &stats_callbacks, next) {
+ if (provider != STATS_PROVIDER__MAX && (provider != entry->provider)) {
+ continue;
+ }
+ entry->stats_cb(stats_results, filter, errp);
+ }
+
+ return stats_results;
+}
+
+StatsSchemaResult *qmp_query_stats_schemas(bool has_provider,
+ StatsProvider provider,
+ Error **errp)
+{
+ StatsSchemaResult *stats_results = g_malloc0(sizeof(*stats_results));
+ StatsCallbacks *entry;
+
+ QTAILQ_FOREACH(entry, &stats_callbacks, next) {
+ if (has_provider && (provider != entry->provider)) {
+ continue;
+ }
+ entry->schemas_cb(stats_results, errp);
+ }
+
+ return stats_results;
+}
+
+StatsResultsEntry *add_vm_stats_entry(StatsResults *stats_results,
+ StatsProvider provider)
+{
+ StatsResultsEntry *entry = g_malloc0(sizeof(*entry));
+ stats_results->target = STATS_TARGET_VM;
+ entry->provider = provider;
+ QAPI_LIST_PREPEND(stats_results->u.vm.list, entry);
+
+ return entry;
+}
+
+StatsResultsEntry *add_vcpu_stats_entry(StatsResults *stats_results,
+ StatsProvider provider,
+ char *path)
+{
+ StatsResultsEntry *entry = g_malloc0(sizeof(*entry));
+ stats_results->target = STATS_TARGET_VCPU;
+ entry->provider = provider;
+
+ /*
+ * Find the corresponding vCPU entry and add to its list. Else, create it.
+ */
+ VCPUResultsEntryList **tailp = &stats_results->u.vcpu.list;
+ VCPUResultsEntryList *tail;
+
+ for (tail = *tailp; tail; tail = tail->next) {
+ if (g_str_equal(tail->value->path, path)) {
+ /* Add to the existing vCPU list */
+ QAPI_LIST_PREPEND(tail->value->list, entry);
+ goto done;
+ }
+ tailp = &tail->next;
+ }
+ /* Create and populate a new entry for the vCPU */
+ VCPUResultsEntry *value = g_malloc0(sizeof(*value));
+ value->path = g_strdup(path);
+ value->list = g_malloc0(sizeof(*value->list));
+ value->list->value = entry;
+ QAPI_LIST_APPEND(tailp, value);
+
+done:
+ return entry;
+}
+
+StatsSchemaProvider *add_vm_stats_schema(StatsSchemaResult *schema_results,
+ StatsProvider provider)
+{
+ StatsSchemaProvider *entry = g_malloc0(sizeof(*entry));
+ schema_results->has_vm = true;
+ entry->provider = provider;
+ QAPI_LIST_PREPEND(schema_results->vm, entry);
+
+ return entry;
+}
+
+StatsSchemaProvider *add_vcpu_stats_schema(StatsSchemaResult *schema_results,
+ StatsProvider provider)
+{
+ StatsSchemaProvider *entry = g_malloc0(sizeof(*entry));
+ schema_results->has_vcpu = true;
+ entry->provider = provider;
+ QAPI_LIST_PREPEND(schema_results->vcpu, entry);
+
+ return entry;
+}
+
+/* True if stat doesn't match a requested name */
+bool stat_name_filter(StatsFilter *filter, StatsTarget type, char *name)
+{
+ strList *str_list = NULL;
+
+ if (type == STATS_TARGET_VM) {
+ if (filter->target != STATS_TARGET_VM) {
+ return false;
+ }
+ if (!filter->u.vm.has_fields) {
+ return false;
+ }
+ str_list = filter->u.vm.fields;
+ } else if (type == STATS_TARGET_VCPU) {
+ if (filter->target != STATS_TARGET_VCPU) {
+ return false;
+ }
+ if (!filter->u.vcpu.has_fields) {
+ return false;
+ }
+ str_list = filter->u.vcpu.fields;
+ }
+
+ for (; str_list; str_list = str_list->next) {
+ if (g_str_equal(name, str_list->value)) {
+ return false;
+ }
+ }
+ return true;
+}
+
+/* True if cpu qom path doesn't match a requested path */
+bool stat_cpu_filter(StatsFilter *filter, char *path)
+{
+ strList *list;
+
+ if (filter->target != STATS_TARGET_VCPU) {
+ return false;
+ }
+
+ if (!filter->u.vcpu.has_paths) {
+ return false;
+ }
+
+ for (list = filter->u.vcpu.paths; list; list = list->next) {
+ if (g_str_equal(list->value, path)) {
+ return false;
+ }
+ }
+ return true;
+}
diff --git a/qapi/misc.json b/qapi/misc.json
index e8054f415b..8d326464f0 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -527,3 +527,256 @@
'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',
+ 'linear-hist', 'log-hist', 'unknown' ] }
+
+##
+# @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' : [ 'none', 'bytes', 'seconds', 'cycles', 'unknown' ] }
+
+##
+# @StatBase:
+#
+# Enumeration of stat base
+#
+# Since: 7.0
+##
+{ 'enum' : 'StatBase',
+ 'data' : [ 'pow10', 'pow2', 'unknown' ] }
+
+##
+# @StatsProvider:
+#
+# Enumeration of stat providers.
+#
+# Since: 7.0
+##
+{ 'enum': 'StatsProvider',
+ 'data': [ ] }
+
+##
+# @StatsTarget:
+#
+# Enumeration of stat targets.
+# @vm: stat is per vm.
+# @vcpu: stat is per vcpu.
+#
+# Since: 7.0
+##
+{ 'enum': 'StatsTarget',
+ 'data': [ 'vm', 'vcpu' ] }
+
+##
+# @StatsVCPURequest:
+#
+# vcpu specific filter element.
+# @paths: list of qom paths.
+#
+# Since: 7.0
+##
+{ 'struct': 'StatsVCPURequest',
+ 'base': 'StatsRequest',
+ 'data': { '*paths': [ 'str' ] } }
+
+##
+# @StatsRequest:
+#
+# Stats filter element.
+# @provider: stat provider.
+# @fields: list of stat names.
+#
+# Since: 7.0
+##
+{ 'struct': 'StatsRequest',
+ 'data': { '*provider': 'StatsProvider',
+ '*fields': [ 'str' ] } }
+
+##
+# @StatsFilter:
+#
+# Target specific filter.
+#
+# Since: 7.0
+##
+{ 'union': 'StatsFilter',
+ 'base': { 'target': 'StatsTarget' },
+ 'discriminator': 'target',
+ 'data': { 'vcpu': 'StatsVCPURequest',
+ 'vm': 'StatsRequest' } }
+
+##
+# @StatsValueArray:
+#
+# uint64 list for StatsValue.
+#
+# Since: 7.0
+##
+{ 'struct': 'StatsValueArray',
+ 'data': { 'list' : [ 'uint64' ] } }
+
+##
+# @StatsValue:
+#
+# @scalar: stat is single uint64.
+# @list: stat is a list of uint64.
+#
+# Since: 7.0
+##
+{ 'alternate': 'StatsValue',
+ 'data': { 'scalar': 'uint64',
+ 'list': 'StatsValueArray' } }
+
+##
+# @Stats:
+#
+# @name: name of stat.
+# @value: stat value.
+#
+# Since: 7.0
+##
+{ 'struct': 'Stats',
+ 'data': { 'name': 'str',
+ 'value' : 'StatsValue' } }
+
+##
+# @StatsResultsEntry:
+#
+# @provider: stat provider.
+# @stats: list of stats.
+#
+# Since: 7.0
+##
+{ 'struct': 'StatsResultsEntry',
+ 'data': { 'provider': 'StatsProvider',
+ 'stats': [ 'Stats' ] } }
+
+##
+# @VCPUResultsEntry:
+#
+# @path: qom path.
+# @list: per provider @StatsResultEntry list.
+#
+# Since: 7.0
+##
+{ 'struct': 'VCPUResultsEntry',
+ 'data': { 'path': 'str',
+ 'list': [ 'StatsResultsEntry' ] } }
+
+##
+# @VMStatsResults:
+#
+# Since: 7.0
+##
+{ 'struct': 'VMStatsResults',
+ 'data': { 'list' : [ 'StatsResultsEntry' ] } }
+
+##
+# @VCPUStatsResults:
+#
+# Since: 7.0
+##
+{ 'struct': 'VCPUStatsResults',
+ 'data': { 'list': [ 'VCPUResultsEntry' ] } }
+
+##
+# @StatsResults:
+#
+# Target specific results.
+#
+# Since: 7.0
+##
+{ 'union': 'StatsResults',
+ 'base': { 'target': 'StatsTarget' },
+ 'discriminator': 'target',
+ 'data': { 'vcpu': 'VCPUStatsResults',
+ 'vm': 'VMStatsResults' } }
+
+##
+# @query-stats:
+#
+# data: @StatsFilter
+# Returns: @StatsResults
+#
+# Since: 7.0
+##
+{ 'command': 'query-stats',
+ 'data': 'StatsFilter',
+ 'boxed': true,
+ 'returns': 'StatsResults' }
+
+##
+# @StatsSchemaValue:
+#
+# Individual stat schema.
+# @name: stat name.
+# @type: @StatType
+# @unit: @StatUnit
+# @base: @StatBase
+# @exponent: Used together with @base.
+#
+# Since: 7.0
+##
+{ 'struct': 'StatsSchemaValue',
+ 'data': { 'name': 'str',
+ 'type': 'StatType',
+ 'unit': 'StatUnit',
+ 'base': 'StatBase',
+ 'exponent': 'int16' } }
+
+##
+# @StatsSchemaProvider:
+#
+# @provider: @StatsProvider.
+# @stats: list of stats.
+#
+# Since: 7.0
+##
+{ 'struct': 'StatsSchemaProvider',
+ 'data': { 'provider': 'StatsProvider',
+ 'stats': [ 'StatsSchemaValue' ] } }
+
+##
+# @StatsSchemaResult:
+#
+# @vm: vm stats schemas.
+# @vcpu: vcpu stats schemas.
+#
+# Since: 7.0
+##
+{ 'struct': 'StatsSchemaResult',
+ 'data': { '*vm': [ 'StatsSchemaProvider' ],
+ '*vcpu': [ 'StatsSchemaProvider' ] } }
+
+##
+# @query-stats-schemas:
+#
+# Query Stats schemas.
+# Returns @StatsSchemaResult
+#
+# Since: 7.0
+##
+{ 'command': 'query-stats-schemas',
+ 'data': { '*provider': 'StatsProvider' },
+ 'returns': 'StatsSchemaResult' }
--
2.27.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v3 1/3] qmp: Support for querying stats
2022-01-31 19:43 ` [PATCH v3 1/3] qmp: Support for querying stats Mark Kanda
@ 2022-02-01 10:51 ` Paolo Bonzini
2022-02-01 11:01 ` Daniel P. Berrangé
2022-02-01 12:08 ` Daniel P. Berrangé
1 sibling, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2022-02-01 10:51 UTC (permalink / raw)
To: Mark Kanda, qemu-devel; +Cc: berrange, armbru
On 1/31/22 20:43, Mark Kanda wrote:
>
> { "execute": "query-stats", "arguments" : { "target": "vm" } }
> { "return": {
> "list": [
> { "provider": "kvm",
> "stats": [
> { "name": "max_mmu_page_hash_collisions", "value": 0 },
> { "name": "max_mmu_rmap_size", "value": 0 },
> { "name": "nx_lpage_splits", "value": 131 },
> ...
> ] }
> { "provider": "provider XYZ",
> ...
> ],
> "target": "vm"
> }
> }
Perhaps it's better to have a better name than "list" for clarity, like
you already did with 'stats':
{ 'struct': 'VCPUResultsEntry',
'data': { 'path': 'str',
'providers': [ 'StatsResultsEntry' ] } }
{ 'struct': 'VCPUStatsResults',
'data': { 'objects': [ 'VCPUResultsEntry' ] } }
{ 'struct': 'VMStatsResults',
'data': { 'providers' : [ 'StatsResultsEntry' ] } }
Also, here:
> +{ 'alternate': 'StatsValue',
> + 'data': { 'scalar': 'uint64',
> + 'list': 'StatsValueArray' } }
is it possible to just do
{ 'alternate': 'StatsValue',
'data': { 'scalar': 'uint64',
'list': ['uint64'] } }
Thanks,
Paolo
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v3 1/3] qmp: Support for querying stats
2022-02-01 10:51 ` Paolo Bonzini
@ 2022-02-01 11:01 ` Daniel P. Berrangé
2022-02-11 13:51 ` Markus Armbruster
0 siblings, 1 reply; 17+ messages in thread
From: Daniel P. Berrangé @ 2022-02-01 11:01 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, armbru
On Tue, Feb 01, 2022 at 11:51:26AM +0100, Paolo Bonzini wrote:
> On 1/31/22 20:43, Mark Kanda wrote:
> >
> > { "execute": "query-stats", "arguments" : { "target": "vm" } }
> > { "return": {
> > "list": [
> > { "provider": "kvm",
> > "stats": [
> > { "name": "max_mmu_page_hash_collisions", "value": 0 },
> > { "name": "max_mmu_rmap_size", "value": 0 },
> > { "name": "nx_lpage_splits", "value": 131 },
> > ...
> > ] }
> > { "provider": "provider XYZ",
> > ...
> > ],
> > "target": "vm"
> > }
> > }
>
> Perhaps it's better to have a better name than "list" for clarity, like you
> already did with 'stats':
>
> { 'struct': 'VCPUResultsEntry',
> 'data': { 'path': 'str',
> 'providers': [ 'StatsResultsEntry' ] } }
>
> { 'struct': 'VCPUStatsResults',
> 'data': { 'objects': [ 'VCPUResultsEntry' ] } }
>
>
> { 'struct': 'VMStatsResults',
> 'data': { 'providers' : [ 'StatsResultsEntry' ] } }
>
> Also, here:
>
> > +{ 'alternate': 'StatsValue',
> > + 'data': { 'scalar': 'uint64',
> > + 'list': 'StatsValueArray' } }
>
> is it possible to just do
>
> { 'alternate': 'StatsValue',
> 'data': { 'scalar': 'uint64',
> 'list': ['uint64'] } }
No, the QAPI generator throws its toys out of the pram.
It claims you can have any set of data types which have a
distinct representation on the wire, so this is valid from
that POV. Something about the parser/code generator can't
cope with this inline array though - it wants a named type
which means a built-in scalar, or a compound type, but not
an array :-(
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] 17+ messages in thread* Re: [PATCH v3 1/3] qmp: Support for querying stats
2022-02-01 11:01 ` Daniel P. Berrangé
@ 2022-02-11 13:51 ` Markus Armbruster
0 siblings, 0 replies; 17+ messages in thread
From: Markus Armbruster @ 2022-02-11 13:51 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: Paolo Bonzini, qemu-devel, armbru
Daniel P. Berrangé <berrange@redhat.com> writes:
> On Tue, Feb 01, 2022 at 11:51:26AM +0100, Paolo Bonzini wrote:
>> On 1/31/22 20:43, Mark Kanda wrote:
>> >
>> > { "execute": "query-stats", "arguments" : { "target": "vm" } }
>> > { "return": {
>> > "list": [
>> > { "provider": "kvm",
>> > "stats": [
>> > { "name": "max_mmu_page_hash_collisions", "value": 0 },
>> > { "name": "max_mmu_rmap_size", "value": 0 },
>> > { "name": "nx_lpage_splits", "value": 131 },
>> > ...
>> > ] }
>> > { "provider": "provider XYZ",
>> > ...
>> > ],
>> > "target": "vm"
>> > }
>> > }
>>
>> Perhaps it's better to have a better name than "list" for clarity, like you
>> already did with 'stats':
>>
>> { 'struct': 'VCPUResultsEntry',
>> 'data': { 'path': 'str',
>> 'providers': [ 'StatsResultsEntry' ] } }
>>
>> { 'struct': 'VCPUStatsResults',
>> 'data': { 'objects': [ 'VCPUResultsEntry' ] } }
>>
>>
>> { 'struct': 'VMStatsResults',
>> 'data': { 'providers' : [ 'StatsResultsEntry' ] } }
>>
>> Also, here:
>>
>> > +{ 'alternate': 'StatsValue',
>> > + 'data': { 'scalar': 'uint64',
>> > + 'list': 'StatsValueArray' } }
>>
>> is it possible to just do
>>
>> { 'alternate': 'StatsValue',
>> 'data': { 'scalar': 'uint64',
>> 'list': ['uint64'] } }
>
> No, the QAPI generator throws its toys out of the pram.
>
> It claims you can have any set of data types which have a
> distinct representation on the wire, so this is valid from
> that POV. Something about the parser/code generator can't
> cope with this inline array though - it wants a named type
> which means a built-in scalar, or a compound type, but not
> an array :-(
Array is not implemented, simply because we haven't had a use for it.
Should not make you settle for an inferior schema design! Implementing
array alternates shouldn't be hard.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/3] qmp: Support for querying stats
2022-01-31 19:43 ` [PATCH v3 1/3] qmp: Support for querying stats Mark Kanda
2022-02-01 10:51 ` Paolo Bonzini
@ 2022-02-01 12:08 ` Daniel P. Berrangé
2022-02-03 18:12 ` Mark Kanda
` (2 more replies)
1 sibling, 3 replies; 17+ messages in thread
From: Daniel P. Berrangé @ 2022-02-01 12:08 UTC (permalink / raw)
To: Mark Kanda; +Cc: pbonzini, qemu-devel, armbru
On Mon, Jan 31, 2022 at 01:43:10PM -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 per target type (only VM and VCPU for now), with
> additional options for specifying stat names, VCPU qom paths, and stat provider.
>
> - query-stats-schemas
> Returns a list of stats included in each schema type, with an option for
> specifying the stat provider.
>
> 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):
>
> - Display all VM stats:
>
> { "execute": "query-stats", "arguments" : { "target": "vm" } }
> { "return": {
> "list": [
> { "provider": "kvm",
> "stats": [
> { "name": "max_mmu_page_hash_collisions", "value": 0 },
> { "name": "max_mmu_rmap_size", "value": 0 },
> { "name": "nx_lpage_splits", "value": 131 },
> ...
> ] }
> { "provider": "provider XYZ",
> ...
> ],
> "target": "vm"
> }
> }
I still feel like this is rather verbose, and should be simplified
down to.
{ "return": {
"vm": {
"kvm": [ ... ]
"provider-XYZ": [ ... ],
...
}
}
While vCPU would need one extra nesting layer
{ "return": {
"vcpus": [
{
"path": "/vcpu0/path"
"kvm": [ ... ]
"provider-XYZ": [ ... ],
...
},
{
"path": "/vcpu1/path"
"kvm": [ ... ]
"provider-XYZ": [ ... ],
...
},
...
],
}
The notable difference here is that we'd be adding new
keys to the StatsResultEntry struct, every time we gain
a new provider, so your QMP code couldn't be entirely
metadata driven - you'd need new code to wire up each
stats provider.
> - Display 'exits' and 'l1d_flush' KVM stats for VCPUs at '/machine/unattached/device[2]'
> and '/machine/unattached/device[4]':
Shows the value of giving CPUs proper paths
https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg06744.html
>
> { "execute": "query-stats",
> "arguments" : { "target": "vcpu",
> "fields": [ "exits", "l1d_flush" ],
> "paths": [ "/machine/unattached/device[2]",
> "/machine/unattached/device[4]" ]
> "provider": "kvm" } }
This design requires multiple query-stats calls to get data from
multiple providers which I think is very undesirable from a
performance POV.
I'd like to see us able to query fields from many providers at
once
ie so we have something that looks like
{ "execute": "query-stats",
"arguments" : { "target": "vcpu",
"vcpus": [ "/machine/unattached/device[2]",
"/machine/unattached/device[4]" ]
"kvm": [ "exits", "l1d_flush" ],
"someprovider: [ "somefield" ] } }
>
> { "return": {
> "list": [
> { "list": [
> { "provider": "kvm",
> "stats": [
> { "name": "l1d_flush", "value": 14690 },
> { "name": "exits", "value": 50898 }
> ] }
> ],
> "path": "/machine/unattached/device[2]"
> },
> { "list": [
> { "provider": "kvm",
> "stats": [
> { "name": "l1d_flush", "value": 24902 },
> { "name": "exits", "value": 74374 }
> ] }
> ],
> "path": "/machine/unattached/device[4]"
> }
> ],
> "target": "vcpu"
> }
> }
>
> - Query stats schemas:
>
> { "execute": "query-stats-schemas" }
> { "return": {
> "vcpu": [
> { "provider": "kvm",
> "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": "provider XYZ",
> ...
> "vm": [
> { "provider": "kvm",
> "stats": [
> { "name": "max_mmu_page_hash_collisions",
> "unit": "none",
> "base": 10,
> "exponent": 0,
> "type": "peak" },
> "provider": "provider XYZ",
> ...
>
> Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
> ---
> include/monitor/stats.h | 36 ++++++
> monitor/qmp-cmds.c | 183 +++++++++++++++++++++++++++++
> qapi/misc.json | 253 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 472 insertions(+)
> create mode 100644 include/monitor/stats.h
>
> diff --git a/qapi/misc.json b/qapi/misc.json
> index e8054f415b..8d326464f0 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
I'd suggest we introduce a 'stats.json' file just for this. We have
quite a few data types introduced, and its good to avoid 'misc.json'
becoming a dumping ground for ranom unrelated stuff.
> @@ -527,3 +527,256 @@
> 'data': { '*option': 'str' },
> 'returns': ['CommandLineOptionInfo'],
> 'allow-preconfig': true }
> +
> +##
> +# @StatType:
There's inconsistency with naming through these changes. Can we
ensure that everything uses 'Stats' (plural) as the prefix for
every type.
> +#
> +# 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.
Not documenting all members
> +#
> +# Since: 7.0
> +##
> +{ 'enum' : 'StatType',
> + 'data' : [ 'cumulative', 'instant', 'peak',
> + 'linear-hist', 'log-hist', 'unknown' ] }
IMHO 'unknown' shouldn't exist at all.
> +##
> +# @StatsVCPURequest:
> +#
> +# vcpu specific filter element.
> +# @paths: list of qom paths.
> +#
> +# Since: 7.0
> +##
> +{ 'struct': 'StatsVCPURequest',
> + 'base': 'StatsRequest',
> + 'data': { '*paths': [ 'str' ] } }
Call the field 'vcpus' instead of 'paths' to make it
clear that we're listing VCPU paths here.
> +##
> +# @StatsRequest:
> +#
> +# Stats filter element.
> +# @provider: stat provider.
> +# @fields: list of stat names.
> +#
> +# Since: 7.0
> +##
> +{ 'struct': 'StatsRequest',
> + 'data': { '*provider': 'StatsProvider',
> + '*fields': [ 'str' ] } }
As mentioned earlier I think we need to have aility to query from
many providers at once. It'd be better to have provider name as
the field name, eg
{ 'struct': 'StatsRequest',
'data': { '*kvm': ['str'],
'*someprovider': [ 'str' ] } }
> +
> +##
> +# @StatsFilter:
> +#
> +# Target specific filter.
> +#
> +# Since: 7.0
> +##
> +{ 'union': 'StatsFilter',
> + 'base': { 'target': 'StatsTarget' },
> + 'discriminator': 'target',
> + 'data': { 'vcpu': 'StatsVCPURequest',
> + 'vm': 'StatsRequest' } }
> +##
> +# @StatsValueArray:
> +#
> +# uint64 list for StatsValue.
> +#
> +# Since: 7.0
> +##
> +{ 'struct': 'StatsValueArray',
> + 'data': { 'list' : [ 'uint64' ] } }
'values' or 'elements' rather than repeating 'list'
> +
> +##
> +# @StatsValue:
> +#
> +# @scalar: stat is single uint64.
> +# @list: stat is a list of uint64.
> +#
> +# Since: 7.0
> +##
> +{ 'alternate': 'StatsValue',
> + 'data': { 'scalar': 'uint64',
> + 'list': 'StatsValueArray' } }
> +
> +##
> +# @Stats:
> +#
> +# @name: name of stat.
> +# @value: stat value.
> +#
> +# Since: 7.0
> +##
> +{ 'struct': 'Stats',
> + 'data': { 'name': 'str',
> + 'value' : 'StatsValue' } }
> +
> +##
> +# @StatsResultsEntry:
> +#
> +# @provider: stat provider.
> +# @stats: list of stats.
> +#
> +# Since: 7.0
> +##
> +{ 'struct': 'StatsResultsEntry',
> + 'data': { 'provider': 'StatsProvider',
> + 'stats': [ 'Stats' ] } }
> +
> +##
> +# @VCPUResultsEntry:
> +#
> +# @path: qom path.
> +# @list: per provider @StatsResultEntry list.
> +#
> +# Since: 7.0
> +##
> +{ 'struct': 'VCPUResultsEntry',
> + 'data': { 'path': 'str',
> + 'list': [ 'StatsResultsEntry' ] } }
> +
> +##
> +# @VMStatsResults:
> +#
> +# Since: 7.0
> +##
> +{ 'struct': 'VMStatsResults',
> + 'data': { 'list' : [ 'StatsResultsEntry' ] } }
> +
> +##
> +# @VCPUStatsResults:
> +#
> +# Since: 7.0
> +##
> +{ 'struct': 'VCPUStatsResults',
> + 'data': { 'list': [ 'VCPUResultsEntry' ] } }
> +
> +##
> +# @StatsResults:
> +#
> +# Target specific results.
> +#
> +# Since: 7.0
> +##
> +{ 'union': 'StatsResults',
> + 'base': { 'target': 'StatsTarget' },
> + 'discriminator': 'target',
> + 'data': { 'vcpu': 'VCPUStatsResults',
> + 'vm': 'VMStatsResults' } }
I feel we can simplify this all down somewhat, eliminating levels
of redundant nesting
{ 'struct': 'StatsResultsEntry',
'data': { '*kvm': [ 'Stats' ] } }
{ 'struct': 'StatsResultsVCPUEntry',
'base': 'StatsResultsEntry',
'data': 'path': 'str' } }
{ 'struct': 'StatsResults',
'data': {
'*vcpus': ['StatsResultsVCPUEntry'],
'*vm': 'StatsResultsEntry'
}
}
> +
> +##
> +# @query-stats:
> +#
> +# data: @StatsFilter
> +# Returns: @StatsResults
> +#
> +# Since: 7.0
> +##
> +{ 'command': 'query-stats',
> + 'data': 'StatsFilter',
> + 'boxed': true,
> + 'returns': 'StatsResults' }
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] 17+ messages in thread* Re: [PATCH v3 1/3] qmp: Support for querying stats
2022-02-01 12:08 ` Daniel P. Berrangé
@ 2022-02-03 18:12 ` Mark Kanda
2022-02-03 18:30 ` Daniel P. Berrangé
2022-02-03 18:39 ` Paolo Bonzini
2022-02-03 18:38 ` Paolo Bonzini
2022-02-03 18:52 ` Mark Kanda
2 siblings, 2 replies; 17+ messages in thread
From: Mark Kanda @ 2022-02-03 18:12 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: pbonzini, qemu-devel, armbru
[-- Attachment #1: Type: text/plain, Size: 661 bytes --]
Thanks Daniel,
On 2/1/2022 6:08 AM, Daniel P. Berrangé wrote:
>> +#
>> +# Since: 7.0
>> +##
>> +{ 'enum' : 'StatType',
>> + 'data' : [ 'cumulative', 'instant', 'peak',
>> + 'linear-hist', 'log-hist', 'unknown' ] }
> IMHO 'unknown' shouldn't exist at all.
>
I added the 'unknown' member here (and in other enums) to handle situations
where QEMU is behind KVM in terms of enumerating the various stat types, units,
etc. I feel this will be a semi-common scenario (old QEMU on a new kernel) and
with 'unknown', QEMU can at least display the raw data.
That said, I happy skip 'unknown' entries if you think that's better.
Thanks/regards,
-Mark
[-- Attachment #2: Type: text/html, Size: 1169 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v3 1/3] qmp: Support for querying stats
2022-02-03 18:12 ` Mark Kanda
@ 2022-02-03 18:30 ` Daniel P. Berrangé
2022-02-03 18:37 ` Mark Kanda
2022-02-03 18:39 ` Paolo Bonzini
1 sibling, 1 reply; 17+ messages in thread
From: Daniel P. Berrangé @ 2022-02-03 18:30 UTC (permalink / raw)
To: Mark Kanda; +Cc: pbonzini, qemu-devel, armbru
On Thu, Feb 03, 2022 at 12:12:57PM -0600, Mark Kanda wrote:
> Thanks Daniel,
>
> On 2/1/2022 6:08 AM, Daniel P. Berrangé wrote:
> > > +#
> > > +# Since: 7.0
> > > +##
> > > +{ 'enum' : 'StatType',
> > > + 'data' : [ 'cumulative', 'instant', 'peak',
> > > + 'linear-hist', 'log-hist', 'unknown' ] }
> > IMHO 'unknown' shouldn't exist at all.
> >
>
> I added the 'unknown' member here (and in other enums) to handle situations
> where QEMU is behind KVM in terms of enumerating the various stat types,
> units, etc. I feel this will be a semi-common scenario (old QEMU on a new
> kernel) and with 'unknown', QEMU can at least display the raw data.
>
> That said, I happy skip 'unknown' entries if you think that's better.
Yep, I don't think we should be including 'unknown' stuff.
An application could use this, and then we add support for the
new type and the application will now break with new QEMU because
it is presented in QMP in a different way.
The same for the 'unknown' base and unit too for that matter.
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] 17+ messages in thread* Re: [PATCH v3 1/3] qmp: Support for querying stats
2022-02-03 18:30 ` Daniel P. Berrangé
@ 2022-02-03 18:37 ` Mark Kanda
0 siblings, 0 replies; 17+ messages in thread
From: Mark Kanda @ 2022-02-03 18:37 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: pbonzini, qemu-devel, armbru
On 2/3/2022 12:30 PM, Daniel P. Berrangé wrote:
> On Thu, Feb 03, 2022 at 12:12:57PM -0600, Mark Kanda wrote:
>> Thanks Daniel,
>>
>> On 2/1/2022 6:08 AM, Daniel P. Berrangé wrote:
>>>> +#
>>>> +# Since: 7.0
>>>> +##
>>>> +{ 'enum' : 'StatType',
>>>> + 'data' : [ 'cumulative', 'instant', 'peak',
>>>> + 'linear-hist', 'log-hist', 'unknown' ] }
>>> IMHO 'unknown' shouldn't exist at all.
>>>
>> I added the 'unknown' member here (and in other enums) to handle situations
>> where QEMU is behind KVM in terms of enumerating the various stat types,
>> units, etc. I feel this will be a semi-common scenario (old QEMU on a new
>> kernel) and with 'unknown', QEMU can at least display the raw data.
>>
>> That said, I happy skip 'unknown' entries if you think that's better.
> Yep, I don't think we should be including 'unknown' stuff.
>
> An application could use this, and then we add support for the
> new type and the application will now break with new QEMU because
> it is presented in QMP in a different way.
>
> The same for the 'unknown' base and unit too for that matter.
>
Sounds good. I'll implement the other changes you suggested in v4.
Thanks/regards,
-Mark
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/3] qmp: Support for querying stats
2022-02-03 18:12 ` Mark Kanda
2022-02-03 18:30 ` Daniel P. Berrangé
@ 2022-02-03 18:39 ` Paolo Bonzini
1 sibling, 0 replies; 17+ messages in thread
From: Paolo Bonzini @ 2022-02-03 18:39 UTC (permalink / raw)
To: Mark Kanda, Daniel P. Berrangé; +Cc: qemu-devel, armbru
On 2/3/22 19:12, Mark Kanda wrote:
>
> I added the 'unknown' member here (and in other enums) to handle
> situations where QEMU is behind KVM in terms of enumerating the various
> stat types, units, etc. I feel this will be a semi-common scenario (old
> QEMU on a new kernel) and with 'unknown', QEMU can at least display the
> raw data.
I think you can skip them, there aren't really plans for new types (the
"metaschema" is based on one that has been in use for quite some time at
Google).
Paolo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/3] qmp: Support for querying stats
2022-02-01 12:08 ` Daniel P. Berrangé
2022-02-03 18:12 ` Mark Kanda
@ 2022-02-03 18:38 ` Paolo Bonzini
2022-02-03 18:53 ` Daniel P. Berrangé
2022-02-03 18:52 ` Mark Kanda
2 siblings, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2022-02-03 18:38 UTC (permalink / raw)
To: Daniel P. Berrangé, Mark Kanda; +Cc: qemu-devel, armbru
On 2/1/22 13:08, Daniel P. Berrangé wrote:
> I still feel like this is rather verbose, and should be simplified
> down to.
>
> { "return": {
> "vm": {
> "kvm": [ ... ]
> "provider-XYZ": [ ... ],
> ...
> }
> }
My main qualm with this is that not just QEMU, but every layer above
then needs to either treat stats as a "dynamic" type unless they want to
only handle providers that they know.
The main reason why I asked Mark to do all this work, was so that new
stats and stats providers could be added transparently, and only new
*targets* would need work all over the stack (but those are fewer, for
example blockdev/netdev/iothread).
Paolo
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v3 1/3] qmp: Support for querying stats
2022-02-03 18:38 ` Paolo Bonzini
@ 2022-02-03 18:53 ` Daniel P. Berrangé
0 siblings, 0 replies; 17+ messages in thread
From: Daniel P. Berrangé @ 2022-02-03 18:53 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, armbru
On Thu, Feb 03, 2022 at 07:38:08PM +0100, Paolo Bonzini wrote:
> On 2/1/22 13:08, Daniel P. Berrangé wrote:
> > I still feel like this is rather verbose, and should be simplified
> > down to.
> >
> > { "return": {
> > "vm": {
> > "kvm": [ ... ]
> > "provider-XYZ": [ ... ],
> > ...
> > }
> > }
>
> My main qualm with this is that not just QEMU, but every layer above then
> needs to either treat stats as a "dynamic" type unless they want to only
> handle providers that they know.
>
> The main reason why I asked Mark to do all this work, was so that new stats
> and stats providers could be added transparently, and only new *targets*
> would need work all over the stack (but those are fewer, for example
> blockdev/netdev/iothread).
Hmm, yes, i see what you mean.
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] 17+ messages in thread
* Re: [PATCH v3 1/3] qmp: Support for querying stats
2022-02-01 12:08 ` Daniel P. Berrangé
2022-02-03 18:12 ` Mark Kanda
2022-02-03 18:38 ` Paolo Bonzini
@ 2022-02-03 18:52 ` Mark Kanda
2 siblings, 0 replies; 17+ messages in thread
From: Mark Kanda @ 2022-02-03 18:52 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: pbonzini, qemu-devel, armbru
[-- Attachment #1: Type: text/plain, Size: 979 bytes --]
On 2/1/2022 6:08 AM, Daniel P. Berrangé wrote:
>> +##
>> +# @StatsResults:
>> +#
>> +# Target specific results.
>> +#
>> +# Since: 7.0
>> +##
>> +{ 'union': 'StatsResults',
>> + 'base': { 'target': 'StatsTarget' },
>> + 'discriminator': 'target',
>> + 'data': { 'vcpu': 'VCPUStatsResults',
>> + 'vm': 'VMStatsResults' } }
> I feel we can simplify this all down somewhat, eliminating levels
> of redundant nesting
>
> { 'struct': 'StatsResultsEntry',
> 'data': { '*kvm': [ 'Stats' ] } }
>
> { 'struct': 'StatsResultsVCPUEntry',
> 'base': 'StatsResultsEntry',
> 'data': 'path': 'str' } }
>
> { 'struct': 'StatsResults',
> 'data': {
> '*vcpus': ['StatsResultsVCPUEntry'],
> '*vm': 'StatsResultsEntry'
> }
> }
>
>
I'm happy to make this change, but I would like Paolo to comment as he had
suggested the StatsResults layout [1].
Thanks Daniel/Paolo,
-Mark
[1] https://lore.kernel.org/all/ee0d6990-06f3-9a1b-f7d5-7c379f0e9773@redhat.com/
[-- Attachment #2: Type: text/html, Size: 1471 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 2/3] hmp: Support for querying stats
2022-01-31 19:43 [PATCH v3 0/3] Support fd-based KVM stats Mark Kanda
2022-01-31 19:43 ` [PATCH v3 1/3] qmp: Support for querying stats Mark Kanda
@ 2022-01-31 19:43 ` Mark Kanda
2022-01-31 19:43 ` [PATCH v3 3/3] kvm: Support for querying fd-based stats Mark Kanda
2 siblings, 0 replies; 17+ messages in thread
From: Mark Kanda @ 2022-01-31 19:43 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, berrange, armbru
Leverage the QMP support for querying stats. The interface supports the same
arguments as the QMP interface. Wildcard char (*) is accepted for names and
stats target.
Examples (with fd-based KVM stats):
- Display all stats
(qemu) info stats
vm
provider: kvm
max_mmu_page_hash_collisions (peak): 0
max_mmu_rmap_size (peak): 0
...
vcpu (qom path: /machine/unattached/device[0])
provider: kvm
guest_mode (instant): 0
directed_yield_successful (cumulative): 0
...
(qemu) info stats-schemas
vm
provider: kvm
max_mmu_page_hash_collisions (peak)
max_mmu_rmap_size (peak)
...
vcpu
provider: kvm
guest_mode (instant)
directed_yield_successful (cumulative)
- Display 'halt_wait_ns' and 'exits' for vCPUs with qom paths
/machine/unattached/device[2] and /machine/unattached/device[4]:
(qemu) info stats exits,halt_wait_ns /machine/unattached/device[2],
/machine/unattached/device[4]
vcpu (qom path: /machine/unattached/device[2])
provider: kvm
exits (cumulative): 52369
halt_wait_ns (cumulative nanoseconds): 416092704390
vcpu (qom path: /machine/unattached/device[4])
provider: kvm
exits (cumulative): 52550
halt_wait_ns (cumulative nanoseconds): 419637402657
- Display all VM stats for provider KVM:
(qemu) info stats * vm kvm
vm
provider: kvm
max_mmu_page_hash_collisions (peak): 0
max_mmu_rmap_size (peak): 0
nx_lpage_splits (instant): 51
...
Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
---
hmp-commands-info.hx | 28 ++++
include/monitor/hmp.h | 2 +
monitor/hmp-cmds.c | 288 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 318 insertions(+)
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index e90f20a107..7365a8e002 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -879,3 +879,31 @@ SRST
``info sgx``
Show intel SGX information.
ERST
+
+ {
+ .name = "stats",
+ .args_type = "names:s?,paths:s?,provider:s?",
+ .params = "[names] [paths] [provider]",
+ .help = "show statistics; optional comma separated names, "
+ "vcpu qom paths, and provider",
+ .cmd = hmp_info_stats,
+ },
+
+SRST
+ ``stats``
+ Show stats
+ERST
+
+ {
+ .name = "stats-schemas",
+ .args_type = "provider:s?",
+ .params = "[provider]",
+ .help = "show statistics schema for each provider",
+ .cmd = hmp_info_stats_schemas,
+ },
+
+SRST
+ ``stats-schemas``
+ Show stats per schema
+ERST
+
diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index 96d014826a..a748511105 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -133,5 +133,7 @@ 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);
#endif
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 8c384dc1b2..19b2a585d6 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -2178,3 +2178,291 @@ 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)
+{
+ monitor_printf(mon, " %s (%s", value->name, StatType_str(value->type));
+
+ if (value->unit == STAT_UNIT_SECONDS &&
+ value->exponent >= -9 && value->exponent <= 0 &&
+ value->exponent % 3 == 0 && value->base == STAT_BASE_POW10) {
+
+ const char *si_prefix[] = { "", "milli", "micro", "nano" };
+ monitor_printf(mon, " %s", si_prefix[value->exponent / -3]);
+
+ } else if (value->unit == STAT_UNIT_BYTES &&
+ value->exponent >= 0 && value->exponent <= 40 &&
+ value->exponent % 10 == 0 && value->base == STAT_BASE_POW2) {
+
+ const char *si_prefix[] = {
+ "", "kilo", "mega", "giga", "tera" };
+ monitor_printf(mon, " %s", si_prefix[value->exponent / 10]);
+
+ } else if (value->exponent) {
+ /* Print the base and exponent as "x <base>^<exp>" */
+ monitor_printf(mon, " x %s^%d", StatBase_str(value->base),
+ value->exponent);
+ }
+
+ /* Don't print "none" unit type */
+ monitor_printf(mon, "%s)", value->unit == STAT_UNIT_NONE ?
+ "" : StatUnit_str(value->unit));
+}
+
+static StatsSchemaValueList *find_schema_value_list(
+ StatsSchemaProviderList *list, StatsProvider provider)
+{
+ StatsSchemaProviderList *schema_provider_list;
+
+ for (schema_provider_list = list;
+ schema_provider_list;
+ schema_provider_list = schema_provider_list->next) {
+ if (schema_provider_list->value->provider == provider) {
+ return schema_provider_list->value->stats;
+ }
+ }
+ return NULL;
+}
+
+static void print_stats_results_entry_list(Monitor *mon, StatsTarget type,
+ StatsResultsEntryList *list,
+ StatsSchemaProviderList *schema)
+{
+ StatsResultsEntryList *results_entry_list;
+
+ for (results_entry_list = list;
+ results_entry_list;
+ results_entry_list = results_entry_list->next) {
+
+ StatsResultsEntry *results_entry = results_entry_list->value;
+ monitor_printf(mon, " provider: %s\n",
+ StatsProvider_str(results_entry->provider));
+
+ /* Find provider schema */
+ StatsSchemaValueList *schema_value_list =
+ find_schema_value_list(schema, results_entry->provider);
+ StatsList *stats_list;
+
+ for (stats_list = results_entry->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, ": %ld\n", stats_value->u.scalar);
+ } else if (stats_value->type == QTYPE_QDICT) {
+ uint64List *list;
+ int i;
+
+ monitor_printf(mon, ": ");
+ for (list = stats_value->u.list.list, i = 1;
+ list;
+ list = list->next, i++) {
+ monitor_printf(mon, "[%d]=%ld ", i, list->value);
+ }
+ monitor_printf(mon, "\n");
+ }
+ }
+ }
+}
+
+static const char wildcard[] = "*";
+static StatsFilter *stats_filter(StatsTarget target, const char *name,
+ StatsProvider provider)
+{
+ StatsFilter *filter = g_malloc0(sizeof(*filter));
+ filter->target = target;
+
+ switch (target) {
+ case STATS_TARGET_VM:
+ if (name && !g_str_equal(name, wildcard)) {
+ filter->u.vm.fields = strList_from_comma_list(name);
+ filter->u.vm.has_fields = true;
+ }
+ if (provider != STATS_PROVIDER__MAX) {
+ filter->u.vm.provider = provider;
+ filter->u.vm.has_provider = true;
+ }
+ break;
+ case STATS_TARGET_VCPU:
+ if (name && !g_str_equal(name, wildcard)) {
+ filter->u.vcpu.fields = strList_from_comma_list(name);
+ filter->u.vcpu.has_fields = true;
+ }
+ if (provider != STATS_PROVIDER__MAX) {
+ filter->u.vcpu.provider = provider;
+ filter->u.vcpu.has_provider = true;
+ }
+ break;
+ default:
+ break;
+ }
+ return filter;
+}
+
+void hmp_info_stats(Monitor *mon, const QDict *qdict)
+{
+ const char *names = qdict_get_try_str(qdict, "names");
+ const char *paths = qdict_get_try_str(qdict, "paths");
+ const char *provider = qdict_get_try_str(qdict, "provider");
+
+ StatsProvider stats_provider = STATS_PROVIDER__MAX;
+ StatsTarget target;
+ Error *err = NULL;
+
+ if (provider) {
+ for (stats_provider = 0; stats_provider < STATS_PROVIDER__MAX;
+ stats_provider++) {
+ if (g_str_equal(StatsProvider_str(stats_provider), provider)) {
+ break;
+ }
+ }
+ if (stats_provider == STATS_PROVIDER__MAX) {
+ monitor_printf(mon, "invalid stats filter provider %s\n",
+ provider);
+ goto exit;
+ }
+ }
+
+ for (target = 0; target < STATS_TARGET__MAX; target++) {
+ StatsResults *stats_results = NULL;
+ StatsSchemaResult *schema_results = NULL;
+ StatsFilter *filter = stats_filter(target, names, stats_provider);
+
+ switch (target) {
+ case STATS_TARGET_VM:
+ if (paths && !g_str_equal(paths, wildcard) &&
+ !g_str_equal(paths, StatsTarget_str(STATS_TARGET_VM))) {
+ break;
+ }
+ stats_results = qmp_query_stats(filter, &err);
+ schema_results =
+ qmp_query_stats_schemas(provider ? true : false,
+ stats_provider, &err);
+
+ if (!stats_results->u.vm.list) {
+ break;
+ }
+ monitor_printf(mon, "%s\n", StatsTarget_str(STATS_TARGET_VM));
+ print_stats_results_entry_list(mon, STATS_TARGET_VM,
+ stats_results->u.vm.list,
+ schema_results->vm);
+ break;
+ case STATS_TARGET_VCPU:
+ if (paths && !g_str_equal(paths, wildcard) &&
+ !g_str_equal(paths, StatsTarget_str(STATS_TARGET_VCPU))) {
+ /* apply filter for specified paths */
+ filter->u.vcpu.paths = strList_from_comma_list(paths);
+ filter->u.vcpu.has_paths = true;
+ }
+
+ stats_results = qmp_query_stats(filter, &err);
+ schema_results =
+ qmp_query_stats_schemas(provider ? true : false,
+ stats_provider, &err);
+
+ VCPUResultsEntryList *results_entry_list;
+ for (results_entry_list = stats_results->u.vcpu.list;
+ results_entry_list;
+ results_entry_list = results_entry_list->next) {
+ monitor_printf(mon, "%s (qom path: %s)\n",
+ StatsTarget_str(STATS_TARGET_VCPU),
+ results_entry_list->value->path);
+ print_stats_results_entry_list(mon, STATS_TARGET_VCPU,
+ results_entry_list->value->list,
+ schema_results->vcpu);
+ }
+ break;
+ default:
+ break;
+ }
+ qapi_free_StatsFilter(filter);
+ qapi_free_StatsSchemaResult(schema_results);
+ qapi_free_StatsResults(stats_results);
+ }
+
+exit:
+ if (err) {
+ monitor_printf(mon, "%s\n", error_get_pretty(err));
+ error_free(err);
+ }
+}
+
+static void print_stats_schema_list(Monitor *mon, StatsSchemaProviderList *list)
+{
+ StatsSchemaProviderList *schema_provider_list;
+
+ for (schema_provider_list = list;
+ schema_provider_list;
+ schema_provider_list = schema_provider_list->next) {
+
+ StatsSchemaProvider *schema_provider =
+ schema_provider_list->value;
+ monitor_printf(mon, " provider: %s\n",
+ StatsProvider_str(schema_provider->provider));
+
+ StatsSchemaValueList *schema_value_list;
+ for (schema_value_list = schema_provider->stats;
+ schema_value_list;
+ schema_value_list = schema_value_list->next) {
+
+ StatsSchemaValue *schema_value = schema_value_list->value;
+ print_stats_schema_value(mon, schema_value);
+ monitor_printf(mon, "\n");
+ }
+ }
+}
+
+void hmp_info_stats_schemas(Monitor *mon, const QDict *qdict)
+{
+ const char *provider = qdict_get_try_str(qdict, "provider");
+ StatsProvider stats_provider = STATS_PROVIDER__MAX;
+ StatsSchemaResult *schema_result;
+ Error *err = NULL;
+
+ if (provider) {
+ for (stats_provider = 0; stats_provider < STATS_PROVIDER__MAX;
+ stats_provider++) {
+ if (g_str_equal(StatsProvider_str(stats_provider), provider)) {
+ break;
+ }
+ }
+ if (stats_provider == STATS_PROVIDER__MAX) {
+ monitor_printf(mon, "invalid stats filter provider %s\n", provider);
+ return;
+ }
+ }
+
+ schema_result =
+ qmp_query_stats_schemas(provider ? true : false, stats_provider, &err);
+
+ if (err) {
+ monitor_printf(mon, "%s\n", error_get_pretty(err));
+ error_free(err);
+ return;
+ }
+
+ monitor_printf(mon, "%s\n", StatsTarget_str(STATS_TARGET_VM));
+ print_stats_schema_list(mon, schema_result->vm);
+ monitor_printf(mon, "%s\n", StatsTarget_str(STATS_TARGET_VCPU));
+ print_stats_schema_list(mon, schema_result->vcpu);
+
+ qapi_free_StatsSchemaResult(schema_result);
+}
--
2.27.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v3 3/3] kvm: Support for querying fd-based stats
2022-01-31 19:43 [PATCH v3 0/3] Support fd-based KVM stats Mark Kanda
2022-01-31 19:43 ` [PATCH v3 1/3] qmp: Support for querying stats Mark Kanda
2022-01-31 19:43 ` [PATCH v3 2/3] hmp: " Mark Kanda
@ 2022-01-31 19:43 ` Mark Kanda
2022-02-01 10:08 ` Daniel P. Berrangé
2022-02-01 10:51 ` Paolo Bonzini
2 siblings, 2 replies; 17+ messages in thread
From: Mark Kanda @ 2022-01-31 19:43 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, berrange, armbru
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 | 308 ++++++++++++++++++++++++++++++++++++++++++++
qapi/misc.json | 2 +-
2 files changed, 309 insertions(+), 1 deletion(-)
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 0e66ebb497..e43e1f1c1c 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/stats.h"
/* This check must be after config-host.h is included */
#ifdef CONFIG_EVENTFD
@@ -2309,6 +2311,9 @@ bool kvm_dirty_ring_enabled(void)
return kvm_state->kvm_dirty_ring_size ? true : false;
}
+static void query_stats_cb(StatsResults *, StatsFilter *, Error **);
+static void query_stats_schemas_cb(StatsSchemaResult *, Error **);
+
static int kvm_init(MachineState *ms)
{
MachineClass *mc = MACHINE_GET_CLASS(ms);
@@ -2637,6 +2642,11 @@ static int kvm_init(MachineState *ms)
}
}
+ if (kvm_check_extension(kvm_state, KVM_CAP_BINARY_STATS_FD)) {
+ add_stats_callbacks(STATS_PROVIDER_KVM, &query_stats_cb,
+ &query_stats_schemas_cb);
+ }
+
return 0;
err:
@@ -3696,3 +3706,301 @@ static void kvm_type_init(void)
}
type_init(kvm_type_init);
+
+typedef struct StatsArgs {
+ StatsFilter *filter;
+ bool is_schema;
+ union {
+ StatsResults *stats;
+ StatsSchemaResult *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 = g_malloc0(sizeof(*stats_entry));
+ Stats *stats = g_malloc0(sizeof(*stats));
+ uint64List *val_list = NULL;
+ int i;
+
+ stats->name = g_strdup(pdesc->name);
+ stats->value = g_malloc0(sizeof(*stats->value));
+
+ /* Alloc and populate data list */
+ if (pdesc->size == 1) {
+ stats->value->u.scalar = *stats_data;
+ stats->value->type = QTYPE_QNUM;
+ } else {
+ 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;
+ }
+ stats->value->u.list.list = val_list;
+ stats->value->type = QTYPE_QDICT;
+ }
+
+ 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;
+
+ schema_entry = g_malloc0(sizeof(*schema_entry));
+ schema_entry->value = g_malloc0(sizeof(*schema_entry->value));
+ schema_entry->value->name = g_strdup(pdesc->name);
+
+ switch (pdesc->flags & KVM_STATS_TYPE_MASK) {
+ case KVM_STATS_TYPE_CUMULATIVE:
+ schema_entry->value->type = STAT_TYPE_CUMULATIVE;
+ break;
+ case KVM_STATS_TYPE_INSTANT:
+ schema_entry->value->type = STAT_TYPE_INSTANT;
+ break;
+ case KVM_STATS_TYPE_PEAK:
+ schema_entry->value->type = STAT_TYPE_PEAK;
+ break;
+ case KVM_STATS_TYPE_LINEAR_HIST:
+ schema_entry->value->type = STAT_TYPE_LINEAR_HIST;
+ break;
+ case KVM_STATS_TYPE_LOG_HIST:
+ schema_entry->value->type = STAT_TYPE_LOG_HIST;
+ break;
+ default:
+ schema_entry->value->type = STAT_TYPE_UNKNOWN;
+ }
+
+ switch (pdesc->flags & KVM_STATS_UNIT_MASK) {
+ case KVM_STATS_UNIT_NONE:
+ schema_entry->value->unit = STAT_UNIT_NONE;
+ break;
+ case KVM_STATS_UNIT_BYTES:
+ schema_entry->value->unit = STAT_UNIT_BYTES;
+ break;
+ case KVM_STATS_UNIT_CYCLES:
+ schema_entry->value->unit = STAT_UNIT_CYCLES;
+ break;
+ case KVM_STATS_UNIT_SECONDS:
+ schema_entry->value->unit = STAT_UNIT_SECONDS;
+ break;
+ default:
+ schema_entry->value->unit = STAT_UNIT_UNKNOWN;
+ }
+
+ switch (pdesc->flags & KVM_STATS_BASE_MASK) {
+ case KVM_STATS_BASE_POW10:
+ schema_entry->value->base = STAT_BASE_POW10;
+ break;
+ case KVM_STATS_BASE_POW2:
+ schema_entry->value->base = STAT_BASE_POW2;
+ break;
+ default:
+ schema_entry->value->base = STAT_BASE_UNKNOWN;
+ }
+
+ schema_entry->value->exponent = pdesc->exponent;
+ schema_entry->next = list;
+ return schema_entry;
+}
+
+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 *stats_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;
+ }
+
+ /* Add entry to the list */
+ if (kvm_stat_args->is_schema) {
+ stats_list = add_kvmschema_entry(pdesc, (StatsSchemaValueList *)
+ stats_list, &local_err);
+ } else {
+ if (stat_name_filter(kvm_stat_args->filter,
+ kvm_stat_args->filter->target, pdesc->name)) {
+ g_free(stats_data);
+ continue;
+ }
+ stats_list = add_kvmstat_entry(pdesc, stats_data,
+ (StatsList *) stats_list,
+ &local_err);
+ }
+ g_free(stats_data);
+ }
+
+ if (!stats_list) {
+ goto exit;
+ }
+
+ if (kvm_stat_args->is_schema) {
+ StatsSchemaProvider *provider;
+ if (kvm_stat_args->filter->target == STATS_TARGET_VM) {
+ provider = add_vm_stats_schema(kvm_stat_args->result.schema,
+ STATS_PROVIDER_KVM);
+ } else if (kvm_stat_args->filter->target == STATS_TARGET_VCPU) {
+ provider = add_vcpu_stats_schema(kvm_stat_args->result.schema,
+ STATS_PROVIDER_KVM);
+ }
+ provider->stats = (StatsSchemaValueList *)stats_list;
+ } else {
+ StatsResultsEntry *results;
+ if (kvm_stat_args->filter->target == STATS_TARGET_VM) {
+ results = add_vm_stats_entry(kvm_stat_args->result.stats,
+ STATS_PROVIDER_KVM);
+ } else if (kvm_stat_args->filter->target == STATS_TARGET_VCPU) {
+ results =
+ add_vcpu_stats_entry(kvm_stat_args->result.stats,
+ STATS_PROVIDER_KVM,
+ current_cpu->parent_obj.canonical_path);
+ }
+ results->stats = (StatsList *)stats_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 void query_stats_cb(StatsResults *stats_result, StatsFilter *filter,
+ Error **errp)
+{
+ KVMState *s = kvm_state;
+ CPUState *cpu;
+ int stats_fd;
+ StatsArgs *stats_args = g_malloc0(sizeof(*stats_args));
+ stats_args->filter = filter;
+ stats_args->errp = errp;
+ stats_args->result.stats = stats_result;
+
+ switch (filter->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");
+ goto done;
+ }
+
+ query_stats(stats_args, stats_fd);
+ break;
+ case STATS_TARGET_VCPU:
+ CPU_FOREACH(cpu) {
+ if (stat_cpu_filter(filter, cpu->parent_obj.canonical_path)) {
+ continue;
+ }
+ run_on_cpu(cpu, query_stats_vcpu, RUN_ON_CPU_HOST_PTR(stats_args));
+ }
+ break;
+ default:
+ break;
+ }
+done:
+ g_free(stats_args);
+}
+
+void query_stats_schemas_cb(StatsSchemaResult *stats_result, Error **errp)
+{
+ KVMState *s = kvm_state;
+ int stats_fd;
+ StatsArgs *stats_args = g_malloc0(sizeof(*stats_args));
+ stats_args->filter = g_malloc0(sizeof(*stats_args->filter));
+ stats_args->errp = errp;
+
+ stats_fd = kvm_vm_ioctl(s, KVM_GET_STATS_FD, NULL);
+ if (stats_fd == -1) {
+ error_setg(errp, "KVM stats: ioctl failed");
+ goto done;
+ }
+
+ stats_args->result.schema = stats_result;
+ stats_args->is_schema = true;
+
+ /* Query VM */
+ stats_args->filter->target = STATS_TARGET_VM;
+ query_stats(stats_args, stats_fd);
+
+ /* Query vCPU */
+ stats_args->filter->target = STATS_TARGET_VCPU;
+ run_on_cpu(first_cpu, query_stats_vcpu, RUN_ON_CPU_HOST_PTR(stats_args));
+done:
+ g_free(stats_args->filter);
+ g_free(stats_args);
+}
+
diff --git a/qapi/misc.json b/qapi/misc.json
index 8d326464f0..39a7d172cb 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -574,7 +574,7 @@
# Since: 7.0
##
{ 'enum': 'StatsProvider',
- 'data': [ ] }
+ 'data': [ 'kvm' ] }
##
# @StatsTarget:
--
2.27.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v3 3/3] kvm: Support for querying fd-based stats
2022-01-31 19:43 ` [PATCH v3 3/3] kvm: Support for querying fd-based stats Mark Kanda
@ 2022-02-01 10:08 ` Daniel P. Berrangé
2022-02-01 10:51 ` Paolo Bonzini
1 sibling, 0 replies; 17+ messages in thread
From: Daniel P. Berrangé @ 2022-02-01 10:08 UTC (permalink / raw)
To: Mark Kanda; +Cc: pbonzini, qemu-devel, armbru
On Mon, Jan 31, 2022 at 01:43:12PM -0600, Mark Kanda wrote:
> 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 | 308 ++++++++++++++++++++++++++++++++++++++++++++
> qapi/misc.json | 2 +-
> 2 files changed, 309 insertions(+), 1 deletion(-)
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 0e66ebb497..e43e1f1c1c 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/stats.h"
>
> /* This check must be after config-host.h is included */
> #ifdef CONFIG_EVENTFD
> @@ -2309,6 +2311,9 @@ bool kvm_dirty_ring_enabled(void)
> return kvm_state->kvm_dirty_ring_size ? true : false;
> }
>
> +static void query_stats_cb(StatsResults *, StatsFilter *, Error **);
> +static void query_stats_schemas_cb(StatsSchemaResult *, Error **);
> +
> static int kvm_init(MachineState *ms)
> {
> MachineClass *mc = MACHINE_GET_CLASS(ms);
> @@ -2637,6 +2642,11 @@ static int kvm_init(MachineState *ms)
> }
> }
>
> + if (kvm_check_extension(kvm_state, KVM_CAP_BINARY_STATS_FD)) {
> + add_stats_callbacks(STATS_PROVIDER_KVM, &query_stats_cb,
> + &query_stats_schemas_cb);
> + }
> +
> return 0;
>
> err:
> @@ -3696,3 +3706,301 @@ static void kvm_type_init(void)
> }
>
> type_init(kvm_type_init);
> +
> +typedef struct StatsArgs {
> + StatsFilter *filter;
> + bool is_schema;
> + union {
> + StatsResults *stats;
> + StatsSchemaResult *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 = g_malloc0(sizeof(*stats_entry));
> + Stats *stats = g_malloc0(sizeof(*stats));
> + uint64List *val_list = NULL;
> + int i;
> +
> + stats->name = g_strdup(pdesc->name);
> + stats->value = g_malloc0(sizeof(*stats->value));
> +
> + /* Alloc and populate data list */
> + if (pdesc->size == 1) {
> + stats->value->u.scalar = *stats_data;
> + stats->value->type = QTYPE_QNUM;
> + } else {
> + 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;
> + }
> + stats->value->u.list.list = val_list;
> + stats->value->type = QTYPE_QDICT;
> + }
> +
> + 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;
> +
> + schema_entry = g_malloc0(sizeof(*schema_entry));
> + schema_entry->value = g_malloc0(sizeof(*schema_entry->value));
> + schema_entry->value->name = g_strdup(pdesc->name);
> +
> + switch (pdesc->flags & KVM_STATS_TYPE_MASK) {
> + case KVM_STATS_TYPE_CUMULATIVE:
> + schema_entry->value->type = STAT_TYPE_CUMULATIVE;
> + break;
> + case KVM_STATS_TYPE_INSTANT:
> + schema_entry->value->type = STAT_TYPE_INSTANT;
> + break;
> + case KVM_STATS_TYPE_PEAK:
> + schema_entry->value->type = STAT_TYPE_PEAK;
> + break;
> + case KVM_STATS_TYPE_LINEAR_HIST:
> + schema_entry->value->type = STAT_TYPE_LINEAR_HIST;
> + break;
> + case KVM_STATS_TYPE_LOG_HIST:
> + schema_entry->value->type = STAT_TYPE_LOG_HIST;
> + break;
> + default:
> + schema_entry->value->type = STAT_TYPE_UNKNOWN;
> + }
> +
> + switch (pdesc->flags & KVM_STATS_UNIT_MASK) {
> + case KVM_STATS_UNIT_NONE:
> + schema_entry->value->unit = STAT_UNIT_NONE;
> + break;
> + case KVM_STATS_UNIT_BYTES:
> + schema_entry->value->unit = STAT_UNIT_BYTES;
> + break;
> + case KVM_STATS_UNIT_CYCLES:
> + schema_entry->value->unit = STAT_UNIT_CYCLES;
> + break;
> + case KVM_STATS_UNIT_SECONDS:
> + schema_entry->value->unit = STAT_UNIT_SECONDS;
> + break;
> + default:
> + schema_entry->value->unit = STAT_UNIT_UNKNOWN;
> + }
> +
> + switch (pdesc->flags & KVM_STATS_BASE_MASK) {
> + case KVM_STATS_BASE_POW10:
> + schema_entry->value->base = STAT_BASE_POW10;
> + break;
> + case KVM_STATS_BASE_POW2:
> + schema_entry->value->base = STAT_BASE_POW2;
> + break;
> + default:
> + schema_entry->value->base = STAT_BASE_UNKNOWN;
> + }
> +
> + schema_entry->value->exponent = pdesc->exponent;
> + schema_entry->next = list;
> + return schema_entry;
> +}
> +
> +static void query_stats(StatsArgs *kvm_stat_args, int stats_fd)
Pass "Error **errp". For that matter, just pass in all fields
directly - no need for 'StatsArgs' indirection here - that's
only needed in the VCPU stats case due to need to pass many
values via the run_on_cpu callback.
> +{
> + size_t size_desc, size_data;
> + struct kvm_stats_header *header;
g_autofree struct kvm_stats_header *header = NULL;
> + struct kvm_stats_desc *stats_desc = NULL;
g_autofree
> + Error *local_err = NULL;
Should not be required if errp is passed in
> + void *stats_list = NULL;
> + char *id = NULL;
g_autofree
> + 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;
> + }
> +
> + /* Add entry to the list */
> + if (kvm_stat_args->is_schema) {
> + stats_list = add_kvmschema_entry(pdesc, (StatsSchemaValueList *)
> + stats_list, &local_err);
> + } else {
> + if (stat_name_filter(kvm_stat_args->filter,
> + kvm_stat_args->filter->target, pdesc->name)) {
> + g_free(stats_data);
> + continue;
> + }
> + stats_list = add_kvmstat_entry(pdesc, stats_data,
> + (StatsList *) stats_list,
> + &local_err);
> + }
> + g_free(stats_data);
> + }
> +
> + if (!stats_list) {
> + goto exit;
> + }
> +
> + if (kvm_stat_args->is_schema) {
> + StatsSchemaProvider *provider;
> + if (kvm_stat_args->filter->target == STATS_TARGET_VM) {
> + provider = add_vm_stats_schema(kvm_stat_args->result.schema,
> + STATS_PROVIDER_KVM);
> + } else if (kvm_stat_args->filter->target == STATS_TARGET_VCPU) {
> + provider = add_vcpu_stats_schema(kvm_stat_args->result.schema,
> + STATS_PROVIDER_KVM);
> + }
> + provider->stats = (StatsSchemaValueList *)stats_list;
The compiler warns about provider being used uninitialized.
This would be better as a switch
StatsSchemaProvider *provider = NULL;
switch (kvm_stat_args->filter->target){
case STATS_TARGET_VM:
provider = add_vm_stats_schema(kvm_stat_args->result.schema,
STATS_PROVIDER_KVM);
break;
case STATS_TARGET_VCPU:
provider = add_vcpu_stats_schema(kvm_stat_args->result.schema,
STATS_PROVIDER_KVM);
break;
default:
break;
}
if (provider) {
provider->stats = (StatsSchemaValueList *)stats_list;
}
> + } else {
> + StatsResultsEntry *results;
> + if (kvm_stat_args->filter->target == STATS_TARGET_VM) {
> + results = add_vm_stats_entry(kvm_stat_args->result.stats,
> + STATS_PROVIDER_KVM);
> + } else if (kvm_stat_args->filter->target == STATS_TARGET_VCPU) {
> + results =
> + add_vcpu_stats_entry(kvm_stat_args->result.stats,
> + STATS_PROVIDER_KVM,
> + current_cpu->parent_obj.canonical_path);
> + }
> + results->stats = (StatsList *)stats_list;
Same compiler around as previous block
> + }
> +exit:
> + error_propagate(kvm_stat_args->errp, local_err);
Not required if we use errp from the caller directly.
> + g_free(stats_desc);
> + g_free(id);
> + g_free(header);
Not needed with g_autofree usage, so we don't need the 'exit:'
label at all.
> +}
> +
> +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);
Get rid of local_err and pass kvm_stat_args->errp straight to error_setg
> + return;
> + }
> + query_stats(kvm_stats_args, stats_fd);
Pass kvm_stats_args->errp into this.
> + close(stats_fd);
> +}
> +
> +static void query_stats_cb(StatsResults *stats_result, StatsFilter *filter,
> + Error **errp)
> +{
> + KVMState *s = kvm_state;
> + CPUState *cpu;
> + int stats_fd;
> + StatsArgs *stats_args = g_malloc0(sizeof(*stats_args));
g_autofree
> + stats_args->filter = filter;
> + stats_args->errp = errp;
> + stats_args->result.stats = stats_result;
> +
> + switch (filter->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");
> + goto done;
> + }
> +
> + query_stats(stats_args, stats_fd);
Pass errp into this.
> + break;
> + case STATS_TARGET_VCPU:
> + CPU_FOREACH(cpu) {
> + if (stat_cpu_filter(filter, cpu->parent_obj.canonical_path)) {
> + continue;
> + }
> + run_on_cpu(cpu, query_stats_vcpu, RUN_ON_CPU_HOST_PTR(stats_args));
> + }
> + break;
> + default:
> + break;
> + }
> +done:
> + g_free(stats_args);
No need for this label / g_free, if we use g_autofree
> +}
> +
> +void query_stats_schemas_cb(StatsSchemaResult *stats_result, Error **errp)
> +{
> + KVMState *s = kvm_state;
> + int stats_fd;
> + StatsArgs *stats_args = g_malloc0(sizeof(*stats_args));
g_autofree + g_new0
> + stats_args->filter = g_malloc0(sizeof(*stats_args->filter));
g_autofree StatsFilter *filter = g_new(StatsFilter, 1);
stats_args->filter = filter;
> + stats_args->errp = errp;
> +
> + stats_fd = kvm_vm_ioctl(s, KVM_GET_STATS_FD, NULL);
> + if (stats_fd == -1) {
> + error_setg(errp, "KVM stats: ioctl failed");
> + goto done;
> + }
> +
> + stats_args->result.schema = stats_result;
> + stats_args->is_schema = true;
> +
> + /* Query VM */
> + stats_args->filter->target = STATS_TARGET_VM;
> + query_stats(stats_args, stats_fd);
> +
> + /* Query vCPU */
> + stats_args->filter->target = STATS_TARGET_VCPU;
> + run_on_cpu(first_cpu, query_stats_vcpu, RUN_ON_CPU_HOST_PTR(stats_args));
> +done:
> + g_free(stats_args->filter);
> + g_free(stats_args);
The label & g_free can be removed with g_autofree usage.
> +}
> +
> diff --git a/qapi/misc.json b/qapi/misc.json
> index 8d326464f0..39a7d172cb 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -574,7 +574,7 @@
> # Since: 7.0
> ##
> { 'enum': 'StatsProvider',
> - 'data': [ ] }
> + 'data': [ 'kvm' ] }
>
> ##
> # @StatsTarget:
> --
> 2.27.0
>
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] 17+ messages in thread* Re: [PATCH v3 3/3] kvm: Support for querying fd-based stats
2022-01-31 19:43 ` [PATCH v3 3/3] kvm: Support for querying fd-based stats Mark Kanda
2022-02-01 10:08 ` Daniel P. Berrangé
@ 2022-02-01 10:51 ` Paolo Bonzini
1 sibling, 0 replies; 17+ messages in thread
From: Paolo Bonzini @ 2022-02-01 10:51 UTC (permalink / raw)
To: Mark Kanda, qemu-devel; +Cc: berrange, armbru
On 1/31/22 20:43, Mark Kanda wrote:
>
> + 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);
How hard would it be to cache the descriptors? Ideally, a query-stats
command would do only one pread.
Paolo
^ permalink raw reply [flat|nested] 17+ messages in thread