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

From: Namhyung Kim
Date: Wed Jul 31 2024 - 14:17:46 EST

Next message: Alan Stern: "Re: [PATCH] usb: storage: ene_ub6250: Fix right shift warnings"
Previous message: Jan Kara: "Re: [PATCH] vfs: Fix potential circular locking through setxattr() and removexattr()"
In reply to: Ben Gainey: "[PATCH v9 4/4] tools/perf: Allow inherit + PERF_SAMPLE_READ when opening events"
Next in thread: Peter Zijlstra: "Re: [PATCH v9 0/4] perf: Support PERF_SAMPLE_READ with inherit"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hello,

On Tue, Jul 30, 2024 at 09:44:17AM +0100, Ben Gainey wrote:
> The "perf record" 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.
>
> Signed-off-by: Ben Gainey <ben.gainey@xxxxxxx>
> ---
> tools/perf/tests/attr/README | 2 +
> .../tests/attr/test-record-group-sampling | 3 +-
> .../tests/attr/test-record-group-sampling1 | 51 ++++++++++++++++
> .../tests/attr/test-record-group-sampling2 | 61 +++++++++++++++++++
> tools/perf/tests/attr/test-record-group2 | 1 +
> ...{test-record-group2 => test-record-group3} | 10 +--
> tools/perf/util/evsel.c | 19 +++++-
> tools/perf/util/evsel.h | 1 +
> 8 files changed, 141 insertions(+), 7 deletions(-)
> create mode 100644 tools/perf/tests/attr/test-record-group-sampling1
> create mode 100644 tools/perf/tests/attr/test-record-group-sampling2
> copy tools/perf/tests/attr/{test-record-group2 => test-record-group3} (81%)
>
> diff --git a/tools/perf/tests/attr/README b/tools/perf/tests/attr/README
> index 4066fec7180a..67c4ca76b85d 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
> index 97e7e64a38f0..da7a5d10785f 100644
> --- a/tools/perf/tests/attr/test-record-group-sampling
> +++ b/tools/perf/tests/attr/test-record-group-sampling
> @@ -2,6 +2,7 @@
> command = record
> args = --no-bpf-event -e '{cycles,cache-misses}:S' kill >/dev/null 2>&1
> ret = 1
> +kernel_until = 6.11

I guess it's v6.12. :)

>
> [event-1:base-record]
> fd=1
> @@ -18,7 +19,7 @@ group_fd=1
> type=0
> config=3
>
> -# default | PERF_SAMPLE_READ
> +# default | PERF_SAMPLE_READ | PERF_SAMPLE_PERIOD
> sample_type=343
>
> # PERF_FORMAT_ID | PERF_FORMAT_GROUP | PERF_FORMAT_LOST
> 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 000000000000..b02de391718d
> --- /dev/null
> +++ b/tools/perf/tests/attr/test-record-group-sampling1
> @@ -0,0 +1,51 @@
> +[config]
> +command = record
> +args = --no-bpf-event -e '{cycles,cache-misses}:S' kill >/dev/null 2>&1
> +ret = 1
> +kernel_since = 6.11
> +
> +[event-1:base-record]
> +fd=1
> +group_fd=-1
> +
> +# cycles
> +type=0
> +config=0
> +
> +# default | PERF_SAMPLE_READ | PERF_SAMPLE_PERIOD
> +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 | PERF_SAMPLE_PERIOD
> +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 000000000000..060fd1d24f63
> --- /dev/null
> +++ b/tools/perf/tests/attr/test-record-group-sampling2
> @@ -0,0 +1,61 @@
> +[config]
> +command = record
> +args = --no-bpf-event -c 10000 -e '{cycles,cache-misses}:S' kill >/dev/null 2>&1
> +ret = 1
> +kernel_since = 6.11
> +
> +[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 cebdaa8e64e4..ad97df77a506 100644
> --- a/tools/perf/tests/attr/test-record-group2
> +++ b/tools/perf/tests/attr/test-record-group2
> @@ -2,6 +2,7 @@
> command = record
> args = --no-bpf-event -e '{cycles/period=1234000/,instructions/period=6789000/}:S' kill >/dev/null 2>&1
> ret = 1
> +kernel_until = 6.11
>
> [event-1:base-record]
> fd=1
> diff --git a/tools/perf/tests/attr/test-record-group2 b/tools/perf/tests/attr/test-record-group3
> similarity index 81%
> copy from tools/perf/tests/attr/test-record-group2
> copy to tools/perf/tests/attr/test-record-group3
> index cebdaa8e64e4..311afb478b85 100644
> --- a/tools/perf/tests/attr/test-record-group2
> +++ b/tools/perf/tests/attr/test-record-group3
> @@ -2,6 +2,7 @@
> command = record
> args = --no-bpf-event -e '{cycles/period=1234000/,instructions/period=6789000/}:S' kill >/dev/null 2>&1
> ret = 1
> +kernel_since = 6.11
>
> [event-1:base-record]
> fd=1
> @@ -9,8 +10,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 +21,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 bc603193c477..ceb09b6a8c2f 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -1171,7 +1171,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;
> }

Also I think we should reset the inherit bit for system-wide events.

$ perf record -a --synth=no true
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.042 MB perf.data (51 samples) ]

$ perf evlist -v | tr ',' '\n' | grep inherit
inherit: 1
inherit: 1

Maybe something like this:

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index bc603193c477..9423cd65c3c4 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -1149,7 +1149,7 @@ void evsel__config(struct evsel *evsel, struct record_opts *opts,
bool per_cpu = opts->target.default_per_cpu && !opts->target.per_thread;

attr->sample_id_all = perf_missing_features.sample_id_all ? 0 : 1;
- attr->inherit = !opts->no_inherit;
+ attr->inherit = target__has_cpu(&opts->target) ? 0 : !opts->no_inherit;
attr->write_backward = opts->overwrite ? 1 : 0;
attr->read_format = PERF_FORMAT_LOST;

Thanks,
Namhyung

> }
>
> @@ -2020,6 +2028,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)
> @@ -2075,7 +2085,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 80b5f6dd868e..bb0c91c23679 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -206,6 +206,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.2
>

Next message: Alan Stern: "Re: [PATCH] usb: storage: ene_ub6250: Fix right shift warnings"
Previous message: Jan Kara: "Re: [PATCH] vfs: Fix potential circular locking through setxattr() and removexattr()"
In reply to: Ben Gainey: "[PATCH v9 4/4] tools/perf: Allow inherit + PERF_SAMPLE_READ when opening events"
Next in thread: Peter Zijlstra: "Re: [PATCH v9 0/4] perf: Support PERF_SAMPLE_READ with inherit"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]