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

[Ian Rogers]

On Tue, Jul 16, 2024 at 6:29 AM <kan.liang@xxxxxxxxxxxxxxx> wrote:
>
> From: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxx>
> Closes: https://lore.kernel.org/all/202407101021.2c8baddb-oliver.sang@xxxxxxxxx/
> Fixes: e3edd6cf6399 ("perf pmu-events: Reduce processed events by passing PMU")
> Signed-off-by: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>

Awesome sauce, thanks!

Reviewed-by: Ian Rogers <irogers@xxxxxxxxxx>

Thanks,
Ian

> ---
>  tools/perf/pmu-events/jevents.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
> index ac9b7ca41856..97a3b018f865 100755
> --- a/tools/perf/pmu-events/jevents.py
> +++ b/tools/perf/pmu-events/jevents.py
> @@ -923,7 +923,7 @@ int pmu_events_table__for_each_event(const struct pmu_events_table *table,
>                          continue;
>
>                  ret = pmu_events_table__for_each_event_pmu(table, table_pmu, fn, data);
> -                if (pmu || ret)
> +                if (ret)
>                          return ret;
>          }
>          return 0;
> --
> 2.38.1
>