Re: [PATCH v2 1/3] qmp: Support for querying stats
On Wed, Jan 19, 2022 at 09:43:23AM +0100, Paolo Bonzini wrote: > On 1/18/22 15:47, Igor Mammedov wrote: > > and what exactly this CPU ID is, > > may QOM path pointing to VCPU instance would be better? > > For x86 it would be the APIC ID but yes, having a QOM path is more future > proof. Thanks Igor for noting this. Whatever format we use to describe a CPU in the results, should be the same as the format uses in the input parameters. I had suggested using a bitmap of CPU IDs, but if we're going to use QOM paths for results, we must use QOM paths to select CPUs too. 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 v2 1/3] qmp: Support for querying stats
On 1/18/22 15:47, Igor Mammedov wrote: and what exactly this CPU ID is, may QOM path pointing to VCPU instance would be better? For x86 it would be the APIC ID but yes, having a QOM path is more future proof. Thanks Igor for noting this. Paolo
Re: [PATCH v2 1/3] qmp: Support for querying stats
On 1/18/22 14:59, Mark Kanda wrote:
OK. Thank you Daniel and Paolo. I'll implement these changes for v3.
To clarify, the command should be
{ 'command': 'query-stats',
'data': 'StatsFilter',
'returns': 'StatsResults' }
so the "target" would be mandatory and there's one level less of nesting
in the arguments.
Paolo
Re: [PATCH v2 1/3] qmp: Support for querying stats
On Tue, 18 Jan 2022 12:52:10 +
Daniel P. Berrangé wrote:
> On Tue, Jan 18, 2022 at 01:26:32PM +0100, Paolo Bonzini wrote:
> > On 1/17/22 16:17, Mark Kanda wrote:
> > > >
> > > > I agree except that I think this and StatsResults should be unions,
> > > > even if it means running multiple query-stats commands.
> > >
> > > IIUC, making StatsResults a union implies the filter is a required
> > > argument (currently it is optional - omitting it dumps all VM and VCPU
> > > stats). Just to confirm - we want the filter to be required?
> >
> > Yeah, I think at least the "kind" (vcpu, vm, perhaps in the future block or
> > net) should be mandatory. If the caller doesn't know of a "kind", chances
> > are it won't be able to understand what object the stats refer to, for
> > example the vcpu "id" here:
> >
> > { 'union': 'StatsResults',
> >'base': { 'target': 'StatsTarget', stats: ['StatsResultsEntry'] },
> >'discriminator': 'target',
> >'data': { 'vcpu': ['id': 'int'] }
> > }
> >
> > (which is another different between Daniel's proposal and mine; his just
> > placed all vcpus into an array with no explicit id, if I understand
> > correctly).
>
> An explicit ID isn't strictly required, since the caller can assume
> the results are ordered on CPU ID, so even if they gave a request
> for a sparse subset of CPUs, the results can be interpreted. None
> the less having a vCPU id included is more friendly, so worth
> having.
and what exactly this CPU ID is,
may QOM path pointing to VCPU instance would be better?
>
> Regards,
> Daniel
Re: [PATCH v2 1/3] qmp: Support for querying stats
On 1/18/2022 6:52 AM, Daniel P. Berrangé wrote:
On Tue, Jan 18, 2022 at 01:26:32PM +0100, Paolo Bonzini wrote:
On 1/17/22 16:17, Mark Kanda wrote:
I agree except that I think this and StatsResults should be unions,
even if it means running multiple query-stats commands.
IIUC, making StatsResults a union implies the filter is a required
argument (currently it is optional - omitting it dumps all VM and VCPU
stats). Just to confirm - we want the filter to be required?
Yeah, I think at least the "kind" (vcpu, vm, perhaps in the future block or
net) should be mandatory. If the caller doesn't know of a "kind", chances
are it won't be able to understand what object the stats refer to, for
example the vcpu "id" here:
{ 'union': 'StatsResults',
'base': { 'target': 'StatsTarget', stats: ['StatsResultsEntry'] },
'discriminator': 'target',
'data': { 'vcpu': ['id': 'int'] }
}
(which is another different between Daniel's proposal and mine; his just
placed all vcpus into an array with no explicit id, if I understand
correctly).
An explicit ID isn't strictly required, since the caller can assume
the results are ordered on CPU ID, so even if they gave a request
for a sparse subset of CPUs, the results can be interpreted. None
the less having a vCPU id included is more friendly, so worth
having.
Regards,
Daniel
OK. Thank you Daniel and Paolo. I'll implement these changes for v3.
Best regards,
-Mark
Re: [PATCH v2 1/3] qmp: Support for querying stats
On Tue, Jan 18, 2022 at 01:26:32PM +0100, Paolo Bonzini wrote:
> On 1/17/22 16:17, Mark Kanda wrote:
> > >
> > > I agree except that I think this and StatsResults should be unions,
> > > even if it means running multiple query-stats commands.
> >
> > IIUC, making StatsResults a union implies the filter is a required
> > argument (currently it is optional - omitting it dumps all VM and VCPU
> > stats). Just to confirm - we want the filter to be required?
>
> Yeah, I think at least the "kind" (vcpu, vm, perhaps in the future block or
> net) should be mandatory. If the caller doesn't know of a "kind", chances
> are it won't be able to understand what object the stats refer to, for
> example the vcpu "id" here:
>
> { 'union': 'StatsResults',
>'base': { 'target': 'StatsTarget', stats: ['StatsResultsEntry'] },
>'discriminator': 'target',
>'data': { 'vcpu': ['id': 'int'] }
> }
>
> (which is another different between Daniel's proposal and mine; his just
> placed all vcpus into an array with no explicit id, if I understand
> correctly).
An explicit ID isn't strictly required, since the caller can assume
the results are ordered on CPU ID, so even if they gave a request
for a sparse subset of CPUs, the results can be interpreted. None
the less having a vCPU id included is more friendly, so worth
having.
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 v2 1/3] qmp: Support for querying stats
On 1/17/22 16:17, Mark Kanda wrote:
I agree except that I think this and StatsResults should be unions,
even if it means running multiple query-stats commands.
IIUC, making StatsResults a union implies the filter is a required
argument (currently it is optional - omitting it dumps all VM and VCPU
stats). Just to confirm - we want the filter to be required?
Yeah, I think at least the "kind" (vcpu, vm, perhaps in the future block
or net) should be mandatory. If the caller doesn't know of a "kind",
chances are it won't be able to understand what object the stats refer
to, for example the vcpu "id" here:
{ 'union': 'StatsResults',
'base': { 'target': 'StatsTarget', stats: ['StatsResultsEntry'] },
'discriminator': 'target',
'data': { 'vcpu': ['id': 'int'] }
}
(which is another different between Daniel's proposal and mine; his just
placed all vcpus into an array with no explicit id, if I understand
correctly).
Paolo
Re: [PATCH v2 1/3] qmp: Support for querying stats
On 1/17/2022 6:05 AM, Paolo Bonzini wrote:
On 12/7/21 19:42, Daniel P. Berrangé wrote:
Now onto the values being reported. AFAICT from the kernel
docs, for all the types of data it currently reports
(cumulative, instant, peak, none), there is only ever going
to be a single value. I assume the ability to report multiple
values is future proofing for a later requirement.
Yes, in fact histogram values have since been added.
Second, for a given named statistic, AFAICT, the data type,
unit, base and exponent are all fixed. I don't see a reason
for us to be reporting that information every time we call
'query-stats'. Just report the name + value(s). Apps that
want a specific named stat won't care about the dimensions,
because they'll already know what the value means.
I agree on this.
The 'name' and 'type' are used for filtering I presume. Only allowing
a single value for each feels pretty inflexible. I'd say we want to
allow mutliple requests at a time for efficiency.
Bearing in mind my other suggestions above, I'd think we should have
something more like
{ 'enum': 'StatsProvider',
'data': ["kvm", "qemu", "tcg", ],
}
{ 'struct': 'StatsRequest',
'data': {
'provider': 'StatsProvider',
// If omitted, report everything for this provider
'*fields': [ 'str' ]
I think provider should be optional as well. See below.
}
}
{ 'struct': 'StatsVCPURequest',
'base': 'StatsRequest',
'data': {
// To request subset of vCPUs e.g.
// "cpu_set": "0-3"
// Could use ['int'] instead if desired
'*cpu_set': str,
Yes, ['int'] is preferrable.
},
}
{ 'struct': 'StatsFilter',
'data': {
// If omitted means don't report that group of data
'*vcpu': 'StatsVCPURequest',
'*vm': 'StatsRequest',
},
}
I agree except that I think this and StatsResults should be unions, even if it
means running multiple query-stats commands.
IIUC, making StatsResults a union implies the filter is a required argument
(currently it is optional - omitting it dumps all VM and VCPU stats). Just to
confirm - we want the filter to be required?
Thanks/regards,
-Mark
There should also be an enum ['vcpu', 'vm'] that acts as the discriminator for
both StatsFilter and StatsResults:
{ 'enum': 'StatsTarget',
'data': [ 'vcpu', 'vm' ] }
{ 'union': 'StatsFilter',
'base': { 'target': 'StatsTarget', '*providers': ['StatsProvider'] },
'discriminator': 'target',
'data': { 'vcpu': ['*cpu-set': ['int']] }
}
{ 'union': 'StatsResults',
'base': { 'target': 'StatsTarget', stats: ['StatsResultsEntry'] },
'discriminator': 'target',
'data': { 'vcpu': ['id': 'int'] }
}
Alternatively, the providers should simply be keys in the dictionary
Paolo
{ 'alternate': 'StatsValue',
'data': { 'scalar': 'int64',
'list': [ 'int64 ' ] }
}
{ 'struct': 'StatsResultsEntry',
'data': {
'provider': 'StatsProvider',
'stats': [ 'StatsValue' ]
}
}
{ 'struct': 'StatsResults':
'data': {
'*vcpu': [ [ 'StatsResultsEntry' ] ],
'*vm': [ 'StatsResultsEntry' ]
}
}
{ 'command': 'query-stats',
'data': { 'filter': '*StatsFilter' },
'returns': 'StatsResults' }
Re: [PATCH v2 1/3] qmp: Support for querying stats
On 12/7/21 19:42, Daniel P. Berrangé wrote:
Now onto the values being reported. AFAICT from the kernel
docs, for all the types of data it currently reports
(cumulative, instant, peak, none), there is only ever going
to be a single value. I assume the ability to report multiple
values is future proofing for a later requirement.
Yes, in fact histogram values have since been added.
Second, for a given named statistic, AFAICT, the data type,
unit, base and exponent are all fixed. I don't see a reason
for us to be reporting that information every time we call
'query-stats'. Just report the name + value(s). Apps that
want a specific named stat won't care about the dimensions,
because they'll already know what the value means.
I agree on this.
The 'name' and 'type' are used for filtering I presume. Only allowing
a single value for each feels pretty inflexible. I'd say we want to
allow mutliple requests at a time for efficiency.
Bearing in mind my other suggestions above, I'd think we should have
something more like
{ 'enum': 'StatsProvider',
'data': ["kvm", "qemu", "tcg", ],
}
{ 'struct': 'StatsRequest',
'data': {
'provider': 'StatsProvider',
// If omitted, report everything for this provider
'*fields': [ 'str' ]
I think provider should be optional as well. See below.
}
}
{ 'struct': 'StatsVCPURequest',
'base': 'StatsRequest',
'data': {
// To request subset of vCPUs e.g.
// "cpu_set": "0-3"
// Could use ['int'] instead if desired
'*cpu_set': str,
Yes, ['int'] is preferrable.
},
}
{ 'struct': 'StatsFilter',
'data': {
// If omitted means don't report that group of data
'*vcpu': 'StatsVCPURequest',
'*vm': 'StatsRequest',
},
}
I agree except that I think this and StatsResults should be unions, even
if it means running multiple query-stats commands. There should also be
an enum ['vcpu', 'vm'] that acts as the discriminator for both
StatsFilter and StatsResults:
{ 'enum': 'StatsTarget',
'data': [ 'vcpu', 'vm' ] }
{ 'union': 'StatsFilter',
'base': { 'target': 'StatsTarget', '*providers': ['StatsProvider'] },
'discriminator': 'target',
'data': { 'vcpu': ['*cpu-set': ['int']] }
}
{ 'union': 'StatsResults',
'base': { 'target': 'StatsTarget', stats: ['StatsResultsEntry'] },
'discriminator': 'target',
'data': { 'vcpu': ['id': 'int'] }
}
Alternatively, the providers should simply be keys in the dictionary
Paolo
{ 'alternate': 'StatsValue',
'data': { 'scalar': 'int64',
'list': [ 'int64 ' ] }
}
{ 'struct': 'StatsResultsEntry',
'data': {
'provider': 'StatsProvider',
'stats': [ 'StatsValue' ]
}
}
{ 'struct': 'StatsResults':
'data': {
'*vcpu': [ [ 'StatsResultsEntry' ] ],
'*vm': [ 'StatsResultsEntry' ]
}
}
{ 'command': 'query-stats',
'data': { 'filter': '*StatsFilter' },
'returns': 'StatsResults' }
Re: [PATCH v2 1/3] qmp: Support for querying stats
Copying in Markus as QAPI maintainer, since I feel this proposed
design is a little oddball from typical QAPI design approach
On Fri, Nov 19, 2021 at 01:51:51PM -0600, Mark Kanda wrote:
> Introduce qmp support for querying stats. Provide a framework for
> adding new stats and support for the following commands:
>
> - query-stats
> Returns a list of all stats, with options for specifying a stat
> name and schema type. A schema type is the set of stats associated
> with a given component (e.g. vm or vcpu).
>
> - query-stats-schemas
> Returns a list of stats included in each schema type, with an
> option for specifying the schema name.
>
> - query-stats-instances
> Returns a list of stat instances and their associated schema type.
>
> The framework provides a method to register callbacks for these qmp
> commands.
>
> The first usecase will be for fd-based KVM stats (in an upcoming
> patch).
>
> Examples (with fd-based KVM stats):
>
> { "execute": "query-stats" }
> { "return": [
> { "name": "vcpu_1",
> "type": "kvm-vcpu",
> "stats": [
> { "name": "guest_mode",
> "unit": "none",
> "base": 10,
> "val": [ 0 ],
> "exponent": 0,
> "type": "instant" },
> { "name": "directed_yield_successful",
> "unit": "none",
> "base": 10,
> "val": [ 0 ],
> "exponent": 0,
> "type": "cumulative" },
> ...
> },
> { "name": "vcpu_0",
> "type": "kvm-vcpu",
> "stats": ...
> ...
> },
> { "name": "vm",
> "type": "kvm-vm",
> "stats": [
> { "name": "max_mmu_page_hash_collisions",
> "unit": "none",
> "base": 10,
> "val": [ 0 ],
> "exponent": 0,
> "type": "peak" },
> ...
So this is essentially exposing the low level kernel data structure
'struct kvm_stats_desc' mapped 1-to-1 into QAPI.
There are pros/cons to doing that should be explored to see whether
this actually makes sense for the QMP design.
I understand this design is intended to be fully self-describing
such that we can add arbitrarily more fields without ever
changing QEMU code, and with a simple mapping from the kernel
kvm_stats_desc.
Taking the first level of data returned, we see the natural
structure of the data wrt vCPUs is flattened:
{ "return": [
{ "name": "vcpu_0",
"type": "kvm-vcpu",
"stats": [...], // stats for vcpu 0
},
{ "name": "vcpu_1",
"type": "kvm-vcpu",
"stats": [...], // stats for vcpu 0
},
...other vCPUs...
{ "name": "vm",
"type": "kvm-vm",
"stats": [...], // stats for the VM
},
}
This name+type stuff is all unnecessarily indirect. If we ever
have to add more data unrelated to the kvm stats, we're going
to need QEMU changes no matter what, so this indirect structure
isn't future proofing it.
I'd rather expect us to have named struct fields for each
different provider of data, respecting the natural hierachy.
ie use an array for vCPU data.
I understand this is future proofed to be able to support
non-KVM stats. If we have KVM per-vCPU stat and non-KVM
per-VCPU stats at the same time, I'd expect them all to
appear in the same place. IOW, overall I'd expect to
see grouping more like
{ "return": {
"vcpus": [
[ // stats for vcpu 0
{ "provider": 'kvm',
"stats": [...] },
{ "provider": 'qemu',
"stats"; [...] }
],
[ // stats for vcpu 1
{ "provider": 'kvm',
"stats": [...] },
{ "provider": 'qemu',
"stats"; [...] }
],
...other vCPUs...
]
"vm": [
{ "provider": 'kvm',
"stats": [...] },
{ "provider": 'qemu',
"stats"; [...] } ],
],
}
Now onto the values being reported. AFAICT from the kernel
docs, for all the types of data it currently reports
(cumulative, instant, peak, none), there is only ever going
to be a single value. I assume the ability to report multiple
values is future proofing for a later requirement. This is
fine from a kerenl POV since they're trying to fit this into
a flat struct. QAPI is way more flexible. It can switch
between reporting an scale or array or scalars for the
same field. So if we know the stat will only ever have
1 value, we should be reporting a scalar, not an array
which will only ever have one value.
Second, for a given named statistic, AFAICT, the data type,
unit, base and exponent are all fixed. I don't see a reason
for us to be reporting that information every time we call
'query-stats'. Just report the name + value(s). Apps that
want a specific named stat won't care about the dimensions,
because they'll already know what the value means.
Apps that want to be metadata driven to handle arbitrary
stats, can just call 'query-stats-schemas' to learn about
the dimensions one time.
This will give
[PATCH v2 1/3] qmp: Support for querying stats
Introduce qmp support for querying stats. Provide a framework for
adding new stats and support for the following commands:
- query-stats
Returns a list of all stats, with options for specifying a stat
name and schema type. A schema type is the set of stats associated
with a given component (e.g. vm or vcpu).
- query-stats-schemas
Returns a list of stats included in each schema type, with an
option for specifying the schema name.
- query-stats-instances
Returns a list of stat instances and their associated schema type.
The framework provides a method to register callbacks for these qmp
commands.
The first usecase will be for fd-based KVM stats (in an upcoming
patch).
Examples (with fd-based KVM stats):
{ "execute": "query-stats" }
{ "return": [
{ "name": "vcpu_1",
"type": "kvm-vcpu",
"stats": [
{ "name": "guest_mode",
"unit": "none",
"base": 10,
"val": [ 0 ],
"exponent": 0,
"type": "instant" },
{ "name": "directed_yield_successful",
"unit": "none",
"base": 10,
"val": [ 0 ],
"exponent": 0,
"type": "cumulative" },
...
},
{ "name": "vcpu_0",
"type": "kvm-vcpu",
"stats": ...
...
},
{ "name": "vm",
"type": "kvm-vm",
"stats": [
{ "name": "max_mmu_page_hash_collisions",
"unit": "none",
"base": 10,
"val": [ 0 ],
"exponent": 0,
"type": "peak" },
...
{ "execute": "query-stats-schemas" }
{ "return": [
{ "type": "kvm-vcpu",
"stats": [
{ "name": "guest_mode" },
{ "name": "directed_yield_successful" },
...
},
{ "type": "kvm-vm",
"stats": [
{ "name": "max_mmu_page_hash_collisions" },
{ "name": "max_mmu_rmap_size" },
...
{ "execute": "query-stats-instances" }
{ "return": [
{ "name": "vcpu_1",
"type": "kvm-vcpu" },
{ "name": "vcpu_0",
"type": "kvm-vcpu" },
{ "name": "vm",
"type": "kvm-vm" } ]
}
Signed-off-by: Mark Kanda
---
include/monitor/monitor.h | 27
monitor/qmp-cmds.c| 71 +++
qapi/misc.json| 142 ++
3 files changed, 240 insertions(+)
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 12d395d62d..14d3432ade 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -56,4 +56,31 @@ void monitor_register_hmp(const char *name, bool info,
void monitor_register_hmp_info_hrt(const char *name,
HumanReadableText *(*handler)(Error
**errp));
+/*
+ * Add qmp stats callbacks to the stats_callbacks list.
+ *
+ * @name: name of stats callbacks
+ * @stats_fn: routine to query stats - with options for name and type:
+ *StatsList *(*stats_fn)(StatsList *list_tail, bool has_name,
+ *const char *name, bool has_type, const char *type, Error **errp)
+ *
+ * @schema_fn: routine to query stat schemas - with an option for type:
+ *StatsSchemaList *(*schemas_fn)(StatsSchemaList *list tail, bool has_type,
+ * const char *type, Error **errp)
+ *
+ * @instance_fn: routine to query stat instances:
+ * StatsInstanceList *(*instances_fn)(StatsInstanceList *list_tail,
+ *Error **errp)
+ */
+void add_stats_callbacks(const char *name,
+ StatsList *(*stats_fn)(StatsList *,
+bool, const char *,
+bool, const char *,
+Error **),
+ StatsSchemaList *(*schemas_fn)(StatsSchemaList *,
+bool, const char *,
+Error **),
+ StatsInstanceList *(*instances_fn)(StatsInstanceList
*,
+Error **));
+
#endif /* MONITOR_H */
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 343353e27a..c7bdff1e1c 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -466,3 +466,74 @@ HumanReadableText *qmp_x_query_irq(Error **errp)
return human_readable_text_from_str(buf);
}
+
+typedef struct StatsCallbacks {
+char *name;
+StatsList *(*stats_cb)(StatsList *, bool, const char *, bool,
+ const char *, Error **);
+StatsSchemaList *(*schemas_cb)(StatsSchemaList *, bool, const char *,
+ Error **);
+StatsInstanceList *(*instances_cb)(StatsInstanceList *, Error **);
+QTAILQ_ENTRY(StatsCallbacks) next;
+} StatsCallbacks;
+
+static QTAILQ_HEAD(, StatsCallbacks) stats_callbacks =
+QTAILQ_HEAD_INITIALIZER(stats_callbacks);
+
+void add_stats_callbacks(const char *name,
+ StatsList
