Allow retrieving only a subset of statistics. This can be useful
for example in order to plot a subset of the statistics many times
a second.
KVM publishes ~40 statistics for each vCPU on x86; retrieving and
serializing all of them would be useless
Another use will be in HMP in the following patch; implementing the
filter in the backend is easy enough that it was deemed okay to make
this a public interface.
Example:
{ "execute": "query-stats",
"arguments": {
"target": "vcpu",
"vcpus": [ "/machine/unattached/device[2]",
"/machine/unattached/device[4]" ],
"providers": [
{ "provider": "kvm",
"names": [ "l1d_flush", "exits" ] } } }
{ "return": {
"vcpus": [
{ "path": "/machine/unattached/device[2]"
"providers": [
{ "provider": "kvm",
"stats": [ { "name": "l1d_flush", "value": 41213 },
{ "name": "exits", "value": 74291 } ] } ] },
{ "path": "/machine/unattached/device[4]"
"providers": [
{ "provider": "kvm",
"stats": [ { "name": "l1d_flush", "value": 16132 },
{ "name": "exits", "value": 57922 } ] } ] } ] } }
Extracted from a patch by Mark Kanda.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
accel/kvm/kvm-all.c | 18 +++++++++++-------
include/monitor/stats.h | 4 ++--
monitor/qmp-cmds.c | 10 +++++++---
qapi/stats.json | 4 +++-
4 files changed, 23 insertions(+), 13 deletions(-)
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index b42008ac07..67253c5a5c 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2311,7 +2311,7 @@ bool kvm_dirty_ring_enabled(void)
return kvm_state->kvm_dirty_ring_size ? true : false;
}
-static void query_stats_cb(StatsResultList **result, StatsTarget target,
+static void query_stats_cb(StatsResultList **result, StatsTarget target, strList *names,
strList *targets, Error **errp);
static void query_stats_schemas_cb(StatsSchemaList **result, Error **errp);
@@ -3713,6 +3713,7 @@ typedef struct StatsArgs {
StatsResultList **stats;
StatsSchemaList **schema;
} result;
+ strList *names;
Error **errp;
} StatsArgs;
@@ -3926,7 +3927,7 @@ static StatsDescriptors *find_stats_descriptors(StatsTarget target, int stats_fd
return descriptors;
}
-static void query_stats(StatsResultList **result, StatsTarget target,
+static void query_stats(StatsResultList **result, StatsTarget target, strList *names,
int stats_fd, Error **errp)
{
struct kvm_stats_desc *kvm_stats_desc;
@@ -3969,6 +3970,9 @@ static void query_stats(StatsResultList **result, StatsTarget target,
/* Add entry to the list */
stats = (void *)stats_data + pdesc->offset;
+ if (!str_in_list(pdesc->name, names)) {
+ continue;
+ }
stats_list = add_kvmstat_entry(pdesc, stats, stats_list, errp);
}
@@ -4030,8 +4034,8 @@ static void query_stats_vcpu(CPUState *cpu, run_on_cpu_data data)
error_propagate(kvm_stats_args->errp, local_err);
return;
}
- query_stats(kvm_stats_args->result.stats, STATS_TARGET_VCPU, stats_fd,
- kvm_stats_args->errp);
+ query_stats(kvm_stats_args->result.stats, STATS_TARGET_VCPU,
+ kvm_stats_args->names, stats_fd, kvm_stats_args->errp);
close(stats_fd);
}
@@ -4052,7 +4056,7 @@ static void query_stats_schema_vcpu(CPUState *cpu, run_on_cpu_data data)
}
static void query_stats_cb(StatsResultList **result, StatsTarget target,
- strList *targets, Error **errp)
+ strList *names, strList *targets, Error **errp)
{
KVMState *s = kvm_state;
CPUState *cpu;
@@ -4066,14 +4070,15 @@ static void query_stats_cb(StatsResultList **result, StatsTarget target,
error_setg(errp, "KVM stats: ioctl failed");
return;
}
- query_stats(result, target, stats_fd, errp);
+ query_stats(result, target, names, stats_fd, errp);
close(stats_fd);
break;
}
case STATS_TARGET_VCPU:
{
StatsArgs stats_args;
stats_args.result.stats = result;
+ stats_args.names = names;
stats_args.errp = errp;
CPU_FOREACH(cpu) {
if (!str_in_list(cpu->parent_obj.canonical_path, targets)) {
diff --git a/include/monitor/stats.h b/include/monitor/stats.h
index acfd975df9..b4123044f7 100644
--- a/include/monitor/stats.h
+++ b/include/monitor/stats.h
@@ -11,8 +11,8 @@
#include "qapi/qapi-types-stats.h"
typedef void StatRetrieveFunc(StatsResultList **result, StatsTarget target,
- strList *targets, Error **errp);
-typedef void SchemaRetrieveFunc(StatsSchemaList **result, Error **errp);
+ strList *names, strList *targets, Error **errp);
+typedef void SchemaRetrieveFunc(StatsSchemaList **, Error **);
/*
* Register callbacks for the QMP query-stats command.
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 25962d8bb4..d0fdb17c82 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -468,15 +468,18 @@ static strList *stats_target_filter(StatsFilter *filter)
}
static bool stats_provider_requested(StatsProvider provider,
- StatsFilter *filter)
+ StatsFilter *filter,
+ strList **p_names)
{
StatsRequestList *request;
if (!filter->has_providers) {
+ *p_names = NULL;
return true;
}
for (request = filter->providers; request; request = request->next) {
if (request->value->provider == provider) {
+ *p_names = request->value->has_names ? request->value->names : NULL;
return true;
}
}
@@ -490,8 +493,9 @@ StatsResultList *qmp_query_stats(StatsFilter *filter, Error **errp)
StatsCallbacks *entry;
QTAILQ_FOREACH(entry, &stats_callbacks, next) {
- if (stats_provider_requested(entry->provider, filter)) {
- entry->stats_cb(&stats_results, filter->target, targets, errp);
+ strList *names = NULL;
+ if (stats_provider_requested(entry->provider, filter, &names)) {
+ entry->stats_cb(&stats_results, filter->target, names, targets, errp);
}
}
diff --git a/qapi/stats.json b/qapi/stats.json
index 33ff6ea7a9..234fbcb7ca 100644
--- a/qapi/stats.json
+++ b/qapi/stats.json
@@ -71,11 +71,14 @@
# Indicates a set of statistics that should be returned by query-stats.
#
# @provider: provider for which to return statistics.
+
+# @names: statistics to be returned (all if omitted).
#
# Since: 7.1
##
{ 'struct': 'StatsRequest',
- 'data': { 'provider': 'StatsProvider' } }
+ 'data': { 'provider': 'StatsProvider',
+ '*names': [ 'str' ] } }
##
# @StatsVCPUFilter:
--
2.35.1* Paolo Bonzini (pbonzini@redhat.com) wrote:
> Allow retrieving only a subset of statistics. This can be useful
> for example in order to plot a subset of the statistics many times
> a second.
>
> KVM publishes ~40 statistics for each vCPU on x86; retrieving and
> serializing all of them would be useless
>
> Another use will be in HMP in the following patch; implementing the
> filter in the backend is easy enough that it was deemed okay to make
> this a public interface.
>
> Example:
>
> { "execute": "query-stats",
> "arguments": {
> "target": "vcpu",
> "vcpus": [ "/machine/unattached/device[2]",
> "/machine/unattached/device[4]" ],
> "providers": [
> { "provider": "kvm",
> "names": [ "l1d_flush", "exits" ] } } }
That looks inconsistent to me; I realise that 'names' has to be a child
of providers (since the names are only relevant to a given provider) but
how about making the "target" work similarly:
{ "execute": "query-stats",
"arguments": {
"target": {
"vcpus": [ "/machine/unattached/device[2]",
"/machine/unattached/device[4]" ] },
"providers": [
{ "provider": "kvm",
"names": [ "l1d_flush", "exits" ] } } }
It's not clear to me whether the "target" should also be specific
to a given provider.
Dave
> { "return": {
> "vcpus": [
> { "path": "/machine/unattached/device[2]"
> "providers": [
> { "provider": "kvm",
> "stats": [ { "name": "l1d_flush", "value": 41213 },
> { "name": "exits", "value": 74291 } ] } ] },
> { "path": "/machine/unattached/device[4]"
> "providers": [
> { "provider": "kvm",
> "stats": [ { "name": "l1d_flush", "value": 16132 },
> { "name": "exits", "value": 57922 } ] } ] } ] } }
>
> Extracted from a patch by Mark Kanda.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> accel/kvm/kvm-all.c | 18 +++++++++++-------
> include/monitor/stats.h | 4 ++--
> monitor/qmp-cmds.c | 10 +++++++---
> qapi/stats.json | 4 +++-
> 4 files changed, 23 insertions(+), 13 deletions(-)
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index b42008ac07..67253c5a5c 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2311,7 +2311,7 @@ bool kvm_dirty_ring_enabled(void)
> return kvm_state->kvm_dirty_ring_size ? true : false;
> }
>
> -static void query_stats_cb(StatsResultList **result, StatsTarget target,
> +static void query_stats_cb(StatsResultList **result, StatsTarget target, strList *names,
> strList *targets, Error **errp);
> static void query_stats_schemas_cb(StatsSchemaList **result, Error **errp);
>
> @@ -3713,6 +3713,7 @@ typedef struct StatsArgs {
> StatsResultList **stats;
> StatsSchemaList **schema;
> } result;
> + strList *names;
> Error **errp;
> } StatsArgs;
>
> @@ -3926,7 +3927,7 @@ static StatsDescriptors *find_stats_descriptors(StatsTarget target, int stats_fd
> return descriptors;
> }
>
> -static void query_stats(StatsResultList **result, StatsTarget target,
> +static void query_stats(StatsResultList **result, StatsTarget target, strList *names,
> int stats_fd, Error **errp)
> {
> struct kvm_stats_desc *kvm_stats_desc;
> @@ -3969,6 +3970,9 @@ static void query_stats(StatsResultList **result, StatsTarget target,
>
> /* Add entry to the list */
> stats = (void *)stats_data + pdesc->offset;
> + if (!str_in_list(pdesc->name, names)) {
> + continue;
> + }
> stats_list = add_kvmstat_entry(pdesc, stats, stats_list, errp);
> }
>
> @@ -4030,8 +4034,8 @@ static void query_stats_vcpu(CPUState *cpu, run_on_cpu_data data)
> error_propagate(kvm_stats_args->errp, local_err);
> return;
> }
> - query_stats(kvm_stats_args->result.stats, STATS_TARGET_VCPU, stats_fd,
> - kvm_stats_args->errp);
> + query_stats(kvm_stats_args->result.stats, STATS_TARGET_VCPU,
> + kvm_stats_args->names, stats_fd, kvm_stats_args->errp);
> close(stats_fd);
> }
>
> @@ -4052,7 +4056,7 @@ static void query_stats_schema_vcpu(CPUState *cpu, run_on_cpu_data data)
> }
>
> static void query_stats_cb(StatsResultList **result, StatsTarget target,
> - strList *targets, Error **errp)
> + strList *names, strList *targets, Error **errp)
> {
> KVMState *s = kvm_state;
> CPUState *cpu;
> @@ -4066,14 +4070,15 @@ static void query_stats_cb(StatsResultList **result, StatsTarget target,
> error_setg(errp, "KVM stats: ioctl failed");
> return;
> }
> - query_stats(result, target, stats_fd, errp);
> + query_stats(result, target, names, stats_fd, errp);
> close(stats_fd);
> break;
> }
> case STATS_TARGET_VCPU:
> {
> StatsArgs stats_args;
> stats_args.result.stats = result;
> + stats_args.names = names;
> stats_args.errp = errp;
> CPU_FOREACH(cpu) {
> if (!str_in_list(cpu->parent_obj.canonical_path, targets)) {
> diff --git a/include/monitor/stats.h b/include/monitor/stats.h
> index acfd975df9..b4123044f7 100644
> --- a/include/monitor/stats.h
> +++ b/include/monitor/stats.h
> @@ -11,8 +11,8 @@
> #include "qapi/qapi-types-stats.h"
>
> typedef void StatRetrieveFunc(StatsResultList **result, StatsTarget target,
> - strList *targets, Error **errp);
> -typedef void SchemaRetrieveFunc(StatsSchemaList **result, Error **errp);
> + strList *names, strList *targets, Error **errp);
> +typedef void SchemaRetrieveFunc(StatsSchemaList **, Error **);
>
> /*
> * Register callbacks for the QMP query-stats command.
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 25962d8bb4..d0fdb17c82 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -468,15 +468,18 @@ static strList *stats_target_filter(StatsFilter *filter)
> }
>
> static bool stats_provider_requested(StatsProvider provider,
> - StatsFilter *filter)
> + StatsFilter *filter,
> + strList **p_names)
> {
> StatsRequestList *request;
>
> if (!filter->has_providers) {
> + *p_names = NULL;
> return true;
> }
> for (request = filter->providers; request; request = request->next) {
> if (request->value->provider == provider) {
> + *p_names = request->value->has_names ? request->value->names : NULL;
> return true;
> }
> }
> @@ -490,8 +493,9 @@ StatsResultList *qmp_query_stats(StatsFilter *filter, Error **errp)
> StatsCallbacks *entry;
>
> QTAILQ_FOREACH(entry, &stats_callbacks, next) {
> - if (stats_provider_requested(entry->provider, filter)) {
> - entry->stats_cb(&stats_results, filter->target, targets, errp);
> + strList *names = NULL;
> + if (stats_provider_requested(entry->provider, filter, &names)) {
> + entry->stats_cb(&stats_results, filter->target, names, targets, errp);
> }
> }
>
> diff --git a/qapi/stats.json b/qapi/stats.json
> index 33ff6ea7a9..234fbcb7ca 100644
> --- a/qapi/stats.json
> +++ b/qapi/stats.json
> @@ -71,11 +71,14 @@
> # Indicates a set of statistics that should be returned by query-stats.
> #
> # @provider: provider for which to return statistics.
> +
> +# @names: statistics to be returned (all if omitted).
> #
> # Since: 7.1
> ##
> { 'struct': 'StatsRequest',
> - 'data': { 'provider': 'StatsProvider' } }
> + 'data': { 'provider': 'StatsProvider',
> + '*names': [ 'str' ] } }
>
> ##
> # @StatsVCPUFilter:
> --
> 2.35.1
>
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 4/27/22 14:01, Dr. David Alan Gilbert wrote:
>> "providers": [
>> { "provider": "kvm",
>> "names": [ "l1d_flush", "exits" ] } } }
> That looks inconsistent to me; I realise that 'names' has to be a child
> of providers (since the names are only relevant to a given provider) but
> how about making the "target" work similarly:
>
> { "execute": "query-stats",
> "arguments": {
> "target": {
> "vcpus": [ "/machine/unattached/device[2]",
> "/machine/unattached/device[4]" ] },
This would allow queries for different types of targets in a single
command, but it would be harder for the client to do filtering:
* with something like "target": { "vcpus": [ array ] }, there is no way
for the client to say "I want this one statistic, but for all the vCPUs"
* if target is optional, a client might expect relatively small output
from today's QEMU, yet suddenly get hundreds of KB from targets other
than VMs and vCPUs (e.g. block devices including all backing files in
the chain, NIC backends, etc.).
> "providers": [
> { "provider": "kvm",
> "names": [ "l1d_flush", "exits" ] } } }
>
> It's not clear to me whether the "target" should also be specific
> to a given provider.
No, the target is a QEMU concept, such as a CPU or a device backend. It
is identified by either a QOM path or a unique id.
Paolo
* Paolo Bonzini (pbonzini@redhat.com) wrote:
> On 4/27/22 14:01, Dr. David Alan Gilbert wrote:
> > > "providers": [
> > > { "provider": "kvm",
> > > "names": [ "l1d_flush", "exits" ] } } }
> > That looks inconsistent to me; I realise that 'names' has to be a child
> > of providers (since the names are only relevant to a given provider) but
> > how about making the "target" work similarly:
> >
> > { "execute": "query-stats",
> > "arguments": {
> > "target": {
> > "vcpus": [ "/machine/unattached/device[2]",
> > "/machine/unattached/device[4]" ] },
>
> This would allow queries for different types of targets in a single command,
> but it would be harder for the client to do filtering:
>
> * with something like "target": { "vcpus": [ array ] }, there is no way for
> the client to say "I want this one statistic, but for all the vCPUs"
>
> * if target is optional, a client might expect relatively small output from
> today's QEMU, yet suddenly get hundreds of KB from targets other than VMs
> and vCPUs (e.g. block devices including all backing files in the chain, NIC
> backends, etc.).
If I specify a 'vm' it's not obvious to me whether I'd get NICs and
block devices in the future?
Adding a syntax for 'all' into the vcpus list would fix that?
> > "providers": [
> > { "provider": "kvm",
> > "names": [ "l1d_flush", "exits" ] } } }
> >
> > It's not clear to me whether the "target" should also be specific
> > to a given provider.
>
> No, the target is a QEMU concept, such as a CPU or a device backend. It is
> identified by either a QOM path or a unique id.
But doesn't 'kvm' as a provider only make sense for vcpus and VMs; if
you're imagining block devices and other things as targets it would seem
wrong to have that set of providers separate.
Dave
> Paolo
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 4/27/22 14:34, Dr. David Alan Gilbert wrote:
> If I specify a 'vm' it's not obvious to me whether I'd get NICs and
> block devices in the future?
VM would not get those (it's global statistics), but the size could
balloon if you specify no target at all.
> Adding a syntax for 'all' into the vcpus list would fix that?
I don't like having special syntax. The current QAPI just doesn't
filter what is not in the arguments.
>>> "providers": [
>>> { "provider": "kvm",
>>> "names": [ "l1d_flush", "exits" ] } } }
>>>
>>> It's not clear to me whether the "target" should also be specific
>>> to a given provider.
>>
>> No, the target is a QEMU concept, such as a CPU or a device backend. It is
>> identified by either a QOM path or a unique id.
>
> But doesn't 'kvm' as a provider only make sense for vcpus and VMs; if
> you're imagining block devices and other things as targets it would seem
> wrong to have that set of providers separate.
Yes, those would have different providers. But a single target can
support multiple providers.
Paolo
* Paolo Bonzini (pbonzini@redhat.com) wrote:
> On 4/27/22 14:34, Dr. David Alan Gilbert wrote:
> > If I specify a 'vm' it's not obvious to me whether I'd get NICs and
> > block devices in the future?
>
> VM would not get those (it's global statistics), but the size could balloon
> if you specify no target at all.
>
> > Adding a syntax for 'all' into the vcpus list would fix that?
>
> I don't like having special syntax. The current QAPI just doesn't filter
> what is not in the arguments.
Is there a object that represents the set of all vcpus?
> > > > "providers": [
> > > > { "provider": "kvm",
> > > > "names": [ "l1d_flush", "exits" ] } } }
> > > >
> > > > It's not clear to me whether the "target" should also be specific
> > > > to a given provider.
> > >
> > > No, the target is a QEMU concept, such as a CPU or a device backend. It is
> > > identified by either a QOM path or a unique id.
> >
> > But doesn't 'kvm' as a provider only make sense for vcpus and VMs; if
> > you're imagining block devices and other things as targets it would seem
> > wrong to have that set of providers separate.
>
> Yes, those would have different providers. But a single target can support
> multiple providers.
Is that just for different implementations - kvm/hcf/tcg etc or do you
envisage multiple providers on an object in a running VM?
Dave
> Paolo
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 4/27/22 17:16, Dr. David Alan Gilbert wrote: > * Paolo Bonzini (pbonzini@redhat.com) wrote: >> On 4/27/22 14:34, Dr. David Alan Gilbert wrote: >>> If I specify a 'vm' it's not obvious to me whether I'd get NICs and >>> block devices in the future? >> >> VM would not get those (it's global statistics), but the size could balloon >> if you specify no target at all. >> >>> Adding a syntax for 'all' into the vcpus list would fix that? >> >> I don't like having special syntax. The current QAPI just doesn't filter >> what is not in the arguments. > > Is there a object that represents the set of all vcpus? No. >> Yes, those would have different providers. But a single target can support >> multiple providers. > > Is that just for different implementations - kvm/hcf/tcg etc or do you > envisage multiple providers on an object in a running VM? I think multiple providers are possible for a single object, for example a device could expose both PCI (how many MSIs, etc.) and SCSI (how many commands sent/succeeded/failed) statistics. Paolo
* Paolo Bonzini (pbonzini@redhat.com) wrote: > On 4/27/22 17:16, Dr. David Alan Gilbert wrote: > > * Paolo Bonzini (pbonzini@redhat.com) wrote: > > > On 4/27/22 14:34, Dr. David Alan Gilbert wrote: > > > > If I specify a 'vm' it's not obvious to me whether I'd get NICs and > > > > block devices in the future? > > > > > > VM would not get those (it's global statistics), but the size could balloon > > > if you specify no target at all. > > > > > > > Adding a syntax for 'all' into the vcpus list would fix that? > > > > > > I don't like having special syntax. The current QAPI just doesn't filter > > > what is not in the arguments. > > > > Is there a object that represents the set of all vcpus? > > No. If it was easy to create one then you could remove all the special casing of vCPUs/VM target? (It feels really like you should call a 'stats' method on the target) > > > Yes, those would have different providers. But a single target can support > > > multiple providers. > > > > Is that just for different implementations - kvm/hcf/tcg etc or do you > > envisage multiple providers on an object in a running VM? > > I think multiple providers are possible for a single object, for example a > device could expose both PCI (how many MSIs, etc.) and SCSI (how many > commands sent/succeeded/failed) statistics. Yeh fair enough. Dave > Paolo > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 4/27/22 19:16, Dr. David Alan Gilbert wrote: > * Paolo Bonzini (pbonzini@redhat.com) wrote: >> On 4/27/22 17:16, Dr. David Alan Gilbert wrote: >>> * Paolo Bonzini (pbonzini@redhat.com) wrote: >>>> On 4/27/22 14:34, Dr. David Alan Gilbert wrote: >>>>> If I specify a 'vm' it's not obvious to me whether I'd get NICs and >>>>> block devices in the future? >>>> >>>> VM would not get those (it's global statistics), but the size could balloon >>>> if you specify no target at all. >>>> >>>>> Adding a syntax for 'all' into the vcpus list would fix that? >>>> >>>> I don't like having special syntax. The current QAPI just doesn't filter >>>> what is not in the arguments. >>> >>> Is there a object that represents the set of all vcpus? >> >> No. > > If it was easy to create one then you could remove all the special > casing of vCPUs/VM target? > (It feels really like you should call a 'stats' method on the target) There are two possibilities for that: 1) add statistics to an object like /machine, that would return the sum/max of the statistics. Advantage: you have an easy way to summarize stats without reading many KBs of data. Disadvantage: it doesn't do what you're asking. :) But it may be an interesting addition. 2) make query-stats return the statistics for all objects below a given QOM path, and then the caller would pass / or /machine as the target. Both make sense, but neither extends easily to the case where you don't have a QOM path, as is the case for block or network devices. Unfortunately, both of them are prime candidates for extending the subsystem, so they can't be dismissed easily, and that is why implemented neither of them. If block or network devices were QOM, it would be possible and easy to have a single "qom-path" argument to replace both "target" and the sub-records like StatsVCPUFilter. Paolo
© 2016 - 2026 Red Hat, Inc.