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

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 4 maggio 2022 15:22:27 CEST, Markus Armbruster <armbru@redhat.com> ha scritto:
>>Can you point to existing uses of KVM binary stats introspection data?
>
> There's none, but Google is using it in house. The same data was available before in debugfs and available via the kvm_stat script, so you could also refer to Christian Borntraeger's KVM Forum 2019 talk. The problems with debugfs are basically that it's only available to root and is disabled by secure boot (both issues are not fixable on general because they are Linux policy).

I keep bothering you about use cases, because I'm habitually opposed to
adding features without credible use cases.

For small features, a bit of plausible hand-waving can suffice, but this
one isn't small enough for that.

Plausible hand-waving can sometimes suffice for *experimental* features.
Say when the use case can't really materialize without the feature.

Double-checking (pardon my ignorance): we're basically exposing the host
kernel's KVM stats via QMP, with the option of extending it to other
sources of stats in the future.  Correct?

I think the argument for accepting the interface is basically "if it's
good enough for the kernel, it's good enough for us".  Valid point.

This means we'll acquire yet another introspection system, unrelated to
the introspection systems we already have in QEMU.

There is overlap.  Quite a few query- commands return stats.  Should
they be redone as statistics provides in this new introspection system?

Assuming the answer is no for at least some of them: what kind of stats
should go where?

I'd love to have a better feel for future directions before comitting to
this.

>>> index 4912b9744e..92d7ecc52c 100644
>>> --- a/qapi/qapi-schema.json
>>> +++ b/qapi/qapi-schema.json
>>> @@ -93,3 +93,4 @@
>>>  { 'include': 'audio.json' }
>>>  { 'include': 'acpi.json' }
>>>  { 'include': 'pci.json' }
>>> +{ 'include': 'stats.json' }
>>> diff --git a/qapi/stats.json b/qapi/stats.json
>>> new file mode 100644
>>> index 0000000000..7454dd7daa
>>> --- /dev/null
>>> +++ b/qapi/stats.json
>>> @@ -0,0 +1,192 @@
>>> +# -*- Mode: Python -*-
>>> +# vim: filetype=python
>>> +#
>>> +# Copyright (c) 2022 Oracle and/or its affiliates.
>>> +#
>>> +# This work is licensed under the terms of the GNU GPL, version 2 or later.
>>> +# See the COPYING file in the top-level directory.
>>> +#
>>> +# SPDX-License-Identifier: GPL-2.0-or-later
>>> +
>>> +##
>>> +# = Statistics
>>> +##
>>> +
>>> +##
>>> +# @StatsType:
>>> +#
>>> +# Enumeration of statistics 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.
>>> +# @linear-hist: stat is a linear histogram.
>>> +# @log-hist: stat is a logarithmic histogram.
>>
>>For better or worse, we tend to eschew abbreviations in schema
>>identifiers.  Would you mind @linear-histogram and @log-histogram?
>
> Sure.
>
>
>>> +# Since: 7.1
>>> +##
>>> +{ 'enum': 'StatsTarget',
>>> +  'data': [ 'vm', 'vcpu' ] }
>>
>>Do VM stats include vCPU stats?  "Entire virtual machine" suggests they
>>do...
>
> No, they don't. They are a different sets of data that is gathered on resources shared by the whole VM. Stuff such as "# of pages taken by the KVM page tables" goes there because VCPUs share a single copy of the page tables, as opposed to "# of page faults" which is a VCPU stat.

I'm fine with whatever partition you think is useful, I'm just pointing
out that to me the documentation suggests something that ain't :)

>>> +# The arguments to the query-stats command; specifies a target for which to
>>> +# request statistics, and which statistics are requested from each provider.
>>> +#
>>> +# Since: 7.1
>>> +##
>>> +{ 'struct': 'StatsFilter',
>>> +  'data': { 'target': 'StatsTarget' } }
>>
>>The "and which statistics" part will be implemented later in this
>>series?
>
> Oh, indeed it is. Thanks for noticing.
>
>>> +{ 'struct': 'StatsResult',
>>> +  'data': { 'provider': 'StatsProvider',
>>> +            '*qom-path': 'str',
>>
>>When exactly will @qom-path be present?
>
> Only if the target is vcpus, for the current set of targets. Because the target is in the command I am not repeating it here with another discriminated record.

Needs to be documented then.

Alternatively, maybe: the "QOM path of the object for which the
statistics are returned" could be "/" or "/machine" when the object is
the VM.

>>> +# @type: kind of statistic, a @StatType.
>>
>>Generated documentation looks like
>>
>>       type: StatsType
>>              kind of statistic, a StatType.
>>
>>I think ", a @StatType" should be dropped.
>>
>>If we decide to keep it: @StatsType.
>
> Gotcha.
>
>>
>>> +#
>>> +# @unit: base unit of measurement for the statistics @StatUnit.
>>
>>"@StatUnit", too.
>>
>>If we decide to keep it: @StatsUnit.
>>
>>@unit is optional.  What's the default?
>
> The stat is an adimensional number: a count of events such a page faults, or the maximum length of a bucket in a hash table,  etc. It's actually the common case.

I've come to prefer defaulting to a value over giving "absent" its own
meaning.  However, own meaning is somewhat entrenched in the schema
language and its usage, and "absent @unit means adimensional" is kind of
fitting, so I'm not objecting.  I am asking for better documentation,
though :)

>>> +# @base: base for the multiple of @unit that the statistic uses, either 2 or 10.
>>> +#        Only present if @exponent is non-zero.
>>> +#
>>> +# @exponent: exponent for the multiple of @unit that the statistic uses
>>
>>Alright, given a stat value 42, what does it mean for the possible
>>combinations of @base and @exponent?
>
> Base and exponent are used to represent units like KiB, nanoseconds, etc.

Put that in doc comments, please.

>>> +# @bucket-size: Used with linear-hist to report the width of each bucket
>>> +#               of the histogram.
>>
>>Feels too terse.  Example, perhaps?
>>
>>I assume @bucket-size is present exactly when @type is @linear-hist.
>>Correct?
>
> Yep, will expand.
>
>>> +##
>>> +# @StatsSchema:
>>> +#
>>> +# Schema for all available statistics for a provider and target.
>>> +#
>>> +# @provider: provider for this set of statistics.
>>> +#
>>> +# @target: kind of object that can be queried through this provider.
>>> +#
>>> +# @stats: list of statistics.
>>> +#
>>> +# Since: 7.1
>>> +##
>>> +{ 'struct': 'StatsSchema',
>>> +  'data': { 'provider': 'StatsProvider',
>>> +            'target': 'StatsTarget',
>>> +            'stats': [ 'StatsSchemaValue' ] } }
>>
>>How am I to connect each element of the result of query-stats to an
>>element of the result of query-stats-schema?
>
> You gave the target to query-stats and the result of query-stats has the provider and name. Target+provider+name uniquely identify a StatsSchemaValue in the result of query-stats-schemas.

Can we have that spelled out in documentation?

Doc comments or something under docs/, up to you.

>
> Paolo
>
>>
>>> +
>>> +##
>>> +# @query-stats-schemas:
>>> +#
>>> +# Return the schema for all available runtime-collected statistics.
>>> +#
>>> +# Since: 7.1
>>> +##
>>> +{ 'command': 'query-stats-schemas',
>>> +  'data': { },
>>> +  'returns': [ 'StatsSchema' ] }
>>