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

[Arnaldo Carvalho de Melo <acme@kernel.org>]

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,

- Arnaldo