tools/perf/util/pmus.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
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.
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
> 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 ?
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 ?
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
>
>
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
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
> >
> >
>
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
> > >
> > >
> >
> On 4 Aug 2024, at 8:36 PM, Eric Lin <eric.lin@sifive.com> wrote:
>
> 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.
>
Hi Eric,
The functions there says alias and to skip duplicate alias. I am not sure if that is for events
Namhyung, Ian, Arnaldo
Any comments here ?
Thank
Athira
>
> 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
On Mon, Aug 5, 2024 at 7:24 AM Athira Rajeev
<atrajeev@linux.vnet.ibm.com> wrote:
>
>
>
> > On 4 Aug 2024, at 8:36 PM, Eric Lin <eric.lin@sifive.com> wrote:
> >
> > 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.
> >
> Hi Eric,
>
> The functions there says alias and to skip duplicate alias. I am not sure if that is for events
Fwiw, I'm trying to get rid of the term alias it should mean event.
For some reason the code always referred to events as aliases as
exemplified by:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/pmu.c?h=tmp.perf-tools-next#n55
But it is possible to have an "alias" (different) name for a PMU and
I'm sure for other things too. So the term alias is ambiguous and
these things are events, so let's just call them events to be most
intention revealing.
> Namhyung, Ian, Arnaldo
> Any comments here ?
I'll take a look.
Thanks,
Ian
> Thank
> Athira
>
> >
> > 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
>
>
On Mon, Aug 5, 2024 at 10:02 AM Ian Rogers <irogers@google.com> wrote:
>
> On Mon, Aug 5, 2024 at 7:24 AM Athira Rajeev
> <atrajeev@linux.vnet.ibm.com> wrote:
> >
> >
> >
> > > On 4 Aug 2024, at 8:36 PM, Eric Lin <eric.lin@sifive.com> wrote:
> > >
> > > 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.
> > >
> > Hi Eric,
> >
> > The functions there says alias and to skip duplicate alias. I am not sure if that is for events
>
> Fwiw, I'm trying to get rid of the term alias it should mean event.
> For some reason the code always referred to events as aliases as
> exemplified by:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/pmu.c?h=tmp.perf-tools-next#n55
> But it is possible to have an "alias" (different) name for a PMU and
> I'm sure for other things too. So the term alias is ambiguous and
> these things are events, so let's just call them events to be most
> intention revealing.
>
> > Namhyung, Ian, Arnaldo
> > Any comments here ?
>
> I'll take a look.
The problematic events all come from copy pasting ArchStdEvent. It
feels better to have an invariant that events appear once so I sent
out a series to clean this up:
https://lore.kernel.org/linux-perf-users/20240805194424.597244-1-irogers@google.com/
If you could test and add a tested-by tag that'd be great!
Thanks,
Ian
On Mon, Aug 05, 2024 at 12:48:23PM -0700, Ian Rogers wrote: > On Mon, Aug 5, 2024 at 10:02 AM Ian Rogers <irogers@google.com> wrote: > > On Mon, Aug 5, 2024 at 7:24 AM Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote: > > > > On 4 Aug 2024, at 8:36 PM, Eric Lin <eric.lin@sifive.com> wrote: > > > > On Sun, Jul 21, 2024 at 11:44 PM Eric Lin <eric.lin@sifive.com> wrote: > > > > Kindly ping. > > > > Can I ask, are there any more comments about this patch? Thanks. > > > The functions there says alias and to skip duplicate alias. I am not sure if that is for events > > Fwiw, I'm trying to get rid of the term alias it should mean event. > > For some reason the code always referred to events as aliases as > > exemplified by: > > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/pmu.c?h=tmp.perf-tools-next#n55 > > But it is possible to have an "alias" (different) name for a PMU and > > I'm sure for other things too. So the term alias is ambiguous and > > these things are events, so let's just call them events to be most > > intention revealing. > > > Namhyung, Ian, Arnaldo > > > Any comments here ? > > I'll take a look. > > The problematic events all come from copy pasting ArchStdEvent. It > feels better to have an invariant that events appear once so I sent > out a series to clean this up: > https://lore.kernel.org/linux-perf-users/20240805194424.597244-1-irogers@google.com/ > If you could test and add a tested-by tag that'd be great! I have the series in tmp.perf-tools-next, will collect Tested-by/Reviewed-by if they are provided, before moving it to perf-tools-next. - Arnaldo
On Mon, Aug 05, 2024 at 07:54:33PM +0530, Athira Rajeev wrote:
>
>
> > On 4 Aug 2024, at 8:36 PM, Eric Lin <eric.lin@sifive.com> wrote:
> >
> > 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.
> >
> Hi Eric,
>
> The functions there says alias and to skip duplicate alias. I am not sure if that is for events
>
> Namhyung, Ian, Arnaldo
> Any comments here ?
So I was trying to reproduce the problem here before looking at the
patch, tried a simple:
⬢[acme@toolbox perf-tools-next]$ git diff
diff --git a/tools/perf/pmu-events/arch/x86/rocketlake/cache.json b/tools/perf/pmu-events/arch/x86/rocketlake/cache.json
index 2e93b7835b41442b..167a41b0309b7cfc 100644
--- a/tools/perf/pmu-events/arch/x86/rocketlake/cache.json
+++ b/tools/perf/pmu-events/arch/x86/rocketlake/cache.json
@@ -1,4 +1,13 @@
[
+ {
+ "BriefDescription": "Counts the number of cache lines replaced in L1 data cache.",
+ "Counter": "0,1,2,3",
+ "EventCode": "0x51",
+ "EventName": "L1D.REPLACEMENT",
+ "PublicDescription": "Counts L1D data line replacements including opportunistic replacements, and replacements that require stall-for-replace or block-for-replace.",
+ "SampleAfterValue": "100003",
+ "UMask": "0x1"
+ },
{
"BriefDescription": "Counts the number of cache lines replaced in L1 data cache.",
"Counter": "0,1,2,3",
⬢[acme@toolbox perf-tools-next]$ grep L1D.REPLACEMENT tools/perf/pmu-events/arch/x86/rocketlake/cache.json
"EventName": "L1D.REPLACEMENT",
"EventName": "L1D.REPLACEMENT",
⬢[acme@toolbox perf-tools-next]$
I.e. duplicated that whole event definition:
Did a make clean and a rebuild and:
root@x1:/home/acme/git/pahole# perf list l1d.replacement
List of pre-defined events (to be used in -e or -M):
cache:
l1d.replacement
[Counts the number of cache lines replaced in L1 data cache. Unit: cpu_core]
root@x1:/home/acme/git/pahole# perf list > /dev/null
root@x1:/home/acme/git/pahole#
No crash, can you provide instructions on how to reproduce the problem?
I would like to use the experience to add a 'perf test' to show this
failing and then after the patch it passing that new test.
- Arnaldo
© 2016 - 2025 Red Hat, Inc.