Received-SPF: pass (google.com: domain of linux-kernel+bounces-188215-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240521133029.83654-1-ben.gainey@arm.com> <20240521133029.83654-5-ben.gainey@arm.com>
In-Reply-To: <20240521133029.83654-5-ben.gainey@arm.com>
From: Namhyung Kim <namhyung@kernel.org>
Date: Thu, 23 May 2024 18:41:25 -0700
Message-ID: <CAM9d7chssov_fW3ZbpYqGRyhS=xuHLyJ7=rkxTNnJ6fkgEdGJQ@mail.gmail.com>
Subject: Re: [PATCH v6 4/4] tools/perf: Allow inherit + PERF_SAMPLE_READ when
 opening events
To: Ben Gainey <ben.gainey@arm.com>
Cc: peterz@infradead.org, mingo@redhat.com, acme@kernel.org, 
	james.clark@arm.com, mark.rutland@arm.com, alexander.shishkin@linux.intel.com, 
	jolsa@kernel.org, irogers@google.com, adrian.hunter@intel.com, 
	linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, May 21, 2024 at 6:30=E2=80=AFAM Ben Gainey <ben.gainey@arm.com> wro=
te:
>
> 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.

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-aa=
rch64)
>    perf record -e '{cycles,instructions}' kill   (test-record-group1)
>    perf record -e '{cycles/period=3D1/,instructions/period=3D2/}:S' kill =
(test-record-group2)
> +  perf record -e '{cycles,cache-misses}:S' kill (test-record-group-sampl=
ing1)
> +  perf record -c 10000 -e '{cycles,cache-misses}:S' kill (test-record-gr=
oup-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/per=
f/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 =3D record
> -args    =3D --no-bpf-event -e '{cycles,cache-misses}:S' kill >/dev/null =
2>&1
> -ret     =3D 1
> -
> -[event-1:base-record]
> -fd=3D1
> -group_fd=3D-1
> -sample_type=3D343
> -read_format=3D12|28
> -inherit=3D0
> -
> -[event-2:base-record]
> -fd=3D2
> -group_fd=3D1
> -
> -# cache-misses
> -type=3D0
> -config=3D3
> -
> -# default | PERF_SAMPLE_READ
> -sample_type=3D343
> -
> -# PERF_FORMAT_ID | PERF_FORMAT_GROUP  | PERF_FORMAT_LOST
> -read_format=3D12|28
> -task=3D0
> -mmap=3D0
> -comm=3D0
> -enable_on_exec=3D0
> -disabled=3D0
> -
> -# inherit is disabled for group sampling
> -inherit=3D0
> -
> -# sampling disabled
> -sample_freq=3D0
> -sample_period=3D0
> -freq=3D0
> -write_backward=3D0
> diff --git a/tools/perf/tests/attr/test-record-group-sampling1 b/tools/pe=
rf/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 =3D record
> +args    =3D --no-bpf-event -e '{cycles,cache-misses}:S' kill >/dev/null =
2>&1
> +ret     =3D 1
> +
> +[event-1:base-record]
> +fd=3D1
> +group_fd=3D-1
> +
> +# cycles
> +type=3D0
> +config=3D0
> +
> +# default | PERF_SAMPLE_READ
> +sample_type=3D343
> +
> +# PERF_FORMAT_ID | PERF_FORMAT_GROUP  | PERF_FORMAT_LOST | PERF_FORMAT_T=
OTAL_TIME_ENABLED | PERF_FORMAT_TOTAL_TIME_RUNNING
> +read_format=3D28|31
> +task=3D1
> +mmap=3D1
> +comm=3D1
> +enable_on_exec=3D1
> +disabled=3D1
> +
> +# inherit is enabled for group sampling
> +inherit=3D1
> +
> +[event-2:base-record]
> +fd=3D2
> +group_fd=3D1
> +
> +# cache-misses
> +type=3D0
> +config=3D3
> +
> +# default | PERF_SAMPLE_READ
> +sample_type=3D343
> +
> +# PERF_FORMAT_ID | PERF_FORMAT_GROUP  | PERF_FORMAT_LOST | PERF_FORMAT_T=
OTAL_TIME_ENABLED | PERF_FORMAT_TOTAL_TIME_RUNNING
> +read_format=3D28|31
> +task=3D0
> +mmap=3D0
> +comm=3D0
> +enable_on_exec=3D0
> +disabled=3D0
> +freq=3D0
> +
> +# inherit is enabled for group sampling
> +inherit=3D1
> +
> diff --git a/tools/perf/tests/attr/test-record-group-sampling2 b/tools/pe=
rf/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 =3D record
> +args    =3D --no-bpf-event -c 10000 -e '{cycles,cache-misses}:S' kill >/=
dev/null 2>&1
> +ret     =3D 1
> +
> +[event-1:base-record]
> +fd=3D1
> +group_fd=3D-1
> +
> +# cycles
> +type=3D0
> +config=3D0
> +
> +# default | PERF_SAMPLE_READ
> +sample_type=3D87
> +
> +# PERF_FORMAT_ID | PERF_FORMAT_GROUP  | PERF_FORMAT_LOST | PERF_FORMAT_T=
OTAL_TIME_ENABLED | PERF_FORMAT_TOTAL_TIME_RUNNING
> +read_format=3D28|31
> +task=3D1
> +mmap=3D1
> +comm=3D1
> +enable_on_exec=3D1
> +disabled=3D1
> +
> +# inherit is enabled for group sampling
> +inherit=3D1
> +
> +# sampling disabled
> +sample_freq=3D0
> +sample_period=3D10000
> +freq=3D0
> +write_backward=3D0
> +
> +[event-2:base-record]
> +fd=3D2
> +group_fd=3D1
> +
> +# cache-misses
> +type=3D0
> +config=3D3
> +
> +# default | PERF_SAMPLE_READ
> +sample_type=3D87
> +
> +# PERF_FORMAT_ID | PERF_FORMAT_GROUP  | PERF_FORMAT_LOST | PERF_FORMAT_T=
OTAL_TIME_ENABLED | PERF_FORMAT_TOTAL_TIME_RUNNING
> +read_format=3D28|31
> +task=3D0
> +mmap=3D0
> +comm=3D0
> +enable_on_exec=3D0
> +disabled=3D0
> +
> +# inherit is enabled for group sampling
> +inherit=3D1
> +
> +# sampling disabled
> +sample_freq=3D0
> +sample_period=3D0
> +freq=3D0
> +write_backward=3D0
> 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=3D-1
>  config=3D0|1
>  sample_period=3D1234000
>  sample_type=3D87
> -read_format=3D12|28
> -inherit=3D0
> +read_format=3D28|31
> +disabled=3D1
> +inherit=3D1
>  freq=3D0
>
>  [event-2:base-record]
> @@ -19,9 +20,9 @@ group_fd=3D1
>  config=3D0|1
>  sample_period=3D6789000
>  sample_type=3D87
> -read_format=3D12|28
> +read_format=3D28|31
>  disabled=3D0
> -inherit=3D0
> +inherit=3D1
>  mmap=3D0
>  comm=3D0
>  freq=3D0
> 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 rec=
ord_opts *opts,
>                  */
>                 if (leader->core.nr_members > 1) {
>                         attr->read_format |=3D PERF_FORMAT_GROUP;
> -                       attr->inherit =3D 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 |=3D
> +                               PERF_FORMAT_ID;
>                 }
>         }
>
> @@ -1832,6 +1840,8 @@ static int __evsel__prepare_open(struct evsel *evse=
l, 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 =3D 0;
>         if (perf_missing_features.branch_counters)
>                 evsel->core.attr.branch_sample_type &=3D ~PERF_SAMPLE_BRA=
NCH_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 & P=
ERF_SAMPLE_READ)) {
> +               perf_missing_features.inherit_sample_read =3D true;
> +               pr_debug2("Using PERF_SAMPLE_READ / :S modifier is not co=
mpatible 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_COU=
NTERS)) {
>                 perf_missing_features.branch_counters =3D 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
>