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

[Mark Kanda <mark.kanda@oracle.com>]

[-- Attachment #1: Type: text/plain, Size: 5473 bytes --]

On 3/21/2022 9:55 AM, Paolo Bonzini wrote:
> 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.
>

Hi Paolo,

FWIW, the attached patch adjusts some tests for alternates with arrays..

Thanks/regards,
-Mark

[-- Attachment #2: 0001-qapi-Add-support-for-alternates-with-arrays.patch --]
[-- Type: text/plain, Size: 4993 bytes --]

From 8d02b1b3cbb0dcc08875e307199d06c3995b3cf2 Mon Sep 17 00:00:00 2001
From: Mark Kanda <mark.kanda@oracle.com>
Date: Tue, 15 Mar 2022 20:42:05 -0500
Subject: [PATCH] qapi: Add support for alternates with arrays

Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
---
 scripts/qapi/expr.py                    | 2 +-
 scripts/qapi/schema.py                  | 6 +++++-
 tests/qapi-schema/alternate-array.err   | 2 --
 tests/qapi-schema/alternate-array.json  | 7 -------
 tests/qapi-schema/alternate-array.out   | 0
 tests/qapi-schema/meson.build           | 1 -
 tests/qapi-schema/qapi-schema-test.json | 6 ++++++
 tests/qapi-schema/qapi-schema-test.out  | 8 ++++++++
 8 files changed, 20 insertions(+), 12 deletions(-)
 delete mode 100644 tests/qapi-schema/alternate-array.err
 delete mode 100644 tests/qapi-schema/alternate-array.json
 delete mode 100644 tests/qapi-schema/alternate-array.out

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..7eedfa6cc2 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -243,7 +243,8 @@ def alternate_qtype(self):
             'number':  'QTYPE_QNUM',
             'int':     'QTYPE_QNUM',
             'boolean': 'QTYPE_QBOOL',
-            'object':  'QTYPE_QDICT'
+            'object':  'QTYPE_QDICT',
+            'array':   'QTYPE_QLIST'
         }
         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):
diff --git a/tests/qapi-schema/alternate-array.err b/tests/qapi-schema/alternate-array.err
deleted file mode 100644
index b1aa1f4e8d..0000000000
--- a/tests/qapi-schema/alternate-array.err
+++ /dev/null
@@ -1,2 +0,0 @@
-alternate-array.json: In alternate 'Alt':
-alternate-array.json:5: 'data' member 'two' cannot be an array
diff --git a/tests/qapi-schema/alternate-array.json b/tests/qapi-schema/alternate-array.json
deleted file mode 100644
index f241aac122..0000000000
--- a/tests/qapi-schema/alternate-array.json
+++ /dev/null
@@ -1,7 +0,0 @@
-# we do not allow array branches in alternates
-# TODO: should we support this?
-{ 'struct': 'One',
-  'data': { 'name': 'str' } }
-{ 'alternate': 'Alt',
-  'data': { 'one': 'One',
-            'two': [ 'int' ] } }
diff --git a/tests/qapi-schema/alternate-array.out b/tests/qapi-schema/alternate-array.out
deleted file mode 100644
index e69de29bb2..0000000000
diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build
index caf0791ba8..3dbd5f8bfb 100644
--- a/tests/qapi-schema/meson.build
+++ b/tests/qapi-schema/meson.build
@@ -4,7 +4,6 @@ test_env.set('PYTHONIOENCODING', 'utf-8')
 
 schemas = [
   'alternate-any.json',
-  'alternate-array.json',
   'alternate-base.json',
   'alternate-branch-if-invalid.json',
   'alternate-clash.json',
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 43b8697002..64dcbbbe2a 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -123,6 +123,12 @@
 # for testing use of 'str' within alternates
 { 'alternate': 'AltStrObj', 'data': { 's': 'str', 'o': 'TestStruct' } }
 
+# for testing use of an 'int' array within alternates
+{ 'alternate': 'AltIntArray', 'data': { 'a': [ 'int' ], 'o': 'TestStruct' } }
+
+# for testing use of an 'TestStruct' array within alternates
+{ 'alternate': 'AltStructArray', 'data': { 'a': [ 'TestStruct' ], 'o': 'TestStruct' } }
+
 { 'struct': 'ArrayStruct',
   'data': { 'integer': ['int'],
             's8': ['int8'],
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 1f9585fa9b..b899c30158 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -125,6 +125,14 @@ alternate AltStrObj
     tag type
     case s: str
     case o: TestStruct
+alternate AltIntArray
+    tag type
+    case a: intList
+    case o: TestStruct
+alternate AltStructArray
+    tag type
+    case a: TestStructList
+    case o: TestStruct
 object ArrayStruct
     member integer: intList optional=False
     member s8: int8List optional=False
-- 
2.27.0