Re: [PATCH 2/2] perf test cs-etm: Test thread attribution

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Unused pipe file descriptors are not closed after `fork()`, which creates a deadlock risk that can hang the test suite indefinitely.
- [Medium] The test script `context_switch_thread.sh` hardcodes greps for the exact symbol names `thread1` and `thread2`, but the functions are declared as `static`. Under Link-Time Optimization (LTO) or when local symbols are stripped, these symbols may be renamed or dropped, causing the test to fail.
- [Low] The user-space test tool incorrectly includes the kernel-specific header `<linux/compiler.h>` to use the `noinline` macro.
--

commit 5c37aa232ad92e715b665569c8d8414545369de0
Author: James Clark <james.clark@linaro.org>

perf test cs-etm: Test thread attribution

Add a workload that creates two threads reading and writing to a pipe to
verify processor trace attributes symbols correctly across context switches.

> diff --git a/tools/perf/tests/shell/coresight/context_switch_loop/context_switch_loop.c b/tools/perf/tests/shell/coresight/context_switch_loop/context_switch_loop.c
> new file mode 100644
> index 0000000000000..6b7cd11952a2f
> --- /dev/null
> +++ b/tools/perf/tests/shell/coresight/context_switch_loop/context_switch_loop.c
> @@ -0,0 +1,83 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/compiler.h>

[Severity: Low]
Is it safe to include the kernel-specific header <linux/compiler.h> in this
user-space test tool to use the noinline macro?

If the test is compiled standalone outside of the perf Makefile tree, the
build might fail to find the header. Would using the standard compiler
attribute __attribute__((noinline)) be a more robust approach here?

> +#include <stdio.h>
> +#include <stdlib.h>

[ ... ]

> +static noinline void thread1(int in_fd, int out_fd)

[Severity: Medium]
Could these static declarations cause the test to fail under Link-Time
Optimization (LTO) or if local symbols are stripped?

The companion script context_switch_thread.sh hardcodes greps for the exact
symbol names thread1 and thread2. Since they are declared with internal
linkage, the compiler or linker might suffix the symbols (e.g.,
thread1.lto_priv.0), inline them despite noinline, or omit them from the
symbol table entirely. If omitted, the greps will fail.

Would removing the static keyword ensure they have external linkage and
their symbol names are strictly preserved?

> +{
> +	for (int i = 0; i < loops; i++) {
> +		read_block(in_fd);
> +		work += i * 3;
> +		write_block(out_fd);
> +	}
> +}
> +
> +static noinline void thread2(int in_fd, int out_fd)
> +{

[ ... ]

> +	thread1_pid = fork();
> +	if (thread1_pid < 0) {
> +		perror("Fork error");
> +		return 1;
> +	}
> +
> +	if (!thread1_pid) {
> +		prctl(PR_SET_NAME, "thread1", 0, 0, 0);
> +		thread1(b_to_a[0], a_to_b[1]);
> +		exit(0);
> +	}
> +
> +	prctl(PR_SET_NAME, "thread2", 0, 0, 0);
> +	thread2(a_to_b[0], b_to_a[1]);

[Severity: Medium]
Does this process setup risk deadlocking the automated test suite?

After fork(), both parent and child processes inherit all four file
descriptors for the two pipes, but neither process closes the unused ends
of the pipes (e.g., the child leaves a_to_b[0] open and the parent leaves
a_to_b[1] open).

If one process exits prematurely, for example via exit(1) upon an
unexpected read or write failure, will the surviving process block
indefinitely in read_block()? It will never receive an EOF because it
still holds a write end of the pipe open itself, which could hang the perf
record command waiting on the workload.

> +
> +	if (waitpid(thread1_pid, &status, 0) != thread1_pid || !WIFEXITED(status) ||
> +	    WEXITSTATUS(status))
> +		return 1;
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260526-james-cs-context-tracking-fix-v1-0-ebd602e18287@linaro.org?part=2