Re: [PATCH v3 18/18] 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 wait loop
--

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

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

This commit adds a shell test for CoreSight ETM with TRBE sinks that records
a workload with multiple child threads in per-thread mode and checks that
samples from all child threads are present in the decoded trace.

> 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..c6bf1ff2e53d1
> --- /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]
Can cleanup_files() abort before removing tmpdir or killing the remaining
process when the script runs under #!/bin/bash -e?

Later in per-thread-multi.sh:

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

If the background perf record process exits with a non-zero status,
wait $perf reaps the process and triggers set -e exit before unset perf
runs. When the EXIT trap enters cleanup_files(), $perf is still set to the
already-reaped PID, so kill -9 $perf fails with ESRCH and aborts
cleanup_files() under set -e before rm -rf "$tmpdir" runs.

Similarly, if $parent exits early before unset parent runs, kill $parent
fails under set -e and cleanup_files() aborts on kill -9 $parent before
killing $perf or removing "$tmpdir".

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

> +
> +trap cleanup_files EXIT
> +trap 'cleanup_files; exit 1' TERM INT
> +
> +# Multiple --per-thread events are only supported with 1:1 sinks/sources
> +if ! ls /sys/bus/coresight/devices/trbe* > /dev/null 2>&1; then
> +	echo "No TRBE sinks, skipping"
> +	exit 2
> +fi
> +
> +# 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,64K --pid $parent > /dev/null 2>&1 &
> +perf=$!
> +
> +sleep 1
> +
> +kill $parent

[Severity: Medium]
Could starting perf record in the background and immediately running sleep 1
before killing $parent race with perf record initialization on slower or
loaded systems?

In tools/perf/builtin-record.c:__cmd_record(), events are opened disabled in
record__open(), followed by record__synthesize() (including BPF and auxtrace
synthesis), and only enabled afterward via evlist__enable(). If startup and
synthesis take most or all of the 1-second sleep window, kill $parent can
terminate the workload before evlist__enable() or before both threads are
sampled, causing perf script to miss thread1 or thread2.

Also, the /proc/"$parent"/task/* polling loop above has no timeout and breaks
as soon as the task directories exist, which can happen before the child
threads call pthread_setname_np() in tools/perf/tests/workloads/named_threads.c.

Would it be more reliable to source tools/perf/tests/shell/lib/waiting.sh to
use wait_for_threads() and wait_for_perf_to_start(), or pass sleep 1 directly
as the perf record command workload so the 1-second window begins only after
events are enabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-james-cs-unformatted-per-thread-fix-v3-0-dbcb0c281b9b@linaro.org?part=18