Messages in this thread |  | | From | Eric Lin <> | | Date | Sun, 4 Aug 2024 23:06:36 +0800 | | Subject | Re: [PATCH] perf pmus: Fix duplicate events caused segfault |
| |
Hi,
On Sun, Jul 21, 2024 at 11:44 PM Eric Lin <eric.lin@sifive.com> wrote: > > Hi Athira, > > On Sat, Jul 20, 2024 at 4:35 PM Athira Rajeev > <atrajeev@linux.vnet.ibm.com> wrote: > > > > > > > > > On 19 Jul 2024, at 1:46 PM, Eric Lin <eric.lin@sifive.com> wrote: > > > > > > Currently, if vendor JSON files have two duplicate event names, > > > the "perf list" command will trigger a segfault. > > > > > > In commit e6ff1eed3584 ("perf pmu: Lazily add JSON events"), > > > pmu_events_table__num_events() gets the number of JSON events > > > from table_pmu->num_entries, which includes duplicate events > > > if there are duplicate event names in the JSON files. > > > > Hi Eric, > > > > Let us consider there are duplicate event names in the JSON files, say : > > > > metric.json with: EventName as pmu_cache_miss, EventCode as 0x1 > > cache.json with: EventName as pmu_cache_miss, EventCode as 0x2 > > > > If we fix the segfault and proceed, still “perf list” will list only one entry for pmu_cache_miss with may be 0x1/0x2 as event code ? > > Can you check the result to confirm what “perf list” will list in this case ? If it’s going to have only one entry in perf list, does it mean there are two event codes for pmu_cache_miss and it can work with either of the event code ? > > > > Sorry for the late reply. > Yes, I've checked if there are duplicate pmu_cache_miss events in the > JSON files, the perf list will have only one entry in perf list. > > > If it happens to be a mistake in json file to have duplicate entry with different event code (ex: with some broken commit), I am thinking if the better fix is to keep only the valid entry in json file ? > > > > Yes, I agree we should fix the duplicate events in vendor JSON files. > > According to this code snippet [1], it seems the perf tool originally > allowed duplicate events to exist and it will skip the duplicate > events not shown on the perf list. > However, after this commit e6ff1eed3584 ("perf pmu: Lazily add JSON > events"), if there are two duplicate events, it causes a segfault. > > Can I ask, do you have any suggestions? Thanks. > > [1] https://github.com/torvalds/linux/blob/master/tools/perf/util/pmus.c#L491 >
Kindly ping.
Can I ask, are there any more comments about this patch? Thanks.
Regards, Eric Lin
> Regards, > Eric Lin > > > Thanks > > Athira > > > > > > > > perf_pmu__for_each_event() adds JSON events to the pmu->alias > > > list and copies sevent data to the aliases array. However, the > > > pmu->alias list does not contain duplicate events, as > > > perf_pmu__new_alias() checks if the name was already created. > > > > > > When sorting the alias data, if there are two duplicate events, > > > it causes a segfault in cmp_sevent() due to invalid aliases in > > > the aliases array. > > > > > > To avoid such segfault caused by duplicate event names in the > > > JSON files, the len should be updated before sorting the aliases. > > > > > > Fixes: e6ff1eed3584 ("perf pmu: Lazily add JSON events") > > > Signed-off-by: Eric Lin <eric.lin@sifive.com> > > > --- > > > tools/perf/util/pmus.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c > > > index b9b4c5eb5002..e38c3fd4d1ff 100644 > > > --- a/tools/perf/util/pmus.c > > > +++ b/tools/perf/util/pmus.c > > > @@ -443,7 +443,7 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p > > > { > > > struct perf_pmu *pmu; > > > int printed = 0; > > > - int len; > > > + size_t len, j; > > > struct sevent *aliases; > > > struct events_callback_state state; > > > bool skip_duplicate_pmus = print_cb->skip_duplicate_pmus(print_state); > > > @@ -474,8 +474,9 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p > > > perf_pmu__for_each_event(pmu, skip_duplicate_pmus, &state, > > > perf_pmus__print_pmu_events__callback); > > > } > > > + len = state.index; > > > qsort(aliases, len, sizeof(struct sevent), cmp_sevent); > > > - for (int j = 0; j < len; j++) { > > > + for (j = 0; j < len; j++) { > > > /* Skip duplicates */ > > > if (j > 0 && pmu_alias_is_duplicate(&aliases[j], &aliases[j - 1])) > > > continue; > > > -- > > > 2.43.2 > > > > > > > >
|  |