| 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: <Z66b9-I_MkmX7pg5@google.com>
Date: Thu, 13 Feb 2025 17:27:19 -0800
From: Namhyung Kim <namhyung@...nel.org>
To: James Clark <james.clark@...aro.org>, Ian Rogers <irogers@...gle.com>
Cc: linux-perf-users@...r.kernel.org, Robin.Murphy@....com,
Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>,
Arnaldo Carvalho de Melo <acme@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Jiri Olsa <jolsa@...nel.org>,
Adrian Hunter <adrian.hunter@...el.com>,
"Liang, Kan" <kan.liang@...ux.intel.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] perf stat: Fix non-uniquified hybrid legacy events
Hello,
On Thu, Feb 13, 2025 at 12:15:30PM +0000, James Clark wrote:
>
>
> On 12/02/2025 9:38 pm, Ian Rogers wrote:
> > On Wed, Feb 12, 2025 at 9:48 AM Ian Rogers <irogers@...gle.com> wrote:
> > >
> > > On Wed, Feb 12, 2025 at 4:24 AM James Clark <james.clark@...aro.org> wrote:
> > > >
> > > > Legacy hybrid events have attr.type == PERF_TYPE_HARDWARE, so they look
> > > > like plain legacy events if we only look at attr.type. But legacy events
> > > > should still be uniquified if they were opened on a non-legacy PMU.
> > > > Previously we looked at the PMU type to determine legacy vs hybrid
> > > > events here so revert this particular check to how it was before the
> > > > linked fixes commit.
> > > >
> > > > counter->pmu doesn't need to be null checked twice, in fact it is
> > > > required for any kind of uniquification so make that a separate check.
> > > >
> > > > This restores PMU names on hybrid systems and also changes "perf stat
> > > > metrics (shadow stat) test" from a FAIL back to a SKIP (on hybrid). The
> > > > test was gated on "cycles" appearing alone which doesn't happen on
> > > > here.
> > > >
> > > > Before:
> > > >
> > > > $ perf stat -- true
> > > > ...
> > > > <not counted> instructions:u (0.00%)
> > > > 162,536 instructions:u # 0.58 insn per cycle
> > > > ...
> > > >
> > > > After:
> > > >
> > > > $ perf stat -- true
> > > > ...
> > > > <not counted> cpu_atom/instructions/u (0.00%)
> > > > 162,541 cpu_core/instructions/u # 0.62 insn per cycle
> > > > ...
> > > >
> > > > Fixes: 357b965deba9 ("perf stat: Changes to event name uniquification")
> > > > Signed-off-by: James Clark <james.clark@...aro.org>
> > > > ---
> > > > tools/perf/util/stat-display.c | 9 +++++++--
> > > > 1 file changed, 7 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> > > > index e65c7e9f15d1..eae34ba95f59 100644
> > > > --- a/tools/perf/util/stat-display.c
> > > > +++ b/tools/perf/util/stat-display.c
> > > > @@ -1688,12 +1688,17 @@ static void evsel__set_needs_uniquify(struct evsel *counter, const struct perf_s
> > > > return;
> > > > }
> > > >
> > > > - if (counter->core.attr.type < PERF_TYPE_MAX && counter->core.attr.type != PERF_TYPE_RAW) {
> > > > + if (!counter->pmu) {
> > >
> > > Thanks James, I wish I had a hybrid laptop so I didn't keep breaking
> > > things like this. I'm uncomfortable using an evsel having/not-having a
> > > PMU as an indication of whether uniquification is necessary. It is
> > > kind of a side-effect of parsing whether the PMU variable is non-NULL,
> > > it'd kind of be nice to stop things using `evsel->pmu` directly and
> > > switch them to `evsel__find_pmu(evsel)`, in the future maybe legacy
> > > events will get the core PMU, a tracepoint PMU, etc. so we'd never
> > > expect this variable to be NULL.
>
> As it stands evsel__uniquify_counter() unconditionally dereferences
> evsel->pmu which is why I thought it made sense to check that first. But if
> that might change then fair enough.
>
> > >
> > > Your commit message gives me enough to think about what the issue is,
> > > so let me give it some thought.
> >
> > I wonder we should just hoist the hybrid test earlier:
> > ```
> > diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> > index e65c7e9f15d1..e852ac0d9847 100644
> > --- a/tools/perf/util/stat-display.c
> > +++ b/tools/perf/util/stat-display.c
> > @@ -1688,6 +1688,12 @@ static void evsel__set_needs_uniquify(struct
> > evsel *counter, const struct per
> > f_s
> > return;
> > }
> >
> > + if (!config->hybrid_merge && evsel__is_hybrid(counter)) {
> > + /* Unique hybrid counters necessary. */
> > + counter->needs_uniquify = true;
> > + return;
> > + }
> > +
> > if (counter->core.attr.type < PERF_TYPE_MAX &&
> > counter->core.attr.type != PERF_TYPE_RAW) {
> > /* Legacy event, don't uniquify. */
> > return;
> > @@ -1705,12 +1711,6 @@ static void evsel__set_needs_uniquify(struct
> > evsel *counter, const struct per
> > f_s
> > return;
> > }
> >
> > - if (!config->hybrid_merge && evsel__is_hybrid(counter)) {
> > - /* Unique hybrid counters necessary. */
> > - counter->needs_uniquify = true;
> > - return;
> > - }
> > -
> > /*
> > * Do other non-merged events in the evlist have the same name? If so
> > * uniquify is necessary.
> >
> > ```
> >
> > The hybrid test is unfortunately expensive at it needs to search for
> > > 1 core PMU, which means loading all sysfs PMUs. I think we're already
> > paying the cost though.
> >
> > Could you check this works James?
> >
> > Thanks,
> > Ian
> >
>
> Yep that works too.
James, can I take it as your Tested-by?
Ian, can you please send a formal patch with that?
Thanks,
Namhyung
Powered by blists - more mailing lists