Messages in this thread | ![/](/images/icornerl.gif) | | From | Ian Rogers <> | Date | Fri, 10 May 2024 17:50:29 -0700 | Subject | Re: [PATCH 2/2] perf pmu: Fix num_events calculation |
| |
On Fri, May 10, 2024 at 1:41 PM Ian Rogers <irogers@google.com> wrote: > > On Thu, May 9, 2024 at 11:52 PM Justin He <Justin.He@arm.com> wrote: > > > > Hi, Ian > > > > > -----Original Message----- > > > From: Ian Rogers <irogers@google.com> > > > Sent: Friday, May 10, 2024 2:17 PM > > > To: Justin He <Justin.He@arm.com> > > > Cc: Peter Zijlstra <peterz@infradead.org>; Ingo Molnar <mingo@redhat.com>; > > > Arnaldo Carvalho de Melo <acme@kernel.org>; Namhyung Kim > > > <namhyung@kernel.org>; Mark Rutland <Mark.Rutland@arm.com>; Alexander > > > Shishkin <alexander.shishkin@linux.intel.com>; Jiri Olsa <jolsa@kernel.org>; > > > Adrian Hunter <adrian.hunter@intel.com>; Kan Liang > > > <kan.liang@linux.intel.com>; James Clark <James.Clark@arm.com>; > > > linux-perf-users@vger.kernel.org; linux-kernel@vger.kernel.org > > > Subject: Re: [PATCH 2/2] perf pmu: Fix num_events calculation > > > > > > On Thu, May 9, 2024 at 7:47 PM Jia He <justin.he@arm.com> wrote: > > > > > > > > When pe is NULL in the function perf_pmu__new_alias(), the total > > > > number of events is added to loaded_json_aliases. However, if > > > > pmu->events_table is NULL and cpu_aliases_added is false, the > > > > calculation for the events number in perf_pmu__num_events() is incorrect. > > > > > > > > Then cause the error report after "perf list": > > > > Unexpected event > > > smmuv3_pmcg_3f062/smmuv3_pmcg_3f062/transaction// > > > > > > > > Fix it by adding loaded_json_aliases in the calculation under the > > > > mentioned conditions. > > > > > > > > Test it also with "perf bench internals pmu-scan" and there is no > > > > regression. > > > > > > > > Fixes: e6ff1eed3584 ("perf pmu: Lazily add JSON events") > > > > Signed-off-by: Jia He <justin.he@arm.com> > > > > --- > > > > tools/perf/util/pmu.c | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > > diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c index > > > > a1eef7b2e389..a53224e2ce7e 100644 > > > > --- a/tools/perf/util/pmu.c > > > > +++ b/tools/perf/util/pmu.c > > > > @@ -1639,6 +1639,8 @@ size_t perf_pmu__num_events(struct perf_pmu > > > *pmu) > > > > nr += pmu->loaded_json_aliases; > > > > else if (pmu->events_table) > > > > nr += > > > pmu_events_table__num_events(pmu->events_table, > > > > pmu) - pmu->loaded_json_aliases; > > > > + else > > > > + nr += pmu->loaded_json_aliases; > > > > > > Thanks for working on this! The "struct pmu_event *pe" in new_alias is an entry > > > from the json data, and "pmu->events_table" should NULL if there is no json > > > data. I believe the code is assuming that these lines aren't necessary as it > > > shouldn't be possible to load json data if the json events table doesn't exist for > > > the PMU - if there is no json data then loaded_json_aliases should be 0 and no > > > addition is necessary. I'm wondering why this case isn't true for you. > > On my Armv8a N2 server, "pmu->events_table" is NULL because perf_pmu__find_events_table() > > return NULL. > > > > I also noticed that pmu->loaded_json_aliases is *not* 0. The missing adding calculation will cause > > perf_pmu__num_events() less than normal case and will trigger latter check failure in > > perf_pmus__print_pmu_events__callback(). > > At last, perf list will report many lines similar as: > > Unexpected event smmuv3_pmcg_3f062/smmuv3_pmcg_3f062/transaction// > > The issue is the sys events which currently are ARM only. Here is an > lldb stack trace: > ``` > frame #6: 0x0000aaaaabc9b49c > perf`__assert_fail(assertion="pmu->events_table", > file="tools/perf/util/pmu.c", line=522, function="int > perf_pmu__new_alias(struct perf_pmu *, const char *, const char *, > const char *, FILE *, const struct pmu_event *)") > frame #7: 0x0000aaaaab41e018 > perf`perf_pmu__new_alias(pmu=0x00007377e7e09b40, > name="hnf_brd_snoops_sent", desc="Counts number of multicast snoops > sent (not including SF back invalidation)", val="eventid=9,type=5", > val_fd=0x0000000000000000, pe=0x0000ffffffffde88) at pmu.c:522:3 > frame #8: 0x0000aaaaab4175cc > perf`pmu_add_sys_aliases_iter_fn(pe=0x0000ffffffffde88, > table=0x0000aaaaac299bb0, vdata=0x00007377e7e09b40) at pmu.c:939:3 > frame #9: 0x0000aaaaaaf6d000 > perf`pmu_events_table__for_each_event_pmu(table=0x0000aaaaac299bb0, > pmu=0x0000aaaaac299238, fn=(perf`pmu_add_sys_aliases_iter_fn at > pmu.c:931), data=0x00007377e7e09b40) at pmu-events.c:5994:23 > frame #10: 0x0000aaaaaaf6cd78 > perf`pmu_events_table__for_each_event(table=0x0000aaaaac299bb0, > pmu=0x0000000000000000, fn=(perf`pmu_add_sys_aliases_iter_fn at > pmu.c:931), data=0x00007377e7e09b40) at pmu-events.c:6057:23 > frame #11: 0x0000aaaaaaf6ec44 > perf`pmu_for_each_sys_event(fn=(perf`pmu_add_sys_aliases_iter_fn at > pmu.c:931), data=0x00007377e7e09b40) at pmu-events.c:6295:27 > frame #12: 0x0000aaaaab41738c > perf`pmu_add_sys_aliases(pmu=0x00007377e7e09b40) at pmu.c:955:2 > frame #13: 0x0000aaaaab4179fc > perf`perf_pmu__lookup(pmus=0x0000aaaaac500970, dirfd=34, > name="arm_cmn_0") at pmu.c:1037:2 > frame #14: 0x0000aaaaab4242d0 perf`perf_pmu__find2(dirfd=34, > name="arm_cmn_0") at pmus.c:161:9 > frame #15: 0x0000aaaaab421614 perf`pmu_read_sysfs(core_only=false) > at pmus.c:209:3 > frame #16: 0x0000aaaaab42278c > perf`perf_pmus__scan_skip_duplicates(pmu=0x0000000000000000) at > pmus.c:297:3 > frame #17: 0x0000aaaaab422078 > perf`perf_pmus__print_pmu_events(print_cb=0x0000ffffffffec68, > print_state=0x0000ffffffffecd0) at pmus.c:462:16 > frame #18: 0x0000aaaaab425d9c > perf`print_events(print_cb=0x0000ffffffffec68, > print_state=0x0000ffffffffecd0) at print-events.c:409:2 > frame #19: 0x0000aaaaab093ef0 perf`cmd_list(argc=0, > argv=0x0000fffffffff340) at builtin-list.c:592:3 > frame #20: 0x0000aaaaaaf5b490 > perf`run_builtin(p=0x0000aaaaac4e8220, argc=1, > argv=0x0000fffffffff340) at perf.c:349:11 > frame #21: 0x0000aaaaaaf5a3e0 perf`handle_internal_command(argc=1, > argv=0x0000fffffffff340) at perf.c:402:8 > frame #22: 0x0000aaaaaaf5b1a0 > perf`run_argv(argcp=0x0000fffffffff1c8, argv=0x0000fffffffff1c0) at > perf.c:446:2 > frame #23: 0x0000aaaaaaf59f44 perf`main(argc=1, > argv=0x0000fffffffff340) at perf.c:562:3 > ``` > > As such the fix here is incomplete. There may be both sys json events > (detected by PMU/id name) and CPU json events (detected by CPUID). I'm > looking into a fix.
Patch sent: https://lore.kernel.org/lkml/20240511003601.2666907-1-irogers@google.com/
Thanks, Ian
> Thanks, > Ian > > > -- > > Cheers, > > Justin (Jia He)
| ![\](/images/icornerr.gif) |