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

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

[Thomas Richter]

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>
---
 tools/perf/util/cpumap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
index 27094211edd8..05ced29385f7 100644
--- a/tools/perf/util/cpumap.c
+++ b/tools/perf/util/cpumap.c
@@ -293,7 +293,7 @@ struct aggr_cpu_id aggr_cpu_id__die(struct perf_cpu cpu, void *data)
 
 	die = cpu__get_die_id(cpu);
 	/* There is no die_id on legacy system. */
-	if (die == -1)
+	if (die <= -1)
 		die = 0;
 
 	/*
-- 
2.48.1

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

[Ian Rogers]

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/
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.

Thanks,
Ian

> ---
>  tools/perf/util/cpumap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
> index 27094211edd8..05ced29385f7 100644
> --- a/tools/perf/util/cpumap.c
> +++ b/tools/perf/util/cpumap.c
> @@ -293,7 +293,7 @@ struct aggr_cpu_id aggr_cpu_id__die(struct perf_cpu cpu, void *data)
>
>         die = cpu__get_die_id(cpu);
>         /* There is no die_id on legacy system. */
> -       if (die == -1)
> +       if (die <= -1)
>                 die = 0;
>
>         /*
> --
> 2.48.1
>

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

[Namhyung Kim]

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