Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
MIME-Version: 1.0
References: <20230913125157.2790375-1-tmricht@linux.ibm.com>
 <CAP-5=fUiHMRPVYhbQv-YM+EMKyBF6TEopea=PPX2thbtdmhGsg@mail.gmail.com> <CAM9d7cgbMZzJea_TJJ46AEhLentykmy8r6twC8bkkgXTNAMqgw@mail.gmail.com>
In-Reply-To: <CAM9d7cgbMZzJea_TJJ46AEhLentykmy8r6twC8bkkgXTNAMqgw@mail.gmail.com>
From:   Ian Rogers <irogers@google.com>
Date:   Fri, 15 Sep 2023 21:10:50 -0700
Message-ID: <CAP-5=fUXgC7QPk+NqeKFds54do70iePaXC5F8nLC6oWPLC50yg@mail.gmail.com>
Subject: Re: [PATCH] perf jevent: fix core dump on software events on s390
To:     Namhyung Kim <namhyung@gmail.com>
Cc:     Thomas Richter <tmricht@linux.ibm.com>,
        linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org,
        acme@kernel.org, sumanthk@linux.ibm.com, dengler@linux.ibm.com,
        svens@linux.ibm.com, gor@linux.ibm.com, hca@linux.ibm.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Sep 15, 2023 at 4:40=E2=80=AFPM Namhyung Kim <namhyung@gmail.com> w=
rote:
>
> Hello,
>
> On Thu, Sep 14, 2023 at 6:14=E2=80=AFAM Ian Rogers <irogers@google.com> w=
rote:
> >
> > On Wed, Sep 13, 2023 at 5:52=E2=80=AFAM Thomas Richter <tmricht@linux.i=
bm.com> 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=3D1)
> > >   +--> pmu_read_sysfs(core_only=3Dfalse)
> > >        +--> perf_pmu__find2(dirfd=3D3, name=3D0x152a113 "software")
> > >             +--> perf_pmu__lookup(pmus=3D0x14f0568 <other_pmus>, dirf=
d=3D3,
> > >                                   lookup_name=3D0x152a113 "software")
> > >                  +--> perf_pmu__find_events_table (pmu=3D0x1532130)
> > >
> > > 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[] =3D {
> > >  const struct pmu_table_entry pmu_events__cf_z13[] =3D {
> > >  const struct pmu_table_entry pmu_metrics__cf_z13[] =3D {
> > >  const struct pmu_table_entry pmu_events__cf_z14[] =3D {
> > >  const struct pmu_table_entry pmu_metrics__cf_z14[] =3D {
> > >  const struct pmu_table_entry pmu_events__cf_z15[] =3D {
> > >  const struct pmu_table_entry pmu_metrics__cf_z15[] =3D {
> > >  const struct pmu_table_entry pmu_events__cf_z16[] =3D {
> > >  const struct pmu_table_entry pmu_metrics__cf_z16[] =3D {
> > >  const struct pmu_table_entry pmu_events__cf_z196[] =3D {
> > >  const struct pmu_table_entry pmu_events__cf_zec12[] =3D {
> > >  const struct pmu_table_entry pmu_metrics__cf_zec12[] =3D {
> > >  const struct pmu_table_entry pmu_events__test_soc_cpu[] =3D {
> > >  const struct pmu_table_entry pmu_metrics__test_soc_cpu[] =3D {
> > >  const struct pmu_table_entry pmu_events__test_soc_sys[] =3D {
> > >  #
> > >
> > > 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 =3D 0;
> > >   for (;;) {
> > >       const struct pmu_events_map *map =3D &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 =3D &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 =3D 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 <tmricht@linux.ibm.com>
> >
> > Reviewed-by: Ian Rogers <irogers@google.com>
>
> I'll add this too, ok?
>
> Fixes: 7c52f10c0d4d8 ("perf pmu: Cache JSON events table")

Looks good, thanks!
Ian

> Thanks,
> Namhyung