Re: [PATCH] perf jevents: Don't stop at the first matched pmu when searching a events table

[Namhyung Kim <namhyung@kernel.org>]

On Thu, Sep 26, 2024 at 02:34:43PM -0700, Ian Rogers wrote:
> On Wed, Jul 31, 2024 at 6:31 AM Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> > On Tue, Jul 16, 2024 at 09:30:01AM -0700, Ian Rogers wrote:
> > > On Tue, Jul 16, 2024 at 6:29 AM <kan.liang@linux.intel.com> wrote:
> > > >
> > > > From: Kan Liang <kan.liang@linux.intel.com>
> > > >
> > > > The "perf all PMU test" fails on a Coffee Lake machine.
> > > >
> > > > The failure is caused by the below change in the commit e2641db83f18
> > > > ("perf vendor events: Add/update skylake events/metrics").
> > > >
> > > > +    {
> > > > +        "BriefDescription": "This 48-bit fixed counter counts the UCLK cycles",
> > > > +        "Counter": "FIXED",
> > > > +        "EventCode": "0xff",
> > > > +        "EventName": "UNC_CLOCK.SOCKET",
> > > > +        "PerPkg": "1",
> > > > +        "PublicDescription": "This 48-bit fixed counter counts the UCLK cycles.",
> > > > +        "Unit": "cbox_0"
> > > >      }
> > > >
> > > > The other cbox events have the unit name "CBOX", while the fixed counter
> > > > has a unit name "cbox_0". So the events_table will maintain separate
> > > > entries for cbox and cbox_0.
> > > >
> > > > The perf_pmus__print_pmu_events() calculates the total number of events,
> > > > allocate an aliases buffer, store all the events into the buffer, sort,
> > > > and print all the aliases one by one.
> > > >
> > > > The problem is that the calculated total number of events doesn't match
> > > > the stored events in the aliases buffer.
> > > >
> > > > The perf_pmu__num_events() is used to calculate the number of events. It
> > > > invokes the pmu_events_table__num_events() to go through the entire
> > > > events_table to find all events. Because of the
> > > > pmu_uncore_alias_match(), the suffix of uncore PMU will be ignored. So
> > > > the events for cbox and cbox_0 are all counted.
> > > >
> > > > When storing events into the aliases buffer, the
> > > > perf_pmu__for_each_event() only process the events for cbox.
> > > >
> > > > Since a bigger buffer was allocated, the last entry are all 0.
> > > > When printing all the aliases, null will be outputted, and trigger the
> > > > failure.
> > > >
> > > > The mismatch was introduced from the commit e3edd6cf6399 ("perf
> > > > pmu-events: Reduce processed events by passing PMU"). The
> > > > pmu_events_table__for_each_event() stops immediately once a pmu is set.
> > > > But for uncore, especially this case, the method is wrong and mismatch
> > > > what perf does in the perf_pmu__num_events().
> > > >
> > > > With the patch,
> > > > $ perf list pmu | grep -A 1 clock.socket
> > > >    unc_clock.socket
> > > >         [This 48-bit fixed counter counts the UCLK cycles. Unit: uncore_cbox_0
> > > > $ perf test "perf all PMU test"
> > > >   107: perf all PMU test                                               : Ok
> > > >
> > > > Reported-by: kernel test robot <oliver.sang@intel.com>
> > > > Closes: https://lore.kernel.org/all/202407101021.2c8baddb-oliver.sang@intel.com/
> > > > Fixes: e3edd6cf6399 ("perf pmu-events: Reduce processed events by passing PMU")
> > > > Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
> > >
> > > Awesome sauce, thanks!
> > >
> > > Reviewed-by: Ian Rogers <irogers@google.com>
> >
> > Thanks, applied to tmp.perf-tools-next,
> 
> Did this get applied? I'm not seeing it in perf-tools-next:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/log/tools/perf/pmu-events/jevents.py?h=perf-tools-next

I'm seeing this build failure after applying this.

    TEST    pmu-events/empty-pmu-events.log
  --- pmu-events/empty-pmu-events.c	2024-09-26 10:27:15.600055460 -0700
  +++ pmu-events/test-empty-pmu-events.c	2024-09-30 13:20:34.631357001 -0700
  @@ -380,7 +380,7 @@
                           continue;
   
                   ret = pmu_events_table__for_each_event_pmu(table, table_pmu, fn, data);
  -                if (pmu || ret)
  +                if (ret)
                           return ret;
           }
           return 0;
  make[3]: *** [pmu-events/Build:42: pmu-events/empty-pmu-events.log] Error 1
  make[3]: *** Deleting file 'pmu-events/empty-pmu-events.log'
  make[2]: *** [Makefile.perf:765: pmu-events/pmu-events-in.o] Error 2
  make[2]: *** Waiting for unfinished jobs....

Thanks,
Namhyung