| Message ID | 20211119195153.11815-2-mark.kanda@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | Support fd-based KVM stats | expand |
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
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' }
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' }
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
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
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
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
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
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
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
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' ] }
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(+)