Re: [PATCH] perf stat: Fix non-uniquified hybrid legacy events

[Namhyung Kim <namhyung@kernel.org>]

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@google.com> wrote:
> > > 
> > > On Wed, Feb 12, 2025 at 4:24 AM James Clark <james.clark@linaro.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@linaro.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