Re: [PATCH v2 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] The test will falsely fail on healthy systems if the tested cgroups are idle.
- [Medium] The capability checks verify unrelated events instead of the tested metric, risking false failures on unsupported hardware.
- [Medium] A loose substring match causes the test to falsely pass if metrics for the root cgroup are missing.
--

commit 87b9c1deb7ab23590530747e324c9b8bf5728b9d
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 with --for-each-cgroup. It checks if system-wide monitoring is
supported and tests both standard mode and BPF counters mode.

> 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..2f74bd9e3f2ee
> --- /dev/null
> +++ b/tools/perf/tests/shell/stat_metrics_cgrp.sh
[ ... ]
> +# skip if system-wide is not supported
> +check_system_wide()
> +{
> +	log_verbose "Checking system-wide..."
> +	if ! perf stat -a -e instructions sleep 0.01 > /dev/null 2>&1
> +	then
> +		log_verbose "Skipping: system-wide monitoring not supported"
> +		exit 2
> +	fi
> +}

This check verifies the instructions event, but the test measures
insn_per_cycle which also requires cycles. If a test environment supports
instructions but lacks hardware support for cycles (such as in some virtual
environments), will this preliminary check pass but cause the actual test
to fail? Could this capability check use --metrics=insn_per_cycle instead?

[ ... ]
> +	# Split test_cgroups by comma
> +	IFS=',' read -r -a cgrps <<< "${test_cgroups}"
> +
> +	for cgrp in "${cgrps[@]}"; do
> +		# Find lines for this cgroup and metric
> +		local cgrp_lines
> +		cgrp_lines=$(echo "${output}" | grep -F "${cgrp}" | grep "insn_per_cycle" || true)

When cgrp is / (the root cgroup), won't grep -F "/" match any sub-cgroup
containing a slash, like /user.slice? If perf stat fails to output metrics
for the root cgroup, it seems cgrp_lines might still be populated by the
child's metrics, potentially causing the test to pass and hide a failure.

> +
> +		if [ -z "${cgrp_lines}" ]
> +		then
> +			echo "FAIL: No metric lines found for cgroup '${cgrp}'"
> +			exit 1
> +		fi
> +
> +		# Check for nan or nested
> +		if echo "${cgrp_lines}" | grep -q -i -E ",nan,|,nested,"
> +		then
> +			echo "FAIL: Invalid metric value (nan/nested) for cgroup '${cgrp}'"
> +			echo "Line: ${cgrp_lines}"
> +			exit 1
> +		fi
> +		# Check for empty metric value
> +		if echo "${cgrp_lines}" | grep -q -E ",,[[:space:]]*[^,]*insn_per_cycle"
> +		then
> +			echo "FAIL: Empty metric value for cgroup '${cgrp}'"

If a tested cgroup like user.slice is completely idle during the 0.1 second
sleep, its cycles and instructions counts will be 0. Would the insn_per_cycle
metric evaluate to NaN and be output as an empty field in CSV mode? 

If so, will this check cause the test to falsely fail on healthy systems
where the tested cgroups happen to be idle?

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