Re: [PATCH v4 04/13] perf tests: Add robust record retry helper and use subsecond workloads

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Commit message inaccuracy and incomplete refactoring regarding `pipe_test.sh` and `stat_all_pfm.sh`.
- [Medium] Synchronous command execution in `test_kvm_record_report` lacks error checking, causing fatal script aborts under `set -e`.
- [Medium] `perf_record_with_retry` permanently leaks and disables the global `set -e` (errexit) flag when invoked within an OR list.
--

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

perf tests: Add robust record retry helper and use subsecond workloads

Introduce perf_record_with_retry and perf_record_cleanup in a shared
library tests/shell/lib/perf_record.sh to prevent record test failures
caused by transient recording or workload delays.

> Update record.sh, record_lbr.sh, pipe_test.sh, kvm.sh, and
> stat_all_pfm.sh to use this robust record retry logic. These tests now
> start with very short durations (e.g. 0.01 seconds) and scale up if the
> initial recording failed to capture samples, significantly improving test
> execution speed on success while remaining resilient to slow systems.

[Severity: Medium]
This isn't a bug, but the commit message states that stat_all_pfm.sh is updated 
to use the robust retry logic, yet stat_all_pfm.sh isn't modified in this patch.

> diff --git a/tools/perf/tests/shell/kvm.sh b/tools/perf/tests/shell/kvm.sh
> index f88e859025c42..a5396f8e6fe5c 100755
> --- a/tools/perf/tests/shell/kvm.sh
> +++ b/tools/perf/tests/shell/kvm.sh
[ ... ]
> @@ -60,22 +71,26 @@ test_kvm_stat() {
>  test_kvm_record_report() {
>  	echo "Testing perf kvm record/report"
>  
> -	echo "Recording kvm profile for pid ${qemu_pid}..."
[ ... ]
> +	local duration
> +	local success=false
> +	for duration in 1 2 4 8; do
> +		echo "Recording kvm profile for pid ${qemu_pid} (duration ${duration}s)..."
> +		rm -f "${perfdata}" "${perfdata}".old
> +
> +		perf kvm --host record -p "${qemu_pid}" -o "${perfdata}" \
> +			-e cpu-clock sleep ${duration}

[Severity: Medium]
Does this unguarded execution bypass the retry loop if it fails?

Since the kvm.sh script operates with `set -e` enabled globally, if 
`perf kvm record` returns a non-zero exit code here, the script will immediately 
abort entirely.

Should this be guarded, perhaps by appending `|| true` to prevent the fatal 
abort so the retry loop can continue?

> +
> +		if [ -e "${perfdata}" ] && \
> +			perf kvm -i "${perfdata}" report --stdio 2>&1 | grep -q "Event count"; then
> +			success=true
> +			break
> +		fi
> +		echo "No samples or report failed, retrying..."
> +	done

> diff --git a/tools/perf/tests/shell/lib/perf_record.sh b/tools/perf/tests/shell/lib/perf_record.sh
> new file mode 100644
> index 0000000000000..e137fa75370de
> --- /dev/null
> +++ b/tools/perf/tests/shell/lib/perf_record.sh
> @@ -0,0 +1,53 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +PERF_RECORD_LOGS=()
> +
> +perf_record_with_retry() {
[ ... ]
> +  # Save the e flag state and disable it
> +  local save_e
> +  if [[ $- == *e* ]]; then
> +    save_e="set -e"
> +  else
> +    save_e="set +e"
> +  fi

[Severity: Medium]
Does this logic permanently disable the `set -e` flag in the caller when 
invoked within an OR list?

In bash, executing a function on the left side of an `||` operator temporarily 
suspends `errexit` and explicitly removes the `e` flag from `$-`.

For example, when `record.sh` invokes this function:
    perf_record_with_retry ... || ret=$?

`[[ $- == *e* ]]` will evaluate to false, causing the function to save the state 
as `set +e`. When the function finishes, it evaluates this saved state, 
permanently disabling `set -e` globally for the remainder of the calling script 
and silently masking all subsequent command failures.

> +  set +e
> +
> +  local duration
> +  local first_run=true
> +  local ret=1

> diff --git a/tools/perf/tests/shell/pipe_test.sh b/tools/perf/tests/shell/pipe_test.sh
> index e459aa99a9515..ce68d850c9838 100755
> --- a/tools/perf/tests/shell/pipe_test.sh
> +++ b/tools/perf/tests/shell/pipe_test.sh
> @@ -12,8 +12,8 @@ skip_test_missing_symbol ${sym}
>  
>  data=$(mktemp /tmp/perf.data.XXXXXX)
>  data2=$(mktemp /tmp/perf.data2.XXXXXX)
> -prog="perf test -w noploop"
> -[ "$(uname -m)" = "s390x" ] && prog="$prog 3"
> +prog="perf test -w noploop 0.1"
> +[ "$(uname -m)" = "s390x" ] && prog="perf test -w noploop 3"

[Severity: Medium]
Will this hardcoded shorter duration make the test more brittle?

The commit message indicates this script was updated to use the robust retry 
logic, but it appears to only use a hardcoded 0.1s duration here. Relying on a 
single 0.1s run without retries might lead to transient failures on slow systems.

>  err=0
>  
>  set -e

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