From: Paolo Bonzini <pbonzini@redhat.com> Date: 2020-05-08 09:44:19
[Answering for Emanuele because he's not available until Monday]
On 07/05/20 19:45, Jonathan Adams wrote:
This is good work. As David Rientjes mentioned, I'm currently investigating
a similar project, based on a google-internal debugfs-based FS we call
"metricfs". It's
designed in a slightly different fashion than statsfs here is, and the
statistics exported are
mostly fed into our OpenTelemetry-like system. We're motivated by
wanting an upstreamed solution, so that we can upstream the metrics we
create that are of general interest, and lower the overall rebasing
burden for our tree.
Cool. We included a public reading API exactly so that there could be
other "frontends". I was mostly thinking of BPF as an in-tree user, but
your metricfs could definitely use the reading API.
- the 8/16/32/64 signed/unsigned integers seems like a wart, and the
built-in support to grab any offset from a structure doesn't seem like
much of an advantage. A simpler interface would be to just support an> "integer" (possibly signed/unsigned) type, which is always 64-bit, and
allow the caller to provide a function pointer to retrieve the value,
with one or two void *s cbargs. Then the framework could provide an
offset-based callback (or callbacks) similar to the existing
functionality, and a similar one for per-CPU based statistics. A
second "clear" callback could be optionally provided to allow for
statistics to be cleared, as in your current proposal.
Ok, so basically splitting get_simple_value into many separate
callbacks. The callbacks would be in a struct like
struct stats_fs_type {
uint64_t (*get)(struct stats_fs_value *, void *);
void (*clear)(struct stats_fs_value *, void *);
bool signed;
}
static uint64_t stats_fs_get_u8(struct stats_fs_value *val, void *base)
{
return *((uint8_t *)(base + (uintptr_t)val->arg);
}
static void stats_fs_clear_u8(struct stats_fs_value *val, void *base)
{
*((uint8_t *)(base + (uintptr_t)val->arg) = 0;
}
struct stats_fs_type stats_fs_type_u8 = {
stats_fs_get_u8,
stats_fs_clear_u8,
false
};
and custom types can be defined using "&(struct stats_fs_type) {...}".
- Beyond the statistic's type, one *very* useful piece of metadata
for telemetry tools is knowing whether a given statistic is
"cumulative" (an unsigned counter which is only ever increased), as
opposed to a floating value (like "amount of memory used").
Good idea. Also, clearing does not make sense for a floating value, so
we can use cumulative/floating to get a default for the mode: KVM
statistics for example are mostly cumulative and mode 644, except a few
that are floating and those are all mode 444. Therefore it makes sense
to add cumulative/floating even before outputting it as metadata.
I'm more
concerned with getting the statistics model and capabilities right
from the beginning, because those are harder to adjust later.
Agreed.
1. Each metricfs metric can have one or two string or integer "keys".
If these exist, they expand the metric from a single value into a
multi-dimensional table. For example, we use this to report a hash
table we keep of functions calling "WARN()", in a 'warnings'
statistic:
% cat .../warnings/values
x86_pmu_stop 1
%
Indicates that the x86_pmu_stop() function has had a WARN() fire once
since the system was booted. If multiple functions have fired
WARN()s, they are listed in this table with their own counts. [1] We
also use these to report per-CPU counters on a CPU-by-CPU basis:
% cat .../irq_x86/NMI/values
0 42
1 18
... one line per cpu
% cat .../rx_bytes/values
lo 501360681
eth0 1457631256
These seem like two different things.
The percpu and per-interface values are best represented as subordinate
sources, one per CPU and one per interface. For interfaces I would just
use a separate directory, but it doesn't really make sense for CPUs. So
if we can cater for it in the model, it's better. For example:
- add a new argument to statsfs_create_source and statsfs_create_values
that makes it not create directories and files respectively.
- add a new "aggregate function" STATS_FS_LIST that directs the parent
to build a table of all the simple values below it
We can also add a helper statsfs_add_values_percpu that creates a new
source for each CPU, I think.
The warnings one instead is a real hash table. It should be possible to
implement it as some kind of customized aggregation, that is implemented
in the client instead of coming from subordinate sources. The
presentation can then just use STATS_FS_LIST. I don't see anything in
the design that is a blocker.
2. We also export some metadata about each statistic. For example,
the metadata for the NMI counter above looks like:
% cat .../NMI/annotations
DESCRIPTION Non-maskable\ interrupts
CUMULATIVE
% cat .../NMI/fields
cpu value
int int
%
Good idea. I would prefer per-directory dot-named files for this. For
example a hypothetical statsfs version of /proc/interrupts could be like
this:
$ cat /sys/kernel/stats/interrupts/.schema
0 // Name
CUMULATIVE // Flags
int:int // Type(s)
IR-IO-APIC 2-edge timer // Description
...
LOC
CUMULATIVE
int:int
Local timer interrupts
...
$ cat /sys/kernel/stats/interrupts/LOC
0 4286815
1 4151572
2 4199361
3 4229248
3. We have a (very few) statistics where the value itself is a string,
usually for device statuses.
Maybe in addition to CUMULATIVE and FLOATING we can have ENUM
properties, and a table to convert those enums to strings. Aggregation
could also be used to make a histogram out of enums in subordinate
sources, e.g.
$ cat /sys/kernel/stats/kvm/637-1/vcpu_state
running 12
uninitialized 0
halted 4
So in general I'd say the sources/values model holds up. We certainly
want to:
- switch immediately to callbacks instead of the type constants (so that
core statsfs code only does signed/unsigned)
- add a field to distinguish cumulative and floating properties (and use
it to determine the default file mode)
- add a new argument to statsfs_create_source and statsfs_create_values
that makes it not create directories and files respectively
- add a new API to look for a statsfs_value recursively in all the
subordinate sources, and pass the source/value pair to a callback
function; and reimplement recursive aggregation and clear in terms of
this function.
For our use cases, we generally don't both output a statistic and it's
aggregation from the kernel; either we sum up things in the kernel
(e.g. over a bunch of per-cpu or per-memcg counters) and only have the
result statistic, or we expect user-space to sum up the data if it's
interested. The tabular form makes it pretty easy to do so (i.e. you
can use awk(1) to sum all of the per-cpu NMI counters).
Yep, the above "not create a dentry" flag would handle the case where
you sum things up in the kernel because the more fine grained counters
would be overwhelming.
Paolo
From: Emanuele Giuseppe Esposito <hidden> Date: 2020-05-11 09:37:15
On 5/8/20 11:44 AM, Paolo Bonzini wrote:
So in general I'd say the sources/values model holds up. We certainly
want to:
- switch immediately to callbacks instead of the type constants (so that
core statsfs code only does signed/unsigned)
- add a field to distinguish cumulative and floating properties (and use
it to determine the default file mode)
- add a new argument to statsfs_create_source and statsfs_create_values
that makes it not create directories and files respectively
- add a new API to look for a statsfs_value recursively in all the
subordinate sources, and pass the source/value pair to a callback
function; and reimplement recursive aggregation and clear in terms of
this function.
Ok I will apply this, thank you for all the suggestions.
I will post the v3 patchset in the next few weeks.
In the meanwhile, I wrote the documentation you asked (even though it's
going to change in v3), you can find it here:
https://github.com/esposem/linux/commit/dfa92f270f1aed73d5f3b7f12640b2a1635c711f
Thank you,
Emanuele
From: Jonathan Adams <hidden> Date: 2020-05-11 17:02:58
On Fri, May 8, 2020 at 2:44 AM Paolo Bonzini [off-list ref] wrote:
[Answering for Emanuele because he's not available until Monday]
On 07/05/20 19:45, Jonathan Adams wrote:
quoted
This is good work. As David Rientjes mentioned, I'm currently investigating
a similar project, based on a google-internal debugfs-based FS we call
"metricfs". It's
designed in a slightly different fashion than statsfs here is, and the
statistics exported are
mostly fed into our OpenTelemetry-like system. We're motivated by
wanting an upstreamed solution, so that we can upstream the metrics we
create that are of general interest, and lower the overall rebasing
burden for our tree.
Cool. We included a public reading API exactly so that there could be
other "frontends". I was mostly thinking of BPF as an in-tree user, but
your metricfs could definitely use the reading API.
quoted
- the 8/16/32/64 signed/unsigned integers seems like a wart, and the
built-in support to grab any offset from a structure doesn't seem like
much of an advantage. A simpler interface would be to just support an> "integer" (possibly signed/unsigned) type, which is always 64-bit, and
allow the caller to provide a function pointer to retrieve the value,
with one or two void *s cbargs. Then the framework could provide an
offset-based callback (or callbacks) similar to the existing
functionality, and a similar one for per-CPU based statistics. A
second "clear" callback could be optionally provided to allow for
statistics to be cleared, as in your current proposal.
Ok, so basically splitting get_simple_value into many separate
callbacks. The callbacks would be in a struct like
struct stats_fs_type {
uint64_t (*get)(struct stats_fs_value *, void *);
void (*clear)(struct stats_fs_value *, void *);
bool signed;
}
...
struct stats_fs_type stats_fs_type_u8 = {
stats_fs_get_u8,
stats_fs_clear_u8,
false
};
and custom types can be defined using "&(struct stats_fs_type) {...}".
That makes sense.
quoted
- Beyond the statistic's type, one *very* useful piece of metadata
for telemetry tools is knowing whether a given statistic is
"cumulative" (an unsigned counter which is only ever increased), as
opposed to a floating value (like "amount of memory used").
Good idea. Also, clearing does not make sense for a floating value, so
we can use cumulative/floating to get a default for the mode: KVM
statistics for example are mostly cumulative and mode 644, except a few
that are floating and those are all mode 444. Therefore it makes sense
to add cumulative/floating even before outputting it as metadata.
quoted
I'm more
concerned with getting the statistics model and capabilities right
from the beginning, because those are harder to adjust later.
Agreed.
quoted
1. Each metricfs metric can have one or two string or integer "keys".
If these exist, they expand the metric from a single value into a
multi-dimensional table. For example, we use this to report a hash
table we keep of functions calling "WARN()", in a 'warnings'
statistic:
% cat .../warnings/values
x86_pmu_stop 1
%
Indicates that the x86_pmu_stop() function has had a WARN() fire once
since the system was booted. If multiple functions have fired
WARN()s, they are listed in this table with their own counts. [1] We
also use these to report per-CPU counters on a CPU-by-CPU basis:
% cat .../irq_x86/NMI/values
0 42
1 18
... one line per cpu
% cat .../rx_bytes/values
lo 501360681
eth0 1457631256
These seem like two different things.
I see your point; I agree that there are two different things here.
The percpu and per-interface values are best represented as subordinate
sources, one per CPU and one per interface. For interfaces I would just
use a separate directory, but it doesn't really make sense for CPUs. So
if we can cater for it in the model, it's better. For example:
- add a new argument to statsfs_create_source and statsfs_create_values
that makes it not create directories and files respectively.
- add a new "aggregate function" STATS_FS_LIST that directs the parent
to build a table of all the simple values below it
We can also add a helper statsfs_add_values_percpu that creates a new
source for each CPU, I think.
I think I'd characterize this slightly differently; we have a set of
statistics which are essentially "in parallel":
- a variety of statistics, N CPUs they're available for, or
- a variety of statistics, N interfaces they're available for.
- a variety of statistics, N kvm object they're available for.
Recreating a parallel hierarchy of statistics any time we add/subtract
a CPU or interface seems like a lot of overhead. Perhaps a better
model would
be some sort of "parameter enumn" (naming is hard; parameter set?), so
when a CPU/network interface/etc is added you'd add its ID to the
"CPUs" we know about, and at removal time you'd take it out; it would
have an associated cbarg for the value getting callback.
Does that make sense as a design?
I'm working on characterizing all of our metricfs usage; I'll see if
this looks like it mostly covers our usecases.
The warnings one instead is a real hash table. It should be possible to
implement it as some kind of customized aggregation, that is implemented
in the client instead of coming from subordinate sources. The
presentation can then just use STATS_FS_LIST. I don't see anything in
the design that is a blocker.
Yes; though if it's low-enough overhead, you could imagine having a
dynamically-updated parameter enum based on the hash table.
quoted
2. We also export some metadata about each statistic. For example,
the metadata for the NMI counter above looks like:
% cat .../NMI/annotations
DESCRIPTION Non-maskable\ interrupts
CUMULATIVE
% cat .../NMI/fields
cpu value
int int
%
Good idea. I would prefer per-directory dot-named files for this. For
example a hypothetical statsfs version of /proc/interrupts could be like
this:
$ cat /sys/kernel/stats/interrupts/.schema
0 // Name
CUMULATIVE // Flags
int:int // Type(s)
IR-IO-APIC 2-edge timer // Description
...
LOC
CUMULATIVE
int:int
Local timer interrupts
...
$ cat /sys/kernel/stats/interrupts/LOC
0 4286815
1 4151572
2 4199361
3 4229248
quoted
3. We have a (very few) statistics where the value itself is a string,
usually for device statuses.
Maybe in addition to CUMULATIVE and FLOATING we can have ENUM
properties, and a table to convert those enums to strings. Aggregation
could also be used to make a histogram out of enums in subordinate
sources, e.g.
$ cat /sys/kernel/stats/kvm/637-1/vcpu_state
running 12
uninitialized 0
halted 4
That's along similar lines to the parameter enums, yeah.
So in general I'd say the sources/values model holds up. We certainly
want to:
- switch immediately to callbacks instead of the type constants (so that
core statsfs code only does signed/unsigned)
- add a field to distinguish cumulative and floating properties (and use
it to determine the default file mode)
Yup, these make sense.
- add a new argument to statsfs_create_source and statsfs_create_values
that makes it not create directories and files respectively
- add a new API to look for a statsfs_value recursively in all the
subordinate sources, and pass the source/value pair to a callback
function; and reimplement recursive aggregation and clear in terms of
this function.
This is where I think a little iteration on the "parameter enums"
should happen before jumping into implementation.
quoted
For our use cases, we generally don't both output a statistic and it's
aggregation from the kernel; either we sum up things in the kernel
(e.g. over a bunch of per-cpu or per-memcg counters) and only have the
result statistic, or we expect user-space to sum up the data if it's
interested. The tabular form makes it pretty easy to do so (i.e. you
can use awk(1) to sum all of the per-cpu NMI counters).
Yep, the above "not create a dentry" flag would handle the case where
you sum things up in the kernel because the more fine grained counters
would be overwhelming.
nodnod; or the callback could handle the sum itself.
Thanks,
- jonathan
From: Paolo Bonzini <pbonzini@redhat.com> Date: 2020-05-11 17:34:37
Hi Jonathan, I think the remaining sticky point is this one:
On 11/05/20 19:02, Jonathan Adams wrote:
I think I'd characterize this slightly differently; we have a set of
statistics which are essentially "in parallel":
- a variety of statistics, N CPUs they're available for, or
- a variety of statistics, N interfaces they're available for.
- a variety of statistics, N kvm object they're available for.
Recreating a parallel hierarchy of statistics any time we add/subtract
a CPU or interface seems like a lot of overhead. Perhaps a better
model would be some sort of "parameter enumn" (naming is hard;
parameter set?), so when a CPU/network interface/etc is added you'd
add its ID to the "CPUs" we know about, and at removal time you'd
take it out; it would have an associated cbarg for the value getting
callback.
quoted
Yep, the above "not create a dentry" flag would handle the case where
you sum things up in the kernel because the more fine grained counters
would be overwhelming.
nodnod; or the callback could handle the sum itself.
In general for statsfs we took a more explicit approach where each
addend in a sum is a separate stats_fs_source. In this version of the
patches it's also a directory, but we'll take your feedback and add both
the ability to hide directories (first) and to list values (second).
So, in the cases of interfaces and KVM objects I would prefer to keep
each addend separate.
For CPUs that however would be pretty bad. Many subsystems might
accumulate stats percpu for performance reason, which would then be
exposed as the sum (usually). So yeah, native handling of percpu values
makes sense. I think it should fit naturally into the same custom
aggregation framework as hash table keys, we'll see if there's any devil
in the details.
Core kernel stats such as /proc/interrupts or /proc/stat are the
exception here, since individual per-CPU values can be vital for
debugging. For those, creating a source per stat, possibly on-the-fly
at hotplug/hot-unplug time because NR_CPUS can be huge, would still be
my preferred way to do it.
Thanks,
Paolo
From: Jonathan Adams <hidden> Date: 2020-05-14 17:36:16
On Mon, May 11, 2020 at 10:34 AM Paolo Bonzini [off-list ref] wrote:
Hi Jonathan, I think the remaining sticky point is this one:
Apologies it took a couple days for me to respond; I wanted to finish
evaluating our current usage to make sure I had a full picture; I'll
summarize our state at the bottom.
On 11/05/20 19:02, Jonathan Adams wrote:
quoted
I think I'd characterize this slightly differently; we have a set of
statistics which are essentially "in parallel":
- a variety of statistics, N CPUs they're available for, or
- a variety of statistics, N interfaces they're available for.
- a variety of statistics, N kvm object they're available for.
Recreating a parallel hierarchy of statistics any time we add/subtract
a CPU or interface seems like a lot of overhead. Perhaps a better
model would be some sort of "parameter enumn" (naming is hard;
parameter set?), so when a CPU/network interface/etc is added you'd
add its ID to the "CPUs" we know about, and at removal time you'd
take it out; it would have an associated cbarg for the value getting
callback.
quoted
Yep, the above "not create a dentry" flag would handle the case where
you sum things up in the kernel because the more fine grained counters
would be overwhelming.
nodnod; or the callback could handle the sum itself.
In general for statsfs we took a more explicit approach where each
addend in a sum is a separate stats_fs_source. In this version of the
patches it's also a directory, but we'll take your feedback and add both
the ability to hide directories (first) and to list values (second).
So, in the cases of interfaces and KVM objects I would prefer to keep
each addend separate.
This just feels like a lot of churn just to add a statistic or object;
in your model, every time a KVM or VCPU is created, you create the N
statistics, leading to N*M total objects. As I was imagining it,
you'd have:
A 'parameter enum' which maps names to object pointers and
A set of statistics which map a statfs path to {callback, cbarg,
zero or more parameter enums}
So adding a new KVM VCPU would just be "add an object to the KVM's
VCPU parameter enum", and removing it would be the opposite, and a
couple callbacks could handle basically all of the stats. The only
tricky part would be making sure the parameter enum value
create/destroy and the callback calls are coordinated correctly.
If you wanted stats for a particular VCPU, we could mark the overall
directory as "include subdirs for VCPU parameter", and you'd
automatically get one directory per VCPU, with the same set of stats
in it, constrained to the single VCPU. I could also imagine having an
".agg_sum/{stata,statb,...}" to report using the aggregations you
have, or a mode to say "stats in this directory are sums over the
following VCPU parameter".
For CPUs that however would be pretty bad. Many subsystems might
accumulate stats percpu for performance reason, which would then be
exposed as the sum (usually). So yeah, native handling of percpu values
makes sense. I think it should fit naturally into the same custom
aggregation framework as hash table keys, we'll see if there's any devil
in the details.
Core kernel stats such as /proc/interrupts or /proc/stat are the
exception here, since individual per-CPU values can be vital for
debugging. For those, creating a source per stat, possibly on-the-fly
at hotplug/hot-unplug time because NR_CPUS can be huge, would still be
my preferred way to do it.
Our metricfs has basically two modes: report all per-CPU values (for
the IPI counts etc; you pass a callback which takes a 'int cpu'
argument) or a callback that sums over CPUs and reports the full
value. It also seems hard to have any subsystem with a per-CPU stat
having to install a hotplug callback to add/remove statistics.
In my model, a "CPU" parameter enum which is automatically kept
up-to-date is probably sufficient for the "report all per-CPU values".
Does this make sense to you? I realize that this is a significant
change to the model y'all are starting with; I'm willing to do the
work to flesh it out.
Thanks for your time,
- Jonathan
P.S. Here's a summary of the types of statistics we use in metricfs
in google, to give a little context:
- integer values (single value per stat, source also a single value);
a couple of these are boolean values exported as '0' or '1'.
- per-CPU integer values, reported as a <cpuid, value> table
- per-CPU integer values, summed and reported as an aggregate
- single-value values, keys related to objects:
- many per-device (disk, network, etc) integer stats
- some per-device string data (version strings, UUIDs, and
occasional statuses.)
- a few histograms (usually counts by duration ranges)
- the "function name" to count for the WARN statistic I mentioned.
- A single statistic with two keys (for livepatch statistics; the
value is the livepatch status as a string)
Most of the stats with keys are "complete" (every key has a value),
but there are several examples of statistics where only some of the
possible keys have values, or (e.g. for networking statistics) only
the keys visible to the reading process (e.g. in its namespaces) are
included.
From: Paolo Bonzini <pbonzini@redhat.com> Date: 2020-05-14 17:43:08
On 14/05/20 19:35, Jonathan Adams wrote:
quoted
In general for statsfs we took a more explicit approach where each
addend in a sum is a separate stats_fs_source. In this version of the
patches it's also a directory, but we'll take your feedback and add both
the ability to hide directories (first) and to list values (second).
So, in the cases of interfaces and KVM objects I would prefer to keep
each addend separate.
This just feels like a lot of churn just to add a statistic or object;
in your model, every time a KVM or VCPU is created, you create the N
statistics, leading to N*M total objects.
While it's N*M files, only O(M) statsfs API calls are needed to create
them. Whether you have O(N*M) total kmalloc-ed objects or O(M) is an
implementation detail.
Having O(N*M) API calls would be a non-started, I agree - especially
once you start thinking of more efficient publishing mechanisms that
unlike files are also O(M).
quoted
For CPUs that however would be pretty bad. Many subsystems might
accumulate stats percpu for performance reason, which would then be
exposed as the sum (usually). So yeah, native handling of percpu values
makes sense. I think it should fit naturally into the same custom
aggregation framework as hash table keys, we'll see if there's any devil
in the details.
Core kernel stats such as /proc/interrupts or /proc/stat are the
exception here, since individual per-CPU values can be vital for
debugging. For those, creating a source per stat, possibly on-the-fly
at hotplug/hot-unplug time because NR_CPUS can be huge, would still be
my preferred way to do it.
Our metricfs has basically two modes: report all per-CPU values (for
the IPI counts etc; you pass a callback which takes a 'int cpu'
argument) or a callback that sums over CPUs and reports the full
value. It also seems hard to have any subsystem with a per-CPU stat
having to install a hotplug callback to add/remove statistics.
Yes, this is also why I think percpu values should have some kind of
native handling. Reporting per-CPU values individually is the exception.
In my model, a "CPU" parameter enum which is automatically kept
up-to-date is probably sufficient for the "report all per-CPU values".
Yes (or a separate CPU source in my model).
Paolo
Does this make sense to you? I realize that this is a significant
change to the model y'all are starting with; I'm willing to do the
work to flesh it out.
Thanks for your time,
- Jonathan
P.S. Here's a summary of the types of statistics we use in metricfs
in google, to give a little context:
- integer values (single value per stat, source also a single value);
a couple of these are boolean values exported as '0' or '1'.
- per-CPU integer values, reported as a <cpuid, value> table
- per-CPU integer values, summed and reported as an aggregate
- single-value values, keys related to objects:
- many per-device (disk, network, etc) integer stats
- some per-device string data (version strings, UUIDs, and
occasional statuses.)
- a few histograms (usually counts by duration ranges)
- the "function name" to count for the WARN statistic I mentioned.
- A single statistic with two keys (for livepatch statistics; the
value is the livepatch status as a string)
Most of the stats with keys are "complete" (every key has a value),
but there are several examples of statistics where only some of the
possible keys have values, or (e.g. for networking statistics) only
the keys visible to the reading process (e.g. in its namespaces) are
included.