Re: [PATCH v6 4/4] tools/perf: Allow inherit + PERF_SAMPLE_READ when opening events

From: Ben Gainey
Date: Wed May 29 2024 - 13:48:23 EST

Next message: Reinette Chatre: "Re: [PATCH v4 10/16] selftests/resctrl: Add ->init() callback into resctrl_val_param"
Previous message: Reinette Chatre: "Re: [PATCH v4 12/16] selftests/resctrl: Make some strings passed to resctrlfs functions const"
In reply to: Ben Gainey: "Re: [PATCH v6 4/4] tools/perf: Allow inherit + PERF_SAMPLE_READ when opening events"
Next in thread: Namhyung Kim: "Re: [PATCH v6 4/4] tools/perf: Allow inherit + PERF_SAMPLE_READ when opening events"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, 2024-05-27 at 18:47 +0100, Ben Gainey wrote:
> On Thu, 2024-05-23 at 18:41 -0700, Namhyung Kim wrote:
> > On Tue, May 21, 2024 at 6:30 AM Ben Gainey <ben.gainey@xxxxxxx>
> > wrote:
> > >
> > > The tool will now default to this new mode if the user specifies
> > > a
> > > sampling group when not in system-wide mode, and when --no-
> > > inherit
> > > is not specified.
> > >
> > > This change updates evsel to allow the combination of inherit
> > > and PERF_SAMPLE_READ.
> > >
> > > A fallback is implemented for kernel versions where this feature
> > > is
> > > not
> > > supported.
> >
> > But I'm afraid the test would fail on old kernels. Maybe we need
> > to
> > put it in the selftests.
> >
>
> Sorry, not clear what you mean?
>
> Is the issue that the fallback on older kernels fails, or that the
> "perf test" tests fail?
>
> Thanks
> Ben

Just to follow up, I've rechecked the fallback on an unmodified 6.9.1
kernel with the following:

perf record -vv -e '{cycles,instructions}:S' ls

With an unpatched version of perf running on an unpatched kernel, the
cycles event is opened as:

------------------------------------------------------------
perf_event_attr:
type 0 (PERF_TYPE_HARDWARE)
size 136
config 0 (PERF_COUNT_HW_CPU_CYCLES)
{ sample_period, sample_freq } 4000
sample_type IP|TID|TIME|READ|ID|PERIOD
read_format ID|GROUP|LOST
disabled 1
exclude_kernel 1
exclude_hv 1
mmap 1
comm 1
freq 1
enable_on_exec 1
task 1
sample_id_all 1
exclude_guest 1
mmap2 1
comm_exec 1
ksymbol 1
bpf_event 1
------------------------------------------------------------

whereas with these patches applied to perf, on an unpatched kernel, the
output is as follows

------------------------------------------------------------
perf_event_attr:
type 0 (PERF_TYPE_HARDWARE)
size 136
config 0 (PERF_COUNT_HW_CPU_CYCLES)
{ sample_period, sample_freq } 4000
sample_type IP|TID|TIME|READ|ID|PERIOD
read_format ID|GROUP|LOST
disabled 1
inherit 1
exclude_kernel 1
exclude_hv 1
mmap 1
comm 1
freq 1
enable_on_exec 1
task 1
sample_id_all 1
exclude_guest 1
mmap2 1
comm_exec 1
ksymbol 1
bpf_event 1
------------------------------------------------------------
sys_perf_event_open: pid 3442954 cpu 0 group_fd -1 flags 0x8
sys_perf_event_open failed, error -22
Using PERF_SAMPLE_READ / :S modifier is not compatible with
inherit, falling back to no-inherit.
------------------------------------------------------------
perf_event_attr:
type 0 (PERF_TYPE_HARDWARE)
size 136
config 0 (PERF_COUNT_HW_CPU_CYCLES)
{ sample_period, sample_freq } 4000
sample_type IP|TID|TIME|READ|ID|PERIOD
read_format ID|GROUP|LOST
disabled 1
exclude_kernel 1
exclude_hv 1
mmap 1
comm 1
freq 1
enable_on_exec 1
task 1
sample_id_all 1
exclude_guest 1
mmap2 1
comm_exec 1
ksymbol 1
bpf_event 1
------------------------------------------------------------

The command falls back to the same configuration as was previously
used. The same is true for the instructions event.

`perf test` fails on an unpatched kernel in "15: Setup struct
perf_event_attr" for the test "test-record-group-sampling1" but that is
surely expected for unpatched kernels?

Is there some very-old kernel version where this would be expected to
succeed by accident?

Thanks
Ben

>
>
>
> > Thanks,
> > Namhyung
> >
> > >
> > > Signed-off-by: Ben Gainey <ben.gainey@xxxxxxx>
> > > ---
> > > tools/perf/tests/attr/README | 2 +
> > > .../tests/attr/test-record-group-sampling | 39 ------------
> > > .../tests/attr/test-record-group-sampling1 | 50
> > > ++++++++++++++++
> > > .../tests/attr/test-record-group-sampling2 | 60
> > > +++++++++++++++++++
> > > tools/perf/tests/attr/test-record-group2 | 9 +--
> > > tools/perf/util/evsel.c | 19 +++++-
> > > tools/perf/util/evsel.h | 1 +
> > > 7 files changed, 135 insertions(+), 45 deletions(-)
> > > delete mode 100644 tools/perf/tests/attr/test-record-group-
> > > sampling
> > > create mode 100644 tools/perf/tests/attr/test-record-group-
> > > sampling1
> > > create mode 100644 tools/perf/tests/attr/test-record-group-
> > > sampling2
> > >
> > > diff --git a/tools/perf/tests/attr/README
> > > b/tools/perf/tests/attr/README
> > > index 4066fec7180a8..67c4ca76b85d5 100644
> > > --- a/tools/perf/tests/attr/README
> > > +++ b/tools/perf/tests/attr/README
> > > @@ -51,6 +51,8 @@ Following tests are defined (with perf
> > > commands):
> > > perf record --call-graph fp kill (test-record-
> > > graph-fp-aarch64)
> > > perf record -e '{cycles,instructions}' kill (test-record-
> > > group1)
> > > perf record -e '{cycles/period=1/,instructions/period=2/}:S'
> > > kill (test-record-group2)
> > > + perf record -e '{cycles,cache-misses}:S' kill (test-record-
> > > group-sampling1)
> > > + perf record -c 10000 -e '{cycles,cache-misses}:S' kill (test-
> > > record-group-sampling2)
> > > perf record -D kill (test-record-no-
> > > delay)
> > > perf record -i kill (test-record-no-
> > > inherit)
> > > perf record -n kill (test-record-no-
> > > samples)
> > > diff --git a/tools/perf/tests/attr/test-record-group-sampling
> > > b/tools/perf/tests/attr/test-record-group-sampling
> > > deleted file mode 100644
> > > index 97e7e64a38f07..0000000000000
> > > --- a/tools/perf/tests/attr/test-record-group-sampling
> > > +++ /dev/null
> > > @@ -1,39 +0,0 @@
> > > -[config]
> > > -command = record
> > > -args = --no-bpf-event -e '{cycles,cache-misses}:S' kill
> > > > /dev/null 2>&1
> > > -ret = 1
> > > -
> > > -[event-1:base-record]
> > > -fd=1
> > > -group_fd=-1
> > > -sample_type=343
> > > -read_format=12|28
> > > -inherit=0
> > > -
> > > -[event-2:base-record]
> > > -fd=2
> > > -group_fd=1
> > > -
> > > -# cache-misses
> > > -type=0
> > > -config=3
> > > -
> > > -# default | PERF_SAMPLE_READ
> > > -sample_type=343
> > > -
> > > -# PERF_FORMAT_ID | PERF_FORMAT_GROUP | PERF_FORMAT_LOST
> > > -read_format=12|28
> > > -task=0
> > > -mmap=0
> > > -comm=0
> > > -enable_on_exec=0
> > > -disabled=0
> > > -
> > > -# inherit is disabled for group sampling
> > > -inherit=0
> > > -
> > > -# sampling disabled
> > > -sample_freq=0
> > > -sample_period=0
> > > -freq=0
> > > -write_backward=0
> > > diff --git a/tools/perf/tests/attr/test-record-group-sampling1
> > > b/tools/perf/tests/attr/test-record-group-sampling1
> > > new file mode 100644
> > > index 0000000000000..9b87306266329
> > > --- /dev/null
> > > +++ b/tools/perf/tests/attr/test-record-group-sampling1
> > > @@ -0,0 +1,50 @@
> > > +[config]
> > > +command = record
> > > +args = --no-bpf-event -e '{cycles,cache-misses}:S' kill
> > > > /dev/null 2>&1
> > > +ret = 1
> > > +
> > > +[event-1:base-record]
> > > +fd=1
> > > +group_fd=-1
> > > +
> > > +# cycles
> > > +type=0
> > > +config=0
> > > +
> > > +# default | PERF_SAMPLE_READ
> > > +sample_type=343
> > > +
> > > +# PERF_FORMAT_ID | PERF_FORMAT_GROUP | PERF_FORMAT_LOST |
> > > PERF_FORMAT_TOTAL_TIME_ENABLED | PERF_FORMAT_TOTAL_TIME_RUNNING
> > > +read_format=28|31
> > > +task=1
> > > +mmap=1
> > > +comm=1
> > > +enable_on_exec=1
> > > +disabled=1
> > > +
> > > +# inherit is enabled for group sampling
> > > +inherit=1
> > > +
> > > +[event-2:base-record]
> > > +fd=2
> > > +group_fd=1
> > > +
> > > +# cache-misses
> > > +type=0
> > > +config=3
> > > +
> > > +# default | PERF_SAMPLE_READ
> > > +sample_type=343
> > > +
> > > +# PERF_FORMAT_ID | PERF_FORMAT_GROUP | PERF_FORMAT_LOST |
> > > PERF_FORMAT_TOTAL_TIME_ENABLED | PERF_FORMAT_TOTAL_TIME_RUNNING
> > > +read_format=28|31
> > > +task=0
> > > +mmap=0
> > > +comm=0
> > > +enable_on_exec=0
> > > +disabled=0
> > > +freq=0
> > > +
> > > +# inherit is enabled for group sampling
> > > +inherit=1
> > > +
> > > diff --git a/tools/perf/tests/attr/test-record-group-sampling2
> > > b/tools/perf/tests/attr/test-record-group-sampling2
> > > new file mode 100644
> > > index 0000000000000..8e29fc13f6668
> > > --- /dev/null
> > > +++ b/tools/perf/tests/attr/test-record-group-sampling2
> > > @@ -0,0 +1,60 @@
> > > +[config]
> > > +command = record
> > > +args = --no-bpf-event -c 10000 -e '{cycles,cache-misses}:S'
> > > kill >/dev/null 2>&1
> > > +ret = 1
> > > +
> > > +[event-1:base-record]
> > > +fd=1
> > > +group_fd=-1
> > > +
> > > +# cycles
> > > +type=0
> > > +config=0
> > > +
> > > +# default | PERF_SAMPLE_READ
> > > +sample_type=87
> > > +
> > > +# PERF_FORMAT_ID | PERF_FORMAT_GROUP | PERF_FORMAT_LOST |
> > > PERF_FORMAT_TOTAL_TIME_ENABLED | PERF_FORMAT_TOTAL_TIME_RUNNING
> > > +read_format=28|31
> > > +task=1
> > > +mmap=1
> > > +comm=1
> > > +enable_on_exec=1
> > > +disabled=1
> > > +
> > > +# inherit is enabled for group sampling
> > > +inherit=1
> > > +
> > > +# sampling disabled
> > > +sample_freq=0
> > > +sample_period=10000
> > > +freq=0
> > > +write_backward=0
> > > +
> > > +[event-2:base-record]
> > > +fd=2
> > > +group_fd=1
> > > +
> > > +# cache-misses
> > > +type=0
> > > +config=3
> > > +
> > > +# default | PERF_SAMPLE_READ
> > > +sample_type=87
> > > +
> > > +# PERF_FORMAT_ID | PERF_FORMAT_GROUP | PERF_FORMAT_LOST |
> > > PERF_FORMAT_TOTAL_TIME_ENABLED | PERF_FORMAT_TOTAL_TIME_RUNNING
> > > +read_format=28|31
> > > +task=0
> > > +mmap=0
> > > +comm=0
> > > +enable_on_exec=0
> > > +disabled=0
> > > +
> > > +# inherit is enabled for group sampling
> > > +inherit=1
> > > +
> > > +# sampling disabled
> > > +sample_freq=0
> > > +sample_period=0
> > > +freq=0
> > > +write_backward=0
> > > diff --git a/tools/perf/tests/attr/test-record-group2
> > > b/tools/perf/tests/attr/test-record-group2
> > > index cebdaa8e64e47..785892a54d9e1 100644
> > > --- a/tools/perf/tests/attr/test-record-group2
> > > +++ b/tools/perf/tests/attr/test-record-group2
> > > @@ -9,8 +9,9 @@ group_fd=-1
> > > config=0|1
> > > sample_period=1234000
> > > sample_type=87
> > > -read_format=12|28
> > > -inherit=0
> > > +read_format=28|31
> > > +disabled=1
> > > +inherit=1
> > > freq=0
> > >
> > > [event-2:base-record]
> > > @@ -19,9 +20,9 @@ group_fd=1
> > > config=0|1
> > > sample_period=6789000
> > > sample_type=87
> > > -read_format=12|28
> > > +read_format=28|31
> > > disabled=0
> > > -inherit=0
> > > +inherit=1
> > > mmap=0
> > > comm=0
> > > freq=0
> > > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > > index 3536404e9447b..557d409c53d6c 100644
> > > --- a/tools/perf/util/evsel.c
> > > +++ b/tools/perf/util/evsel.c
> > > @@ -1156,7 +1156,15 @@ void evsel__config(struct evsel *evsel,
> > > struct record_opts *opts,
> > > */
> > > if (leader->core.nr_members > 1) {
> > > attr->read_format |= PERF_FORMAT_GROUP;
> > > - attr->inherit = 0;
> > > + }
> > > +
> > > + /*
> > > + * Inherit + SAMPLE_READ requires SAMPLE_TID in
> > > the
> > > read_format
> > > + */
> > > + if (attr->inherit) {
> > > + evsel__set_sample_bit(evsel, TID);
> > > + evsel->core.attr.read_format |=
> > > + PERF_FORMAT_ID;
> > > }
> > > }
> > >
> > > @@ -1832,6 +1840,8 @@ static int __evsel__prepare_open(struct
> > > evsel
> > > *evsel, struct perf_cpu_map *cpus,
> > >
> > > static void evsel__disable_missing_features(struct evsel *evsel)
> > > {
> > > + if (perf_missing_features.inherit_sample_read)
> > > + evsel->core.attr.inherit = 0;
> > > if (perf_missing_features.branch_counters)
> > > evsel->core.attr.branch_sample_type &=
> > > ~PERF_SAMPLE_BRANCH_COUNTERS;
> > > if (perf_missing_features.read_lost)
> > > @@ -1887,7 +1897,12 @@ bool evsel__detect_missing_features(struct
> > > evsel *evsel)
> > > * Must probe features in the order they were added to
> > > the
> > > * perf_event_attr interface.
> > > */
> > > - if (!perf_missing_features.branch_counters &&
> > > + if (!perf_missing_features.inherit_sample_read &&
> > > + evsel->core.attr.inherit && (evsel-
> > > > core.attr.sample_type & PERF_SAMPLE_READ)) {
> > > + perf_missing_features.inherit_sample_read = true;
> > > + pr_debug2("Using PERF_SAMPLE_READ / :S modifier
> > > is
> > > not compatible with inherit, falling back to no-inherit.\n");
> > > + return true;
> > > + } else if (!perf_missing_features.branch_counters &&
> > > (evsel->core.attr.branch_sample_type &
> > > PERF_SAMPLE_BRANCH_COUNTERS)) {
> > > perf_missing_features.branch_counters = true;
> > > pr_debug2("switching off branch counters
> > > support\n");
> > > diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> > > index 517cff431de20..21b8b7e70e75e 100644
> > > --- a/tools/perf/util/evsel.h
> > > +++ b/tools/perf/util/evsel.h
> > > @@ -192,6 +192,7 @@ struct perf_missing_features {
> > > bool weight_struct;
> > > bool read_lost;
> > > bool branch_counters;
> > > + bool inherit_sample_read;
> > > };
> > >
> > > extern struct perf_missing_features perf_missing_features;
> > > --
> > > 2.45.1
> > >
>

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Next message: Reinette Chatre: "Re: [PATCH v4 10/16] selftests/resctrl: Add ->init() callback into resctrl_val_param"
Previous message: Reinette Chatre: "Re: [PATCH v4 12/16] selftests/resctrl: Make some strings passed to resctrlfs functions const"
In reply to: Ben Gainey: "Re: [PATCH v6 4/4] tools/perf: Allow inherit + PERF_SAMPLE_READ when opening events"
Next in thread: Namhyung Kim: "Re: [PATCH v6 4/4] tools/perf: Allow inherit + PERF_SAMPLE_READ when opening events"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]