Re: [RFC PATCH v1 08/14] perf stat: Extend CSV output linter to test core aggregation checks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The metric-only CSV formatter uses literal hardware IDs (via print_aggr_id_csv) instead of static labels for CSV column headers.
- [Medium] The metric-only CSV formatter unconditionally prints column headers on every invocation, breaking interval mode CSV structure.
--

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

perf stat: Extend CSV output linter to test core aggregation checks

This patch extends tools/perf/tests/shell/stat+csv_output.sh to run all
basic and core aggregation linter checks a second time under the --new
CSV print flag. This guarantees that CSV outputs produced by the decoupled
printing engine are formally verified and column-valid.

> diff --git a/tools/perf/tests/shell/stat+csv_output.sh b/tools/perf/tests/shell/stat+csv_output.sh
> index cd6fff5970911..e4153a03d7164 100755
> --- a/tools/perf/tests/shell/stat+csv_output.sh
> +++ b/tools/perf/tests/shell/stat+csv_output.sh
> @@ -88,5 +88,21 @@ then
>  else
>  	echo "[Skip] Skipping tests for system_wide_no_aggr, per_core, per_die and per_socket since socket id exposed via topology is invalid"
>  fi
> +# New API CSV checks
> +perf_cmd="--new -x$csv_sep -o ${stat_output}"
> +check_no_args "CSV (New API)" "$perf_cmd"
> +check_system_wide "CSV (New API)" "$perf_cmd"

[Severity: Medium]
Will this script also test the metric-only mode for CSV output?

When running the new CSV formatter with --metric-only, the column header
literally contains the data values of the first hardware group instead of
static column labels like 'core,cpus'.

In csv_metric_only_print_end(), the code calls print_aggr_id_csv() to generate
the aggregation prefix for the header row:

tools/perf/util/stat-print-csv.c:csv_metric_only_print_end() {
    ...
	/* Print aggregation prefix header in CSV metric-only mode */
	if (config->aggr_map && first_aggr >= 0) {
		struct aggr_cpu_id id = config->aggr_map->map[first_aggr];
        ...
		print_aggr_id_csv(config, output, mock_evsel, id, aggr_nr);
	}
    ...
}

Because print_aggr_id_csv() resolves and prints the runtime value of the
first aggregation group (e.g., S0-D0-C0), this produces a malformed CSV
where the header row contains specific hardware IDs rather than labels.

Should we use static labels for the metric-only CSV header?

> +check_interval "CSV (New API)" "$perf_cmd"

[Severity: Medium]
Does the interval mode work correctly when --metric-only is used?

When running perf stat in interval mode with --metric-only,
perf_stat__print_csv() is invoked periodically. Each invocation instantiates
a fresh csv_metric_only_print_state context and calls
csv_metric_only_print_end():

tools/perf/util/stat-print-csv.c:perf_stat__print_csv() {
	if (config->metric_only) {
		struct csv_metric_only_print_state ps = {
			.fp = config->output,
            ...
		};
        ...
		return perf_stat__print_cb(evlist, config, target, ts, argc, argv,
					   &csv_metric_only_print_callbacks, &ps);
    ...
}

Inside csv_metric_only_print_end(), it unconditionally iterates over
ps->queued_metrics to print the column headers before printing the values.

Because there is no state tracking across intervals, the CSV output will
contain interleaved header and data rows across intervals, breaking the CSV
structure which requires exactly one header row.

Is it intended to print headers repeatedly in interval mode, or should we
add state tracking to print the header only once?

> +check_event "CSV (New API)" "$perf_cmd"
> +check_per_thread "CSV (New API)" "$perf_cmd"
> +check_per_node "CSV (New API)" "$perf_cmd"
> +if [ $skip_test -ne 1 ]
> +then
> +	check_system_wide_no_aggr "CSV (New API)" "$perf_cmd"
> +	check_per_core "CSV (New API)" "$perf_cmd"
> +	check_per_die "CSV (New API)" "$perf_cmd"
> +	check_per_socket "CSV (New API)" "$perf_cmd"
> +fi
> +
>  cleanup
>  exit 0

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