Re: [PATCH v2 4/4] perf test: Update test for --for-each-cgroup option

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The `check_evlist_expand()` test fails to exercise the bug it intends to verify because it specifies `-e cpu-clock`.
- [Medium] Bash pipeline semantics mask `perf stat` command failures, potentially causing false test passes.
--

--- Patch [4]: [PATCH v2 4/4] perf test: Update test for --for-each-cgroup option ---
commit b42b21fdf494fb61acde3f563ac29e9b49df9327
Author: Namhyung Kim <namhyung@kernel.org>

perf test: Update test for --for-each-cgroup option

This commit updates the stat_bpf_counters_cgrp.sh test to simply check the
number of output lines with and without the --for-each-cgroup option.

> diff --git a/tools/perf/tests/shell/stat_bpf_counters_cgrp.sh b/tools/perf/tests/shell/stat_bpf_counters_cgrp.sh
> index ff2e06c408bc6..17b17b112169c 100755
> --- a/tools/perf/tests/shell/stat_bpf_counters_cgrp.sh
> +++ b/tools/perf/tests/shell/stat_bpf_counters_cgrp.sh
[ ... ]
> @@ -58,9 +58,24 @@ check_system_wide_counted()
>  	fi
>  }
>  
> +# Missing flags in evlist__clone() resulted in different output.
> +# Just check the number of output lines for simple verification.
> +check_evlist_expand()
> +{
> +	normal_output=$(perf stat -a -e cpu-clock -x, true 2>&1 | wc -l)
> +	expand_output=$(perf stat -a -e cpu-clock -x, --bpf-counters --for-each-cgroup / true 2>&1 | wc -l)

[Severity: Medium]
Will this actually test the bug with evlist__clone() missing flags?

By specifying -e cpu-clock, the loading of default events is disabled,
meaning default_metricgroup and default_show_events are false. This prevents
the duplication bug from occurring during evsel__clone(), rendering the test
ineffective at catching the regression it was meant to verify.

[Severity: Medium]
Also, does using the pipeline | wc -l mask any failures from perf stat?

Without set -o pipefail, the pipeline's exit code will just be the success of
wc -l, so if perf stat fails (e.g. from missing permissions or PMU errors),
the script will miss the failure and might falsely report a test pass.

> +	if [ "${normal_output}" != "${expand_output}" ]; then
> +		if [ "${verbose}" = "1" ]; then
> +			echo "Normal output has ${normal_output} lines, but it now has ${expand_output}"
> +		fi
> +		exit 1
> +	fi
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710234143.1934263-1-namhyung@kernel.org?part=4