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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The test incorrectly reads the `instructions` event count for both `cycles_val` and `inst_val`, failing to verify if the `cycles` event was actually counted before enforcing the metric check.
--

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

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

This commit adds a new shell test to verify metric reporting with
the --for-each-cgroup flag in perf stat. It checks that metrics like
insn_per_cycle are properly reported per-cgroup and do not contain
invalid values when events are actively counted.

> 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..b413849193a0b
> --- /dev/null
> +++ b/tools/perf/tests/shell/stat_metrics_cgrp.sh
[ ... ]
> +	for cgrp in "${cgrps[@]}"; do
> +		# Find metric lines for this cgroup
> +		# We use exact cgroup match with surrounding commas
> +		local cgrp_lines
> +		cgrp_lines=$(echo "${output}" | grep -F ",${cgrp}," | grep "insn_per_cycle" || true)
[ ... ]
> +		# Parse each metric line
> +		while read -r line; do
> +			[ -z "${line}" ] && continue
> +
> +			local cycles_val
> +			cycles_val=$(echo "${line}" | cut -d, -f1)
> +
> +			local event_name
> +			event_name=$(echo "${line}" | cut -d, -f3)
> +
> +			# Find corresponding instructions event line
> +			local inst_event_name
> +			inst_event_name="${event_name/cpu-cycles/instructions}"
> +			inst_event_name="${inst_event_name/cycles/instructions}"
> +
> +			local inst_line
> +			inst_line=$(echo "${output}" | \
> +				grep -F ",${cgrp}," | \
> +				grep -F "${inst_event_name}" || true)
> +
> +			local inst_val
> +			inst_val=$(echo "${inst_line}" | cut -d, -f1)

Since cgrp_lines is filtered by insn_per_cycle earlier, the event_name
extracted from the third field will typically be instructions rather than
cycles.

Does this cause both cycles_val and inst_val to read the exact same
instructions count? If event_name is already instructions, the string
replacement leaves it unchanged, and inst_line will match the original line.

This could mean the script never actually verifies if the cycles event
itself was successfully counted before enforcing the metric check.

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