[PATCH 0/8] qmp, hmp: statistics subsystem and KVM suport.

[PATCH 0/8] qmp, hmp: statistics subsystem and KVM suport.

[Paolo Bonzini]

This patchset adds QEMU support for querying fd-based KVM statistics.
This allows the user to analyze the behavior of the VM without access
to debugfs.

However, instead of adding an ad hoc command, the new QMP entry point
can be extended in the future to more statistics provider than KVM
(for example TCG, tap, or the block layer) and to more objects than
the VM and vCPUS (for example network interfaces or block devices).

Because the statistics exposed by KVM are not known at compile time,
the kernel interface also comes with an introspectable schema.  This
schema is exposed by the query-stats-schemas QMP command.

Patches 1 and 2 add the basic support, respectively the QMP command
and the KVM producer.

Patches 3 and 4 add a basic HMP implementation.  The first of the two
adds a basic filtering mechanism to the QMP command, which is then used
by HMP (which only shows vCPU statistics for the currently selected
guest CPU; this is consistent with other HMP commands and does not
flood the user with an overwhelming amount of output).

The remaining patches add more filtering, respectively by provider
and by the name of a statistic.

Compared to the previous version that Mark sent, the changes are
as follows:

* changed the QAPI schema so that vm, vcpus etc. are not keys of
  QAPI objects anymore.  This simplifies the interface exposed
  to stats callbacks as well.

* changed the QAPI schema to use optional fields a bit more (e.g.
  avoiding unit == 'none', or omitting base if exponent is 0).

* reorganized the patches to introduce filtering separately.  This
  also resulted in some changes to the statistics callbacks that
  the producers have to define.

* removed "info stats-schemas" HMP command.  The information in
  query-stats-schemas is already printed by "info stats".

Paolo

Supersedes: <20220215150433.2310711-1-mark.kanda@oracle.com>

Mark Kanda (3):
  qmp: Support for querying stats
  kvm: Support for querying fd-based stats
  hmp: add basic "info stats" implementation

Paolo Bonzini (5):
  qmp: add filtering of statistics by target vCPU
  qmp: add filtering of statistics by provider
  hmp: add filtering of statistics by provider
  qmp: add filtering of statistics by name
  hmp: add filtering of statistics by name

 accel/kvm/kvm-all.c     | 413 ++++++++++++++++++++++++++++++++++++++++
 hmp-commands-info.hx    |  14 ++
 include/monitor/hmp.h   |   1 +
 include/monitor/stats.h |  42 ++++
 monitor/hmp-cmds.c      | 232 ++++++++++++++++++++++
 monitor/qmp-cmds.c      | 132 +++++++++++++
 qapi/meson.build        |   1 +
 qapi/qapi-schema.json   |   1 +
 qapi/stats.json         | 218 +++++++++++++++++++++
 9 files changed, 1054 insertions(+)
 create mode 100644 include/monitor/stats.h
 create mode 100644 qapi/stats.json

-- 
2.35.1

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

[Paolo Bonzini]

From: Mark Kanda <mark.kanda@oracle.com>

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 to start), with
additional options for specifying stat names, vCPU qom paths, and providers.

- query-stats-schemas
Returns a list of stats included in each target type, with an option for
specifying the provider.  The concepts in the schema are based on the
KVM binary stats' own introspection data, just translated to QAPI.

The framework provides a method to register callbacks for these QMP commands.
Most of the work in fact is done by the callbacks, and a large majority of
this patch is new QAPI structs and commands.

The first use-case will be for fd-based KVM stats (in an upcoming patch).

Examples (with fd-based KVM stats):

- Query all VM stats:

{ "execute": "query-stats", "arguments" : { "target": "vm" } }

{ "return": [
     { "provider": "kvm",
       "stats": [
          { "name": "max_mmu_page_hash_collisions", "value": 0 },
          { "name": "max_mmu_rmap_size", "value": 0 },
          { "name": "nx_lpage_splits", "value": 148 },
          ... ] },
     { "provider": "xyz",
       "stats": [ ... ] }
] }

- Query all vCPU stats:

{ "execute": "query-stats", "arguments" : { "target": "vcpu" } }

{ "return": [
     { "provider": "kvm",
       "qom_path": "/machine/unattached/device[0]"
       "stats": [
          { "name": "guest_mode", "value": 0 },
          { "name": "directed_yield_successful", "value": 0 },
          { "name": "directed_yield_attempted", "value": 106 },
          ... ] },
     { "provider": "kvm",
       "qom_path": "/machine/unattached/device[1]"
       "stats": [
          { "name": "guest_mode", "value": 0 },
          { "name": "directed_yield_successful", "value": 0 },
          { "name": "directed_yield_attempted", "value": 106 },
          ... ] },
] }

- Retrieve the schemas:

{ "execute": "query-stats-schemas" }

{ "return": [
    { "provider": "kvm",
      "target": "vcpu",
      "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": "kvm",
      "target": "vm",
      "stats": [
        { "name": "max_mmu_page_hash_collisions",
           "unit": "none",
           "base": 10,
           "exponent": 0,
           "type": "peak" },
        ... ]
    },
    { "provider": "xyz",
      "target": "vm",
      "stats": [ ... ]
    }
] }

Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/monitor/stats.h |  33 +++++++
 monitor/qmp-cmds.c      |  71 +++++++++++++++
 qapi/meson.build        |   1 +
 qapi/qapi-schema.json   |   1 +
 qapi/stats.json         | 192 ++++++++++++++++++++++++++++++++++++++++
 5 files changed, 298 insertions(+)
 create mode 100644 include/monitor/stats.h
 create mode 100644 qapi/stats.json

diff --git a/include/monitor/stats.h b/include/monitor/stats.h
new file mode 100644
index 0000000000..89552ab06f
--- /dev/null
+++ b/include/monitor/stats.h
@@ -0,0 +1,33 @@
+/*
+ * Copyright (c) 2022 Oracle and/or its affiliates.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef STATS_H
+#define STATS_H
+
+#include "qapi/qapi-types-stats.h"
+
+typedef void StatRetrieveFunc(StatsResultList **result, StatsTarget target, Error **errp);
+typedef void SchemaRetrieveFunc(StatsSchemaList **result, Error **errp);
+
+/*
+ * Register callbacks for the QMP query-stats command.
+ *
+ * @stats_fn: routine to query stats:
+ * @schema_fn: routine to query stat schemas:
+ */
+void add_stats_callbacks(StatRetrieveFunc *stats_fn,
+                         SchemaRetrieveFunc *schemas_fn);
+
+/*
+ * Helper routines for adding stats entries to the results lists.
+ */
+void add_stats_entry(StatsResultList **, StatsProvider, const char *id,
+                     StatsList *stats_list);
+void add_stats_schema(StatsSchemaList **, StatsProvider, StatsTarget,
+                      StatsSchemaValueList *);
+
+#endif /* STATS_H */
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 5e7302cbb9..97825b25fa 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -35,6 +35,7 @@
 #include "qapi/qapi-commands-control.h"
 #include "qapi/qapi-commands-machine.h"
 #include "qapi/qapi-commands-misc.h"
+#include "qapi/qapi-commands-stats.h"
 #include "qapi/qapi-commands-ui.h"
 #include "qapi/type-helpers.h"
 #include "qapi/qmp/qerror.h"
@@ -43,6 +44,7 @@
 #include "hw/acpi/acpi_dev_interface.h"
 #include "hw/intc/intc.h"
 #include "hw/rdma/rdma.h"
+#include "monitor/stats.h"
 
 NameInfo *qmp_query_name(Error **errp)
 {
@@ -426,3 +428,72 @@ HumanReadableText *qmp_x_query_irq(Error **errp)
 
     return human_readable_text_from_str(buf);
 }
+
+typedef struct StatsCallbacks {
+    StatRetrieveFunc *stats_cb;
+    SchemaRetrieveFunc *schemas_cb;
+    QTAILQ_ENTRY(StatsCallbacks) next;
+} StatsCallbacks;
+
+static QTAILQ_HEAD(, StatsCallbacks) stats_callbacks =
+    QTAILQ_HEAD_INITIALIZER(stats_callbacks);
+
+void add_stats_callbacks(StatRetrieveFunc *stats_fn,
+                         SchemaRetrieveFunc *schemas_fn)
+{
+    StatsCallbacks *entry = g_new(StatsCallbacks, 1);
+    entry->stats_cb = stats_fn;
+    entry->schemas_cb = schemas_fn;
+
+    QTAILQ_INSERT_TAIL(&stats_callbacks, entry, next);
+}
+
+StatsResultList *qmp_query_stats(StatsFilter *filter, Error **errp)
+{
+    StatsResultList *stats_results = NULL;
+    StatsCallbacks *entry;
+
+    QTAILQ_FOREACH(entry, &stats_callbacks, next) {
+        entry->stats_cb(&stats_results, filter->target, errp);
+    }
+
+    return stats_results;
+}
+
+StatsSchemaList *qmp_query_stats_schemas(Error **errp)
+{
+    StatsSchemaList *stats_results = NULL;
+    StatsCallbacks *entry;
+
+    QTAILQ_FOREACH(entry, &stats_callbacks, next) {
+        entry->schemas_cb(&stats_results, errp);
+    }
+
+    return stats_results;
+}
+
+void add_stats_entry(StatsResultList **stats_results, StatsProvider provider,
+                     const char *qom_path, StatsList *stats_list)
+{
+    StatsResult *entry = g_new0(StatsResult, 1);
+    entry->provider = provider;
+    if (qom_path) {
+        entry->has_qom_path = true;
+        entry->qom_path = g_strdup(qom_path);
+    }
+    entry->stats = stats_list;
+
+    QAPI_LIST_PREPEND(*stats_results, entry);
+}
+
+void add_stats_schema(StatsSchemaList **schema_results,
+                      StatsProvider provider, StatsTarget target,
+                      StatsSchemaValueList *stats_list)
+{
+    StatsSchema *entry = g_new0(StatsSchema, 1);
+
+    entry->provider = provider;
+    entry->target = target;
+    entry->stats = stats_list;
+    QAPI_LIST_PREPEND(*schema_results, entry);
+}
diff --git a/qapi/meson.build b/qapi/meson.build
index 656ef0e039..fd5c93d643 100644
--- a/qapi/meson.build
+++ b/qapi/meson.build
@@ -46,6 +46,7 @@ qapi_all_modules = [
   'replay',
   'run-state',
   'sockets',
+  'stats',
   'trace',
   'transaction',
   'yank',
diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
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.
+#
+# Since: 7.1
+##
+{ 'enum' : 'StatsType',
+  'data' : [ 'cumulative', 'instant', 'peak', 'linear-hist', 'log-hist' ] }
+
+##
+# @StatsUnit:
+#
+# Enumeration of unit of measurement for statistics
+#
+# @bytes: stat reported in bytes.
+# @seconds: stat reported in seconds.
+# @cycles: stat reported in clock cycles.
+#
+# Since: 7.1
+##
+{ 'enum' : 'StatsUnit',
+  'data' : [ 'bytes', 'seconds', 'cycles' ] }
+
+##
+# @StatsProvider:
+#
+# Enumeration of statistics providers.
+#
+# Since: 7.1
+##
+{ 'enum': 'StatsProvider',
+  'data': [ ] }
+
+##
+# @StatsTarget:
+#
+# The kinds of objects on which one can request statistics.
+#
+# @vm: the entire virtual machine.
+# @vcpu: a virtual CPU.
+#
+# Since: 7.1
+##
+{ 'enum': 'StatsTarget',
+  'data': [ 'vm', 'vcpu' ] }
+
+##
+# @StatsFilter:
+#
+# 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' } }
+
+##
+# @StatsValue:
+#
+# @scalar: single uint64.
+# @list: list of uint64.
+#
+# Since: 7.1
+##
+{ 'alternate': 'StatsValue',
+  'data': { 'scalar': 'uint64',
+            'list': [ 'uint64' ] } }
+
+##
+# @Stats:
+#
+# @name: name of stat.
+# @value: stat value.
+#
+# Since: 7.1
+##
+{ 'struct': 'Stats',
+  'data': { 'name': 'str',
+            'value' : 'StatsValue' } }
+
+##
+# @StatsResult:
+#
+# @provider: provider for this set of statistics.
+# @qom-path: QOM path of the object for which the statistics are returned
+# @stats: list of statistics.
+#
+# Since: 7.1
+##
+{ 'struct': 'StatsResult',
+  'data': { 'provider': 'StatsProvider',
+            '*qom-path': 'str',
+            'stats': [ 'Stats' ] } }
+
+##
+# @query-stats:
+#
+# Return runtime-collected statistics for objects such as the
+# VM or its vCPUs.
+#
+# The arguments are a StatsFilter and specify the provider and objects
+# to return statistics about.
+#
+# Returns: a list of StatsResult, one for each provider and object
+#          (e.g., for each vCPU).
+#
+# Since: 7.1
+##
+{ 'command': 'query-stats',
+  'data': 'StatsFilter',
+  'boxed': true,
+  'returns': [ 'StatsResult' ] }
+
+##
+# @StatsSchemaValue:
+#
+# Schema for a single statistic.
+#
+# @name: stat name.
+#
+# @type: kind of statistic, a @StatType.
+#
+# @unit: base unit of measurement for the statistics @StatUnit.
+#
+# @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
+#
+# @bucket-size: Used with linear-hist to report the width of each bucket
+#               of the histogram.
+#
+# Since: 7.1
+##
+{ 'struct': 'StatsSchemaValue',
+  'data': { 'name': 'str',
+            'type': 'StatsType',
+            '*unit': 'StatsUnit',
+            '*base': 'int8',
+            'exponent': 'int16',
+            '*bucket-size': 'uint32' } }
+
+##
+# @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' ] } }
+
+##
+# @query-stats-schemas:
+#
+# Return the schema for all available runtime-collected statistics.
+#
+# Since: 7.1
+##
+{ 'command': 'query-stats-schemas',
+  'data': { },
+  'returns': [ 'StatsSchema' ] }
-- 
2.35.1

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

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> From: Mark Kanda <mark.kanda@oracle.com>
> 
> 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 to start), with
> additional options for specifying stat names, vCPU qom paths, and providers.
> 
> - query-stats-schemas
> Returns a list of stats included in each target type, with an option for
> specifying the provider.  The concepts in the schema are based on the
> KVM binary stats' own introspection data, just translated to QAPI.
> 
> The framework provides a method to register callbacks for these QMP commands.
> Most of the work in fact is done by the callbacks, and a large majority of
> this patch is new QAPI structs and commands.
> 
> The first use-case will be for fd-based KVM stats (in an upcoming patch).
> 
> Examples (with fd-based KVM stats):
> 
> - Query all VM stats:
> 
> { "execute": "query-stats", "arguments" : { "target": "vm" } }
> 
> { "return": [
>      { "provider": "kvm",
>        "stats": [
>           { "name": "max_mmu_page_hash_collisions", "value": 0 },
>           { "name": "max_mmu_rmap_size", "value": 0 },
>           { "name": "nx_lpage_splits", "value": 148 },

Is there any hierarchy to the naming or is it just a big flat name
space?

>           ... ] },
>      { "provider": "xyz",
>        "stats": [ ... ] }
> ] }
> 
> - Query all vCPU stats:
> 
> { "execute": "query-stats", "arguments" : { "target": "vcpu" } }
> 
> { "return": [
>      { "provider": "kvm",
>        "qom_path": "/machine/unattached/device[0]"
>        "stats": [
>           { "name": "guest_mode", "value": 0 },
>           { "name": "directed_yield_successful", "value": 0 },
>           { "name": "directed_yield_attempted", "value": 106 },
>           ... ] },
>      { "provider": "kvm",
>        "qom_path": "/machine/unattached/device[1]"
>        "stats": [
>           { "name": "guest_mode", "value": 0 },
>           { "name": "directed_yield_successful", "value": 0 },
>           { "name": "directed_yield_attempted", "value": 106 },
>           ... ] },
> ] }
> 
> - Retrieve the schemas:
> 
> { "execute": "query-stats-schemas" }
> 
> { "return": [
>     { "provider": "kvm",
>       "target": "vcpu",
>       "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": "kvm",
>       "target": "vm",
>       "stats": [
>         { "name": "max_mmu_page_hash_collisions",
>            "unit": "none",
>            "base": 10,
>            "exponent": 0,
>            "type": "peak" },
>         ... ]
>     },

Is there some way to reset the peak or cumulative values?

Dave

>     { "provider": "xyz",
>       "target": "vm",
>       "stats": [ ... ]
>     }
> ] }
> 
> Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/monitor/stats.h |  33 +++++++
>  monitor/qmp-cmds.c      |  71 +++++++++++++++
>  qapi/meson.build        |   1 +
>  qapi/qapi-schema.json   |   1 +
>  qapi/stats.json         | 192 ++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 298 insertions(+)
>  create mode 100644 include/monitor/stats.h
>  create mode 100644 qapi/stats.json
> 
> diff --git a/include/monitor/stats.h b/include/monitor/stats.h
> new file mode 100644
> index 0000000000..89552ab06f
> --- /dev/null
> +++ b/include/monitor/stats.h
> @@ -0,0 +1,33 @@
> +/*
> + * Copyright (c) 2022 Oracle and/or its affiliates.
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef STATS_H
> +#define STATS_H
> +
> +#include "qapi/qapi-types-stats.h"
> +
> +typedef void StatRetrieveFunc(StatsResultList **result, StatsTarget target, Error **errp);
> +typedef void SchemaRetrieveFunc(StatsSchemaList **result, Error **errp);
> +
> +/*
> + * Register callbacks for the QMP query-stats command.
> + *
> + * @stats_fn: routine to query stats:
> + * @schema_fn: routine to query stat schemas:
> + */
> +void add_stats_callbacks(StatRetrieveFunc *stats_fn,
> +                         SchemaRetrieveFunc *schemas_fn);
> +
> +/*
> + * Helper routines for adding stats entries to the results lists.
> + */
> +void add_stats_entry(StatsResultList **, StatsProvider, const char *id,
> +                     StatsList *stats_list);
> +void add_stats_schema(StatsSchemaList **, StatsProvider, StatsTarget,
> +                      StatsSchemaValueList *);
> +
> +#endif /* STATS_H */
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 5e7302cbb9..97825b25fa 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -35,6 +35,7 @@
>  #include "qapi/qapi-commands-control.h"
>  #include "qapi/qapi-commands-machine.h"
>  #include "qapi/qapi-commands-misc.h"
> +#include "qapi/qapi-commands-stats.h"
>  #include "qapi/qapi-commands-ui.h"
>  #include "qapi/type-helpers.h"
>  #include "qapi/qmp/qerror.h"
> @@ -43,6 +44,7 @@
>  #include "hw/acpi/acpi_dev_interface.h"
>  #include "hw/intc/intc.h"
>  #include "hw/rdma/rdma.h"
> +#include "monitor/stats.h"
>  
>  NameInfo *qmp_query_name(Error **errp)
>  {
> @@ -426,3 +428,72 @@ HumanReadableText *qmp_x_query_irq(Error **errp)
>  
>      return human_readable_text_from_str(buf);
>  }
> +
> +typedef struct StatsCallbacks {
> +    StatRetrieveFunc *stats_cb;
> +    SchemaRetrieveFunc *schemas_cb;
> +    QTAILQ_ENTRY(StatsCallbacks) next;
> +} StatsCallbacks;
> +
> +static QTAILQ_HEAD(, StatsCallbacks) stats_callbacks =
> +    QTAILQ_HEAD_INITIALIZER(stats_callbacks);
> +
> +void add_stats_callbacks(StatRetrieveFunc *stats_fn,
> +                         SchemaRetrieveFunc *schemas_fn)
> +{
> +    StatsCallbacks *entry = g_new(StatsCallbacks, 1);
> +    entry->stats_cb = stats_fn;
> +    entry->schemas_cb = schemas_fn;
> +
> +    QTAILQ_INSERT_TAIL(&stats_callbacks, entry, next);
> +}
> +
> +StatsResultList *qmp_query_stats(StatsFilter *filter, Error **errp)
> +{
> +    StatsResultList *stats_results = NULL;
> +    StatsCallbacks *entry;
> +
> +    QTAILQ_FOREACH(entry, &stats_callbacks, next) {
> +        entry->stats_cb(&stats_results, filter->target, errp);
> +    }
> +
> +    return stats_results;
> +}
> +
> +StatsSchemaList *qmp_query_stats_schemas(Error **errp)
> +{
> +    StatsSchemaList *stats_results = NULL;
> +    StatsCallbacks *entry;
> +
> +    QTAILQ_FOREACH(entry, &stats_callbacks, next) {
> +        entry->schemas_cb(&stats_results, errp);
> +    }
> +
> +    return stats_results;
> +}
> +
> +void add_stats_entry(StatsResultList **stats_results, StatsProvider provider,
> +                     const char *qom_path, StatsList *stats_list)
> +{
> +    StatsResult *entry = g_new0(StatsResult, 1);
> +    entry->provider = provider;
> +    if (qom_path) {
> +        entry->has_qom_path = true;
> +        entry->qom_path = g_strdup(qom_path);
> +    }
> +    entry->stats = stats_list;
> +
> +    QAPI_LIST_PREPEND(*stats_results, entry);
> +}
> +
> +void add_stats_schema(StatsSchemaList **schema_results,
> +                      StatsProvider provider, StatsTarget target,
> +                      StatsSchemaValueList *stats_list)
> +{
> +    StatsSchema *entry = g_new0(StatsSchema, 1);
> +
> +    entry->provider = provider;
> +    entry->target = target;
> +    entry->stats = stats_list;
> +    QAPI_LIST_PREPEND(*schema_results, entry);
> +}
> diff --git a/qapi/meson.build b/qapi/meson.build
> index 656ef0e039..fd5c93d643 100644
> --- a/qapi/meson.build
> +++ b/qapi/meson.build
> @@ -46,6 +46,7 @@ qapi_all_modules = [
>    'replay',
>    'run-state',
>    'sockets',
> +  'stats',
>    'trace',
>    'transaction',
>    'yank',
> diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
> 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.
> +#
> +# Since: 7.1
> +##
> +{ 'enum' : 'StatsType',
> +  'data' : [ 'cumulative', 'instant', 'peak', 'linear-hist', 'log-hist' ] }
> +
> +##
> +# @StatsUnit:
> +#
> +# Enumeration of unit of measurement for statistics
> +#
> +# @bytes: stat reported in bytes.
> +# @seconds: stat reported in seconds.
> +# @cycles: stat reported in clock cycles.
> +#
> +# Since: 7.1
> +##
> +{ 'enum' : 'StatsUnit',
> +  'data' : [ 'bytes', 'seconds', 'cycles' ] }
> +
> +##
> +# @StatsProvider:
> +#
> +# Enumeration of statistics providers.
> +#
> +# Since: 7.1
> +##
> +{ 'enum': 'StatsProvider',
> +  'data': [ ] }
> +
> +##
> +# @StatsTarget:
> +#
> +# The kinds of objects on which one can request statistics.
> +#
> +# @vm: the entire virtual machine.
> +# @vcpu: a virtual CPU.
> +#
> +# Since: 7.1
> +##
> +{ 'enum': 'StatsTarget',
> +  'data': [ 'vm', 'vcpu' ] }
> +
> +##
> +# @StatsFilter:
> +#
> +# 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' } }
> +
> +##
> +# @StatsValue:
> +#
> +# @scalar: single uint64.
> +# @list: list of uint64.
> +#
> +# Since: 7.1
> +##
> +{ 'alternate': 'StatsValue',
> +  'data': { 'scalar': 'uint64',
> +            'list': [ 'uint64' ] } }
> +
> +##
> +# @Stats:
> +#
> +# @name: name of stat.
> +# @value: stat value.
> +#
> +# Since: 7.1
> +##
> +{ 'struct': 'Stats',
> +  'data': { 'name': 'str',
> +            'value' : 'StatsValue' } }
> +
> +##
> +# @StatsResult:
> +#
> +# @provider: provider for this set of statistics.
> +# @qom-path: QOM path of the object for which the statistics are returned
> +# @stats: list of statistics.
> +#
> +# Since: 7.1
> +##
> +{ 'struct': 'StatsResult',
> +  'data': { 'provider': 'StatsProvider',
> +            '*qom-path': 'str',
> +            'stats': [ 'Stats' ] } }
> +
> +##
> +# @query-stats:
> +#
> +# Return runtime-collected statistics for objects such as the
> +# VM or its vCPUs.
> +#
> +# The arguments are a StatsFilter and specify the provider and objects
> +# to return statistics about.
> +#
> +# Returns: a list of StatsResult, one for each provider and object
> +#          (e.g., for each vCPU).
> +#
> +# Since: 7.1
> +##
> +{ 'command': 'query-stats',
> +  'data': 'StatsFilter',
> +  'boxed': true,
> +  'returns': [ 'StatsResult' ] }
> +
> +##
> +# @StatsSchemaValue:
> +#
> +# Schema for a single statistic.
> +#
> +# @name: stat name.
> +#
> +# @type: kind of statistic, a @StatType.
> +#
> +# @unit: base unit of measurement for the statistics @StatUnit.
> +#
> +# @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
> +#
> +# @bucket-size: Used with linear-hist to report the width of each bucket
> +#               of the histogram.
> +#
> +# Since: 7.1
> +##
> +{ 'struct': 'StatsSchemaValue',
> +  'data': { 'name': 'str',
> +            'type': 'StatsType',
> +            '*unit': 'StatsUnit',
> +            '*base': 'int8',
> +            'exponent': 'int16',
> +            '*bucket-size': 'uint32' } }
> +
> +##
> +# @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' ] } }
> +
> +##
> +# @query-stats-schemas:
> +#
> +# Return the schema for all available runtime-collected statistics.
> +#
> +# Since: 7.1
> +##
> +{ 'command': 'query-stats-schemas',
> +  'data': { },
> +  'returns': [ 'StatsSchema' ] }
> -- 
> 2.35.1
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Paolo Bonzini]

On 4/27/22 11:19, Dr. David Alan Gilbert wrote:
>> { "return": [
>>       { "provider": "kvm",
>>         "stats": [
>>            { "name": "max_mmu_page_hash_collisions", "value": 0 },
>>            { "name": "max_mmu_rmap_size", "value": 0 },
>>            { "name": "nx_lpage_splits", "value": 148 },
> 
> Is there any hierarchy to the naming or is it just a big flat name
> space?

Within KVM no, but there is a hierarchy of provider->stat.

>>      { "provider": "kvm",
>>        "target": "vm",
>>        "stats": [
>>          { "name": "max_mmu_page_hash_collisions",
>>             "unit": "none",
>>             "base": 10,
>>             "exponent": 0,
>>             "type": "peak" },
>>          ... ]
>>      },
> 
> Is there some way to reset the peak or cumulative values?

Not yet, but the plan is to allow pwrite for peak and cumulative 
statistics, and possibly for histograms as well.  Alternatively it could 
be a ioctl.  Indecision about write support is also the reason why mmap 
is not allowed yet.

Paolo

> Dave
> 
>>      { "provider": "xyz",
>>        "target": "vm",
>>        "stats": [ ... ]
>>      }
>> ] }
>>
>> Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>   include/monitor/stats.h |  33 +++++++
>>   monitor/qmp-cmds.c      |  71 +++++++++++++++
>>   qapi/meson.build        |   1 +
>>   qapi/qapi-schema.json   |   1 +
>>   qapi/stats.json         | 192 ++++++++++++++++++++++++++++++++++++++++
>>   5 files changed, 298 insertions(+)
>>   create mode 100644 include/monitor/stats.h
>>   create mode 100644 qapi/stats.json
>>
>> diff --git a/include/monitor/stats.h b/include/monitor/stats.h
>> new file mode 100644
>> index 0000000000..89552ab06f
>> --- /dev/null
>> +++ b/include/monitor/stats.h
>> @@ -0,0 +1,33 @@
>> +/*
>> + * Copyright (c) 2022 Oracle and/or its affiliates.
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#ifndef STATS_H
>> +#define STATS_H
>> +
>> +#include "qapi/qapi-types-stats.h"
>> +
>> +typedef void StatRetrieveFunc(StatsResultList **result, StatsTarget target, Error **errp);
>> +typedef void SchemaRetrieveFunc(StatsSchemaList **result, Error **errp);
>> +
>> +/*
>> + * Register callbacks for the QMP query-stats command.
>> + *
>> + * @stats_fn: routine to query stats:
>> + * @schema_fn: routine to query stat schemas:
>> + */
>> +void add_stats_callbacks(StatRetrieveFunc *stats_fn,
>> +                         SchemaRetrieveFunc *schemas_fn);
>> +
>> +/*
>> + * Helper routines for adding stats entries to the results lists.
>> + */
>> +void add_stats_entry(StatsResultList **, StatsProvider, const char *id,
>> +                     StatsList *stats_list);
>> +void add_stats_schema(StatsSchemaList **, StatsProvider, StatsTarget,
>> +                      StatsSchemaValueList *);
>> +
>> +#endif /* STATS_H */
>> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
>> index 5e7302cbb9..97825b25fa 100644
>> --- a/monitor/qmp-cmds.c
>> +++ b/monitor/qmp-cmds.c
>> @@ -35,6 +35,7 @@
>>   #include "qapi/qapi-commands-control.h"
>>   #include "qapi/qapi-commands-machine.h"
>>   #include "qapi/qapi-commands-misc.h"
>> +#include "qapi/qapi-commands-stats.h"
>>   #include "qapi/qapi-commands-ui.h"
>>   #include "qapi/type-helpers.h"
>>   #include "qapi/qmp/qerror.h"
>> @@ -43,6 +44,7 @@
>>   #include "hw/acpi/acpi_dev_interface.h"
>>   #include "hw/intc/intc.h"
>>   #include "hw/rdma/rdma.h"
>> +#include "monitor/stats.h"
>>   
>>   NameInfo *qmp_query_name(Error **errp)
>>   {
>> @@ -426,3 +428,72 @@ HumanReadableText *qmp_x_query_irq(Error **errp)
>>   
>>       return human_readable_text_from_str(buf);
>>   }
>> +
>> +typedef struct StatsCallbacks {
>> +    StatRetrieveFunc *stats_cb;
>> +    SchemaRetrieveFunc *schemas_cb;
>> +    QTAILQ_ENTRY(StatsCallbacks) next;
>> +} StatsCallbacks;
>> +
>> +static QTAILQ_HEAD(, StatsCallbacks) stats_callbacks =
>> +    QTAILQ_HEAD_INITIALIZER(stats_callbacks);
>> +
>> +void add_stats_callbacks(StatRetrieveFunc *stats_fn,
>> +                         SchemaRetrieveFunc *schemas_fn)
>> +{
>> +    StatsCallbacks *entry = g_new(StatsCallbacks, 1);
>> +    entry->stats_cb = stats_fn;
>> +    entry->schemas_cb = schemas_fn;
>> +
>> +    QTAILQ_INSERT_TAIL(&stats_callbacks, entry, next);
>> +}
>> +
>> +StatsResultList *qmp_query_stats(StatsFilter *filter, Error **errp)
>> +{
>> +    StatsResultList *stats_results = NULL;
>> +    StatsCallbacks *entry;
>> +
>> +    QTAILQ_FOREACH(entry, &stats_callbacks, next) {
>> +        entry->stats_cb(&stats_results, filter->target, errp);
>> +    }
>> +
>> +    return stats_results;
>> +}
>> +
>> +StatsSchemaList *qmp_query_stats_schemas(Error **errp)
>> +{
>> +    StatsSchemaList *stats_results = NULL;
>> +    StatsCallbacks *entry;
>> +
>> +    QTAILQ_FOREACH(entry, &stats_callbacks, next) {
>> +        entry->schemas_cb(&stats_results, errp);
>> +    }
>> +
>> +    return stats_results;
>> +}
>> +
>> +void add_stats_entry(StatsResultList **stats_results, StatsProvider provider,
>> +                     const char *qom_path, StatsList *stats_list)
>> +{
>> +    StatsResult *entry = g_new0(StatsResult, 1);
>> +    entry->provider = provider;
>> +    if (qom_path) {
>> +        entry->has_qom_path = true;
>> +        entry->qom_path = g_strdup(qom_path);
>> +    }
>> +    entry->stats = stats_list;
>> +
>> +    QAPI_LIST_PREPEND(*stats_results, entry);
>> +}
>> +
>> +void add_stats_schema(StatsSchemaList **schema_results,
>> +                      StatsProvider provider, StatsTarget target,
>> +                      StatsSchemaValueList *stats_list)
>> +{
>> +    StatsSchema *entry = g_new0(StatsSchema, 1);
>> +
>> +    entry->provider = provider;
>> +    entry->target = target;
>> +    entry->stats = stats_list;
>> +    QAPI_LIST_PREPEND(*schema_results, entry);
>> +}
>> diff --git a/qapi/meson.build b/qapi/meson.build
>> index 656ef0e039..fd5c93d643 100644
>> --- a/qapi/meson.build
>> +++ b/qapi/meson.build
>> @@ -46,6 +46,7 @@ qapi_all_modules = [
>>     'replay',
>>     'run-state',
>>     'sockets',
>> +  'stats',
>>     'trace',
>>     'transaction',
>>     'yank',
>> diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
>> 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.
>> +#
>> +# Since: 7.1
>> +##
>> +{ 'enum' : 'StatsType',
>> +  'data' : [ 'cumulative', 'instant', 'peak', 'linear-hist', 'log-hist' ] }
>> +
>> +##
>> +# @StatsUnit:
>> +#
>> +# Enumeration of unit of measurement for statistics
>> +#
>> +# @bytes: stat reported in bytes.
>> +# @seconds: stat reported in seconds.
>> +# @cycles: stat reported in clock cycles.
>> +#
>> +# Since: 7.1
>> +##
>> +{ 'enum' : 'StatsUnit',
>> +  'data' : [ 'bytes', 'seconds', 'cycles' ] }
>> +
>> +##
>> +# @StatsProvider:
>> +#
>> +# Enumeration of statistics providers.
>> +#
>> +# Since: 7.1
>> +##
>> +{ 'enum': 'StatsProvider',
>> +  'data': [ ] }
>> +
>> +##
>> +# @StatsTarget:
>> +#
>> +# The kinds of objects on which one can request statistics.
>> +#
>> +# @vm: the entire virtual machine.
>> +# @vcpu: a virtual CPU.
>> +#
>> +# Since: 7.1
>> +##
>> +{ 'enum': 'StatsTarget',
>> +  'data': [ 'vm', 'vcpu' ] }
>> +
>> +##
>> +# @StatsFilter:
>> +#
>> +# 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' } }
>> +
>> +##
>> +# @StatsValue:
>> +#
>> +# @scalar: single uint64.
>> +# @list: list of uint64.
>> +#
>> +# Since: 7.1
>> +##
>> +{ 'alternate': 'StatsValue',
>> +  'data': { 'scalar': 'uint64',
>> +            'list': [ 'uint64' ] } }
>> +
>> +##
>> +# @Stats:
>> +#
>> +# @name: name of stat.
>> +# @value: stat value.
>> +#
>> +# Since: 7.1
>> +##
>> +{ 'struct': 'Stats',
>> +  'data': { 'name': 'str',
>> +            'value' : 'StatsValue' } }
>> +
>> +##
>> +# @StatsResult:
>> +#
>> +# @provider: provider for this set of statistics.
>> +# @qom-path: QOM path of the object for which the statistics are returned
>> +# @stats: list of statistics.
>> +#
>> +# Since: 7.1
>> +##
>> +{ 'struct': 'StatsResult',
>> +  'data': { 'provider': 'StatsProvider',
>> +            '*qom-path': 'str',
>> +            'stats': [ 'Stats' ] } }
>> +
>> +##
>> +# @query-stats:
>> +#
>> +# Return runtime-collected statistics for objects such as the
>> +# VM or its vCPUs.
>> +#
>> +# The arguments are a StatsFilter and specify the provider and objects
>> +# to return statistics about.
>> +#
>> +# Returns: a list of StatsResult, one for each provider and object
>> +#          (e.g., for each vCPU).
>> +#
>> +# Since: 7.1
>> +##
>> +{ 'command': 'query-stats',
>> +  'data': 'StatsFilter',
>> +  'boxed': true,
>> +  'returns': [ 'StatsResult' ] }
>> +
>> +##
>> +# @StatsSchemaValue:
>> +#
>> +# Schema for a single statistic.
>> +#
>> +# @name: stat name.
>> +#
>> +# @type: kind of statistic, a @StatType.
>> +#
>> +# @unit: base unit of measurement for the statistics @StatUnit.
>> +#
>> +# @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
>> +#
>> +# @bucket-size: Used with linear-hist to report the width of each bucket
>> +#               of the histogram.
>> +#
>> +# Since: 7.1
>> +##
>> +{ 'struct': 'StatsSchemaValue',
>> +  'data': { 'name': 'str',
>> +            'type': 'StatsType',
>> +            '*unit': 'StatsUnit',
>> +            '*base': 'int8',
>> +            'exponent': 'int16',
>> +            '*bucket-size': 'uint32' } }
>> +
>> +##
>> +# @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' ] } }
>> +
>> +##
>> +# @query-stats-schemas:
>> +#
>> +# Return the schema for all available runtime-collected statistics.
>> +#
>> +# Since: 7.1
>> +##
>> +{ 'command': 'query-stats-schemas',
>> +  'data': { },
>> +  'returns': [ 'StatsSchema' ] }
>> -- 
>> 2.35.1
>>
>>

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> From: Mark Kanda <mark.kanda@oracle.com>
>
> 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 to start), with
> additional options for specifying stat names, vCPU qom paths, and providers.
>
> - query-stats-schemas
> Returns a list of stats included in each target type, with an option for
> specifying the provider.  The concepts in the schema are based on the
> KVM binary stats' own introspection data, just translated to QAPI.

The second sentence helps build the case for "we actually need this
stuff".

Can you point to existing uses of KVM binary stats introspection data?

>
> The framework provides a method to register callbacks for these QMP commands.
> Most of the work in fact is done by the callbacks, and a large majority of
> this patch is new QAPI structs and commands.
>
> The first use-case will be for fd-based KVM stats (in an upcoming patch).
>
> Examples (with fd-based KVM stats):
>
> - Query all VM stats:
>
> { "execute": "query-stats", "arguments" : { "target": "vm" } }
>
> { "return": [
>      { "provider": "kvm",
>        "stats": [
>           { "name": "max_mmu_page_hash_collisions", "value": 0 },
>           { "name": "max_mmu_rmap_size", "value": 0 },
>           { "name": "nx_lpage_splits", "value": 148 },
>           ... ] },
>      { "provider": "xyz",
>        "stats": [ ... ] }
> ] }
>
> - Query all vCPU stats:
>
> { "execute": "query-stats", "arguments" : { "target": "vcpu" } }
>
> { "return": [
>      { "provider": "kvm",
>        "qom_path": "/machine/unattached/device[0]"
>        "stats": [
>           { "name": "guest_mode", "value": 0 },
>           { "name": "directed_yield_successful", "value": 0 },
>           { "name": "directed_yield_attempted", "value": 106 },
>           ... ] },
>      { "provider": "kvm",
>        "qom_path": "/machine/unattached/device[1]"
>        "stats": [
>           { "name": "guest_mode", "value": 0 },
>           { "name": "directed_yield_successful", "value": 0 },
>           { "name": "directed_yield_attempted", "value": 106 },
>           ... ] },
> ] }
>
> - Retrieve the schemas:
>
> { "execute": "query-stats-schemas" }
>
> { "return": [
>     { "provider": "kvm",
>       "target": "vcpu",
>       "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": "kvm",
>       "target": "vm",
>       "stats": [
>         { "name": "max_mmu_page_hash_collisions",
>            "unit": "none",
>            "base": 10,
>            "exponent": 0,
>            "type": "peak" },
>         ... ]
>     },
>     { "provider": "xyz",
>       "target": "vm",
>       "stats": [ ... ]
>     }
> ] }
>
> Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

[...]

> diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
> 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?

> +#
> +# Since: 7.1
> +##
> +{ 'enum' : 'StatsType',
> +  'data' : [ 'cumulative', 'instant', 'peak', 'linear-hist', 'log-hist' ] }
> +
> +##
> +# @StatsUnit:
> +#
> +# Enumeration of unit of measurement for statistics
> +#
> +# @bytes: stat reported in bytes.
> +# @seconds: stat reported in seconds.
> +# @cycles: stat reported in clock cycles.
> +#
> +# Since: 7.1
> +##
> +{ 'enum' : 'StatsUnit',
> +  'data' : [ 'bytes', 'seconds', 'cycles' ] }
> +
> +##
> +# @StatsProvider:
> +#
> +# Enumeration of statistics providers.
> +#
> +# Since: 7.1
> +##
> +{ 'enum': 'StatsProvider',
> +  'data': [ ] }
> +
> +##
> +# @StatsTarget:
> +#
> +# The kinds of objects on which one can request statistics.
> +#
> +# @vm: the entire virtual machine.
> +# @vcpu: a virtual CPU.
> +#
> +# Since: 7.1
> +##
> +{ 'enum': 'StatsTarget',
> +  'data': [ 'vm', 'vcpu' ] }

Do VM stats include vCPU stats?  "Entire virtual machine" suggests they
do...

> +
> +##
> +# @StatsFilter:
> +#
> +# 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?

> +
> +##
> +# @StatsValue:
> +#
> +# @scalar: single uint64.
> +# @list: list of uint64.

Recommend to spell out uint64 as "unsigned 64 bit integer".

> +#
> +# Since: 7.1
> +##
> +{ 'alternate': 'StatsValue',
> +  'data': { 'scalar': 'uint64',
> +            'list': [ 'uint64' ] } }
> +
> +##
> +# @Stats:
> +#
> +# @name: name of stat.
> +# @value: stat value.
> +#
> +# Since: 7.1
> +##
> +{ 'struct': 'Stats',
> +  'data': { 'name': 'str',
> +            'value' : 'StatsValue' } }
> +
> +##
> +# @StatsResult:
> +#
> +# @provider: provider for this set of statistics.
> +# @qom-path: QOM path of the object for which the statistics are returned
> +# @stats: list of statistics.
> +#
> +# Since: 7.1
> +##
> +{ 'struct': 'StatsResult',
> +  'data': { 'provider': 'StatsProvider',
> +            '*qom-path': 'str',

When exactly will @qom-path be present?

> +            'stats': [ 'Stats' ] } }
> +
> +##
> +# @query-stats:
> +#
> +# Return runtime-collected statistics for objects such as the
> +# VM or its vCPUs.
> +#
> +# The arguments are a StatsFilter and specify the provider and objects
> +# to return statistics about.
> +#
> +# Returns: a list of StatsResult, one for each provider and object
> +#          (e.g., for each vCPU).
> +#
> +# Since: 7.1
> +##
> +{ 'command': 'query-stats',
> +  'data': 'StatsFilter',
> +  'boxed': true,
> +  'returns': [ 'StatsResult' ] }
> +
> +##
> +# @StatsSchemaValue:
> +#
> +# Schema for a single statistic.
> +#
> +# @name: stat name.
> +#
> +# @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.

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

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

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

> +#
> +# Since: 7.1
> +##
> +{ 'struct': 'StatsSchemaValue',
> +  'data': { 'name': 'str',
> +            'type': 'StatsType',
> +            '*unit': 'StatsUnit',
> +            '*base': 'int8',
> +            'exponent': 'int16',
> +            '*bucket-size': 'uint32' } }
> +
> +##
> +# @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?

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

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

[Paolo Bonzini]

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

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

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

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

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

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

Paolo

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

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

[Daniel P. Berrangé]

On Thu, May 05, 2022 at 09:10:17AM +0200, Paolo Bonzini wrote:
> 
> 
> 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).

Libvirt currently uses debugfs to get

 /sys/kernel/debug/kvm/-/halt_poll_success_ns

when we report on CPU usage for VMs. WHen kernel lockdown is enforced
under secure boot we're unable to access this file and even worse
every attempt to access it spams dmesg[1].  We need this query stats
QMP support for that single statistic alone today.


With regards,
Daniel

[1] https://gitlab.com/libvirt/libvirt/-/issues/213
-- 
|: 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 :|

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

[Markus Armbruster]

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' ] }
>>

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

[Daniel P. Berrangé]

On Thu, May 05, 2022 at 03:28:23PM +0200, Markus Armbruster wrote:
> 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.

The second introspection system was the bit I disliked the most.

The inherant tension we have in that respect is that traditionally
with QMP we explicitly /want/ the developer to have todo design+coding
work to expose every new piece of data. Similarly on the client side
we are expecting work to consume any new piece of data.

With this command we explicitly do NOT want the developer to do
any new design+coding work, but instead allow almost arbitrary
passthrough of whatever data the kernel decides to expose, and
consumption of arbitrary data without writing new code.

There is some appeal in why we want todo that, but it is certainly
a divergance from our historical approach to QMP, so we shouldn't
make this decision lightly.

With 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 :|

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

[Dr. David Alan Gilbert]

* Daniel P. Berrangé (berrange@redhat.com) wrote:
> On Thu, May 05, 2022 at 03:28:23PM +0200, Markus Armbruster wrote:
> > 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.
> 
> The second introspection system was the bit I disliked the most.
> 
> The inherant tension we have in that respect is that traditionally
> with QMP we explicitly /want/ the developer to have todo design+coding
> work to expose every new piece of data. Similarly on the client side
> we are expecting work to consume any new piece of data.
> 
> With this command we explicitly do NOT want the developer to do
> any new design+coding work, but instead allow almost arbitrary
> passthrough of whatever data the kernel decides to expose, and
> consumption of arbitrary data without writing new code.

The developer is going to have had to made that design when they put it
in the kernel; they don't really want to repeat the bikeshedding at each
further layer up the stack.  We have to be able to accept that we're
dealing with another (open) interface which has already gone through
review.

Dave

> There is some appeal in why we want todo that, but it is certainly
> a divergance from our historical approach to QMP, so we shouldn't
> make this decision lightly.
> 
> With 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 :|
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> 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'd like this to be captured in documentation and / or a commit message,
because ...

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

... ^^^ needs justification.  Explain why passing the kernel's
existing interface through QEMU is useful, and to whom.

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

How strong do we feel about the stability of the stats exposed by this
command?  Separate answers for *structure* of the stats and concrete
stats.

If we're confident neither structure nor concrete stats will change
incompatibly, the commands are stable without reservations.

If we're confident the structure is stable, but unable or unwilling to
commit to the concrete stats, we should explain this in documentation.

If we're unsure about both, the commands should be marked unstable.  We
can always upgrade stability later.

[...]

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

[Paolo Bonzini]

On 5/13/22 15:10, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>> On 5/5/22 15:28, Markus Armbruster wrote:
>>> This means we'll acquire yet another introspection system, unrelated to
>>> the introspection systems we already have in QEMU.
> 
> ... ^^^ needs justification.  Explain why passing the kernel's
> existing interface through QEMU is useful, and to whom.

There are two justifications.

The first is the contents of the schemas: the new introspection data 
provides different information than the QAPI data, namely unit of 
measurement, how the numbers are gathered and change 
(peak/instant/cumulative/histogram), and histogram bucket sizes.  Unless 
you think those should be added to the QAPI introspection (and IMO there 
might be a case only for the unit of measure---and even then it's only a 
very weak case), the separate introspection data justifies itself.

So the existence of query-stats-schemas in my opinion is justified even 
if don't consider the usecase of passing through the kernel's descriptions.

The second justification however is indeed about the dynamicity of the 
schema.  The QAPI introspection data is very much static; and while QOM 
is somewhat more dynamic, generally we consider that to be a bug rather 
than a feature these days.  On the other hand, running old QEMU with new 
kernel is a supported usecase; if old QEMU cannot expose statistics from 
a new kernel, or if a kernel developer needs to change QEMU before 
gathering new info from the new kernel, then that is a poor user interface.

Gathering statistics is important for development, for monitoring and 
for performance measurement.  There are tools such as kvm_stat that do 
this and they rely on the _user_ knowing the interesting data points 
rather than the tool (which can treat them as opaque).  The goal here is 
to take the capabilities of these tools and making them available 
throughout the whole virtualization stack, so that one can observe, 
monitor and measure virtual machines without having shell access + root 
on the host that runs them.

> How strong do we feel about the stability of the stats exposed by this
> command?  Separate answers for *structure* of the stats and concrete
> stats.

I'll try to answer this from the point of view of the kernel:

- will "some" statistics ever be available for all targets that are 
currently supported?  Yes, resoundingly.

- are the names of statistics stable?  Mostly, but not 100%.  If 
somebody notices the same value is being tracked with different names in 
two different architectures, one of them might be renamed.  If the 
statistic tracks a variable that does not exist anymore as the code 
changes, the statistic will go away.  If KVM grows two different ways to 
do the same thing and the default changes, some statistics that were 
previously useful could now be stuck at 0.  All of these events are 
expected to be rare, but 100% stability is neither a goal nor attainable 
in my opinion.

- is the schema format stable?  Yes, it is designed to be extensible but 
it will be backwards compatible.  Don't break userspace and all that.

And for QEMU:

- will "some" statistics ever be available for all targets that are 
currently supported?  Yes, and this will be true even if other 
QEMU-specific targets are added, e.g. block devices.

- will other providers have the same guarantees of stability?  It 
depends.  Statistics based on the current "query-blockstats" output will 
probably be even more stable than KVM stats.  TCG stats might be of 
variable stability.  We can add "x-" in front of providers if we decide 
that such a convention is useful.

- is the QEMU schema format stable?  Yes.  What we have is more or less 
a 1:1 conversion of the KVM schema format, which is pretty 
comprehensive. But if an addition to the schema proves itself worthwhile 
it can be added with the usual care to QAPI backwards compatibility.

> If we're confident neither structure nor concrete stats will change
> incompatibly, the commands are stable without reservations.
> 
> If we're confident the structure is stable, but unable or unwilling to
> commit to the concrete stats, we should explain this in documentation.

Based on the above text do you have a suggested wording and, especially, 
a suggested place?  For example, do you think it would fit better in the 
query-stats or query-stats-schemas documentation?

Thanks,

Paolo

> If we're unsure about both, the commands should be marked unstable.  We
> can always upgrade stability later.

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 5/13/22 15:10, Markus Armbruster wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>> On 5/5/22 15:28, Markus Armbruster wrote:
>>>> This means we'll acquire yet another introspection system, unrelated to
>>>> the introspection systems we already have in QEMU.
>> 
>> ... ^^^ needs justification.  Explain why passing the kernel's
>> existing interface through QEMU is useful, and to whom.
>
> There are two justifications.
>
> The first is the contents of the schemas: the new introspection data 
> provides different information than the QAPI data, namely unit of 
> measurement, how the numbers are gathered and change 
> (peak/instant/cumulative/histogram), and histogram bucket sizes.  Unless 
> you think those should be added to the QAPI introspection (and IMO there 
> might be a case only for the unit of measure---and even then it's only a 
> very weak case), the separate introspection data justifies itself.
>
> So the existence of query-stats-schemas in my opinion is justified even 
> if don't consider the usecase of passing through the kernel's descriptions.
>
> The second justification however is indeed about the dynamicity of the 
> schema.  The QAPI introspection data is very much static; and while QOM 
> is somewhat more dynamic, generally we consider that to be a bug rather 
> than a feature these days.  On the other hand, running old QEMU with new 
> kernel is a supported usecase; if old QEMU cannot expose statistics from 
> a new kernel, or if a kernel developer needs to change QEMU before 
> gathering new info from the new kernel, then that is a poor user interface.
>
> Gathering statistics is important for development, for monitoring and 
> for performance measurement.  There are tools such as kvm_stat that do 
> this and they rely on the _user_ knowing the interesting data points 
> rather than the tool (which can treat them as opaque).  The goal here is 
> to take the capabilities of these tools and making them available 
> throughout the whole virtualization stack, so that one can observe, 
> monitor and measure virtual machines without having shell access + root 
> on the host that runs them.

Work this into one of the commit messages, please.

>> How strong do we feel about the stability of the stats exposed by this
>> command?  Separate answers for *structure* of the stats and concrete
>> stats.
>
> I'll try to answer this from the point of view of the kernel:
>
> - will "some" statistics ever be available for all targets that are 
> currently supported?  Yes, resoundingly.
>
> - are the names of statistics stable?  Mostly, but not 100%.  If 
> somebody notices the same value is being tracked with different names in 
> two different architectures, one of them might be renamed.  If the 
> statistic tracks a variable that does not exist anymore as the code 
> changes, the statistic will go away.  If KVM grows two different ways to 
> do the same thing and the default changes, some statistics that were 
> previously useful could now be stuck at 0.  All of these events are 
> expected to be rare, but 100% stability is neither a goal nor attainable 
> in my opinion.
>
> - is the schema format stable?  Yes, it is designed to be extensible but 
> it will be backwards compatible.  Don't break userspace and all that.
>
> And for QEMU:
>
> - will "some" statistics ever be available for all targets that are 
> currently supported?  Yes, and this will be true even if other 
> QEMU-specific targets are added, e.g. block devices.
>
> - will other providers have the same guarantees of stability?  It 
> depends.  Statistics based on the current "query-blockstats" output will 
> probably be even more stable than KVM stats.  TCG stats might be of 
> variable stability.  We can add "x-" in front of providers if we decide 
> that such a convention is useful.
>
> - is the QEMU schema format stable?  Yes.  What we have is more or less 
> a 1:1 conversion of the KVM schema format, which is pretty 
> comprehensive. But if an addition to the schema proves itself worthwhile 
> it can be added with the usual care to QAPI backwards compatibility.
>
>> If we're confident neither structure nor concrete stats will change
>> incompatibly, the commands are stable without reservations.
>> 
>> If we're confident the structure is stable, but unable or unwilling to
>> commit to the concrete stats, we should explain this in documentation.
>
> Based on the above text do you have a suggested wording and, especially, 

Friday afternoon, worst time for word-smithing...  Feel free to ask
again on Monday :)

> a suggested place?  For example, do you think it would fit better in the 
> query-stats or query-stats-schemas documentation?

No obvious best choice.  I'd lean towards query-stats-schema.  Or
perhaps neither; write a separate introduction instead, like this:

    ##
    # = Statistics
    #
    # Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do
    # eiusmod tempor incididunt ut labore et dolore magna aliqua.  Ut enim
    # ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut
    # aliquip ex ea commodo consequat.  Duis aute irure dolor in
    # reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla
    # pariatur.  Excepteur sint occaecat cupidatat non proident, sunt in
    # culpa qui officia deserunt mollit anim id est laborum.
    ##

Comes out in HTML as you'd expect, except it gets also included in the
table of contents, which is a bug.

>> If we're unsure about both, the commands should be marked unstable.  We
>> can always upgrade stability later.

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

[Paolo Bonzini]

On 5/13/22 16:35, Markus Armbruster wrote:
> Friday afternoon, worst time for word-smithing...  Feel free to ask
> again on Monday:)
> 
>> a suggested place?  For example, do you think it would fit better in the
>> query-stats or query-stats-schemas documentation?

I think query-stats-schemas is good enough.

# *Note*: While QEMU and other providers of runtime-collected statistics
# will try to keep the set of available data stable, together with their
# names, it is impossible to provide a full guarantee.  For example, if
# the same value is being tracked with different names on different
# architectures or by different providers, one of them might be renamed.
# A statistic might go away if an algorithm is changed or some code is
# removed; changing a default might cause previously useful statistics
# to always report 0.  Such changes fall outside QEMU's usual deprecation
# policies (also because statistics might be sourced externally, e.g.
# from Linux).  However, they are expected to be rare.


Paolo

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 5/13/22 16:35, Markus Armbruster wrote:
>> Friday afternoon, worst time for word-smithing...  Feel free to ask
>> again on Monday:)
>> 
>>> a suggested place?  For example, do you think it would fit better in the
>>> query-stats or query-stats-schemas documentation?
>
> I think query-stats-schemas is good enough.
>
> # *Note*: While QEMU and other providers of runtime-collected statistics
> # will try to keep the set of available data stable, together with their
> # names, it is impossible to provide a full guarantee.  For example, if
> # the same value is being tracked with different names on different
> # architectures or by different providers, one of them might be renamed.
> # A statistic might go away if an algorithm is changed or some code is
> # removed; changing a default might cause previously useful statistics
> # to always report 0.  Such changes fall outside QEMU's usual deprecation
> # policies (also because statistics might be sourced externally, e.g.
> # from Linux).  However, they are expected to be rare.

Works for me!

Markup hint: "Note:" is a note section tag, while "*Note*:" is just
text.  I figure we want the section tag here.

[PATCH 2/8] kvm: Support for querying fd-based stats

[Paolo Bonzini]

From: Mark Kanda <mark.kanda@oracle.com>

Add support for querying fd-based KVM stats - as introduced by Linux kernel
commit:

cb082bfab59a ("KVM: stats: Add fd-based API to read binary stats data")

This allows the user to analyze the behavior of the VM without access
to debugfs.

Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 accel/kvm/kvm-all.c | 403 ++++++++++++++++++++++++++++++++++++++++++++
 qapi/stats.json     |   2 +-
 2 files changed, 404 insertions(+), 1 deletion(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 32e177bd26..4a753d4445 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -47,6 +47,7 @@
 #include "kvm-cpus.h"
 
 #include "hw/boards.h"
+#include "monitor/stats.h"
 
 /* This check must be after config-host.h is included */
 #ifdef CONFIG_EVENTFD
@@ -2310,6 +2311,9 @@ bool kvm_dirty_ring_enabled(void)
     return kvm_state->kvm_dirty_ring_size ? true : false;
 }
 
+static void query_stats_cb(StatsResultList **result, StatsTarget target, Error **errp);
+static void query_stats_schemas_cb(StatsSchemaList **result, Error **errp);
+
 static int kvm_init(MachineState *ms)
 {
     MachineClass *mc = MACHINE_GET_CLASS(ms);
@@ -2638,6 +2642,10 @@ static int kvm_init(MachineState *ms)
         }
     }
 
+    if (kvm_check_extension(kvm_state, KVM_CAP_BINARY_STATS_FD)) {
+        add_stats_callbacks(query_stats_cb, query_stats_schemas_cb);
+    }
+
     return 0;
 
 err:
@@ -3697,3 +3705,398 @@ static void kvm_type_init(void)
 }
 
 type_init(kvm_type_init);
+
+typedef struct StatsArgs {
+    union StatsResultsType {
+        StatsResultList **stats;
+        StatsSchemaList **schema;
+    } result;
+    Error **errp;
+} StatsArgs;
+
+static StatsList *add_kvmstat_entry(struct kvm_stats_desc *pdesc,
+                                    uint64_t *stats_data,
+                                    StatsList *stats_list,
+                                    Error **errp)
+{
+
+    StatsList *stats_entry;
+    Stats *stats;
+    uint64List *val_list = NULL;
+
+    switch (pdesc->flags & KVM_STATS_TYPE_MASK) {
+    case KVM_STATS_TYPE_CUMULATIVE:
+    case KVM_STATS_TYPE_INSTANT:
+    case KVM_STATS_TYPE_PEAK:
+    case KVM_STATS_TYPE_LINEAR_HIST:
+    case KVM_STATS_TYPE_LOG_HIST:
+        break;
+    default:
+        return stats_list;
+    }
+
+    switch (pdesc->flags & KVM_STATS_UNIT_MASK) {
+    case KVM_STATS_UNIT_NONE:
+    case KVM_STATS_UNIT_BYTES:
+    case KVM_STATS_UNIT_CYCLES:
+    case KVM_STATS_UNIT_SECONDS:
+        break;
+    default:
+        return stats_list;
+    }
+
+    switch (pdesc->flags & KVM_STATS_BASE_MASK) {
+    case KVM_STATS_BASE_POW10:
+    case KVM_STATS_BASE_POW2:
+        break;
+    default:
+        return stats_list;
+    }
+
+    /* Alloc and populate data list */
+    stats_entry = g_new0(StatsList, 1);
+    stats = g_new0(Stats, 1);
+    stats->name = g_strdup(pdesc->name);
+    stats->value = g_new0(StatsValue, 1);;
+
+    if (pdesc->size == 1) {
+        stats->value->u.scalar = *stats_data;
+        stats->value->type = QTYPE_QNUM;
+    } else {
+        int i;
+        for (i = 0; i < pdesc->size; i++) {
+            uint64List *val_entry = g_new0(uint64List, 1);
+            val_entry->value = stats_data[i];
+            val_entry->next = val_list;
+            val_list = val_entry;
+        }
+        stats->value->u.list = val_list;
+        stats->value->type = QTYPE_QLIST;
+    }
+
+    stats_entry->value = stats;
+    stats_entry->next = stats_list;
+
+    return stats_entry;
+}
+
+static StatsSchemaValueList *add_kvmschema_entry(struct kvm_stats_desc *pdesc,
+                                                 StatsSchemaValueList *list,
+                                                 Error **errp)
+{
+    StatsSchemaValueList *schema_entry = g_new0(StatsSchemaValueList, 1);
+    schema_entry->value = g_new0(StatsSchemaValue, 1);
+
+    switch (pdesc->flags & KVM_STATS_TYPE_MASK) {
+    case KVM_STATS_TYPE_CUMULATIVE:
+        schema_entry->value->type = STATS_TYPE_CUMULATIVE;
+        break;
+    case KVM_STATS_TYPE_INSTANT:
+        schema_entry->value->type = STATS_TYPE_INSTANT;
+        break;
+    case KVM_STATS_TYPE_PEAK:
+        schema_entry->value->type = STATS_TYPE_PEAK;
+        break;
+    case KVM_STATS_TYPE_LINEAR_HIST:
+        schema_entry->value->type = STATS_TYPE_LINEAR_HIST;
+        schema_entry->value->bucket_size = pdesc->bucket_size;
+        schema_entry->value->has_bucket_size = true;
+        break;
+    case KVM_STATS_TYPE_LOG_HIST:
+        schema_entry->value->type = STATS_TYPE_LOG_HIST;
+        break;
+    default:
+        goto exit;
+    }
+
+    switch (pdesc->flags & KVM_STATS_UNIT_MASK) {
+    case KVM_STATS_UNIT_NONE:
+        break;
+    case KVM_STATS_UNIT_BYTES:
+        schema_entry->value->has_unit = true;
+        schema_entry->value->unit = STATS_UNIT_BYTES;
+        break;
+    case KVM_STATS_UNIT_CYCLES:
+        schema_entry->value->has_unit = true;
+        schema_entry->value->unit = STATS_UNIT_CYCLES;
+        break;
+    case KVM_STATS_UNIT_SECONDS:
+        schema_entry->value->has_unit = true;
+        schema_entry->value->unit = STATS_UNIT_SECONDS;
+        break;
+    default:
+        goto exit;
+    }
+
+    schema_entry->value->exponent = pdesc->exponent;
+    if (pdesc->exponent) {
+        switch (pdesc->flags & KVM_STATS_BASE_MASK) {
+        case KVM_STATS_BASE_POW10:
+            schema_entry->value->has_base = true;
+            schema_entry->value->base = 10;
+            break;
+        case KVM_STATS_BASE_POW2:
+            schema_entry->value->has_base = true;
+            schema_entry->value->base = 2;
+            break;
+        default:
+            goto exit;
+        }
+    }
+
+    schema_entry->value->name = g_strdup(pdesc->name);
+    schema_entry->next = list;
+    return schema_entry;
+exit:
+    g_free(schema_entry->value);
+    g_free(schema_entry);
+    return list;
+}
+
+/* Cached stats descriptors */
+typedef struct StatsDescriptors {
+    char *ident; /* 'vm' or vCPU qom path */
+    struct kvm_stats_desc *kvm_stats_desc;
+    struct kvm_stats_header *kvm_stats_header;
+    QTAILQ_ENTRY(StatsDescriptors) next;
+} StatsDescriptors;
+
+static QTAILQ_HEAD(, StatsDescriptors) stats_descriptors =
+    QTAILQ_HEAD_INITIALIZER(stats_descriptors);
+
+static StatsDescriptors *find_stats_descriptors(StatsTarget target, int stats_fd,
+                                                Error **errp)
+{
+    StatsDescriptors *descriptors;
+    const char *ident;
+    struct kvm_stats_desc *kvm_stats_desc;
+    struct kvm_stats_header *kvm_stats_header;
+    size_t size_desc;
+    ssize_t ret;
+
+    switch (target) {
+    case STATS_TARGET_VM:
+        ident = StatsTarget_str(STATS_TARGET_VM);
+        break;
+    case STATS_TARGET_VCPU:
+        ident = current_cpu->parent_obj.canonical_path;
+        break;
+    default:
+        abort();
+    }
+
+    QTAILQ_FOREACH(descriptors, &stats_descriptors, next) {
+        if (g_str_equal(descriptors->ident, ident)) {
+            return descriptors;
+        }
+    }
+
+    descriptors = g_new0(StatsDescriptors, 1);
+
+    /* Read stats header */
+    kvm_stats_header = g_malloc(sizeof(*kvm_stats_header));
+    ret = read(stats_fd, kvm_stats_header, sizeof(*kvm_stats_header));
+    if (ret != sizeof(*kvm_stats_header)) {
+        error_setg(errp, "KVM stats: failed to read stats header: "
+                   "expected %zu actual %zu",
+                   sizeof(*kvm_stats_header), ret);
+        return NULL;
+    }
+    size_desc = sizeof(*kvm_stats_desc) + kvm_stats_header->name_size;
+
+    /* Read stats descriptors */
+    kvm_stats_desc = g_malloc0_n(kvm_stats_header->num_desc, size_desc);
+    ret = pread(stats_fd, kvm_stats_desc,
+                size_desc * kvm_stats_header->num_desc,
+                kvm_stats_header->desc_offset);
+
+    if (ret != size_desc * kvm_stats_header->num_desc) {
+        error_setg(errp, "KVM stats: failed to read stats descriptors: "
+                   "expected %zu actual %zu",
+                   size_desc * kvm_stats_header->num_desc, ret);
+        g_free(descriptors);
+        return NULL;
+    }
+    descriptors->kvm_stats_header = kvm_stats_header;
+    descriptors->kvm_stats_desc = kvm_stats_desc;
+    descriptors->ident = g_strdup(ident);
+    QTAILQ_INSERT_TAIL(&stats_descriptors, descriptors, next);
+    return descriptors;
+}
+
+static void query_stats(StatsResultList **result, StatsTarget target,
+                        int stats_fd, Error **errp)
+{
+    struct kvm_stats_desc *kvm_stats_desc;
+    struct kvm_stats_header *kvm_stats_header;
+    StatsDescriptors *descriptors;
+    g_autofree uint64_t *stats_data = NULL;
+    struct kvm_stats_desc *pdesc;
+    StatsList *stats_list = NULL;
+    size_t size_desc, size_data = 0;
+    ssize_t ret;
+    int i;
+
+    descriptors = find_stats_descriptors(target, stats_fd, errp);
+    if (!descriptors) {
+        return;
+    }
+
+    kvm_stats_header = descriptors->kvm_stats_header;
+    kvm_stats_desc = descriptors->kvm_stats_desc;
+    size_desc = sizeof(*kvm_stats_desc) + kvm_stats_header->name_size;
+
+    /* Tally the total data size; read schema data */
+    for (i = 0; i < kvm_stats_header->num_desc; ++i) {
+        pdesc = (void *)kvm_stats_desc + i * size_desc;
+        size_data += pdesc->size * sizeof(*stats_data);
+    }
+
+    stats_data = g_malloc0(size_data);
+    ret = pread(stats_fd, stats_data, size_data, kvm_stats_header->data_offset);
+
+    if (ret != size_data) {
+        error_setg(errp, "KVM stats: failed to read data: "
+                   "expected %zu actual %zu", size_data, ret);
+        return;
+    }
+
+    for (i = 0; i < kvm_stats_header->num_desc; ++i) {
+        uint64_t *stats;
+        pdesc = (void *)kvm_stats_desc + i * size_desc;
+
+        /* Add entry to the list */
+        stats = (void *)stats_data + pdesc->offset;
+        stats_list = add_kvmstat_entry(pdesc, stats, stats_list, errp);
+    }
+
+    if (!stats_list) {
+        return;
+    }
+
+    switch (target) {
+    case STATS_TARGET_VM:
+        add_stats_entry(result, STATS_PROVIDER_KVM, NULL, stats_list);
+        break;
+    case STATS_TARGET_VCPU:
+        add_stats_entry(result, STATS_PROVIDER_KVM,
+                        current_cpu->parent_obj.canonical_path,
+                        stats_list);
+        break;
+    default:
+        break;
+    }
+}
+
+static void query_stats_schema(StatsSchemaList **result, StatsTarget target,
+                               int stats_fd, Error **errp)
+{
+    struct kvm_stats_desc *kvm_stats_desc;
+    struct kvm_stats_header *kvm_stats_header;
+    StatsDescriptors *descriptors;
+    struct kvm_stats_desc *pdesc;
+    StatsSchemaValueList *stats_list = NULL;
+    size_t size_desc;
+    int i;
+
+    descriptors = find_stats_descriptors(target, stats_fd, errp);
+    if (!descriptors) {
+        return;
+    }
+
+    kvm_stats_header = descriptors->kvm_stats_header;
+    kvm_stats_desc = descriptors->kvm_stats_desc;
+    size_desc = sizeof(*kvm_stats_desc) + kvm_stats_header->name_size;
+
+    /* Tally the total data size; read schema data */
+    for (i = 0; i < kvm_stats_header->num_desc; ++i) {
+        pdesc = (void *)kvm_stats_desc + i * size_desc;
+        stats_list = add_kvmschema_entry(pdesc, stats_list, errp);
+    }
+
+    add_stats_schema(result, STATS_PROVIDER_KVM, target, stats_list);
+}
+
+static void query_stats_vcpu(CPUState *cpu, run_on_cpu_data data)
+{
+    StatsArgs *kvm_stats_args = (StatsArgs *) data.host_ptr;
+    int stats_fd = kvm_vcpu_ioctl(cpu, KVM_GET_STATS_FD, NULL);
+    Error *local_err = NULL;
+
+    if (stats_fd == -1) {
+        error_setg(&local_err, "KVM stats: ioctl failed");
+        error_propagate(kvm_stats_args->errp, local_err);
+        return;
+    }
+    query_stats(kvm_stats_args->result.stats, STATS_TARGET_VCPU, stats_fd,
+                kvm_stats_args->errp);
+    close(stats_fd);
+}
+
+static void query_stats_schema_vcpu(CPUState *cpu, run_on_cpu_data data)
+{
+    StatsArgs *kvm_stats_args = (StatsArgs *) data.host_ptr;
+    int stats_fd = kvm_vcpu_ioctl(cpu, KVM_GET_STATS_FD, NULL);
+    Error *local_err = NULL;
+
+    if (stats_fd == -1) {
+        error_setg(&local_err, "KVM stats: ioctl failed");
+        error_propagate(kvm_stats_args->errp, local_err);
+        return;
+    }
+    query_stats_schema(kvm_stats_args->result.schema, STATS_TARGET_VCPU, stats_fd,
+                       kvm_stats_args->errp);
+    close(stats_fd);
+}
+
+static void query_stats_cb(StatsResultList **result, StatsTarget target, Error **errp)
+{
+    KVMState *s = kvm_state;
+    CPUState *cpu;
+    int stats_fd;
+
+    switch (target) {
+    case STATS_TARGET_VM:
+    {
+        stats_fd = kvm_vm_ioctl(s, KVM_GET_STATS_FD, NULL);
+        if (stats_fd == -1) {
+            error_setg(errp, "KVM stats: ioctl failed");
+            return;
+        }
+        query_stats(result, target, stats_fd, errp);
+        close(stats_fd);
+        break;
+    }
+    case STATS_TARGET_VCPU:
+    {
+        StatsArgs stats_args;
+        stats_args.result.stats = result;
+        stats_args.errp = errp;
+        CPU_FOREACH(cpu) {
+            run_on_cpu(cpu, query_stats_vcpu, RUN_ON_CPU_HOST_PTR(&stats_args));
+        }
+        break;
+    }
+    default:
+        break;
+    }
+}
+
+void query_stats_schemas_cb(StatsSchemaList **result, Error **errp)
+{
+    StatsArgs stats_args;
+    KVMState *s = kvm_state;
+    int stats_fd;
+
+    stats_fd = kvm_vm_ioctl(s, KVM_GET_STATS_FD, NULL);
+    if (stats_fd == -1) {
+        error_setg(errp, "KVM stats: ioctl failed");
+        return;
+    }
+    query_stats_schema(result, STATS_TARGET_VM, stats_fd, errp);
+    close(stats_fd);
+
+    stats_args.result.schema = result;
+    stats_args.errp = errp;
+    run_on_cpu(first_cpu, query_stats_schema_vcpu, RUN_ON_CPU_HOST_PTR(&stats_args));
+}
diff --git a/qapi/stats.json b/qapi/stats.json
index 7454dd7daa..bcc897258a 100644
--- a/qapi/stats.json
+++ b/qapi/stats.json
@@ -50,7 +50,7 @@
 # Since: 7.1
 ##
 { 'enum': 'StatsProvider',
-  'data': [ ] }
+  'data': [ 'kvm' ] }
 
 ##
 # @StatsTarget:
-- 
2.35.1

[PATCH 3/8] qmp: add filtering of statistics by target vCPU

[Paolo Bonzini]

Introduce a simple filtering of statistics, that allows to retrieve
statistics for a subset of the guest vCPUs.  This will be used for
example by the HMP monitor, in order to retrieve the statistics
for the currently selected CPU.

Example:
{ "execute": "query-stats",
  "arguments": {
    "target": "vcpu",
    "vcpus": [ "/machine/unattached/device[2]",
               "/machine/unattached/device[4]" ] } }

Extracted from a patch by Mark Kanda.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 accel/kvm/kvm-all.c     |  9 +++++++--
 include/monitor/stats.h |  9 ++++++++-
 monitor/qmp-cmds.c      | 34 +++++++++++++++++++++++++++++++++-
 qapi/stats.json         | 16 ++++++++++++++--
 4 files changed, 62 insertions(+), 6 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 4a753d4445..bfd81039a1 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2311,7 +2311,8 @@ bool kvm_dirty_ring_enabled(void)
     return kvm_state->kvm_dirty_ring_size ? true : false;
 }
 
-static void query_stats_cb(StatsResultList **result, StatsTarget target, Error **errp);
+static void query_stats_cb(StatsResultList **result, StatsTarget target,
+                           strList *targets, Error **errp);
 static void query_stats_schemas_cb(StatsSchemaList **result, Error **errp);
 
 static int kvm_init(MachineState *ms)
@@ -4049,7 +4050,8 @@ static void query_stats_schema_vcpu(CPUState *cpu, run_on_cpu_data data)
     close(stats_fd);
 }
 
-static void query_stats_cb(StatsResultList **result, StatsTarget target, Error **errp)
+static void query_stats_cb(StatsResultList **result, StatsTarget target,
+                           strList *targets, Error **errp)
 {
     KVMState *s = kvm_state;
     CPUState *cpu;
@@ -4073,6 +4075,9 @@ static void query_stats_cb(StatsResultList **result, StatsTarget target, Error *
         stats_args.result.stats = result;
         stats_args.errp = errp;
         CPU_FOREACH(cpu) {
+            if (!str_in_list(cpu->parent_obj.canonical_path, targets)) {
+                continue;
+            }
             run_on_cpu(cpu, query_stats_vcpu, RUN_ON_CPU_HOST_PTR(&stats_args));
         }
         break;
diff --git a/include/monitor/stats.h b/include/monitor/stats.h
index 89552ab06f..92a1df3072 100644
--- a/include/monitor/stats.h
+++ b/include/monitor/stats.h
@@ -10,7 +10,8 @@
 
 #include "qapi/qapi-types-stats.h"
 
-typedef void StatRetrieveFunc(StatsResultList **result, StatsTarget target, Error **errp);
+typedef void StatRetrieveFunc(StatsResultList **result, StatsTarget target,
+                              strList *targets, Error **errp);
 typedef void SchemaRetrieveFunc(StatsSchemaList **result, Error **errp);
 
 /*
@@ -30,4 +31,10 @@ void add_stats_entry(StatsResultList **, StatsProvider, const char *id,
 void add_stats_schema(StatsSchemaList **, StatsProvider, StatsTarget,
                       StatsSchemaValueList *);
 
+/*
+ * True if a string matches the filter passed to the stats_fn callabck,
+ * false otherwise.
+ */
+bool str_in_list(const char *string, strList *list);
+
 #endif /* STATS_H */
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 97825b25fa..5e3f4c9685 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -448,13 +448,30 @@ void add_stats_callbacks(StatRetrieveFunc *stats_fn,
     QTAILQ_INSERT_TAIL(&stats_callbacks, entry, next);
 }
 
+static strList *stats_target_filter(StatsFilter *filter)
+{
+    switch (filter->target) {
+    case STATS_TARGET_VM:
+        return NULL;
+    case STATS_TARGET_VCPU:
+        if (!filter->u.vcpu.has_vcpus) {
+            return NULL;
+        }
+        return filter->u.vcpu.vcpus;
+        break;
+    default:
+        abort();
+    }
+}
+
 StatsResultList *qmp_query_stats(StatsFilter *filter, Error **errp)
 {
     StatsResultList *stats_results = NULL;
+    strList *targets = stats_target_filter(filter);
     StatsCallbacks *entry;
 
     QTAILQ_FOREACH(entry, &stats_callbacks, next) {
-        entry->stats_cb(&stats_results, filter->target, errp);
+        entry->stats_cb(&stats_results, filter->target, targets, errp);
     }
 
     return stats_results;
@@ -497,3 +514,18 @@ void add_stats_schema(StatsSchemaList **schema_results,
     entry->stats = stats_list;
     QAPI_LIST_PREPEND(*schema_results, entry);
 }
+
+bool str_in_list(const char *string, strList *list)
+{
+    strList *str_list = NULL;
+
+    if (!list) {
+        return true;
+    }
+    for (str_list = list; str_list; str_list = str_list->next) {
+        if (g_str_equal(string, str_list->value)) {
+            return true;
+        }
+    }
+    return false;
+}
diff --git a/qapi/stats.json b/qapi/stats.json
index bcc897258a..26ee69588f 100644
--- a/qapi/stats.json
+++ b/qapi/stats.json
@@ -65,6 +65,16 @@
 { 'enum': 'StatsTarget',
   'data': [ 'vm', 'vcpu' ] }
 
+##
+# @StatsVCPUFilter:
+#
+# @vcpus: list of qom paths for the desired vCPU objects.
+#
+# Since: 7.1
+##
+{ 'struct': 'StatsVCPUFilter',
+  'data': { '*vcpus': [ 'str' ] } }
+
 ##
 # @StatsFilter:
 #
@@ -73,8 +83,10 @@
 #
 # Since: 7.1
 ##
-{ 'struct': 'StatsFilter',
-  'data': { 'target': 'StatsTarget' } }
+{ 'union': 'StatsFilter',
+        'base': { 'target': 'StatsTarget' },
+  'discriminator': 'target',
+  'data': { 'vcpu': 'StatsVCPUFilter' } }
 
 ##
 # @StatsValue:
-- 
2.35.1

Re: [PATCH 3/8] qmp: add filtering of statistics by target vCPU

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Introduce a simple filtering of statistics, that allows to retrieve
> statistics for a subset of the guest vCPUs.  This will be used for
> example by the HMP monitor, in order to retrieve the statistics
> for the currently selected CPU.
>
> Example:
> { "execute": "query-stats",
>   "arguments": {
>     "target": "vcpu",
>     "vcpus": [ "/machine/unattached/device[2]",
>                "/machine/unattached/device[4]" ] } }

What heartless people put these poor vCPUs in the orphanage?

>
> Extracted from a patch by Mark Kanda.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

[...]

> diff --git a/qapi/stats.json b/qapi/stats.json
> index bcc897258a..26ee69588f 100644
> --- a/qapi/stats.json
> +++ b/qapi/stats.json
> @@ -65,6 +65,16 @@
>  { 'enum': 'StatsTarget',
>    'data': [ 'vm', 'vcpu' ] }
>  
> +##
> +# @StatsVCPUFilter:
> +#
> +# @vcpus: list of qom paths for the desired vCPU objects.

"QOM paths", because that's how we spell it elsewhere.

> +#
> +# Since: 7.1
> +##
> +{ 'struct': 'StatsVCPUFilter',
> +  'data': { '*vcpus': [ 'str' ] } }
> +
>  ##
>  # @StatsFilter:
>  #
> @@ -73,8 +83,10 @@
>  #
>  # Since: 7.1
>  ##
> -{ 'struct': 'StatsFilter',
> -  'data': { 'target': 'StatsTarget' } }
> +{ 'union': 'StatsFilter',
> +        'base': { 'target': 'StatsTarget' },
> +  'discriminator': 'target',
> +  'data': { 'vcpu': 'StatsVCPUFilter' } }
>  
>  ##
>  # @StatsValue:

Re: [PATCH 3/8] qmp: add filtering of statistics by target vCPU

[Paolo Bonzini]

On 5/5/22 15:45, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> Introduce a simple filtering of statistics, that allows to retrieve
>> statistics for a subset of the guest vCPUs.  This will be used for
>> example by the HMP monitor, in order to retrieve the statistics
>> for the currently selected CPU.
>>
>> Example:
>> { "execute": "query-stats",
>>    "arguments": {
>>      "target": "vcpu",
>>      "vcpus": [ "/machine/unattached/device[2]",
>>                 "/machine/unattached/device[4]" ] } }
> 
> What heartless people put these poor vCPUs in the orphanage?

Phil tried to fix it but the testsuites hung in weird ways.

Paolo

>>
>> Extracted from a patch by Mark Kanda.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> [...]
> 
>> diff --git a/qapi/stats.json b/qapi/stats.json
>> index bcc897258a..26ee69588f 100644
>> --- a/qapi/stats.json
>> +++ b/qapi/stats.json
>> @@ -65,6 +65,16 @@
>>   { 'enum': 'StatsTarget',
>>     'data': [ 'vm', 'vcpu' ] }
>>   
>> +##
>> +# @StatsVCPUFilter:
>> +#
>> +# @vcpus: list of qom paths for the desired vCPU objects.
> 
> "QOM paths", because that's how we spell it elsewhere.
> 
>> +#
>> +# Since: 7.1
>> +##
>> +{ 'struct': 'StatsVCPUFilter',
>> +  'data': { '*vcpus': [ 'str' ] } }
>> +
>>   ##
>>   # @StatsFilter:
>>   #
>> @@ -73,8 +83,10 @@
>>   #
>>   # Since: 7.1
>>   ##
>> -{ 'struct': 'StatsFilter',
>> -  'data': { 'target': 'StatsTarget' } }
>> +{ 'union': 'StatsFilter',
>> +        'base': { 'target': 'StatsTarget' },
>> +  'discriminator': 'target',
>> +  'data': { 'vcpu': 'StatsVCPUFilter' } }
>>   
>>   ##
>>   # @StatsValue:
> 
> 

[PATCH 4/8] hmp: add basic "info stats" implementation

[Paolo Bonzini]

From: Mark Kanda <mark.kanda@oracle.com>

Add an HMP command to retrieve statistics collected at run-time.
The command will retrieve and print either all VM-level statistics,
or all vCPU-level statistics for the currently selected CPU.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hmp-commands-info.hx  |  13 +++
 include/monitor/hmp.h |   1 +
 monitor/hmp-cmds.c    | 189 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 203 insertions(+)

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index adfa085a9b..221feab8c0 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -894,3 +894,16 @@ SRST
   ``info via``
     Show guest mos6522 VIA devices.
 ERST
+
+    {
+        .name       = "stats",
+        .args_type  = "target:s",
+        .params     = "target",
+        .help       = "show statistics; target is either vm or vcpu",
+        .cmd        = hmp_info_stats,
+    },
+
+SRST
+  ``stats``
+    Show runtime-collected statistics
+ERST
diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index 96d014826a..2e89a97bd6 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -133,5 +133,6 @@ void hmp_info_dirty_rate(Monitor *mon, const QDict *qdict);
 void hmp_calc_dirty_rate(Monitor *mon, const QDict *qdict);
 void hmp_human_readable_text_helper(Monitor *mon,
                                     HumanReadableText *(*qmp_handler)(Error **));
+void hmp_info_stats(Monitor *mon, const QDict *qdict);
 
 #endif
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index d8b98bed6c..1ac00ba124 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -40,6 +40,7 @@
 #include "qapi/qapi-commands-pci.h"
 #include "qapi/qapi-commands-rocker.h"
 #include "qapi/qapi-commands-run-state.h"
+#include "qapi/qapi-commands-stats.h"
 #include "qapi/qapi-commands-tpm.h"
 #include "qapi/qapi-commands-ui.h"
 #include "qapi/qapi-visit-net.h"
@@ -52,6 +53,7 @@
 #include "ui/console.h"
 #include "qemu/cutils.h"
 #include "qemu/error-report.h"
+#include "hw/core/cpu.h"
 #include "hw/intc/intc.h"
 #include "migration/snapshot.h"
 #include "migration/misc.h"
@@ -2223,3 +2225,190 @@ void hmp_info_memory_size_summary(Monitor *mon, const QDict *qdict)
     }
     hmp_handle_error(mon, err);
 }
+
+static void print_stats_schema_value(Monitor *mon, StatsSchemaValue *value)
+{
+    const char *prefix = "";
+    monitor_printf(mon, "    %s (%s", value->name, StatsType_str(value->type));
+
+    if (value->has_unit && value->unit == STATS_UNIT_SECONDS &&
+        (value->exponent == 0 || value->base == 10) &&
+        value->exponent >= -9 && value->exponent <= 0 &&
+        value->exponent % 3 == 0) {
+
+        static const char *si_prefix[] = { "", "milli", "micro", "nano" };
+        prefix = si_prefix[value->exponent / -3];
+
+    } else if (value->has_unit && value->unit == STATS_UNIT_BYTES &&
+               (value->exponent == 0 || value->base == 2) &&
+               value->exponent >= 0 && value->exponent <= 40 &&
+               value->exponent % 10 == 0) {
+
+        static const char *si_prefix[] = {
+            "", "kilo", "mega", "giga", "tera" };
+        prefix = si_prefix[value->exponent / 10];
+
+    } else if (value->exponent) {
+        /* Print the base and exponent as "x <base>^<exp>" */
+        monitor_printf(mon, ", * %d^%d", value->base,
+                       value->exponent);
+    }
+
+    if (value->has_unit) {
+        monitor_printf(mon, " %s%s", prefix, StatsUnit_str(value->unit));
+    }
+
+    /* Print bucket size for linear histograms */
+    if (value->type == STATS_TYPE_LINEAR_HIST && value->has_bucket_size) {
+        monitor_printf(mon, ", bucket size=%d", value->bucket_size);
+    }
+    monitor_printf(mon, ")");
+}
+
+static StatsSchemaValueList *find_schema_value_list(
+    StatsSchemaList *list, StatsProvider provider,
+    StatsTarget target)
+{
+    StatsSchemaList *node;
+
+    for (node = list; node; node = node->next) {
+        if (node->value->provider == provider &&
+            node->value->target == target) {
+            return node->value->stats;
+        }
+    }
+    return NULL;
+}
+
+static void print_stats_results(Monitor *mon, StatsTarget target,
+                                StatsResult *result,
+                                StatsSchemaList *schema)
+{
+    /* Find provider schema */
+    StatsSchemaValueList *schema_value_list =
+        find_schema_value_list(schema, result->provider, target);
+    StatsList *stats_list;
+
+    if (!schema_value_list) {
+        monitor_printf(mon, "failed to find schema list for %s\n",
+                       StatsProvider_str(result->provider));
+        return;
+    }
+
+    monitor_printf(mon, "provider: %s\n",
+                   StatsProvider_str(result->provider));
+
+    for (stats_list = result->stats; stats_list;
+             stats_list = stats_list->next,
+             schema_value_list = schema_value_list->next) {
+
+        Stats *stats = stats_list->value;
+        StatsValue *stats_value = stats->value;
+        StatsSchemaValue *schema_value = schema_value_list->value;
+
+        /* Find schema entry */
+        while (!g_str_equal(stats->name, schema_value->name)) {
+            if (!schema_value_list->next) {
+                monitor_printf(mon, "failed to find schema entry for %s\n",
+                               stats->name);
+                return;
+            }
+            schema_value_list = schema_value_list->next;
+            schema_value = schema_value_list->value;
+        }
+
+        print_stats_schema_value(mon, schema_value);
+
+        if (stats_value->type == QTYPE_QNUM) {
+            monitor_printf(mon, ": %" PRId64 "\n", stats_value->u.scalar);
+        } else if (stats_value->type == QTYPE_QLIST) {
+            uint64List *list;
+            int i;
+
+            monitor_printf(mon, ": ");
+            for (list = stats_value->u.list, i = 1;
+                 list;
+                 list = list->next, i++) {
+                monitor_printf(mon, "[%d]=%" PRId64 " ", i, list->value);
+            }
+            monitor_printf(mon, "\n");
+        }
+    }
+}
+
+/* Create the StatsFilter that is needed for an "info stats" invocation.  */
+static StatsFilter *stats_filter(StatsTarget target, int cpu_index)
+{
+    StatsFilter *filter = g_malloc0(sizeof(*filter));
+
+    filter->target = target;
+    switch (target) {
+    case STATS_TARGET_VM:
+        break;
+    case STATS_TARGET_VCPU:
+    {
+        strList *vcpu_list = NULL;
+        CPUState *cpu = qemu_get_cpu(cpu_index);
+        char *canonical_path = object_get_canonical_path(OBJECT(cpu));
+
+        QAPI_LIST_PREPEND(vcpu_list, canonical_path);
+        filter->u.vcpu.has_vcpus = true;
+        filter->u.vcpu.vcpus = vcpu_list;
+        break;
+    }
+    default:
+        break;
+    }
+    return filter;
+}
+
+void hmp_info_stats(Monitor *mon, const QDict *qdict)
+{
+    const char *target_str = qdict_get_str(qdict, "target");
+    StatsTarget target;
+    Error *err = NULL;
+    StatsResultList *stats = NULL;
+    StatsSchemaList *schema = NULL;
+    StatsFilter *filter = NULL;
+
+    target = qapi_enum_parse(&StatsTarget_lookup, target_str, -1, &err);
+    if (err) {
+        monitor_printf(mon, "invalid stats target %s\n", target_str);
+        goto exit_no_print;
+    }
+
+    schema = qmp_query_stats_schemas(&err);
+    if (err) {
+        goto exit;
+    }
+
+    switch (target) {
+    case STATS_TARGET_VM:
+        filter = stats_filter(target, -1);
+       break;
+    case STATS_TARGET_VCPU: {}
+        int cpu_index = monitor_get_cpu_index(mon);
+        filter = stats_filter(target, cpu_index);
+        break;
+    default:
+        abort();
+    }
+
+    stats = qmp_query_stats(filter, &err);
+    if (err) {
+        goto exit;
+    }
+    if (stats) {
+        print_stats_results(mon, target, stats->value, schema);
+    }
+    qapi_free_StatsFilter(filter);
+    qapi_free_StatsSchemaList(schema);
+    qapi_free_StatsResultList(stats);
+
+exit:
+    if (err) {
+        monitor_printf(mon, "%s\n", error_get_pretty(err));
+    }
+exit_no_print:
+    error_free(err);
+}
-- 
2.35.1

[PATCH 5/8] qmp: add filtering of statistics by provider

[Paolo Bonzini]

Allow retrieving the statistics from a specific provider only.
This can be used in the future by HMP commands such as "info
sync-profile" or "info profile".  The next patch also adds
filter-by-provider capabilities to the HMP equivalent of
query-stats, "info stats".

Example:

{ "execute": "query-stats",
  "arguments": {
    "target": "vm",
    "providers": [
      { "provider": "kvm" } ] } }

The QAPI is a bit more verbose than just a list of StatsProvider,
so that it can be subsequently extended with filtering of statistics
by name.

Extracted from a patch by Mark Kanda.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 accel/kvm/kvm-all.c     |  3 ++-
 include/monitor/stats.h |  4 +++-
 monitor/hmp-cmds.c      |  2 +-
 monitor/qmp-cmds.c      | 33 +++++++++++++++++++++++++++++----
 qapi/stats.json         | 16 ++++++++++++++--
 5 files changed, 49 insertions(+), 9 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index bfd81039a1..b42008ac07 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2644,7 +2644,8 @@ static int kvm_init(MachineState *ms)
     }
 
     if (kvm_check_extension(kvm_state, KVM_CAP_BINARY_STATS_FD)) {
-        add_stats_callbacks(query_stats_cb, query_stats_schemas_cb);
+        add_stats_callbacks(STATS_PROVIDER_KVM, query_stats_cb,
+                            query_stats_schemas_cb);
     }
 
     return 0;
diff --git a/include/monitor/stats.h b/include/monitor/stats.h
index 92a1df3072..acfd975df9 100644
--- a/include/monitor/stats.h
+++ b/include/monitor/stats.h
@@ -17,10 +17,12 @@ typedef void SchemaRetrieveFunc(StatsSchemaList **result, Error **errp);
 /*
  * Register callbacks for the QMP query-stats command.
  *
+ * @provider: stats provider
  * @stats_fn: routine to query stats:
  * @schema_fn: routine to query stat schemas:
  */
-void add_stats_callbacks(StatRetrieveFunc *stats_fn,
+void add_stats_callbacks(StatsProvider provider,
+                         StatRetrieveFunc *stats_fn,
                          SchemaRetrieveFunc *schemas_fn);
 
 /*
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 1ac00ba124..2085d88e7d 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -2377,7 +2377,7 @@ void hmp_info_stats(Monitor *mon, const QDict *qdict)
         goto exit_no_print;
     }
 
-    schema = qmp_query_stats_schemas(&err);
+    schema = qmp_query_stats_schemas(false, 0, &err);
     if (err) {
         goto exit;
     }
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 5e3f4c9685..25962d8bb4 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -430,6 +430,7 @@ HumanReadableText *qmp_x_query_irq(Error **errp)
 }
 
 typedef struct StatsCallbacks {
+    StatsProvider provider;
     StatRetrieveFunc *stats_cb;
     SchemaRetrieveFunc *schemas_cb;
     QTAILQ_ENTRY(StatsCallbacks) next;
@@ -438,10 +439,12 @@ typedef struct StatsCallbacks {
 static QTAILQ_HEAD(, StatsCallbacks) stats_callbacks =
     QTAILQ_HEAD_INITIALIZER(stats_callbacks);
 
-void add_stats_callbacks(StatRetrieveFunc *stats_fn,
+void add_stats_callbacks(StatsProvider provider,
+                         StatRetrieveFunc *stats_fn,
                          SchemaRetrieveFunc *schemas_fn)
 {
     StatsCallbacks *entry = g_new(StatsCallbacks, 1);
+    entry->provider = provider;
     entry->stats_cb = stats_fn;
     entry->schemas_cb = schemas_fn;
 
@@ -464,6 +467,22 @@ static strList *stats_target_filter(StatsFilter *filter)
     }
 }
 
+static bool stats_provider_requested(StatsProvider provider,
+                                     StatsFilter *filter)
+{
+    StatsRequestList *request;
+
+    if (!filter->has_providers) {
+        return true;
+    }
+    for (request = filter->providers; request; request = request->next) {
+        if (request->value->provider == provider) {
+            return true;
+        }
+    }
+    return false;
+}
+
 StatsResultList *qmp_query_stats(StatsFilter *filter, Error **errp)
 {
     StatsResultList *stats_results = NULL;
@@ -471,19 +490,25 @@ StatsResultList *qmp_query_stats(StatsFilter *filter, Error **errp)
     StatsCallbacks *entry;
 
     QTAILQ_FOREACH(entry, &stats_callbacks, next) {
-        entry->stats_cb(&stats_results, filter->target, targets, errp);
+        if (stats_provider_requested(entry->provider, filter)) {
+            entry->stats_cb(&stats_results, filter->target, targets, errp);
+        }
     }
 
     return stats_results;
 }
 
-StatsSchemaList *qmp_query_stats_schemas(Error **errp)
+StatsSchemaList *qmp_query_stats_schemas(bool has_provider,
+                                         StatsProvider provider,
+                                         Error **errp)
 {
     StatsSchemaList *stats_results = NULL;
     StatsCallbacks *entry;
 
     QTAILQ_FOREACH(entry, &stats_callbacks, next) {
-        entry->schemas_cb(&stats_results, errp);
+        if (!has_provider || provider == entry->provider) {
+            entry->schemas_cb(&stats_results, errp);
+        }
     }
 
     return stats_results;
diff --git a/qapi/stats.json b/qapi/stats.json
index 26ee69588f..33ff6ea7a9 100644
--- a/qapi/stats.json
+++ b/qapi/stats.json
@@ -65,6 +65,18 @@
 { 'enum': 'StatsTarget',
   'data': [ 'vm', 'vcpu' ] }
 
+##
+# @StatsRequest:
+#
+# Indicates a set of statistics that should be returned by query-stats.
+#
+# @provider: provider for which to return statistics.
+#
+# Since: 7.1
+##
+{ 'struct': 'StatsRequest',
+  'data': { 'provider': 'StatsProvider' } }
+
 ##
 # @StatsVCPUFilter:
 #
@@ -84,7 +96,7 @@
 # Since: 7.1
 ##
 { 'union': 'StatsFilter',
-        'base': { 'target': 'StatsTarget' },
+        'base': { 'target': 'StatsTarget', '*providers': [ 'StatsRequest' ] },
   'discriminator': 'target',
   'data': { 'vcpu': 'StatsVCPUFilter' } }
 
@@ -200,5 +212,5 @@
 # Since: 7.1
 ##
 { 'command': 'query-stats-schemas',
-  'data': { },
+  'data': { '*provider': 'StatsProvider' },
   'returns': [ 'StatsSchema' ] }
-- 
2.35.1

[PATCH 6/8] hmp: add filtering of statistics by provider

[Paolo Bonzini]

Allow the user to request statistics for a single provider of interest.
Extracted from a patch by Mark Kanda.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hmp-commands-info.hx |  7 ++++---
 monitor/hmp-cmds.c   | 46 +++++++++++++++++++++++++++++++++++---------
 2 files changed, 41 insertions(+), 12 deletions(-)

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index 221feab8c0..1a3e16175f 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -897,9 +897,10 @@ ERST
 
     {
         .name       = "stats",
-        .args_type  = "target:s",
-        .params     = "target",
-        .help       = "show statistics; target is either vm or vcpu",
+        .args_type  = "target:s,provider:s?",
+        .params     = "target [provider]",
+        .help       = "show statistics for the given target (vm or vcpu); optionally filter by "
+	              "provider",
         .cmd        = hmp_info_stats,
     },
 
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 2085d88e7d..2bce9d7016 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -2281,6 +2281,7 @@ static StatsSchemaValueList *find_schema_value_list(
 }
 
 static void print_stats_results(Monitor *mon, StatsTarget target,
+                                bool show_provider,
                                 StatsResult *result,
                                 StatsSchemaList *schema)
 {
@@ -2295,8 +2296,10 @@ static void print_stats_results(Monitor *mon, StatsTarget target,
         return;
     }
 
-    monitor_printf(mon, "provider: %s\n",
-                   StatsProvider_str(result->provider));
+    if (show_provider) {
+        monitor_printf(mon, "provider: %s\n",
+                       StatsProvider_str(result->provider));
+    }
 
     for (stats_list = result->stats; stats_list;
              stats_list = stats_list->next,
@@ -2337,7 +2340,8 @@ static void print_stats_results(Monitor *mon, StatsTarget target,
 }
 
 /* Create the StatsFilter that is needed for an "info stats" invocation.  */
-static StatsFilter *stats_filter(StatsTarget target, int cpu_index)
+static StatsFilter *stats_filter(StatsTarget target, int cpu_index,
+                                 StatsProvider provider)
 {
     StatsFilter *filter = g_malloc0(sizeof(*filter));
 
@@ -2359,15 +2363,31 @@ static StatsFilter *stats_filter(StatsTarget target, int cpu_index)
     default:
         break;
     }
+
+    if (provider == STATS_PROVIDER__MAX) {
+        return filter;
+    }
+
+    /* "info stats" can only query either one or all the providers.  */
+    StatsRequest *request = g_new0(StatsRequest, 1);
+    request->provider = provider;
+    StatsRequestList *request_list = g_new0(StatsRequestList, 1);
+    QAPI_LIST_PREPEND(request_list, request);
+    filter->has_providers = true;
+    filter->providers = request_list;
     return filter;
 }
 
 void hmp_info_stats(Monitor *mon, const QDict *qdict)
 {
     const char *target_str = qdict_get_str(qdict, "target");
+    const char *provider_str = qdict_get_try_str(qdict, "provider");
+
+    StatsProvider provider = STATS_PROVIDER__MAX;
     StatsTarget target;
     Error *err = NULL;
     StatsResultList *stats = NULL;
+    StatsResultList *entry;
     StatsSchemaList *schema = NULL;
     StatsFilter *filter = NULL;
 
@@ -2376,19 +2396,27 @@ void hmp_info_stats(Monitor *mon, const QDict *qdict)
         monitor_printf(mon, "invalid stats target %s\n", target_str);
         goto exit_no_print;
     }
+    if (provider_str) {
+        provider = qapi_enum_parse(&StatsProvider_lookup, provider_str, -1, &err);
+        if (err) {
+            monitor_printf(mon, "invalid stats provider %s\n", provider_str);
+            goto exit_no_print;
+        }
+    }
 
-    schema = qmp_query_stats_schemas(false, 0, &err);
+    schema = qmp_query_stats_schemas(provider_str ? true : false,
+                                     provider, &err);
     if (err) {
         goto exit;
     }
 
     switch (target) {
     case STATS_TARGET_VM:
-        filter = stats_filter(target, -1);
-       break;
+        filter = stats_filter(target, -1, provider);
+        break;
     case STATS_TARGET_VCPU: {}
         int cpu_index = monitor_get_cpu_index(mon);
-        filter = stats_filter(target, cpu_index);
+        filter = stats_filter(target, cpu_index, provider);
         break;
     default:
         abort();
@@ -2398,8 +2426,8 @@ void hmp_info_stats(Monitor *mon, const QDict *qdict)
     if (err) {
         goto exit;
     }
-    if (stats) {
-        print_stats_results(mon, target, stats->value, schema);
+    for (entry = stats; entry; entry = entry->next) {
+        print_stats_results(mon, target, provider_str == NULL, entry->value, schema);
     }
     qapi_free_StatsFilter(filter);
     qapi_free_StatsSchemaList(schema);
-- 
2.35.1

[PATCH 7/8] qmp: add filtering of statistics by name

[Paolo Bonzini]

Allow retrieving only a subset of statistics.  This can be useful
for example in order to plot a subset of the statistics many times
a second.

KVM publishes ~40 statistics for each vCPU on x86; retrieving and
serializing all of them would be useless

Another use will be in HMP in the following patch; implementing the
filter in the backend is easy enough that it was deemed okay to make
this a public interface.

Example:

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

{ "return": {
    "vcpus": [
      { "path": "/machine/unattached/device[2]"
        "providers": [
          { "provider": "kvm",
            "stats": [ { "name": "l1d_flush", "value": 41213 },
                       { "name": "exits", "value": 74291 } ] } ] },
      { "path": "/machine/unattached/device[4]"
        "providers": [
          { "provider": "kvm",
            "stats": [ { "name": "l1d_flush", "value": 16132 },
                       { "name": "exits", "value": 57922 } ] } ] } ] } }

Extracted from a patch by Mark Kanda.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 accel/kvm/kvm-all.c     | 18 +++++++++++-------
 include/monitor/stats.h |  4 ++--
 monitor/qmp-cmds.c      | 10 +++++++---
 qapi/stats.json         |  4 +++-
 4 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index b42008ac07..67253c5a5c 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2311,7 +2311,7 @@ bool kvm_dirty_ring_enabled(void)
     return kvm_state->kvm_dirty_ring_size ? true : false;
 }
 
-static void query_stats_cb(StatsResultList **result, StatsTarget target,
+static void query_stats_cb(StatsResultList **result, StatsTarget target, strList *names,
                            strList *targets, Error **errp);
 static void query_stats_schemas_cb(StatsSchemaList **result, Error **errp);
 
@@ -3713,6 +3713,7 @@ typedef struct StatsArgs {
         StatsResultList **stats;
         StatsSchemaList **schema;
     } result;
+    strList *names;
     Error **errp;
 } StatsArgs;
 
@@ -3926,7 +3927,7 @@ static StatsDescriptors *find_stats_descriptors(StatsTarget target, int stats_fd
     return descriptors;
 }
 
-static void query_stats(StatsResultList **result, StatsTarget target,
+static void query_stats(StatsResultList **result, StatsTarget target, strList *names,
                         int stats_fd, Error **errp)
 {
     struct kvm_stats_desc *kvm_stats_desc;
@@ -3969,6 +3970,9 @@ static void query_stats(StatsResultList **result, StatsTarget target,
 
         /* Add entry to the list */
         stats = (void *)stats_data + pdesc->offset;
+        if (!str_in_list(pdesc->name, names)) {
+            continue;
+        }
         stats_list = add_kvmstat_entry(pdesc, stats, stats_list, errp);
     }
 
@@ -4030,8 +4034,8 @@ static void query_stats_vcpu(CPUState *cpu, run_on_cpu_data data)
         error_propagate(kvm_stats_args->errp, local_err);
         return;
     }
-    query_stats(kvm_stats_args->result.stats, STATS_TARGET_VCPU, stats_fd,
-                kvm_stats_args->errp);
+    query_stats(kvm_stats_args->result.stats, STATS_TARGET_VCPU,
+                kvm_stats_args->names, stats_fd, kvm_stats_args->errp);
     close(stats_fd);
 }
 
@@ -4052,7 +4056,7 @@ static void query_stats_schema_vcpu(CPUState *cpu, run_on_cpu_data data)
 }
 
 static void query_stats_cb(StatsResultList **result, StatsTarget target,
-                           strList *targets, Error **errp)
+                           strList *names, strList *targets, Error **errp)
 {
     KVMState *s = kvm_state;
     CPUState *cpu;
@@ -4066,14 +4070,15 @@ static void query_stats_cb(StatsResultList **result, StatsTarget target,
             error_setg(errp, "KVM stats: ioctl failed");
             return;
         }
-        query_stats(result, target, stats_fd, errp);
+        query_stats(result, target, names, stats_fd, errp);
         close(stats_fd);
         break;
     }
     case STATS_TARGET_VCPU:
     {
         StatsArgs stats_args;
         stats_args.result.stats = result;
+        stats_args.names = names;
         stats_args.errp = errp;
         CPU_FOREACH(cpu) {
             if (!str_in_list(cpu->parent_obj.canonical_path, targets)) {
diff --git a/include/monitor/stats.h b/include/monitor/stats.h
index acfd975df9..b4123044f7 100644
--- a/include/monitor/stats.h
+++ b/include/monitor/stats.h
@@ -11,8 +11,8 @@
 #include "qapi/qapi-types-stats.h"
 
 typedef void StatRetrieveFunc(StatsResultList **result, StatsTarget target,
-                              strList *targets, Error **errp);
-typedef void SchemaRetrieveFunc(StatsSchemaList **result, Error **errp);
+                              strList *names, strList *targets, Error **errp);
+typedef void SchemaRetrieveFunc(StatsSchemaList **, Error **);
 
 /*
  * Register callbacks for the QMP query-stats command.
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 25962d8bb4..d0fdb17c82 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -468,15 +468,18 @@ static strList *stats_target_filter(StatsFilter *filter)
 }
 
 static bool stats_provider_requested(StatsProvider provider,
-                                     StatsFilter *filter)
+                                     StatsFilter *filter,
+                                     strList **p_names)
 {
     StatsRequestList *request;
 
     if (!filter->has_providers) {
+        *p_names = NULL;
         return true;
     }
     for (request = filter->providers; request; request = request->next) {
         if (request->value->provider == provider) {
+            *p_names = request->value->has_names ? request->value->names : NULL;
             return true;
         }
     }
@@ -490,8 +493,9 @@ StatsResultList *qmp_query_stats(StatsFilter *filter, Error **errp)
     StatsCallbacks *entry;
 
     QTAILQ_FOREACH(entry, &stats_callbacks, next) {
-        if (stats_provider_requested(entry->provider, filter)) {
-            entry->stats_cb(&stats_results, filter->target, targets, errp);
+        strList *names = NULL;
+        if (stats_provider_requested(entry->provider, filter, &names)) {
+            entry->stats_cb(&stats_results, filter->target, names, targets, errp);
         }
     }
 
diff --git a/qapi/stats.json b/qapi/stats.json
index 33ff6ea7a9..234fbcb7ca 100644
--- a/qapi/stats.json
+++ b/qapi/stats.json
@@ -71,11 +71,14 @@
 # Indicates a set of statistics that should be returned by query-stats.
 #
 # @provider: provider for which to return statistics.
+
+# @names: statistics to be returned (all if omitted).
 #
 # Since: 7.1
 ##
 { 'struct': 'StatsRequest',
-  'data': { 'provider': 'StatsProvider' } }
+  'data': { 'provider': 'StatsProvider',
+            '*names': [ 'str' ] } }
 
 ##
 # @StatsVCPUFilter:
-- 
2.35.1

Re: [PATCH 7/8] qmp: add filtering of statistics by name

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> Allow retrieving only a subset of statistics.  This can be useful
> for example in order to plot a subset of the statistics many times
> a second.
> 
> KVM publishes ~40 statistics for each vCPU on x86; retrieving and
> serializing all of them would be useless
> 
> Another use will be in HMP in the following patch; implementing the
> filter in the backend is easy enough that it was deemed okay to make
> this a public interface.
> 
> Example:
> 
> { "execute": "query-stats",
>   "arguments": {
>     "target": "vcpu",
>     "vcpus": [ "/machine/unattached/device[2]",
>                "/machine/unattached/device[4]" ],
>     "providers": [
>       { "provider": "kvm",
>         "names": [ "l1d_flush", "exits" ] } } }

That looks inconsistent to me; I realise that 'names' has to be a child
of providers (since the names are only relevant to a given provider) but
how about making the "target" work similarly:

  
{ "execute": "query-stats",
  "arguments": {
    "target": {
      "vcpus": [ "/machine/unattached/device[2]",
                 "/machine/unattached/device[4]" ] },
   
    "providers": [
       { "provider": "kvm",
         "names": [ "l1d_flush", "exits" ] } } }

It's not clear to me whether the "target" should also be specific
to a given provider.

Dave

> { "return": {
>     "vcpus": [
>       { "path": "/machine/unattached/device[2]"
>         "providers": [
>           { "provider": "kvm",
>             "stats": [ { "name": "l1d_flush", "value": 41213 },
>                        { "name": "exits", "value": 74291 } ] } ] },
>       { "path": "/machine/unattached/device[4]"
>         "providers": [
>           { "provider": "kvm",
>             "stats": [ { "name": "l1d_flush", "value": 16132 },
>                        { "name": "exits", "value": 57922 } ] } ] } ] } }
> 
> Extracted from a patch by Mark Kanda.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  accel/kvm/kvm-all.c     | 18 +++++++++++-------
>  include/monitor/stats.h |  4 ++--
>  monitor/qmp-cmds.c      | 10 +++++++---
>  qapi/stats.json         |  4 +++-
>  4 files changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index b42008ac07..67253c5a5c 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2311,7 +2311,7 @@ bool kvm_dirty_ring_enabled(void)
>      return kvm_state->kvm_dirty_ring_size ? true : false;
>  }
>  
> -static void query_stats_cb(StatsResultList **result, StatsTarget target,
> +static void query_stats_cb(StatsResultList **result, StatsTarget target, strList *names,
>                             strList *targets, Error **errp);
>  static void query_stats_schemas_cb(StatsSchemaList **result, Error **errp);
>  
> @@ -3713,6 +3713,7 @@ typedef struct StatsArgs {
>          StatsResultList **stats;
>          StatsSchemaList **schema;
>      } result;
> +    strList *names;
>      Error **errp;
>  } StatsArgs;
>  
> @@ -3926,7 +3927,7 @@ static StatsDescriptors *find_stats_descriptors(StatsTarget target, int stats_fd
>      return descriptors;
>  }
>  
> -static void query_stats(StatsResultList **result, StatsTarget target,
> +static void query_stats(StatsResultList **result, StatsTarget target, strList *names,
>                          int stats_fd, Error **errp)
>  {
>      struct kvm_stats_desc *kvm_stats_desc;
> @@ -3969,6 +3970,9 @@ static void query_stats(StatsResultList **result, StatsTarget target,
>  
>          /* Add entry to the list */
>          stats = (void *)stats_data + pdesc->offset;
> +        if (!str_in_list(pdesc->name, names)) {
> +            continue;
> +        }
>          stats_list = add_kvmstat_entry(pdesc, stats, stats_list, errp);
>      }
>  
> @@ -4030,8 +4034,8 @@ static void query_stats_vcpu(CPUState *cpu, run_on_cpu_data data)
>          error_propagate(kvm_stats_args->errp, local_err);
>          return;
>      }
> -    query_stats(kvm_stats_args->result.stats, STATS_TARGET_VCPU, stats_fd,
> -                kvm_stats_args->errp);
> +    query_stats(kvm_stats_args->result.stats, STATS_TARGET_VCPU,
> +                kvm_stats_args->names, stats_fd, kvm_stats_args->errp);
>      close(stats_fd);
>  }
>  
> @@ -4052,7 +4056,7 @@ static void query_stats_schema_vcpu(CPUState *cpu, run_on_cpu_data data)
>  }
>  
>  static void query_stats_cb(StatsResultList **result, StatsTarget target,
> -                           strList *targets, Error **errp)
> +                           strList *names, strList *targets, Error **errp)
>  {
>      KVMState *s = kvm_state;
>      CPUState *cpu;
> @@ -4066,14 +4070,15 @@ static void query_stats_cb(StatsResultList **result, StatsTarget target,
>              error_setg(errp, "KVM stats: ioctl failed");
>              return;
>          }
> -        query_stats(result, target, stats_fd, errp);
> +        query_stats(result, target, names, stats_fd, errp);
>          close(stats_fd);
>          break;
>      }
>      case STATS_TARGET_VCPU:
>      {
>          StatsArgs stats_args;
>          stats_args.result.stats = result;
> +        stats_args.names = names;
>          stats_args.errp = errp;
>          CPU_FOREACH(cpu) {
>              if (!str_in_list(cpu->parent_obj.canonical_path, targets)) {
> diff --git a/include/monitor/stats.h b/include/monitor/stats.h
> index acfd975df9..b4123044f7 100644
> --- a/include/monitor/stats.h
> +++ b/include/monitor/stats.h
> @@ -11,8 +11,8 @@
>  #include "qapi/qapi-types-stats.h"
>  
>  typedef void StatRetrieveFunc(StatsResultList **result, StatsTarget target,
> -                              strList *targets, Error **errp);
> -typedef void SchemaRetrieveFunc(StatsSchemaList **result, Error **errp);
> +                              strList *names, strList *targets, Error **errp);
> +typedef void SchemaRetrieveFunc(StatsSchemaList **, Error **);
>  
>  /*
>   * Register callbacks for the QMP query-stats command.
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 25962d8bb4..d0fdb17c82 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -468,15 +468,18 @@ static strList *stats_target_filter(StatsFilter *filter)
>  }
>  
>  static bool stats_provider_requested(StatsProvider provider,
> -                                     StatsFilter *filter)
> +                                     StatsFilter *filter,
> +                                     strList **p_names)
>  {
>      StatsRequestList *request;
>  
>      if (!filter->has_providers) {
> +        *p_names = NULL;
>          return true;
>      }
>      for (request = filter->providers; request; request = request->next) {
>          if (request->value->provider == provider) {
> +            *p_names = request->value->has_names ? request->value->names : NULL;
>              return true;
>          }
>      }
> @@ -490,8 +493,9 @@ StatsResultList *qmp_query_stats(StatsFilter *filter, Error **errp)
>      StatsCallbacks *entry;
>  
>      QTAILQ_FOREACH(entry, &stats_callbacks, next) {
> -        if (stats_provider_requested(entry->provider, filter)) {
> -            entry->stats_cb(&stats_results, filter->target, targets, errp);
> +        strList *names = NULL;
> +        if (stats_provider_requested(entry->provider, filter, &names)) {
> +            entry->stats_cb(&stats_results, filter->target, names, targets, errp);
>          }
>      }
>  
> diff --git a/qapi/stats.json b/qapi/stats.json
> index 33ff6ea7a9..234fbcb7ca 100644
> --- a/qapi/stats.json
> +++ b/qapi/stats.json
> @@ -71,11 +71,14 @@
>  # Indicates a set of statistics that should be returned by query-stats.
>  #
>  # @provider: provider for which to return statistics.
> +
> +# @names: statistics to be returned (all if omitted).
>  #
>  # Since: 7.1
>  ##
>  { 'struct': 'StatsRequest',
> -  'data': { 'provider': 'StatsProvider' } }
> +  'data': { 'provider': 'StatsProvider',
> +            '*names': [ 'str' ] } }
>  
>  ##
>  # @StatsVCPUFilter:
> -- 
> 2.35.1
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 7/8] qmp: add filtering of statistics by name

[Paolo Bonzini]

On 4/27/22 14:01, Dr. David Alan Gilbert wrote:
>>      "providers": [
>>        { "provider": "kvm",
>>          "names": [ "l1d_flush", "exits" ] } } }
> That looks inconsistent to me; I realise that 'names' has to be a child
> of providers (since the names are only relevant to a given provider) but
> how about making the "target" work similarly:
> 
> { "execute": "query-stats",
>    "arguments": {
>      "target": {
>        "vcpus": [ "/machine/unattached/device[2]",
>                   "/machine/unattached/device[4]" ] },

This would allow queries for different types of targets in a single 
command, but it would be harder for the client to do filtering:

* with something like "target": { "vcpus": [ array ] }, there is no way 
for the client to say "I want this one statistic, but for all the vCPUs"

* if target is optional, a client might expect relatively small output 
from today's QEMU, yet suddenly get hundreds of KB from targets other 
than VMs and vCPUs (e.g. block devices including all backing files in 
the chain, NIC backends, etc.).

>      "providers": [
>         { "provider": "kvm",
>           "names": [ "l1d_flush", "exits" ] } } }
> 
> It's not clear to me whether the "target" should also be specific
> to a given provider.

No, the target is a QEMU concept, such as a CPU or a device backend.  It 
is identified by either a QOM path or a unique id.

Paolo

Re: [PATCH 7/8] qmp: add filtering of statistics by name

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> On 4/27/22 14:01, Dr. David Alan Gilbert wrote:
> > >      "providers": [
> > >        { "provider": "kvm",
> > >          "names": [ "l1d_flush", "exits" ] } } }
> > That looks inconsistent to me; I realise that 'names' has to be a child
> > of providers (since the names are only relevant to a given provider) but
> > how about making the "target" work similarly:
> > 
> > { "execute": "query-stats",
> >    "arguments": {
> >      "target": {
> >        "vcpus": [ "/machine/unattached/device[2]",
> >                   "/machine/unattached/device[4]" ] },
> 
> This would allow queries for different types of targets in a single command,
> but it would be harder for the client to do filtering:
> 
> * with something like "target": { "vcpus": [ array ] }, there is no way for
> the client to say "I want this one statistic, but for all the vCPUs"
> 
> * if target is optional, a client might expect relatively small output from
> today's QEMU, yet suddenly get hundreds of KB from targets other than VMs
> and vCPUs (e.g. block devices including all backing files in the chain, NIC
> backends, etc.).

If I specify a 'vm' it's not obvious to me whether I'd get NICs and
block devices in the future?
Adding a syntax for 'all' into the vcpus list would fix that?

> >      "providers": [
> >         { "provider": "kvm",
> >           "names": [ "l1d_flush", "exits" ] } } }
> > 
> > It's not clear to me whether the "target" should also be specific
> > to a given provider.
> 
> No, the target is a QEMU concept, such as a CPU or a device backend.  It is
> identified by either a QOM path or a unique id.

But doesn't 'kvm' as a provider only make sense for vcpus and VMs; if
you're imagining block devices and other things as targets it would seem
wrong to have that set of providers separate.

Dave

> Paolo
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 7/8] qmp: add filtering of statistics by name

[Paolo Bonzini]

On 4/27/22 14:34, Dr. David Alan Gilbert wrote:
> If I specify a 'vm' it's not obvious to me whether I'd get NICs and
> block devices in the future?

VM would not get those (it's global statistics), but the size could 
balloon if you specify no target at all.

> Adding a syntax for 'all' into the vcpus list would fix that?

I don't like having special syntax.  The current QAPI just doesn't 
filter what is not in the arguments.

>>>       "providers": [
>>>          { "provider": "kvm",
>>>            "names": [ "l1d_flush", "exits" ] } } }
>>>
>>> It's not clear to me whether the "target" should also be specific
>>> to a given provider.
>>
>> No, the target is a QEMU concept, such as a CPU or a device backend.  It is
>> identified by either a QOM path or a unique id.
> 
> But doesn't 'kvm' as a provider only make sense for vcpus and VMs; if
> you're imagining block devices and other things as targets it would seem
> wrong to have that set of providers separate.

Yes, those would have different providers.  But a single target can 
support multiple providers.

Paolo

Re: [PATCH 7/8] qmp: add filtering of statistics by name

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> On 4/27/22 14:34, Dr. David Alan Gilbert wrote:
> > If I specify a 'vm' it's not obvious to me whether I'd get NICs and
> > block devices in the future?
> 
> VM would not get those (it's global statistics), but the size could balloon
> if you specify no target at all.
> 
> > Adding a syntax for 'all' into the vcpus list would fix that?
> 
> I don't like having special syntax.  The current QAPI just doesn't filter
> what is not in the arguments.

Is there a object that represents the set of all vcpus?

> > > >       "providers": [
> > > >          { "provider": "kvm",
> > > >            "names": [ "l1d_flush", "exits" ] } } }
> > > > 
> > > > It's not clear to me whether the "target" should also be specific
> > > > to a given provider.
> > > 
> > > No, the target is a QEMU concept, such as a CPU or a device backend.  It is
> > > identified by either a QOM path or a unique id.
> > 
> > But doesn't 'kvm' as a provider only make sense for vcpus and VMs; if
> > you're imagining block devices and other things as targets it would seem
> > wrong to have that set of providers separate.
> 
> Yes, those would have different providers.  But a single target can support
> multiple providers.

Is that just for different implementations - kvm/hcf/tcg etc or do you
envisage multiple providers on an object in a running VM?

Dave
> Paolo
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 7/8] qmp: add filtering of statistics by name

[Paolo Bonzini]

On 4/27/22 17:16, Dr. David Alan Gilbert wrote:
> * Paolo Bonzini (pbonzini@redhat.com) wrote:
>> On 4/27/22 14:34, Dr. David Alan Gilbert wrote:
>>> If I specify a 'vm' it's not obvious to me whether I'd get NICs and
>>> block devices in the future?
>>
>> VM would not get those (it's global statistics), but the size could balloon
>> if you specify no target at all.
>>
>>> Adding a syntax for 'all' into the vcpus list would fix that?
>>
>> I don't like having special syntax.  The current QAPI just doesn't filter
>> what is not in the arguments.
> 
> Is there a object that represents the set of all vcpus?

No.

>> Yes, those would have different providers.  But a single target can support
>> multiple providers.
> 
> Is that just for different implementations - kvm/hcf/tcg etc or do you
> envisage multiple providers on an object in a running VM?

I think multiple providers are possible for a single object, for example 
a device could expose both PCI (how many MSIs, etc.) and SCSI (how many 
commands sent/succeeded/failed) statistics.

Paolo

Re: [PATCH 7/8] qmp: add filtering of statistics by name

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> On 4/27/22 17:16, Dr. David Alan Gilbert wrote:
> > * Paolo Bonzini (pbonzini@redhat.com) wrote:
> > > On 4/27/22 14:34, Dr. David Alan Gilbert wrote:
> > > > If I specify a 'vm' it's not obvious to me whether I'd get NICs and
> > > > block devices in the future?
> > > 
> > > VM would not get those (it's global statistics), but the size could balloon
> > > if you specify no target at all.
> > > 
> > > > Adding a syntax for 'all' into the vcpus list would fix that?
> > > 
> > > I don't like having special syntax.  The current QAPI just doesn't filter
> > > what is not in the arguments.
> > 
> > Is there a object that represents the set of all vcpus?
> 
> No.

If it was easy to create one then you could remove all the special
casing of vCPUs/VM target?
(It feels really like you should call a 'stats' method on the target)

> > > Yes, those would have different providers.  But a single target can support
> > > multiple providers.
> > 
> > Is that just for different implementations - kvm/hcf/tcg etc or do you
> > envisage multiple providers on an object in a running VM?
> 
> I think multiple providers are possible for a single object, for example a
> device could expose both PCI (how many MSIs, etc.) and SCSI (how many
> commands sent/succeeded/failed) statistics.

Yeh fair enough.

Dave

> Paolo
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 7/8] qmp: add filtering of statistics by name

[Paolo Bonzini]

On 4/27/22 19:16, Dr. David Alan Gilbert wrote:
> * Paolo Bonzini (pbonzini@redhat.com) wrote:
>> On 4/27/22 17:16, Dr. David Alan Gilbert wrote:
>>> * Paolo Bonzini (pbonzini@redhat.com) wrote:
>>>> On 4/27/22 14:34, Dr. David Alan Gilbert wrote:
>>>>> If I specify a 'vm' it's not obvious to me whether I'd get NICs and
>>>>> block devices in the future?
>>>>
>>>> VM would not get those (it's global statistics), but the size could balloon
>>>> if you specify no target at all.
>>>>
>>>>> Adding a syntax for 'all' into the vcpus list would fix that?
>>>>
>>>> I don't like having special syntax.  The current QAPI just doesn't filter
>>>> what is not in the arguments.
>>>
>>> Is there a object that represents the set of all vcpus?
>>
>> No.
> 
> If it was easy to create one then you could remove all the special
> casing of vCPUs/VM target?
> (It feels really like you should call a 'stats' method on the target)

There are two possibilities for that:

1) add statistics to an object like /machine, that would return the 
sum/max of the statistics.  Advantage: you have an easy way to summarize 
stats without reading many KBs of data. Disadvantage: it doesn't do what 
you're asking. :)  But it may be an interesting addition.

2) make query-stats return the statistics for all objects below a given 
QOM path, and then the caller would pass / or /machine as the target.

Both make sense, but neither extends easily to the case where you don't 
have a QOM path, as is the case for block or network devices. 
Unfortunately, both of them are prime candidates for extending the 
subsystem, so they can't be dismissed easily, and that is why 
implemented neither of them.

If block or network devices were QOM, it would be possible and easy to 
have a single "qom-path" argument to replace both "target" and the 
sub-records like StatsVCPUFilter.

Paolo

[PATCH 8/8] hmp: add filtering of statistics by name

[Paolo Bonzini]

Allow the user to request only a specific subset of statistics.
This can be useful when working on a feature or optimization that is
known to affect that statistic.

Extracted from a patch by Mark Kanda.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hmp-commands-info.hx |  8 ++++----
 monitor/hmp-cmds.c   | 35 +++++++++++++++++++++++++----------
 2 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index 1a3e16175f..d533036bc9 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -897,10 +897,10 @@ ERST
 
     {
         .name       = "stats",
-        .args_type  = "target:s,provider:s?",
-        .params     = "target [provider]",
-        .help       = "show statistics for the given target (vm or vcpu); optionally filter by "
-	              "provider",
+        .args_type  = "target:s,names:s?,provider:s?",
+        .params     = "target [names] [provider]",
+        .help       = "show statistics for the given target (vm or vcpu); optionally filter by"
+	              "name (comma-separated list, or * for all) and provider",
         .cmd        = hmp_info_stats,
     },
 
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 2bce9d7016..4150d76fd9 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -2340,10 +2340,12 @@ static void print_stats_results(Monitor *mon, StatsTarget target,
 }
 
 /* Create the StatsFilter that is needed for an "info stats" invocation.  */
-static StatsFilter *stats_filter(StatsTarget target, int cpu_index,
-                                 StatsProvider provider)
+static StatsFilter *stats_filter(StatsTarget target, const char *names,
+                                 int cpu_index, StatsProvider provider)
 {
     StatsFilter *filter = g_malloc0(sizeof(*filter));
+    StatsProvider provider_idx;
+    StatsRequestList *request_list = NULL;
 
     filter->target = target;
     switch (target) {
@@ -2364,15 +2366,27 @@ static StatsFilter *stats_filter(StatsTarget target, int cpu_index,
         break;
     }
 
-    if (provider == STATS_PROVIDER__MAX) {
+    if (!names && provider == STATS_PROVIDER__MAX) {
         return filter;
     }
 
-    /* "info stats" can only query either one or all the providers.  */
-    StatsRequest *request = g_new0(StatsRequest, 1);
-    request->provider = provider;
-    StatsRequestList *request_list = g_new0(StatsRequestList, 1);
-    QAPI_LIST_PREPEND(request_list, request);
+    /*
+     * "info stats" can only query either one or all the providers.  Querying
+     * by name, but not by provider, requires the creation of one filter per
+     * provider.
+     */
+    for (provider_idx = 0; provider_idx < STATS_PROVIDER__MAX; provider_idx++) {
+        if (provider == STATS_PROVIDER__MAX || provider == provider_idx) {
+            StatsRequest *request = g_new0(StatsRequest, 1);
+            request->provider = provider_idx;
+            if (names && !g_str_equal(names, "*")) {
+                request->has_names = true;
+                request->names = strList_from_comma_list(names);
+            }
+            QAPI_LIST_PREPEND(request_list, request);
+        }
+    }
+
     filter->has_providers = true;
     filter->providers = request_list;
     return filter;
@@ -2382,6 +2396,7 @@ void hmp_info_stats(Monitor *mon, const QDict *qdict)
 {
     const char *target_str = qdict_get_str(qdict, "target");
     const char *provider_str = qdict_get_try_str(qdict, "provider");
+    const char *names = qdict_get_try_str(qdict, "names");
 
     StatsProvider provider = STATS_PROVIDER__MAX;
     StatsTarget target;
@@ -2412,11 +2427,11 @@ void hmp_info_stats(Monitor *mon, const QDict *qdict)
 
     switch (target) {
     case STATS_TARGET_VM:
-        filter = stats_filter(target, -1, provider);
+        filter = stats_filter(target, names, -1, provider);
         break;
     case STATS_TARGET_VCPU: {}
         int cpu_index = monitor_get_cpu_index(mon);
-        filter = stats_filter(target, cpu_index, provider);
+        filter = stats_filter(target, names, cpu_index, provider);
         break;
     default:
         abort();
-- 
2.35.1

[PATCH v4 0/8] qmp, hmp: statistics subsystem and KVM suport.

[Paolo Bonzini]

This patchset adds QEMU support for querying fd-based KVM statistics.
This allows the user to analyze the behavior of the VM without access
to debugfs.

However, instead of adding an ad hoc command, the new QMP entry point
can be extended in the future to more statistics provider than KVM
(for example TCG, tap, or the block layer) and to more objects than
the VM and vCPUS (for example network interfaces or block devices).

Because the statistics exposed by KVM are not known at compile time,
the kernel interface also comes with an introspectable schema.  This
schema is exposed by the query-stats-schemas QMP command.

Patches 1 and 2 add the basic support, respectively the QMP command
and the KVM producer.

Patches 3 and 4 add a basic HMP implementation.  The first of the two
adds a basic filtering mechanism to the QMP command, which is then used
by HMP (which only shows vCPU statistics for the currently selected
guest CPU; this is consistent with other HMP commands and does not
flood the user with an overwhelming amount of output).

The remaining patches add more filtering, respectively by provider
and by the name of a statistic.

Paolo

v3->v4:
- correctly handle errors from the callbacks (patch 1)
- renamed str_in_list to apply_str_list_filter (patch 3)
- handle empty filter early by avoiding the query altogether (patch 3, 7)

(patches 2/4/6/8 are identical to v3; patch 5 only has context changes)

Mark Kanda (3):
  qmp: Support for querying stats
  kvm: Support for querying fd-based stats
  hmp: add basic "info stats" implementation

Paolo Bonzini (5):
  qmp: add filtering of statistics by target vCPU
  qmp: add filtering of statistics by provider
  hmp: add filtering of statistics by provider
  qmp: add filtering of statistics by name
  hmp: add filtering of statistics by name

 accel/kvm/kvm-all.c     | 414 ++++++++++++++++++++++++++++++++++++++++
 hmp-commands-info.hx    |  14 ++
 include/monitor/hmp.h   |   1 +
 include/monitor/stats.h |  45 +++++
 monitor/hmp-cmds.c      | 229 ++++++++++++++++++++++
 monitor/qmp-cmds.c      | 149 +++++++++++++++
 qapi/meson.build        |   1 +
 qapi/qapi-schema.json   |   1 +
 qapi/stats.json         | 246 ++++++++++++++++++++++++
 9 files changed, 1100 insertions(+)
 create mode 100644 include/monitor/stats.h
 create mode 100644 qapi/stats.json

-- 
2.36.0

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

[Paolo Bonzini]

From: Mark Kanda <mark.kanda@oracle.com>

Gathering statistics is important for development, for monitoring and
for performance measurement.  There are tools such as kvm_stat that do
this and they rely on the _user_ knowing the interesting data points
rather than the tool (which can treat them as opaque).

The commands introduced in this commit introduce QMP support for
querying stats; the goal is to take the capabilities of these tools
and making them available throughout the whole virtualization stack,
so that one can observe, monitor and measure virtual machines without
having shell access + root on the host that runs them.

query-stats returns a list of all stats per target type (only VM
and vCPU to start); future commits add extra options for specifying
stat names, vCPU qom paths, and providers.  All these are used by the
HMP command "info stats".  Because of the development usecases around
statistics, a good HMP interface is important.

query-stats-schemas returns a list of stats included in each target
type, with an option for specifying the provider.  The concepts in the
schema are based on the KVM binary stats' own introspection data, just
translated to QAPI.

There are two reasons to have a separate schema that is not tied to
the QAPI schema.  The first is the contents of the schemas: the new
introspection data provides different information than the QAPI data,
namely unit of measurement, how the numbers are gathered and change
(peak/instant/cumulative/histogram), and histogram bucket sizes.
There's really no reason to have this kind of metadata in the QAPI
introspection schema (except possibly for the unit of measure, but
there's a very weak justification).

Another reason is the dynamicity of the schema.  The QAPI introspection
data is very much static; and while QOM is somewhat more dynamic,
generally we consider that to be a bug rather than a feature these days.
On the other hand, the statistics that are exposed by QEMU might be
passed through from another source, such as KVM, and the disadvantages of
manually updating the QAPI schema for outweight the benefits from vetting
the statistics and filtering out anything that seems "too unstable".
Running old QEMU with new kernel is a supported usecase; if old QEMU
cannot expose statistics from a new kernel, or if a kernel developer
needs to change QEMU before gathering new info from the new kernel,
then that is a poor user interface.

The framework provides a method to register callbacks for these QMP
commands.  Most of the work in fact is done by the callbacks, and a
large majority of this patch is new QAPI structs and commands.

Examples (with KVM stats):

- Query all VM stats:

{ "execute": "query-stats", "arguments" : { "target": "vm" } }

{ "return": [
     { "provider": "kvm",
       "stats": [
          { "name": "max_mmu_page_hash_collisions", "value": 0 },
          { "name": "max_mmu_rmap_size", "value": 0 },
          { "name": "nx_lpage_splits", "value": 148 },
          ... ] },
     { "provider": "xyz",
       "stats": [ ... ] }
] }

- Query all vCPU stats:

{ "execute": "query-stats", "arguments" : { "target": "vcpu" } }

{ "return": [
     { "provider": "kvm",
       "qom_path": "/machine/unattached/device[0]"
       "stats": [
          { "name": "guest_mode", "value": 0 },
          { "name": "directed_yield_successful", "value": 0 },
          { "name": "directed_yield_attempted", "value": 106 },
          ... ] },
     { "provider": "kvm",
       "qom_path": "/machine/unattached/device[1]"
       "stats": [
          { "name": "guest_mode", "value": 0 },
          { "name": "directed_yield_successful", "value": 0 },
          { "name": "directed_yield_attempted", "value": 106 },
          ... ] },
] }

- Retrieve the schemas:

{ "execute": "query-stats-schemas" }

{ "return": [
    { "provider": "kvm",
      "target": "vcpu",
      "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": "kvm",
      "target": "vm",
      "stats": [
        { "name": "max_mmu_page_hash_collisions",
           "unit": "none",
           "base": 10,
           "exponent": 0,
           "type": "peak" },
        ... ]
    },
    { "provider": "xyz",
      "target": "vm",
      "stats": [ ... ]
    }
] }

Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
v3->v4: correctly handle errors from the callbacks

 include/monitor/stats.h |  34 +++++++
 monitor/qmp-cmds.c      |  82 +++++++++++++++
 qapi/meson.build        |   1 +
 qapi/qapi-schema.json   |   1 +
 qapi/stats.json         | 215 ++++++++++++++++++++++++++++++++++++++++
 5 files changed, 333 insertions(+)
 create mode 100644 include/monitor/stats.h
 create mode 100644 qapi/stats.json

diff --git a/include/monitor/stats.h b/include/monitor/stats.h
new file mode 100644
index 0000000000..912eeadb2f
--- /dev/null
+++ b/include/monitor/stats.h
@@ -0,0 +1,34 @@
+/*
+ * Copyright (c) 2022 Oracle and/or its affiliates.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef STATS_H
+#define STATS_H
+
+#include "qapi/qapi-types-stats.h"
+
+typedef void StatRetrieveFunc(StatsResultList **result, StatsTarget target,
+                              Error **errp);
+typedef void SchemaRetrieveFunc(StatsSchemaList **result, Error **errp);
+
+/*
+ * Register callbacks for the QMP query-stats command.
+ *
+ * @stats_fn: routine to query stats:
+ * @schema_fn: routine to query stat schemas:
+ */
+void add_stats_callbacks(StatRetrieveFunc *stats_fn,
+                         SchemaRetrieveFunc *schemas_fn);
+
+/*
+ * Helper routines for adding stats entries to the results lists.
+ */
+void add_stats_entry(StatsResultList **, StatsProvider, const char *id,
+                     StatsList *stats_list);
+void add_stats_schema(StatsSchemaList **, StatsProvider, StatsTarget,
+                      StatsSchemaValueList *);
+
+#endif /* STATS_H */
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 1ebb89f46c..d65c5f0257 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -35,6 +35,7 @@
 #include "qapi/qapi-commands-control.h"
 #include "qapi/qapi-commands-machine.h"
 #include "qapi/qapi-commands-misc.h"
+#include "qapi/qapi-commands-stats.h"
 #include "qapi/qapi-commands-ui.h"
 #include "qapi/type-helpers.h"
 #include "qapi/qmp/qerror.h"
@@ -43,6 +44,7 @@
 #include "hw/acpi/acpi_dev_interface.h"
 #include "hw/intc/intc.h"
 #include "hw/rdma/rdma.h"
+#include "monitor/stats.h"
 
 NameInfo *qmp_query_name(Error **errp)
 {
@@ -441,3 +443,83 @@ HumanReadableText *qmp_x_query_irq(Error **errp)
 
     return human_readable_text_from_str(buf);
 }
+
+typedef struct StatsCallbacks {
+    StatRetrieveFunc *stats_cb;
+    SchemaRetrieveFunc *schemas_cb;
+    QTAILQ_ENTRY(StatsCallbacks) next;
+} StatsCallbacks;
+
+static QTAILQ_HEAD(, StatsCallbacks) stats_callbacks =
+    QTAILQ_HEAD_INITIALIZER(stats_callbacks);
+
+void add_stats_callbacks(StatRetrieveFunc *stats_fn,
+                         SchemaRetrieveFunc *schemas_fn)
+{
+    StatsCallbacks *entry = g_new(StatsCallbacks, 1);
+    entry->stats_cb = stats_fn;
+    entry->schemas_cb = schemas_fn;
+
+    QTAILQ_INSERT_TAIL(&stats_callbacks, entry, next);
+}
+
+StatsResultList *qmp_query_stats(StatsFilter *filter, Error **errp)
+{
+    StatsResultList *stats_results = NULL;
+    StatsCallbacks *entry;
+    ERRP_GUARD();
+
+    QTAILQ_FOREACH(entry, &stats_callbacks, next) {
+        entry->stats_cb(&stats_results, filter->target, errp);
+        if (*errp) {
+            qapi_free_StatsResultList(stats_results);
+            return NULL;
+        }
+    }
+
+    return stats_results;
+}
+
+StatsSchemaList *qmp_query_stats_schemas(Error **errp)
+{
+    StatsSchemaList *stats_results = NULL;
+    StatsCallbacks *entry;
+    ERRP_GUARD();
+
+    QTAILQ_FOREACH(entry, &stats_callbacks, next) {
+        entry->schemas_cb(&stats_results, errp);
+        if (*errp) {
+            qapi_free_StatsSchemaList(stats_results);
+            return NULL;
+        }
+    }
+
+    return stats_results;
+}
+
+void add_stats_entry(StatsResultList **stats_results, StatsProvider provider,
+                     const char *qom_path, StatsList *stats_list)
+{
+    StatsResult *entry = g_new0(StatsResult, 1);
+
+    entry->provider = provider;
+    if (qom_path) {
+        entry->has_qom_path = true;
+        entry->qom_path = g_strdup(qom_path);
+    }
+    entry->stats = stats_list;
+
+    QAPI_LIST_PREPEND(*stats_results, entry);
+}
+
+void add_stats_schema(StatsSchemaList **schema_results,
+                      StatsProvider provider, StatsTarget target,
+                      StatsSchemaValueList *stats_list)
+{
+    StatsSchema *entry = g_new0(StatsSchema, 1);
+
+    entry->provider = provider;
+    entry->target = target;
+    entry->stats = stats_list;
+    QAPI_LIST_PREPEND(*schema_results, entry);
+}
diff --git a/qapi/meson.build b/qapi/meson.build
index 656ef0e039..fd5c93d643 100644
--- a/qapi/meson.build
+++ b/qapi/meson.build
@@ -46,6 +46,7 @@ qapi_all_modules = [
   'replay',
   'run-state',
   'sockets',
+  'stats',
   'trace',
   'transaction',
   'yank',
diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
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..650d883297
--- /dev/null
+++ b/qapi/stats.json
@@ -0,0 +1,215 @@
+# -*- 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-histogram: stat is a linear histogram.
+# @log2-histogram: stat is a logarithmic histogram, with one bucket
+#                  for each power of two.
+#
+# Since: 7.1
+##
+{ 'enum' : 'StatsType',
+  'data' : [ 'cumulative', 'instant', 'peak', 'linear-histogram', 'log2-histogram' ] }
+
+##
+# @StatsUnit:
+#
+# Enumeration of unit of measurement for statistics
+#
+# @bytes: stat reported in bytes.
+# @seconds: stat reported in seconds.
+# @cycles: stat reported in clock cycles.
+#
+# Since: 7.1
+##
+{ 'enum' : 'StatsUnit',
+  'data' : [ 'bytes', 'seconds', 'cycles' ] }
+
+##
+# @StatsProvider:
+#
+# Enumeration of statistics providers.
+#
+# Since: 7.1
+##
+{ 'enum': 'StatsProvider',
+  'data': [ ] }
+
+##
+# @StatsTarget:
+#
+# The kinds of objects on which one can request statistics.
+#
+# @vm: statistics that apply to the entire virtual machine or
+#      the entire QEMU process.
+#
+# @vcpu: statistics that apply to a single virtual CPU.
+#
+# Since: 7.1
+##
+{ 'enum': 'StatsTarget',
+  'data': [ 'vm', 'vcpu' ] }
+
+##
+# @StatsFilter:
+#
+# The arguments to the query-stats command; specifies a target for which to
+# request statistics.
+#
+# Since: 7.1
+##
+{ 'struct': 'StatsFilter',
+  'data': { 'target': 'StatsTarget' } }
+
+##
+# @StatsValue:
+#
+# @scalar: single unsigned 64-bit integers.
+# @list: list of unsigned 64-bit integers (used for histograms).
+#
+# Since: 7.1
+##
+{ 'alternate': 'StatsValue',
+  'data': { 'scalar': 'uint64',
+            'list': [ 'uint64' ] } }
+
+##
+# @Stats:
+#
+# @name: name of stat.
+# @value: stat value.
+#
+# Since: 7.1
+##
+{ 'struct': 'Stats',
+  'data': { 'name': 'str',
+            'value' : 'StatsValue' } }
+
+##
+# @StatsResult:
+#
+# @provider: provider for this set of statistics.
+#
+# @qom-path: Path to the object for which the statistics are returned,
+#            if the object is exposed in the QOM tree
+#
+# @stats: list of statistics.
+#
+# Since: 7.1
+##
+{ 'struct': 'StatsResult',
+  'data': { 'provider': 'StatsProvider',
+            '*qom-path': 'str',
+            'stats': [ 'Stats' ] } }
+
+##
+# @query-stats:
+#
+# Return runtime-collected statistics for objects such as the
+# VM or its vCPUs.
+#
+# The arguments are a StatsFilter and specify the provider and objects
+# to return statistics about.
+#
+# Returns: a list of StatsResult, one for each provider and object
+#          (e.g., for each vCPU).
+#
+# Since: 7.1
+##
+{ 'command': 'query-stats',
+  'data': 'StatsFilter',
+  'boxed': true,
+  'returns': [ 'StatsResult' ] }
+
+##
+# @StatsSchemaValue:
+#
+# Schema for a single statistic.
+#
+# @name: name of the statistic; each element of the schema is uniquely
+#        identified by a target, a provider (both available in @StatsSchema)
+#        and the name.
+#
+# @type: kind of statistic.
+#
+# @unit: basic unit of measure for the statistic; if missing, the statistic
+#        is a simple number or counter.
+#
+# @base: base for the multiple of @unit in which the statistic is measured.
+#        Only present if @exponent is non-zero; @base and @exponent together
+#        form a SI prefix (e.g., _nano-_ for ``base=10`` and ``exponent=-9``)
+#        or IEC binary prefix (e.g. _kibi-_ for ``base=2`` and ``exponent=10``)
+#
+# @exponent: exponent for the multiple of @unit in which the statistic is
+#            expressed, or 0 for the basic unit
+#
+# @bucket-size: Present when @type is "linear-histogram", contains the width
+#               of each bucket of the histogram.
+#
+# Since: 7.1
+##
+{ 'struct': 'StatsSchemaValue',
+  'data': { 'name': 'str',
+            'type': 'StatsType',
+            '*unit': 'StatsUnit',
+            '*base': 'int8',
+            'exponent': 'int16',
+            '*bucket-size': 'uint32' } }
+
+##
+# @StatsSchema:
+#
+# Schema for all available statistics for a provider and target.
+#
+# @provider: provider for this set of statistics.
+#
+# @target: the kind of object that can be queried through the provider.
+#
+# @stats: list of statistics.
+#
+# Since: 7.1
+##
+{ 'struct': 'StatsSchema',
+  'data': { 'provider': 'StatsProvider',
+            'target': 'StatsTarget',
+            'stats': [ 'StatsSchemaValue' ] } }
+
+##
+# @query-stats-schemas:
+#
+# Return the schema for all available runtime-collected statistics.
+#
+# Note: runtime-collected statistics and their names fall outside QEMU's
+# usual deprecation policies.  QEMU will try to keep the set of available
+# data stable, together with their names, but will not guarantee stability
+# at all costs; the same is true of providers that source statistics
+# externally, e.g. from Linux.  For example, if the same value is being
+# tracked with different names on different architectures or by different
+# providers, one of them might be renamed.  A statistic might go away if
+# an algorithm is changed or some code is removed; changing a default might
+# cause previously useful statistics to always report 0.  Such changes,
+# however, are expected to be rare.
+#
+# Since: 7.1
+##
+{ 'command': 'query-stats-schemas',
+  'data': { },
+  'returns': [ 'StatsSchema' ] }
-- 
2.36.0

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> From: Mark Kanda <mark.kanda@oracle.com>
>
> Gathering statistics is important for development, for monitoring and
> for performance measurement.  There are tools such as kvm_stat that do
> this and they rely on the _user_ knowing the interesting data points
> rather than the tool (which can treat them as opaque).
>
> The commands introduced in this commit introduce QMP support for
> querying stats; the goal is to take the capabilities of these tools
> and making them available throughout the whole virtualization stack,
> so that one can observe, monitor and measure virtual machines without
> having shell access + root on the host that runs them.
>
> query-stats returns a list of all stats per target type (only VM
> and vCPU to start); future commits add extra options for specifying
> stat names, vCPU qom paths, and providers.  All these are used by the
> HMP command "info stats".  Because of the development usecases around
> statistics, a good HMP interface is important.
>
> query-stats-schemas returns a list of stats included in each target
> type, with an option for specifying the provider.  The concepts in the
> schema are based on the KVM binary stats' own introspection data, just
> translated to QAPI.
>
> There are two reasons to have a separate schema that is not tied to
> the QAPI schema.  The first is the contents of the schemas: the new
> introspection data provides different information than the QAPI data,
> namely unit of measurement, how the numbers are gathered and change
> (peak/instant/cumulative/histogram), and histogram bucket sizes.
> There's really no reason to have this kind of metadata in the QAPI
> introspection schema (except possibly for the unit of measure, but
> there's a very weak justification).
>
> Another reason is the dynamicity of the schema.  The QAPI introspection
> data is very much static; and while QOM is somewhat more dynamic,
> generally we consider that to be a bug rather than a feature these days.
> On the other hand, the statistics that are exposed by QEMU might be
> passed through from another source, such as KVM, and the disadvantages of
> manually updating the QAPI schema for outweight the benefits from vetting
> the statistics and filtering out anything that seems "too unstable".
> Running old QEMU with new kernel is a supported usecase; if old QEMU
> cannot expose statistics from a new kernel, or if a kernel developer
> needs to change QEMU before gathering new info from the new kernel,
> then that is a poor user interface.
>
> The framework provides a method to register callbacks for these QMP
> commands.  Most of the work in fact is done by the callbacks, and a
> large majority of this patch is new QAPI structs and commands.
>
> Examples (with KVM stats):
>
> - Query all VM stats:
>
> { "execute": "query-stats", "arguments" : { "target": "vm" } }
>
> { "return": [
>      { "provider": "kvm",
>        "stats": [
>           { "name": "max_mmu_page_hash_collisions", "value": 0 },
>           { "name": "max_mmu_rmap_size", "value": 0 },
>           { "name": "nx_lpage_splits", "value": 148 },
>           ... ] },
>      { "provider": "xyz",
>        "stats": [ ... ] }
> ] }
>
> - Query all vCPU stats:
>
> { "execute": "query-stats", "arguments" : { "target": "vcpu" } }
>
> { "return": [
>      { "provider": "kvm",
>        "qom_path": "/machine/unattached/device[0]"
>        "stats": [
>           { "name": "guest_mode", "value": 0 },
>           { "name": "directed_yield_successful", "value": 0 },
>           { "name": "directed_yield_attempted", "value": 106 },
>           ... ] },
>      { "provider": "kvm",
>        "qom_path": "/machine/unattached/device[1]"
>        "stats": [
>           { "name": "guest_mode", "value": 0 },
>           { "name": "directed_yield_successful", "value": 0 },
>           { "name": "directed_yield_attempted", "value": 106 },
>           ... ] },
> ] }
>
> - Retrieve the schemas:
>
> { "execute": "query-stats-schemas" }
>
> { "return": [
>     { "provider": "kvm",
>       "target": "vcpu",
>       "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": "kvm",
>       "target": "vm",
>       "stats": [
>         { "name": "max_mmu_page_hash_collisions",
>            "unit": "none",
>            "base": 10,
>            "exponent": 0,
>            "type": "peak" },
>         ... ]
>     },
>     { "provider": "xyz",
>       "target": "vm",
>       "stats": [ ... ]
>     }
> ] }
>
> Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> v3->v4: correctly handle errors from the callbacks
>
>  include/monitor/stats.h |  34 +++++++
>  monitor/qmp-cmds.c      |  82 +++++++++++++++
>  qapi/meson.build        |   1 +
>  qapi/qapi-schema.json   |   1 +
>  qapi/stats.json         | 215 ++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 333 insertions(+)
>  create mode 100644 include/monitor/stats.h
>  create mode 100644 qapi/stats.json
>
> diff --git a/include/monitor/stats.h b/include/monitor/stats.h
> new file mode 100644
> index 0000000000..912eeadb2f
> --- /dev/null
> +++ b/include/monitor/stats.h
> @@ -0,0 +1,34 @@
> +/*
> + * Copyright (c) 2022 Oracle and/or its affiliates.
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef STATS_H
> +#define STATS_H
> +
> +#include "qapi/qapi-types-stats.h"
> +
> +typedef void StatRetrieveFunc(StatsResultList **result, StatsTarget target,
> +                              Error **errp);
> +typedef void SchemaRetrieveFunc(StatsSchemaList **result, Error **errp);
> +
> +/*
> + * Register callbacks for the QMP query-stats command.
> + *
> + * @stats_fn: routine to query stats:
> + * @schema_fn: routine to query stat schemas:

Contracts would be nice.

> + */
> +void add_stats_callbacks(StatRetrieveFunc *stats_fn,
> +                         SchemaRetrieveFunc *schemas_fn);
> +
> +/*
> + * Helper routines for adding stats entries to the results lists.
> + */
> +void add_stats_entry(StatsResultList **, StatsProvider, const char *id,
> +                     StatsList *stats_list);
> +void add_stats_schema(StatsSchemaList **, StatsProvider, StatsTarget,
> +                      StatsSchemaValueList *);
> +
> +#endif /* STATS_H */
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 1ebb89f46c..d65c5f0257 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -35,6 +35,7 @@
>  #include "qapi/qapi-commands-control.h"
>  #include "qapi/qapi-commands-machine.h"
>  #include "qapi/qapi-commands-misc.h"
> +#include "qapi/qapi-commands-stats.h"
>  #include "qapi/qapi-commands-ui.h"
>  #include "qapi/type-helpers.h"
>  #include "qapi/qmp/qerror.h"
> @@ -43,6 +44,7 @@
>  #include "hw/acpi/acpi_dev_interface.h"
>  #include "hw/intc/intc.h"
>  #include "hw/rdma/rdma.h"
> +#include "monitor/stats.h"
>  
>  NameInfo *qmp_query_name(Error **errp)
>  {
> @@ -441,3 +443,83 @@ HumanReadableText *qmp_x_query_irq(Error **errp)
>  
>      return human_readable_text_from_str(buf);
>  }
> +
> +typedef struct StatsCallbacks {
> +    StatRetrieveFunc *stats_cb;
> +    SchemaRetrieveFunc *schemas_cb;
> +    QTAILQ_ENTRY(StatsCallbacks) next;
> +} StatsCallbacks;
> +
> +static QTAILQ_HEAD(, StatsCallbacks) stats_callbacks =
> +    QTAILQ_HEAD_INITIALIZER(stats_callbacks);
> +
> +void add_stats_callbacks(StatRetrieveFunc *stats_fn,
> +                         SchemaRetrieveFunc *schemas_fn)
> +{
> +    StatsCallbacks *entry = g_new(StatsCallbacks, 1);
> +    entry->stats_cb = stats_fn;
> +    entry->schemas_cb = schemas_fn;
> +
> +    QTAILQ_INSERT_TAIL(&stats_callbacks, entry, next);
> +}
> +
> +StatsResultList *qmp_query_stats(StatsFilter *filter, Error **errp)
> +{
> +    StatsResultList *stats_results = NULL;
> +    StatsCallbacks *entry;
> +    ERRP_GUARD();
> +
> +    QTAILQ_FOREACH(entry, &stats_callbacks, next) {
> +        entry->stats_cb(&stats_results, filter->target, errp);
> +        if (*errp) {
> +            qapi_free_StatsResultList(stats_results);
> +            return NULL;
> +        }
> +    }
> +
> +    return stats_results;
> +}
> +
> +StatsSchemaList *qmp_query_stats_schemas(Error **errp)
> +{
> +    StatsSchemaList *stats_results = NULL;
> +    StatsCallbacks *entry;
> +    ERRP_GUARD();
> +
> +    QTAILQ_FOREACH(entry, &stats_callbacks, next) {
> +        entry->schemas_cb(&stats_results, errp);
> +        if (*errp) {
> +            qapi_free_StatsSchemaList(stats_results);
> +            return NULL;
> +        }
> +    }
> +
> +    return stats_results;
> +}

Have you considered making the callbacks return bool, following
error.h's "Whenever practical, also return a value that indicates
success / failure" recommendation?

> +
> +void add_stats_entry(StatsResultList **stats_results, StatsProvider provider,
> +                     const char *qom_path, StatsList *stats_list)
> +{
> +    StatsResult *entry = g_new0(StatsResult, 1);
> +
> +    entry->provider = provider;
> +    if (qom_path) {
> +        entry->has_qom_path = true;
> +        entry->qom_path = g_strdup(qom_path);
> +    }
> +    entry->stats = stats_list;
> +
> +    QAPI_LIST_PREPEND(*stats_results, entry);
> +}
> +
> +void add_stats_schema(StatsSchemaList **schema_results,
> +                      StatsProvider provider, StatsTarget target,
> +                      StatsSchemaValueList *stats_list)
> +{
> +    StatsSchema *entry = g_new0(StatsSchema, 1);
> +
> +    entry->provider = provider;
> +    entry->target = target;
> +    entry->stats = stats_list;
> +    QAPI_LIST_PREPEND(*schema_results, entry);
> +}
> diff --git a/qapi/meson.build b/qapi/meson.build
> index 656ef0e039..fd5c93d643 100644
> --- a/qapi/meson.build
> +++ b/qapi/meson.build
> @@ -46,6 +46,7 @@ qapi_all_modules = [
>    'replay',
>    'run-state',
>    'sockets',
> +  'stats',
>    'trace',
>    'transaction',
>    'yank',
> diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
> 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..650d883297
> --- /dev/null
> +++ b/qapi/stats.json
> @@ -0,0 +1,215 @@
> +# -*- 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-histogram: stat is a linear histogram.
> +# @log2-histogram: stat is a logarithmic histogram, with one bucket
> +#                  for each power of two.
> +#
> +# Since: 7.1
> +##
> +{ 'enum' : 'StatsType',
> +  'data' : [ 'cumulative', 'instant', 'peak', 'linear-histogram', 'log2-histogram' ] }

Long line.

> +
> +##
> +# @StatsUnit:
> +#
> +# Enumeration of unit of measurement for statistics
> +#
> +# @bytes: stat reported in bytes.
> +# @seconds: stat reported in seconds.
> +# @cycles: stat reported in clock cycles.
> +#
> +# Since: 7.1
> +##
> +{ 'enum' : 'StatsUnit',
> +  'data' : [ 'bytes', 'seconds', 'cycles' ] }
> +
> +##
> +# @StatsProvider:
> +#
> +# Enumeration of statistics providers.
> +#
> +# Since: 7.1
> +##
> +{ 'enum': 'StatsProvider',
> +  'data': [ ] }
> +
> +##
> +# @StatsTarget:
> +#
> +# The kinds of objects on which one can request statistics.
> +#
> +# @vm: statistics that apply to the entire virtual machine or
> +#      the entire QEMU process.
> +#
> +# @vcpu: statistics that apply to a single virtual CPU.
> +#
> +# Since: 7.1
> +##
> +{ 'enum': 'StatsTarget',
> +  'data': [ 'vm', 'vcpu' ] }
> +
> +##
> +# @StatsFilter:
> +#
> +# The arguments to the query-stats command; specifies a target for which to
> +# request statistics.
> +#
> +# Since: 7.1
> +##
> +{ 'struct': 'StatsFilter',
> +  'data': { 'target': 'StatsTarget' } }
> +
> +##
> +# @StatsValue:
> +#
> +# @scalar: single unsigned 64-bit integers.
> +# @list: list of unsigned 64-bit integers (used for histograms).
> +#
> +# Since: 7.1
> +##
> +{ 'alternate': 'StatsValue',
> +  'data': { 'scalar': 'uint64',
> +            'list': [ 'uint64' ] } }
> +
> +##
> +# @Stats:
> +#
> +# @name: name of stat.
> +# @value: stat value.
> +#
> +# Since: 7.1
> +##
> +{ 'struct': 'Stats',
> +  'data': { 'name': 'str',
> +            'value' : 'StatsValue' } }
> +
> +##
> +# @StatsResult:
> +#
> +# @provider: provider for this set of statistics.
> +#
> +# @qom-path: Path to the object for which the statistics are returned,
> +#            if the object is exposed in the QOM tree
> +#
> +# @stats: list of statistics.
> +#
> +# Since: 7.1
> +##
> +{ 'struct': 'StatsResult',
> +  'data': { 'provider': 'StatsProvider',
> +            '*qom-path': 'str',
> +            'stats': [ 'Stats' ] } }
> +
> +##
> +# @query-stats:
> +#
> +# Return runtime-collected statistics for objects such as the
> +# VM or its vCPUs.
> +#
> +# The arguments are a StatsFilter and specify the provider and objects
> +# to return statistics about.
> +#
> +# Returns: a list of StatsResult, one for each provider and object
> +#          (e.g., for each vCPU).
> +#
> +# Since: 7.1
> +##
> +{ 'command': 'query-stats',
> +  'data': 'StatsFilter',
> +  'boxed': true,
> +  'returns': [ 'StatsResult' ] }
> +
> +##
> +# @StatsSchemaValue:
> +#
> +# Schema for a single statistic.
> +#
> +# @name: name of the statistic; each element of the schema is uniquely
> +#        identified by a target, a provider (both available in @StatsSchema)
> +#        and the name.
> +#
> +# @type: kind of statistic.
> +#
> +# @unit: basic unit of measure for the statistic; if missing, the statistic
> +#        is a simple number or counter.
> +#
> +# @base: base for the multiple of @unit in which the statistic is measured.
> +#        Only present if @exponent is non-zero; @base and @exponent together
> +#        form a SI prefix (e.g., _nano-_ for ``base=10`` and ``exponent=-9``)
> +#        or IEC binary prefix (e.g. _kibi-_ for ``base=2`` and ``exponent=10``)
> +#
> +# @exponent: exponent for the multiple of @unit in which the statistic is
> +#            expressed, or 0 for the basic unit
> +#
> +# @bucket-size: Present when @type is "linear-histogram", contains the width
> +#               of each bucket of the histogram.
> +#
> +# Since: 7.1
> +##
> +{ 'struct': 'StatsSchemaValue',
> +  'data': { 'name': 'str',
> +            'type': 'StatsType',
> +            '*unit': 'StatsUnit',
> +            '*base': 'int8',
> +            'exponent': 'int16',
> +            '*bucket-size': 'uint32' } }
> +
> +##
> +# @StatsSchema:
> +#
> +# Schema for all available statistics for a provider and target.
> +#
> +# @provider: provider for this set of statistics.
> +#
> +# @target: the kind of object that can be queried through the provider.
> +#
> +# @stats: list of statistics.
> +#
> +# Since: 7.1
> +##
> +{ 'struct': 'StatsSchema',
> +  'data': { 'provider': 'StatsProvider',
> +            'target': 'StatsTarget',
> +            'stats': [ 'StatsSchemaValue' ] } }
> +
> +##
> +# @query-stats-schemas:
> +#
> +# Return the schema for all available runtime-collected statistics.
> +#
> +# Note: runtime-collected statistics and their names fall outside QEMU's
> +# usual deprecation policies.  QEMU will try to keep the set of available
> +# data stable, together with their names, but will not guarantee stability
> +# at all costs; the same is true of providers that source statistics
> +# externally, e.g. from Linux.  For example, if the same value is being
> +# tracked with different names on different architectures or by different
> +# providers, one of them might be renamed.  A statistic might go away if
> +# an algorithm is changed or some code is removed; changing a default might
> +# cause previously useful statistics to always report 0.  Such changes,
> +# however, are expected to be rare.
> +#
> +# Since: 7.1
> +##
> +{ 'command': 'query-stats-schemas',
> +  'data': { },
> +  'returns': [ 'StatsSchema' ] }

With the long line wrapped:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Paolo Bonzini]

On 5/24/22 12:41, Markus Armbruster wrote:
>> +    return stats_results;
>> +}
> Have you considered making the callbacks return bool, following
> error.h's "Whenever practical, also return a value that indicates
> success / failure" recommendation?
> 

No, because I generally do not consider that recommendation to apply to 
callbacks.

Callbacks are written many times and typically invoked just once, so I 
prefer an O(1) loss in convenience to an O(n) chance of making a mistake 
in the return value.

Paolo

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

[Markus Armbruster]

[...]

> diff --git a/qapi/stats.json b/qapi/stats.json
> new file mode 100644
> index 0000000000..650d883297
> --- /dev/null
> +++ b/qapi/stats.json
[...]
> +##
> +# @query-stats-schemas:
> +#
> +# Return the schema for all available runtime-collected statistics.
> +#
> +# Note: runtime-collected statistics and their names fall outside QEMU's
> +# usual deprecation policies.  QEMU will try to keep the set of available

/work/armbru/qemu/scripts/qapi-gen.py: In file included from ../qapi/qapi-schema.json:96:
../qapi/stats.json:201:1: unexpected de-indent (expected at least 6 spaces)

> +# data stable, together with their names, but will not guarantee stability
> +# at all costs; the same is true of providers that source statistics
> +# externally, e.g. from Linux.  For example, if the same value is being
> +# tracked with different names on different architectures or by different
> +# providers, one of them might be renamed.  A statistic might go away if
> +# an algorithm is changed or some code is removed; changing a default might
> +# cause previously useful statistics to always report 0.  Such changes,
> +# however, are expected to be rare.
> +#
> +# Since: 7.1
> +##
> +{ 'command': 'query-stats-schemas',
> +  'data': { },
> +  'returns': [ 'StatsSchema' ] }