Re: [PATCH v1 2/2] perf test: Add stat metrics --for-each-cgroup test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Missing 'insn_per_cycle' metric support check contradicts the commit message and causes false-positive test failures.
- [Low] Unquoted variables in bash script can cause syntax errors and command execution failures.
- [Low] Unsafe regex matching with `grep` for cgroup paths.
--

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

perf test: Add stat metrics --for-each-cgroup test

This commit adds a new shell test stat_metrics_cgrp.sh to verify metric
reporting when using --for-each-cgroup. It tests metric outputs for cgroups
with and without BPF counters.

> The test:
> - Checks if system-wide monitoring is supported (skips if not).
> - Checks if insn_per_cycle metric is supported (skips if not).
> - Finds cgroups to test.

The commit message mentions checking if the insn_per_cycle metric is
supported, but the script only seems to check for system-wide support
using 'perf stat -a -e instructions'.

If the metric is unavailable on the test architecture, will the script fail
with exit code 1 ("No metric lines found") instead of gracefully skipping?

> diff --git a/tools/perf/tests/shell/stat_metrics_cgrp.sh b/tools/perf/tests/shell/stat_metrics_cgrp.sh
> new file mode 100755
> index 0000000000000..4a29eb9a90674
> --- /dev/null
> +++ b/tools/perf/tests/shell/stat_metrics_cgrp.sh
[ ... ]
> +	# try root and self cgroups
> +	find_cgroups_self_cgrp=$(grep perf_event /proc/self/cgroup | cut -d: -f3)
> +	if [ -z ${find_cgroups_self_cgrp} ]

This isn't a bug, but could this cause a bash syntax error ("too many
arguments") if the cgroup path happens to contain spaces? Enclosing the
variable in double quotes would prevent word splitting.

> +	then
> +		# cgroup v2 doesn't specify perf_event
> +		find_cgroups_self_cgrp=$(grep ^0: /proc/self/cgroup | cut -d: -f3)
> +	fi
> +
> +	if [ -z ${find_cgroups_self_cgrp} ]

Are double quotes also needed here to prevent word splitting?

> +	then
> +		test_cgroups="/"
> +	else
> +		test_cgroups="/,${find_cgroups_self_cgrp}"
> +	fi
[ ... ]
> +check_metric_reported()
> +{
> +	local opts="$1"
> +	local output
> +
> +	log_verbose "Running check_metric_reported with opts '${opts}'..."
> +	# Run perf stat
> +	if ! output=$(perf stat -a ${opts} \
> +			--metrics=insn_per_cycle \
> +			--for-each-cgroup ${test_cgroups} \

Similarly, should ${test_cgroups} be quoted to ensure perf stat doesn't
misinterpret space-separated cgroup paths as commands?

> +			-x, sleep 0.1 2>&1)
> +	then
> +		echo "FAIL: perf stat failed with exit code $?"
> +		echo "Output: ${output}"
> +		exit 1
> +	fi
[ ... ]
> +	for cgrp in "${cgrps[@]}"; do
> +		# Find lines for this cgroup and metric
> +		local cgrp_lines
> +		cgrp_lines=$(echo "${output}" | grep "${cgrp}" | grep "insn_per_cycle" || true)

Since cgroup names can contain dots (like "system.slice"), will grep
interpret the dot as a regex wildcard?

Using 'grep -F "${cgrp}"' might be safer to ensure a literal string match.

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