* [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
* 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 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 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-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-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
* [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 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
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.