Re: [PATCH v3 13/14] perf stat: Add --new support to PMU metrics Python validator

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] is_basic_shadow_metric() performs an unchecked strcmp() on a potentially NULL mexp->metric_name.
- [Medium] Cgroup layout formatting is completely stripped from CSV, JSON, and STD output modes.
- [Medium] Zero-counter skipping logic was mistakenly scoped only to STD output, breaking CSV and JSON modes with output spam.
--

--- Patch [13]: [PATCH v3 13/14] perf stat: Add --new support to PMU metrics Python validator ---
commit ac2ffcbd7ec0fccf19ec9629f7f74d37b92d6545
Author: Ian Rogers <irogers@google.com>

perf stat: Add --new support to PMU metrics Python validator

This patch extends the performance metrics value Python validation script
tools/perf/tests/shell/lib/perf_metric_validation.py to support a new
command-line argument -new. When set, the Validator class appends the --new
option flag to its internally spawned perf stat commands. This enables
validating Intel PMU metric mathematical values generated specifically by the
decoupled, streaming JSON printing callbacks.

> diff --git a/tools/perf/tests/shell/lib/perf_metric_validation.py b/tools/perf/tests/shell/lib/perf_metric_validation.py
> --- a/tools/perf/tests/shell/lib/perf_metric_validation.py
> +++ b/tools/perf/tests/shell/lib/perf_metric_validation.py
[ ... ]
> @@ -379,7 +380,10 @@ class Validator:
>  
>      def _run_perf(self, metric, workload: str):
>          tool = 'perf'
> -        command = [tool, 'stat', '--cputype', self.cputype, '-j', '-M', f"{metric}", "-a"]
> +        command = [tool, 'stat']
> +        if self.new_print:
> +            command.append('--new')

[Severity: Medium]
While enabling this test for the new API, does the underlying --new
implementation safely handle metric evaluation?

In tools/perf/util/stat-print.c, is_basic_shadow_metric() performs an
unchecked string comparison:

tools/perf/util/stat-print.c:is_basic_shadow_metric() {
    ...
    for (i = 0; i < ARRAY_SIZE(basic_metrics); i++) {
        if (!strcmp(basic_metrics[i], name))
            return true;
    ...
}

If perf_stat_print_metricgroup() iterates through metric expressions and
passes a NULL mexp->metric_name (which can happen with unnamed or dynamic
metric expressions), will this strcmp trigger a segmentation fault?

[Severity: Medium]
Additionally, does the new printing API preserve cgroup aggregation
information?

When perf_stat__print_cb() traverses events in tools/perf/util/stat-print.c,
the generic callback signature omits cgroup parameters entirely:

tools/perf/util/stat-print.c:perf_stat__print_cb() {
    ...
    ret = cb->print_event(ctx, config, counter, 0, val, ena,
                          run, stdev_pct);
    ...
}

Will this cause a complete loss of cgroup layout formatting for CSV, JSON,
and STD output modes since the callbacks never receive the cgroup
identifiers?

[Severity: Medium]
Finally, how does the new API handle zero-counter skipping for CSV and JSON
modes? The core traversal engine perf_stat__print_cb() passes events to
callbacks without checking should_skip_zero_counter().

While the STD format callback might implement this filter locally, the CSV
and JSON callbacks do not appear to check it:

tools/perf/util/stat-print-csv.c:csv_print_event() {
    ...
    ev->val = val;
    ev->ena = ena;
    ev->run = run;
    ...
}

Could this result in CSV and JSON outputs being flooded with zero-valued
ghost events, such as uncore PMU events on non-leader CPUs, causing
incorrect statistical exports and output spam?

> +        command.extend(['--cputype', self.cputype, '-j', '-M', f"{metric}", "-a"])
>          wl = workload.split()
>          command.extend(wl)
>          print(" ".join(command))

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