Re: [PATCH v2] perf test: Fixes for check branch stack sampling

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [Medium] Outdated comments and output messages in `test_kernel_branches` explicitly contradict the new test logic and the commit message.
- [Medium] Missing `perf record` output redirection in `test_trap_eret_branches`.
--

commit 123631eaa14de0c5e9e90fa3e9a1064ed9d669af
Author: Ian Rogers <irogers@google.com>

perf test: Fixes for check branch stack sampling

This commit updates branch stack sampling tests to handle user/kernel
boundary crossings. It also increases the duration of the system call
sampling test for reliability, and redirects unneeded perf record output
to a temporary file.

> diff --git a/tools/perf/tests/shell/test_brstack.sh b/tools/perf/tests/shell/test_brstack.sh
> --- a/tools/perf/tests/shell/test_brstack.sh
> +++ b/tools/perf/tests/shell/test_brstack.sh

[ ... ]

>  test_trap_eret_branches() {
>  	echo "Testing trap & eret branches"
> +
>  	if ! is_arm64; then
> -		echo "skip: not arm64"
> +		echo "Testing trap & eret branches [Skipped not arm64]"
> +		return
> +	fi
> +	start_err=$err
> +	err=0
> +	perf record -o $TMPDIR/perf.data --branch-filter any,save_type,u,k -- \
> +		perf test -w traploop 1000

The commit message states that unneeded perf record output is put into a
temporary file to avoid cluttering the test output. Was the redirection
intentionally omitted for this specific test?

> +	perf script -i $TMPDIR/perf.data --fields brstacksym | \
> +		tr ' ' '\n' > $TMPDIR/perf.script

[ ... ]

>  test_kernel_branches() {
>  	echo "Testing that k option only includes kernel source addresses"

Does this output message still reflect the test's intent? The commit message
states that the test now allows the source to be in user land, but this
message says it "only includes kernel source addresses".

>  
> -	if ! perf record --branch-filter any,k -o- -- true > /dev/null; then
> -		echo "skip: not enough privileges"
> +	if ! perf record --branch-filter any,k -o- -- true > "$TMPDIR/record.txt" 2>&1; then
> +		echo "Testing that k option [Skipped not enough privileges]"
> +		return
> +	fi
> +	start_err=$err
> +	err=0
> +	perf record -o $TMPDIR/perf.data --branch-filter any,k -- \
> +		perf bench syscall basic --loop 1000 > "$TMPDIR/record.txt" 2>&1
> +	perf script -i $TMPDIR/perf.data --fields brstack | \
> +		tr ' ' '\n' > $TMPDIR/perf.script
> +
> +	# Example of branch entries:
> +	#       "0xffffffff93bda241/0xffffffff93bda20f/M/-/-/..."
> +	# Source addresses come first and target address can be either
> +	# userspace or kernel even with k option, as long as the source
> +	# is in kernel.

This isn't a bug, but this comment appears to contradict the new test logic.
The new test logic below correctly checks that the target address is in the
kernel rather than the source address. Should this comment be updated?

> +
> +	# Look for source addresses with top bit set
> +	if ! grep -q -E -m1 "^0x[89a-f][0-9a-f]{15}" $TMPDIR/perf.script; then
> +		echo "Testing that k option [Failed kernel branches missing]"
> +		err=1
> +	fi
> +	# Look for no target addresses without top bit set
> +	if grep -q -E -m1 "^0x[0-9a-f]{0,16}/0x[0-7][0-9a-f]{0,15}/" $TMPDIR/perf.script; then
> +		echo "Testing that k option [Failed user branches found with kernel filter]"
> +		err=1
> +	fi

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260408062048.61252-1-irogers@google.com?part=1