Re: [PATCH v3 1/3] qmp: Support for querying stats

[Daniel P . Berrangé]

On Mon, Jan 31, 2022 at 01:43:10PM -0600, Mark Kanda wrote:
> Introduce QMP support for querying stats. Provide a framework for adding new
> stats and support for the following commands:
> 
> - query-stats
> Returns a list of all stats per target type (only VM and VCPU for now), with
> additional options for specifying stat names, VCPU qom paths, and stat 
> provider.
> 
> - query-stats-schemas
> Returns a list of stats included in each schema type, with an option for
> specifying the stat provider.
> 
> The framework provides a method to register callbacks for these QMP commands.
> 
> The first usecase will be for fd-based KVM stats (in an upcoming patch).
> 
> Examples (with fd-based KVM stats):
> 
> - Display all VM stats:
> 
> { "execute": "query-stats", "arguments" : { "target": "vm" } }
> { "return": {
>     "list": [
>       { "provider": "kvm",
>         "stats": [
>           { "name": "max_mmu_page_hash_collisions", "value": 0 },
>           { "name": "max_mmu_rmap_size", "value": 0 },
>           { "name": "nx_lpage_splits", "value": 131 },
>          ...
>         ] }
>       { "provider": "provider XYZ",
>       ...
>     ],
>     "target": "vm"
>   }
> }

I still feel like this is rather verbose, and should be simplified
down to.

 { "return": {
     "vm": {
       "kvm": [ ... ]
       "provider-XYZ": [ ... ],
       ...
     }
 }


While vCPU would need one extra nesting layer

 { "return": {
     "vcpus": [
       {
         "path": "/vcpu0/path"
         "kvm": [ ... ]
         "provider-XYZ": [ ... ],
         ...
       },
       {
         "path": "/vcpu1/path"
         "kvm": [ ... ]
         "provider-XYZ": [ ... ],
         ...
       },
       ...
     ],
 }


The notable difference here is that we'd be adding new
keys to the StatsResultEntry struct, every time we gain
a new provider, so your QMP code couldn't be entirely
metadata driven - you'd need new code to wire up each
stats provider. 


> - Display 'exits' and 'l1d_flush' KVM stats for VCPUs at 
> '/machine/unattached/device[2]'
> and '/machine/unattached/device[4]':

Shows the value of giving CPUs proper paths

  https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg06744.html

> 
> { "execute": "query-stats",
>   "arguments" : { "target": "vcpu",
>                   "fields": [ "exits", "l1d_flush" ],
>                 "paths": [ "/machine/unattached/device[2]",
>                             "/machine/unattached/device[4]" ]
>                   "provider": "kvm" } }

This design requires multiple query-stats calls to get data from
multiple providers which I think is very undesirable from a
performance POV.

I'd like to see us able to query fields from many providers at
once

ie so we have something that looks like
 
 { "execute": "query-stats",
   "arguments" : { "target": "vcpu",
                   "vcpus": [ "/machine/unattached/device[2]",
                              "/machine/unattached/device[4]" ]
                   "kvm": [ "exits", "l1d_flush" ],
                   "someprovider: [ "somefield" ] } }


> 
> { "return": {
>     "list": [
>       { "list": [
>           { "provider": "kvm",
>             "stats": [
>               { "name": "l1d_flush", "value": 14690 },
>               { "name": "exits", "value": 50898 }
>             ] }
>         ],
>         "path": "/machine/unattached/device[2]"
>       },
>       { "list": [
>           { "provider": "kvm",
>             "stats": [
>               { "name": "l1d_flush", "value": 24902 },
>               { "name": "exits", "value": 74374 }
>             ] }
>        ],
>         "path": "/machine/unattached/device[4]"
>       }
>     ],
>     "target": "vcpu"
>   }
> }
> 
> - Query stats schemas:
> 
> { "execute": "query-stats-schemas" }
> { "return": {
>     "vcpu": [
>       { "provider": "kvm",
>         "stats": [
>            { "name": "guest_mode",
>              "unit": "none",
>              "base": 10,
>              "exponent": 0,
>              "type": "instant" },
>          { "name": "directed_yield_successful",
>              "unit": "none",
>              "base": 10,
>              "exponent": 0,
>              "type": "cumulative" },
>              ...
>       "provider": "provider XYZ",
> ...
>    "vm": [
>       { "provider": "kvm",
>         "stats": [
>            { "name": "max_mmu_page_hash_collisions",
>              "unit": "none",
>              "base": 10,
>              "exponent": 0,
>              "type": "peak" },
>       "provider": "provider XYZ",
> ...
> 
> Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
> ---
>  include/monitor/stats.h |  36 ++++++
>  monitor/qmp-cmds.c      | 183 +++++++++++++++++++++++++++++
>  qapi/misc.json          | 253 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 472 insertions(+)
>  create mode 100644 include/monitor/stats.h
> 

> diff --git a/qapi/misc.json b/qapi/misc.json
> index e8054f415b..8d326464f0 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json

I'd suggest we introduce a 'stats.json' file just for this. We have
quite a few data types introduced, and its good to avoid 'misc.json'
becoming a dumping ground for ranom unrelated stuff.

> @@ -527,3 +527,256 @@
>   'data': { '*option': 'str' },
>   'returns': ['CommandLineOptionInfo'],
>   'allow-preconfig': true }
> +
> +##
> +# @StatType:

There's inconsistency with naming through these changes. Can we
ensure that everything uses 'Stats' (plural) as the prefix for
every type.

> +#
> +# Enumeration of stat types
> +# @cumulative: stat is cumulative; value can only increase.
> +# @instant: stat is instantaneous; value can increase or decrease.
> +# @peak: stat is the peak value; value can only increase.

Not documenting all members

> +#
> +# Since: 7.0
> +##
> +{ 'enum' : 'StatType',
> +  'data' : [ 'cumulative', 'instant', 'peak',
> +             'linear-hist', 'log-hist', 'unknown' ] }

IMHO 'unknown' shouldn't exist at all.

> +##
> +# @StatsVCPURequest:
> +#
> +# vcpu specific filter element.
> +# @paths: list of qom paths.
> +#
> +# Since: 7.0
> +##
> +{ 'struct': 'StatsVCPURequest',
> +  'base': 'StatsRequest',
> +  'data': { '*paths': [ 'str' ] } }

Call the field 'vcpus' instead of 'paths' to make it
clear that we're listing VCPU paths here.

> +##
> +# @StatsRequest:
> +#
> +# Stats filter element.
> +# @provider: stat provider.
> +# @fields: list of stat names.
> +#
> +# Since: 7.0
> +##
> +{ 'struct': 'StatsRequest',
> +  'data': { '*provider': 'StatsProvider',
> +            '*fields': [ 'str' ] } }

As mentioned earlier I think we need to have aility to query from
many providers at once. It'd be better to have provider name as
the field name, eg

 { 'struct': 'StatsRequest',
   'data': { '*kvm': ['str'],
             '*someprovider': [ 'str' ] } }

> +
> +##
> +# @StatsFilter:
> +#
> +# Target specific filter.
> +#
> +# Since: 7.0
> +##
> +{ 'union': 'StatsFilter',
> +  'base': { 'target': 'StatsTarget' },
> +  'discriminator': 'target',
> +  'data': { 'vcpu': 'StatsVCPURequest',
> +            'vm': 'StatsRequest' } }

> +##
> +# @StatsValueArray:
> +#
> +# uint64 list for StatsValue.
> +#
> +# Since: 7.0
> +##
> +{ 'struct': 'StatsValueArray',
> +  'data': { 'list' : [ 'uint64' ] } }

'values' or 'elements' rather than repeating 'list'

> +
> +##
> +# @StatsValue:
> +#
> +# @scalar: stat is single uint64.
> +# @list: stat is a list of uint64.
> +#
> +# Since: 7.0
> +##
> +{ 'alternate': 'StatsValue',
> +  'data': { 'scalar': 'uint64',
> +            'list': 'StatsValueArray' } }
> +
> +##
> +# @Stats:
> +#
> +# @name: name of stat.
> +# @value: stat value.
> +#
> +# Since: 7.0
> +##
> +{ 'struct': 'Stats',
> +  'data': { 'name': 'str',
> +            'value' : 'StatsValue' } }
> +
> +##
> +# @StatsResultsEntry:
> +#
> +# @provider: stat provider.
> +# @stats: list of stats.
> +#
> +# Since: 7.0
> +##
> +{ 'struct': 'StatsResultsEntry',
> +  'data': { 'provider': 'StatsProvider',
> +            'stats': [ 'Stats' ] } }
> +
> +##
> +# @VCPUResultsEntry:
> +#
> +# @path: qom path.
> +# @list: per provider @StatsResultEntry list.
> +#
> +# Since: 7.0
> +##
> +{ 'struct': 'VCPUResultsEntry',
> +  'data': { 'path': 'str',
> +            'list': [ 'StatsResultsEntry' ] } }
> +
> +##
> +# @VMStatsResults:
> +#
> +# Since: 7.0
> +##
> +{ 'struct': 'VMStatsResults',
> +  'data': { 'list' : [ 'StatsResultsEntry' ] } }
> +
> +##
> +# @VCPUStatsResults:
> +#
> +# Since: 7.0
> +##
> +{ 'struct': 'VCPUStatsResults',
> +  'data': { 'list': [ 'VCPUResultsEntry' ] } }
> +
> +##
> +# @StatsResults:
> +#
> +# Target specific results.
> +#
> +# Since: 7.0
> +##
> +{ 'union': 'StatsResults',
> +  'base': { 'target': 'StatsTarget' },
> +  'discriminator': 'target',
> +  'data': { 'vcpu': 'VCPUStatsResults',
> +            'vm': 'VMStatsResults' } }

I feel we can simplify this all down somewhat, eliminating levels
of redundant nesting

{ 'struct': 'StatsResultsEntry',
  'data': { '*kvm': [ 'Stats' ] } }

{ 'struct': 'StatsResultsVCPUEntry',
  'base': 'StatsResultsEntry',
  'data': 'path': 'str' } }

{ 'struct': 'StatsResults',
  'data': {
      '*vcpus': ['StatsResultsVCPUEntry'],
      '*vm': 'StatsResultsEntry'
  }
}


> +
> +##
> +# @query-stats:
> +#
> +# data: @StatsFilter
> +# Returns: @StatsResults
> +#
> +# Since: 7.0
> +##
> +{ 'command': 'query-stats',
> +  'data': 'StatsFilter',
> +  'boxed': true,
> +  'returns': 'StatsResults' }

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|