Re: [PATCH] perf test: Fix failing test case Session Topology

[Namhyung Kim <namhyung@kernel.org>]

Hello,

On Tue, Jan 28, 2025 at 09:07:22AM -0800, Ian Rogers wrote:
> On Tue, Jan 28, 2025 at 5:07 AM Thomas Richter <tmricht@linux.ibm.com> wrote:
> >
> > Test case 37 Session topology fails on s390. I bisected this to
> >
> > commit 05be17eed774 ("tool api fs: Correctly encode errno for read/write open failures")
> >
> > When perf tool is invoked, it builds the perf.data file using the
> > following function sequence:
> >
> >   #0  has_die_topology () at util/cputopo.c:223
> >   #1  cpu_topology__new () at util/cputopo.c:302
> >   ....
> >   #9  0x00000000012a5c7a in perf_session__write_header (...)
> >         at util/header.c:3774
> >   #10 0x00000000011ccd64 in session_write_header (...)
> >         at tests/topology.c:53
> >   #11 0x00000000011cde9c in test__session_topology (...)
> >         at tests/topology.c:215
> >   #12 0x0000000001190276 in start_test (...)
> >
> > The function has_die_topology() checks for existence of sys file
> > /sys/devices/system/cpu/cpu0/topology/die_cpus_list
> > which does not exist on s390. This function returns false and
> > information for all CPU dies is omitted in the
> > perf data file section HEADER_CPU_TOPOLOGY.
> >
> > Later on the test case reads the perf.data file back using
> > functions.
> >
> >   check_cpu_topology()
> >   +--> perf_session__new()
> >
> > the missing CPU die information in the HEADER_CPU_TOPOLOGY section
> > causes the id_die field set to 0 in the session->header.env.cpu[0].
> >
> > Later on when the test case result is verified, the functions
> >
> >   test__session_topology()
> >   +--> check_cpu_topology()
> >        +--> aggr_cpu_id__cpu()
> >             +--> aggr_cpu_id__core()
> >                  +--> aggr_cpu_id__cluster()
> >                       +--> aggr_cpu_id__die()
> >                            +--> cpu__get_topology_int()
> >                                 +--> sysfs__read_int()
> >
> > Function aggr_cpu_id__die() reads the die_id from the CPU's
> > /sys/devices/system/cpu/cpu0/topology/die_id file entry.
> > This file does not exist and function
> > sysfs__read_int() now returns error number -2 (ENOENT).
> > Structure member id.die has value of -2.
> >
> > This new return value of -2 bypasses this check in function
> > aggr_cpu_id__die():
> >
> >         /* There is no die_id on legacy system. */
> >         if (die == -1)
> >                 die = 0;
> > and the test verification fails in:
> >
> >   TEST_ASSERT_VAL("Cpu map - Die ID doesn't match",
> >         session->header.env.cpu[cpu.cpu].die_id == id.die)
> >
> > Output before:
> >  # ./perf test -F 37
> >  37: Session topology               : FAILED!
> >  #
> >
> > Output after:
> >  # ./perf test -F 37
> >  37: Session topology               : Ok
> >  #
> >
> > Fixes: 05be17eed774 ("tool api fs: Correctly encode errno for read/write open failures")
> > Cc: Ian Rogers <irogers@google.com>
> > Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
> 
> Thanks Thomas, there is also James' patch which I think is equivalent:
> https://lore.kernel.org/linux-perf-users/20241218115552.912517-1-james.clark@linaro.org/

Oh, I thought it's already merged.  Will add it to perf-tools soon.


> and my longer series:
> https://lore.kernel.org/lkml/20241216232459.427642-1-irogers@google.com/
> which is what I'm carrying in Googe's tree, although James suggested
> corrections.

Please send a new verion with updates.

Thanks,
Namhyung