| Message ID | 20230129025747.682282-12-pizhenwei@bytedance.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Refactor cryptodev | expand |
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
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
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! [...]
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 :|
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
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 :|
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 :| >
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:
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(-)