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 v6 4/4] tools/perf: Allow inherit + PERF_SAMPLE_READ when opening events
Date: Thu, 23 May 2024 18:41:25 -0700 [thread overview]
Message-ID: <CAM9d7chssov_fW3ZbpYqGRyhS=xuHLyJ7=rkxTNnJ6fkgEdGJQ@mail.gmail.com> (raw)
In-Reply-To: <20240521133029.83654-5-ben.gainey@arm.com>
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.
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
>
next prev parent reply other threads:[~2024-05-24 1:41 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-21 13:30 [PATCH v6 0/4] perf: Support PERF_SAMPLE_READ with inherit Ben Gainey
2024-05-21 13:30 ` [PATCH v6 1/4] " Ben Gainey
2024-05-24 1:38 ` Namhyung Kim
2024-05-21 13:30 ` [PATCH v6 2/4] tools/perf: Track where perf_sample_ids need per-thread periods Ben Gainey
2024-05-21 13:30 ` [PATCH v6 3/4] tools/perf: Correctly calculate sample period for inherited SAMPLE_READ values Ben Gainey
2024-05-21 13:30 ` [PATCH v6 4/4] tools/perf: Allow inherit + PERF_SAMPLE_READ when opening events Ben Gainey
2024-05-24 1:41 ` Namhyung Kim [this message]
2024-05-27 17:47 ` Ben Gainey
2024-05-29 17:47 ` Ben Gainey
2024-05-29 19:18 ` Namhyung Kim
2024-05-30 12:54 ` James Clark
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='CAM9d7chssov_fW3ZbpYqGRyhS=xuHLyJ7=rkxTNnJ6fkgEdGJQ@mail.gmail.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).