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

[Paolo Bonzini]

On 5/5/22 15:28, Markus Armbruster wrote:
> 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?

Yes.  As long as KVM is the only source, it's basically an opaque 1:1 
mapping of what the kernel gives.

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

Also, it was designed from the beginning to be extensible to other 
_kernel_ subsystems as well; i.e. it's not virt-specific in any way.

There is one important point: theoretically, stats names are not part of 
the kernel API.  In practice, you know what the chief penguin thinks of 
breaking userspace and anyway I don't think any of the stats have ever 
been removed when they were in debugfs (which makes them even less of a 
stable API).

For a similar situation see https://lwn.net/Articles/737530/: kernel 
developers hate that tracepoints are part of the stable API, but in 
practice they are (and stats are much harder to break than tracepoints, 
if it's worth exposing them to userspace in the first place).

> 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?

I think so, potentially all of them can be moved.  Whether it is worth 
doing it is another story.

In addition, query-stats provides a home for TCG statistics that 
currently QMP exposes only via x- commands; they can be added without 
having to design the whole QAPI thing, and with a slightly less strong 
guarantee of stability.

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

I like that in principle, however it's not possible to make qom_path 
mandatory.  For example block devices would not have a QOM path.
> > 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 :)

Will document.

> > > > +# @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.

Ok, I'll make an example.

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

Hmm, it seemed obvious but I can add something to StatsSchemaValue.

Paolo