| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <7b8da3f40cbc30a25fe5fa856627245dc2b6a991.camel@arm.com>
Date: Thu, 1 Aug 2024 12:28:25 +0000
From: Ben Gainey <Ben.Gainey@....com>
To: "namhyung@...nel.org" <namhyung@...nel.org>
CC: "alexander.shishkin@...ux.intel.com" <alexander.shishkin@...ux.intel.com>,
"peterz@...radead.org" <peterz@...radead.org>, "acme@...nel.org"
<acme@...nel.org>, "mingo@...hat.com" <mingo@...hat.com>, James Clark
<James.Clark@....com>, "adrian.hunter@...el.com" <adrian.hunter@...el.com>,
"irogers@...gle.com" <irogers@...gle.com>, "jolsa@...nel.org"
<jolsa@...nel.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "linux-perf-users@...r.kernel.org"
<linux-perf-users@...r.kernel.org>, Mark Rutland <Mark.Rutland@....com>
Subject: Re: [PATCH v9 4/4] tools/perf: Allow inherit + PERF_SAMPLE_READ when
opening events
On Wed, 2024-07-31 at 11:17 -0700, Namhyung Kim wrote:
> 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@....com>
> > ---
> >
snip
> > 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
>
Done, though this looks benign; the current tools appear to do this,
and there are a couple of files in tests/attr that assumed inherit=1
for system wide mode that I've had to update.
# perf --version
perf version 6.10-1
# perf record -vvv -a --synth=no true
...
------------------------------------------------------------
perf_event_attr:
type 0 (PERF_TYPE_HARDWARE)
size 136
config 0 (PERF_COUNT_HW_CPU_CYCLES)
{ sample_period, sample_freq } 4000
sample_type IP|TID|TIME|CPU|PERIOD|IDENTIFIER
read_format ID|LOST
disabled 1
inherit 1
freq 1
precise_ip 3
sample_id_all 1
------------------------------------------------------------
Testing your fix with -a, -C <n> as well as with application profiling
appears to do the right thing.
Regards
Ben
>
> > }
> >
> > @@ -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
> >
Powered by blists - more mailing lists