1. Patchew
  2. QEMU
  3. [PATCH v4 00/12] Refactor cryptodev
  4. View patch

[PATCH v4 11/12] cryptodev: Support query-stats QMP command

zhenwei pi posted 12 patches 3 years ago
Maintainers: "Gonglei (Arei)" <arei.gonglei@huawei.com>, Zhenwei Pi <pizhenwei@bytedance.com>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, Markus Armbruster <armbru@redhat.com>, Eric Blake <eblake@redhat.com>, Michael Roth <michael.roth@amd.com>, Paolo Bonzini <pbonzini@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Eduardo Habkost <eduardo@habkost.net>
There is a newer version of this series
[PATCH v4 11/12] cryptodev: Support query-stats QMP command
Posted by zhenwei pi 3 years ago 
Now we can use "query-stats" QMP command to query statistics of
crypto devices. (Originally this was designed to show statistics
by '{"execute": "query-cryptodev"}'. Daniel Berrangé suggested that
querying configuration info by "query-cryptodev", and querying
runtime performance info by "query-stats". This makes sense!)

Example:
~# virsh qemu-monitor-command vm '{"execute": "query-stats", \
   "arguments": {"target": "cryptodev"} }' | jq
{
  "return": [
    {
      "provider": "cryptodev",
      "stats": [
        {
          "name": "asym-verify-bytes",
          "value": 7680
        },
        ...
        {
          "name": "asym-decrypt-ops",
          "value": 32
        },
        {
          "name": "asym-encrypt-ops",
          "value": 48
        }
      ],
      "qom-path": "/objects/cryptodev0" # support asym only
    },
    {
      "provider": "cryptodev",
      "stats": [
        {
          "name": "asym-verify-bytes",
          "value": 0
        },
        ...
        {
          "name": "sym-decrypt-bytes",
          "value": 5376
        },
        ...
      ],
      "qom-path": "/objects/cryptodev1" # support asym/sym
    }
  ],
  "id": "libvirt-422"
}

Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
 backends/cryptodev.c | 141 +++++++++++++++++++++++++++++++++++++++++++
 monitor/hmp-cmds.c   |   5 ++
 monitor/qmp-cmds.c   |   2 +
 qapi/stats.json      |  10 ++-
 4 files changed, 156 insertions(+), 2 deletions(-)

diff --git a/backends/cryptodev.c b/backends/cryptodev.c
index 09ffdd345f..9d52220772 100644
--- a/backends/cryptodev.c
+++ b/backends/cryptodev.c
@@ -22,9 +22,11 @@
  */
 
 #include "qemu/osdep.h"
+#include "monitor/stats.h"
 #include "sysemu/cryptodev.h"
 #include "qapi/error.h"
 #include "qapi/qapi-commands-cryptodev.h"
+#include "qapi/qapi-types-stats.h"
 #include "qapi/visitor.h"
 #include "qemu/config-file.h"
 #include "qemu/error-report.h"
@@ -32,6 +34,14 @@
 #include "qom/object_interfaces.h"
 #include "hw/virtio/virtio-crypto.h"
 
+typedef struct StatsArgs {
+    union StatsResultsType {
+        StatsResultList **stats;
+        StatsSchemaList **schema;
+    } result;
+    strList *names;
+    Error **errp;
+} StatsArgs;
 
 static QTAILQ_HEAD(, CryptoDevBackendClient) crypto_clients;
 
@@ -435,6 +445,134 @@ static void cryptodev_backend_finalize(Object *obj)
     }
 }
 
+static StatsList *cryptodev_backend_stats_add(const char *name, int64_t *val,
+                                              StatsList *stats_list)
+{
+    Stats *stats = g_new0(Stats, 1);
+
+    stats->name = g_strdup(name);
+    stats->value = g_new0(StatsValue, 1);
+    stats->value->type = QTYPE_QNUM;
+    stats->value->u.scalar = *val;
+
+    QAPI_LIST_PREPEND(stats_list, stats);
+    return stats_list;
+}
+
+static int cryptodev_backend_stats_query(Object *obj, void *data)
+{
+    StatsArgs *stats_args = data;
+    StatsResultList **stats_results = stats_args->result.stats;
+    StatsList *stats_list = NULL;
+    StatsResult *entry;
+    CryptoDevBackend *backend;
+    QCryptodevBackendSymStat *sym_stat;
+    QCryptodevBackendAsymStat *asym_stat;
+
+    if (!object_dynamic_cast(obj, TYPE_CRYPTODEV_BACKEND)) {
+        return 0;
+    }
+
+    backend = CRYPTODEV_BACKEND(obj);
+    sym_stat = backend->sym_stat;
+    if (sym_stat) {
+        stats_list = cryptodev_backend_stats_add("sym-encrypt-ops",
+                         &sym_stat->encrypt_ops, stats_list);
+        stats_list = cryptodev_backend_stats_add("sym-decrypt-ops",
+                         &sym_stat->decrypt_ops, stats_list);
+        stats_list = cryptodev_backend_stats_add("sym-encrypt-bytes",
+                         &sym_stat->encrypt_bytes, stats_list);
+        stats_list = cryptodev_backend_stats_add("sym-decrypt-bytes",
+                         &sym_stat->decrypt_bytes, stats_list);
+    }
+
+    asym_stat = backend->asym_stat;
+    if (asym_stat) {
+        stats_list = cryptodev_backend_stats_add("asym-encrypt-ops",
+                         &asym_stat->encrypt_ops, stats_list);
+        stats_list = cryptodev_backend_stats_add("asym-decrypt-ops",
+                         &asym_stat->decrypt_ops, stats_list);
+        stats_list = cryptodev_backend_stats_add("asym-sign-ops",
+                         &asym_stat->sign_ops, stats_list);
+        stats_list = cryptodev_backend_stats_add("asym-verify-ops",
+                         &asym_stat->verify_ops, stats_list);
+        stats_list = cryptodev_backend_stats_add("asym-encrypt-bytes",
+                         &asym_stat->encrypt_bytes, stats_list);
+        stats_list = cryptodev_backend_stats_add("asym-decrypt-bytes",
+                         &asym_stat->decrypt_bytes, stats_list);
+        stats_list = cryptodev_backend_stats_add("asym-sign-bytes",
+                         &asym_stat->sign_bytes, stats_list);
+        stats_list = cryptodev_backend_stats_add("asym-verify-bytes",
+                         &asym_stat->verify_bytes, stats_list);
+    }
+
+    entry = g_new0(StatsResult, 1);
+    entry->provider = STATS_PROVIDER_CRYPTODEV;
+    entry->qom_path = g_strdup(object_get_canonical_path(obj));
+    entry->stats = stats_list;
+    QAPI_LIST_PREPEND(*stats_results, entry);
+
+    return 0;
+}
+
+static void cryptodev_backend_stats_cb(StatsResultList **result,
+                                       StatsTarget target,
+                                       strList *names, strList *targets,
+                                       Error **errp)
+{
+    switch (target) {
+    case STATS_TARGET_CRYPTODEV:
+    {
+        Object *objs = container_get(object_get_root(), "/objects");
+        StatsArgs stats_args;
+        stats_args.result.stats = result;
+        stats_args.names = names;
+        stats_args.errp = errp;
+
+        object_child_foreach(objs, cryptodev_backend_stats_query, &stats_args);
+        break;
+    }
+    default:
+        break;
+    }
+}
+
+static StatsSchemaValueList *cryptodev_backend_schemas_add(const char *name,
+                                 StatsSchemaValueList *list)
+{
+    StatsSchemaValueList *schema_entry = g_new0(StatsSchemaValueList, 1);
+
+    schema_entry->value = g_new0(StatsSchemaValue, 1);
+    schema_entry->value->type = STATS_TYPE_CUMULATIVE;
+    schema_entry->value->name = g_strdup(name);
+    schema_entry->next = list;
+
+    return schema_entry;
+}
+
+static void cryptodev_backend_schemas_cb(StatsSchemaList **result,
+                                         Error **errp)
+{
+    StatsSchemaValueList *stats_list = NULL;
+    const char *sym_stats[] = {"sym-encrypt-ops", "sym-decrypt-ops",
+                               "sym-encrypt-bytes", "sym-decrypt-bytes"};
+    const char *asym_stats[] = {"asym-encrypt-ops", "asym-decrypt-ops",
+                                "asym-sign-ops", "asym-verify-ops",
+                                "asym-encrypt-bytes", "asym-decrypt-bytes",
+                                "asym-sign-bytes", "asym-verify-bytes"};
+
+    for (int i = 0; i < ARRAY_SIZE(sym_stats); i++) {
+        stats_list = cryptodev_backend_schemas_add(sym_stats[i], stats_list);
+    }
+
+    for (int i = 0; i < ARRAY_SIZE(asym_stats); i++) {
+        stats_list = cryptodev_backend_schemas_add(asym_stats[i], stats_list);
+    }
+
+    add_stats_schema(result, STATS_PROVIDER_CRYPTODEV, STATS_TARGET_CRYPTODEV,
+                     stats_list);
+}
+
 static void
 cryptodev_backend_class_init(ObjectClass *oc, void *data)
 {
@@ -456,6 +594,9 @@ cryptodev_backend_class_init(ObjectClass *oc, void *data)
                               cryptodev_backend_get_ops,
                               cryptodev_backend_set_ops,
                               NULL, NULL);
+
+    add_stats_callbacks(STATS_PROVIDER_CRYPTODEV, cryptodev_backend_stats_cb,
+                        cryptodev_backend_schemas_cb);
 }
 
 static const TypeInfo cryptodev_backend_info = {
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index cda52c2526..dda69a1098 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1883,6 +1883,8 @@ static StatsFilter *stats_filter(StatsTarget target, const char *names,
         filter->u.vcpu.vcpus = vcpu_list;
         break;
     }
+    case STATS_TARGET_CRYPTODEV:
+        break;
     default:
         break;
     }
@@ -1954,6 +1956,9 @@ void hmp_info_stats(Monitor *mon, const QDict *qdict)
         int cpu_index = monitor_get_cpu_index(mon);
         filter = stats_filter(target, names, cpu_index, provider);
         break;
+    case STATS_TARGET_CRYPTODEV:
+        filter = stats_filter(target, names, -1, provider);
+        break;
     default:
         abort();
     }
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index bf22a8c5a6..dd31936f6a 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -385,6 +385,8 @@ static bool invoke_stats_cb(StatsCallbacks *entry,
             targets = filter->u.vcpu.vcpus;
         }
         break;
+    case STATS_TARGET_CRYPTODEV:
+        break;
     default:
         abort();
     }
diff --git a/qapi/stats.json b/qapi/stats.json
index 57db5b1c74..f9dec18066 100644
--- a/qapi/stats.json
+++ b/qapi/stats.json
@@ -50,10 +50,14 @@
 #
 # Enumeration of statistics providers.
 #
+# @kvm: since 7.1
+#
+# @cryptodev: since 8.0
+#
 # Since: 7.1
 ##
 { 'enum': 'StatsProvider',
-  'data': [ 'kvm' ] }
+  'data': [ 'kvm', 'cryptodev' ] }
 
 ##
 # @StatsTarget:
@@ -65,10 +69,12 @@
 #
 # @vcpu: statistics that apply to a single virtual CPU.
 #
+# @cryptodev: statistics that apply to a crypto device.
+#
 # Since: 7.1
 ##
 { 'enum': 'StatsTarget',
-  'data': [ 'vm', 'vcpu' ] }
+  'data': [ 'vm', 'vcpu', 'cryptodev' ] }
 
 ##
 # @StatsRequest:
-- 
2.34.1
Re: [PATCH v4 11/12] cryptodev: Support query-stats QMP command
Posted by Michael S. Tsirkin 2 years, 11 months ago 
On Sun, Jan 29, 2023 at 10:57:46AM +0800, zhenwei pi wrote:
> Now we can use "query-stats" QMP command to query statistics of
> crypto devices. (Originally this was designed to show statistics
> by '{"execute": "query-cryptodev"}'. Daniel Berrangé suggested that
> querying configuration info by "query-cryptodev", and querying
> runtime performance info by "query-stats". This makes sense!)
> 
> Example:
> ~# virsh qemu-monitor-command vm '{"execute": "query-stats", \
>    "arguments": {"target": "cryptodev"} }' | jq
> {
>   "return": [
>     {
>       "provider": "cryptodev",
>       "stats": [
>         {
>           "name": "asym-verify-bytes",
>           "value": 7680
>         },
>         ...
>         {
>           "name": "asym-decrypt-ops",
>           "value": 32
>         },
>         {
>           "name": "asym-encrypt-ops",
>           "value": 48
>         }
>       ],
>       "qom-path": "/objects/cryptodev0" # support asym only
>     },
>     {
>       "provider": "cryptodev",
>       "stats": [
>         {
>           "name": "asym-verify-bytes",
>           "value": 0
>         },
>         ...
>         {
>           "name": "sym-decrypt-bytes",
>           "value": 5376
>         },
>         ...
>       ],
>       "qom-path": "/objects/cryptodev1" # support asym/sym
>     }
>   ],
>   "id": "libvirt-422"
> }
> 
> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>

I assume since this has been out a long time and no
comments by maintainers it's ok from QAPI POV.


> ---
>  backends/cryptodev.c | 141 +++++++++++++++++++++++++++++++++++++++++++
>  monitor/hmp-cmds.c   |   5 ++
>  monitor/qmp-cmds.c   |   2 +
>  qapi/stats.json      |  10 ++-
>  4 files changed, 156 insertions(+), 2 deletions(-)
> 
> diff --git a/backends/cryptodev.c b/backends/cryptodev.c
> index 09ffdd345f..9d52220772 100644
> --- a/backends/cryptodev.c
> +++ b/backends/cryptodev.c
> @@ -22,9 +22,11 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "monitor/stats.h"
>  #include "sysemu/cryptodev.h"
>  #include "qapi/error.h"
>  #include "qapi/qapi-commands-cryptodev.h"
> +#include "qapi/qapi-types-stats.h"
>  #include "qapi/visitor.h"
>  #include "qemu/config-file.h"
>  #include "qemu/error-report.h"
> @@ -32,6 +34,14 @@
>  #include "qom/object_interfaces.h"
>  #include "hw/virtio/virtio-crypto.h"
>  
> +typedef struct StatsArgs {
> +    union StatsResultsType {
> +        StatsResultList **stats;
> +        StatsSchemaList **schema;
> +    } result;
> +    strList *names;
> +    Error **errp;
> +} StatsArgs;
>  
>  static QTAILQ_HEAD(, CryptoDevBackendClient) crypto_clients;
>  
> @@ -435,6 +445,134 @@ static void cryptodev_backend_finalize(Object *obj)
>      }
>  }
>  
> +static StatsList *cryptodev_backend_stats_add(const char *name, int64_t *val,
> +                                              StatsList *stats_list)
> +{
> +    Stats *stats = g_new0(Stats, 1);
> +
> +    stats->name = g_strdup(name);
> +    stats->value = g_new0(StatsValue, 1);
> +    stats->value->type = QTYPE_QNUM;
> +    stats->value->u.scalar = *val;
> +
> +    QAPI_LIST_PREPEND(stats_list, stats);
> +    return stats_list;
> +}
> +
> +static int cryptodev_backend_stats_query(Object *obj, void *data)
> +{
> +    StatsArgs *stats_args = data;
> +    StatsResultList **stats_results = stats_args->result.stats;
> +    StatsList *stats_list = NULL;
> +    StatsResult *entry;
> +    CryptoDevBackend *backend;
> +    QCryptodevBackendSymStat *sym_stat;
> +    QCryptodevBackendAsymStat *asym_stat;
> +
> +    if (!object_dynamic_cast(obj, TYPE_CRYPTODEV_BACKEND)) {
> +        return 0;
> +    }
> +
> +    backend = CRYPTODEV_BACKEND(obj);
> +    sym_stat = backend->sym_stat;
> +    if (sym_stat) {
> +        stats_list = cryptodev_backend_stats_add("sym-encrypt-ops",
> +                         &sym_stat->encrypt_ops, stats_list);
> +        stats_list = cryptodev_backend_stats_add("sym-decrypt-ops",
> +                         &sym_stat->decrypt_ops, stats_list);
> +        stats_list = cryptodev_backend_stats_add("sym-encrypt-bytes",
> +                         &sym_stat->encrypt_bytes, stats_list);
> +        stats_list = cryptodev_backend_stats_add("sym-decrypt-bytes",
> +                         &sym_stat->decrypt_bytes, stats_list);
> +    }
> +
> +    asym_stat = backend->asym_stat;
> +    if (asym_stat) {
> +        stats_list = cryptodev_backend_stats_add("asym-encrypt-ops",
> +                         &asym_stat->encrypt_ops, stats_list);
> +        stats_list = cryptodev_backend_stats_add("asym-decrypt-ops",
> +                         &asym_stat->decrypt_ops, stats_list);
> +        stats_list = cryptodev_backend_stats_add("asym-sign-ops",
> +                         &asym_stat->sign_ops, stats_list);
> +        stats_list = cryptodev_backend_stats_add("asym-verify-ops",
> +                         &asym_stat->verify_ops, stats_list);
> +        stats_list = cryptodev_backend_stats_add("asym-encrypt-bytes",
> +                         &asym_stat->encrypt_bytes, stats_list);
> +        stats_list = cryptodev_backend_stats_add("asym-decrypt-bytes",
> +                         &asym_stat->decrypt_bytes, stats_list);
> +        stats_list = cryptodev_backend_stats_add("asym-sign-bytes",
> +                         &asym_stat->sign_bytes, stats_list);
> +        stats_list = cryptodev_backend_stats_add("asym-verify-bytes",
> +                         &asym_stat->verify_bytes, stats_list);
> +    }
> +
> +    entry = g_new0(StatsResult, 1);
> +    entry->provider = STATS_PROVIDER_CRYPTODEV;
> +    entry->qom_path = g_strdup(object_get_canonical_path(obj));
> +    entry->stats = stats_list;
> +    QAPI_LIST_PREPEND(*stats_results, entry);
> +
> +    return 0;
> +}
> +
> +static void cryptodev_backend_stats_cb(StatsResultList **result,
> +                                       StatsTarget target,
> +                                       strList *names, strList *targets,
> +                                       Error **errp)
> +{
> +    switch (target) {
> +    case STATS_TARGET_CRYPTODEV:
> +    {
> +        Object *objs = container_get(object_get_root(), "/objects");
> +        StatsArgs stats_args;
> +        stats_args.result.stats = result;
> +        stats_args.names = names;
> +        stats_args.errp = errp;
> +
> +        object_child_foreach(objs, cryptodev_backend_stats_query, &stats_args);
> +        break;
> +    }
> +    default:
> +        break;
> +    }
> +}
> +
> +static StatsSchemaValueList *cryptodev_backend_schemas_add(const char *name,
> +                                 StatsSchemaValueList *list)
> +{
> +    StatsSchemaValueList *schema_entry = g_new0(StatsSchemaValueList, 1);
> +
> +    schema_entry->value = g_new0(StatsSchemaValue, 1);
> +    schema_entry->value->type = STATS_TYPE_CUMULATIVE;
> +    schema_entry->value->name = g_strdup(name);
> +    schema_entry->next = list;
> +
> +    return schema_entry;
> +}
> +
> +static void cryptodev_backend_schemas_cb(StatsSchemaList **result,
> +                                         Error **errp)
> +{
> +    StatsSchemaValueList *stats_list = NULL;
> +    const char *sym_stats[] = {"sym-encrypt-ops", "sym-decrypt-ops",
> +                               "sym-encrypt-bytes", "sym-decrypt-bytes"};
> +    const char *asym_stats[] = {"asym-encrypt-ops", "asym-decrypt-ops",
> +                                "asym-sign-ops", "asym-verify-ops",
> +                                "asym-encrypt-bytes", "asym-decrypt-bytes",
> +                                "asym-sign-bytes", "asym-verify-bytes"};
> +
> +    for (int i = 0; i < ARRAY_SIZE(sym_stats); i++) {
> +        stats_list = cryptodev_backend_schemas_add(sym_stats[i], stats_list);
> +    }
> +
> +    for (int i = 0; i < ARRAY_SIZE(asym_stats); i++) {
> +        stats_list = cryptodev_backend_schemas_add(asym_stats[i], stats_list);
> +    }
> +
> +    add_stats_schema(result, STATS_PROVIDER_CRYPTODEV, STATS_TARGET_CRYPTODEV,
> +                     stats_list);
> +}
> +
>  static void
>  cryptodev_backend_class_init(ObjectClass *oc, void *data)
>  {
> @@ -456,6 +594,9 @@ cryptodev_backend_class_init(ObjectClass *oc, void *data)
>                                cryptodev_backend_get_ops,
>                                cryptodev_backend_set_ops,
>                                NULL, NULL);
> +
> +    add_stats_callbacks(STATS_PROVIDER_CRYPTODEV, cryptodev_backend_stats_cb,
> +                        cryptodev_backend_schemas_cb);
>  }
>  
>  static const TypeInfo cryptodev_backend_info = {
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index cda52c2526..dda69a1098 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1883,6 +1883,8 @@ static StatsFilter *stats_filter(StatsTarget target, const char *names,
>          filter->u.vcpu.vcpus = vcpu_list;
>          break;
>      }
> +    case STATS_TARGET_CRYPTODEV:
> +        break;
>      default:
>          break;
>      }
> @@ -1954,6 +1956,9 @@ void hmp_info_stats(Monitor *mon, const QDict *qdict)
>          int cpu_index = monitor_get_cpu_index(mon);
>          filter = stats_filter(target, names, cpu_index, provider);
>          break;
> +    case STATS_TARGET_CRYPTODEV:
> +        filter = stats_filter(target, names, -1, provider);
> +        break;
>      default:
>          abort();
>      }
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index bf22a8c5a6..dd31936f6a 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -385,6 +385,8 @@ static bool invoke_stats_cb(StatsCallbacks *entry,
>              targets = filter->u.vcpu.vcpus;
>          }
>          break;
> +    case STATS_TARGET_CRYPTODEV:
> +        break;
>      default:
>          abort();
>      }
> diff --git a/qapi/stats.json b/qapi/stats.json
> index 57db5b1c74..f9dec18066 100644
> --- a/qapi/stats.json
> +++ b/qapi/stats.json
> @@ -50,10 +50,14 @@
>  #
>  # Enumeration of statistics providers.
>  #
> +# @kvm: since 7.1
> +#
> +# @cryptodev: since 8.0
> +#
>  # Since: 7.1
>  ##
>  { 'enum': 'StatsProvider',
> -  'data': [ 'kvm' ] }
> +  'data': [ 'kvm', 'cryptodev' ] }
>  
>  ##
>  # @StatsTarget:
> @@ -65,10 +69,12 @@
>  #
>  # @vcpu: statistics that apply to a single virtual CPU.
>  #
> +# @cryptodev: statistics that apply to a crypto device.
> +#
>  # Since: 7.1
>  ##
>  { 'enum': 'StatsTarget',
> -  'data': [ 'vm', 'vcpu' ] }
> +  'data': [ 'vm', 'vcpu', 'cryptodev' ] }
>  
>  ##
>  # @StatsRequest:
> -- 
> 2.34.1
Re: [PATCH v4 11/12] cryptodev: Support query-stats QMP command
Posted by Daniel P. Berrangé 2 years, 11 months ago 
On Tue, Feb 28, 2023 at 07:56:14AM -0500, Michael S. Tsirkin wrote:
> On Sun, Jan 29, 2023 at 10:57:46AM +0800, zhenwei pi wrote:
> > Now we can use "query-stats" QMP command to query statistics of
> > crypto devices. (Originally this was designed to show statistics
> > by '{"execute": "query-cryptodev"}'. Daniel Berrangé suggested that
> > querying configuration info by "query-cryptodev", and querying
> > runtime performance info by "query-stats". This makes sense!)
> > 
> > Example:
> > ~# virsh qemu-monitor-command vm '{"execute": "query-stats", \
> >    "arguments": {"target": "cryptodev"} }' | jq
> > {
> >   "return": [
> >     {
> >       "provider": "cryptodev",
> >       "stats": [
> >         {
> >           "name": "asym-verify-bytes",
> >           "value": 7680
> >         },
> >         ...
> >         {
> >           "name": "asym-decrypt-ops",
> >           "value": 32
> >         },
> >         {
> >           "name": "asym-encrypt-ops",
> >           "value": 48
> >         }
> >       ],
> >       "qom-path": "/objects/cryptodev0" # support asym only
> >     },
> >     {
> >       "provider": "cryptodev",
> >       "stats": [
> >         {
> >           "name": "asym-verify-bytes",
> >           "value": 0
> >         },
> >         ...
> >         {
> >           "name": "sym-decrypt-bytes",
> >           "value": 5376
> >         },
> >         ...
> >       ],
> >       "qom-path": "/objects/cryptodev1" # support asym/sym
> >     }
> >   ],
> >   "id": "libvirt-422"
> > }
> > 
> > Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> > Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> 
> I assume since this has been out a long time and no
> comments by maintainers it's ok from QAPI POV.

I'm not the QAPI maintainer, but I think this worked out
pretty nicely compared to the previous versions of the
series which didn't use query-stats.. just a minor point
below.

> 
> > ---
> >  backends/cryptodev.c | 141 +++++++++++++++++++++++++++++++++++++++++++
> >  monitor/hmp-cmds.c   |   5 ++
> >  monitor/qmp-cmds.c   |   2 +
> >  qapi/stats.json      |  10 ++-
> >  4 files changed, 156 insertions(+), 2 deletions(-)
> > 
> > diff --git a/backends/cryptodev.c b/backends/cryptodev.c
> > index 09ffdd345f..9d52220772 100644
> > --- a/backends/cryptodev.c
> > +++ b/backends/cryptodev.c
> > @@ -22,9 +22,11 @@
> >   */
> >  
> >  #include "qemu/osdep.h"
> > +#include "monitor/stats.h"
> >  #include "sysemu/cryptodev.h"
> >  #include "qapi/error.h"
> >  #include "qapi/qapi-commands-cryptodev.h"
> > +#include "qapi/qapi-types-stats.h"
> >  #include "qapi/visitor.h"
> >  #include "qemu/config-file.h"
> >  #include "qemu/error-report.h"
> > @@ -32,6 +34,14 @@
> >  #include "qom/object_interfaces.h"
> >  #include "hw/virtio/virtio-crypto.h"
> >  
> > +typedef struct StatsArgs {
> > +    union StatsResultsType {
> > +        StatsResultList **stats;
> > +        StatsSchemaList **schema;
> > +    } result;
> > +    strList *names;
> > +    Error **errp;
> > +} StatsArgs;
> >  
> >  static QTAILQ_HEAD(, CryptoDevBackendClient) crypto_clients;
> >  
> > @@ -435,6 +445,134 @@ static void cryptodev_backend_finalize(Object *obj)
> >      }
> >  }
> >  
> > +static StatsList *cryptodev_backend_stats_add(const char *name, int64_t *val,
> > +                                              StatsList *stats_list)
> > +{
> > +    Stats *stats = g_new0(Stats, 1);
> > +
> > +    stats->name = g_strdup(name);
> > +    stats->value = g_new0(StatsValue, 1);
> > +    stats->value->type = QTYPE_QNUM;
> > +    stats->value->u.scalar = *val;
> > +
> > +    QAPI_LIST_PREPEND(stats_list, stats);
> > +    return stats_list;
> > +}
> > +
> > +static int cryptodev_backend_stats_query(Object *obj, void *data)
> > +{
> > +    StatsArgs *stats_args = data;
> > +    StatsResultList **stats_results = stats_args->result.stats;
> > +    StatsList *stats_list = NULL;
> > +    StatsResult *entry;
> > +    CryptoDevBackend *backend;
> > +    QCryptodevBackendSymStat *sym_stat;
> > +    QCryptodevBackendAsymStat *asym_stat;
> > +
> > +    if (!object_dynamic_cast(obj, TYPE_CRYPTODEV_BACKEND)) {
> > +        return 0;
> > +    }
> > +
> > +    backend = CRYPTODEV_BACKEND(obj);
> > +    sym_stat = backend->sym_stat;
> > +    if (sym_stat) {
> > +        stats_list = cryptodev_backend_stats_add("sym-encrypt-ops",
> > +                         &sym_stat->encrypt_ops, stats_list);
> > +        stats_list = cryptodev_backend_stats_add("sym-decrypt-ops",
> > +                         &sym_stat->decrypt_ops, stats_list);
> > +        stats_list = cryptodev_backend_stats_add("sym-encrypt-bytes",
> > +                         &sym_stat->encrypt_bytes, stats_list);
> > +        stats_list = cryptodev_backend_stats_add("sym-decrypt-bytes",
> > +                         &sym_stat->decrypt_bytes, stats_list);
> > +    }
> > +
> > +    asym_stat = backend->asym_stat;
> > +    if (asym_stat) {
> > +        stats_list = cryptodev_backend_stats_add("asym-encrypt-ops",
> > +                         &asym_stat->encrypt_ops, stats_list);
> > +        stats_list = cryptodev_backend_stats_add("asym-decrypt-ops",
> > +                         &asym_stat->decrypt_ops, stats_list);
> > +        stats_list = cryptodev_backend_stats_add("asym-sign-ops",
> > +                         &asym_stat->sign_ops, stats_list);
> > +        stats_list = cryptodev_backend_stats_add("asym-verify-ops",
> > +                         &asym_stat->verify_ops, stats_list);
> > +        stats_list = cryptodev_backend_stats_add("asym-encrypt-bytes",
> > +                         &asym_stat->encrypt_bytes, stats_list);
> > +        stats_list = cryptodev_backend_stats_add("asym-decrypt-bytes",
> > +                         &asym_stat->decrypt_bytes, stats_list);
> > +        stats_list = cryptodev_backend_stats_add("asym-sign-bytes",
> > +                         &asym_stat->sign_bytes, stats_list);
> > +        stats_list = cryptodev_backend_stats_add("asym-verify-bytes",
> > +                         &asym_stat->verify_bytes, stats_list);

Perhaps there's benefit in having constants defined for these strings,
since they become part of our API, and the same strings are repeated
again further down.....

> > +    }
> > +
> > +    entry = g_new0(StatsResult, 1);
> > +    entry->provider = STATS_PROVIDER_CRYPTODEV;
> > +    entry->qom_path = g_strdup(object_get_canonical_path(obj));
> > +    entry->stats = stats_list;
> > +    QAPI_LIST_PREPEND(*stats_results, entry);
> > +
> > +    return 0;
> > +}

...snip...

> > +
> > +static void cryptodev_backend_schemas_cb(StatsSchemaList **result,
> > +                                         Error **errp)
> > +{
> > +    StatsSchemaValueList *stats_list = NULL;
> > +    const char *sym_stats[] = {"sym-encrypt-ops", "sym-decrypt-ops",
> > +                               "sym-encrypt-bytes", "sym-decrypt-bytes"};
> > +    const char *asym_stats[] = {"asym-encrypt-ops", "asym-decrypt-ops",
> > +                                "asym-sign-ops", "asym-verify-ops",
> > +                                "asym-encrypt-bytes", "asym-decrypt-bytes",
> > +                                "asym-sign-bytes", "asym-verify-bytes"};

... here's the repeated use of the constant strings from earlier

> > +
> > +    for (int i = 0; i < ARRAY_SIZE(sym_stats); i++) {
> > +        stats_list = cryptodev_backend_schemas_add(sym_stats[i], stats_list);
> > +    }
> > +
> > +    for (int i = 0; i < ARRAY_SIZE(asym_stats); i++) {
> > +        stats_list = cryptodev_backend_schemas_add(asym_stats[i], stats_list);
> > +    }
> > +
> > +    add_stats_schema(result, STATS_PROVIDER_CRYPTODEV, STATS_TARGET_CRYPTODEV,
> > +                     stats_list);
> > +}

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [PATCH v4 11/12] cryptodev: Support query-stats QMP command
Posted by Michael S. Tsirkin 2 years, 11 months ago 
On Tue, Feb 28, 2023 at 01:17:52PM +0000, Daniel P. Berrangé wrote:
> On Tue, Feb 28, 2023 at 07:56:14AM -0500, Michael S. Tsirkin wrote:
> > On Sun, Jan 29, 2023 at 10:57:46AM +0800, zhenwei pi wrote:
> > > Now we can use "query-stats" QMP command to query statistics of
> > > crypto devices. (Originally this was designed to show statistics
> > > by '{"execute": "query-cryptodev"}'. Daniel Berrangé suggested that
> > > querying configuration info by "query-cryptodev", and querying
> > > runtime performance info by "query-stats". This makes sense!)
> > > 
> > > Example:
> > > ~# virsh qemu-monitor-command vm '{"execute": "query-stats", \
> > >    "arguments": {"target": "cryptodev"} }' | jq
> > > {
> > >   "return": [
> > >     {
> > >       "provider": "cryptodev",
> > >       "stats": [
> > >         {
> > >           "name": "asym-verify-bytes",
> > >           "value": 7680
> > >         },
> > >         ...
> > >         {
> > >           "name": "asym-decrypt-ops",
> > >           "value": 32
> > >         },
> > >         {
> > >           "name": "asym-encrypt-ops",
> > >           "value": 48
> > >         }
> > >       ],
> > >       "qom-path": "/objects/cryptodev0" # support asym only
> > >     },
> > >     {
> > >       "provider": "cryptodev",
> > >       "stats": [
> > >         {
> > >           "name": "asym-verify-bytes",
> > >           "value": 0
> > >         },
> > >         ...
> > >         {
> > >           "name": "sym-decrypt-bytes",
> > >           "value": 5376
> > >         },
> > >         ...
> > >       ],
> > >       "qom-path": "/objects/cryptodev1" # support asym/sym
> > >     }
> > >   ],
> > >   "id": "libvirt-422"
> > > }
> > > 
> > > Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> > > Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> > 
> > I assume since this has been out a long time and no
> > comments by maintainers it's ok from QAPI POV.
> 
> I'm not the QAPI maintainer, but I think this worked out
> pretty nicely compared to the previous versions of the
> series which didn't use query-stats.. just a minor point
> below.

Hmm applied already ... is this ok to fix with patch on top
or do I have to revert?


> > 
> > > ---
> > >  backends/cryptodev.c | 141 +++++++++++++++++++++++++++++++++++++++++++
> > >  monitor/hmp-cmds.c   |   5 ++
> > >  monitor/qmp-cmds.c   |   2 +
> > >  qapi/stats.json      |  10 ++-
> > >  4 files changed, 156 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/backends/cryptodev.c b/backends/cryptodev.c
> > > index 09ffdd345f..9d52220772 100644
> > > --- a/backends/cryptodev.c
> > > +++ b/backends/cryptodev.c
> > > @@ -22,9 +22,11 @@
> > >   */
> > >  
> > >  #include "qemu/osdep.h"
> > > +#include "monitor/stats.h"
> > >  #include "sysemu/cryptodev.h"
> > >  #include "qapi/error.h"
> > >  #include "qapi/qapi-commands-cryptodev.h"
> > > +#include "qapi/qapi-types-stats.h"
> > >  #include "qapi/visitor.h"
> > >  #include "qemu/config-file.h"
> > >  #include "qemu/error-report.h"
> > > @@ -32,6 +34,14 @@
> > >  #include "qom/object_interfaces.h"
> > >  #include "hw/virtio/virtio-crypto.h"
> > >  
> > > +typedef struct StatsArgs {
> > > +    union StatsResultsType {
> > > +        StatsResultList **stats;
> > > +        StatsSchemaList **schema;
> > > +    } result;
> > > +    strList *names;
> > > +    Error **errp;
> > > +} StatsArgs;
> > >  
> > >  static QTAILQ_HEAD(, CryptoDevBackendClient) crypto_clients;
> > >  
> > > @@ -435,6 +445,134 @@ static void cryptodev_backend_finalize(Object *obj)
> > >      }
> > >  }
> > >  
> > > +static StatsList *cryptodev_backend_stats_add(const char *name, int64_t *val,
> > > +                                              StatsList *stats_list)
> > > +{
> > > +    Stats *stats = g_new0(Stats, 1);
> > > +
> > > +    stats->name = g_strdup(name);
> > > +    stats->value = g_new0(StatsValue, 1);
> > > +    stats->value->type = QTYPE_QNUM;
> > > +    stats->value->u.scalar = *val;
> > > +
> > > +    QAPI_LIST_PREPEND(stats_list, stats);
> > > +    return stats_list;
> > > +}
> > > +
> > > +static int cryptodev_backend_stats_query(Object *obj, void *data)
> > > +{
> > > +    StatsArgs *stats_args = data;
> > > +    StatsResultList **stats_results = stats_args->result.stats;
> > > +    StatsList *stats_list = NULL;
> > > +    StatsResult *entry;
> > > +    CryptoDevBackend *backend;
> > > +    QCryptodevBackendSymStat *sym_stat;
> > > +    QCryptodevBackendAsymStat *asym_stat;
> > > +
> > > +    if (!object_dynamic_cast(obj, TYPE_CRYPTODEV_BACKEND)) {
> > > +        return 0;
> > > +    }
> > > +
> > > +    backend = CRYPTODEV_BACKEND(obj);
> > > +    sym_stat = backend->sym_stat;
> > > +    if (sym_stat) {
> > > +        stats_list = cryptodev_backend_stats_add("sym-encrypt-ops",
> > > +                         &sym_stat->encrypt_ops, stats_list);
> > > +        stats_list = cryptodev_backend_stats_add("sym-decrypt-ops",
> > > +                         &sym_stat->decrypt_ops, stats_list);
> > > +        stats_list = cryptodev_backend_stats_add("sym-encrypt-bytes",
> > > +                         &sym_stat->encrypt_bytes, stats_list);
> > > +        stats_list = cryptodev_backend_stats_add("sym-decrypt-bytes",
> > > +                         &sym_stat->decrypt_bytes, stats_list);
> > > +    }
> > > +
> > > +    asym_stat = backend->asym_stat;
> > > +    if (asym_stat) {
> > > +        stats_list = cryptodev_backend_stats_add("asym-encrypt-ops",
> > > +                         &asym_stat->encrypt_ops, stats_list);
> > > +        stats_list = cryptodev_backend_stats_add("asym-decrypt-ops",
> > > +                         &asym_stat->decrypt_ops, stats_list);
> > > +        stats_list = cryptodev_backend_stats_add("asym-sign-ops",
> > > +                         &asym_stat->sign_ops, stats_list);
> > > +        stats_list = cryptodev_backend_stats_add("asym-verify-ops",
> > > +                         &asym_stat->verify_ops, stats_list);
> > > +        stats_list = cryptodev_backend_stats_add("asym-encrypt-bytes",
> > > +                         &asym_stat->encrypt_bytes, stats_list);
> > > +        stats_list = cryptodev_backend_stats_add("asym-decrypt-bytes",
> > > +                         &asym_stat->decrypt_bytes, stats_list);
> > > +        stats_list = cryptodev_backend_stats_add("asym-sign-bytes",
> > > +                         &asym_stat->sign_bytes, stats_list);
> > > +        stats_list = cryptodev_backend_stats_add("asym-verify-bytes",
> > > +                         &asym_stat->verify_bytes, stats_list);
> 
> Perhaps there's benefit in having constants defined for these strings,
> since they become part of our API, and the same strings are repeated
> again further down.....
> 
> > > +    }
> > > +
> > > +    entry = g_new0(StatsResult, 1);
> > > +    entry->provider = STATS_PROVIDER_CRYPTODEV;
> > > +    entry->qom_path = g_strdup(object_get_canonical_path(obj));
> > > +    entry->stats = stats_list;
> > > +    QAPI_LIST_PREPEND(*stats_results, entry);
> > > +
> > > +    return 0;
> > > +}
> 
> ...snip...
> 
> > > +
> > > +static void cryptodev_backend_schemas_cb(StatsSchemaList **result,
> > > +                                         Error **errp)
> > > +{
> > > +    StatsSchemaValueList *stats_list = NULL;
> > > +    const char *sym_stats[] = {"sym-encrypt-ops", "sym-decrypt-ops",
> > > +                               "sym-encrypt-bytes", "sym-decrypt-bytes"};
> > > +    const char *asym_stats[] = {"asym-encrypt-ops", "asym-decrypt-ops",
> > > +                                "asym-sign-ops", "asym-verify-ops",
> > > +                                "asym-encrypt-bytes", "asym-decrypt-bytes",
> > > +                                "asym-sign-bytes", "asym-verify-bytes"};
> 
> ... here's the repeated use of the constant strings from earlier
> 
> > > +
> > > +    for (int i = 0; i < ARRAY_SIZE(sym_stats); i++) {
> > > +        stats_list = cryptodev_backend_schemas_add(sym_stats[i], stats_list);
> > > +    }
> > > +
> > > +    for (int i = 0; i < ARRAY_SIZE(asym_stats); i++) {
> > > +        stats_list = cryptodev_backend_schemas_add(asym_stats[i], stats_list);
> > > +    }
> > > +
> > > +    add_stats_schema(result, STATS_PROVIDER_CRYPTODEV, STATS_TARGET_CRYPTODEV,
> > > +                     stats_list);
> > > +}
> 
> With regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [PATCH v4 11/12] cryptodev: Support query-stats QMP command
Posted by Daniel P. Berrangé 2 years, 11 months ago 
On Tue, Feb 28, 2023 at 09:13:30AM -0500, Michael S. Tsirkin wrote:
> On Tue, Feb 28, 2023 at 01:17:52PM +0000, Daniel P. Berrangé wrote:
> > On Tue, Feb 28, 2023 at 07:56:14AM -0500, Michael S. Tsirkin wrote:
> > > On Sun, Jan 29, 2023 at 10:57:46AM +0800, zhenwei pi wrote:
> > > > Now we can use "query-stats" QMP command to query statistics of
> > > > crypto devices. (Originally this was designed to show statistics
> > > > by '{"execute": "query-cryptodev"}'. Daniel Berrangé suggested that
> > > > querying configuration info by "query-cryptodev", and querying
> > > > runtime performance info by "query-stats". This makes sense!)
> > > > 
> > > > Example:
> > > > ~# virsh qemu-monitor-command vm '{"execute": "query-stats", \
> > > >    "arguments": {"target": "cryptodev"} }' | jq
> > > > {
> > > >   "return": [
> > > >     {
> > > >       "provider": "cryptodev",
> > > >       "stats": [
> > > >         {
> > > >           "name": "asym-verify-bytes",
> > > >           "value": 7680
> > > >         },
> > > >         ...
> > > >         {
> > > >           "name": "asym-decrypt-ops",
> > > >           "value": 32
> > > >         },
> > > >         {
> > > >           "name": "asym-encrypt-ops",
> > > >           "value": 48
> > > >         }
> > > >       ],
> > > >       "qom-path": "/objects/cryptodev0" # support asym only
> > > >     },
> > > >     {
> > > >       "provider": "cryptodev",
> > > >       "stats": [
> > > >         {
> > > >           "name": "asym-verify-bytes",
> > > >           "value": 0
> > > >         },
> > > >         ...
> > > >         {
> > > >           "name": "sym-decrypt-bytes",
> > > >           "value": 5376
> > > >         },
> > > >         ...
> > > >       ],
> > > >       "qom-path": "/objects/cryptodev1" # support asym/sym
> > > >     }
> > > >   ],
> > > >   "id": "libvirt-422"
> > > > }
> > > > 
> > > > Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> > > > Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> > > 
> > > I assume since this has been out a long time and no
> > > comments by maintainers it's ok from QAPI POV.
> > 
> > I'm not the QAPI maintainer, but I think this worked out
> > pretty nicely compared to the previous versions of the
> > series which didn't use query-stats.. just a minor point
> > below.
> 
> Hmm applied already ... is this ok to fix with patch on top
> or do I have to revert?

Not a big deal. It'd be fine as a followup improvement if
zhenwei wants to submit a later cleanup.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [PATCH v4 11/12] cryptodev: Support query-stats QMP command
Posted by Michael S. Tsirkin 2 years, 11 months ago 
On Tue, Feb 28, 2023 at 02:21:17PM +0000, Daniel P. Berrangé wrote:
> On Tue, Feb 28, 2023 at 09:13:30AM -0500, Michael S. Tsirkin wrote:
> > On Tue, Feb 28, 2023 at 01:17:52PM +0000, Daniel P. Berrangé wrote:
> > > On Tue, Feb 28, 2023 at 07:56:14AM -0500, Michael S. Tsirkin wrote:
> > > > On Sun, Jan 29, 2023 at 10:57:46AM +0800, zhenwei pi wrote:
> > > > > Now we can use "query-stats" QMP command to query statistics of
> > > > > crypto devices. (Originally this was designed to show statistics
> > > > > by '{"execute": "query-cryptodev"}'. Daniel Berrangé suggested that
> > > > > querying configuration info by "query-cryptodev", and querying
> > > > > runtime performance info by "query-stats". This makes sense!)
> > > > > 
> > > > > Example:
> > > > > ~# virsh qemu-monitor-command vm '{"execute": "query-stats", \
> > > > >    "arguments": {"target": "cryptodev"} }' | jq
> > > > > {
> > > > >   "return": [
> > > > >     {
> > > > >       "provider": "cryptodev",
> > > > >       "stats": [
> > > > >         {
> > > > >           "name": "asym-verify-bytes",
> > > > >           "value": 7680
> > > > >         },
> > > > >         ...
> > > > >         {
> > > > >           "name": "asym-decrypt-ops",
> > > > >           "value": 32
> > > > >         },
> > > > >         {
> > > > >           "name": "asym-encrypt-ops",
> > > > >           "value": 48
> > > > >         }
> > > > >       ],
> > > > >       "qom-path": "/objects/cryptodev0" # support asym only
> > > > >     },
> > > > >     {
> > > > >       "provider": "cryptodev",
> > > > >       "stats": [
> > > > >         {
> > > > >           "name": "asym-verify-bytes",
> > > > >           "value": 0
> > > > >         },
> > > > >         ...
> > > > >         {
> > > > >           "name": "sym-decrypt-bytes",
> > > > >           "value": 5376
> > > > >         },
> > > > >         ...
> > > > >       ],
> > > > >       "qom-path": "/objects/cryptodev1" # support asym/sym
> > > > >     }
> > > > >   ],
> > > > >   "id": "libvirt-422"
> > > > > }
> > > > > 
> > > > > Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> > > > > Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> > > > 
> > > > I assume since this has been out a long time and no
> > > > comments by maintainers it's ok from QAPI POV.
> > > 
> > > I'm not the QAPI maintainer, but I think this worked out
> > > pretty nicely compared to the previous versions of the
> > > series which didn't use query-stats.. just a minor point
> > > below.
> > 
> > Hmm applied already ... is this ok to fix with patch on top
> > or do I have to revert?
> 
> Not a big deal. It'd be fine as a followup improvement if
> zhenwei wants to submit a later cleanup.
> 
> With regards,
> Daniel

zhenwei can you confirm pls?

> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: Re: [PATCH v4 11/12] cryptodev: Support query-stats QMP command
Posted by zhenwei pi 2 years, 11 months ago 

On 2/28/23 22:58, Michael S. Tsirkin wrote:
> On Tue, Feb 28, 2023 at 02:21:17PM +0000, Daniel P. Berrangé wrote:
>> On Tue, Feb 28, 2023 at 09:13:30AM -0500, Michael S. Tsirkin wrote:
>>> On Tue, Feb 28, 2023 at 01:17:52PM +0000, Daniel P. Berrangé wrote:
>>>> On Tue, Feb 28, 2023 at 07:56:14AM -0500, Michael S. Tsirkin wrote:
>>>>> On Sun, Jan 29, 2023 at 10:57:46AM +0800, zhenwei pi wrote:
>>>>>> Now we can use "query-stats" QMP command to query statistics of
>>>>>> crypto devices. (Originally this was designed to show statistics
>>>>>> by '{"execute": "query-cryptodev"}'. Daniel Berrangé suggested that
>>>>>> querying configuration info by "query-cryptodev", and querying
>>>>>> runtime performance info by "query-stats". This makes sense!)
>>>>>>
>>>>>> Example:
>>>>>> ~# virsh qemu-monitor-command vm '{"execute": "query-stats", \
>>>>>>     "arguments": {"target": "cryptodev"} }' | jq
>>>>>> {
>>>>>>    "return": [
>>>>>>      {
>>>>>>        "provider": "cryptodev",
>>>>>>        "stats": [
>>>>>>          {
>>>>>>            "name": "asym-verify-bytes",
>>>>>>            "value": 7680
>>>>>>          },
>>>>>>          ...
>>>>>>          {
>>>>>>            "name": "asym-decrypt-ops",
>>>>>>            "value": 32
>>>>>>          },
>>>>>>          {
>>>>>>            "name": "asym-encrypt-ops",
>>>>>>            "value": 48
>>>>>>          }
>>>>>>        ],
>>>>>>        "qom-path": "/objects/cryptodev0" # support asym only
>>>>>>      },
>>>>>>      {
>>>>>>        "provider": "cryptodev",
>>>>>>        "stats": [
>>>>>>          {
>>>>>>            "name": "asym-verify-bytes",
>>>>>>            "value": 0
>>>>>>          },
>>>>>>          ...
>>>>>>          {
>>>>>>            "name": "sym-decrypt-bytes",
>>>>>>            "value": 5376
>>>>>>          },
>>>>>>          ...
>>>>>>        ],
>>>>>>        "qom-path": "/objects/cryptodev1" # support asym/sym
>>>>>>      }
>>>>>>    ],
>>>>>>    "id": "libvirt-422"
>>>>>> }
>>>>>>
>>>>>> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
>>>>>> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
>>>>>
>>>>> I assume since this has been out a long time and no
>>>>> comments by maintainers it's ok from QAPI POV.
>>>>
>>>> I'm not the QAPI maintainer, but I think this worked out
>>>> pretty nicely compared to the previous versions of the
>>>> series which didn't use query-stats.. just a minor point
>>>> below.
>>>
>>> Hmm applied already ... is this ok to fix with patch on top
>>> or do I have to revert?
>>
>> Not a big deal. It'd be fine as a followup improvement if
>> zhenwei wants to submit a later cleanup.
>>
>> With regards,
>> Daniel
> 
> zhenwei can you confirm pls?
> 

Hi,
I noticed that Markus has already separated HMP commands from monitor/* 
into subsystems, so I need rework this part of this series.
I sent a new version which includes: 1, HMP part; 2, suggested by Daniel

>> -- 
>> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
>> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
>> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> 

-- 
zhenwei pi
Re: [PATCH v4 11/12] cryptodev: Support query-stats QMP command
Posted by Markus Armbruster 2 years, 11 months ago 
Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Feb 28, 2023 at 07:56:14AM -0500, Michael S. Tsirkin wrote:
>> I assume since this has been out a long time and no
>> comments by maintainers it's ok from QAPI POV.
>
> I'm not the QAPI maintainer, but I think this worked out
> pretty nicely compared to the previous versions of the
> series which didn't use query-stats.. just a minor point
> below.

I'm sorry, but I can't keep up with the amount of patches touching the
QAPI schema.  Happy to rely on Daniel's judgement!

[...]

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.