Re: [PATCH v4 09/13] perf tests: Fix flakiness in branch stack sampling tests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The newly introduced retry loop logic causes false-positive log pollution for some tests on intermediate failures, while completely stripping necessary failure diagnostics from others.
--

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

perf tests: Fix flakiness in branch stack sampling tests

This commit increases the workload loop counts to 100,000 across three test
sections to guarantee robust branch stack capture during brief benchmark
execution. The iterations were introduced in a retry loop to prevent false
negatives.

> diff --git a/tools/perf/tests/shell/test_brstack.sh b/tools/perf/tests/shell/test_brstack.sh
> index eb5837f82e390..71550e0b37baa 100755
> --- a/tools/perf/tests/shell/test_brstack.sh
> +++ b/tools/perf/tests/shell/test_brstack.sh
> @@ -110,20 +110,29 @@ test_trap_eret_branches() {

[ ... ]

> -	err=0
> -	perf record -o $TMPDIR/perf.data --branch-filter any,save_type,u,k -- \
> -		perf test -w traploop 1000 > "$TMPDIR/record.txt" 2>&1
> -	perf script -i $TMPDIR/perf.data --fields brstacksym | \
> -		tr ' ' '\n' > $TMPDIR/perf.script
> -
> -	# BRBINF<n>.TYPE == TRAP are mapped to PERF_BR_IRQ by the BRBE driver
> -	check_branches "^trap_bench\+[^ ]+/[^ ]/IRQ/"
> -	check_branches "^[^ ]+/trap_bench\+[^ ]+/ERET/"
> -	if [ $err -eq 0 ]; then
> +	local ret=1
> +	for loops in 1000 10000 100000; do
> +		err=0
> +		perf record -o $TMPDIR/perf.data --branch-filter any,save_type,u,k -- \
> +			perf test -w traploop $loops > "$TMPDIR/record.txt" 2>&1
> +		perf script -i $TMPDIR/perf.data --fields brstacksym | \
> +			tr ' ' '\n' > $TMPDIR/perf.script
> +
> +		# BRBINF<n>.TYPE == TRAP are mapped to PERF_BR_IRQ by the BRBE driver
> +		check_branches "^trap_bench\+[^ ]+/[^ ]/IRQ/"
> +		check_branches "^[^ ]+/trap_bench\+[^ ]+/ERET/"

[Severity: Medium]
Will these check_branches() calls pollute the test logs with false-positive
errors during intermediate loop iterations? 

The shared check_branches() helper unconditionally prints "ERROR: Branches
missing" when it fails to find samples. This might trigger automated CI log
parser failures when an initial retryable loop iteration fails, even if a
subsequent iteration succeeds. This also applies to the similar loop added
to test_syscall().

> +		if [ $err -eq 0 ]; then
> +			ret=0
> +			break
> +		fi
> +	done

[ ... ]

> @@ -135,32 +144,40 @@ test_kernel_branches() {

[ ... ]

> -	# 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 kernel branch sampling [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]{1,15}/" $TMPDIR/perf.script; then
> -		echo "Testing kernel branch sampling [Failed user branches found]"
> -		err=1
> -	fi
> -	if [ $err -eq 0 ]; then
> +	local ret=1
> +	for loops in 1000 10000 100000; do
> +		err=0
> +		perf record -o $TMPDIR/perf.data --branch-filter any,k -- \
> +			perf bench syscall basic --loop $loops > "$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 in user or kernel code. Next is the target
> +		# address that must be in the kernel.
> +
> +		# Look for source addresses with top bit set
> +		if ! grep -q -E -m1 "^0x[89a-f][0-9a-f]{15}" $TMPDIR/perf.script; then
> +			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]{1,15}/" \
> +			$TMPDIR/perf.script; then
> +			err=1
> +		fi

[Severity: Medium]
Does removing these diagnostic echo statements strip necessary failure
diagnostics? 

If the test definitively fails after all retry iterations are exhausted, it
appears we will no longer know whether the failure was due to missing kernel
branches or unexpected user branches since the specific echo outputs have
been removed.

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