[PATCH 0/2] Support fd-based KVM stats

[PATCH 0/2] Support fd-based KVM stats

[Mark Kanda]

This patchset adds QEMU support for querying fd-based KVM stats. The kernel
support is provided by:

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

Patch 1 adds QMP support; patch 2 adds HMP support.

Mark Kanda (2):
  qmp: Support fd-based KVM stats query
  hmp: Support fd-based KVM stats query

 accel/kvm/kvm-all.c   | 246 ++++++++++++++++++++++++++++++++++++++++++
 hmp-commands-info.hx  |  13 +++
 include/monitor/hmp.h |   1 +
 monitor/hmp-cmds.c    |  52 +++++++++
 qapi/misc.json        |  73 +++++++++++++
 5 files changed, 385 insertions(+)

-- 
2.26.2

[PATCH 1/2] qmp: Support fd-based KVM stats query

[Mark Kanda]

Introduce a QMP command 'query-kvmstats' to query KVM for vm and vcpu
fd-based statistics. The kernel support is provided by commit:

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

The interface supports an optional 'filter' argument to specify a
particular stat to query.

Examples:
{ "execute": "query-kvmstats" }

{ "return": [
    { "name": "vm",
      "stats": [
        { "name": "max_mmu_page_hash_collisions",
          "unit": "none",
          "base": 10,
          "val": [ 0 ],
          "exponent": 0,
          "type": "peak" },
        { "name": "nx_lpage_splits",
          "unit": "none",
          "base": 10,
          "val": [ 120 ],
          "exponent": 0,
          "type": "instant" },
...
    } ] },
    { "name": "vcpu_0",
      "stats": [
        { "name": "req_event",
          "unit": "none",
          "base": 10,
          "val": [ 500 ],
          "exponent": 0,
          "type": "cumulative" },
...

{ "execute": "query-kvmstats", "arguments" : { "filter" : "req_event" } }

{ "return": [
    { "name": "vm",
      "stats": [] },
    { "name": "vcpu_0",
      "stats": [
        { "name": "req_event",
          "unit": "none",
          "base": 10,
          "val": [ 500 ],
          "exponent": 0,
          "type": "cumulative" }
    ] },
    { "name": "vcpu_1",
      "stats": [
        { "name": "req_event",
          "unit": "none",
          "base": 10,
          "val": [ 61 ],
          "exponent": 0,
          "type": "cumulative" }
] } ] }

Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
---
 accel/kvm/kvm-all.c | 246 ++++++++++++++++++++++++++++++++++++++++++++
 qapi/misc.json      |  73 +++++++++++++
 2 files changed, 319 insertions(+)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index db8d83b137..597d0c7a09 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 "qapi/qapi-commands-misc.h"
 
 /* This check must be after config-host.h is included */
 #ifdef CONFIG_EVENTFD
@@ -3406,6 +3407,251 @@ int kvm_on_sigbus(int code, void *addr)
 #endif
 }
 
+typedef struct KvmStatsArgs {
+    KvmStats *kvm_stat; /* QAPI auto-generated struct */
+    char *filter;
+    Error **errp;
+} KvmStatsArgs;
+
+static KvmStatDataList *add_kvmstat_entry(struct kvm_stats_desc *pdesc,
+                                          uint64_t *stats_data,
+                                          KvmStatDataList *data_list,
+                                          Error **errp)
+{
+    KvmStatDataList *data_entry;
+    uint64List *val_list = NULL;
+    Error *local_err = NULL;
+    int i;
+
+    data_entry = g_malloc0(sizeof(*data_entry));
+    data_entry->value = g_malloc0(sizeof(*data_entry->value));
+    data_entry->value->name = g_strdup(pdesc->name);
+
+    /* Convert flags to type, unit and base (QAPI auto-generated enums) */
+    switch (pdesc->flags & KVM_STATS_TYPE_MASK) {
+    case KVM_STATS_TYPE_CUMULATIVE:
+        data_entry->value->type = KVM_STAT_TYPE_CUMULATIVE;
+        break;
+    case KVM_STATS_TYPE_INSTANT:
+        data_entry->value->type = KVM_STAT_TYPE_INSTANT;
+        break;
+    case KVM_STATS_TYPE_PEAK:
+        data_entry->value->type = KVM_STAT_TYPE_PEAK;
+        break;
+    default:
+        error_setg(&local_err, "KVM stats: invalid type %u",
+                   (pdesc->flags & KVM_STATS_TYPE_MASK)
+                   >> KVM_STATS_TYPE_SHIFT);
+        goto exit;
+    }
+
+    switch (pdesc->flags & KVM_STATS_UNIT_MASK) {
+    case KVM_STATS_UNIT_NONE:
+        data_entry->value->unit = KVM_STAT_UNIT_NONE;
+        break;
+    case KVM_STATS_UNIT_BYTES:
+        data_entry->value->unit = KVM_STAT_UNIT_BYTES;
+        break;
+    case KVM_STATS_UNIT_CYCLES:
+        data_entry->value->unit = KVM_STAT_UNIT_CYCLES;
+        break;
+    case KVM_STATS_UNIT_SECONDS:
+        data_entry->value->unit = KVM_STAT_UNIT_SECONDS;
+        break;
+    default:
+        error_setg(&local_err, "KVM stats: invalid unit %u",
+                   (pdesc->flags & KVM_STATS_UNIT_MASK)
+                   >> KVM_STATS_UNIT_SHIFT);
+        goto exit;
+    }
+
+    switch (pdesc->flags & KVM_STATS_BASE_MASK) {
+    case KVM_STATS_BASE_POW10:
+        data_entry->value->base = 10;
+        break;
+    case  KVM_STATS_BASE_POW2:
+        data_entry->value->base = 2;
+        break;
+    default:
+        error_setg(&local_err, "KVM stats: invalid base %u",
+                   (pdesc->flags & KVM_STATS_BASE_MASK)
+                   >> KVM_STATS_BASE_SHIFT);
+        goto exit;
+    }
+
+    data_entry->value->exponent = pdesc->exponent;
+
+    /* Alloc and populate data list */
+    for (i = 0; i < pdesc->size; i++) {
+        uint64List *val_entry = g_malloc0(sizeof(*val_entry));
+        val_entry->value = stats_data[i];
+        val_entry->next = val_list;
+        val_list = val_entry;
+    }
+    data_entry->value->val = val_list;
+    data_entry->next = data_list;
+    data_list = data_entry;
+
+    return data_list;
+
+exit:
+    error_propagate(errp, local_err);
+    g_free(data_entry->value->name);
+    g_free(data_entry->value);
+    g_free(data_entry);
+
+    return data_list;
+}
+
+static void query_kvmstats(KvmStatsArgs *kvm_stat_args, int stats_fd)
+{
+    size_t size_desc, size_data;
+    struct kvm_stats_header *header;
+    char *id = NULL;
+    struct kvm_stats_desc *stats_desc = NULL;
+    Error *local_err = NULL;
+    ssize_t ret;
+    int i;
+    KvmStatDataList *data_list = NULL; /* QAPI auto-generated struct */
+
+    /* Read kvm stats header */
+    header = g_malloc(sizeof(*header));
+    ret = read(stats_fd, header, sizeof(*header));
+    if (ret != sizeof(*header)) {
+        error_setg(&local_err, "KVM stats: failed to read stats header: "
+                   "expected %zu actual %zu", sizeof(*header), ret);
+        goto exit;
+    }
+    size_desc = sizeof(*stats_desc) + header->name_size;
+
+    /* Read kvm stats id string */
+    id = g_malloc(header->name_size);
+    ret = read(stats_fd, id, header->name_size);
+    if (ret != header->name_size) {
+        error_setg(&local_err, "KVM stats: failed to read id string: "
+                   "expected %zu actual %zu", (size_t) header->name_size, ret);
+        goto exit;
+    }
+
+    /* Read kvm stats descriptors */
+    stats_desc = g_malloc0(header->num_desc * size_desc);
+    ret = pread(stats_fd, stats_desc,
+                size_desc * header->num_desc, header->desc_offset);
+
+    if (ret != size_desc * header->num_desc) {
+        error_setg(&local_err, "KVM stats: failed to read stats descriptors: "
+                   "expected %zu actual %zu",
+                   size_desc * header->num_desc, ret);
+        goto exit;
+    }
+
+    for (i = 0; i < header->num_desc; ++i) {
+        struct kvm_stats_desc *pdesc = (void *)stats_desc + i * size_desc;
+        size_data = pdesc->size * sizeof(uint64_t);
+        uint64_t *stats_data = g_malloc(size_data);
+
+        ret = pread(stats_fd, stats_data, size_data,
+                    header->data_offset + pdesc->offset);
+
+        if (ret != pdesc->size * sizeof(*stats_data)) {
+            error_setg(&local_err, "KVM stats: failed to read data: "
+                       "expected %zu actual %zu",
+                       pdesc->size * sizeof(*stats_data), ret);
+            g_free(stats_data);
+            goto exit;
+        }
+
+        if (kvm_stat_args->filter) {
+            if (g_strcmp0(kvm_stat_args->filter, pdesc->name)) {
+                g_free(stats_data);
+                continue;
+            }
+        }
+
+        /* Add stats entry to the list */
+        data_list = add_kvmstat_entry(pdesc, stats_data, data_list, &local_err);
+        g_free(stats_data);
+    }
+    kvm_stat_args->kvm_stat->stats = data_list;
+
+exit:
+    error_propagate(kvm_stat_args->errp, local_err);
+    g_free(stats_desc);
+    g_free(id);
+    g_free(header);
+}
+
+static void query_kvmstats_vcpu(CPUState *cpu, run_on_cpu_data data)
+{
+    KvmStatsArgs *kvm_stats_args = (KvmStatsArgs *) 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_kvmstats(kvm_stats_args, stats_fd);
+    close(stats_fd);
+}
+
+KvmStatsList *qmp_query_kvmstats(bool has_filter, const char *filter,
+                                 Error **errp)
+{
+    KvmStatsList *stats_list_head, **stats_list_tail = &stats_list_head;
+    KvmStatsArgs *stats_args;
+    CPUState *cpu;
+    KVMState *s = kvm_state;
+    KvmStats *value;
+    int stats_fd;
+
+    if (!kvm_enabled()) {
+        error_setg(errp, "KVM stats: KVM not enabled");
+        return NULL;
+    }
+
+    if (!kvm_check_extension(s, KVM_CAP_BINARY_STATS_FD)) {
+        error_setg(errp, "KVM stats: not supported");
+        return NULL;
+    }
+
+    stats_args = g_malloc0(sizeof(*stats_args));
+    stats_args->errp = errp;
+
+    if (has_filter) {
+        stats_args->filter = g_strdup(filter);
+    }
+
+    /* Query vm stats */
+    stats_fd = kvm_vm_ioctl(s, KVM_GET_STATS_FD, NULL);
+    if (stats_fd == -1) {
+        error_setg(errp, "KVM stats: ioctl failed");
+        g_free(stats_args);
+        return NULL;
+    }
+    value = g_malloc0(sizeof(*value));
+    value->name = g_strdup("vm");
+    QAPI_LIST_APPEND(stats_list_tail, value);
+
+    stats_args->kvm_stat = value;
+    query_kvmstats(stats_args, stats_fd);
+    close(stats_fd);
+
+    /* Query vcpu stats */
+    CPU_FOREACH(cpu) {
+        KvmStats *value = g_malloc0(sizeof(*value));
+        value->name = g_strdup_printf("vcpu_%d", cpu->cpu_index);
+        QAPI_LIST_APPEND(stats_list_tail, value);
+
+        stats_args->kvm_stat = value;
+        run_on_cpu(cpu, query_kvmstats_vcpu, RUN_ON_CPU_HOST_PTR(stats_args));
+    }
+
+    g_free(stats_args);
+    return stats_list_head;
+}
+
 int kvm_create_device(KVMState *s, uint64_t type, bool test)
 {
     int ret;
diff --git a/qapi/misc.json b/qapi/misc.json
index 5c2ca3b556..0ced6a856c 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -523,3 +523,76 @@
  'data': { '*option': 'str' },
  'returns': ['CommandLineOptionInfo'],
  'allow-preconfig': true }
+
+##
+# @KvmStatType:
+#
+# Enumeration of KVM stat 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.
+#
+# Since: 6.2
+##
+{ 'enum' : 'KvmStatType',
+  'data' : [ 'cumulative', 'instant', 'peak' ] }
+
+##
+# @KvmStatUnit:
+#
+# Enumeration of KVM stat units
+# @bytes: stat reported in bytes.
+# @seconds: stat reported in seconds.
+# @cycles: stat reported in clock cycles.
+# @none: no unit for this stat.
+#
+# Since: 6.2
+##
+{ 'enum' : 'KvmStatUnit',
+  'data' : [ 'bytes', 'seconds', 'cycles', 'none' ] }
+
+##
+# @KvmStatData:
+#
+# Individual KVM stat
+# @name: Stat name
+# @type: @KvmStatType
+# @unit: @KvmStatUnit
+# @base: Exponent base (2 or 10)
+# @exponent: Used together with @base
+# @val: List of uint64 values
+#
+# Since: 6.2
+##
+{ 'struct': 'KvmStatData',
+  'data': { 'name': 'str',
+            'type': 'KvmStatType',
+            'unit': 'KvmStatUnit',
+            'base': 'uint8',
+            'exponent': 'int16',
+            'val': [ 'uint64' ] } }
+
+##
+# @KvmStats:
+#
+# KvmStats per vm or vcpu
+# @name: vm or vcpu_<number>
+# @stats: List of @KvmStatData
+#
+# Since: 6.2
+##
+{ 'struct': 'KvmStats',
+  'data': {'name': 'str',
+           'stats': [ 'KvmStatData' ] } }
+
+##
+# @query-kvmstats:
+#
+# @filter: KVM stat name (optional)
+# Returns: List of @KvmStats
+#
+# Since: 6.2
+##
+{ 'command': 'query-kvmstats',
+  'data': { '*filter': 'str' },
+  'returns': [ 'KvmStats' ] }
-- 
2.26.2

[PATCH 2/2] hmp: Support fd-based KVM stats query

[Mark Kanda]

Leverage the QMP support for fd-based KVM stats.

The interface supports an optional 'filter' argument to specify a
particular stat to query. Base and exponent are displayed in human
readable format.

Examples:

(qemu) info kvmstats

vm:
  max_mmu_page_hash_collisions (peak): 0
  nx_lpage_splits (instant): 114
  lpages (instant): 193
  mmu_unsync (instant): 0
  mmu_cache_miss (cumulative): 293
  mmu_recycled (cumulative): 0
  mmu_flooded (cumulative): 0
  mmu_pde_zapped (cumulative): 0
  mmu_pte_write (cumulative): 0
  mmu_shadow_zapped (cumulative): 178
  remote_tlb_flush (cumulative): 63

vcpu_0:
  req_event (cumulative): 538
  nmi_injections (cumulative): 0
...

(qemu) info kvmstats halt_poll_fail_ns

vm:

vcpu_0:
  halt_poll_fail_ns (cumulative): 20*10^-9 seconds

vcpu_1:
  halt_poll_fail_ns (cumulative): 30*10^-9 seconds

Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
---
 hmp-commands-info.hx  | 13 +++++++++++
 include/monitor/hmp.h |  1 +
 monitor/hmp-cmds.c    | 52 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+)

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index 4c966e8a6b..ef5bca01d9 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -335,6 +335,19 @@ SRST
     Show KVM information.
 ERST
 
+    {
+        .name       = "kvmstats",
+        .args_type  = "filter:s?",
+        .params     = "filter",
+        .help       = "show KVM statistics; optional filter for stat name",
+        .cmd        = hmp_info_kvmstats,
+    },
+
+SRST
+  ``info kvmstats``
+    Show KVM statistics.
+ERST
+
     {
         .name       = "numa",
         .args_type  = "",
diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index 6bc27639e0..20be8f8586 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -21,6 +21,7 @@ void hmp_handle_error(Monitor *mon, Error *err);
 void hmp_info_name(Monitor *mon, const QDict *qdict);
 void hmp_info_version(Monitor *mon, const QDict *qdict);
 void hmp_info_kvm(Monitor *mon, const QDict *qdict);
+void hmp_info_kvmstats(Monitor *mon, const QDict *qdict);
 void hmp_info_status(Monitor *mon, const QDict *qdict);
 void hmp_info_uuid(Monitor *mon, const QDict *qdict);
 void hmp_info_chardev(Monitor *mon, const QDict *qdict);
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index bcaa41350e..24a545a66b 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -134,6 +134,58 @@ void hmp_info_kvm(Monitor *mon, const QDict *qdict)
     qapi_free_KvmInfo(info);
 }
 
+void hmp_info_kvmstats(Monitor *mon, const QDict *qdict)
+{
+    KvmStatsList *stats_list, *stats_list_entry;
+    KvmStats *stats_entry;
+    KvmStatDataList *data_entry;
+    KvmStatData *kvm_stat;
+    uint64List *val;
+    const char *filter;
+    Error *err = NULL;
+
+    filter = qdict_get_try_str(qdict, "filter");
+    if (filter) {
+        stats_list = qmp_query_kvmstats(TRUE, filter, &err);
+    } else {
+        stats_list = qmp_query_kvmstats(FALSE, NULL, &err);
+    }
+
+    if (err) {
+        monitor_printf(mon, "%s\n", error_get_pretty(err));
+        error_free(err);
+        return;
+    }
+
+    for (stats_list_entry = stats_list; stats_list_entry;
+         stats_list_entry = stats_list_entry->next) {
+        stats_entry = stats_list_entry->value;
+        monitor_printf(mon, "\n%s:\n", stats_entry->name);
+
+        for (data_entry = stats_entry->stats; data_entry;
+             data_entry = data_entry->next) {
+            kvm_stat = data_entry->value;
+            monitor_printf(mon, "  %s (%s):", kvm_stat->name,
+                           KvmStatType_str(kvm_stat->type));
+
+            for (val = kvm_stat->val; val; val = val->next) {
+                if (kvm_stat->exponent) {
+                    /* Print the base and exponent as "*<base>^<exp>" */
+                    monitor_printf(mon, " %lu*%d^%d", val->value,
+                                   kvm_stat->base, kvm_stat->exponent);
+                } else {
+                    monitor_printf(mon, " %lu", val->value);
+                }
+            }
+
+            /* Don't print "none" unit type */
+            monitor_printf(mon, " %s\n", kvm_stat->unit == KVM_STAT_UNIT_NONE ?
+                           "" : KvmStatUnit_str(kvm_stat->unit));
+        }
+    }
+    qapi_free_KvmStatsList(stats_list);
+}
+
 void hmp_info_status(Monitor *mon, const QDict *qdict)
 {
     StatusInfo *info;
-- 
2.26.2

Re: [PATCH 0/2] Support fd-based KVM stats

[Paolo Bonzini]

On 19/10/21 22:29, Mark Kanda wrote:
> This patchset adds QEMU support for querying fd-based KVM stats. The kernel
> support is provided by:
> 
> cb082bfab59a ("KVM: stats: Add fd-based API to read binary stats data")
> 
> Patch 1 adds QMP support; patch 2 adds HMP support.

Hi Mark,

that's awesome, thanks!

Just a couple remarks on the implementation:

1) for QMP, it would be nice to have separate commands for the schema 
and the value.  This is because in the future we could consider passing 
the file descriptor directly to the QMP client; in which case, it would 
still be nicer to have QEMU turn the schema information into something 
more easily consumable.

2) for HMP, it would be nice to have the exponent converted to e.g. 
"nanoseconds" if the KvmStatUnit is seconds.  It's not super important, 
but it would be nice.  Something as simple as

if (kvm_stat->unit == KVM_STAT_UNIT_SECONDS &&
     kvm_stat->exponent >= -9 && kvm_stat->exponent <= 0 &&
     kvm_stat->exponent % 3 == 0 && kvm_stat->base == 10) {
     const char *si_prefix[] = { "", "milli", "micro", "nano" };
     monitor_printf(" %lu %sseconds", val->value,
                    si_prefix[kvm_stat->exponent / -3]);
} else ...

(Yes, this is nitpicking; but in practice nanoseconds are the only case 
where currently the exponent is not 0, so...).

3) more recent versions of Linux also support histograms, for which you 
have to include the bucket size and the kind (logarithmic/linear) in 
QMP.  For HMP it would be nice to have it printed as

     halt_wait_hist: [1 nanosecond]=200 [2]=30 [4]=50 [8]=120

but, for the first version, it's also okay if the QMP commands just 
skips unknown KVM_STATS_TYPE values altogether.

Paolo

> Mark Kanda (2):
>    qmp: Support fd-based KVM stats query
>    hmp: Support fd-based KVM stats query
> 
>   accel/kvm/kvm-all.c   | 246 ++++++++++++++++++++++++++++++++++++++++++
>   hmp-commands-info.hx  |  13 +++
>   include/monitor/hmp.h |   1 +
>   monitor/hmp-cmds.c    |  52 +++++++++
>   qapi/misc.json        |  73 +++++++++++++
>   5 files changed, 385 insertions(+)
> 

Re: [PATCH 1/2] qmp: Support fd-based KVM stats query

[Paolo Bonzini]

On 19/10/21 22:29, Mark Kanda wrote:
> +#
> +# @filter: KVM stat name (optional)

Nitpicking on the name, "filter" makes me think that some kind of 
globbing is supported.  So I would just call it "name".

Also nitpicking on the name, let's drop "kvm" from everything.  In the 
future other subsystems could expose statistics, both in the kernel and 
in QEMU (e.g. vhost or TAP; even query-blockstats could be integrated).

Finally, I think we should decouple the description of the stats and the 
description of the sources of said stats.  For example:

    name			type
    ----			----
    vm			kvm-vm
    vcpu_0		kvm-vcpu
    ...

This way the caller knows that the "vcpu_*" stats follow the same 
schema. That would be:

# e.g. name: 'vm', 'type: 'kvm-vm'
# or name: 'vcpu_0', 'type: 'kvm-vcpu'
{ 'struct': 'StatsInstance',
   'data': {'name': 'str', 'type': 'str'}}

{ 'struct': 'StatsSchema',
   'data': {'type': 'str', 'stats': [ 'KvmStatSchema' ] } }

{ 'struct': 'StatsValues',
   'data': {'name': 'str', type': 'str', 'values': [ 'int64' ] } }

{ 'command': 'query-stats-instances',
   'returns': [ 'StatsInstances' ] }

{ 'command': 'query-stats-schemas',
   'data': { '*type': 'str' },
   'returns': [ 'StatsSchema' ] }

{ 'command': 'query-stats',
   'data': { '*name': 'str', '*type': 'str' },
   'returns': [ 'StatsValues' ] }

This should be enough to make the API more future proof.

Paolo