Re: [PATCH v2] perf stat: Make metric only column line up with header

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Cumulative column drift misalignment in --metric-only output.
- [Medium] Metric group header alignment is broken in standard (non-metric-only) perf stat output.
--

commit 1fbd687246bd068cad87b8edb8dcd01fdb03fe25
Author: Andi Kleen <ak@linux.intel.com>

perf stat: Make metric only column line up with header

This commit attempts to fix metric-only output column alignment by simplifying
the column width computation. It ignores EVNAME_LEN, MGROUP_LEN, and
config->unit_width, and relies solely on config->metric_only_len and the
actual unit width.

>     Fix all those. I simplified the column width computation significantly,
>     ignoring EVNAME_LEN, MGROUP_LEN, config->unit_width which don't
>     seem to be useful in the metric only context. It now only uses
>     the actual unit width as well as config->metric_only_len. The result
>     is more code removed than added.

This isn't a bug, but the commit message states these constants aren't useful
in the metric only context. However, print_metricgroup_header_std() is
explicitly used for standard (non-metric-only) output, where these constants
are still necessary for proper alignment.

> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index 993f4c4b8f442..2b69d238858cf 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -580,16 +580,13 @@ static void print_metricgroup_header_std(struct perf_stat_config *config,
>  					 const char *metricgroup_name)
>  {
>  	struct outstate *os = ctx;
>  
>  	if (!metricgroup_name) {
>  		__new_line_std(config, os);
>  		return;
>  	}
>  
> -	n = fprintf(config->output, " %*s", EVNAME_LEN, metricgroup_name);
> -
> -	fprintf(config->output, "%*s", MGROUP_LEN + config->unit_width + 2 - n, "");
> +	fprintf(config->output, " %*s", config->metric_only_len, metricgroup_name);

Does this change break metric group header alignment in standard perf stat
output?

print_metricgroup_header_std() is used in the standard (non-metric-only)
context. By reducing the padding from EVNAME_LEN + MGROUP_LEN (roughly 56
characters) to config->metric_only_len (typically 20), the metric group header
is printed too early, misaligning it with the subsequent metric columns.

[ ... ]

> @@ -599,19 +596,20 @@ static void print_metric_only(struct perf_stat_config *config,
>  	struct outstate *os = ctx;
>  	FILE *out = os->fh;
>  	char str[1024];
> -	unsigned mlen = config->metric_only_len;
> +	unsigned mlen;
>  	const char *color = metric_threshold_classify__color(thresh);
> +	int olen;
>  
> -	if (!unit)
> -		unit = "";
> -	if (mlen < strlen(unit))
> -		mlen = strlen(unit) + 1;
> +	if (!unit) {
> +		os->first = false;
> +		return;
> +	}
>  
> -	if (color)
> -		mlen += strlen(color) + sizeof(PERF_COLOR_RESET) - 1;
> +	mlen = max_t(unsigned, strlen(unit), config->metric_only_len);
>  
> +	olen = snprintf(str, sizeof(str), fmt ?: "", val);
>  	color_snprintf(str, sizeof(str), color ?: "", fmt ?: "", val);
> -	fprintf(out, "%*s ", mlen, str);
> +	fprintf(out, "%*s%s", max_t(int, mlen - olen, 1), "", str);

Can this cause a cumulative column drift in the --metric-only output?

The headers are printed with max(metric_only_len, strlen(unit)) characters
plus an unconditional trailing space, making the header column width exactly
mlen + 1 characters.

The modified print_metric_only() outputs exactly mlen characters with no
trailing space. Because each value column is exactly 1 character narrower than
its corresponding header column, the alignment breaks cumulatively across the
line, shifting left by 1 character per successive column.

>  	os->first = false;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513144906.557896-1-ak@linux.intel.com?part=1