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

[James Clark]

On 14/02/2025 1:27 am, Namhyung Kim wrote:
> 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@xxxxxxxxxx> wrote:
> > > > On Wed, Feb 12, 2025 at 4:24 AM James Clark <james.clark@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > > > ---
> > > > >    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?

Yep sure

> Ian, can you please send a formal patch with that?
>
> Thanks,
> Namhyung