Re: [PATCH v2 04/12] 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] GNU `mktemp` syntax error breaks the recording loop for all tests.
- [Medium] Local variable shadowing in `perf_record_with_retry` breaks `test_uid`'s permission check.
- [Medium] `perf_record_cleanup` uses a global wildcard deletion that clobbers log files of concurrently executing tests.
--

commit d3b4c0d4db4697b810bf2709918c20b98619d0e1
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.

> diff --git a/tools/perf/tests/shell/lib/perf_record.sh b/tools/perf/tests/shell/lib/perf_record.sh
> new file mode 100644
> --- /dev/null
> +++ b/tools/perf/tests/shell/lib/perf_record.sh
> @@ -0,0 +1,47 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +perf_record_with_retry() {
> +  local perfdata="$1"
> +  local check_cmd="$2"
> +  local testprog_base="$3"
> +  shift 3
> +
> +  local logfile
> +  logfile=$(mktemp /tmp/__perf_record_retry.XXXXXX.log)

[Severity: Medium]
Will this mktemp invocation fail because the X's are not at the end of the
template?

GNU mktemp requires the X characters to be at the absolute end of the template
string (unless the --suffix option is used). Since this fails with 'too few
X's in template', the logfile variable is left empty.

[ ... ]

> +  local duration
> +  local first_run=true
> +  local ret=1
> +  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

[Severity: Medium]
Can the empty logfile variable cause a syntax error here?

Because mktemp failed and logfile is empty, bash attempts to redirect the
output to an empty string. This aborts the command with a 'No such file or
directory' error and prevents perf record from running at all.

[ ... ]

> +perf_record_cleanup() {
> +  rm -f /tmp/__perf_record_retry.*.log

[Severity: Medium]
Could this wildcard deletion remove the log files of other concurrently
running tests?

Since perf test can execute tests in parallel, using a global wildcard
deletion like this can clobber the active temporary log files of other tests.
This breaks test isolation and can cause sporadic failures or masked errors.

> diff --git a/tools/perf/tests/shell/record.sh b/tools/perf/tests/shell/record.sh
> --- a/tools/perf/tests/shell/record.sh
> +++ b/tools/perf/tests/shell/record.sh
[ ... ]
> @@ -235,16 +240,19 @@ test_uid() {
>    echo "Uid sampling test"
> -  if ! perf record -aB --synth=no --uid "$(id -u)" -o "${perfdata}" ${testprog} \
> -    > "${script_output}" 2>&1
> -  then
> -    if grep -q "libbpf.*EPERM" "${script_output}"
> +  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"

[Severity: Medium]
Is it possible the grep will fail to find the EPERM error because the logs
were written to a different file?

The perf_record_with_retry helper declares its own 'local logfile' and sets it
to a dynamically generated mktemp path. This shadows the caller's logfile
variable without exporting the actual filename used.

When test_uid attempts to grep its own logfile path, it operates on a
non-existent file.

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