[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAP-5=fVZVU7kMOHz7CC7O1+2dX844dHqhqMebzEKWMA=59am-g@mail.gmail.com>
Date: Tue, 16 Jul 2024 09:30:01 -0700
From: Ian Rogers <irogers@...gle.com>
To: kan.liang@...ux.intel.com
Cc: acme@...nel.org, namhyung@...nel.org, adrian.hunter@...el.com,
peterz@...radead.org, mingo@...nel.org, linux-kernel@...r.kernel.org,
kernel test robot <oliver.sang@...el.com>
Subject: Re: [PATCH] perf jevents: Don't stop at the first matched pmu when
searching a events table
On Tue, Jul 16, 2024 at 6:29 AM <kan.liang@...ux.intel.com> wrote:
>
> From: Kan Liang <kan.liang@...ux.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@...el.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@...ux.intel.com>
Awesome sauce, thanks!
Reviewed-by: Ian Rogers <irogers@...gle.com>
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
>
Powered by blists - more mailing lists