Re: [PATCH v4 18/19] perf test: cs-etm: Test multiple per-thread threads

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] perf test: cs-etm: unguarded kill in cleanup_files() aborts under set -e
- [Medium] perf test: cs-etm: unsynchronized perf record startup and thread wait loop
--

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

perf test: cs-etm: Test multiple per-thread threads

This commit adds a shell test for CoreSight per-thread tracing across
multiple child threads on systems with TRBE sinks. It spawns the
named_threads workload, attaches perf record in --per-thread mode, and
checks that perf script output includes samples from all child threads.

> diff --git a/tools/perf/tests/shell/coresight/per-thread-multi.sh b/tools/perf/tests/shell/coresight/per-thread-multi.sh
> new file mode 100755
> index 0000000000000..bace76cb6a510
> --- /dev/null
> +++ b/tools/perf/tests/shell/coresight/per-thread-multi.sh
> @@ -0,0 +1,76 @@
> +#!/bin/bash -e
> +# CoreSight per-thread multiple threads (exclusive)
> +
> +# SPDX-License-Identifier: GPL-2.0
> +
> +# If CoreSight is not available, skip the test
> +perf list pmu | grep -q cs_etm || exit 2
> +
> +if ! tmpdir=$(mktemp -d /tmp/perf-cs-per-thread-multi-test.XXXXXX); then
> +	echo "mktemp failed"
> +	exit 1
> +fi
> +
> +cleanup_files()
> +{
> +	if [[ $parent ]]; then
> +		kill -9 $parent
> +		wait $parent || true
> +	fi
> +	if [[ $perf ]]; then
> +		kill -9 $perf
> +		wait $perf || true
> +	fi
> +	rm -rf "$tmpdir"
> +	trap - EXIT TERM INT
> +}

[Severity: Medium]
As reported by Sashiko on v3 [1], because the script runs under
#!/bin/bash -e, errexit remains active inside cleanup_files(). Later in
the script:

    kill $parent
    wait $parent || true
    unset parent
    wait $perf
    unset perf

If the background perf record process fails and exits with a non-zero
status, wait $perf reaps the process and triggers an immediate shell exit
before unset perf runs, invoking cleanup_files() via the EXIT trap while
$perf is still set. Inside cleanup_files(), kill -9 $perf fails with ESRCH
because $perf has already exited and been reaped, aborting cleanup_files()
before rm -rf "$tmpdir" can remove /tmp/perf-cs-per-thread-multi-test.XXXXXX.

Similarly, if $parent exits unexpectedly before unset parent runs, kill
$parent fails and cleanup_files() aborts at kill -9 $parent before killing
$perf or removing $tmpdir.

Should kill -9 $parent and kill -9 $perf in cleanup_files() be guarded with
2>/dev/null || true?

[ ... ]

> +# Launch 2 threads to run indefinitely
> +nthreads=2
> +perf test -w named_threads $nthreads 0 &
> +parent=$!
> +
> +# While parent still exists, wait for the 2 children to spawn
> +while kill -0 "$parent" 2>/dev/null; do
> +  threads=(/proc/"$parent"/task/*)
> +
> +  if (( ${#threads[@]} >= $(($nthreads + 1)))); then
> +    break
> +  fi
> +
> +  sleep 0.1
> +done
> +
> +echo "Recording..."
> +perf record -o "$tmpdir/data" -e cs_etm//u --per-thread -Se -m,128K --pid $parent > /dev/null 2>&1 &
> +perf=$!
> +
> +sleep 1
> +
> +kill $parent

[Severity: Medium]
As reported by Sashiko on v3 [2], could this startup and thread wait sequence
cause flaky failures or hangs on loaded systems?

When perf record --pid $parent is started in the background, sleep 1 runs
immediately in parallel with perf record initialization. In
tools/perf/builtin-record.c, __cmd_record() opens cs_etm//u and dummy:u in
the disabled state in record__open(), performs synthesis in
record__synthesize() (including CoreSight sysfs reads, BPF synthesis, and
/proc task and mmap synthesis), and only enables the events at
evlist__enable(). If initialization takes most of or longer than the 1
second sleep window, kill $parent terminates the workload before
evlist__enable() or before both threads are sampled, causing the grep check
on "$tmpdir/script" to fail.

Additionally, the /proc/"$parent"/task/* polling loop has no timeout if the
children fail to spawn, and breaks as soon as clone() creates the task
directories, before the child threads have necessarily called
pthread_setname_np() in tools/perf/tests/workloads/named_threads.c. If
pthread_setname_np() runs after __machine__synthesize_threads() reads
/proc/$parent/task/$tid/status but before evlist__enable() enables dummy:u,
the PERF_RECORD_COMM event can be missed.

Would it be more reliable to use wait_for_threads() and
wait_for_perf_to_start() from tools/perf/tests/shell/lib/waiting.sh, or pass
sleep 1 as the perf record child workload so evlist__start_workload() runs
it only after evlist__enable()?

[1] https://lore.kernel.org/r/20261102-james-cs-unformatted-per-thread-fix-v3-18-dbcb0c281b9b@linaro.org
[2] https://lore.kernel.org/r/20261102-james-cs-unformatted-per-thread-fix-v3-18-dbcb0c281b9b@linaro.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-james-cs-unformatted-per-thread-fix-v4-0-7759e0ce2449@linaro.org?part=18