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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAP-5=fVbZGnRcRbAc+kQyKn5aKdpWkSzVemERpiV2kncxnQ6pg@mail.gmail.com>
Date: Tue, 28 Jan 2025 09:07:22 -0800
From: Ian Rogers <irogers@...gle.com>
To: Thomas Richter <tmricht@...ux.ibm.com>, James Clark <james.clark@...aro.org>, acme@...nel.org, 
	Namhyung Kim <namhyung@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org, 
	agordeev@...ux.ibm.com, gor@...ux.ibm.com, sumanthk@...ux.ibm.com, 
	hca@...ux.ibm.com
Subject: Re: [PATCH] perf test: Fix failing test case Session Topology

On Tue, Jan 28, 2025 at 5:07 AM Thomas Richter <tmricht@...ux.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@...gle.com>
> Signed-off-by: Thomas Richter <tmricht@...ux.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
>