Re: [PATCH v4 02/14] perf stat: Implement standard console (STD) formatting callbacks

[Namhyung Kim <namhyung@kernel.org>]

On Thu, Jul 16, 2026 at 12:02:51AM -0700, Ian Rogers wrote:
> This patch implements standard console formatting callbacks inside
> util/stat-print-std.c, replacing the empty stubs introduced in Commit 1.
> 
> Introduces the format-private `struct queued_event` and `struct
> queued_metric` DOM nodes to buffer traversal streams, and fully
> encapsulates DOM state initialization and queue cleanups inside
> std_print_start() and std_print_end().
> 
> Utilizes the newly centralized unified aggregation helpers to resolve
> CPU and thread prefixes cleanly, and incorporates full interval-mode
> timestamp printing support across all rows.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> Assisted-by: Antigravity:gemini-3.5-flash
> Acked-by: Chun-Tse Shao <ctshao@google.com>
> ---
[SNIP]
> +static int std_print_event(void *ctx, struct perf_stat_config *config,
> +			   struct evsel *evsel, int aggr_idx, u64 val, u64 ena,
> +			   u64 run, double stdev_pct, const char *cgrp)
> +{
> +	struct std_print_state *ps = ctx;
> +	struct queued_event *ev;
> +	/* Skip zero counters locally in STD callbacks if they qualify */
> +	if (val == 0 && std_should_skip_zero_counter(config, evsel, aggr_idx)) {
> +		ps->current_event = NULL;
> +		return 0;
> +	}
> +
> +	ev = malloc(sizeof(*ev));
> +	if (!ev)
> +		return -ENOMEM;

I expected this function prints the event info right away.  Why we need
to save it and print later?

> +
> +	ev->name = strdup(evsel__name(evsel));
> +	if (!ev->name) {
> +		free(ev);
> +		return -ENOMEM;
> +	}
> +
> +	if (cgrp && cgrp[0]) {
> +		ev->cgrp = strdup(cgrp);
> +		if (!ev->cgrp) {
> +			free(ev->name);
> +			free(ev);
> +			return -ENOMEM;
> +		}
> +	} else {
> +		ev->cgrp = NULL;
> +	}

Do you really need this?  Why not using evsel__name() and
evsel->cgrp->name directly?


> +
> +	ev->evsel = evsel;
> +	ev->val = val;
> +	ev->ena = ena;
> +	ev->run = run;
> +	ev->stdev_pct = stdev_pct;
> +	ev->aggr_idx = aggr_idx;
> +	INIT_LIST_HEAD(&ev->metrics_list);
> +
> +	list_add_tail(&ev->list, &ps->events_list);
> +	ps->current_event = ev;
> +
> +	return 0;
> +}
[SNIP]
> +static int std_print_end(void *ctx, struct perf_stat_config *config)
>  {
> +	struct std_print_state *ps = ctx;
> +	struct queued_event *ev, *tmp_ev;
> +	struct queued_metric *met, *tmp_met;
> +	FILE *out = ps->fp;
> +	bool first;
> +	const char *last_mg_name = NULL;
> +	const struct perf_pmu *last_pmu = NULL;
> +	int last_aggr_idx = -1;
> +
> +	/* Print the formatted header prefix (only in non-interval mode) */
> +	if (!config->interval)
> +		print_header_std(config, ps->target, ps->argc, ps->argv);
> +
> +	list_for_each_entry_safe(ev, tmp_ev, &ps->events_list, list) {
> +		struct evsel *evsel = ev->evsel;
> +		double sc = evsel->scale;
> +		const char *fmt;
> +		const char *bad_count = evsel->supported ? CNTR_NOT_COUNTED : CNTR_NOT_SUPPORTED;
> +		struct metric_event *me =
> +			metricgroup__lookup(&evsel->evlist->metric_events, evsel, false);
> +		bool is_metricgroup = false;
> +		bool skip_header = false;
> +		char full_name[128] = "";
> +
> +		if (me && me->is_default && !evsel->default_show_events) {
> +			struct metric_expr *mexp =
> +				list_first_entry(&me->head, struct metric_expr, nd);
> +			const char *mg_name = mexp->default_metricgroup_name;
> +			bool need_full_name = perf_pmus__num_core_pmus() > 1;
> +
> +			if (need_full_name && evsel->pmu)
> +				scnprintf(full_name, sizeof(full_name), "%s (%s)", mg_name,
> +					  evsel->pmu->name);
> +			else
> +				scnprintf(full_name, sizeof(full_name), "%s", mg_name);
> +			is_metricgroup = true;
> +
> +			if (last_mg_name && !strcmp(last_mg_name, mg_name) &&
> +			    last_pmu == evsel->pmu && last_aggr_idx == ev->aggr_idx) {
> +				skip_header = true;
> +			}
> +			last_mg_name = mg_name;
> +			last_pmu = evsel->pmu;
> +			last_aggr_idx = ev->aggr_idx;
> +		}
> +
> +		/* Print interval timestamp if configured */
> +		if (config->interval && ps->timestamp[0] && !skip_header)
> +			fprintf(out, "%s", ps->timestamp);
> +
> +		/* 1. Print aggregation prefix first (if we don't skip header) */
> +		if (!skip_header && config->aggr_map && ev->aggr_idx >= 0) {
> +			struct aggr_cpu_id id = config->aggr_map->map[ev->aggr_idx];
> +			int aggr_nr = 0;
> +
> +			if (evsel->stats && evsel->stats->aggr)
> +				aggr_nr = evsel->stats->aggr[ev->aggr_idx].nr;
> +
> +			print_aggr_id_std(config, out, evsel, id, aggr_nr);
> +		}
> +
> +		/* 2. Print event value (scaled) or spaces if metricgroup */
> +		if (is_metricgroup) {
> +			if (!skip_header) {
> +				int n = fprintf(out, " %*s", EVNAME_LEN, full_name);
> +				int pad = MGROUP_LEN + config->unit_width + 2 - n;
> +
> +				fprintf(out, "%*s", pad > 0 ? pad : 0, "");
> +			}
> +		} else {
> +			if (config->big_num)
> +				fmt = floor(sc) != sc ? "%'*.2f " : "%'*.0f ";
> +			else
> +				fmt = floor(sc) != sc ? "%*.2f " : "%*.0f ";
> +
> +			if (ev->run == 0 || ev->ena == 0) {
> +				fprintf(out, "%*s ", COUNTS_LEN, bad_count);
> +			} else {
> +				double scaled = (double)ev->val;
> +				double avg;
> +
> +				if (ev->ena < ev->run)
> +					scaled = (double)ev->val * ev->run / ev->ena;

I think the value is already scaled in process_counter_values().

Thanks,
Namhyung

> +
> +				avg = scaled * sc;
> +				fprintf(out, fmt, COUNTS_LEN, avg);
> +			}