From: Namhyung Kim <namhyung@kernel.org>
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
Subject: Re: [PATCH v9 4/4] tools/perf: Allow inherit + PERF_SAMPLE_READ when opening events
Date: Wed, 31 Jul 2024 11:17:34 -0700 [thread overview]
Message-ID: <Zqp_vqCn0FEfGFwX@google.com> (raw)
In-Reply-To: <20240730084417.7693-5-ben.gainey@arm.com>
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@arm.com>
> ---
> 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 prev parent reply other threads:[~2024-07-31 18:17 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-07-30 8:44 [PATCH v9 0/4] perf: Support PERF_SAMPLE_READ with inherit Ben Gainey
2024-07-30 8:44 ` [PATCH v9 1/4] perf: Rename perf_event_context.nr_pending to nr_no_switch_fast Ben Gainey
2024-08-05 11:56 ` [tip: perf/core] " tip-bot2 for Ben Gainey
2024-07-30 8:44 ` [PATCH v9 2/4] perf: Support PERF_SAMPLE_READ with inherit Ben Gainey
2024-08-05 11:56 ` [tip: perf/core] " tip-bot2 for Ben Gainey
2024-07-30 8:44 ` [PATCH v9 3/4] tools/perf: Correctly calculate sample period for inherited SAMPLE_READ values Ben Gainey
2024-07-30 8:44 ` [PATCH v9 4/4] tools/perf: Allow inherit + PERF_SAMPLE_READ when opening events Ben Gainey
2024-07-31 18:17 ` Namhyung Kim [this message]
2024-08-01 12:28 ` Ben Gainey
2024-07-30 12:25 ` [PATCH v9 0/4] perf: Support PERF_SAMPLE_READ with inherit Peter Zijlstra
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Zqp_vqCn0FEfGFwX@google.com \
--to=namhyung@kernel.org \
--cc=acme@kernel.org \
--cc=adrian.hunter@intel.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=ben.gainey@arm.com \
--cc=irogers@google.com \
--cc=james.clark@arm.com \
--cc=jolsa@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.