All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCHSET 00/19] perf stat: Improve perf stat output (v1)
@ 2022-11-14 23:02 Namhyung Kim
  2022-11-14 23:02 ` [PATCH 01/19] perf stat: Clear screen only if output file is a tty Namhyung Kim
                   ` (19 more replies)
  0 siblings, 20 replies; 21+ messages in thread
From: Namhyung Kim @ 2022-11-14 23:02 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Jiri Olsa
  Cc: Ingo Molnar, Peter Zijlstra, LKML, Ian Rogers, Adrian Hunter,
	linux-perf-users, Kan Liang, Zhengjun Xing, James Clark,
	Athira Jajeev

Hello,

I'm working on cleanup up the perf stat code.  The main focus this time
is the display logic which has various combinations of options.

I split the code for each output mode - std, csv and json.  And then
organize them according to the purpose like header, prefix, value,
metric and footer.  I hope this would help maintaining the code a bit
more.

Also I found that cgroup support is missing or insufficient.
Specifically when --for-each-cgroup option is given, it'd have multiple
copies of the events for those cgroups.  Then the output should group
the result.  This is true for the normal output mode, but the metric-
only mode didn't support it well.

With this change, I can see the per-cgroup topdown metrics like below:

  $ sudo ./perf stat -a --topdown --for-each-cgroup user.slice,system.slice sleep 3
  nmi_watchdog enabled with topdown. May give wrong results.
  Disable with echo 0 > /proc/sys/kernel/nmi_watchdog

   Performance counter stats for 'system wide':

                                  retiring      bad speculation   frontend bound    backend bound
  S0-D0-C0      2  user.slice            117.3%         3.9%            47.8%           -69.1%
  S0-D0-C1      2  user.slice            119.8%         4.1%            49.3%           -73.2%
  S0-D0-C2      2  user.slice             24.4%         7.9%            68.4%             0.0%
  S0-D0-C3      2  user.slice             24.0%         9.2%            91.2%           -24.4%
  S0-D0-C0      2  system.slice           73.5%         4.0%            19.4%             3.1%
  S0-D0-C1      2  system.slice           90.0%         5.8%            19.7%           -15.5%
  S0-D0-C2      2  system.slice          101.2%         6.6%            33.4%           -41.1%
  S0-D0-C3      2  system.slice           90.7%         4.9%            22.3%           -18.0%

         3.001678216 seconds time elapsed

You can get it from 'perf/stat-display-v1' branch in

  git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git

Thanks,
Namhyung

Namhyung Kim (19):
  perf stat: Clear screen only if output file is a tty
  perf stat: Split print_running() function
  perf stat: Split print_noise_pct() function
  perf stat: Split print_cgroup() function
  perf stat: Split aggr_printout() function
  perf stat: Factor out print_counter_value() function
  perf stat: Handle bad events in abs_printout()
  perf stat: Add before_metric argument
  perf stat: Align cgroup names
  perf stat: Split print_metric_headers() function
  perf stat: Factor out prepare_interval()
  perf stat: Cleanup interval print alignment
  perf stat: Remove impossible condition
  perf stat: Rework header display
  perf stat: Move condition to print_footer()
  perf stat: Factor out prefix display
  perf stat: Factor out print_metric_{begin,end}()
  perf stat: Support --for-each-cgroup and --metric-only
  perf stat: Add print_aggr_cgroup() for --for-each-cgroup and --topdown

 tools/perf/builtin-stat.c      |   8 +
 tools/perf/util/stat-display.c | 996 ++++++++++++++++++++-------------
 2 files changed, 624 insertions(+), 380 deletions(-)


base-commit: 7565f9617efac0c0c8e2dbd08dbe0695d56684f5
-- 
2.38.1.493.g58b659f92b-goog


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 01/19] perf stat: Clear screen only if output file is a tty
  2022-11-14 23:02 [PATCHSET 00/19] perf stat: Improve perf stat output (v1) Namhyung Kim
@ 2022-11-14 23:02 ` Namhyung Kim
  2022-11-14 23:02 ` [PATCH 02/19] perf stat: Split print_running() function Namhyung Kim
                   ` (18 subsequent siblings)
  19 siblings, 0 replies; 21+ messages in thread
From: Namhyung Kim @ 2022-11-14 23:02 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Jiri Olsa
  Cc: Ingo Molnar, Peter Zijlstra, LKML, Ian Rogers, Adrian Hunter,
	linux-perf-users, Kan Liang, Zhengjun Xing, James Clark,
	Athira Jajeev

The --interval-clear option makes perf stat to clear the terminal at
each interval.  But it doesn't need to clear the screen when it saves
to a file.  Make it fail when it's enabled with the output options.

  $ perf stat -I 1 --interval-clear -o myfile true
  --interval-clear does not work with output

   Usage: perf stat [<options>] [<command>]

      -o, --output <file>   output file name
          --log-fd <n>      log output to fd, instead of stderr
          --interval-clear  clear screen in between new interval

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-stat.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index d5e1670bca20..1d79801f4e84 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -2403,6 +2403,14 @@ int cmd_stat(int argc, const char **argv)
 		}
 	}
 
+	if (stat_config.interval_clear && !isatty(fileno(output))) {
+		fprintf(stderr, "--interval-clear does not work with output\n");
+		parse_options_usage(stat_usage, stat_options, "o", 1);
+		parse_options_usage(NULL, stat_options, "log-fd", 0);
+		parse_options_usage(NULL, stat_options, "interval-clear", 0);
+		return -1;
+	}
+
 	stat_config.output = output;
 
 	/*
-- 
2.38.1.493.g58b659f92b-goog


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 02/19] perf stat: Split print_running() function
  2022-11-14 23:02 [PATCHSET 00/19] perf stat: Improve perf stat output (v1) Namhyung Kim
  2022-11-14 23:02 ` [PATCH 01/19] perf stat: Clear screen only if output file is a tty Namhyung Kim
@ 2022-11-14 23:02 ` Namhyung Kim
  2022-11-14 23:02 ` [PATCH 03/19] perf stat: Split print_noise_pct() function Namhyung Kim
                   ` (17 subsequent siblings)
  19 siblings, 0 replies; 21+ messages in thread
From: Namhyung Kim @ 2022-11-14 23:02 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Jiri Olsa
  Cc: Ingo Molnar, Peter Zijlstra, LKML, Ian Rogers, Adrian Hunter,
	linux-perf-users, Kan Liang, Zhengjun Xing, James Clark,
	Athira Jajeev

To make the code more obvious and hopefully simpler, factor out the
code for each output mode - stdio, CSV, JSON.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/stat-display.c | 37 +++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index 2a3c1e0098b9..281b811f8574 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -25,24 +25,41 @@
 #define CNTR_NOT_SUPPORTED	"<not supported>"
 #define CNTR_NOT_COUNTED	"<not counted>"
 
-static void print_running(struct perf_stat_config *config,
-			  u64 run, u64 ena)
+static void print_running_std(struct perf_stat_config *config, u64 run, u64 ena)
+{
+	if (run != ena)
+		fprintf(config->output, "  (%.2f%%)", 100.0 * run / ena);
+}
+
+static void print_running_csv(struct perf_stat_config *config, u64 run, u64 ena)
 {
+	double enabled_percent = 100;
+
+	if (run != ena)
+		enabled_percent = 100 * run / ena;
+	fprintf(config->output, "%s%" PRIu64 "%s%.2f",
+		config->csv_sep, run, config->csv_sep, enabled_percent);
+}
 
+static void print_running_json(struct perf_stat_config *config, u64 run, u64 ena)
+{
 	double enabled_percent = 100;
 
 	if (run != ena)
 		enabled_percent = 100 * run / ena;
+	fprintf(config->output, "\"event-runtime\" : %" PRIu64 ", \"pcnt-running\" : %.2f, ",
+		run, enabled_percent);
+}
+
+static void print_running(struct perf_stat_config *config,
+			  u64 run, u64 ena)
+{
 	if (config->json_output)
-		fprintf(config->output,
-			"\"event-runtime\" : %" PRIu64 ", \"pcnt-running\" : %.2f, ",
-			run, enabled_percent);
+		print_running_json(config, run, ena);
 	else if (config->csv_output)
-		fprintf(config->output,
-			"%s%" PRIu64 "%s%.2f", config->csv_sep,
-			run, config->csv_sep, enabled_percent);
-	else if (run != ena)
-		fprintf(config->output, "  (%.2f%%)", 100.0 * run / ena);
+		print_running_csv(config, run, ena);
+	else
+		print_running_std(config, run, ena);
 }
 
 static void print_noise_pct(struct perf_stat_config *config,
-- 
2.38.1.493.g58b659f92b-goog


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 03/19] perf stat: Split print_noise_pct() function
  2022-11-14 23:02 [PATCHSET 00/19] perf stat: Improve perf stat output (v1) Namhyung Kim
  2022-11-14 23:02 ` [PATCH 01/19] perf stat: Clear screen only if output file is a tty Namhyung Kim
  2022-11-14 23:02 ` [PATCH 02/19] perf stat: Split print_running() function Namhyung Kim
@ 2022-11-14 23:02 ` Namhyung Kim
  2022-11-14 23:02 ` [PATCH 04/19] perf stat: Split print_cgroup() function Namhyung Kim
                   ` (16 subsequent siblings)
  19 siblings, 0 replies; 21+ messages in thread
From: Namhyung Kim @ 2022-11-14 23:02 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Jiri Olsa
  Cc: Ingo Molnar, Peter Zijlstra, LKML, Ian Rogers, Adrian Hunter,
	linux-perf-users, Kan Liang, Zhengjun Xing, James Clark,
	Athira Jajeev

Likewise, split print_noise_pct() for each output mode.  Although it's
a tiny function, more logic will be added soon so it'd be better split
it and treat it in the same way.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/stat-display.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index 281b811f8574..a230f65efa62 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -62,17 +62,36 @@ static void print_running(struct perf_stat_config *config,
 		print_running_std(config, run, ena);
 }
 
+static void print_noise_pct_std(struct perf_stat_config *config,
+				double pct)
+{
+	if (pct)
+		fprintf(config->output, "  ( +-%6.2f%% )", pct);
+}
+
+static void print_noise_pct_csv(struct perf_stat_config *config,
+				double pct)
+{
+	fprintf(config->output, "%s%.2f%%", config->csv_sep, pct);
+}
+
+static void print_noise_pct_json(struct perf_stat_config *config,
+				 double pct)
+{
+	fprintf(config->output, "\"variance\" : %.2f, ", pct);
+}
+
 static void print_noise_pct(struct perf_stat_config *config,
 			    double total, double avg)
 {
 	double pct = rel_stddev_stats(total, avg);
 
 	if (config->json_output)
-		fprintf(config->output, "\"variance\" : %.2f, ", pct);
+		print_noise_pct_json(config, pct);
 	else if (config->csv_output)
-		fprintf(config->output, "%s%.2f%%", config->csv_sep, pct);
-	else if (pct)
-		fprintf(config->output, "  ( +-%6.2f%% )", pct);
+		print_noise_pct_csv(config, pct);
+	else
+		print_noise_pct_std(config, pct);
 }
 
 static void print_noise(struct perf_stat_config *config,
-- 
2.38.1.493.g58b659f92b-goog


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 04/19] perf stat: Split print_cgroup() function
  2022-11-14 23:02 [PATCHSET 00/19] perf stat: Improve perf stat output (v1) Namhyung Kim
                   ` (2 preceding siblings ...)
  2022-11-14 23:02 ` [PATCH 03/19] perf stat: Split print_noise_pct() function Namhyung Kim
@ 2022-11-14 23:02 ` Namhyung Kim
  2022-11-14 23:02 ` [PATCH 05/19] perf stat: Split aggr_printout() function Namhyung Kim
                   ` (15 subsequent siblings)
  19 siblings, 0 replies; 21+ messages in thread
From: Namhyung Kim @ 2022-11-14 23:02 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Jiri Olsa
  Cc: Ingo Molnar, Peter Zijlstra, LKML, Ian Rogers, Adrian Hunter,
	linux-perf-users, Kan Liang, Zhengjun Xing, James Clark,
	Athira Jajeev

Likewise, split print_cgroup() for each output mode.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/stat-display.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index a230f65efa62..af2a561eb20c 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -106,15 +106,32 @@ static void print_noise(struct perf_stat_config *config,
 	print_noise_pct(config, stddev_stats(&ps->res_stats), avg);
 }
 
+static void print_cgroup_std(struct perf_stat_config *config, const char *cgrp_name)
+{
+	fprintf(config->output, " %s", cgrp_name);
+}
+
+static void print_cgroup_csv(struct perf_stat_config *config, const char *cgrp_name)
+{
+	fprintf(config->output, "%s%s", config->csv_sep, cgrp_name);
+}
+
+static void print_cgroup_json(struct perf_stat_config *config, const char *cgrp_name)
+{
+	fprintf(config->output, "\"cgroup\" : \"%s\", ", cgrp_name);
+}
+
 static void print_cgroup(struct perf_stat_config *config, struct evsel *evsel)
 {
 	if (nr_cgroups) {
 		const char *cgrp_name = evsel->cgrp ? evsel->cgrp->name  : "";
 
 		if (config->json_output)
-			fprintf(config->output, "\"cgroup\" : \"%s\", ", cgrp_name);
+			print_cgroup_json(config, cgrp_name);
+		if (config->csv_output)
+			print_cgroup_csv(config, cgrp_name);
 		else
-			fprintf(config->output, "%s%s", config->csv_sep, cgrp_name);
+			print_cgroup_std(config, cgrp_name);
 	}
 }
 
-- 
2.38.1.493.g58b659f92b-goog


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 05/19] perf stat: Split aggr_printout() function
  2022-11-14 23:02 [PATCHSET 00/19] perf stat: Improve perf stat output (v1) Namhyung Kim
                   ` (3 preceding siblings ...)
  2022-11-14 23:02 ` [PATCH 04/19] perf stat: Split print_cgroup() function Namhyung Kim
@ 2022-11-14 23:02 ` Namhyung Kim
  2022-11-14 23:02 ` [PATCH 06/19] perf stat: Factor out print_counter_value() function Namhyung Kim
                   ` (14 subsequent siblings)
  19 siblings, 0 replies; 21+ messages in thread
From: Namhyung Kim @ 2022-11-14 23:02 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Jiri Olsa
  Cc: Ingo Molnar, Peter Zijlstra, LKML, Ian Rogers, Adrian Hunter,
	linux-perf-users, Kan Liang, Zhengjun Xing, James Clark,
	Athira Jajeev

The aggr_printout() function is to print aggr_id and count (nr).
Split it for each output mode to simplify the code.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/stat-display.c | 220 ++++++++++++++++++---------------
 1 file changed, 121 insertions(+), 99 deletions(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index af2a561eb20c..ed421f6d512f 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -135,124 +135,135 @@ static void print_cgroup(struct perf_stat_config *config, struct evsel *evsel)
 	}
 }
 
-
-static void aggr_printout(struct perf_stat_config *config,
-			  struct evsel *evsel, struct aggr_cpu_id id, int nr)
+static void print_aggr_id_std(struct perf_stat_config *config,
+			      struct evsel *evsel, struct aggr_cpu_id id, int nr)
 {
+	FILE *output = config->output;
 
+	switch (config->aggr_mode) {
+	case AGGR_CORE:
+		fprintf(output, "S%d-D%d-C%*d %*d ",
+			id.socket, id.die, -8, id.core, 4, nr);
+		break;
+	case AGGR_DIE:
+		fprintf(output, "S%d-D%*d %*d ",
+			id.socket, -8, id.die, 4, nr);
+		break;
+	case AGGR_SOCKET:
+		fprintf(output, "S%*d %*d ",
+			-5, id.socket, 4, nr);
+		break;
+	case AGGR_NODE:
+		fprintf(output, "N%*d %*d ",
+			-5, id.node, 4, nr);
+		break;
+	case AGGR_NONE:
+		if (evsel->percore && !config->percore_show_thread) {
+			fprintf(output, "S%d-D%d-C%*d ",
+				id.socket, id.die, -3, id.core);
+		} else if (id.cpu.cpu > -1) {
+			fprintf(output, "CPU%*d ",
+				-7, id.cpu.cpu);
+		}
+		break;
+	case AGGR_THREAD:
+		fprintf(output, "%*s-%*d ",
+			16, perf_thread_map__comm(evsel->core.threads, id.thread_idx),
+			-8, perf_thread_map__pid(evsel->core.threads, id.thread_idx));
+		break;
+	case AGGR_GLOBAL:
+	case AGGR_UNSET:
+	case AGGR_MAX:
+	default:
+		break;
+	}
+}
 
-	if (config->json_output && !config->interval)
-		fprintf(config->output, "{");
+static void print_aggr_id_csv(struct perf_stat_config *config,
+			      struct evsel *evsel, struct aggr_cpu_id id, int nr)
+{
+	FILE *output = config->output;
+	const char *sep = config->csv_sep;
 
 	switch (config->aggr_mode) {
 	case AGGR_CORE:
-		if (config->json_output) {
-			fprintf(config->output,
-				"\"core\" : \"S%d-D%d-C%d\", \"aggregate-number\" : %d, ",
-				id.socket,
-				id.die,
-				id.core,
-				nr);
-		} else {
-			fprintf(config->output, "S%d-D%d-C%*d%s%*d%s",
-				id.socket,
-				id.die,
-				config->csv_output ? 0 : -8,
-				id.core,
-				config->csv_sep,
-				config->csv_output ? 0 : 4,
-				nr,
-				config->csv_sep);
-		}
+		fprintf(output, "S%d-D%d-C%d%s%d%s",
+			id.socket, id.die, id.core, sep, nr, sep);
 		break;
 	case AGGR_DIE:
-		if (config->json_output) {
-			fprintf(config->output,
-				"\"die\" : \"S%d-D%d\", \"aggregate-number\" : %d, ",
-				id.socket,
-				id.die,
-				nr);
-		} else {
-			fprintf(config->output, "S%d-D%*d%s%*d%s",
-				id.socket,
-				config->csv_output ? 0 : -8,
-				id.die,
-				config->csv_sep,
-				config->csv_output ? 0 : 4,
-				nr,
-				config->csv_sep);
-		}
+		fprintf(output, "S%d-D%d%s%d%s",
+			id.socket, id.die, sep, nr, sep);
 		break;
 	case AGGR_SOCKET:
-		if (config->json_output) {
-			fprintf(config->output,
-				"\"socket\" : \"S%d\", \"aggregate-number\" : %d, ",
-				id.socket,
-				nr);
-		} else {
-			fprintf(config->output, "S%*d%s%*d%s",
-				config->csv_output ? 0 : -5,
-				id.socket,
-				config->csv_sep,
-				config->csv_output ? 0 : 4,
-				nr,
-				config->csv_sep);
-		}
+		fprintf(output, "S%d%s%d%s",
+			id.socket, sep, nr, sep);
 		break;
 	case AGGR_NODE:
-		if (config->json_output) {
-			fprintf(config->output, "\"node\" : \"N%d\", \"aggregate-number\" : %d, ",
-				id.node,
-				nr);
-		} else {
-			fprintf(config->output, "N%*d%s%*d%s",
-				config->csv_output ? 0 : -5,
-				id.node,
-				config->csv_sep,
-				config->csv_output ? 0 : 4,
-				nr,
-				config->csv_sep);
-		}
+		fprintf(output, "N%d%s%d%s",
+			id.node, sep, nr, sep);
 		break;
 	case AGGR_NONE:
-		if (config->json_output) {
-			if (evsel->percore && !config->percore_show_thread) {
-				fprintf(config->output, "\"core\" : \"S%d-D%d-C%d\"",
-					id.socket,
-					id.die,
-					id.core);
-			} else if (id.cpu.cpu > -1) {
-				fprintf(config->output, "\"cpu\" : \"%d\", ",
-					id.cpu.cpu);
-			}
-		} else {
-			if (evsel->percore && !config->percore_show_thread) {
-				fprintf(config->output, "S%d-D%d-C%*d%s",
-					id.socket,
-					id.die,
-					config->csv_output ? 0 : -3,
-					id.core, config->csv_sep);
-			} else if (id.cpu.cpu > -1) {
-				fprintf(config->output, "CPU%*d%s",
-					config->csv_output ? 0 : -7,
-					id.cpu.cpu, config->csv_sep);
-			}
+		if (evsel->percore && !config->percore_show_thread) {
+			fprintf(output, "S%d-D%d-C%d%s",
+				id.socket, id.die, id.core, sep);
+		} else if (id.cpu.cpu > -1) {
+			fprintf(output, "CPU%d%s",
+				id.cpu.cpu, sep);
 		}
 		break;
 	case AGGR_THREAD:
-		if (config->json_output) {
-			fprintf(config->output, "\"thread\" : \"%s-%d\", ",
-				perf_thread_map__comm(evsel->core.threads, id.thread_idx),
-				perf_thread_map__pid(evsel->core.threads, id.thread_idx));
-		} else {
-			fprintf(config->output, "%*s-%*d%s",
-				config->csv_output ? 0 : 16,
-				perf_thread_map__comm(evsel->core.threads, id.thread_idx),
-				config->csv_output ? 0 : -8,
-				perf_thread_map__pid(evsel->core.threads, id.thread_idx),
-				config->csv_sep);
+		fprintf(output, "%s-%d%s",
+			perf_thread_map__comm(evsel->core.threads, id.thread_idx),
+			perf_thread_map__pid(evsel->core.threads, id.thread_idx),
+			sep);
+		break;
+	case AGGR_GLOBAL:
+	case AGGR_UNSET:
+	case AGGR_MAX:
+	default:
+		break;
+	}
+}
+
+static void print_aggr_id_json(struct perf_stat_config *config,
+			       struct evsel *evsel, struct aggr_cpu_id id, int nr)
+{
+	FILE *output = config->output;
+
+	if (!config->interval)
+		fputc('{', output);
+
+	switch (config->aggr_mode) {
+	case AGGR_CORE:
+		fprintf(output, "\"core\" : \"S%d-D%d-C%d\", \"aggregate-number\" : %d, ",
+			id.socket, id.die, id.core, nr);
+		break;
+	case AGGR_DIE:
+		fprintf(output, "\"die\" : \"S%d-D%d\", \"aggregate-number\" : %d, ",
+			id.socket, id.die, nr);
+		break;
+	case AGGR_SOCKET:
+		fprintf(output, "\"socket\" : \"S%d\", \"aggregate-number\" : %d, ",
+			id.socket, nr);
+		break;
+	case AGGR_NODE:
+		fprintf(output, "\"node\" : \"N%d\", \"aggregate-number\" : %d, ",
+			id.node, nr);
+		break;
+	case AGGR_NONE:
+		if (evsel->percore && !config->percore_show_thread) {
+			fprintf(output, "\"core\" : \"S%d-D%d-C%d\"",
+				id.socket, id.die, id.core);
+		} else if (id.cpu.cpu > -1) {
+			fprintf(output, "\"cpu\" : \"%d\", ",
+				id.cpu.cpu);
 		}
 		break;
+	case AGGR_THREAD:
+		fprintf(output, "\"thread\" : \"%s-%d\", ",
+			perf_thread_map__comm(evsel->core.threads, id.thread_idx),
+			perf_thread_map__pid(evsel->core.threads, id.thread_idx));
+		break;
 	case AGGR_GLOBAL:
 	case AGGR_UNSET:
 	case AGGR_MAX:
@@ -261,6 +272,17 @@ static void aggr_printout(struct perf_stat_config *config,
 	}
 }
 
+static void aggr_printout(struct perf_stat_config *config,
+			  struct evsel *evsel, struct aggr_cpu_id id, int nr)
+{
+	if (config->json_output)
+		print_aggr_id_json(config, evsel, id, nr);
+	else if (config->csv_output)
+		print_aggr_id_csv(config, evsel, id, nr);
+	else
+		print_aggr_id_std(config, evsel, id, nr);
+}
+
 struct outstate {
 	FILE *fh;
 	bool newline;
-- 
2.38.1.493.g58b659f92b-goog


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 06/19] perf stat: Factor out print_counter_value() function
  2022-11-14 23:02 [PATCHSET 00/19] perf stat: Improve perf stat output (v1) Namhyung Kim
                   ` (4 preceding siblings ...)
  2022-11-14 23:02 ` [PATCH 05/19] perf stat: Split aggr_printout() function Namhyung Kim
@ 2022-11-14 23:02 ` Namhyung Kim
  2022-11-14 23:02 ` [PATCH 07/19] perf stat: Handle bad events in abs_printout() Namhyung Kim
                   ` (13 subsequent siblings)
  19 siblings, 0 replies; 21+ messages in thread
From: Namhyung Kim @ 2022-11-14 23:02 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Jiri Olsa
  Cc: Ingo Molnar, Peter Zijlstra, LKML, Ian Rogers, Adrian Hunter,
	linux-perf-users, Kan Liang, Zhengjun Xing, James Clark,
	Athira Jajeev

And split it for each output mode like others.  I believe it makes the
code simpler and more intuitive.  Now abs_printout() becomes just to
call sub-functions.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/stat-display.c | 81 ++++++++++++++++++++++------------
 1 file changed, 53 insertions(+), 28 deletions(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index ed421f6d512f..a72c7442ff3d 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -517,46 +517,71 @@ static void print_metric_header(struct perf_stat_config *config,
 		fprintf(os->fh, "%*s ", config->metric_only_len, unit);
 }
 
-static void abs_printout(struct perf_stat_config *config,
-			 struct aggr_cpu_id id, int nr, struct evsel *evsel, double avg)
+static void print_counter_value_std(struct perf_stat_config *config,
+				    struct evsel *evsel, double avg)
 {
 	FILE *output = config->output;
 	double sc =  evsel->scale;
 	const char *fmt;
 
-	if (config->csv_output) {
-		fmt = floor(sc) != sc ?  "%.2f%s" : "%.0f%s";
-	} else {
-		if (config->big_num)
-			fmt = floor(sc) != sc ? "%'18.2f%s" : "%'18.0f%s";
-		else
-			fmt = floor(sc) != sc ? "%18.2f%s" : "%18.0f%s";
-	}
+	if (config->big_num)
+		fmt = floor(sc) != sc ? "%'18.2f " : "%'18.0f ";
+	else
+		fmt = floor(sc) != sc ? "%18.2f " : "%18.0f ";
 
-	aggr_printout(config, evsel, id, nr);
+	fprintf(output, fmt, avg);
 
-	if (config->json_output)
-		fprintf(output, "\"counter-value\" : \"%f\", ", avg);
-	else
-		fprintf(output, fmt, avg, config->csv_sep);
+	if (evsel->unit)
+		fprintf(output, "%-*s ", config->unit_width, evsel->unit);
 
-	if (config->json_output) {
-		if (evsel->unit) {
-			fprintf(output, "\"unit\" : \"%s\", ",
-				evsel->unit);
-		}
-	} else {
-		if (evsel->unit)
-			fprintf(output, "%-*s%s",
-				config->csv_output ? 0 : config->unit_width,
-				evsel->unit, config->csv_sep);
-	}
+	fprintf(output, "%-*s", 32, evsel__name(evsel));
+}
 
+static void print_counter_value_csv(struct perf_stat_config *config,
+				    struct evsel *evsel, double avg)
+{
+	FILE *output = config->output;
+	double sc =  evsel->scale;
+	const char *sep = config->csv_sep;
+	const char *fmt = floor(sc) != sc ? "%.2f%s" : "%.0f%s";
+
+	fprintf(output, fmt, avg, sep);
+
+	if (evsel->unit)
+		fprintf(output, "%s%s", evsel->unit, sep);
+
+	fprintf(output, "%s", evsel__name(evsel));
+}
+
+static void print_counter_value_json(struct perf_stat_config *config,
+				     struct evsel *evsel, double avg)
+{
+	FILE *output = config->output;
+
+	fprintf(output, "\"counter-value\" : \"%f\", ", avg);
+
+	if (evsel->unit)
+		fprintf(output, "\"unit\" : \"%s\", ", evsel->unit);
+
+	fprintf(output, "\"event\" : \"%s\", ", evsel__name(evsel));
+}
+
+static void print_counter_value(struct perf_stat_config *config,
+				struct evsel *evsel, double avg)
+{
 	if (config->json_output)
-		fprintf(output, "\"event\" : \"%s\", ", evsel__name(evsel));
+		print_counter_value_json(config, evsel, avg);
+	else if (config->csv_output)
+		print_counter_value_csv(config, evsel, avg);
 	else
-		fprintf(output, "%-*s", config->csv_output ? 0 : 32, evsel__name(evsel));
+		print_counter_value_std(config, evsel, avg);
+}
 
+static void abs_printout(struct perf_stat_config *config,
+			 struct aggr_cpu_id id, int nr, struct evsel *evsel, double avg)
+{
+	aggr_printout(config, evsel, id, nr);
+	print_counter_value(config, evsel, avg);
 	print_cgroup(config, evsel);
 }
 
-- 
2.38.1.493.g58b659f92b-goog


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 07/19] perf stat: Handle bad events in abs_printout()
  2022-11-14 23:02 [PATCHSET 00/19] perf stat: Improve perf stat output (v1) Namhyung Kim
                   ` (5 preceding siblings ...)
  2022-11-14 23:02 ` [PATCH 06/19] perf stat: Factor out print_counter_value() function Namhyung Kim
@ 2022-11-14 23:02 ` Namhyung Kim
  2022-11-14 23:02 ` [PATCH 08/19] perf stat: Add before_metric argument Namhyung Kim
                   ` (12 subsequent siblings)
  19 siblings, 0 replies; 21+ messages in thread
From: Namhyung Kim @ 2022-11-14 23:02 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Jiri Olsa
  Cc: Ingo Molnar, Peter Zijlstra, LKML, Ian Rogers, Adrian Hunter,
	linux-perf-users, Kan Liang, Zhengjun Xing, James Clark,
	Athira Jajeev

In the printout() function, it checks if the event is bad (i.e. not
counted or not supported) and print the result.  But it does the same
what abs_printout() is doing.  So add an argument to indicate the value
is ok or not and use the same function in both cases.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/stat-display.c | 68 ++++++++++++++--------------------
 1 file changed, 27 insertions(+), 41 deletions(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index a72c7442ff3d..fe5483893289 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -518,18 +518,22 @@ static void print_metric_header(struct perf_stat_config *config,
 }
 
 static void print_counter_value_std(struct perf_stat_config *config,
-				    struct evsel *evsel, double avg)
+				    struct evsel *evsel, double avg, bool ok)
 {
 	FILE *output = config->output;
 	double sc =  evsel->scale;
 	const char *fmt;
+	const char *bad_count = evsel->supported ? CNTR_NOT_COUNTED : CNTR_NOT_SUPPORTED;
 
 	if (config->big_num)
 		fmt = floor(sc) != sc ? "%'18.2f " : "%'18.0f ";
 	else
 		fmt = floor(sc) != sc ? "%18.2f " : "%18.0f ";
 
-	fprintf(output, fmt, avg);
+	if (ok)
+		fprintf(output, fmt, avg);
+	else
+		fprintf(output, "%18s ", bad_count);
 
 	if (evsel->unit)
 		fprintf(output, "%-*s ", config->unit_width, evsel->unit);
@@ -538,14 +542,18 @@ static void print_counter_value_std(struct perf_stat_config *config,
 }
 
 static void print_counter_value_csv(struct perf_stat_config *config,
-				    struct evsel *evsel, double avg)
+				    struct evsel *evsel, double avg, bool ok)
 {
 	FILE *output = config->output;
 	double sc =  evsel->scale;
 	const char *sep = config->csv_sep;
 	const char *fmt = floor(sc) != sc ? "%.2f%s" : "%.0f%s";
+	const char *bad_count = evsel->supported ? CNTR_NOT_COUNTED : CNTR_NOT_SUPPORTED;
 
-	fprintf(output, fmt, avg, sep);
+	if (ok)
+		fprintf(output, fmt, avg, sep);
+	else
+		fprintf(output, "%s%s", bad_count, sep);
 
 	if (evsel->unit)
 		fprintf(output, "%s%s", evsel->unit, sep);
@@ -554,11 +562,15 @@ static void print_counter_value_csv(struct perf_stat_config *config,
 }
 
 static void print_counter_value_json(struct perf_stat_config *config,
-				     struct evsel *evsel, double avg)
+				     struct evsel *evsel, double avg, bool ok)
 {
 	FILE *output = config->output;
+	const char *bad_count = evsel->supported ? CNTR_NOT_COUNTED : CNTR_NOT_SUPPORTED;
 
-	fprintf(output, "\"counter-value\" : \"%f\", ", avg);
+	if (ok)
+		fprintf(output, "\"counter-value\" : \"%f\", ", avg);
+	else
+		fprintf(output, "\"counter-value\" : \"%s\", ", bad_count);
 
 	if (evsel->unit)
 		fprintf(output, "\"unit\" : \"%s\", ", evsel->unit);
@@ -567,21 +579,22 @@ static void print_counter_value_json(struct perf_stat_config *config,
 }
 
 static void print_counter_value(struct perf_stat_config *config,
-				struct evsel *evsel, double avg)
+				struct evsel *evsel, double avg, bool ok)
 {
 	if (config->json_output)
-		print_counter_value_json(config, evsel, avg);
+		print_counter_value_json(config, evsel, avg, ok);
 	else if (config->csv_output)
-		print_counter_value_csv(config, evsel, avg);
+		print_counter_value_csv(config, evsel, avg, ok);
 	else
-		print_counter_value_std(config, evsel, avg);
+		print_counter_value_std(config, evsel, avg, ok);
 }
 
 static void abs_printout(struct perf_stat_config *config,
-			 struct aggr_cpu_id id, int nr, struct evsel *evsel, double avg)
+			 struct aggr_cpu_id id, int nr,
+			 struct evsel *evsel, double avg, bool ok)
 {
 	aggr_printout(config, evsel, id, nr);
-	print_counter_value(config, evsel, avg);
+	print_counter_value(config, evsel, avg, ok);
 	print_cgroup(config, evsel);
 }
 
@@ -658,17 +671,8 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
 			pm(config, &os, NULL, "", "", 0);
 			return;
 		}
-		aggr_printout(config, counter, id, nr);
 
-		if (config->json_output) {
-			fprintf(config->output, "\"counter-value\" : \"%s\", ",
-					counter->supported ? CNTR_NOT_COUNTED : CNTR_NOT_SUPPORTED);
-		} else {
-			fprintf(config->output, "%*s%s",
-				config->csv_output ? 0 : 18,
-				counter->supported ? CNTR_NOT_COUNTED : CNTR_NOT_SUPPORTED,
-				config->csv_sep);
-		}
+		abs_printout(config, id, nr, counter, uval, /*ok=*/false);
 
 		if (counter->supported) {
 			if (!evlist__has_hybrid(counter->evlist)) {
@@ -678,24 +682,6 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
 			}
 		}
 
-		if (config->json_output) {
-			fprintf(config->output, "\"unit\" : \"%s\", ", counter->unit);
-		} else {
-			fprintf(config->output, "%-*s%s",
-				config->csv_output ? 0 : config->unit_width,
-				counter->unit, config->csv_sep);
-		}
-
-		if (config->json_output) {
-			fprintf(config->output, "\"event\" : \"%s\", ",
-				evsel__name(counter));
-		} else {
-			fprintf(config->output, "%*s",
-				 config->csv_output ? 0 : -25, evsel__name(counter));
-		}
-
-		print_cgroup(config, counter);
-
 		if (!config->csv_output && !config->json_output)
 			pm(config, &os, NULL, NULL, "", 0);
 		print_noise(config, counter, noise);
@@ -706,7 +692,7 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
 	}
 
 	if (!config->metric_only)
-		abs_printout(config, id, nr, counter, uval);
+		abs_printout(config, id, nr, counter, uval, /*ok=*/true);
 
 	out.print_metric = pm;
 	out.new_line = nl;
-- 
2.38.1.493.g58b659f92b-goog


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 08/19] perf stat: Add before_metric argument
  2022-11-14 23:02 [PATCHSET 00/19] perf stat: Improve perf stat output (v1) Namhyung Kim
                   ` (6 preceding siblings ...)
  2022-11-14 23:02 ` [PATCH 07/19] perf stat: Handle bad events in abs_printout() Namhyung Kim
@ 2022-11-14 23:02 ` Namhyung Kim
  2022-11-14 23:02 ` [PATCH 09/19] perf stat: Align cgroup names Namhyung Kim
                   ` (11 subsequent siblings)
  19 siblings, 0 replies; 21+ messages in thread
From: Namhyung Kim @ 2022-11-14 23:02 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Jiri Olsa
  Cc: Ingo Molnar, Peter Zijlstra, LKML, Ian Rogers, Adrian Hunter,
	linux-perf-users, Kan Liang, Zhengjun Xing, James Clark,
	Athira Jajeev

Unfortunately, event running time, percentage and noise data are printed
in different positions in normal output than CSV/JSON.  I think it's
better to put such details in where it actually prints.

So add before_metric argument to print_noise() and print_running() and
call them twice before and after the metric.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/stat-display.c | 82 +++++++++++++++++-----------------
 1 file changed, 42 insertions(+), 40 deletions(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index fe5483893289..bf3f2f9d5dee 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -52,14 +52,18 @@ static void print_running_json(struct perf_stat_config *config, u64 run, u64 ena
 }
 
 static void print_running(struct perf_stat_config *config,
-			  u64 run, u64 ena)
+			  u64 run, u64 ena, bool before_metric)
 {
-	if (config->json_output)
-		print_running_json(config, run, ena);
-	else if (config->csv_output)
-		print_running_csv(config, run, ena);
-	else
-		print_running_std(config, run, ena);
+	if (config->json_output) {
+		if (before_metric)
+			print_running_json(config, run, ena);
+	} else if (config->csv_output) {
+		if (before_metric)
+			print_running_csv(config, run, ena);
+	} else {
+		if (!before_metric)
+			print_running_std(config, run, ena);
+	}
 }
 
 static void print_noise_pct_std(struct perf_stat_config *config,
@@ -82,20 +86,24 @@ static void print_noise_pct_json(struct perf_stat_config *config,
 }
 
 static void print_noise_pct(struct perf_stat_config *config,
-			    double total, double avg)
+			    double total, double avg, bool before_metric)
 {
 	double pct = rel_stddev_stats(total, avg);
 
-	if (config->json_output)
-		print_noise_pct_json(config, pct);
-	else if (config->csv_output)
-		print_noise_pct_csv(config, pct);
-	else
-		print_noise_pct_std(config, pct);
+	if (config->json_output) {
+		if (before_metric)
+			print_noise_pct_json(config, pct);
+	} else if (config->csv_output) {
+		if (before_metric)
+			print_noise_pct_csv(config, pct);
+	} else {
+		if (!before_metric)
+			print_noise_pct_std(config, pct);
+	}
 }
 
 static void print_noise(struct perf_stat_config *config,
-			struct evsel *evsel, double avg)
+			struct evsel *evsel, double avg, bool before_metric)
 {
 	struct perf_stat_evsel *ps;
 
@@ -103,7 +111,7 @@ static void print_noise(struct perf_stat_config *config,
 		return;
 
 	ps = evsel->stats;
-	print_noise_pct(config, stddev_stats(&ps->res_stats), avg);
+	print_noise_pct(config, stddev_stats(&ps->res_stats), avg, before_metric);
 }
 
 static void print_cgroup_std(struct perf_stat_config *config, const char *cgrp_name)
@@ -637,6 +645,7 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
 	};
 	print_metric_t pm;
 	new_line_t nl;
+	bool ok = true;
 
 	if (config->csv_output) {
 		static const int aggr_fields[AGGR_MAX] = {
@@ -672,7 +681,7 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
 			return;
 		}
 
-		abs_printout(config, id, nr, counter, uval, /*ok=*/false);
+		ok = false;
 
 		if (counter->supported) {
 			if (!evlist__has_hybrid(counter->evlist)) {
@@ -681,37 +690,30 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
 					config->print_mixed_hw_group_error = 1;
 			}
 		}
-
-		if (!config->csv_output && !config->json_output)
-			pm(config, &os, NULL, NULL, "", 0);
-		print_noise(config, counter, noise);
-		print_running(config, run, ena);
-		if (config->csv_output || config->json_output)
-			pm(config, &os, NULL, NULL, "", 0);
-		return;
 	}
 
-	if (!config->metric_only)
-		abs_printout(config, id, nr, counter, uval, /*ok=*/true);
-
 	out.print_metric = pm;
 	out.new_line = nl;
 	out.ctx = &os;
 	out.force_header = false;
 
-	if (config->csv_output && !config->metric_only) {
-		print_noise(config, counter, noise);
-		print_running(config, run, ena);
-	} else if (config->json_output && !config->metric_only) {
-		print_noise(config, counter, noise);
-		print_running(config, run, ena);
+	if (!config->metric_only) {
+		abs_printout(config, id, nr, counter, uval, ok);
+
+		print_noise(config, counter, noise, /*before_metric=*/true);
+		print_running(config, run, ena, /*before_metric=*/true);
+	}
+
+	if (ok) {
+		perf_stat__print_shadow_stats(config, counter, uval, map_idx,
+					      &out, &config->metric_events, st);
+	} else {
+		pm(config, &os, /*color=*/NULL, /*format=*/NULL, /*unit=*/"", /*val=*/0);
 	}
 
-	perf_stat__print_shadow_stats(config, counter, uval, map_idx,
-				&out, &config->metric_events, st);
-	if (!config->csv_output && !config->metric_only && !config->json_output) {
-		print_noise(config, counter, noise);
-		print_running(config, run, ena);
+	if (!config->metric_only) {
+		print_noise(config, counter, noise, /*before_metric=*/false);
+		print_running(config, run, ena, /*before_metric=*/false);
 	}
 }
 
@@ -1151,7 +1153,7 @@ static void print_footer(struct perf_stat_config *config)
 		fprintf(output, " %17.*f +- %.*f seconds time elapsed",
 			precision, avg, precision, sd);
 
-		print_noise_pct(config, sd, avg);
+		print_noise_pct(config, sd, avg, /*before_metric=*/false);
 	}
 	fprintf(output, "\n\n");
 
-- 
2.38.1.493.g58b659f92b-goog


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 09/19] perf stat: Align cgroup names
  2022-11-14 23:02 [PATCHSET 00/19] perf stat: Improve perf stat output (v1) Namhyung Kim
                   ` (7 preceding siblings ...)
  2022-11-14 23:02 ` [PATCH 08/19] perf stat: Add before_metric argument Namhyung Kim
@ 2022-11-14 23:02 ` Namhyung Kim
  2022-11-14 23:02 ` [PATCH 10/19] perf stat: Split print_metric_headers() function Namhyung Kim
                   ` (10 subsequent siblings)
  19 siblings, 0 replies; 21+ messages in thread
From: Namhyung Kim @ 2022-11-14 23:02 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Jiri Olsa
  Cc: Ingo Molnar, Peter Zijlstra, LKML, Ian Rogers, Adrian Hunter,
	linux-perf-users, Kan Liang, Zhengjun Xing, James Clark,
	Athira Jajeev

We don't know how long cgroup name is, but at least we can align short
ones like below.

  $ perf stat -a --for-each-cgroup system.slice,user.slice true

   Performance counter stats for 'system wide':

                0.13 msec cpu-clock                        system.slice     #    0.010 CPUs utilized
                   4      context-switches                 system.slice     #   31.989 K/sec
                   1      cpu-migrations                   system.slice     #    7.997 K/sec
                   0      page-faults                      system.slice     #    0.000 /sec
             450,673      cycles                           system.slice     #    3.604 GHz                         (92.41%)
             161,216      instructions                     system.slice     #    0.36  insn per cycle              (92.41%)
              32,678      branches                         system.slice     #  261.332 M/sec                       (92.41%)
               2,628      branch-misses                    system.slice     #    8.04% of all branches             (92.41%)
               14.29 msec cpu-clock                        user.slice       #    1.163 CPUs utilized
                  35      context-switches                 user.slice       #    2.449 K/sec
                  12      cpu-migrations                   user.slice       #  839.691 /sec
                  57      page-faults                      user.slice       #    3.989 K/sec
          49,683,026      cycles                           user.slice       #    3.477 GHz                         (99.38%)
         110,790,266      instructions                     user.slice       #    2.23  insn per cycle              (99.38%)
          24,552,255      branches                         user.slice       #    1.718 G/sec                       (99.38%)
             127,779      branch-misses                    user.slice       #    0.52% of all branches             (99.38%)

         0.012289431 seconds time elapsed

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/stat-display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index bf3f2f9d5dee..e66f766a3d78 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -116,7 +116,7 @@ static void print_noise(struct perf_stat_config *config,
 
 static void print_cgroup_std(struct perf_stat_config *config, const char *cgrp_name)
 {
-	fprintf(config->output, " %s", cgrp_name);
+	fprintf(config->output, " %-16s", cgrp_name);
 }
 
 static void print_cgroup_csv(struct perf_stat_config *config, const char *cgrp_name)
-- 
2.38.1.493.g58b659f92b-goog


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 10/19] perf stat: Split print_metric_headers() function
  2022-11-14 23:02 [PATCHSET 00/19] perf stat: Improve perf stat output (v1) Namhyung Kim
                   ` (8 preceding siblings ...)
  2022-11-14 23:02 ` [PATCH 09/19] perf stat: Align cgroup names Namhyung Kim
@ 2022-11-14 23:02 ` Namhyung Kim
  2022-11-14 23:02 ` [PATCH 11/19] perf stat: Factor out prepare_interval() Namhyung Kim
                   ` (9 subsequent siblings)
  19 siblings, 0 replies; 21+ messages in thread
From: Namhyung Kim @ 2022-11-14 23:02 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Jiri Olsa
  Cc: Ingo Molnar, Peter Zijlstra, LKML, Ian Rogers, Adrian Hunter,
	linux-perf-users, Kan Liang, Zhengjun Xing, James Clark,
	Athira Jajeev

The print_metric_headers() shows metric headers a little bit for each
mode.  Split it out to make the code clearer.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/stat-display.c | 52 ++++++++++++++++++++++++----------
 1 file changed, 37 insertions(+), 15 deletions(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index e66f766a3d78..bb2791459f5f 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -924,6 +924,37 @@ static const char *aggr_header_csv[] = {
 	[AGGR_GLOBAL] 	=	""
 };
 
+static void print_metric_headers_std(struct perf_stat_config *config,
+				     const char *prefix, bool no_indent)
+{
+	if (prefix)
+		fprintf(config->output, "%s", prefix);
+	if (!no_indent) {
+		fprintf(config->output, "%*s",
+			aggr_header_lens[config->aggr_mode], "");
+	}
+}
+
+static void print_metric_headers_csv(struct perf_stat_config *config,
+				     const char *prefix,
+				     bool no_indent __maybe_unused)
+{
+	if (prefix)
+		fprintf(config->output, "%s", prefix);
+	if (config->interval)
+		fputs("time,", config->output);
+	if (!config->iostat_run)
+		fputs(aggr_header_csv[config->aggr_mode], config->output);
+}
+
+static void print_metric_headers_json(struct perf_stat_config *config,
+				      const char *prefix __maybe_unused,
+				      bool no_indent __maybe_unused)
+{
+	if (config->interval)
+		fputs("{\"unit\" : \"sec\"}", config->output);
+}
+
 static void print_metric_headers(struct perf_stat_config *config,
 				 struct evlist *evlist,
 				 const char *prefix, bool no_indent)
@@ -939,22 +970,13 @@ static void print_metric_headers(struct perf_stat_config *config,
 		.force_header = true,
 	};
 
-	if (prefix && !config->json_output)
-		fprintf(config->output, "%s", prefix);
+	if (config->json_output)
+		print_metric_headers_json(config, prefix, no_indent);
+	else if (config->csv_output)
+		print_metric_headers_csv(config, prefix, no_indent);
+	else
+		print_metric_headers_std(config, prefix, no_indent);
 
-	if (!config->csv_output && !config->json_output && !no_indent)
-		fprintf(config->output, "%*s",
-			aggr_header_lens[config->aggr_mode], "");
-	if (config->csv_output) {
-		if (config->interval)
-			fputs("time,", config->output);
-		if (!config->iostat_run)
-			fputs(aggr_header_csv[config->aggr_mode], config->output);
-	}
-	if (config->json_output) {
-		if (config->interval)
-			fputs("{\"unit\" : \"sec\"}", config->output);
-	}
 	if (config->iostat_run)
 		iostat_print_header_prefix(config);
 
-- 
2.38.1.493.g58b659f92b-goog


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 11/19] perf stat: Factor out prepare_interval()
  2022-11-14 23:02 [PATCHSET 00/19] perf stat: Improve perf stat output (v1) Namhyung Kim
                   ` (9 preceding siblings ...)
  2022-11-14 23:02 ` [PATCH 10/19] perf stat: Split print_metric_headers() function Namhyung Kim
@ 2022-11-14 23:02 ` Namhyung Kim
  2022-11-14 23:02 ` [PATCH 12/19] perf stat: Cleanup interval print alignment Namhyung Kim
                   ` (8 subsequent siblings)
  19 siblings, 0 replies; 21+ messages in thread
From: Namhyung Kim @ 2022-11-14 23:02 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Jiri Olsa
  Cc: Ingo Molnar, Peter Zijlstra, LKML, Ian Rogers, Adrian Hunter,
	linux-perf-users, Kan Liang, Zhengjun Xing, James Clark,
	Athira Jajeev

This logic does not print the time directly, but it just puts the
timestamp in the buffer as a prefix.  To reduce the confusion, factor
out the code into a separate function.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/stat-display.c | 39 +++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index bb2791459f5f..c234be656db9 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -993,9 +993,25 @@ static void print_metric_headers(struct perf_stat_config *config,
 	fputc('\n', config->output);
 }
 
+static void prepare_interval(struct perf_stat_config *config,
+			     char *prefix, struct timespec *ts)
+{
+	if (config->iostat_run)
+		return;
+
+	if (!config->json_output)
+		sprintf(prefix, "%6lu.%09lu%s", (unsigned long) ts->tv_sec,
+				 ts->tv_nsec, config->csv_sep);
+	else if (!config->metric_only)
+		sprintf(prefix, "{\"interval\" : %lu.%09lu, ", (unsigned long)
+				 ts->tv_sec, ts->tv_nsec);
+	else
+		sprintf(prefix, "{\"interval\" : %lu.%09lu}", (unsigned long)
+				 ts->tv_sec, ts->tv_nsec);
+}
+
 static void print_interval(struct perf_stat_config *config,
-			   struct evlist *evlist,
-			   char *prefix, struct timespec *ts)
+			   struct evlist *evlist)
 {
 	bool metric_only = config->metric_only;
 	unsigned int unit_width = config->unit_width;
@@ -1005,16 +1021,6 @@ static void print_interval(struct perf_stat_config *config,
 	if (config->interval_clear && isatty(fileno(output)))
 		puts(CONSOLE_CLEAR);
 
-	if (!config->iostat_run && !config->json_output)
-		sprintf(prefix, "%6lu.%09lu%s", (unsigned long) ts->tv_sec,
-				 ts->tv_nsec, config->csv_sep);
-	if (!config->iostat_run && config->json_output && !config->metric_only)
-		sprintf(prefix, "{\"interval\" : %lu.%09lu, ", (unsigned long)
-				 ts->tv_sec, ts->tv_nsec);
-	if (!config->iostat_run && config->json_output && config->metric_only)
-		sprintf(prefix, "{\"interval\" : %lu.%09lu}", (unsigned long)
-				 ts->tv_sec, ts->tv_nsec);
-
 	if ((num_print_interval == 0 || config->interval_clear) &&
 			!config->csv_output && !config->json_output) {
 		switch (config->aggr_mode) {
@@ -1252,10 +1258,13 @@ void evlist__print_counters(struct evlist *evlist, struct perf_stat_config *conf
 	if (config->iostat_run)
 		evlist->selected = evlist__first(evlist);
 
-	if (interval)
-		print_interval(config, evlist, prefix = buf, ts);
-	else
+	if (interval) {
+		prefix = buf;
+		prepare_interval(config, prefix, ts);
+		print_interval(config, evlist);
+	} else {
 		print_header(config, _target, argc, argv);
+	}
 
 	if (metric_only) {
 		static int num_print_iv;
-- 
2.38.1.493.g58b659f92b-goog


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 12/19] perf stat: Cleanup interval print alignment
  2022-11-14 23:02 [PATCHSET 00/19] perf stat: Improve perf stat output (v1) Namhyung Kim
                   ` (10 preceding siblings ...)
  2022-11-14 23:02 ` [PATCH 11/19] perf stat: Factor out prepare_interval() Namhyung Kim
@ 2022-11-14 23:02 ` Namhyung Kim
  2022-11-14 23:02 ` [PATCH 13/19] perf stat: Remove impossible condition Namhyung Kim
                   ` (7 subsequent siblings)
  19 siblings, 0 replies; 21+ messages in thread
From: Namhyung Kim @ 2022-11-14 23:02 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Jiri Olsa
  Cc: Ingo Molnar, Peter Zijlstra, LKML, Ian Rogers, Adrian Hunter,
	linux-perf-users, Kan Liang, Zhengjun Xing, James Clark,
	Athira Jajeev

Instead of using magic values, define symbolic constants and use them.
Also add aggr_header_std[] array to simplify aggr_mode handling.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/stat-display.c | 165 ++++++++++++++++++---------------
 1 file changed, 91 insertions(+), 74 deletions(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index c234be656db9..f983432aaddd 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -25,6 +25,45 @@
 #define CNTR_NOT_SUPPORTED	"<not supported>"
 #define CNTR_NOT_COUNTED	"<not counted>"
 
+#define METRIC_LEN   38
+#define EVNAME_LEN   32
+#define COUNTS_LEN   18
+#define INTERVAL_LEN 16
+#define CGROUP_LEN   16
+#define COMM_LEN     16
+#define PID_LEN       7
+#define CPUS_LEN      4
+
+static int aggr_header_lens[] = {
+	[AGGR_CORE] 	= 18,
+	[AGGR_DIE] 	= 12,
+	[AGGR_SOCKET] 	= 6,
+	[AGGR_NODE] 	= 6,
+	[AGGR_NONE] 	= 6,
+	[AGGR_THREAD] 	= 16,
+	[AGGR_GLOBAL] 	= 0,
+};
+
+static const char *aggr_header_csv[] = {
+	[AGGR_CORE] 	= 	"core,cpus,",
+	[AGGR_DIE] 	= 	"die,cpus,",
+	[AGGR_SOCKET] 	= 	"socket,cpus,",
+	[AGGR_NONE] 	= 	"cpu,",
+	[AGGR_THREAD] 	= 	"comm-pid,",
+	[AGGR_NODE] 	= 	"node,",
+	[AGGR_GLOBAL] 	=	""
+};
+
+static const char *aggr_header_std[] = {
+	[AGGR_CORE] 	= 	"core",
+	[AGGR_DIE] 	= 	"die",
+	[AGGR_SOCKET] 	= 	"socket",
+	[AGGR_NONE] 	= 	"cpu",
+	[AGGR_THREAD] 	= 	"comm-pid",
+	[AGGR_NODE] 	= 	"node",
+	[AGGR_GLOBAL] 	=	""
+};
+
 static void print_running_std(struct perf_stat_config *config, u64 run, u64 ena)
 {
 	if (run != ena)
@@ -116,7 +155,7 @@ static void print_noise(struct perf_stat_config *config,
 
 static void print_cgroup_std(struct perf_stat_config *config, const char *cgrp_name)
 {
-	fprintf(config->output, " %-16s", cgrp_name);
+	fprintf(config->output, " %-*s", CGROUP_LEN, cgrp_name);
 }
 
 static void print_cgroup_csv(struct perf_stat_config *config, const char *cgrp_name)
@@ -147,44 +186,46 @@ static void print_aggr_id_std(struct perf_stat_config *config,
 			      struct evsel *evsel, struct aggr_cpu_id id, int nr)
 {
 	FILE *output = config->output;
+	int idx = config->aggr_mode;
+	char buf[128];
 
 	switch (config->aggr_mode) {
 	case AGGR_CORE:
-		fprintf(output, "S%d-D%d-C%*d %*d ",
-			id.socket, id.die, -8, id.core, 4, nr);
+		snprintf(buf, sizeof(buf), "S%d-D%d-C%d", id.socket, id.die, id.core);
 		break;
 	case AGGR_DIE:
-		fprintf(output, "S%d-D%*d %*d ",
-			id.socket, -8, id.die, 4, nr);
+		snprintf(buf, sizeof(buf), "S%d-D%d", id.socket, id.die);
 		break;
 	case AGGR_SOCKET:
-		fprintf(output, "S%*d %*d ",
-			-5, id.socket, 4, nr);
+		snprintf(buf, sizeof(buf), "S%d", id.socket);
 		break;
 	case AGGR_NODE:
-		fprintf(output, "N%*d %*d ",
-			-5, id.node, 4, nr);
+		snprintf(buf, sizeof(buf), "N%d", id.node);
 		break;
 	case AGGR_NONE:
 		if (evsel->percore && !config->percore_show_thread) {
-			fprintf(output, "S%d-D%d-C%*d ",
-				id.socket, id.die, -3, id.core);
+			snprintf(buf, sizeof(buf), "S%d-D%d-C%d ",
+				id.socket, id.die, id.core);
+			fprintf(output, "%-*s ",
+				aggr_header_lens[AGGR_CORE], buf);
 		} else if (id.cpu.cpu > -1) {
-			fprintf(output, "CPU%*d ",
-				-7, id.cpu.cpu);
+			fprintf(output, "CPU%-*d ",
+				aggr_header_lens[AGGR_NONE] - 3, id.cpu.cpu);
 		}
-		break;
+		return;
 	case AGGR_THREAD:
-		fprintf(output, "%*s-%*d ",
-			16, perf_thread_map__comm(evsel->core.threads, id.thread_idx),
-			-8, perf_thread_map__pid(evsel->core.threads, id.thread_idx));
-		break;
+		fprintf(output, "%*s-%-*d ",
+			COMM_LEN, perf_thread_map__comm(evsel->core.threads, id.thread_idx),
+			PID_LEN, perf_thread_map__pid(evsel->core.threads, id.thread_idx));
+		return;
 	case AGGR_GLOBAL:
 	case AGGR_UNSET:
 	case AGGR_MAX:
 	default:
-		break;
+		return;
 	}
+
+	fprintf(output, "%-*s %*d ", aggr_header_lens[idx], buf, 4, nr);
 }
 
 static void print_aggr_id_csv(struct perf_stat_config *config,
@@ -301,8 +342,6 @@ struct outstate {
 	struct evsel *evsel;
 };
 
-#define METRIC_LEN  38
-
 static void new_line_std(struct perf_stat_config *config __maybe_unused,
 			 void *ctx)
 {
@@ -534,19 +573,19 @@ static void print_counter_value_std(struct perf_stat_config *config,
 	const char *bad_count = evsel->supported ? CNTR_NOT_COUNTED : CNTR_NOT_SUPPORTED;
 
 	if (config->big_num)
-		fmt = floor(sc) != sc ? "%'18.2f " : "%'18.0f ";
+		fmt = floor(sc) != sc ? "%'*.2f " : "%'*.0f ";
 	else
-		fmt = floor(sc) != sc ? "%18.2f " : "%18.0f ";
+		fmt = floor(sc) != sc ? "%*.2f " : "%*.0f ";
 
 	if (ok)
-		fprintf(output, fmt, avg);
+		fprintf(output, fmt, COUNTS_LEN, avg);
 	else
-		fprintf(output, "%18s ", bad_count);
+		fprintf(output, "%*s ", COUNTS_LEN, bad_count);
 
 	if (evsel->unit)
 		fprintf(output, "%-*s ", config->unit_width, evsel->unit);
 
-	fprintf(output, "%-*s", 32, evsel__name(evsel));
+	fprintf(output, "%-*s", EVNAME_LEN, evsel__name(evsel));
 }
 
 static void print_counter_value_csv(struct perf_stat_config *config,
@@ -904,34 +943,19 @@ static void print_no_aggr_metric(struct perf_stat_config *config,
 	}
 }
 
-static int aggr_header_lens[] = {
-	[AGGR_CORE] = 24,
-	[AGGR_DIE] = 18,
-	[AGGR_SOCKET] = 12,
-	[AGGR_NONE] = 6,
-	[AGGR_THREAD] = 24,
-	[AGGR_NODE] = 6,
-	[AGGR_GLOBAL] = 0,
-};
-
-static const char *aggr_header_csv[] = {
-	[AGGR_CORE] 	= 	"core,cpus,",
-	[AGGR_DIE] 	= 	"die,cpus,",
-	[AGGR_SOCKET] 	= 	"socket,cpus,",
-	[AGGR_NONE] 	= 	"cpu,",
-	[AGGR_THREAD] 	= 	"comm-pid,",
-	[AGGR_NODE] 	= 	"node,",
-	[AGGR_GLOBAL] 	=	""
-};
-
 static void print_metric_headers_std(struct perf_stat_config *config,
 				     const char *prefix, bool no_indent)
 {
 	if (prefix)
 		fprintf(config->output, "%s", prefix);
+
 	if (!no_indent) {
-		fprintf(config->output, "%*s",
-			aggr_header_lens[config->aggr_mode], "");
+		int len = aggr_header_lens[config->aggr_mode];
+
+		if (nr_cgroups)
+			len += CGROUP_LEN + 1;
+
+		fprintf(config->output, "%*s", len, "");
 	}
 }
 
@@ -1025,46 +1049,39 @@ static void print_interval(struct perf_stat_config *config,
 			!config->csv_output && !config->json_output) {
 		switch (config->aggr_mode) {
 		case AGGR_NODE:
-			fprintf(output, "#           time node   cpus");
-			if (!metric_only)
-				fprintf(output, "             counts %*s events\n", unit_width, "unit");
-			break;
 		case AGGR_SOCKET:
-			fprintf(output, "#           time socket cpus");
-			if (!metric_only)
-				fprintf(output, "             counts %*s events\n", unit_width, "unit");
-			break;
 		case AGGR_DIE:
-			fprintf(output, "#           time die          cpus");
-			if (!metric_only)
-				fprintf(output, "             counts %*s events\n", unit_width, "unit");
-			break;
 		case AGGR_CORE:
-			fprintf(output, "#           time core            cpus");
-			if (!metric_only)
-				fprintf(output, "             counts %*s events\n", unit_width, "unit");
+			fprintf(output, "#%*s %-*s cpus",
+				INTERVAL_LEN - 1, "time",
+				aggr_header_lens[config->aggr_mode],
+				aggr_header_std[config->aggr_mode]);
 			break;
 		case AGGR_NONE:
-			fprintf(output, "#           time CPU    ");
-			if (!metric_only)
-				fprintf(output, "                counts %*s events\n", unit_width, "unit");
+			fprintf(output, "#%*s %-*s",
+				INTERVAL_LEN - 1, "time",
+				aggr_header_lens[config->aggr_mode],
+				aggr_header_std[config->aggr_mode]);
 			break;
 		case AGGR_THREAD:
-			fprintf(output, "#           time             comm-pid");
-			if (!metric_only)
-				fprintf(output, "                  counts %*s events\n", unit_width, "unit");
+			fprintf(output, "#%*s %*s-%-*s",
+				INTERVAL_LEN - 1, "time",
+				COMM_LEN, "comm", PID_LEN, "pid");
 			break;
 		case AGGR_GLOBAL:
 		default:
-			if (!config->iostat_run) {
-				fprintf(output, "#           time");
-				if (!metric_only)
-					fprintf(output, "             counts %*s events\n", unit_width, "unit");
-			}
+			if (!config->iostat_run)
+				fprintf(output, "#%*s",
+					INTERVAL_LEN - 1, "time");
 		case AGGR_UNSET:
 		case AGGR_MAX:
 			break;
 		}
+
+		if (!metric_only) {
+			fprintf(output, " %*s %*s events\n",
+				COUNTS_LEN, "counts", unit_width, "unit");
+		}
 	}
 
 	if ((num_print_interval == 0 || config->interval_clear) && metric_only)
-- 
2.38.1.493.g58b659f92b-goog


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 13/19] perf stat: Remove impossible condition
  2022-11-14 23:02 [PATCHSET 00/19] perf stat: Improve perf stat output (v1) Namhyung Kim
                   ` (11 preceding siblings ...)
  2022-11-14 23:02 ` [PATCH 12/19] perf stat: Cleanup interval print alignment Namhyung Kim
@ 2022-11-14 23:02 ` Namhyung Kim
  2022-11-14 23:02 ` [PATCH 14/19] perf stat: Rework header display Namhyung Kim
                   ` (6 subsequent siblings)
  19 siblings, 0 replies; 21+ messages in thread
From: Namhyung Kim @ 2022-11-14 23:02 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Jiri Olsa
  Cc: Ingo Molnar, Peter Zijlstra, LKML, Ian Rogers, Adrian Hunter,
	linux-perf-users, Kan Liang, Zhengjun Xing, James Clark,
	Athira Jajeev

The print would run only if metric_only is not set, but it's already in a
block that says it's in metric_only case.  And there's no place to change
the setting.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/stat-display.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index f983432aaddd..cc8bb6d07dcb 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -1292,9 +1292,6 @@ void evlist__print_counters(struct evlist *evlist, struct perf_stat_config *conf
 			num_print_iv = 0;
 		if (config->aggr_mode == AGGR_GLOBAL && prefix && !config->iostat_run)
 			fprintf(config->output, "%s", prefix);
-
-		if (config->json_output && !config->metric_only)
-			fprintf(config->output, "}");
 	}
 
 	switch (config->aggr_mode) {
-- 
2.38.1.493.g58b659f92b-goog


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 14/19] perf stat: Rework header display
  2022-11-14 23:02 [PATCHSET 00/19] perf stat: Improve perf stat output (v1) Namhyung Kim
                   ` (12 preceding siblings ...)
  2022-11-14 23:02 ` [PATCH 13/19] perf stat: Remove impossible condition Namhyung Kim
@ 2022-11-14 23:02 ` Namhyung Kim
  2022-11-14 23:02 ` [PATCH 15/19] perf stat: Move condition to print_footer() Namhyung Kim
                   ` (5 subsequent siblings)
  19 siblings, 0 replies; 21+ messages in thread
From: Namhyung Kim @ 2022-11-14 23:02 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Jiri Olsa
  Cc: Ingo Molnar, Peter Zijlstra, LKML, Ian Rogers, Adrian Hunter,
	linux-perf-users, Kan Liang, Zhengjun Xing, James Clark,
	Athira Jajeev

There are print_header() and print_interval() to print header lines before
actual counter values.  Also print_metric_headers() needs to be called for
the metric-only case.

Let's move all these logics to a single place including num_print_iv to
refresh the headers for interval mode.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/stat-display.c | 185 +++++++++++++++++++--------------
 1 file changed, 106 insertions(+), 79 deletions(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index cc8bb6d07dcb..f97817628478 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -1034,94 +1034,129 @@ static void prepare_interval(struct perf_stat_config *config,
 				 ts->tv_sec, ts->tv_nsec);
 }
 
-static void print_interval(struct perf_stat_config *config,
-			   struct evlist *evlist)
+static void print_header_interval_std(struct perf_stat_config *config,
+				      struct target *_target __maybe_unused,
+				      struct evlist *evlist,
+				      int argc __maybe_unused,
+				      const char **argv __maybe_unused)
 {
-	bool metric_only = config->metric_only;
-	unsigned int unit_width = config->unit_width;
 	FILE *output = config->output;
-	static int num_print_interval;
 
-	if (config->interval_clear && isatty(fileno(output)))
-		puts(CONSOLE_CLEAR);
+	switch (config->aggr_mode) {
+	case AGGR_NODE:
+	case AGGR_SOCKET:
+	case AGGR_DIE:
+	case AGGR_CORE:
+		fprintf(output, "#%*s %-*s cpus",
+			INTERVAL_LEN - 1, "time",
+			aggr_header_lens[config->aggr_mode],
+			aggr_header_std[config->aggr_mode]);
+		break;
+	case AGGR_NONE:
+		fprintf(output, "#%*s %-*s",
+			INTERVAL_LEN - 1, "time",
+			aggr_header_lens[config->aggr_mode],
+			aggr_header_std[config->aggr_mode]);
+		break;
+	case AGGR_THREAD:
+		fprintf(output, "#%*s %*s-%-*s",
+			INTERVAL_LEN - 1, "time",
+			COMM_LEN, "comm", PID_LEN, "pid");
+		break;
+	case AGGR_GLOBAL:
+	default:
+		if (!config->iostat_run)
+			fprintf(output, "#%*s",
+				INTERVAL_LEN - 1, "time");
+	case AGGR_UNSET:
+	case AGGR_MAX:
+		break;
+	}
 
-	if ((num_print_interval == 0 || config->interval_clear) &&
-			!config->csv_output && !config->json_output) {
-		switch (config->aggr_mode) {
-		case AGGR_NODE:
-		case AGGR_SOCKET:
-		case AGGR_DIE:
-		case AGGR_CORE:
-			fprintf(output, "#%*s %-*s cpus",
-				INTERVAL_LEN - 1, "time",
-				aggr_header_lens[config->aggr_mode],
-				aggr_header_std[config->aggr_mode]);
-			break;
-		case AGGR_NONE:
-			fprintf(output, "#%*s %-*s",
-				INTERVAL_LEN - 1, "time",
-				aggr_header_lens[config->aggr_mode],
-				aggr_header_std[config->aggr_mode]);
-			break;
-		case AGGR_THREAD:
-			fprintf(output, "#%*s %*s-%-*s",
-				INTERVAL_LEN - 1, "time",
-				COMM_LEN, "comm", PID_LEN, "pid");
-			break;
-		case AGGR_GLOBAL:
-		default:
-			if (!config->iostat_run)
-				fprintf(output, "#%*s",
-					INTERVAL_LEN - 1, "time");
-		case AGGR_UNSET:
-		case AGGR_MAX:
-			break;
-		}
+	if (config->metric_only)
+		print_metric_headers(config, evlist, " ", true);
+	else
+		fprintf(output, " %*s %*s events\n",
+			COUNTS_LEN, "counts", config->unit_width, "unit");
+}
 
-		if (!metric_only) {
-			fprintf(output, " %*s %*s events\n",
-				COUNTS_LEN, "counts", unit_width, "unit");
-		}
-	}
+static void print_header_std(struct perf_stat_config *config,
+			     struct target *_target, struct evlist *evlist,
+			     int argc, const char **argv)
+{
+	FILE *output = config->output;
+	int i;
+
+	fprintf(output, "\n");
+	fprintf(output, " Performance counter stats for ");
+	if (_target->bpf_str)
+		fprintf(output, "\'BPF program(s) %s", _target->bpf_str);
+	else if (_target->system_wide)
+		fprintf(output, "\'system wide");
+	else if (_target->cpu_list)
+		fprintf(output, "\'CPU(s) %s", _target->cpu_list);
+	else if (!target__has_task(_target)) {
+		fprintf(output, "\'%s", argv ? argv[0] : "pipe");
+		for (i = 1; argv && (i < argc); i++)
+			fprintf(output, " %s", argv[i]);
+	} else if (_target->pid)
+		fprintf(output, "process id \'%s", _target->pid);
+	else
+		fprintf(output, "thread id \'%s", _target->tid);
 
-	if ((num_print_interval == 0 || config->interval_clear) && metric_only)
+	fprintf(output, "\'");
+	if (config->run_count > 1)
+		fprintf(output, " (%d runs)", config->run_count);
+	fprintf(output, ":\n\n");
+
+	if (config->metric_only)
+		print_metric_headers(config, evlist, " ", false);
+}
+
+static void print_header_csv(struct perf_stat_config *config,
+			     struct target *_target __maybe_unused,
+			     struct evlist *evlist,
+			     int argc __maybe_unused,
+			     const char **argv __maybe_unused)
+{
+	if (config->metric_only)
+		print_metric_headers(config, evlist, " ", true);
+}
+static void print_header_json(struct perf_stat_config *config,
+			      struct target *_target __maybe_unused,
+			      struct evlist *evlist,
+			      int argc __maybe_unused,
+			      const char **argv __maybe_unused)
+{
+	if (config->metric_only)
 		print_metric_headers(config, evlist, " ", true);
-	if (++num_print_interval == 25)
-		num_print_interval = 0;
 }
 
 static void print_header(struct perf_stat_config *config,
 			 struct target *_target,
+			 struct evlist *evlist,
 			 int argc, const char **argv)
 {
-	FILE *output = config->output;
-	int i;
+	static int num_print_iv;
 
 	fflush(stdout);
 
-	if (!config->csv_output && !config->json_output) {
-		fprintf(output, "\n");
-		fprintf(output, " Performance counter stats for ");
-		if (_target->bpf_str)
-			fprintf(output, "\'BPF program(s) %s", _target->bpf_str);
-		else if (_target->system_wide)
-			fprintf(output, "\'system wide");
-		else if (_target->cpu_list)
-			fprintf(output, "\'CPU(s) %s", _target->cpu_list);
-		else if (!target__has_task(_target)) {
-			fprintf(output, "\'%s", argv ? argv[0] : "pipe");
-			for (i = 1; argv && (i < argc); i++)
-				fprintf(output, " %s", argv[i]);
-		} else if (_target->pid)
-			fprintf(output, "process id \'%s", _target->pid);
-		else
-			fprintf(output, "thread id \'%s", _target->tid);
+	if (config->interval_clear)
+		puts(CONSOLE_CLEAR);
 
-		fprintf(output, "\'");
-		if (config->run_count > 1)
-			fprintf(output, " (%d runs)", config->run_count);
-		fprintf(output, ":\n\n");
+	if (num_print_iv == 0 || config->interval_clear) {
+		if (config->json_output)
+			print_header_json(config, _target, evlist, argc, argv);
+		else if (config->csv_output)
+			print_header_csv(config, _target, evlist, argc, argv);
+		else if (config->interval)
+			print_header_interval_std(config, _target, evlist, argc, argv);
+		else
+			print_header_std(config, _target, evlist, argc, argv);
 	}
+
+	if (num_print_iv++ == 25)
+		num_print_iv = 0;
 }
 
 static int get_precision(double num)
@@ -1278,18 +1313,10 @@ void evlist__print_counters(struct evlist *evlist, struct perf_stat_config *conf
 	if (interval) {
 		prefix = buf;
 		prepare_interval(config, prefix, ts);
-		print_interval(config, evlist);
-	} else {
-		print_header(config, _target, argc, argv);
 	}
 
+	print_header(config, _target, evlist, argc, argv);
 	if (metric_only) {
-		static int num_print_iv;
-
-		if (num_print_iv == 0 && !interval)
-			print_metric_headers(config, evlist, prefix, false);
-		if (num_print_iv++ == 25)
-			num_print_iv = 0;
 		if (config->aggr_mode == AGGR_GLOBAL && prefix && !config->iostat_run)
 			fprintf(config->output, "%s", prefix);
 	}
-- 
2.38.1.493.g58b659f92b-goog


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 15/19] perf stat: Move condition to print_footer()
  2022-11-14 23:02 [PATCHSET 00/19] perf stat: Improve perf stat output (v1) Namhyung Kim
                   ` (13 preceding siblings ...)
  2022-11-14 23:02 ` [PATCH 14/19] perf stat: Rework header display Namhyung Kim
@ 2022-11-14 23:02 ` Namhyung Kim
  2022-11-14 23:02 ` [PATCH 16/19] perf stat: Factor out prefix display Namhyung Kim
                   ` (4 subsequent siblings)
  19 siblings, 0 replies; 21+ messages in thread
From: Namhyung Kim @ 2022-11-14 23:02 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Jiri Olsa
  Cc: Ingo Molnar, Peter Zijlstra, LKML, Ian Rogers, Adrian Hunter,
	linux-perf-users, Kan Liang, Zhengjun Xing, James Clark,
	Athira Jajeev

Likewise, I think it'd better to have the control inside the function, and keep
the higher level function clearer.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/stat-display.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index f97817628478..73cf898060c0 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -1205,6 +1205,9 @@ static void print_footer(struct perf_stat_config *config)
 	double avg = avg_stats(config->walltime_nsecs_stats) / NSEC_PER_SEC;
 	FILE *output = config->output;
 
+	if (config->interval || config->csv_output || config->json_output)
+		return;
+
 	if (!config->null_run)
 		fprintf(output, "\n");
 
@@ -1359,8 +1362,7 @@ void evlist__print_counters(struct evlist *evlist, struct perf_stat_config *conf
 		break;
 	}
 
-	if (!interval && !config->csv_output && !config->json_output)
-		print_footer(config);
+	print_footer(config);
 
 	fflush(config->output);
 }
-- 
2.38.1.493.g58b659f92b-goog


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 16/19] perf stat: Factor out prefix display
  2022-11-14 23:02 [PATCHSET 00/19] perf stat: Improve perf stat output (v1) Namhyung Kim
                   ` (14 preceding siblings ...)
  2022-11-14 23:02 ` [PATCH 15/19] perf stat: Move condition to print_footer() Namhyung Kim
@ 2022-11-14 23:02 ` Namhyung Kim
  2022-11-14 23:02 ` [PATCH 17/19] perf stat: Factor out print_metric_{begin,end}() Namhyung Kim
                   ` (3 subsequent siblings)
  19 siblings, 0 replies; 21+ messages in thread
From: Namhyung Kim @ 2022-11-14 23:02 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Jiri Olsa
  Cc: Ingo Molnar, Peter Zijlstra, LKML, Ian Rogers, Adrian Hunter,
	linux-perf-users, Kan Liang, Zhengjun Xing, James Clark,
	Athira Jajeev

The prefix is needed for interval mode to print timestamp at the
beginning of each line.  But the it's tricky for the metric only
mode since it doesn't print every evsel and combines the metrics
into a single line.

So it needed to pass 'first' argument to print_counter_aggrdata()
to determine if the current event is being printed at first.  This
makes the code hard to read.

Let's move the logic out of the function and do it in the outer
print loop.  This would enable further cleanups later.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/stat-display.c | 43 ++++++++++++----------------------
 1 file changed, 15 insertions(+), 28 deletions(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index 73cf898060c0..bb40ed29300d 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -805,8 +805,7 @@ static void uniquify_counter(struct perf_stat_config *config, struct evsel *coun
 
 static void print_counter_aggrdata(struct perf_stat_config *config,
 				   struct evsel *counter, int s,
-				   char *prefix, bool metric_only,
-				   bool *first)
+				   char *prefix, bool metric_only)
 {
 	FILE *output = config->output;
 	u64 ena, run, val;
@@ -825,10 +824,6 @@ static void print_counter_aggrdata(struct perf_stat_config *config,
 	ena = aggr->counts.ena;
 	run = aggr->counts.run;
 
-	if (*first && metric_only) {
-		*first = false;
-		aggr_printout(config, counter, id, aggr->nr);
-	}
 	if (prefix && !metric_only)
 		fprintf(output, "%s", prefix);
 
@@ -849,7 +844,6 @@ static void print_aggr(struct perf_stat_config *config,
 	FILE *output = config->output;
 	struct evsel *counter;
 	int s;
-	bool first;
 
 	if (!config->aggr_map || !config->aggr_get_id)
 		return;
@@ -860,21 +854,23 @@ static void print_aggr(struct perf_stat_config *config,
 	 */
 	for (s = 0; s < config->aggr_map->nr; s++) {
 		if (metric_only) {
+			struct perf_stat_aggr *aggr;
+			struct aggr_cpu_id id = config->aggr_map->map[s];
+
 			if (prefix)
 				fprintf(output, "%s", prefix);
-			else if (config->summary && !config->no_csv_summary &&
-				 config->csv_output && !config->interval)
-				fprintf(output, "%16s%s", "summary", config->csv_sep);
+
+			counter = evlist__first(evlist);
+			aggr = &counter->stats->aggr[s];
+			aggr_printout(config, counter, id, aggr->nr);
 		}
 
-		first = true;
 		evlist__for_each_entry(evlist, counter) {
 			if (counter->merged_stat)
 				continue;
 
-			print_counter_aggrdata(config, counter, s,
-					       prefix, metric_only,
-					       &first);
+			print_counter_aggrdata(config, counter, s, prefix,
+					       metric_only);
 		}
 		if (metric_only)
 			fputc('\n', output);
@@ -885,7 +881,6 @@ static void print_counter(struct perf_stat_config *config,
 			  struct evsel *counter, char *prefix)
 {
 	bool metric_only = config->metric_only;
-	bool first = false;
 	int s;
 
 	/* AGGR_THREAD doesn't have config->aggr_get_id */
@@ -896,9 +891,8 @@ static void print_counter(struct perf_stat_config *config,
 		return;
 
 	for (s = 0; s < config->aggr_map->nr; s++) {
-		print_counter_aggrdata(config, counter, s,
-				       prefix, metric_only,
-				       &first);
+		print_counter_aggrdata(config, counter, s, prefix,
+				       metric_only);
 	}
 }
 
@@ -1260,7 +1254,6 @@ static void print_percore(struct perf_stat_config *config,
 	FILE *output = config->output;
 	struct cpu_aggr_map *core_map;
 	int s, c, i;
-	bool first = true;
 
 	if (!config->aggr_map || !config->aggr_get_id)
 		return;
@@ -1288,11 +1281,7 @@ static void print_percore(struct perf_stat_config *config,
 		if (found)
 			continue;
 
-		if (prefix && metric_only)
-			fprintf(output, "%s", prefix);
-
-		print_counter_aggrdata(config, counter, s,
-				       prefix, metric_only, &first);
+		print_counter_aggrdata(config, counter, s, prefix, metric_only);
 
 		core_map->map[c++] = core_id;
 	}
@@ -1319,10 +1308,6 @@ void evlist__print_counters(struct evlist *evlist, struct perf_stat_config *conf
 	}
 
 	print_header(config, _target, evlist, argc, argv);
-	if (metric_only) {
-		if (config->aggr_mode == AGGR_GLOBAL && prefix && !config->iostat_run)
-			fprintf(config->output, "%s", prefix);
-	}
 
 	switch (config->aggr_mode) {
 	case AGGR_CORE:
@@ -1337,6 +1322,8 @@ void evlist__print_counters(struct evlist *evlist, struct perf_stat_config *conf
 			iostat_print_counters(evlist, config, ts, prefix = buf,
 					      print_counter);
 		else {
+			if (prefix && metric_only)
+				fprintf(config->output, "%s", prefix);
 			evlist__for_each_entry(evlist, counter) {
 				print_counter(config, counter, prefix);
 			}
-- 
2.38.1.493.g58b659f92b-goog


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 17/19] perf stat: Factor out print_metric_{begin,end}()
  2022-11-14 23:02 [PATCHSET 00/19] perf stat: Improve perf stat output (v1) Namhyung Kim
                   ` (15 preceding siblings ...)
  2022-11-14 23:02 ` [PATCH 16/19] perf stat: Factor out prefix display Namhyung Kim
@ 2022-11-14 23:02 ` Namhyung Kim
  2022-11-14 23:02 ` [PATCH 18/19] perf stat: Support --for-each-cgroup and --metric-only Namhyung Kim
                   ` (2 subsequent siblings)
  19 siblings, 0 replies; 21+ messages in thread
From: Namhyung Kim @ 2022-11-14 23:02 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Jiri Olsa
  Cc: Ingo Molnar, Peter Zijlstra, LKML, Ian Rogers, Adrian Hunter,
	linux-perf-users, Kan Liang, Zhengjun Xing, James Clark,
	Athira Jajeev

For the metric-only case, add new functions to handle the start and the
end of each metric display.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/stat-display.c | 56 +++++++++++++++++++++-------------
 1 file changed, 34 insertions(+), 22 deletions(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index bb40ed29300d..7a0673be720b 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -836,12 +836,39 @@ static void print_counter_aggrdata(struct perf_stat_config *config,
 		fputc('\n', output);
 }
 
+static void print_metric_begin(struct perf_stat_config *config,
+			       struct evlist *evlist,
+			       char *prefix, int aggr_idx)
+{
+	struct perf_stat_aggr *aggr;
+	struct aggr_cpu_id id;
+	struct evsel *evsel;
+
+	if (!config->metric_only)
+		return;
+
+	if (prefix)
+		fprintf(config->output, "%s", prefix);
+
+	evsel = evlist__first(evlist);
+	id = config->aggr_map->map[aggr_idx];
+	aggr = &evsel->stats->aggr[aggr_idx];
+	aggr_printout(config, evsel, id, aggr->nr);
+}
+
+static void print_metric_end(struct perf_stat_config *config)
+{
+	if (!config->metric_only)
+		return;
+
+	fputc('\n', config->output);
+}
+
 static void print_aggr(struct perf_stat_config *config,
 		       struct evlist *evlist,
 		       char *prefix)
 {
 	bool metric_only = config->metric_only;
-	FILE *output = config->output;
 	struct evsel *counter;
 	int s;
 
@@ -853,17 +880,7 @@ static void print_aggr(struct perf_stat_config *config,
 	 * Without each counter has its own line.
 	 */
 	for (s = 0; s < config->aggr_map->nr; s++) {
-		if (metric_only) {
-			struct perf_stat_aggr *aggr;
-			struct aggr_cpu_id id = config->aggr_map->map[s];
-
-			if (prefix)
-				fprintf(output, "%s", prefix);
-
-			counter = evlist__first(evlist);
-			aggr = &counter->stats->aggr[s];
-			aggr_printout(config, counter, id, aggr->nr);
-		}
+		print_metric_begin(config, evlist, prefix, s);
 
 		evlist__for_each_entry(evlist, counter) {
 			if (counter->merged_stat)
@@ -872,8 +889,7 @@ static void print_aggr(struct perf_stat_config *config,
 			print_counter_aggrdata(config, counter, s, prefix,
 					       metric_only);
 		}
-		if (metric_only)
-			fputc('\n', output);
+		print_metric_end(config);
 	}
 }
 
@@ -919,9 +935,7 @@ static void print_no_aggr_metric(struct perf_stat_config *config,
 
 			id = aggr_cpu_id__cpu(cpu, /*data=*/NULL);
 			if (first) {
-				if (prefix)
-					fputs(prefix, config->output);
-				aggr_printout(config, counter, id, 0);
+				print_metric_begin(config, evlist, prefix, counter_idx);
 				first = false;
 			}
 			val = ps->aggr[counter_idx].counts.val;
@@ -933,7 +947,7 @@ static void print_no_aggr_metric(struct perf_stat_config *config,
 				 run, ena, 1.0, &rt_stat, counter_idx);
 		}
 		if (!first)
-			fputc('\n', config->output);
+			print_metric_end(config);
 	}
 }
 
@@ -1322,13 +1336,11 @@ void evlist__print_counters(struct evlist *evlist, struct perf_stat_config *conf
 			iostat_print_counters(evlist, config, ts, prefix = buf,
 					      print_counter);
 		else {
-			if (prefix && metric_only)
-				fprintf(config->output, "%s", prefix);
+			print_metric_begin(config, evlist, prefix, /*aggr_idx=*/0);
 			evlist__for_each_entry(evlist, counter) {
 				print_counter(config, counter, prefix);
 			}
-			if (metric_only)
-				fputc('\n', config->output);
+			print_metric_end(config);
 		}
 		break;
 	case AGGR_NONE:
-- 
2.38.1.493.g58b659f92b-goog


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 18/19] perf stat: Support --for-each-cgroup and --metric-only
  2022-11-14 23:02 [PATCHSET 00/19] perf stat: Improve perf stat output (v1) Namhyung Kim
                   ` (16 preceding siblings ...)
  2022-11-14 23:02 ` [PATCH 17/19] perf stat: Factor out print_metric_{begin,end}() Namhyung Kim
@ 2022-11-14 23:02 ` Namhyung Kim
  2022-11-14 23:02 ` [PATCH 19/19] perf stat: Add print_aggr_cgroup() for --for-each-cgroup and --topdown Namhyung Kim
  2022-11-15 14:04 ` [PATCHSET 00/19] perf stat: Improve perf stat output (v1) Arnaldo Carvalho de Melo
  19 siblings, 0 replies; 21+ messages in thread
From: Namhyung Kim @ 2022-11-14 23:02 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Jiri Olsa
  Cc: Ingo Molnar, Peter Zijlstra, LKML, Ian Rogers, Adrian Hunter,
	linux-perf-users, Kan Liang, Zhengjun Xing, James Clark,
	Athira Jajeev

When we have events for each cgroup, the metric should be printed for
each cgroup separately.  Add print_cgroup_counter() to handle that
situation properly.

Also change print_metric_headers() not to print duplicate headers
by checking cgroups.

  $ perf stat -a --for-each-cgroup system.slice,user.slice --metric-only sleep 1

   Performance counter stats for 'system wide':

                                     GHz       insn per cycle branch-misses of all branches
   system.slice                   3.792                0.61                                3.24%
   user.slice                     3.661                2.32                                0.37%

         1.016111516 seconds time elapsed

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/stat-display.c | 58 +++++++++++++++++++++++++++-------
 1 file changed, 47 insertions(+), 11 deletions(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index 7a0673be720b..cf25ed99b5df 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -168,10 +168,10 @@ static void print_cgroup_json(struct perf_stat_config *config, const char *cgrp_
 	fprintf(config->output, "\"cgroup\" : \"%s\", ", cgrp_name);
 }
 
-static void print_cgroup(struct perf_stat_config *config, struct evsel *evsel)
+static void print_cgroup(struct perf_stat_config *config, struct cgroup *cgrp)
 {
-	if (nr_cgroups) {
-		const char *cgrp_name = evsel->cgrp ? evsel->cgrp->name  : "";
+	if (nr_cgroups || config->cgroup_list) {
+		const char *cgrp_name = cgrp ? cgrp->name  : "";
 
 		if (config->json_output)
 			print_cgroup_json(config, cgrp_name);
@@ -340,6 +340,7 @@ struct outstate {
 	int  nr;
 	struct aggr_cpu_id id;
 	struct evsel *evsel;
+	struct cgroup *cgrp;
 };
 
 static void new_line_std(struct perf_stat_config *config __maybe_unused,
@@ -552,6 +553,9 @@ static void print_metric_header(struct perf_stat_config *config,
 	    os->evsel->priv != os->evsel->evlist->selected->priv)
 		return;
 
+	if (os->evsel->cgrp != os->cgrp)
+		return;
+
 	if (!valid_only_metric(unit))
 		return;
 	unit = fixunit(tbuf, os->evsel, unit);
@@ -642,7 +646,7 @@ static void abs_printout(struct perf_stat_config *config,
 {
 	aggr_printout(config, evsel, id, nr);
 	print_counter_value(config, evsel, avg, ok);
-	print_cgroup(config, evsel);
+	print_cgroup(config, evsel->cgrp);
 }
 
 static bool is_mixed_hw_group(struct evsel *counter)
@@ -838,7 +842,8 @@ static void print_counter_aggrdata(struct perf_stat_config *config,
 
 static void print_metric_begin(struct perf_stat_config *config,
 			       struct evlist *evlist,
-			       char *prefix, int aggr_idx)
+			       char *prefix, int aggr_idx,
+			       struct cgroup *cgrp)
 {
 	struct perf_stat_aggr *aggr;
 	struct aggr_cpu_id id;
@@ -854,6 +859,8 @@ static void print_metric_begin(struct perf_stat_config *config,
 	id = config->aggr_map->map[aggr_idx];
 	aggr = &evsel->stats->aggr[aggr_idx];
 	aggr_printout(config, evsel, id, aggr->nr);
+
+	print_cgroup(config, cgrp);
 }
 
 static void print_metric_end(struct perf_stat_config *config)
@@ -880,7 +887,7 @@ static void print_aggr(struct perf_stat_config *config,
 	 * Without each counter has its own line.
 	 */
 	for (s = 0; s < config->aggr_map->nr; s++) {
-		print_metric_begin(config, evlist, prefix, s);
+		print_metric_begin(config, evlist, prefix, s, /*cgrp=*/NULL);
 
 		evlist__for_each_entry(evlist, counter) {
 			if (counter->merged_stat)
@@ -935,7 +942,8 @@ static void print_no_aggr_metric(struct perf_stat_config *config,
 
 			id = aggr_cpu_id__cpu(cpu, /*data=*/NULL);
 			if (first) {
-				print_metric_begin(config, evlist, prefix, counter_idx);
+				print_metric_begin(config, evlist, prefix,
+						   counter_idx, /*cgrp=*/NULL);
 				first = false;
 			}
 			val = ps->aggr[counter_idx].counts.val;
@@ -960,7 +968,7 @@ static void print_metric_headers_std(struct perf_stat_config *config,
 	if (!no_indent) {
 		int len = aggr_header_lens[config->aggr_mode];
 
-		if (nr_cgroups)
+		if (nr_cgroups || config->cgroup_list)
 			len += CGROUP_LEN + 1;
 
 		fprintf(config->output, "%*s", len, "");
@@ -1012,6 +1020,9 @@ static void print_metric_headers(struct perf_stat_config *config,
 	if (config->iostat_run)
 		iostat_print_header_prefix(config);
 
+	if (config->cgroup_list)
+		os.cgrp = evlist__first(evlist)->cgrp;
+
 	/* Print metrics headers only */
 	evlist__for_each_entry(evlist, counter) {
 		os.evsel = counter;
@@ -1305,6 +1316,28 @@ static void print_percore(struct perf_stat_config *config,
 		fputc('\n', output);
 }
 
+static void print_cgroup_counter(struct perf_stat_config *config, struct evlist *evlist,
+				 char *prefix)
+{
+	struct cgroup *cgrp = NULL;
+	struct evsel *counter;
+
+	evlist__for_each_entry(evlist, counter) {
+		if (cgrp != counter->cgrp) {
+			if (cgrp != NULL)
+				print_metric_end(config);
+
+			cgrp = counter->cgrp;
+			print_metric_begin(config, evlist, prefix,
+					   /*aggr_idx=*/0, cgrp);
+		}
+
+		print_counter(config, counter, prefix);
+	}
+	if (cgrp)
+		print_metric_end(config);
+}
+
 void evlist__print_counters(struct evlist *evlist, struct perf_stat_config *config,
 			    struct target *_target, struct timespec *ts, int argc, const char **argv)
 {
@@ -1332,11 +1365,14 @@ void evlist__print_counters(struct evlist *evlist, struct perf_stat_config *conf
 		break;
 	case AGGR_THREAD:
 	case AGGR_GLOBAL:
-		if (config->iostat_run)
+		if (config->iostat_run) {
 			iostat_print_counters(evlist, config, ts, prefix = buf,
 					      print_counter);
-		else {
-			print_metric_begin(config, evlist, prefix, /*aggr_idx=*/0);
+		} else if (config->cgroup_list) {
+			print_cgroup_counter(config, evlist, prefix);
+		} else {
+			print_metric_begin(config, evlist, prefix,
+					   /*aggr_idx=*/0, /*cgrp=*/NULL);
 			evlist__for_each_entry(evlist, counter) {
 				print_counter(config, counter, prefix);
 			}
-- 
2.38.1.493.g58b659f92b-goog


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 19/19] perf stat: Add print_aggr_cgroup() for --for-each-cgroup and --topdown
  2022-11-14 23:02 [PATCHSET 00/19] perf stat: Improve perf stat output (v1) Namhyung Kim
                   ` (17 preceding siblings ...)
  2022-11-14 23:02 ` [PATCH 18/19] perf stat: Support --for-each-cgroup and --metric-only Namhyung Kim
@ 2022-11-14 23:02 ` Namhyung Kim
  2022-11-15 14:04 ` [PATCHSET 00/19] perf stat: Improve perf stat output (v1) Arnaldo Carvalho de Melo
  19 siblings, 0 replies; 21+ messages in thread
From: Namhyung Kim @ 2022-11-14 23:02 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo, Jiri Olsa
  Cc: Ingo Molnar, Peter Zijlstra, LKML, Ian Rogers, Adrian Hunter,
	linux-perf-users, Kan Liang, Zhengjun Xing, James Clark,
	Athira Jajeev

Normally, --for-each-cgroup only works with AGGR_GLOBAL.  However
the --topdown on some cpu (e.g. Intel Skylake) converts it to the
AGGR_CORE internally.

To support those machines, add print_aggr_cgroup and handle the events
like in print_cgroup_events().

  $ perf stat -a --for-each-cgroup system.slice,user.slice --topdown sleep 1
  nmi_watchdog enabled with topdown. May give wrong results.
  Disable with echo 0 > /proc/sys/kernel/nmi_watchdog

   Performance counter stats for 'system wide':

                                                  retiring      bad speculation       frontend bound        backend bound
  S0-D0-C0              2  system.slice                   49.0%               -46.6%                31.4%
  S0-D0-C1              2  system.slice                   55.5%                 8.0%                45.5%                -9.0%
  S0-D0-C2              2  system.slice                   87.8%                22.1%                30.3%               -40.3%
  S0-D0-C3              2  system.slice                   53.3%               -11.9%                45.2%                13.4%
  S0-D0-C0              2  user.slice                    123.5%                 4.0%                48.5%               -75.9%
  S0-D0-C1              2  user.slice                     19.9%                 6.5%                89.9%               -16.3%
  S0-D0-C2              2  user.slice                     29.9%                 7.9%                71.3%                -9.1%
  S0-D0-C3              2  user.slice                     28.0%                 7.2%                43.3%                21.5%

         1.004136937 seconds time elapsed

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/stat-display.c | 41 +++++++++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index cf25ed99b5df..f5501760ff2e 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -900,6 +900,42 @@ static void print_aggr(struct perf_stat_config *config,
 	}
 }
 
+static void print_aggr_cgroup(struct perf_stat_config *config,
+			      struct evlist *evlist,
+			      char *prefix)
+{
+	bool metric_only = config->metric_only;
+	struct evsel *counter, *evsel;
+	struct cgroup *cgrp = NULL;
+	int s;
+
+	if (!config->aggr_map || !config->aggr_get_id)
+		return;
+
+	evlist__for_each_entry(evlist, evsel) {
+		if (cgrp == evsel->cgrp)
+			continue;
+
+		cgrp = evsel->cgrp;
+
+		for (s = 0; s < config->aggr_map->nr; s++) {
+			print_metric_begin(config, evlist, prefix, s, cgrp);
+
+			evlist__for_each_entry(evlist, counter) {
+				if (counter->merged_stat)
+					continue;
+
+				if (counter->cgrp != cgrp)
+					continue;
+
+				print_counter_aggrdata(config, counter, s, prefix,
+						       metric_only);
+			}
+			print_metric_end(config);
+		}
+	}
+}
+
 static void print_counter(struct perf_stat_config *config,
 			  struct evsel *counter, char *prefix)
 {
@@ -1361,7 +1397,10 @@ void evlist__print_counters(struct evlist *evlist, struct perf_stat_config *conf
 	case AGGR_DIE:
 	case AGGR_SOCKET:
 	case AGGR_NODE:
-		print_aggr(config, evlist, prefix);
+		if (config->cgroup_list)
+			print_aggr_cgroup(config, evlist, prefix);
+		else
+			print_aggr(config, evlist, prefix);
 		break;
 	case AGGR_THREAD:
 	case AGGR_GLOBAL:
-- 
2.38.1.493.g58b659f92b-goog


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCHSET 00/19] perf stat: Improve perf stat output (v1)
  2022-11-14 23:02 [PATCHSET 00/19] perf stat: Improve perf stat output (v1) Namhyung Kim
                   ` (18 preceding siblings ...)
  2022-11-14 23:02 ` [PATCH 19/19] perf stat: Add print_aggr_cgroup() for --for-each-cgroup and --topdown Namhyung Kim
@ 2022-11-15 14:04 ` Arnaldo Carvalho de Melo
  19 siblings, 0 replies; 21+ messages in thread
From: Arnaldo Carvalho de Melo @ 2022-11-15 14:04 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Jiri Olsa, Ingo Molnar, Peter Zijlstra, LKML, Ian Rogers,
	Adrian Hunter, linux-perf-users, Kan Liang, Zhengjun Xing,
	James Clark, Athira Jajeev

Em Mon, Nov 14, 2022 at 03:02:08PM -0800, Namhyung Kim escreveu:
> Hello,
> 
> I'm working on cleanup up the perf stat code.  The main focus this time
> is the display logic which has various combinations of options.
> 
> I split the code for each output mode - std, csv and json.  And then
> organize them according to the purpose like header, prefix, value,
> metric and footer.  I hope this would help maintaining the code a bit
> more.
> 
> Also I found that cgroup support is missing or insufficient.
> Specifically when --for-each-cgroup option is given, it'd have multiple
> copies of the events for those cgroups.  Then the output should group
> the result.  This is true for the normal output mode, but the metric-
> only mode didn't support it well.
> 
> With this change, I can see the per-cgroup topdown metrics like below:
> 
>   $ sudo ./perf stat -a --topdown --for-each-cgroup user.slice,system.slice sleep 3
>   nmi_watchdog enabled with topdown. May give wrong results.
>   Disable with echo 0 > /proc/sys/kernel/nmi_watchdog
> 
>    Performance counter stats for 'system wide':
> 
>                                   retiring      bad speculation   frontend bound    backend bound
>   S0-D0-C0      2  user.slice            117.3%         3.9%            47.8%           -69.1%
>   S0-D0-C1      2  user.slice            119.8%         4.1%            49.3%           -73.2%
>   S0-D0-C2      2  user.slice             24.4%         7.9%            68.4%             0.0%
>   S0-D0-C3      2  user.slice             24.0%         9.2%            91.2%           -24.4%
>   S0-D0-C0      2  system.slice           73.5%         4.0%            19.4%             3.1%
>   S0-D0-C1      2  system.slice           90.0%         5.8%            19.7%           -15.5%
>   S0-D0-C2      2  system.slice          101.2%         6.6%            33.4%           -41.1%
>   S0-D0-C3      2  system.slice           90.7%         4.9%            22.3%           -18.0%
> 
>          3.001678216 seconds time elapsed
> 
> You can get it from 'perf/stat-display-v1' branch in
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git

applied locally, build testing.

- Arnaldo
 
> Thanks,
> Namhyung
> 
> Namhyung Kim (19):
>   perf stat: Clear screen only if output file is a tty
>   perf stat: Split print_running() function
>   perf stat: Split print_noise_pct() function
>   perf stat: Split print_cgroup() function
>   perf stat: Split aggr_printout() function
>   perf stat: Factor out print_counter_value() function
>   perf stat: Handle bad events in abs_printout()
>   perf stat: Add before_metric argument
>   perf stat: Align cgroup names
>   perf stat: Split print_metric_headers() function
>   perf stat: Factor out prepare_interval()
>   perf stat: Cleanup interval print alignment
>   perf stat: Remove impossible condition
>   perf stat: Rework header display
>   perf stat: Move condition to print_footer()
>   perf stat: Factor out prefix display
>   perf stat: Factor out print_metric_{begin,end}()
>   perf stat: Support --for-each-cgroup and --metric-only
>   perf stat: Add print_aggr_cgroup() for --for-each-cgroup and --topdown
> 
>  tools/perf/builtin-stat.c      |   8 +
>  tools/perf/util/stat-display.c | 996 ++++++++++++++++++++-------------
>  2 files changed, 624 insertions(+), 380 deletions(-)
> 
> 
> base-commit: 7565f9617efac0c0c8e2dbd08dbe0695d56684f5
> -- 
> 2.38.1.493.g58b659f92b-goog

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2022-11-15 14:04 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-14 23:02 [PATCHSET 00/19] perf stat: Improve perf stat output (v1) Namhyung Kim
2022-11-14 23:02 ` [PATCH 01/19] perf stat: Clear screen only if output file is a tty Namhyung Kim
2022-11-14 23:02 ` [PATCH 02/19] perf stat: Split print_running() function Namhyung Kim
2022-11-14 23:02 ` [PATCH 03/19] perf stat: Split print_noise_pct() function Namhyung Kim
2022-11-14 23:02 ` [PATCH 04/19] perf stat: Split print_cgroup() function Namhyung Kim
2022-11-14 23:02 ` [PATCH 05/19] perf stat: Split aggr_printout() function Namhyung Kim
2022-11-14 23:02 ` [PATCH 06/19] perf stat: Factor out print_counter_value() function Namhyung Kim
2022-11-14 23:02 ` [PATCH 07/19] perf stat: Handle bad events in abs_printout() Namhyung Kim
2022-11-14 23:02 ` [PATCH 08/19] perf stat: Add before_metric argument Namhyung Kim
2022-11-14 23:02 ` [PATCH 09/19] perf stat: Align cgroup names Namhyung Kim
2022-11-14 23:02 ` [PATCH 10/19] perf stat: Split print_metric_headers() function Namhyung Kim
2022-11-14 23:02 ` [PATCH 11/19] perf stat: Factor out prepare_interval() Namhyung Kim
2022-11-14 23:02 ` [PATCH 12/19] perf stat: Cleanup interval print alignment Namhyung Kim
2022-11-14 23:02 ` [PATCH 13/19] perf stat: Remove impossible condition Namhyung Kim
2022-11-14 23:02 ` [PATCH 14/19] perf stat: Rework header display Namhyung Kim
2022-11-14 23:02 ` [PATCH 15/19] perf stat: Move condition to print_footer() Namhyung Kim
2022-11-14 23:02 ` [PATCH 16/19] perf stat: Factor out prefix display Namhyung Kim
2022-11-14 23:02 ` [PATCH 17/19] perf stat: Factor out print_metric_{begin,end}() Namhyung Kim
2022-11-14 23:02 ` [PATCH 18/19] perf stat: Support --for-each-cgroup and --metric-only Namhyung Kim
2022-11-14 23:02 ` [PATCH 19/19] perf stat: Add print_aggr_cgroup() for --for-each-cgroup and --topdown Namhyung Kim
2022-11-15 14:04 ` [PATCHSET 00/19] perf stat: Improve perf stat output (v1) Arnaldo Carvalho de Melo

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.