Re: [PATCH v2 13/14] perf test: Add test for per-thread mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The test unconditionally hardcodes execution on CPUs 0, 1, and 2, which will cause spurious test failures on environments with fewer than 3 CPUs, isolated cgroups, or asymmetric architectures lacking CoreSight ETM capabilities on those specific cores.
- [Medium] The test hardcodes assertions against a known incorrect perf decoding behavior, which guarantees a test failure when the underlying `perf` decode bug is fixed.
--

--- Patch [13]: [PATCH v2 13/14] perf test: Add test for per-thread mode ---
commit 5b1cfaf160332a1a985ebf60b00fdda5fa16efd7
Author: James Clark <james.clark@linaro.org>

perf test: Add test for per-thread mode

This commit introduces a new shell script test to verify that per-thread
mode works correctly when a process migrates across CPUs, and that the
decode happens in the correct order.

> diff --git a/tools/perf/tests/shell/coresight/per-thread.sh b/tools/perf/tests/shell/coresight/per-thread.sh
> new file mode 100755
> index 0000000000000..77d1ed3405a9d
> --- /dev/null
> +++ b/tools/perf/tests/shell/coresight/per-thread.sh
> @@ -0,0 +1,48 @@
[ ... ]
> +echo "Recording..."
> +perf record -o "$tmpdir/data" -e cs_etm//u --per-thread -- \
> +	taskset --cpu-list 0 taskset --cpu-list 1 taskset --cpu-list 2 true > /dev/null 2>&1

[Severity: Medium]
Will this unconditionally hardcode execution on CPUs 0, 1, and 2?

Running this on a system with fewer than 3 CPUs (such as typical 2-core CI
environments) might cause the taskset command to fail. Since the script
runs under set -e, could this fail the test spuriously instead of properly
skipping it?

Additionally, on architectures where these specific CPUs exist but lack ETM
capabilities, this could fail to capture trace data.

> +
> +echo "Decoding..."
> +perf script -i "$tmpdir/data" --itrace=b -F comm,cpu 2> /dev/null | \
> +	grep -Eo '(taskset|true).*(\[[0-9]+\])' | \
> +	uniq | tail -n 3 > "$tmpdir/script" 2>/dev/null
> +
> +# Check that the decode says it ran on CPU 0, 1, 2, in that order. TODO: The
> +# correct result should be "taskset [0,1,2], true 2" but we don't decode trace
> +# in order of the Perf events yet, so everything is associated with the last
> +# exec.
> +cat > "$tmpdir/expected" << EOF
> +true [000]
> +true [001]
> +true [002]
> +EOF

[Severity: Medium]
Does asserting this known broken behavior make the test unnecessarily fragile?

The expected output strictly asserts the current incorrect decoding behavior.
When perf is later fixed to properly associate the events with the earlier
taskset commands, the diff will fail, guaranteeing a test failure.

Since the test's goal is to verify per-thread CPU attribution, would it be
better to assert the CPU sequence rather than the broken comm string?

> +
> +if ! diff -q "$tmpdir/script" "$tmpdir/expected"; then
> +	echo "FAIL: per-thread output doesn't match expected:"
> +	cat "$tmpdir/script"
> +	exit 1
> +fi
> +
> +exit 0

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260821-james-cs-unformatted-per-thread-fix-v2-0-00c4fd0701b4@linaro.org?part=13