Re: [PATCH v1 2/2] perf tests: Add test for stat delay option with duration_time

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The test lacks a lower bound check, failing to verify that duration_time is actually ~1s as claimed.
- [Medium] Parsing formatted counter output assumes commas for thousands separators, causing silent test validation bypass in non-English locales.
- [Medium] The test uses a tight 1.5s threshold for duration_time, making it flaky on heavily loaded systems.
--

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

perf tests: Add test for stat delay option with duration_time

This commit adds a new test case to stat.sh to verify that the duration_time
event correctly excludes the delay period when using the delay option.

> diff --git a/tools/perf/tests/shell/stat.sh b/tools/perf/tests/shell/stat.sh
> index 4edb04039036e..e9a9b2c91cae3 100755
> --- a/tools/perf/tests/shell/stat.sh
> +++ b/tools/perf/tests/shell/stat.sh
[ ... ]
> @@ -483,6 +483,38 @@ test_stat_pid() {
>    wait $pid 2>/dev/null || true
>  }
>  
> +test_stat_delay() {
> +  echo "stat -D test"
> +  if ! perf stat -D 1000 -e duration_time sleep 2 > "${stat_output}" 2>&1
> +  then
> +    echo "stat -D test [Failed - command failed]"
> +    cat "${stat_output}"
> +    err=1
> +    return
> +  fi
> +
> +  duration=$(grep "duration_time" "${stat_output}" | awk '{print $1}' | tr -d ',')

Does this correctly parse the output in non-English locales?

If the locale uses periods or spaces for thousands separators instead of
commas, the string will retain those characters. When bash evaluates the
integer condition later, a string like 1.000.000.000 causes a syntax error.
Because this occurs inside an if condition, the script will silently bypass
the failure check and incorrectly report success. Should the perf stat
command be executed with the C locale to guarantee a consistent format?

> +  if [ -z "$duration" ]
> +  then
> +    echo "stat -D test [Failed - failed to find duration_time in output]"
> +    cat "${stat_output}"
> +    err=1
> +    return
> +  fi
> +
> +  # duration is in ns. We expect it to be ~1s (1,000,000,000 ns) because
> +  # sleep 2 runs for 2s, and we delay by 1s, so measured window is 1s.
> +  # If it includes delay, it will be > 2s (2,000,000,000 ns).
> +  if [ "$duration" -gt 1500000000 ]

Could a heavily loaded test machine or CI environment cause the sleep command
to take significantly longer than 2 seconds? If it takes 2.6 seconds, the
duration would exceed the 1500000000 ns threshold and cause the test to
falsely fail.

Additionally, is there a need to check a lower bound? If the delay option
breaks the counter and duration is zero or abnormally small, this upper bound
check will evaluate to false, which bypasses the failure block and leads
to a false success.

> +  then
> +    echo "stat -D test [Failed - duration_time ($duration ns) includes delay]"
> +    cat "${stat_output}"
> +    err=1
> +    return
> +  fi
> +  echo "stat -D test [Success]"
> +}

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