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

[Ben Gainey <Ben.Gainey@arm.com>]

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@arm.com>
> 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



> Thanks,
> Namhyung
>
> >
> > Signed-off-by: Ben Gainey <ben.gainey@arm.com>
> > ---
> >  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.