Re: [PATCH v2 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 1 potential issue(s) to consider:
- [Medium] The `test_stat_delay` test lacks an upper bound check, failing to verify that the delay is excluded from `duration_time`.
--

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

perf tests: Add test for stat delay option with duration_time

This commit adds a test case test_stat_delay to stat.sh to verify that
duration_time correctly excludes the delay period when using the delay
option (-D). It runs a perf stat command with a 1000ms delay and a 2 second
sleep, and checks the resulting duration_time.

> diff --git a/tools/perf/tests/shell/stat.sh b/tools/perf/tests/shell/stat.sh
> index 4edb04039036e..a9eadd57e1644 100755
> --- a/tools/perf/tests/shell/stat.sh
> +++ b/tools/perf/tests/shell/stat.sh
[ ... ]
> @@ -483,6 +483,53 @@ test_stat_pid() {
>    wait $pid 2>/dev/null || true
>  }
>  
> +test_stat_delay() {
> +  echo "stat -D test"
> +  if ! env LC_ALL=C 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 ',')
> +  elapsed=$(grep "seconds time elapsed" "${stat_output}" | awk '{print $1}')
> +
> +  if [ -z "$duration" ] || [ -z "$elapsed" ]
> +  then
> +    echo "stat -D test [Failed - failed to find duration_time or time elapsed in output]"
> +    cat "${stat_output}"
> +    err=1
> +    return
> +  fi
> +
> +  # Compare duration (ns) and elapsed (s) using awk to handle float and allow tolerance.
> +  if ! awk -v d="$duration" -v e="$elapsed" '
> +    BEGIN {
> +      diff = d - (e * 1e9);
> +      if (diff < 0) diff = -diff;
> +      # Allow 200ms tolerance (200,000,000 ns) for loaded CI machines.
> +      if (diff > 200000000) {
> +        printf "Fail: duration (%d ns) and elapsed (%f s) mismatch (diff %d ns)\n", d, e, diff;
> +        exit 1;
> +      }
> +      # Lower bound check: must be at least 0.5s.
> +      if (d < 500000000) {
> +        printf "Fail: duration (%d ns) is abnormally small\n", d;
> +        exit 1;
> +      }

Is there a missing upper bound check here to verify the delay was actually
excluded from duration_time?

The commit message states the test verifies duration_time is ~1s and not ~2s.
However, if a regression caused the -D option to be ignored, both duration
and elapsed would measure the full 2 seconds of the sleep command.

In that scenario, the difference between them would be near 0 (passing the
mismatch check), and duration would be ~2s which is > 0.5s (passing the lower
bound check). The test would silently pass, failing to detect the issue.

Could an upper bound check be added to ensure duration_time is strictly less
than 2 seconds?

> +      exit 0;
> +    }'

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