All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 0/8] qmp, hmp: statistics subsystem and KVM suport.
@ 2022-05-23 15:05 Paolo Bonzini
  2022-05-23 15:07 ` [PATCH 1/8] qmp: Support for querying stats Paolo Bonzini
  0 siblings, 1 reply; 22+ messages in thread
From: Paolo Bonzini @ 2022-05-23 15:05 UTC (permalink / raw)
  To: qemu-devel; +Cc: armbru

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



^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 1/8] qmp: Support for querying stats
  2022-05-23 15:05 [PATCH v4 0/8] qmp, hmp: statistics subsystem and KVM suport Paolo Bonzini
@ 2022-05-23 15:07 ` Paolo Bonzini
  2022-05-23 15:07   ` [PATCH 2/8] kvm: Support for querying fd-based stats Paolo Bonzini
                     ` (8 more replies)
  0 siblings, 9 replies; 22+ messages in thread
From: Paolo Bonzini @ 2022-05-23 15:07 UTC (permalink / raw)
  To: qemu-devel; +Cc: armbru, Mark Kanda

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




^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 2/8] kvm: Support for querying fd-based stats
  2022-05-23 15:07 ` [PATCH 1/8] qmp: Support for querying stats Paolo Bonzini
@ 2022-05-23 15:07   ` Paolo Bonzini
  2022-05-23 15:07   ` [PATCH 3/8] qmp: add filtering of statistics by target vCPU Paolo Bonzini
                     ` (7 subsequent siblings)
  8 siblings, 0 replies; 22+ messages in thread
From: Paolo Bonzini @ 2022-05-23 15:07 UTC (permalink / raw)
  To: qemu-devel; +Cc: armbru, Mark Kanda

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..6a6bbe2994 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_HISTOGRAM;
+        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_LOG2_HISTOGRAM;
+        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 650d883297..1129ba49bc 100644
--- a/qapi/stats.json
+++ b/qapi/stats.json
@@ -51,7 +51,7 @@
 # Since: 7.1
 ##
 { 'enum': 'StatsProvider',
-  'data': [ ] }
+  'data': [ 'kvm' ] }
 
 ##
 # @StatsTarget:
-- 
2.36.0




^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 3/8] qmp: add filtering of statistics by target vCPU
  2022-05-23 15:07 ` [PATCH 1/8] qmp: Support for querying stats Paolo Bonzini
  2022-05-23 15:07   ` [PATCH 2/8] kvm: Support for querying fd-based stats Paolo Bonzini
@ 2022-05-23 15:07   ` Paolo Bonzini
  2022-05-24 11:20     ` Markus Armbruster
  2022-05-23 15:07   ` [PATCH 4/8] hmp: add basic "info stats" implementation Paolo Bonzini
                     ` (6 subsequent siblings)
  8 siblings, 1 reply; 22+ messages in thread
From: Paolo Bonzini @ 2022-05-23 15:07 UTC (permalink / raw)
  To: qemu-devel; +Cc: armbru, Mark Kanda

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>
---
v3->v4: renamed str_in_list to apply_str_list_filter
        handle empty filter early by avoiding the query altogether

 accel/kvm/kvm-all.c     |  9 +++++++--
 include/monitor/stats.h | 11 ++++++++++-
 monitor/qmp-cmds.c      | 34 +++++++++++++++++++++++++++++++++-
 qapi/stats.json         | 24 +++++++++++++++++++-----
 4 files changed, 69 insertions(+), 9 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 6a6bbe2994..3ee431a431 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 (!apply_str_list_filter(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 912eeadb2f..8c50feeaa9 100644
--- a/include/monitor/stats.h
+++ b/include/monitor/stats.h
@@ -11,7 +11,7 @@
 #include "qapi/qapi-types-stats.h"
 
 typedef void StatRetrieveFunc(StatsResultList **result, StatsTarget target,
-                              Error **errp);
+                              strList *targets, Error **errp);
 typedef void SchemaRetrieveFunc(StatsSchemaList **result, Error **errp);
 
 /*
@@ -31,4 +31,13 @@ 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.
+ *
+ * Note that an empty list means no filtering, i.e. all strings will
+ * return true.
+ */
+bool apply_str_list_filter(const char *string, strList *list);
+
 #endif /* STATS_H */
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index d65c5f0257..ebf6f49446 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -466,11 +466,28 @@ void add_stats_callbacks(StatRetrieveFunc *stats_fn,
 StatsResultList *qmp_query_stats(StatsFilter *filter, Error **errp)
 {
     StatsResultList *stats_results = NULL;
+    strList *targets = NULL;
     StatsCallbacks *entry;
     ERRP_GUARD();
 
+    switch (filter->target) {
+    case STATS_TARGET_VM:
+        break;
+    case STATS_TARGET_VCPU:
+        if (filter->u.vcpu.has_vcpus) {
+            if (!filter->u.vcpu.vcpus) {
+                /* No targets allowed?  Return no statistics.  */
+                return NULL;
+            }
+            targets = filter->u.vcpu.vcpus;
+        }
+        break;
+    default:
+        abort();
+    }
+
     QTAILQ_FOREACH(entry, &stats_callbacks, next) {
-        entry->stats_cb(&stats_results, filter->target, errp);
+        entry->stats_cb(&stats_results, filter->target, targets, errp);
         if (*errp) {
             qapi_free_StatsResultList(stats_results);
             return NULL;
@@ -523,3 +540,18 @@ void add_stats_schema(StatsSchemaList **schema_results,
     entry->stats = stats_list;
     QAPI_LIST_PREPEND(*schema_results, entry);
 }
+
+bool apply_str_list_filter(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 1129ba49bc..e8ed907793 100644
--- a/qapi/stats.json
+++ b/qapi/stats.json
@@ -69,15 +69,29 @@
   'data': [ 'vm', 'vcpu' ] }
 
 ##
-# @StatsFilter:
+# @StatsVCPUFilter:
 #
-# The arguments to the query-stats command; specifies a target for which to
-# request statistics.
+# @vcpus: list of QOM paths for the desired vCPU objects.
 #
 # Since: 7.1
 ##
-{ 'struct': 'StatsFilter',
-  'data': { 'target': 'StatsTarget' } }
+{ 'struct': 'StatsVCPUFilter',
+  'data': { '*vcpus': [ 'str' ] } }
+
+##
+# @StatsFilter:
+#
+# The arguments to the query-stats command; specifies a target for which to
+# request statistics and optionally the required subset of information for
+# that target:
+# - which vCPUs to request statistics for
+#
+# Since: 7.1
+##
+{ 'union': 'StatsFilter',
+        'base': { 'target': 'StatsTarget' },
+  'discriminator': 'target',
+  'data': { 'vcpu': 'StatsVCPUFilter' } }
 
 ##
 # @StatsValue:
-- 
2.36.0




^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 4/8] hmp: add basic "info stats" implementation
  2022-05-23 15:07 ` [PATCH 1/8] qmp: Support for querying stats Paolo Bonzini
  2022-05-23 15:07   ` [PATCH 2/8] kvm: Support for querying fd-based stats Paolo Bonzini
  2022-05-23 15:07   ` [PATCH 3/8] qmp: add filtering of statistics by target vCPU Paolo Bonzini
@ 2022-05-23 15:07   ` Paolo Bonzini
  2022-05-24 17:22     ` Mark Kanda
  2022-05-23 15:07   ` [PATCH 5/8] qmp: add filtering of statistics by provider Paolo Bonzini
                     ` (5 subsequent siblings)
  8 siblings, 1 reply; 22+ messages in thread
From: Paolo Bonzini @ 2022-05-23 15:07 UTC (permalink / raw)
  To: qemu-devel; +Cc: armbru, Mark Kanda

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    | 187 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 201 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 622c783c32..c0cb440902 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"
@@ -2239,3 +2241,188 @@ 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_HISTOGRAM && 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;
+    g_autoptr(StatsSchemaList) schema = NULL;
+    g_autoptr(StatsResultList) stats = NULL;
+    g_autoptr(StatsFilter) filter = NULL;
+    StatsResultList *entry;
+
+    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;
+    }
+    for (entry = stats; entry; entry = entry->next) {
+        print_stats_results(mon, target, entry->value, schema);
+    }
+
+exit:
+    if (err) {
+        monitor_printf(mon, "%s\n", error_get_pretty(err));
+    }
+exit_no_print:
+    error_free(err);
+}
-- 
2.36.0




^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 5/8] qmp: add filtering of statistics by provider
  2022-05-23 15:07 ` [PATCH 1/8] qmp: Support for querying stats Paolo Bonzini
                     ` (2 preceding siblings ...)
  2022-05-23 15:07   ` [PATCH 4/8] hmp: add basic "info stats" implementation Paolo Bonzini
@ 2022-05-23 15:07   ` Paolo Bonzini
  2022-05-24 12:32     ` Markus Armbruster
  2022-05-23 15:07   ` [PATCH 6/8] hmp: " Paolo Bonzini
                     ` (4 subsequent siblings)
  8 siblings, 1 reply; 22+ messages in thread
From: Paolo Bonzini @ 2022-05-23 15:07 UTC (permalink / raw)
  To: qemu-devel; +Cc: armbru, Mark Kanda

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      | 45 ++++++++++++++++++++++++++++++++---------
 qapi/stats.json         | 17 ++++++++++++++--
 5 files changed, 56 insertions(+), 15 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 3ee431a431..461b6cf927 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 8c50feeaa9..840c4ed08e 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 c0cb440902..8d2c4792d5 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -2394,7 +2394,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 ebf6f49446..7be0e7414a 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -445,6 +445,7 @@ HumanReadableText *qmp_x_query_irq(Error **errp)
 }
 
 typedef struct StatsCallbacks {
+    StatsProvider provider;
     StatRetrieveFunc *stats_cb;
     SchemaRetrieveFunc *schemas_cb;
     QTAILQ_ENTRY(StatsCallbacks) next;
@@ -453,16 +454,34 @@ 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;
 
     QTAILQ_INSERT_TAIL(&stats_callbacks, entry, next);
 }
 
+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;
@@ -487,27 +506,33 @@ StatsResultList *qmp_query_stats(StatsFilter *filter, Error **errp)
     }
 
     QTAILQ_FOREACH(entry, &stats_callbacks, next) {
-        entry->stats_cb(&stats_results, filter->target, targets, errp);
-        if (*errp) {
-            qapi_free_StatsResultList(stats_results);
-            return NULL;
+        if (stats_provider_requested(entry->provider, filter)) {
+            entry->stats_cb(&stats_results, filter->target, targets, errp);
+            if (*errp) {
+                qapi_free_StatsResultList(stats_results);
+                return NULL;
+            }
         }
     }
 
     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;
     ERRP_GUARD();
 
     QTAILQ_FOREACH(entry, &stats_callbacks, next) {
-        entry->schemas_cb(&stats_results, errp);
-        if (*errp) {
-            qapi_free_StatsSchemaList(stats_results);
-            return NULL;
+        if (!has_provider || provider == entry->provider) {
+            entry->schemas_cb(&stats_results, errp);
+            if (*errp) {
+                qapi_free_StatsSchemaList(stats_results);
+                return NULL;
+            }
         }
     }
 
diff --git a/qapi/stats.json b/qapi/stats.json
index e8ed907793..b75bb86124 100644
--- a/qapi/stats.json
+++ b/qapi/stats.json
@@ -68,6 +68,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:
 #
@@ -85,11 +97,12 @@
 # request statistics and optionally the required subset of information for
 # that target:
 # - which vCPUs to request statistics for
+# - which provider to request statistics from
 #
 # Since: 7.1
 ##
 { 'union': 'StatsFilter',
-        'base': { 'target': 'StatsTarget' },
+        'base': { 'target': 'StatsTarget', '*providers': [ 'StatsRequest' ] },
   'discriminator': 'target',
   'data': { 'vcpu': 'StatsVCPUFilter' } }
 
@@ -225,5 +238,5 @@
 # Since: 7.1
 ##
 { 'command': 'query-stats-schemas',
-  'data': { },
+  'data': { '*provider': 'StatsProvider' },
   'returns': [ 'StatsSchema' ] }
-- 
2.36.0




^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 6/8] hmp: add filtering of statistics by provider
  2022-05-23 15:07 ` [PATCH 1/8] qmp: Support for querying stats Paolo Bonzini
                     ` (3 preceding siblings ...)
  2022-05-23 15:07   ` [PATCH 5/8] qmp: add filtering of statistics by provider Paolo Bonzini
@ 2022-05-23 15:07   ` Paolo Bonzini
  2022-05-23 15:07   ` [PATCH 7/8] qmp: add filtering of statistics by name Paolo Bonzini
                     ` (3 subsequent siblings)
  8 siblings, 0 replies; 22+ messages in thread
From: Paolo Bonzini @ 2022-05-23 15:07 UTC (permalink / raw)
  To: qemu-devel; +Cc: armbru, Mark Kanda

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   | 41 ++++++++++++++++++++++++++++++++++-------
 2 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index 221feab8c0..d4d8a1e618 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 8d2c4792d5..02a455090d 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -2297,6 +2297,7 @@ static StatsSchemaValueList *find_schema_value_list(
 }
 
 static void print_stats_results(Monitor *mon, StatsTarget target,
+                                bool show_provider,
                                 StatsResult *result,
                                 StatsSchemaList *schema)
 {
@@ -2311,8 +2312,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,
@@ -2353,7 +2356,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));
 
@@ -2375,12 +2379,27 @@ 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;
     g_autoptr(StatsSchemaList) schema = NULL;
@@ -2393,19 +2412,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);
+        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();
@@ -2416,7 +2443,7 @@ void hmp_info_stats(Monitor *mon, const QDict *qdict)
         goto exit;
     }
     for (entry = stats; entry; entry = entry->next) {
-        print_stats_results(mon, target, entry->value, schema);
+        print_stats_results(mon, target, provider_str == NULL, entry->value, schema);
     }
 
 exit:
-- 
2.36.0




^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 7/8] qmp: add filtering of statistics by name
  2022-05-23 15:07 ` [PATCH 1/8] qmp: Support for querying stats Paolo Bonzini
                     ` (4 preceding siblings ...)
  2022-05-23 15:07   ` [PATCH 6/8] hmp: " Paolo Bonzini
@ 2022-05-23 15:07   ` Paolo Bonzini
  2022-05-24 13:08     ` Markus Armbruster
  2022-05-23 15:07   ` [PATCH 8/8] hmp: " Paolo Bonzini
                     ` (2 subsequent siblings)
  8 siblings, 1 reply; 22+ messages in thread
From: Paolo Bonzini @ 2022-05-23 15:07 UTC (permalink / raw)
  To: qemu-devel; +Cc: armbru, Mark Kanda

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>
---
v3->v4: handle empty filter early by avoiding the query altogether

 accel/kvm/kvm-all.c     | 17 +++++++++++------
 include/monitor/stats.h |  4 ++--
 monitor/qmp-cmds.c      | 16 +++++++++++++---
 qapi/stats.json         |  6 +++++-
 4 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 461b6cf927..a61d17719e 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 (!apply_str_list_filter(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,7 +4070,7 @@ 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;
     }
@@ -4074,6 +4078,7 @@ static void query_stats_cb(StatsResultList **result, StatsTarget target,
     {
         StatsArgs stats_args;
         stats_args.result.stats = result;
+        stats_args.names = names;
         stats_args.errp = errp;
         CPU_FOREACH(cpu) {
             if (!apply_str_list_filter(cpu->parent_obj.canonical_path, targets)) {
diff --git a/include/monitor/stats.h b/include/monitor/stats.h
index 840c4ed08e..88f046f568 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 7be0e7414a..e822f1b0a9 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -467,15 +467,24 @@ void add_stats_callbacks(StatsProvider provider,
 }
 
 static bool stats_provider_requested(StatsProvider provider,
-                                     StatsFilter *filter)
+                                     StatsFilter *filter,
+                                     strList **p_names)
 {
     StatsRequestList *request;
 
+    *p_names = NULL;
     if (!filter->has_providers) {
         return true;
     }
     for (request = filter->providers; request; request = request->next) {
         if (request->value->provider == provider) {
+            if (request->value->has_names) {
+                if (!request->value->names) {
+                    /* No names allowed is the same as skipping the provider.  */
+                    return false;
+                }
+                *p_names = request->value->names;
+            }
             return true;
         }
     }
@@ -506,8 +515,9 @@ StatsResultList *qmp_query_stats(StatsFilter *filter, Error **errp)
     }
 
     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);
             if (*errp) {
                 qapi_free_StatsResultList(stats_results);
                 return NULL;
diff --git a/qapi/stats.json b/qapi/stats.json
index b75bb86124..2dbf188d36 100644
--- a/qapi/stats.json
+++ b/qapi/stats.json
@@ -74,11 +74,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:
@@ -98,6 +101,7 @@
 # that target:
 # - which vCPUs to request statistics for
 # - which provider to request statistics from
+# - which values to return within each provider
 #
 # Since: 7.1
 ##
-- 
2.36.0




^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 8/8] hmp: add filtering of statistics by name
  2022-05-23 15:07 ` [PATCH 1/8] qmp: Support for querying stats Paolo Bonzini
                     ` (5 preceding siblings ...)
  2022-05-23 15:07   ` [PATCH 7/8] qmp: add filtering of statistics by name Paolo Bonzini
@ 2022-05-23 15:07   ` Paolo Bonzini
  2022-05-24 10:41   ` [PATCH 1/8] qmp: Support for querying stats Markus Armbruster
  2022-05-24 12:20   ` Markus Armbruster
  8 siblings, 0 replies; 22+ messages in thread
From: Paolo Bonzini @ 2022-05-23 15:07 UTC (permalink / raw)
  To: qemu-devel; +Cc: armbru, Mark Kanda

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 d4d8a1e618..767aafd1ea 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 02a455090d..7fe8a9cdc8 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -2356,10 +2356,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) {
@@ -2380,15 +2382,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;
@@ -2398,6 +2412,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;
@@ -2428,11 +2443,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.36.0



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/8] qmp: Support for querying stats
  2022-05-23 15:07 ` [PATCH 1/8] qmp: Support for querying stats Paolo Bonzini
                     ` (6 preceding siblings ...)
  2022-05-23 15:07   ` [PATCH 8/8] hmp: " Paolo Bonzini
@ 2022-05-24 10:41   ` Markus Armbruster
  2022-05-24 16:47     ` Paolo Bonzini
  2022-05-24 12:20   ` Markus Armbruster
  8 siblings, 1 reply; 22+ messages in thread
From: Markus Armbruster @ 2022-05-24 10:41 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel, Mark Kanda

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>



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 3/8] qmp: add filtering of statistics by target vCPU
  2022-05-23 15:07   ` [PATCH 3/8] qmp: add filtering of statistics by target vCPU Paolo Bonzini
@ 2022-05-24 11:20     ` Markus Armbruster
  0 siblings, 0 replies; 22+ messages in thread
From: Markus Armbruster @ 2022-05-24 11:20 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel, Mark Kanda

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]" ] } }
>
> Extracted from a patch by Mark Kanda.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/8] qmp: Support for querying stats
  2022-05-23 15:07 ` [PATCH 1/8] qmp: Support for querying stats Paolo Bonzini
                     ` (7 preceding siblings ...)
  2022-05-24 10:41   ` [PATCH 1/8] qmp: Support for querying stats Markus Armbruster
@ 2022-05-24 12:20   ` Markus Armbruster
  8 siblings, 0 replies; 22+ messages in thread
From: Markus Armbruster @ 2022-05-24 12:20 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel, Mark Kanda

[...]

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



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 5/8] qmp: add filtering of statistics by provider
  2022-05-23 15:07   ` [PATCH 5/8] qmp: add filtering of statistics by provider Paolo Bonzini
@ 2022-05-24 12:32     ` Markus Armbruster
  2022-05-24 16:42       ` Paolo Bonzini
  0 siblings, 1 reply; 22+ messages in thread
From: Markus Armbruster @ 2022-05-24 12:32 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel, armbru, Mark Kanda

Paolo Bonzini <pbonzini@redhat.com> writes:

> 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      | 45 ++++++++++++++++++++++++++++++++---------
>  qapi/stats.json         | 17 ++++++++++++++--
>  5 files changed, 56 insertions(+), 15 deletions(-)
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 3ee431a431..461b6cf927 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 8c50feeaa9..840c4ed08e 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

Argument documentation that merely paraphrases the type is redundant.
May I have a proper contract?

>   * @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 c0cb440902..8d2c4792d5 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -2394,7 +2394,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);

NULL instead of 0 please.

>      if (err) {
>          goto exit;
>      }
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index ebf6f49446..7be0e7414a 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -445,6 +445,7 @@ HumanReadableText *qmp_x_query_irq(Error **errp)
>  }
>  
>  typedef struct StatsCallbacks {
> +    StatsProvider provider;
>      StatRetrieveFunc *stats_cb;
>      SchemaRetrieveFunc *schemas_cb;
>      QTAILQ_ENTRY(StatsCallbacks) next;
> @@ -453,16 +454,34 @@ 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;
>  
>      QTAILQ_INSERT_TAIL(&stats_callbacks, entry, next);
>  }
>  
> +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;
> +}
> +

This is just like apply_str_list_filter().  Good!  Could we make the two
names similar, too?

>  StatsResultList *qmp_query_stats(StatsFilter *filter, Error **errp)
>  {
>      StatsResultList *stats_results = NULL;
> @@ -487,27 +506,33 @@ StatsResultList *qmp_query_stats(StatsFilter *filter, Error **errp)
>      }
>  
>      QTAILQ_FOREACH(entry, &stats_callbacks, next) {
> -        entry->stats_cb(&stats_results, filter->target, targets, errp);
> -        if (*errp) {
> -            qapi_free_StatsResultList(stats_results);
> -            return NULL;
> +        if (stats_provider_requested(entry->provider, filter)) {
> +            entry->stats_cb(&stats_results, filter->target, targets, errp);
> +            if (*errp) {
> +                qapi_free_StatsResultList(stats_results);
> +                return NULL;
> +            }

I like if (!wanted) continue in such loops for less nesting.  Clearly a
matter of taste.

>          }
>      }
>  
>      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;
>      ERRP_GUARD();
>  
>      QTAILQ_FOREACH(entry, &stats_callbacks, next) {
> -        entry->schemas_cb(&stats_results, errp);
> -        if (*errp) {
> -            qapi_free_StatsSchemaList(stats_results);
> -            return NULL;
> +        if (!has_provider || provider == entry->provider) {
> +            entry->schemas_cb(&stats_results, errp);
> +            if (*errp) {
> +                qapi_free_StatsSchemaList(stats_results);
> +                return NULL;
> +            }

Likewise.

>          }
>      }
>  
> diff --git a/qapi/stats.json b/qapi/stats.json
> index e8ed907793..b75bb86124 100644
> --- a/qapi/stats.json
> +++ b/qapi/stats.json
> @@ -68,6 +68,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:
>  #
> @@ -85,11 +97,12 @@
>  # request statistics and optionally the required subset of information for
>  # that target:
>  # - which vCPUs to request statistics for
> +# - which provider to request statistics from
>  #
>  # Since: 7.1
>  ##
>  { 'union': 'StatsFilter',
> -        'base': { 'target': 'StatsTarget' },
> +        'base': { 'target': 'StatsTarget', '*providers': [ 'StatsRequest' ] },

Easier to read, I think:

           'base': {
               'target': 'StatsTarget',
               '*providers': [ 'StatsRequest' ] },

>    'discriminator': 'target',
>    'data': { 'vcpu': 'StatsVCPUFilter' } }
>  
> @@ -225,5 +238,5 @@
>  # Since: 7.1
>  ##
>  { 'command': 'query-stats-schemas',
> -  'data': { },
> +  'data': { '*provider': 'StatsProvider' },
>    'returns': [ 'StatsSchema' ] }

Only the "NULL instead of 0" is a request, the remainder suggestions.
So, with that request addressed
Reviewed-by: Markus Armbruster <armbru@redhat.com>

PS: Consider adding

    [diff]
            orderFile = scripts/git.orderfile

to your .git/config.



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 7/8] qmp: add filtering of statistics by name
  2022-05-23 15:07   ` [PATCH 7/8] qmp: add filtering of statistics by name Paolo Bonzini
@ 2022-05-24 13:08     ` Markus Armbruster
  2022-05-24 16:49       ` Paolo Bonzini
  0 siblings, 1 reply; 22+ messages in thread
From: Markus Armbruster @ 2022-05-24 13:08 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel, Mark Kanda

Paolo Bonzini <pbonzini@redhat.com> writes:

> 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>
> ---
> v3->v4: handle empty filter early by avoiding the query altogether
>
>  accel/kvm/kvm-all.c     | 17 +++++++++++------
>  include/monitor/stats.h |  4 ++--
>  monitor/qmp-cmds.c      | 16 +++++++++++++---
>  qapi/stats.json         |  6 +++++-
>  4 files changed, 31 insertions(+), 12 deletions(-)
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 461b6cf927..a61d17719e 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,

Long line.

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

Long line.

>                          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 (!apply_str_list_filter(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,7 +4070,7 @@ 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;
>      }
> @@ -4074,6 +4078,7 @@ static void query_stats_cb(StatsResultList **result, StatsTarget target,
>      {
>          StatsArgs stats_args;
>          stats_args.result.stats = result;
> +        stats_args.names = names;
>          stats_args.errp = errp;
>          CPU_FOREACH(cpu) {
>              if (!apply_str_list_filter(cpu->parent_obj.canonical_path, targets)) {
> diff --git a/include/monitor/stats.h b/include/monitor/stats.h
> index 840c4ed08e..88f046f568 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 **);

Did you drop the parameter names intentionally?

>  
>  /*
>   * Register callbacks for the QMP query-stats command.
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 7be0e7414a..e822f1b0a9 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -467,15 +467,24 @@ void add_stats_callbacks(StatsProvider provider,
>  }
>  
>  static bool stats_provider_requested(StatsProvider provider,
> -                                     StatsFilter *filter)
> +                                     StatsFilter *filter,
> +                                     strList **p_names)
>  {
>      StatsRequestList *request;
>  
> +    *p_names = NULL;
>      if (!filter->has_providers) {
>          return true;
>      }
>      for (request = filter->providers; request; request = request->next) {
>          if (request->value->provider == provider) {
> +            if (request->value->has_names) {
> +                if (!request->value->names) {
> +                    /* No names allowed is the same as skipping the provider.  */

Long line.

> +                    return false;

Any other elements of filter->providers that match @provider will be
silently ignored.  Is this what you want?

Uh, do we leak @p_names if earlier elements matched?

> +                }
> +                *p_names = request->value->names;
> +            }
>              return true;
>          }
>      }
> @@ -506,8 +515,9 @@ StatsResultList *qmp_query_stats(StatsFilter *filter, Error **errp)
>      }
>  
>      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);

Brain cramp (broken night)... where is @names freed?

Long line.

>              if (*errp) {
>                  qapi_free_StatsResultList(stats_results);
>                  return NULL;
> diff --git a/qapi/stats.json b/qapi/stats.json
> index b75bb86124..2dbf188d36 100644
> --- a/qapi/stats.json
> +++ b/qapi/stats.json
> @@ -74,11 +74,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:
> @@ -98,6 +101,7 @@
>  # that target:
>  # - which vCPUs to request statistics for
>  # - which provider to request statistics from

Should this be "which providers"?

> +# - which values to return within each provider

This is member @provider sub-member @names.  Hmm.  "which named values
to return"?

>  #
>  # Since: 7.1
>  ##



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 5/8] qmp: add filtering of statistics by provider
  2022-05-24 12:32     ` Markus Armbruster
@ 2022-05-24 16:42       ` Paolo Bonzini
  0 siblings, 0 replies; 22+ messages in thread
From: Paolo Bonzini @ 2022-05-24 16:42 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: qemu-devel, Mark Kanda

On 5/24/22 14:32, Markus Armbruster wrote:
>> + * @provider: stats provider
> Argument documentation that merely paraphrases the type is redundant.
> May I have a proper contract?
> 

"stats provider matched against QMP command arguments"?

>>
>> +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;
>> +}
>> +
> 
> This is just like apply_str_list_filter().  Good!  Could we make the two
> names similar, too?

It looks similar but there is a difference in patch 7, in that it also 
returns the "names" filter.  I can rename it to 
find_stats_provider_filter() if you prefer.

All other suggestions applied, thanks.

Paolo


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/8] qmp: Support for querying stats
  2022-05-24 10:41   ` [PATCH 1/8] qmp: Support for querying stats Markus Armbruster
@ 2022-05-24 16:47     ` Paolo Bonzini
  0 siblings, 0 replies; 22+ messages in thread
From: Paolo Bonzini @ 2022-05-24 16:47 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: qemu-devel, Mark Kanda

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


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 7/8] qmp: add filtering of statistics by name
  2022-05-24 13:08     ` Markus Armbruster
@ 2022-05-24 16:49       ` Paolo Bonzini
  2022-05-25  7:49         ` Markus Armbruster
  0 siblings, 1 reply; 22+ messages in thread
From: Paolo Bonzini @ 2022-05-24 16:49 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: qemu-devel, Mark Kanda

On 5/24/22 15:08, Markus Armbruster wrote:
>> -typedef void SchemaRetrieveFunc(StatsSchemaList **result, Error **errp);
>> +                              strList *names, strList *targets, Error **errp);
>> +typedef void SchemaRetrieveFunc(StatsSchemaList **, Error **);
> Did you drop the parameter names intentionally? 

No, I didn't.

>> +                    /* No names allowed is the same as skipping the provider.  */
> 
> Long line.
> 
>> +                    return false;
> 
> Any other elements of filter->providers that match @provider will be
> silently ignored.  Is this what you want?

Hmm, key/value pairs are ugly in QMP.

I'll see if I can make it work nicely without inlining 
stats_provider_requested() in the caller.

> Uh, do we leak @p_names if earlier elements matched?

No, it's not copied so there are no leaks.


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 4/8] hmp: add basic "info stats" implementation
  2022-05-23 15:07   ` [PATCH 4/8] hmp: add basic "info stats" implementation Paolo Bonzini
@ 2022-05-24 17:22     ` Mark Kanda
  0 siblings, 0 replies; 22+ messages in thread
From: Mark Kanda @ 2022-05-24 17:22 UTC (permalink / raw)
  To: Paolo Bonzini, qemu-devel; +Cc: armbru

On 5/23/2022 10:07 AM, Paolo Bonzini wrote:
> 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.
>
As I'm credited as the 'poster' (thank you Paolo), I assume this should be added:

Signed-off-by: Mark Kanda <mark.kanda@oracle.com>

Thanks/regards,
-Mark

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hmp-commands-info.hx  |  13 +++
>   include/monitor/hmp.h |   1 +
>   monitor/hmp-cmds.c    | 187 ++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 201 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 622c783c32..c0cb440902 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"
> @@ -2239,3 +2241,188 @@ 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_HISTOGRAM && 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;
> +    g_autoptr(StatsSchemaList) schema = NULL;
> +    g_autoptr(StatsResultList) stats = NULL;
> +    g_autoptr(StatsFilter) filter = NULL;
> +    StatsResultList *entry;
> +
> +    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;
> +    }
> +    for (entry = stats; entry; entry = entry->next) {
> +        print_stats_results(mon, target, entry->value, schema);
> +    }
> +
> +exit:
> +    if (err) {
> +        monitor_printf(mon, "%s\n", error_get_pretty(err));
> +    }
> +exit_no_print:
> +    error_free(err);
> +}



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 7/8] qmp: add filtering of statistics by name
  2022-05-24 16:49       ` Paolo Bonzini
@ 2022-05-25  7:49         ` Markus Armbruster
  0 siblings, 0 replies; 22+ messages in thread
From: Markus Armbruster @ 2022-05-25  7:49 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel, Mark Kanda

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 5/24/22 15:08, Markus Armbruster wrote:
>>> -typedef void SchemaRetrieveFunc(StatsSchemaList **result, Error **errp);
>>> +                              strList *names, strList *targets, Error **errp);
>>> +typedef void SchemaRetrieveFunc(StatsSchemaList **, Error **);
>>
>> Did you drop the parameter names intentionally? 
>
> No, I didn't.

Easy enough to revert :)

>>> +                    /* No names allowed is the same as skipping the provider.  */
>>
>> Long line.
>> 
>>> +                    return false;
>>
>> Any other elements of filter->providers that match @provider will be
>> silently ignored.  Is this what you want?
>
> Hmm, key/value pairs are ugly in QMP.

Funny, considering what JSON objects are, isn't it?

Ways to do maps in QMP:

1. You can always use a JSON array of objects.  Any combination of
members can be a key.  Any semantic constaint "keys are unique" you get
to enforce manually.

2. If the key is a string, you can use a JSON object.

In either case, you may or may not be able to define a compile-time
static schema.  If you are, then 1.'s schema can be ['UnionType'], where
the key is in the UnionType's base, and 2.'s can be a struct with
optional members.  Else, you get to play with 'any', I guess.

> I'll see if I can make it work nicely without inlining stats_provider_requested() in the caller.
>
>> Uh, do we leak @p_names if earlier elements matched?
>
> No, it's not copied so there are no leaks.



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 3/8] qmp: add filtering of statistics by target vCPU
  2022-05-05 13:45   ` Markus Armbruster
@ 2022-05-05 13:59     ` Paolo Bonzini
  0 siblings, 0 replies; 22+ messages in thread
From: Paolo Bonzini @ 2022-05-05 13:59 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: qemu-devel, mark.kanda, berrange, dgilbert

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



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 3/8] qmp: add filtering of statistics by target vCPU
  2022-04-26 14:16 ` [PATCH 3/8] qmp: add filtering of statistics by target vCPU Paolo Bonzini
@ 2022-05-05 13:45   ` Markus Armbruster
  2022-05-05 13:59     ` Paolo Bonzini
  0 siblings, 1 reply; 22+ messages in thread
From: Markus Armbruster @ 2022-05-05 13:45 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: qemu-devel, mark.kanda, berrange, dgilbert

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:



^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 3/8] qmp: add filtering of statistics by target vCPU
  2022-04-26 14:16 [PATCH 0/8] qmp, hmp: statistics subsystem and KVM suport Paolo Bonzini
@ 2022-04-26 14:16 ` Paolo Bonzini
  2022-05-05 13:45   ` Markus Armbruster
  0 siblings, 1 reply; 22+ messages in thread
From: Paolo Bonzini @ 2022-04-26 14:16 UTC (permalink / raw)
  To: qemu-devel; +Cc: berrange, armbru, dgilbert

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




^ permalink raw reply related	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2022-05-25  7:53 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-23 15:05 [PATCH v4 0/8] qmp, hmp: statistics subsystem and KVM suport Paolo Bonzini
2022-05-23 15:07 ` [PATCH 1/8] qmp: Support for querying stats Paolo Bonzini
2022-05-23 15:07   ` [PATCH 2/8] kvm: Support for querying fd-based stats Paolo Bonzini
2022-05-23 15:07   ` [PATCH 3/8] qmp: add filtering of statistics by target vCPU Paolo Bonzini
2022-05-24 11:20     ` Markus Armbruster
2022-05-23 15:07   ` [PATCH 4/8] hmp: add basic "info stats" implementation Paolo Bonzini
2022-05-24 17:22     ` Mark Kanda
2022-05-23 15:07   ` [PATCH 5/8] qmp: add filtering of statistics by provider Paolo Bonzini
2022-05-24 12:32     ` Markus Armbruster
2022-05-24 16:42       ` Paolo Bonzini
2022-05-23 15:07   ` [PATCH 6/8] hmp: " Paolo Bonzini
2022-05-23 15:07   ` [PATCH 7/8] qmp: add filtering of statistics by name Paolo Bonzini
2022-05-24 13:08     ` Markus Armbruster
2022-05-24 16:49       ` Paolo Bonzini
2022-05-25  7:49         ` Markus Armbruster
2022-05-23 15:07   ` [PATCH 8/8] hmp: " Paolo Bonzini
2022-05-24 10:41   ` [PATCH 1/8] qmp: Support for querying stats Markus Armbruster
2022-05-24 16:47     ` Paolo Bonzini
2022-05-24 12:20   ` Markus Armbruster
  -- strict thread matches above, loose matches on Subject: below --
2022-04-26 14:16 [PATCH 0/8] qmp, hmp: statistics subsystem and KVM suport Paolo Bonzini
2022-04-26 14:16 ` [PATCH 3/8] qmp: add filtering of statistics by target vCPU Paolo Bonzini
2022-05-05 13:45   ` Markus Armbruster
2022-05-05 13:59     ` Paolo Bonzini

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.