Message-ID: <CAP-5=fV0rWEL-ewGpoDwaJ3rvbTPXSx0YTuF5p9=6+h5oUhsfg@mail.gmail.com>
Date: Wed, 12 Feb 2025 13:38:14 -0800
From: Ian Rogers <irogers@...gle.com>
To: James Clark <james.clark@...aro.org>
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>, Namhyung Kim <namhyung@...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

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.
>
> 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

> Thanks!
> Ian
>
> > +               /* evsel__uniquify_counter() uses counter->pmu for the name */
> > +               return;
> > +       }
> > +
> > +       if (counter->pmu->type < PERF_TYPE_MAX && counter->pmu->type != PERF_TYPE_RAW) {
> >                 /* Legacy event, don't uniquify. */
> >                 return;
> >         }
> >
> > -       if (counter->pmu && counter->pmu->is_core &&
> > +       if (counter->pmu->is_core &&
> >             counter->alternate_hw_config != PERF_COUNT_HW_MAX) {
> >                 /* A sysfs or json event replacing a legacy event, don't uniquify. */
> >                 return;
> > --
> > 2.34.1
> >

Powered by blists - more mailing lists