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

[Paolo Bonzini]

On 3/21/22 14:50, Markus Armbruster wrote:
> Mark Kanda <mark.kanda@oracle.com> writes:
> > Thank you Markus.
> > On 3/11/2022 7:06 AM, Markus Armbruster wrote:
> > > Are the stats bulky enough to justfify the extra complexity of
> > > filtering?
> >
> > If this was only for KVM, the complexity probably isn't worth it. However, the
> > framework is intended to support future stats with new providers and targets
> > (there has also been mention of moving existing stats to this framework).
> > Without some sort of filtering, I think the payload could become unmanageable.
>
> I'm deeply wary of "may need $complexity in the future" when $complexity
> could be added when we actually need it :)

I think it's better to have the filtering already.  There are several 
uses for it.

Regarding filtering by provider, consider that a command like "info jit" 
should be a wrapper over

{ "execute": "query-stats", "arguments" : { "target": "vm",
  "filters": [ { "provider": "tcg" } ] } }

So we have an example of the intended use already within QEMU.  Yes, the 
usefulness depends on actually having >1 provider but I think it's 
pretty central to the idea of having a statistics *subsystem*.

Regarding filtering by name, query-stats mostly has two usecases.  The 
first is retrieving all stats and publishing them up to the user, for 
example once per minute per VM.  The second is monitoring a small number 
and building a relatively continuous plot (e.g. 1-10 times per second 
per vCPU).  For the latter, not having to return hundreds of values 
unnecessarily (KVM has almost 60 stats, multiply by the number of vCPUs 
and the frequency) is worth having even just with the KVM provider.

> > > Can you give a use case for query-stats-schemas?
> >
> > 'query-stats-schemas' provide the the type details about each stat; such as the
> > unit, base, etc. These details are not reported by 'query-stats' (only the stat
> > name and raw values are returned).
>
> Yes, but what is going to use these type details, and for what purpose?

QEMU does not know in advance which stats are provided.  The types, etc. 
are provided by the kernel and can change by architecture and kernel 
version.  In the case of KVM, introspection is done through a file 
descriptor.  QEMU passes these up as QMP and in the future it 
could/should extend this to other providers (such as TCG) and devices 
(such as block devices).

See the "info stats" implementation for how it uses the schema:

vcpu (qom path: /machine/unattached/device[2])
  provider: kvm
    exits (cumulative): 52369
    halt_wait_ns (cumulative nanoseconds): 416092704390

Information such as "cumulative nanoseconds" is provided by the schema.

> Have you considered splitting this up into three parts: unfiltered
> query-stats, filtering, and query-stats-schemas?

Splitting could be an idea, but I think only filtering would be a 
separate step.  The stats are not really usable without a schema that 
tells you the units, or whether a number can go down or only up.  (Well, 
a human export could use them through its intuition, but a HMP-level 
command could not be provided).

> We could perhaps merge with the current schema, then clean it up on top,
> both in 7.1, if that's easier for you.

The serialized JSON would change, so that would be a bit worrisome (but 
it makes me feel a little less bad about this missing 7.0).  It seems to 
be as easy as this, as far as alternates go:

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 3cb389e875..48578e1698 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -554,7 +554,7 @@ def check_alternate(expr: _JSONObject, info: 
QAPISourceInfo) -> None:
         check_name_lower(key, info, source)
         check_keys(value, info, source, ['type'], ['if'])
         check_if(value, info, source)
-        check_type(value['type'], info, source)
+        check_type(value['type'], info, source, allow_array=True)


 def check_command(expr: _JSONObject, info: QAPISourceInfo) -> None:

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index b7b3fc0ce4..3728340c37 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -243,6 +243,7 @@ def alternate_qtype(self):
             'number':  'QTYPE_QNUM',
             'int':     'QTYPE_QNUM',
             'boolean': 'QTYPE_QBOOL',
+            'array':   'QTYPE_QLIST',
             'object':  'QTYPE_QDICT'
         }
         return json2qtype.get(self.json_type())
@@ -1069,6 +1070,9 @@ def _def_struct_type(self, expr, info, doc):
             None))

     def _make_variant(self, case, typ, ifcond, info):
+        if isinstance(typ, list):
+            assert len(typ) == 1
+            typ = self._make_array_type(typ[0], info)
         return QAPISchemaVariant(case, info, typ, ifcond)

     def _def_union_type(self, expr, info, doc):


I'll try to write some testcases and also cover other uses of
_make_variant, which will undoubtedly find some issue.

Paolo