2023-09-13 20:43:34

by Thomas Richter

[permalink] [raw]
Subject: [PATCH] perf jevent: fix core dump on software events on s390

Running commands such as
# ./perf stat -e cs -- true
Segmentation fault (core dumped)
# ./perf stat -e cpu-clock-- true
Segmentation fault (core dumped)
#

dump core. This should not happen as these events are defined
even when no hardware PMU is available.
Debugging this reveals this call chain:

perf_pmus__find_by_type(type=1)
+--> pmu_read_sysfs(core_only=false)
+--> perf_pmu__find2(dirfd=3, name=0x152a113 "software")
+--> perf_pmu__lookup(pmus=0x14f0568 <other_pmus>, dirfd=3,
lookup_name=0x152a113 "software")
+--> perf_pmu__find_events_table (pmu=0x1532130)

Now the pmu is "software" and it tries to find a proper table
generated by the pmu-event generation process for s390:

# cd pmu-events/
# ./jevents.py s390 all /root/linux/tools/perf/pmu-events/arch |\
grep -E '^const struct pmu_table_entry'
const struct pmu_table_entry pmu_events__cf_z10[] = {
const struct pmu_table_entry pmu_events__cf_z13[] = {
const struct pmu_table_entry pmu_metrics__cf_z13[] = {
const struct pmu_table_entry pmu_events__cf_z14[] = {
const struct pmu_table_entry pmu_metrics__cf_z14[] = {
const struct pmu_table_entry pmu_events__cf_z15[] = {
const struct pmu_table_entry pmu_metrics__cf_z15[] = {
const struct pmu_table_entry pmu_events__cf_z16[] = {
const struct pmu_table_entry pmu_metrics__cf_z16[] = {
const struct pmu_table_entry pmu_events__cf_z196[] = {
const struct pmu_table_entry pmu_events__cf_zec12[] = {
const struct pmu_table_entry pmu_metrics__cf_zec12[] = {
const struct pmu_table_entry pmu_events__test_soc_cpu[] = {
const struct pmu_table_entry pmu_metrics__test_soc_cpu[] = {
const struct pmu_table_entry pmu_events__test_soc_sys[] = {
#

However event "software" is not listed, as can be seen in the
generated const struct pmu_events_map pmu_events_map[].
So in function perf_pmu__find_events_table(), the variable
table is initialized to NULL, but never set to a proper
value. The function scans all generated &pmu_events_map[]
tables, but no table matches, because the tables are
s390 CPU Measurement unit specific:

i = 0;
for (;;) {
const struct pmu_events_map *map = &pmu_events_map[i++];
if (!map->arch)
break;

--> the maps are there because the build generated them

if (!strcmp_cpuid_str(map->cpuid, cpuid)) {
table = &map->event_table;
break;
}
--> Since no matching CPU string the table var remains 0x0
}
free(cpuid);
if (!pmu)
return table;

--> The pmu is "software" so it exists and no return

--> and here perf dies because table is 0x0
for (i = 0; i < table->num_pmus; i++) {
...
}
return NULL;

Fix this and do not access the table variable. Instead return 0x0
which is the same return code when the for-loop was not successful.

Output after:
# ./perf stat -e cs -- true

Performance counter stats for 'true':

0 cs

0.000853105 seconds time elapsed

0.000061000 seconds user
0.000827000 seconds sys

# ./perf stat -e cpu-clock -- true

Performance counter stats for 'true':

0.25 msec cpu-clock # 0.341 CPUs utilized

0.000728383 seconds time elapsed

0.000055000 seconds user
0.000706000 seconds sys

# ./perf stat -e cycles -- true

Performance counter stats for 'true':

<not supported> cycles

0.000767298 seconds time elapsed

0.000055000 seconds user
0.000739000 seconds sys

#

Signed-off-by: Thomas Richter <[email protected]>
---
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 a7e88332276d..72ba4a9239c6 100755
--- a/tools/perf/pmu-events/jevents.py
+++ b/tools/perf/pmu-events/jevents.py
@@ -991,7 +991,7 @@ const struct pmu_events_table *perf_pmu__find_events_table(struct perf_pmu *pmu)
}
}
free(cpuid);
- if (!pmu)
+ if (!pmu || !table)
return table;

for (i = 0; i < table->num_pmus; i++) {
--
2.41.0


2023-09-14 04:02:27

by Ian Rogers

[permalink] [raw]
Subject: Re: [PATCH] perf jevent: fix core dump on software events on s390

On Wed, Sep 13, 2023 at 5:52 AM Thomas Richter <[email protected]> wrote:
>
> Running commands such as
> # ./perf stat -e cs -- true
> Segmentation fault (core dumped)
> # ./perf stat -e cpu-clock-- true
> Segmentation fault (core dumped)
> #
>
> dump core. This should not happen as these events are defined
> even when no hardware PMU is available.
> Debugging this reveals this call chain:
>
> perf_pmus__find_by_type(type=1)
> +--> pmu_read_sysfs(core_only=false)
> +--> perf_pmu__find2(dirfd=3, name=0x152a113 "software")
> +--> perf_pmu__lookup(pmus=0x14f0568 <other_pmus>, dirfd=3,
> lookup_name=0x152a113 "software")
> +--> perf_pmu__find_events_table (pmu=0x1532130)
>
> Now the pmu is "software" and it tries to find a proper table
> generated by the pmu-event generation process for s390:
>
> # cd pmu-events/
> # ./jevents.py s390 all /root/linux/tools/perf/pmu-events/arch |\
> grep -E '^const struct pmu_table_entry'
> const struct pmu_table_entry pmu_events__cf_z10[] = {
> const struct pmu_table_entry pmu_events__cf_z13[] = {
> const struct pmu_table_entry pmu_metrics__cf_z13[] = {
> const struct pmu_table_entry pmu_events__cf_z14[] = {
> const struct pmu_table_entry pmu_metrics__cf_z14[] = {
> const struct pmu_table_entry pmu_events__cf_z15[] = {
> const struct pmu_table_entry pmu_metrics__cf_z15[] = {
> const struct pmu_table_entry pmu_events__cf_z16[] = {
> const struct pmu_table_entry pmu_metrics__cf_z16[] = {
> const struct pmu_table_entry pmu_events__cf_z196[] = {
> const struct pmu_table_entry pmu_events__cf_zec12[] = {
> const struct pmu_table_entry pmu_metrics__cf_zec12[] = {
> const struct pmu_table_entry pmu_events__test_soc_cpu[] = {
> const struct pmu_table_entry pmu_metrics__test_soc_cpu[] = {
> const struct pmu_table_entry pmu_events__test_soc_sys[] = {
> #
>
> However event "software" is not listed, as can be seen in the
> generated const struct pmu_events_map pmu_events_map[].
> So in function perf_pmu__find_events_table(), the variable
> table is initialized to NULL, but never set to a proper
> value. The function scans all generated &pmu_events_map[]
> tables, but no table matches, because the tables are
> s390 CPU Measurement unit specific:
>
> i = 0;
> for (;;) {
> const struct pmu_events_map *map = &pmu_events_map[i++];
> if (!map->arch)
> break;
>
> --> the maps are there because the build generated them
>
> if (!strcmp_cpuid_str(map->cpuid, cpuid)) {
> table = &map->event_table;
> break;
> }
> --> Since no matching CPU string the table var remains 0x0
> }
> free(cpuid);
> if (!pmu)
> return table;
>
> --> The pmu is "software" so it exists and no return
>
> --> and here perf dies because table is 0x0
> for (i = 0; i < table->num_pmus; i++) {
> ...
> }
> return NULL;
>
> Fix this and do not access the table variable. Instead return 0x0
> which is the same return code when the for-loop was not successful.
>
> Output after:
> # ./perf stat -e cs -- true
>
> Performance counter stats for 'true':
>
> 0 cs
>
> 0.000853105 seconds time elapsed
>
> 0.000061000 seconds user
> 0.000827000 seconds sys
>
> # ./perf stat -e cpu-clock -- true
>
> Performance counter stats for 'true':
>
> 0.25 msec cpu-clock # 0.341 CPUs utilized
>
> 0.000728383 seconds time elapsed
>
> 0.000055000 seconds user
> 0.000706000 seconds sys
>
> # ./perf stat -e cycles -- true
>
> Performance counter stats for 'true':
>
> <not supported> cycles
>
> 0.000767298 seconds time elapsed
>
> 0.000055000 seconds user
> 0.000739000 seconds sys
>
> #
>
> Signed-off-by: Thomas Richter <[email protected]>

Reviewed-by: Ian Rogers <[email protected]>

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 a7e88332276d..72ba4a9239c6 100755
> --- a/tools/perf/pmu-events/jevents.py
> +++ b/tools/perf/pmu-events/jevents.py
> @@ -991,7 +991,7 @@ const struct pmu_events_table *perf_pmu__find_events_table(struct perf_pmu *pmu)
> }
> }
> free(cpuid);
> - if (!pmu)
> + if (!pmu || !table)
> return table;
>
> for (i = 0; i < table->num_pmus; i++) {
> --
> 2.41.0
>

2023-09-16 05:11:52

by Ian Rogers

[permalink] [raw]
Subject: Re: [PATCH] perf jevent: fix core dump on software events on s390

On Fri, Sep 15, 2023 at 4:40 PM Namhyung Kim <[email protected]> wrote:
>
> Hello,
>
> On Thu, Sep 14, 2023 at 6:14 AM Ian Rogers <[email protected]> wrote:
> >
> > On Wed, Sep 13, 2023 at 5:52 AM Thomas Richter <[email protected]> wrote:
> > >
> > > Running commands such as
> > > # ./perf stat -e cs -- true
> > > Segmentation fault (core dumped)
> > > # ./perf stat -e cpu-clock-- true
> > > Segmentation fault (core dumped)
> > > #
> > >
> > > dump core. This should not happen as these events are defined
> > > even when no hardware PMU is available.
> > > Debugging this reveals this call chain:
> > >
> > > perf_pmus__find_by_type(type=1)
> > > +--> pmu_read_sysfs(core_only=false)
> > > +--> perf_pmu__find2(dirfd=3, name=0x152a113 "software")
> > > +--> perf_pmu__lookup(pmus=0x14f0568 <other_pmus>, dirfd=3,
> > > lookup_name=0x152a113 "software")
> > > +--> perf_pmu__find_events_table (pmu=0x1532130)
> > >
> > > Now the pmu is "software" and it tries to find a proper table
> > > generated by the pmu-event generation process for s390:
> > >
> > > # cd pmu-events/
> > > # ./jevents.py s390 all /root/linux/tools/perf/pmu-events/arch |\
> > > grep -E '^const struct pmu_table_entry'
> > > const struct pmu_table_entry pmu_events__cf_z10[] = {
> > > const struct pmu_table_entry pmu_events__cf_z13[] = {
> > > const struct pmu_table_entry pmu_metrics__cf_z13[] = {
> > > const struct pmu_table_entry pmu_events__cf_z14[] = {
> > > const struct pmu_table_entry pmu_metrics__cf_z14[] = {
> > > const struct pmu_table_entry pmu_events__cf_z15[] = {
> > > const struct pmu_table_entry pmu_metrics__cf_z15[] = {
> > > const struct pmu_table_entry pmu_events__cf_z16[] = {
> > > const struct pmu_table_entry pmu_metrics__cf_z16[] = {
> > > const struct pmu_table_entry pmu_events__cf_z196[] = {
> > > const struct pmu_table_entry pmu_events__cf_zec12[] = {
> > > const struct pmu_table_entry pmu_metrics__cf_zec12[] = {
> > > const struct pmu_table_entry pmu_events__test_soc_cpu[] = {
> > > const struct pmu_table_entry pmu_metrics__test_soc_cpu[] = {
> > > const struct pmu_table_entry pmu_events__test_soc_sys[] = {
> > > #
> > >
> > > However event "software" is not listed, as can be seen in the
> > > generated const struct pmu_events_map pmu_events_map[].
> > > So in function perf_pmu__find_events_table(), the variable
> > > table is initialized to NULL, but never set to a proper
> > > value. The function scans all generated &pmu_events_map[]
> > > tables, but no table matches, because the tables are
> > > s390 CPU Measurement unit specific:
> > >
> > > i = 0;
> > > for (;;) {
> > > const struct pmu_events_map *map = &pmu_events_map[i++];
> > > if (!map->arch)
> > > break;
> > >
> > > --> the maps are there because the build generated them
> > >
> > > if (!strcmp_cpuid_str(map->cpuid, cpuid)) {
> > > table = &map->event_table;
> > > break;
> > > }
> > > --> Since no matching CPU string the table var remains 0x0
> > > }
> > > free(cpuid);
> > > if (!pmu)
> > > return table;
> > >
> > > --> The pmu is "software" so it exists and no return
> > >
> > > --> and here perf dies because table is 0x0
> > > for (i = 0; i < table->num_pmus; i++) {
> > > ...
> > > }
> > > return NULL;
> > >
> > > Fix this and do not access the table variable. Instead return 0x0
> > > which is the same return code when the for-loop was not successful.
> > >
> > > Output after:
> > > # ./perf stat -e cs -- true
> > >
> > > Performance counter stats for 'true':
> > >
> > > 0 cs
> > >
> > > 0.000853105 seconds time elapsed
> > >
> > > 0.000061000 seconds user
> > > 0.000827000 seconds sys
> > >
> > > # ./perf stat -e cpu-clock -- true
> > >
> > > Performance counter stats for 'true':
> > >
> > > 0.25 msec cpu-clock # 0.341 CPUs utilized
> > >
> > > 0.000728383 seconds time elapsed
> > >
> > > 0.000055000 seconds user
> > > 0.000706000 seconds sys
> > >
> > > # ./perf stat -e cycles -- true
> > >
> > > Performance counter stats for 'true':
> > >
> > > <not supported> cycles
> > >
> > > 0.000767298 seconds time elapsed
> > >
> > > 0.000055000 seconds user
> > > 0.000739000 seconds sys
> > >
> > > #
> > >
> > > Signed-off-by: Thomas Richter <[email protected]>
> >
> > Reviewed-by: Ian Rogers <[email protected]>
>
> I'll add this too, ok?
>
> Fixes: 7c52f10c0d4d8 ("perf pmu: Cache JSON events table")

Looks good, thanks!
Ian

> Thanks,
> Namhyung

2023-09-16 10:02:07

by Namhyung Kim

[permalink] [raw]
Subject: Re: [PATCH] perf jevent: fix core dump on software events on s390

Hello,

On Thu, Sep 14, 2023 at 6:14 AM Ian Rogers <[email protected]> wrote:
>
> On Wed, Sep 13, 2023 at 5:52 AM Thomas Richter <[email protected]> wrote:
> >
> > Running commands such as
> > # ./perf stat -e cs -- true
> > Segmentation fault (core dumped)
> > # ./perf stat -e cpu-clock-- true
> > Segmentation fault (core dumped)
> > #
> >
> > dump core. This should not happen as these events are defined
> > even when no hardware PMU is available.
> > Debugging this reveals this call chain:
> >
> > perf_pmus__find_by_type(type=1)
> > +--> pmu_read_sysfs(core_only=false)
> > +--> perf_pmu__find2(dirfd=3, name=0x152a113 "software")
> > +--> perf_pmu__lookup(pmus=0x14f0568 <other_pmus>, dirfd=3,
> > lookup_name=0x152a113 "software")
> > +--> perf_pmu__find_events_table (pmu=0x1532130)
> >
> > Now the pmu is "software" and it tries to find a proper table
> > generated by the pmu-event generation process for s390:
> >
> > # cd pmu-events/
> > # ./jevents.py s390 all /root/linux/tools/perf/pmu-events/arch |\
> > grep -E '^const struct pmu_table_entry'
> > const struct pmu_table_entry pmu_events__cf_z10[] = {
> > const struct pmu_table_entry pmu_events__cf_z13[] = {
> > const struct pmu_table_entry pmu_metrics__cf_z13[] = {
> > const struct pmu_table_entry pmu_events__cf_z14[] = {
> > const struct pmu_table_entry pmu_metrics__cf_z14[] = {
> > const struct pmu_table_entry pmu_events__cf_z15[] = {
> > const struct pmu_table_entry pmu_metrics__cf_z15[] = {
> > const struct pmu_table_entry pmu_events__cf_z16[] = {
> > const struct pmu_table_entry pmu_metrics__cf_z16[] = {
> > const struct pmu_table_entry pmu_events__cf_z196[] = {
> > const struct pmu_table_entry pmu_events__cf_zec12[] = {
> > const struct pmu_table_entry pmu_metrics__cf_zec12[] = {
> > const struct pmu_table_entry pmu_events__test_soc_cpu[] = {
> > const struct pmu_table_entry pmu_metrics__test_soc_cpu[] = {
> > const struct pmu_table_entry pmu_events__test_soc_sys[] = {
> > #
> >
> > However event "software" is not listed, as can be seen in the
> > generated const struct pmu_events_map pmu_events_map[].
> > So in function perf_pmu__find_events_table(), the variable
> > table is initialized to NULL, but never set to a proper
> > value. The function scans all generated &pmu_events_map[]
> > tables, but no table matches, because the tables are
> > s390 CPU Measurement unit specific:
> >
> > i = 0;
> > for (;;) {
> > const struct pmu_events_map *map = &pmu_events_map[i++];
> > if (!map->arch)
> > break;
> >
> > --> the maps are there because the build generated them
> >
> > if (!strcmp_cpuid_str(map->cpuid, cpuid)) {
> > table = &map->event_table;
> > break;
> > }
> > --> Since no matching CPU string the table var remains 0x0
> > }
> > free(cpuid);
> > if (!pmu)
> > return table;
> >
> > --> The pmu is "software" so it exists and no return
> >
> > --> and here perf dies because table is 0x0
> > for (i = 0; i < table->num_pmus; i++) {
> > ...
> > }
> > return NULL;
> >
> > Fix this and do not access the table variable. Instead return 0x0
> > which is the same return code when the for-loop was not successful.
> >
> > Output after:
> > # ./perf stat -e cs -- true
> >
> > Performance counter stats for 'true':
> >
> > 0 cs
> >
> > 0.000853105 seconds time elapsed
> >
> > 0.000061000 seconds user
> > 0.000827000 seconds sys
> >
> > # ./perf stat -e cpu-clock -- true
> >
> > Performance counter stats for 'true':
> >
> > 0.25 msec cpu-clock # 0.341 CPUs utilized
> >
> > 0.000728383 seconds time elapsed
> >
> > 0.000055000 seconds user
> > 0.000706000 seconds sys
> >
> > # ./perf stat -e cycles -- true
> >
> > Performance counter stats for 'true':
> >
> > <not supported> cycles
> >
> > 0.000767298 seconds time elapsed
> >
> > 0.000055000 seconds user
> > 0.000739000 seconds sys
> >
> > #
> >
> > Signed-off-by: Thomas Richter <[email protected]>
>
> Reviewed-by: Ian Rogers <[email protected]>

I'll add this too, ok?

Fixes: 7c52f10c0d4d8 ("perf pmu: Cache JSON events table")

Thanks,
Namhyung

2023-09-17 08:06:49

by Namhyung Kim

[permalink] [raw]
Subject: Re: [PATCH] perf jevent: fix core dump on software events on s390

On Fri, Sep 15, 2023 at 9:11 PM Ian Rogers <[email protected]> wrote:
>
> On Fri, Sep 15, 2023 at 4:40 PM Namhyung Kim <[email protected]> wrote:
> >
> > Hello,
> >
> > On Thu, Sep 14, 2023 at 6:14 AM Ian Rogers <[email protected]> wrote:
> > >
> > > On Wed, Sep 13, 2023 at 5:52 AM Thomas Richter <[email protected]> wrote:
> > > >
> > > > Running commands such as
> > > > # ./perf stat -e cs -- true
> > > > Segmentation fault (core dumped)
> > > > # ./perf stat -e cpu-clock-- true
> > > > Segmentation fault (core dumped)
> > > > #
> > > >
> > > > dump core. This should not happen as these events are defined
> > > > even when no hardware PMU is available.
> > > > Debugging this reveals this call chain:
> > > >
> > > > perf_pmus__find_by_type(type=1)
> > > > +--> pmu_read_sysfs(core_only=false)
> > > > +--> perf_pmu__find2(dirfd=3, name=0x152a113 "software")
> > > > +--> perf_pmu__lookup(pmus=0x14f0568 <other_pmus>, dirfd=3,
> > > > lookup_name=0x152a113 "software")
> > > > +--> perf_pmu__find_events_table (pmu=0x1532130)
> > > >
> > > > Now the pmu is "software" and it tries to find a proper table
> > > > generated by the pmu-event generation process for s390:
> > > >
> > > > # cd pmu-events/
> > > > # ./jevents.py s390 all /root/linux/tools/perf/pmu-events/arch |\
> > > > grep -E '^const struct pmu_table_entry'
> > > > const struct pmu_table_entry pmu_events__cf_z10[] = {
> > > > const struct pmu_table_entry pmu_events__cf_z13[] = {
> > > > const struct pmu_table_entry pmu_metrics__cf_z13[] = {
> > > > const struct pmu_table_entry pmu_events__cf_z14[] = {
> > > > const struct pmu_table_entry pmu_metrics__cf_z14[] = {
> > > > const struct pmu_table_entry pmu_events__cf_z15[] = {
> > > > const struct pmu_table_entry pmu_metrics__cf_z15[] = {
> > > > const struct pmu_table_entry pmu_events__cf_z16[] = {
> > > > const struct pmu_table_entry pmu_metrics__cf_z16[] = {
> > > > const struct pmu_table_entry pmu_events__cf_z196[] = {
> > > > const struct pmu_table_entry pmu_events__cf_zec12[] = {
> > > > const struct pmu_table_entry pmu_metrics__cf_zec12[] = {
> > > > const struct pmu_table_entry pmu_events__test_soc_cpu[] = {
> > > > const struct pmu_table_entry pmu_metrics__test_soc_cpu[] = {
> > > > const struct pmu_table_entry pmu_events__test_soc_sys[] = {
> > > > #
> > > >
> > > > However event "software" is not listed, as can be seen in the
> > > > generated const struct pmu_events_map pmu_events_map[].
> > > > So in function perf_pmu__find_events_table(), the variable
> > > > table is initialized to NULL, but never set to a proper
> > > > value. The function scans all generated &pmu_events_map[]
> > > > tables, but no table matches, because the tables are
> > > > s390 CPU Measurement unit specific:
> > > >
> > > > i = 0;
> > > > for (;;) {
> > > > const struct pmu_events_map *map = &pmu_events_map[i++];
> > > > if (!map->arch)
> > > > break;
> > > >
> > > > --> the maps are there because the build generated them
> > > >
> > > > if (!strcmp_cpuid_str(map->cpuid, cpuid)) {
> > > > table = &map->event_table;
> > > > break;
> > > > }
> > > > --> Since no matching CPU string the table var remains 0x0
> > > > }
> > > > free(cpuid);
> > > > if (!pmu)
> > > > return table;
> > > >
> > > > --> The pmu is "software" so it exists and no return
> > > >
> > > > --> and here perf dies because table is 0x0
> > > > for (i = 0; i < table->num_pmus; i++) {
> > > > ...
> > > > }
> > > > return NULL;
> > > >
> > > > Fix this and do not access the table variable. Instead return 0x0
> > > > which is the same return code when the for-loop was not successful.
> > > >
> > > > Output after:
> > > > # ./perf stat -e cs -- true
> > > >
> > > > Performance counter stats for 'true':
> > > >
> > > > 0 cs
> > > >
> > > > 0.000853105 seconds time elapsed
> > > >
> > > > 0.000061000 seconds user
> > > > 0.000827000 seconds sys
> > > >
> > > > # ./perf stat -e cpu-clock -- true
> > > >
> > > > Performance counter stats for 'true':
> > > >
> > > > 0.25 msec cpu-clock # 0.341 CPUs utilized
> > > >
> > > > 0.000728383 seconds time elapsed
> > > >
> > > > 0.000055000 seconds user
> > > > 0.000706000 seconds sys
> > > >
> > > > # ./perf stat -e cycles -- true
> > > >
> > > > Performance counter stats for 'true':
> > > >
> > > > <not supported> cycles
> > > >
> > > > 0.000767298 seconds time elapsed
> > > >
> > > > 0.000055000 seconds user
> > > > 0.000739000 seconds sys
> > > >
> > > > #
> > > >
> > > > Signed-off-by: Thomas Richter <[email protected]>
> > >
> > > Reviewed-by: Ian Rogers <[email protected]>
> >
> > I'll add this too, ok?
> >
> > Fixes: 7c52f10c0d4d8 ("perf pmu: Cache JSON events table")
>
> Looks good, thanks!
> Ian

Applied to perf-tools, thanks!

Namhyung

2023-09-18 09:42:20

by Thomas Richter

[permalink] [raw]
Subject: Re: [PATCH] perf jevent: fix core dump on software events on s390

On 9/16/23 01:40, Namhyung Kim wrote:
> Hello,
>
> On Thu, Sep 14, 2023 at 6:14 AM Ian Rogers <[email protected]> wrote:
>>
>> On Wed, Sep 13, 2023 at 5:52 AM Thomas Richter <[email protected]> wrote:
>>>
>>> Running commands such as
>>> # ./perf stat -e cs -- true
>>> Segmentation fault (core dumped)
>>> # ./perf stat -e cpu-clock-- true
>>> Segmentation fault (core dumped)
>>> #
>>>
>>> dump core. This should not happen as these events are defined
>>> even when no hardware PMU is available.
>>> Debugging this reveals this call chain:
>>>
>>> perf_pmus__find_by_type(type=1)
>>> +--> pmu_read_sysfs(core_only=false)
>>> +--> perf_pmu__find2(dirfd=3, name=0x152a113 "software")
>>> +--> perf_pmu__lookup(pmus=0x14f0568 <other_pmus>, dirfd=3,
>>> lookup_name=0x152a113 "software")
>>> +--> perf_pmu__find_events_table (pmu=0x1532130)
>>>
>>> Now the pmu is "software" and it tries to find a proper table
>>> generated by the pmu-event generation process for s390:
>>>
>>> # cd pmu-events/
>>> # ./jevents.py s390 all /root/linux/tools/perf/pmu-events/arch |\
>>> grep -E '^const struct pmu_table_entry'
>>> const struct pmu_table_entry pmu_events__cf_z10[] = {
>>> const struct pmu_table_entry pmu_events__cf_z13[] = {
>>> const struct pmu_table_entry pmu_metrics__cf_z13[] = {
>>> const struct pmu_table_entry pmu_events__cf_z14[] = {
>>> const struct pmu_table_entry pmu_metrics__cf_z14[] = {
>>> const struct pmu_table_entry pmu_events__cf_z15[] = {
>>> const struct pmu_table_entry pmu_metrics__cf_z15[] = {
>>> const struct pmu_table_entry pmu_events__cf_z16[] = {
>>> const struct pmu_table_entry pmu_metrics__cf_z16[] = {
>>> const struct pmu_table_entry pmu_events__cf_z196[] = {
>>> const struct pmu_table_entry pmu_events__cf_zec12[] = {
>>> const struct pmu_table_entry pmu_metrics__cf_zec12[] = {
>>> const struct pmu_table_entry pmu_events__test_soc_cpu[] = {
>>> const struct pmu_table_entry pmu_metrics__test_soc_cpu[] = {
>>> const struct pmu_table_entry pmu_events__test_soc_sys[] = {
>>> #
>>>
>>> However event "software" is not listed, as can be seen in the
>>> generated const struct pmu_events_map pmu_events_map[].
>>> So in function perf_pmu__find_events_table(), the variable
>>> table is initialized to NULL, but never set to a proper
>>> value. The function scans all generated &pmu_events_map[]
>>> tables, but no table matches, because the tables are
>>> s390 CPU Measurement unit specific:
>>>
>>> i = 0;
>>> for (;;) {
>>> const struct pmu_events_map *map = &pmu_events_map[i++];
>>> if (!map->arch)
>>> break;
>>>
>>> --> the maps are there because the build generated them
>>>
>>> if (!strcmp_cpuid_str(map->cpuid, cpuid)) {
>>> table = &map->event_table;
>>> break;
>>> }
>>> --> Since no matching CPU string the table var remains 0x0
>>> }
>>> free(cpuid);
>>> if (!pmu)
>>> return table;
>>>
>>> --> The pmu is "software" so it exists and no return
>>>
>>> --> and here perf dies because table is 0x0
>>> for (i = 0; i < table->num_pmus; i++) {
>>> ...
>>> }
>>> return NULL;
>>>
>>> Fix this and do not access the table variable. Instead return 0x0
>>> which is the same return code when the for-loop was not successful.
>>>
>>> Output after:
>>> # ./perf stat -e cs -- true
>>>
>>> Performance counter stats for 'true':
>>>
>>> 0 cs
>>>
>>> 0.000853105 seconds time elapsed
>>>
>>> 0.000061000 seconds user
>>> 0.000827000 seconds sys
>>>
>>> # ./perf stat -e cpu-clock -- true
>>>
>>> Performance counter stats for 'true':
>>>
>>> 0.25 msec cpu-clock # 0.341 CPUs utilized
>>>
>>> 0.000728383 seconds time elapsed
>>>
>>> 0.000055000 seconds user
>>> 0.000706000 seconds sys
>>>
>>> # ./perf stat -e cycles -- true
>>>
>>> Performance counter stats for 'true':
>>>
>>> <not supported> cycles
>>>
>>> 0.000767298 seconds time elapsed
>>>
>>> 0.000055000 seconds user
>>> 0.000739000 seconds sys
>>>
>>> #
>>>
>>> Signed-off-by: Thomas Richter <[email protected]>
>>
>> Reviewed-by: Ian Rogers <[email protected]>
>
> I'll add this too, ok?
>
> Fixes: 7c52f10c0d4d8 ("perf pmu: Cache JSON events table")
>
> Thanks,
> Namhyung

Yep fine with me.
--
Thomas Richter, Dept 3303, IBM s390 Linux Development, Boeblingen, Germany
--
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294