Re: [PATCH v2 07/12] perf tests: Fix flakiness in trace record and replay test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The `perf_record_with_retry` helper declares a `local logfile` which shadows the caller's `logfile` variable, breaking permission checks in callers like `test_uid`.
- [Medium] The `perf_record_cleanup` function indiscriminately deletes all temporary log files globally, destroying test isolation for parallel runs.
--

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

perf tests: Fix flakiness in trace record and replay test
    
This commit updates the trace_record_replay.sh test to use a robust retry
loop for capturing the required nanosleep events. It generalizes the
perf_record_with_retry helper to support a custom command prefix via the
PERF_RECORD_CMD environment variable.

> diff --git a/tools/perf/tests/shell/lib/perf_record.sh b/tools/perf/tests/shell/lib/perf_record.sh
> index fe5721427e58b..2c705840d5547 100644
> --- a/tools/perf/tests/shell/lib/perf_record.sh
> +++ b/tools/perf/tests/shell/lib/perf_record.sh
> @@ -21,9 +21,14 @@ perf_record_with_retry() {
>    local duration
>    local first_run=true
>    local ret=1
> +  local cmd_prefix="perf record"
> +  if [ -n "${PERF_RECORD_CMD}" ]; then
> +    cmd_prefix="${PERF_RECORD_CMD}"
> +  fi
> +
>    for duration in 0.01 0.1 0.3 1.0 2.0; do
>      rm -f "${perfdata}".old
> -    perf record "$@" -o "${perfdata}" ${testprog_base} ${duration} > "$logfile" 2>&1
> +    ${cmd_prefix} "$@" -o "${perfdata}" ${testprog_base} ${duration} > "$logfile" 2>&1

[Severity: Medium]
Does this code shadow the caller's logfile variable, breaking permission
checks in callers like test_uid()?

In perf_record_with_retry(), a local logfile is declared:

tools/perf/tests/shell/lib/perf_record.sh:perf_record_with_retry() {
  ...
  local logfile
  logfile=$(mktemp /tmp/__perf_record_retry.XXXXXX.log)
  ...
}

But the caller test_uid() attempts to verify the failure reason by reading
its own unwritten logfile:

tools/perf/tests/shell/record.sh:test_uid() {
  local logfile
  logfile="/tmp/__perf_record_retry.$(id -u).$BASHPID.log"
  local ret=0
  perf_record_with_retry "${perfdata}" "check_uid" "perf test -w thloop" \
    -aB --synth=no --uid "$(id -u)" || ret=$?
  if [ $ret -eq 2 ]; then
    if grep -q -E "libbpf.*EPERM|Access to performance monitoring|Permission denied|Failure to open any events" \
      "$logfile"
    ...
}

Because the local logfile in perf_record_with_retry() captures the output,
the grep in test_uid() silently fails.

> diff --git a/tools/perf/tests/shell/trace_record_replay.sh b/tools/perf/tests/shell/trace_record_replay.sh
> index 88d30a03dcecb..f27e32b186978 100755
> --- a/tools/perf/tests/shell/trace_record_replay.sh
> +++ b/tools/perf/tests/shell/trace_record_replay.sh

[ ... ]

> @@ -6,16 +6,26 @@
>  
>  # shellcheck source=lib/probe.sh
>  . "$(dirname $0)"/lib/probe.sh
> +# shellcheck source=lib/perf_record.sh
> +. "$(dirname $0)"/lib/perf_record.sh
>  
>  skip_if_no_perf_trace || exit 2
>  [ "$(id -u)" = 0 ] || exit 2
>  
>  file=$(mktemp /tmp/temporary_file.XXXXX)
>  
> -perf trace record -o ${file} sleep 1 || exit 1
> -if ! perf trace -i ${file} 2>&1 | grep nanosleep; then
> +check_nanosleep() {
> +  perf trace -i "${file}" 2>&1 | grep -q nanosleep
> +}
> +
> +PERF_RECORD_CMD="perf trace record" perf_record_with_retry "${file}" "check_nanosleep" "sleep"
> +err=$?
> +
> +perf_record_cleanup

[Severity: Medium]
Could calling perf_record_cleanup() here clobber the log files of other tests
running concurrently?

The perf_record_cleanup() function performs a wildcard deletion:

tools/perf/tests/shell/lib/perf_record.sh:perf_record_cleanup() {
  rm -f /tmp/__perf_record_retry.*.log
}

This will indiscriminately delete all temporary log files globally,
destroying test isolation and potentially leading to race conditions and
unpredictable flaky failures across the test suite (e.g., via perf test -p).

> +rm -f ${file}
> +
> +if [ $err -ne 0 ]; then
>  	echo "Failed: cannot find *nanosleep syscall"
>  	exit 1
>  fi

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