[PATCHSET 00/15] perf stat: Improve perf stat output (v2)

[PATCHSET 00/15] perf stat: Improve perf stat output (v2)

[Namhyung Kim]

Hello,

This is a continuation of my perf stat cleanup work focusing on the
metric-only mode with JSON output now.  It's based on the previous
patchset which is already merged to acme/perf/core.

The JSON output + metric-only with aggregation has been broken for a
while.  The last update fixed the crash but it still produced invalid
JSON objects.  Also having a separate header and objects with their own
"metric-value" key is not convenient to process.

  # perf stat -aj --metric-only --per-socket sleep 1
  {"unit" : "GHz"}{"unit" : "insn per cycle"}{"unit" : "branch-misses of all branches"}
  {"socket" : "S0", "aggregate-number" : 8, {"metric-value" : "0.809"}{"metric-value" : "2.10"}{"metric-value" : "0.37"}

So I removed the header and move the metric keys to the object.  Then
it doesn't need separate objects anymore.  The new output looks like:

  # perf stat -aj --metric-only --per-socket sleep 1
  {"socket" : "S0", "cpu-count" : 8, "GHz" : "0.817", "insn per cycle" : "2.15", "branch-misses of all branches" : "0.38"}

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

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

Thanks,
Namhyung

Namhyung Kim (15):
  perf stat: Fix cgroup display in JSON output
  perf stat: Move summary prefix printing logic in CSV output
  perf stat: Do not align time prefix in CSV output
  perf stat: Use scnprintf() in prepare_interval()
  perf stat: Remove prefix argument in print_metric_headers()
  perf stat: Remove metric_only argument in print_counter_aggrdata()
  perf stat: Pass const char *prefix to display routines
  perf stat: Use struct outstate in evlist__print_counters()
  perf stat: Pass struct outstate to print_metric_begin()
  perf stat: Pass struct outstate to printout()
  perf stat: Do not pass runtime_stat to printout()
  perf stat: Pass through struct outstate
  perf stat: Fix JSON output in metric-only mode
  perf stat: Rename "aggregate-number" to "cpu-count" in JSON
  perf stat: Tidy up JSON metric-only output when no metrics

 tools/perf/arch/x86/util/iostat.c |   4 +-
 tools/perf/util/iostat.c          |   3 +-
 tools/perf/util/iostat.h          |   4 +-
 tools/perf/util/stat-display.c    | 257 +++++++++++++++---------------
 4 files changed, 133 insertions(+), 135 deletions(-)


base-commit: 63a3bf5e8d9e79ce456c8f73d4395a5a51d841b1
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH 01/15] perf stat: Fix cgroup display in JSON output

[Namhyung Kim]

It missed the 'else' keyword after checking json output mode.

Fixes: 41cb875242e7 ("perf stat: Split print_cgroup() function")
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 f5501760ff2e..46e90f0bb423 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -175,7 +175,7 @@ static void print_cgroup(struct perf_stat_config *config, struct cgroup *cgrp)
 
 		if (config->json_output)
 			print_cgroup_json(config, cgrp_name);
-		if (config->csv_output)
+		else if (config->csv_output)
 			print_cgroup_csv(config, cgrp_name);
 		else
 			print_cgroup_std(config, cgrp_name);
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH 02/15] perf stat: Move summary prefix printing logic in CSV output

[Namhyung Kim]

It matches to the prefix (interval timestamp), so better to have them together.
No functional change intended.

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

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index 46e90f0bb423..d86f2f8e020d 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -713,11 +713,6 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
 		nl = config->metric_only ? new_line_metric : new_line_std;
 	}
 
-	if (!config->no_csv_summary && config->csv_output &&
-	    config->summary && !config->interval && !config->metric_only) {
-		fprintf(config->output, "%16s%s", "summary", config->csv_sep);
-	}
-
 	if (run == 0 || ena == 0 || counter->counts->scaled == -1) {
 		if (config->metric_only) {
 			pm(config, &os, NULL, "", "", 0);
@@ -828,8 +823,13 @@ static void print_counter_aggrdata(struct perf_stat_config *config,
 	ena = aggr->counts.ena;
 	run = aggr->counts.run;
 
-	if (prefix && !metric_only)
-		fprintf(output, "%s", prefix);
+	if (!metric_only) {
+		if (prefix)
+			fprintf(output, "%s", prefix);
+		else if (config->summary && config->csv_output &&
+			 !config->no_csv_summary && !config->interval)
+			fprintf(output, "%16s%s", "summary", config->csv_sep);
+	}
 
 	uval = val * counter->scale;
 
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH 03/15] perf stat: Do not align time prefix in CSV output

[Namhyung Kim]

We don't care about the alignment in the CSV output as it's intended for machine
processing.  Let's get rid of it to make the output more compact.

Before:
  # perf stat -a --summary -I 1 -x, true
       0.001149309,219.20,msec,cpu-clock,219322251,100.00,219.200,CPUs utilized
       0.001149309,144,,context-switches,219241902,100.00,656.935,/sec
       0.001149309,38,,cpu-migrations,219173705,100.00,173.358,/sec
       0.001149309,61,,page-faults,219093635,100.00,278.285,/sec
       0.001149309,10679310,,cycles,218746228,100.00,0.049,GHz
       0.001149309,6288296,,instructions,218589869,100.00,0.59,insn per cycle
       0.001149309,1386904,,branches,218428851,100.00,6.327,M/sec
       0.001149309,56863,,branch-misses,218219951,100.00,4.10,of all branches
           summary,219.20,msec,cpu-clock,219322251,100.00,20.025,CPUs utilized
           summary,144,,context-switches,219241902,100.00,656.935,/sec
           summary,38,,cpu-migrations,219173705,100.00,173.358,/sec
           summary,61,,page-faults,219093635,100.00,278.285,/sec
           summary,10679310,,cycles,218746228,100.00,0.049,GHz
           summary,6288296,,instructions,218589869,100.00,0.59,insn per cycle
           summary,1386904,,branches,218428851,100.00,6.327,M/sec
           summary,56863,,branch-misses,218219951,100.00,4.10,of all branches

After:
  0.001148449,224.75,msec,cpu-clock,224870589,100.00,224.747,CPUs utilized
  0.001148449,176,,context-switches,224775564,100.00,783.103,/sec
  0.001148449,38,,cpu-migrations,224707428,100.00,169.079,/sec
  0.001148449,61,,page-faults,224629326,100.00,271.416,/sec
  0.001148449,12172071,,cycles,224266368,100.00,0.054,GHz
  0.001148449,6901907,,instructions,224108764,100.00,0.57,insn per cycle
  0.001148449,1515655,,branches,223946693,100.00,6.744,M/sec
  0.001148449,70027,,branch-misses,223735385,100.00,4.62,of all branches
  summary,224.75,msec,cpu-clock,224870589,100.00,21.066,CPUs utilized
  summary,176,,context-switches,224775564,100.00,783.103,/sec
  summary,38,,cpu-migrations,224707428,100.00,169.079,/sec
  summary,61,,page-faults,224629326,100.00,271.416,/sec
  summary,12172071,,cycles,224266368,100.00,0.054,GHz
  summary,6901907,,instructions,224108764,100.00,0.57,insn per cycle
  summary,1515655,,branches,223946693,100.00,6.744,M/sec
  summary,70027,,branch-misses,223735385,100.00,4.62,of all branches

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

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index d86f2f8e020d..15c88b9b5aa3 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -828,7 +828,7 @@ static void print_counter_aggrdata(struct perf_stat_config *config,
 			fprintf(output, "%s", prefix);
 		else if (config->summary && config->csv_output &&
 			 !config->no_csv_summary && !config->interval)
-			fprintf(output, "%16s%s", "summary", config->csv_sep);
+			fprintf(output, "%s%s", "summary", config->csv_sep);
 	}
 
 	uval = val * counter->scale;
@@ -1078,9 +1078,12 @@ static void prepare_interval(struct perf_stat_config *config,
 	if (config->iostat_run)
 		return;
 
-	if (!config->json_output)
-		sprintf(prefix, "%6lu.%09lu%s", (unsigned long) ts->tv_sec,
+	if (config->csv_output)
+		sprintf(prefix, "%lu.%09lu%s", (unsigned long) ts->tv_sec,
 				 ts->tv_nsec, config->csv_sep);
+	else if (!config->json_output)
+		sprintf(prefix, "%6lu.%09lu ", (unsigned long) ts->tv_sec,
+				 ts->tv_nsec);
 	else if (!config->metric_only)
 		sprintf(prefix, "{\"interval\" : %lu.%09lu, ", (unsigned long)
 				 ts->tv_sec, ts->tv_nsec);
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH 04/15] perf stat: Use scnprintf() in prepare_interval()

[Namhyung Kim]

It should not use sprintf() anymore.  Let's pass the buffer size and use the
safer scnprintf() instead.

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

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index 15c88b9b5aa3..744b7a40f59a 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -1073,23 +1073,23 @@ static void print_metric_headers(struct perf_stat_config *config,
 }
 
 static void prepare_interval(struct perf_stat_config *config,
-			     char *prefix, struct timespec *ts)
+			     char *prefix, size_t len, struct timespec *ts)
 {
 	if (config->iostat_run)
 		return;
 
 	if (config->csv_output)
-		sprintf(prefix, "%lu.%09lu%s", (unsigned long) ts->tv_sec,
-				 ts->tv_nsec, config->csv_sep);
+		scnprintf(prefix, len, "%lu.%09lu%s",
+			  (unsigned long) ts->tv_sec, ts->tv_nsec, config->csv_sep);
 	else if (!config->json_output)
-		sprintf(prefix, "%6lu.%09lu ", (unsigned long) ts->tv_sec,
-				 ts->tv_nsec);
+		scnprintf(prefix, len, "%6lu.%09lu ",
+			  (unsigned long) ts->tv_sec, ts->tv_nsec);
 	else if (!config->metric_only)
-		sprintf(prefix, "{\"interval\" : %lu.%09lu, ", (unsigned long)
-				 ts->tv_sec, ts->tv_nsec);
+		scnprintf(prefix, len, "{\"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);
+		scnprintf(prefix, len, "{\"interval\" : %lu.%09lu}",
+			  (unsigned long) ts->tv_sec, ts->tv_nsec);
 }
 
 static void print_header_interval_std(struct perf_stat_config *config,
@@ -1390,7 +1390,7 @@ void evlist__print_counters(struct evlist *evlist, struct perf_stat_config *conf
 
 	if (interval) {
 		prefix = buf;
-		prepare_interval(config, prefix, ts);
+		prepare_interval(config, buf, sizeof(buf), ts);
 	}
 
 	print_header(config, _target, evlist, argc, argv);
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH 05/15] perf stat: Remove prefix argument in print_metric_headers()

[Namhyung Kim]

It always passes a whitespace to the function, thus we can just add it to the
function body.  Furthermore, it's only used in the normal output mode.

Well, actually CSV used it but it doesn't need to since we don't care about the
indentation or alignment in the CSV output.

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

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index 744b7a40f59a..deed6ccf072f 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -996,10 +996,9 @@ static void print_no_aggr_metric(struct perf_stat_config *config,
 }
 
 static void print_metric_headers_std(struct perf_stat_config *config,
-				     const char *prefix, bool no_indent)
+				     bool no_indent)
 {
-	if (prefix)
-		fprintf(config->output, "%s", prefix);
+	fputc(' ', config->output);
 
 	if (!no_indent) {
 		int len = aggr_header_lens[config->aggr_mode];
@@ -1012,11 +1011,8 @@ static void print_metric_headers_std(struct perf_stat_config *config,
 }
 
 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)
@@ -1024,7 +1020,6 @@ static void print_metric_headers_csv(struct perf_stat_config *config,
 }
 
 static void print_metric_headers_json(struct perf_stat_config *config,
-				      const char *prefix __maybe_unused,
 				      bool no_indent __maybe_unused)
 {
 	if (config->interval)
@@ -1032,8 +1027,7 @@ static void print_metric_headers_json(struct perf_stat_config *config,
 }
 
 static void print_metric_headers(struct perf_stat_config *config,
-				 struct evlist *evlist,
-				 const char *prefix, bool no_indent)
+				 struct evlist *evlist, bool no_indent)
 {
 	struct evsel *counter;
 	struct outstate os = {
@@ -1047,11 +1041,11 @@ static void print_metric_headers(struct perf_stat_config *config,
 	};
 
 	if (config->json_output)
-		print_metric_headers_json(config, prefix, no_indent);
+		print_metric_headers_json(config, no_indent);
 	else if (config->csv_output)
-		print_metric_headers_csv(config, prefix, no_indent);
+		print_metric_headers_csv(config, no_indent);
 	else
-		print_metric_headers_std(config, prefix, no_indent);
+		print_metric_headers_std(config, no_indent);
 
 	if (config->iostat_run)
 		iostat_print_header_prefix(config);
@@ -1132,7 +1126,7 @@ static void print_header_interval_std(struct perf_stat_config *config,
 	}
 
 	if (config->metric_only)
-		print_metric_headers(config, evlist, " ", true);
+		print_metric_headers(config, evlist, true);
 	else
 		fprintf(output, " %*s %*s events\n",
 			COUNTS_LEN, "counts", config->unit_width, "unit");
@@ -1168,7 +1162,7 @@ static void print_header_std(struct perf_stat_config *config,
 	fprintf(output, ":\n\n");
 
 	if (config->metric_only)
-		print_metric_headers(config, evlist, " ", false);
+		print_metric_headers(config, evlist, false);
 }
 
 static void print_header_csv(struct perf_stat_config *config,
@@ -1178,7 +1172,7 @@ static void print_header_csv(struct perf_stat_config *config,
 			     const char **argv __maybe_unused)
 {
 	if (config->metric_only)
-		print_metric_headers(config, evlist, " ", true);
+		print_metric_headers(config, evlist, true);
 }
 static void print_header_json(struct perf_stat_config *config,
 			      struct target *_target __maybe_unused,
@@ -1187,7 +1181,7 @@ static void print_header_json(struct perf_stat_config *config,
 			      const char **argv __maybe_unused)
 {
 	if (config->metric_only)
-		print_metric_headers(config, evlist, " ", true);
+		print_metric_headers(config, evlist, true);
 }
 
 static void print_header(struct perf_stat_config *config,
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH 06/15] perf stat: Remove metric_only argument in print_counter_aggrdata()

[Namhyung Kim]

It already passes the stat_config argument, then it can find the value in the
config.  No need to pass it separately.

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

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index deed6ccf072f..b8432c0a0ec3 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -804,7 +804,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)
+				   char *prefix)
 {
 	FILE *output = config->output;
 	u64 ena, run, val;
@@ -813,6 +813,7 @@ static void print_counter_aggrdata(struct perf_stat_config *config,
 	struct perf_stat_aggr *aggr = &ps->aggr[s];
 	struct aggr_cpu_id id = config->aggr_map->map[s];
 	double avg = aggr->counts.val;
+	bool metric_only = config->metric_only;
 
 	if (counter->supported && aggr->nr == 0)
 		return;
@@ -875,7 +876,6 @@ static void print_aggr(struct perf_stat_config *config,
 		       struct evlist *evlist,
 		       char *prefix)
 {
-	bool metric_only = config->metric_only;
 	struct evsel *counter;
 	int s;
 
@@ -893,8 +893,7 @@ static void print_aggr(struct perf_stat_config *config,
 			if (counter->merged_stat)
 				continue;
 
-			print_counter_aggrdata(config, counter, s, prefix,
-					       metric_only);
+			print_counter_aggrdata(config, counter, s, prefix);
 		}
 		print_metric_end(config);
 	}
@@ -904,7 +903,6 @@ 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;
@@ -928,8 +926,7 @@ static void print_aggr_cgroup(struct perf_stat_config *config,
 				if (counter->cgrp != cgrp)
 					continue;
 
-				print_counter_aggrdata(config, counter, s, prefix,
-						       metric_only);
+				print_counter_aggrdata(config, counter, s, prefix);
 			}
 			print_metric_end(config);
 		}
@@ -939,7 +936,6 @@ static void print_aggr_cgroup(struct perf_stat_config *config,
 static void print_counter(struct perf_stat_config *config,
 			  struct evsel *counter, char *prefix)
 {
-	bool metric_only = config->metric_only;
 	int s;
 
 	/* AGGR_THREAD doesn't have config->aggr_get_id */
@@ -950,8 +946,7 @@ 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);
+		print_counter_aggrdata(config, counter, s, prefix);
 	}
 }
 
@@ -1339,7 +1334,7 @@ static void print_percore(struct perf_stat_config *config,
 		if (found)
 			continue;
 
-		print_counter_aggrdata(config, counter, s, prefix, metric_only);
+		print_counter_aggrdata(config, counter, s, prefix);
 
 		core_map->map[c++] = core_id;
 	}
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH 07/15] perf stat: Pass const char *prefix to display routines

[Namhyung Kim]

This is a minor cleanup and preparation for the later change.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/iostat.h       |  2 +-
 tools/perf/util/stat-display.c | 18 +++++++++---------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/tools/perf/util/iostat.h b/tools/perf/util/iostat.h
index 23c1c46a331a..c22688f87cb2 100644
--- a/tools/perf/util/iostat.h
+++ b/tools/perf/util/iostat.h
@@ -28,7 +28,7 @@ enum iostat_mode_t {
 
 extern enum iostat_mode_t iostat_mode;
 
-typedef void (*iostat_print_counter_t)(struct perf_stat_config *, struct evsel *, char *);
+typedef void (*iostat_print_counter_t)(struct perf_stat_config *, struct evsel *, const char *);
 
 int iostat_prepare(struct evlist *evlist, struct perf_stat_config *config);
 int iostat_parse(const struct option *opt, const char *str,
diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index b8432c0a0ec3..d2894a519d61 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -675,7 +675,7 @@ static bool is_mixed_hw_group(struct evsel *counter)
 
 static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int nr,
 		     struct evsel *counter, double uval,
-		     char *prefix, u64 run, u64 ena, double noise,
+		     const char *prefix, u64 run, u64 ena, double noise,
 		     struct runtime_stat *st, int map_idx)
 {
 	struct perf_stat_output_ctx out;
@@ -804,7 +804,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)
+				   const char *prefix)
 {
 	FILE *output = config->output;
 	u64 ena, run, val;
@@ -843,7 +843,7 @@ 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,
+			       const char *prefix, int aggr_idx,
 			       struct cgroup *cgrp)
 {
 	struct perf_stat_aggr *aggr;
@@ -874,7 +874,7 @@ static void print_metric_end(struct perf_stat_config *config)
 
 static void print_aggr(struct perf_stat_config *config,
 		       struct evlist *evlist,
-		       char *prefix)
+		       const char *prefix)
 {
 	struct evsel *counter;
 	int s;
@@ -901,7 +901,7 @@ static void print_aggr(struct perf_stat_config *config,
 
 static void print_aggr_cgroup(struct perf_stat_config *config,
 			      struct evlist *evlist,
-			      char *prefix)
+			      const char *prefix)
 {
 	struct evsel *counter, *evsel;
 	struct cgroup *cgrp = NULL;
@@ -934,7 +934,7 @@ static void print_aggr_cgroup(struct perf_stat_config *config,
 }
 
 static void print_counter(struct perf_stat_config *config,
-			  struct evsel *counter, char *prefix)
+			  struct evsel *counter, const char *prefix)
 {
 	int s;
 
@@ -952,7 +952,7 @@ static void print_counter(struct perf_stat_config *config,
 
 static void print_no_aggr_metric(struct perf_stat_config *config,
 				 struct evlist *evlist,
-				 char *prefix)
+				 const char *prefix)
 {
 	int all_idx;
 	struct perf_cpu cpu;
@@ -1301,7 +1301,7 @@ static void print_footer(struct perf_stat_config *config)
 }
 
 static void print_percore(struct perf_stat_config *config,
-			  struct evsel *counter, char *prefix)
+			  struct evsel *counter, const char *prefix)
 {
 	bool metric_only = config->metric_only;
 	FILE *output = config->output;
@@ -1345,7 +1345,7 @@ static void print_percore(struct perf_stat_config *config,
 }
 
 static void print_cgroup_counter(struct perf_stat_config *config, struct evlist *evlist,
-				 char *prefix)
+				 const char *prefix)
 {
 	struct cgroup *cgrp = NULL;
 	struct evsel *counter;
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH 08/15] perf stat: Use struct outstate in evlist__print_counters()

[Namhyung Kim]

This is a preparation for the later cleanup.  No functional changes
intended.

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

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index d2894a519d61..70aebf359e16 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -1372,13 +1372,16 @@ void evlist__print_counters(struct evlist *evlist, struct perf_stat_config *conf
 	bool metric_only = config->metric_only;
 	int interval = config->interval;
 	struct evsel *counter;
-	char buf[64], *prefix = NULL;
+	char buf[64];
+	struct outstate os = {
+		.fh = config->output,
+	};
 
 	if (config->iostat_run)
 		evlist->selected = evlist__first(evlist);
 
 	if (interval) {
-		prefix = buf;
+		os.prefix = buf;
 		prepare_interval(config, buf, sizeof(buf), ts);
 	}
 
@@ -1390,35 +1393,35 @@ void evlist__print_counters(struct evlist *evlist, struct perf_stat_config *conf
 	case AGGR_SOCKET:
 	case AGGR_NODE:
 		if (config->cgroup_list)
-			print_aggr_cgroup(config, evlist, prefix);
+			print_aggr_cgroup(config, evlist, os.prefix);
 		else
-			print_aggr(config, evlist, prefix);
+			print_aggr(config, evlist, os.prefix);
 		break;
 	case AGGR_THREAD:
 	case AGGR_GLOBAL:
 		if (config->iostat_run) {
-			iostat_print_counters(evlist, config, ts, prefix = buf,
+			iostat_print_counters(evlist, config, ts, buf,
 					      print_counter);
 		} else if (config->cgroup_list) {
-			print_cgroup_counter(config, evlist, prefix);
+			print_cgroup_counter(config, evlist, os.prefix);
 		} else {
-			print_metric_begin(config, evlist, prefix,
+			print_metric_begin(config, evlist, os.prefix,
 					   /*aggr_idx=*/0, /*cgrp=*/NULL);
 			evlist__for_each_entry(evlist, counter) {
-				print_counter(config, counter, prefix);
+				print_counter(config, counter, os.prefix);
 			}
 			print_metric_end(config);
 		}
 		break;
 	case AGGR_NONE:
 		if (metric_only)
-			print_no_aggr_metric(config, evlist, prefix);
+			print_no_aggr_metric(config, evlist, os.prefix);
 		else {
 			evlist__for_each_entry(evlist, counter) {
 				if (counter->percore)
-					print_percore(config, counter, prefix);
+					print_percore(config, counter, os.prefix);
 				else
-					print_counter(config, counter, prefix);
+					print_counter(config, counter, os.prefix);
 			}
 		}
 		break;
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH 09/15] perf stat: Pass struct outstate to print_metric_begin()

[Namhyung Kim]

It passes prefix and cgroup pointers but the outstate already has them.
Let's pass the outstate pointer instead.

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

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index 70aebf359e16..3ed63061d6f8 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -843,8 +843,7 @@ static void print_counter_aggrdata(struct perf_stat_config *config,
 
 static void print_metric_begin(struct perf_stat_config *config,
 			       struct evlist *evlist,
-			       const char *prefix, int aggr_idx,
-			       struct cgroup *cgrp)
+			       struct outstate *os, int aggr_idx)
 {
 	struct perf_stat_aggr *aggr;
 	struct aggr_cpu_id id;
@@ -853,15 +852,15 @@ static void print_metric_begin(struct perf_stat_config *config,
 	if (!config->metric_only)
 		return;
 
-	if (prefix)
-		fprintf(config->output, "%s", prefix);
+	if (os->prefix)
+		fprintf(config->output, "%s", os->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);
 
-	print_cgroup(config, cgrp);
+	print_cgroup(config, os->cgrp);
 }
 
 static void print_metric_end(struct perf_stat_config *config)
@@ -877,6 +876,9 @@ static void print_aggr(struct perf_stat_config *config,
 		       const char *prefix)
 {
 	struct evsel *counter;
+	struct outstate os = {
+		.prefix = prefix,
+	};
 	int s;
 
 	if (!config->aggr_map || !config->aggr_get_id)
@@ -887,7 +889,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, /*cgrp=*/NULL);
+		print_metric_begin(config, evlist, &os, s);
 
 		evlist__for_each_entry(evlist, counter) {
 			if (counter->merged_stat)
@@ -904,26 +906,28 @@ static void print_aggr_cgroup(struct perf_stat_config *config,
 			      const char *prefix)
 {
 	struct evsel *counter, *evsel;
-	struct cgroup *cgrp = NULL;
+	struct outstate os = {
+		.prefix = prefix,
+	};
 	int s;
 
 	if (!config->aggr_map || !config->aggr_get_id)
 		return;
 
 	evlist__for_each_entry(evlist, evsel) {
-		if (cgrp == evsel->cgrp)
+		if (os.cgrp == evsel->cgrp)
 			continue;
 
-		cgrp = evsel->cgrp;
+		os.cgrp = evsel->cgrp;
 
 		for (s = 0; s < config->aggr_map->nr; s++) {
-			print_metric_begin(config, evlist, prefix, s, cgrp);
+			print_metric_begin(config, evlist, &os, s);
 
 			evlist__for_each_entry(evlist, counter) {
 				if (counter->merged_stat)
 					continue;
 
-				if (counter->cgrp != cgrp)
+				if (counter->cgrp != os.cgrp)
 					continue;
 
 				print_counter_aggrdata(config, counter, s, prefix);
@@ -956,6 +960,9 @@ static void print_no_aggr_metric(struct perf_stat_config *config,
 {
 	int all_idx;
 	struct perf_cpu cpu;
+	struct outstate os = {
+		.prefix = prefix,
+	};
 
 	perf_cpu_map__for_each_cpu(cpu, all_idx, evlist->core.user_requested_cpus) {
 		struct evsel *counter;
@@ -973,8 +980,7 @@ 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, /*cgrp=*/NULL);
+				print_metric_begin(config, evlist, &os, counter_idx);
 				first = false;
 			}
 			val = ps->aggr[counter_idx].counts.val;
@@ -1347,22 +1353,23 @@ static void print_percore(struct perf_stat_config *config,
 static void print_cgroup_counter(struct perf_stat_config *config, struct evlist *evlist,
 				 const char *prefix)
 {
-	struct cgroup *cgrp = NULL;
 	struct evsel *counter;
+	struct outstate os = {
+		.prefix = prefix,
+	};
 
 	evlist__for_each_entry(evlist, counter) {
-		if (cgrp != counter->cgrp) {
-			if (cgrp != NULL)
+		if (os.cgrp != counter->cgrp) {
+			if (os.cgrp != NULL)
 				print_metric_end(config);
 
-			cgrp = counter->cgrp;
-			print_metric_begin(config, evlist, prefix,
-					   /*aggr_idx=*/0, cgrp);
+			os.cgrp = counter->cgrp;
+			print_metric_begin(config, evlist, &os, /*aggr_idx=*/0);
 		}
 
 		print_counter(config, counter, prefix);
 	}
-	if (cgrp)
+	if (os.cgrp)
 		print_metric_end(config);
 }
 
@@ -1405,8 +1412,7 @@ void evlist__print_counters(struct evlist *evlist, struct perf_stat_config *conf
 		} else if (config->cgroup_list) {
 			print_cgroup_counter(config, evlist, os.prefix);
 		} else {
-			print_metric_begin(config, evlist, os.prefix,
-					   /*aggr_idx=*/0, /*cgrp=*/NULL);
+			print_metric_begin(config, evlist, &os, /*aggr_idx=*/0);
 			evlist__for_each_entry(evlist, counter) {
 				print_counter(config, counter, os.prefix);
 			}
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH 10/15] perf stat: Pass struct outstate to printout()

[Namhyung Kim]

The printout() takes a lot of arguments and sets an outstate with the
value.  Instead, we can fill the outstate first and then pass it to
reduce the number of arguments.

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

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index 3ed63061d6f8..dd190f71e933 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -673,22 +673,15 @@ static bool is_mixed_hw_group(struct evsel *counter)
 	return false;
 }
 
-static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int nr,
-		     struct evsel *counter, double uval,
-		     const char *prefix, u64 run, u64 ena, double noise,
+static void printout(struct perf_stat_config *config, struct outstate *os,
+		     double uval, u64 run, u64 ena, double noise,
 		     struct runtime_stat *st, int map_idx)
 {
 	struct perf_stat_output_ctx out;
-	struct outstate os = {
-		.fh = config->output,
-		.prefix = prefix ? prefix : "",
-		.id = id,
-		.nr = nr,
-		.evsel = counter,
-	};
 	print_metric_t pm;
 	new_line_t nl;
 	bool ok = true;
+	struct evsel *counter = os->evsel;
 
 	if (config->csv_output) {
 		static const int aggr_fields[AGGR_MAX] = {
@@ -704,7 +697,7 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
 
 		pm = config->metric_only ? print_metric_only_csv : print_metric_csv;
 		nl = config->metric_only ? new_line_metric : new_line_csv;
-		os.nfields = 3 + aggr_fields[config->aggr_mode] + (counter->cgrp ? 1 : 0);
+		os->nfields = 3 + aggr_fields[config->aggr_mode] + (counter->cgrp ? 1 : 0);
 	} else if (config->json_output) {
 		pm = config->metric_only ? print_metric_only_json : print_metric_json;
 		nl = config->metric_only ? new_line_metric : new_line_json;
@@ -715,7 +708,7 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
 
 	if (run == 0 || ena == 0 || counter->counts->scaled == -1) {
 		if (config->metric_only) {
-			pm(config, &os, NULL, "", "", 0);
+			pm(config, os, NULL, "", "", 0);
 			return;
 		}
 
@@ -732,11 +725,11 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
 
 	out.print_metric = pm;
 	out.new_line = nl;
-	out.ctx = &os;
+	out.ctx = os;
 	out.force_header = false;
 
 	if (!config->metric_only) {
-		abs_printout(config, id, nr, counter, uval, ok);
+		abs_printout(config, os->id, os->nr, counter, uval, ok);
 
 		print_noise(config, counter, noise, /*before_metric=*/true);
 		print_running(config, run, ena, /*before_metric=*/true);
@@ -814,6 +807,13 @@ static void print_counter_aggrdata(struct perf_stat_config *config,
 	struct aggr_cpu_id id = config->aggr_map->map[s];
 	double avg = aggr->counts.val;
 	bool metric_only = config->metric_only;
+	struct outstate os = {
+		.fh = config->output,
+		.prefix = prefix ? prefix : "",
+		.id = id,
+		.nr = aggr->nr,
+		.evsel = counter,
+	};
 
 	if (counter->supported && aggr->nr == 0)
 		return;
@@ -834,8 +834,7 @@ static void print_counter_aggrdata(struct perf_stat_config *config,
 
 	uval = val * counter->scale;
 
-	printout(config, id, aggr->nr, counter, uval,
-		 prefix, run, ena, avg, &rt_stat, s);
+	printout(config, &os, uval, run, ena, avg, &rt_stat, s);
 
 	if (!metric_only)
 		fputc('\n', output);
@@ -971,14 +970,14 @@ static void print_no_aggr_metric(struct perf_stat_config *config,
 		evlist__for_each_entry(evlist, counter) {
 			u64 ena, run, val;
 			double uval;
-			struct aggr_cpu_id id;
 			struct perf_stat_evsel *ps = counter->stats;
 			int counter_idx = perf_cpu_map__idx(evsel__cpus(counter), cpu);
 
 			if (counter_idx < 0)
 				continue;
 
-			id = aggr_cpu_id__cpu(cpu, /*data=*/NULL);
+			os.evsel = counter;
+			os.id = aggr_cpu_id__cpu(cpu, /*data=*/NULL);
 			if (first) {
 				print_metric_begin(config, evlist, &os, counter_idx);
 				first = false;
@@ -988,8 +987,7 @@ static void print_no_aggr_metric(struct perf_stat_config *config,
 			run = ps->aggr[counter_idx].counts.run;
 
 			uval = val * counter->scale;
-			printout(config, id, 0, counter, uval, prefix,
-				 run, ena, 1.0, &rt_stat, counter_idx);
+			printout(config, &os, uval, run, ena, 1.0, &rt_stat, counter_idx);
 		}
 		if (!first)
 			print_metric_end(config);
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH 11/15] perf stat: Do not pass runtime_stat to printout()

[Namhyung Kim]

It always passes a pointer to rt_stat as it's the only one.  Let's not
pass it and directly refer it in the printout().

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

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index dd190f71e933..cdf4ca7f6e3a 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -674,8 +674,7 @@ static bool is_mixed_hw_group(struct evsel *counter)
 }
 
 static void printout(struct perf_stat_config *config, struct outstate *os,
-		     double uval, u64 run, u64 ena, double noise,
-		     struct runtime_stat *st, int map_idx)
+		     double uval, u64 run, u64 ena, double noise, int map_idx)
 {
 	struct perf_stat_output_ctx out;
 	print_metric_t pm;
@@ -737,7 +736,7 @@ static void printout(struct perf_stat_config *config, struct outstate *os,
 
 	if (ok) {
 		perf_stat__print_shadow_stats(config, counter, uval, map_idx,
-					      &out, &config->metric_events, st);
+					      &out, &config->metric_events, &rt_stat);
 	} else {
 		pm(config, &os, /*color=*/NULL, /*format=*/NULL, /*unit=*/"", /*val=*/0);
 	}
@@ -834,7 +833,7 @@ static void print_counter_aggrdata(struct perf_stat_config *config,
 
 	uval = val * counter->scale;
 
-	printout(config, &os, uval, run, ena, avg, &rt_stat, s);
+	printout(config, &os, uval, run, ena, avg, s);
 
 	if (!metric_only)
 		fputc('\n', output);
@@ -987,7 +986,7 @@ static void print_no_aggr_metric(struct perf_stat_config *config,
 			run = ps->aggr[counter_idx].counts.run;
 
 			uval = val * counter->scale;
-			printout(config, &os, uval, run, ena, 1.0, &rt_stat, counter_idx);
+			printout(config, &os, uval, run, ena, 1.0, counter_idx);
 		}
 		if (!first)
 			print_metric_end(config);
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH 12/15] perf stat: Pass through struct outstate

[Namhyung Kim]

Now most of the print functions take a pointer to the struct outstate.
We have one in the evlist__print_counters() and pass it through the
child functions.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/arch/x86/util/iostat.c |   4 +-
 tools/perf/util/iostat.c          |   3 +-
 tools/perf/util/iostat.h          |   4 +-
 tools/perf/util/stat-display.c    | 102 +++++++++++++-----------------
 4 files changed, 50 insertions(+), 63 deletions(-)

diff --git a/tools/perf/arch/x86/util/iostat.c b/tools/perf/arch/x86/util/iostat.c
index 404de795ec0b..7eb0a7b00b95 100644
--- a/tools/perf/arch/x86/util/iostat.c
+++ b/tools/perf/arch/x86/util/iostat.c
@@ -449,7 +449,7 @@ void iostat_print_metric(struct perf_stat_config *config, struct evsel *evsel,
 
 void iostat_print_counters(struct evlist *evlist,
 			   struct perf_stat_config *config, struct timespec *ts,
-			   char *prefix, iostat_print_counter_t print_cnt_cb)
+			   char *prefix, iostat_print_counter_t print_cnt_cb, void *arg)
 {
 	void *perf_device = NULL;
 	struct evsel *counter = evlist__first(evlist);
@@ -464,7 +464,7 @@ void iostat_print_counters(struct evlist *evlist,
 			iostat_prefix(evlist, config, prefix, ts);
 			fprintf(config->output, "\n%s", prefix);
 		}
-		print_cnt_cb(config, counter, prefix);
+		print_cnt_cb(config, counter, arg);
 	}
 	fputc('\n', config->output);
 }
diff --git a/tools/perf/util/iostat.c b/tools/perf/util/iostat.c
index 57dd49da28fe..b770bd473af7 100644
--- a/tools/perf/util/iostat.c
+++ b/tools/perf/util/iostat.c
@@ -48,6 +48,7 @@ __weak void iostat_print_counters(struct evlist *evlist __maybe_unused,
 				  struct perf_stat_config *config __maybe_unused,
 				  struct timespec *ts __maybe_unused,
 				  char *prefix __maybe_unused,
-				  iostat_print_counter_t print_cnt_cb __maybe_unused)
+				  iostat_print_counter_t print_cnt_cb __maybe_unused,
+				  void *arg __maybe_unused)
 {
 }
diff --git a/tools/perf/util/iostat.h b/tools/perf/util/iostat.h
index c22688f87cb2..a4e7299c5c2f 100644
--- a/tools/perf/util/iostat.h
+++ b/tools/perf/util/iostat.h
@@ -28,7 +28,7 @@ enum iostat_mode_t {
 
 extern enum iostat_mode_t iostat_mode;
 
-typedef void (*iostat_print_counter_t)(struct perf_stat_config *, struct evsel *, const char *);
+typedef void (*iostat_print_counter_t)(struct perf_stat_config *, struct evsel *, void *);
 
 int iostat_prepare(struct evlist *evlist, struct perf_stat_config *config);
 int iostat_parse(const struct option *opt, const char *str,
@@ -42,6 +42,6 @@ void iostat_print_metric(struct perf_stat_config *config, struct evsel *evsel,
 			 struct perf_stat_output_ctx *out);
 void iostat_print_counters(struct evlist *evlist,
 			   struct perf_stat_config *config, struct timespec *ts,
-			   char *prefix, iostat_print_counter_t print_cnt_cb);
+			   char *prefix, iostat_print_counter_t print_cnt_cb, void *arg);
 
 #endif /* _IOSTAT_H */
diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index cdf4ca7f6e3a..335627e8542d 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -796,7 +796,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,
-				   const char *prefix)
+				   struct outstate *os)
 {
 	FILE *output = config->output;
 	u64 ena, run, val;
@@ -806,13 +806,10 @@ static void print_counter_aggrdata(struct perf_stat_config *config,
 	struct aggr_cpu_id id = config->aggr_map->map[s];
 	double avg = aggr->counts.val;
 	bool metric_only = config->metric_only;
-	struct outstate os = {
-		.fh = config->output,
-		.prefix = prefix ? prefix : "",
-		.id = id,
-		.nr = aggr->nr,
-		.evsel = counter,
-	};
+
+	os->id = id;
+	os->nr = aggr->nr;
+	os->evsel = counter;
 
 	if (counter->supported && aggr->nr == 0)
 		return;
@@ -824,8 +821,8 @@ static void print_counter_aggrdata(struct perf_stat_config *config,
 	run = aggr->counts.run;
 
 	if (!metric_only) {
-		if (prefix)
-			fprintf(output, "%s", prefix);
+		if (os->prefix)
+			fprintf(output, "%s", os->prefix);
 		else if (config->summary && config->csv_output &&
 			 !config->no_csv_summary && !config->interval)
 			fprintf(output, "%s%s", "summary", config->csv_sep);
@@ -833,7 +830,7 @@ static void print_counter_aggrdata(struct perf_stat_config *config,
 
 	uval = val * counter->scale;
 
-	printout(config, &os, uval, run, ena, avg, s);
+	printout(config, os, uval, run, ena, avg, s);
 
 	if (!metric_only)
 		fputc('\n', output);
@@ -871,12 +868,9 @@ static void print_metric_end(struct perf_stat_config *config)
 
 static void print_aggr(struct perf_stat_config *config,
 		       struct evlist *evlist,
-		       const char *prefix)
+		       struct outstate *os)
 {
 	struct evsel *counter;
-	struct outstate os = {
-		.prefix = prefix,
-	};
 	int s;
 
 	if (!config->aggr_map || !config->aggr_get_id)
@@ -887,13 +881,13 @@ 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, &os, s);
+		print_metric_begin(config, evlist, os, s);
 
 		evlist__for_each_entry(evlist, counter) {
 			if (counter->merged_stat)
 				continue;
 
-			print_counter_aggrdata(config, counter, s, prefix);
+			print_counter_aggrdata(config, counter, s, os);
 		}
 		print_metric_end(config);
 	}
@@ -901,34 +895,31 @@ static void print_aggr(struct perf_stat_config *config,
 
 static void print_aggr_cgroup(struct perf_stat_config *config,
 			      struct evlist *evlist,
-			      const char *prefix)
+			      struct outstate *os)
 {
 	struct evsel *counter, *evsel;
-	struct outstate os = {
-		.prefix = prefix,
-	};
 	int s;
 
 	if (!config->aggr_map || !config->aggr_get_id)
 		return;
 
 	evlist__for_each_entry(evlist, evsel) {
-		if (os.cgrp == evsel->cgrp)
+		if (os->cgrp == evsel->cgrp)
 			continue;
 
-		os.cgrp = evsel->cgrp;
+		os->cgrp = evsel->cgrp;
 
 		for (s = 0; s < config->aggr_map->nr; s++) {
-			print_metric_begin(config, evlist, &os, s);
+			print_metric_begin(config, evlist, os, s);
 
 			evlist__for_each_entry(evlist, counter) {
 				if (counter->merged_stat)
 					continue;
 
-				if (counter->cgrp != os.cgrp)
+				if (counter->cgrp != os->cgrp)
 					continue;
 
-				print_counter_aggrdata(config, counter, s, prefix);
+				print_counter_aggrdata(config, counter, s, os);
 			}
 			print_metric_end(config);
 		}
@@ -936,7 +927,7 @@ static void print_aggr_cgroup(struct perf_stat_config *config,
 }
 
 static void print_counter(struct perf_stat_config *config,
-			  struct evsel *counter, const char *prefix)
+			  struct evsel *counter, struct outstate *os)
 {
 	int s;
 
@@ -948,19 +939,16 @@ 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);
+		print_counter_aggrdata(config, counter, s, os);
 	}
 }
 
 static void print_no_aggr_metric(struct perf_stat_config *config,
 				 struct evlist *evlist,
-				 const char *prefix)
+				 struct outstate *os)
 {
 	int all_idx;
 	struct perf_cpu cpu;
-	struct outstate os = {
-		.prefix = prefix,
-	};
 
 	perf_cpu_map__for_each_cpu(cpu, all_idx, evlist->core.user_requested_cpus) {
 		struct evsel *counter;
@@ -975,10 +963,10 @@ static void print_no_aggr_metric(struct perf_stat_config *config,
 			if (counter_idx < 0)
 				continue;
 
-			os.evsel = counter;
-			os.id = aggr_cpu_id__cpu(cpu, /*data=*/NULL);
+			os->evsel = counter;
+			os->id = aggr_cpu_id__cpu(cpu, /*data=*/NULL);
 			if (first) {
-				print_metric_begin(config, evlist, &os, counter_idx);
+				print_metric_begin(config, evlist, os, counter_idx);
 				first = false;
 			}
 			val = ps->aggr[counter_idx].counts.val;
@@ -986,7 +974,7 @@ static void print_no_aggr_metric(struct perf_stat_config *config,
 			run = ps->aggr[counter_idx].counts.run;
 
 			uval = val * counter->scale;
-			printout(config, &os, uval, run, ena, 1.0, counter_idx);
+			printout(config, os, uval, run, ena, 1.0, counter_idx);
 		}
 		if (!first)
 			print_metric_end(config);
@@ -1304,7 +1292,7 @@ static void print_footer(struct perf_stat_config *config)
 }
 
 static void print_percore(struct perf_stat_config *config,
-			  struct evsel *counter, const char *prefix)
+			  struct evsel *counter, struct outstate *os)
 {
 	bool metric_only = config->metric_only;
 	FILE *output = config->output;
@@ -1315,7 +1303,7 @@ static void print_percore(struct perf_stat_config *config,
 		return;
 
 	if (config->percore_show_thread)
-		return print_counter(config, counter, prefix);
+		return print_counter(config, counter, os);
 
 	core_map = cpu_aggr_map__empty_new(config->aggr_map->nr);
 	if (core_map == NULL) {
@@ -1337,7 +1325,7 @@ static void print_percore(struct perf_stat_config *config,
 		if (found)
 			continue;
 
-		print_counter_aggrdata(config, counter, s, prefix);
+		print_counter_aggrdata(config, counter, s, os);
 
 		core_map->map[c++] = core_id;
 	}
@@ -1348,30 +1336,28 @@ static void print_percore(struct perf_stat_config *config,
 }
 
 static void print_cgroup_counter(struct perf_stat_config *config, struct evlist *evlist,
-				 const char *prefix)
+				 struct outstate *os)
 {
 	struct evsel *counter;
-	struct outstate os = {
-		.prefix = prefix,
-	};
 
 	evlist__for_each_entry(evlist, counter) {
-		if (os.cgrp != counter->cgrp) {
-			if (os.cgrp != NULL)
+		if (os->cgrp != counter->cgrp) {
+			if (os->cgrp != NULL)
 				print_metric_end(config);
 
-			os.cgrp = counter->cgrp;
-			print_metric_begin(config, evlist, &os, /*aggr_idx=*/0);
+			os->cgrp = counter->cgrp;
+			print_metric_begin(config, evlist, os, /*aggr_idx=*/0);
 		}
 
-		print_counter(config, counter, prefix);
+		print_counter(config, counter, os);
 	}
-	if (os.cgrp)
+	if (os->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)
+			    struct target *_target, struct timespec *ts,
+			    int argc, const char **argv)
 {
 	bool metric_only = config->metric_only;
 	int interval = config->interval;
@@ -1397,34 +1383,34 @@ void evlist__print_counters(struct evlist *evlist, struct perf_stat_config *conf
 	case AGGR_SOCKET:
 	case AGGR_NODE:
 		if (config->cgroup_list)
-			print_aggr_cgroup(config, evlist, os.prefix);
+			print_aggr_cgroup(config, evlist, &os);
 		else
-			print_aggr(config, evlist, os.prefix);
+			print_aggr(config, evlist, &os);
 		break;
 	case AGGR_THREAD:
 	case AGGR_GLOBAL:
 		if (config->iostat_run) {
 			iostat_print_counters(evlist, config, ts, buf,
-					      print_counter);
+					      (iostat_print_counter_t)print_counter, &os);
 		} else if (config->cgroup_list) {
-			print_cgroup_counter(config, evlist, os.prefix);
+			print_cgroup_counter(config, evlist, &os);
 		} else {
 			print_metric_begin(config, evlist, &os, /*aggr_idx=*/0);
 			evlist__for_each_entry(evlist, counter) {
-				print_counter(config, counter, os.prefix);
+				print_counter(config, counter, &os);
 			}
 			print_metric_end(config);
 		}
 		break;
 	case AGGR_NONE:
 		if (metric_only)
-			print_no_aggr_metric(config, evlist, os.prefix);
+			print_no_aggr_metric(config, evlist, &os);
 		else {
 			evlist__for_each_entry(evlist, counter) {
 				if (counter->percore)
-					print_percore(config, counter, os.prefix);
+					print_percore(config, counter, &os);
 				else
-					print_counter(config, counter, os.prefix);
+					print_counter(config, counter, &os);
 			}
 		}
 		break;
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH 13/15] perf stat: Fix JSON output in metric-only mode

[Namhyung Kim]

It generated a broken JSON output when aggregation mode or cgroup is
used with --metric-only option.  Also get rid of the header line and
make the output single line for each entry.

It needs to know whether the current metric is the first one or not.
So add 'first' field in the outstate and mark it false after printing.

Before:
  # perf stat -a -j --metric-only true
  {"unit" : "GHz"}{"unit" : "insn per cycle"}{"unit" : "branch-misses of all branches"}
  {{"metric-value" : "0.797"}{"metric-value" : "1.65"}{"metric-value" : "0.89"}
   ^

  # perf stat -a -j --metric-only --per-socket true
  {"unit" : "GHz"}{"unit" : "insn per cycle"}{"unit" : "branch-misses of all branches"}
  {"socket" : "S0", "aggregate-number" : 8, {"metric-value" : "0.295"}{"metric-value" : "1.88"}{"metric-value" : "0.64"}
                                           ^

After:
  # perf stat -a -j --metric-only true
  {"GHz" : "0.990", "insn per cycle" : "2.06", "branch-misses of all branches" : "0.59"}

  # perf stat -a -j --metric-only --per-socket true
  {"socket" : "S0", "aggregate-number" : 8, "GHz" : "0.439", "insn per cycle" : "2.14", "branch-misses of all branches" : "0.51"}

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

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index 335627e8542d..43640115454c 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -279,9 +279,6 @@ static void print_aggr_id_json(struct perf_stat_config *config,
 {
 	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, ",
@@ -335,6 +332,7 @@ static void aggr_printout(struct perf_stat_config *config,
 struct outstate {
 	FILE *fh;
 	bool newline;
+	bool first;
 	const char *prefix;
 	int  nfields;
 	int  nr;
@@ -491,6 +489,7 @@ static void print_metric_only(struct perf_stat_config *config,
 
 	color_snprintf(str, sizeof(str), color ?: "", fmt, val);
 	fprintf(out, "%*s ", mlen, str);
+	os->first = false;
 }
 
 static void print_metric_only_csv(struct perf_stat_config *config __maybe_unused,
@@ -512,6 +511,7 @@ static void print_metric_only_csv(struct perf_stat_config *config __maybe_unused
 		ends++;
 	*ends = 0;
 	fprintf(out, "%s%s", vals, config->csv_sep);
+	os->first = false;
 }
 
 static void print_metric_only_json(struct perf_stat_config *config __maybe_unused,
@@ -532,7 +532,8 @@ static void print_metric_only_json(struct perf_stat_config *config __maybe_unuse
 	while (isdigit(*ends) || *ends == '.')
 		ends++;
 	*ends = 0;
-	fprintf(out, "{\"metric-value\" : \"%s\"}", vals);
+	fprintf(out, "%s\"%s\" : \"%s\"", os->first ? "" : ", ", unit, vals);
+	os->first = false;
 }
 
 static void new_line_metric(struct perf_stat_config *config __maybe_unused,
@@ -561,7 +562,7 @@ static void print_metric_header(struct perf_stat_config *config,
 	unit = fixunit(tbuf, os->evsel, unit);
 
 	if (config->json_output)
-		fprintf(os->fh, "{\"unit\" : \"%s\"}", unit);
+		return;
 	else if (config->csv_output)
 		fprintf(os->fh, "%s%s", unit, config->csv_sep);
 	else
@@ -821,6 +822,8 @@ static void print_counter_aggrdata(struct perf_stat_config *config,
 	run = aggr->counts.run;
 
 	if (!metric_only) {
+		if (config->json_output)
+			fputc('{', output);
 		if (os->prefix)
 			fprintf(output, "%s", os->prefix);
 		else if (config->summary && config->csv_output &&
@@ -844,9 +847,12 @@ static void print_metric_begin(struct perf_stat_config *config,
 	struct aggr_cpu_id id;
 	struct evsel *evsel;
 
+	os->first = true;
 	if (!config->metric_only)
 		return;
 
+	if (config->json_output)
+		fputc('{', config->output);
 	if (os->prefix)
 		fprintf(config->output, "%s", os->prefix);
 
@@ -855,7 +861,7 @@ static void print_metric_begin(struct perf_stat_config *config,
 	aggr = &evsel->stats->aggr[aggr_idx];
 	aggr_printout(config, evsel, id, aggr->nr);
 
-	print_cgroup(config, os->cgrp);
+	print_cgroup(config, os->cgrp ? : evsel->cgrp);
 }
 
 static void print_metric_end(struct perf_stat_config *config)
@@ -863,6 +869,8 @@ static void print_metric_end(struct perf_stat_config *config)
 	if (!config->metric_only)
 		return;
 
+	if (config->json_output)
+		fputc('}', config->output);
 	fputc('\n', config->output);
 }
 
@@ -1005,11 +1013,9 @@ static void print_metric_headers_csv(struct perf_stat_config *config,
 		fputs(aggr_header_csv[config->aggr_mode], config->output);
 }
 
-static void print_metric_headers_json(struct perf_stat_config *config,
+static void print_metric_headers_json(struct perf_stat_config *config __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,
@@ -1049,7 +1055,9 @@ static void print_metric_headers(struct perf_stat_config *config,
 					      &config->metric_events,
 					      &rt_stat);
 	}
-	fputc('\n', config->output);
+
+	if (!config->json_output)
+		fputc('\n', config->output);
 }
 
 static void prepare_interval(struct perf_stat_config *config,
@@ -1058,17 +1066,14 @@ static void prepare_interval(struct perf_stat_config *config,
 	if (config->iostat_run)
 		return;
 
-	if (config->csv_output)
+	if (config->json_output)
+		scnprintf(prefix, len, "\"interval\" : %lu.%09lu, ",
+			  (unsigned long) ts->tv_sec, ts->tv_nsec);
+	else if (config->csv_output)
 		scnprintf(prefix, len, "%lu.%09lu%s",
 			  (unsigned long) ts->tv_sec, ts->tv_nsec, config->csv_sep);
-	else if (!config->json_output)
-		scnprintf(prefix, len, "%6lu.%09lu ",
-			  (unsigned long) ts->tv_sec, ts->tv_nsec);
-	else if (!config->metric_only)
-		scnprintf(prefix, len, "{\"interval\" : %lu.%09lu, ",
-			  (unsigned long) ts->tv_sec, ts->tv_nsec);
 	else
-		scnprintf(prefix, len, "{\"interval\" : %lu.%09lu}",
+		scnprintf(prefix, len, "%6lu.%09lu ",
 			  (unsigned long) ts->tv_sec, ts->tv_nsec);
 }
 
@@ -1365,6 +1370,7 @@ void evlist__print_counters(struct evlist *evlist, struct perf_stat_config *conf
 	char buf[64];
 	struct outstate os = {
 		.fh = config->output,
+		.first = true,
 	};
 
 	if (config->iostat_run)
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH 14/15] perf stat: Rename "aggregate-number" to "cpu-count" in JSON

[Namhyung Kim]

As the JSON output has been broken for a little while, I guess there are
not many users.  Let's rename the field to more intuitive one. :)

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

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index 43640115454c..7a39a1a7261d 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -281,19 +281,19 @@ static void print_aggr_id_json(struct perf_stat_config *config,
 
 	switch (config->aggr_mode) {
 	case AGGR_CORE:
-		fprintf(output, "\"core\" : \"S%d-D%d-C%d\", \"aggregate-number\" : %d, ",
+		fprintf(output, "\"core\" : \"S%d-D%d-C%d\", \"cpu-count\" : %d, ",
 			id.socket, id.die, id.core, nr);
 		break;
 	case AGGR_DIE:
-		fprintf(output, "\"die\" : \"S%d-D%d\", \"aggregate-number\" : %d, ",
+		fprintf(output, "\"die\" : \"S%d-D%d\", \"cpu-count\" : %d, ",
 			id.socket, id.die, nr);
 		break;
 	case AGGR_SOCKET:
-		fprintf(output, "\"socket\" : \"S%d\", \"aggregate-number\" : %d, ",
+		fprintf(output, "\"socket\" : \"S%d\", \"cpu-count\" : %d, ",
 			id.socket, nr);
 		break;
 	case AGGR_NODE:
-		fprintf(output, "\"node\" : \"N%d\", \"aggregate-number\" : %d, ",
+		fprintf(output, "\"node\" : \"N%d\", \"cpu-count\" : %d, ",
 			id.node, nr);
 		break;
 	case AGGR_NONE:
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH 15/15] perf stat: Tidy up JSON metric-only output when no metrics

[Namhyung Kim]

It printed empty strings for each metric.  I guess it's needed for CSV
output to match the column number.  We could just ignore the empty
metrics in JSON but it ended up with a broken JSON object with a
trailing comma.

So I added a dummy '"metric-value" : "none"' part.  To do that, it
needs to pass struct outstate to print_metric_end() to check if any
metric value is printed or not.

Before:
  # perf stat -aj --metric-only --per-socket --for-each-cgroup system.slice true
  {"socket" : "S0", "cpu-count" : 8, "cgroup" : "system.slice", "" : "", "" : "", "" : "", "" : "", "" : "", "" : "", "" : "", "" : ""}

After:
  # perf stat -aj --metric-only --per-socket --for-each-cgroup system.slice true
  {"socket" : "S0", "cpu-count" : 8, "cgroup" : "system.slice", "metric-value" : "none"}

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

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index 7a39a1a7261d..847acdb5dc40 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -532,6 +532,8 @@ static void print_metric_only_json(struct perf_stat_config *config __maybe_unuse
 	while (isdigit(*ends) || *ends == '.')
 		ends++;
 	*ends = 0;
+	if (!unit[0] || !vals[0])
+		return;
 	fprintf(out, "%s\"%s\" : \"%s\"", os->first ? "" : ", ", unit, vals);
 	os->first = false;
 }
@@ -864,14 +866,19 @@ static void print_metric_begin(struct perf_stat_config *config,
 	print_cgroup(config, os->cgrp ? : evsel->cgrp);
 }
 
-static void print_metric_end(struct perf_stat_config *config)
+static void print_metric_end(struct perf_stat_config *config, struct outstate *os)
 {
+	FILE *output = config->output;
+
 	if (!config->metric_only)
 		return;
 
-	if (config->json_output)
-		fputc('}', config->output);
-	fputc('\n', config->output);
+	if (config->json_output) {
+		if (os->first)
+			fputs("\"metric-value\" : \"none\"", output);
+		fputc('}', output);
+	}
+	fputc('\n', output);
 }
 
 static void print_aggr(struct perf_stat_config *config,
@@ -897,7 +904,7 @@ static void print_aggr(struct perf_stat_config *config,
 
 			print_counter_aggrdata(config, counter, s, os);
 		}
-		print_metric_end(config);
+		print_metric_end(config, os);
 	}
 }
 
@@ -929,7 +936,7 @@ static void print_aggr_cgroup(struct perf_stat_config *config,
 
 				print_counter_aggrdata(config, counter, s, os);
 			}
-			print_metric_end(config);
+			print_metric_end(config, os);
 		}
 	}
 }
@@ -985,7 +992,7 @@ static void print_no_aggr_metric(struct perf_stat_config *config,
 			printout(config, os, uval, run, ena, 1.0, counter_idx);
 		}
 		if (!first)
-			print_metric_end(config);
+			print_metric_end(config, os);
 	}
 }
 
@@ -1348,7 +1355,7 @@ static void print_cgroup_counter(struct perf_stat_config *config, struct evlist
 	evlist__for_each_entry(evlist, counter) {
 		if (os->cgrp != counter->cgrp) {
 			if (os->cgrp != NULL)
-				print_metric_end(config);
+				print_metric_end(config, os);
 
 			os->cgrp = counter->cgrp;
 			print_metric_begin(config, evlist, os, /*aggr_idx=*/0);
@@ -1357,7 +1364,7 @@ static void print_cgroup_counter(struct perf_stat_config *config, struct evlist
 		print_counter(config, counter, os);
 	}
 	if (os->cgrp)
-		print_metric_end(config);
+		print_metric_end(config, os);
 }
 
 void evlist__print_counters(struct evlist *evlist, struct perf_stat_config *config,
@@ -1405,7 +1412,7 @@ void evlist__print_counters(struct evlist *evlist, struct perf_stat_config *conf
 			evlist__for_each_entry(evlist, counter) {
 				print_counter(config, counter, &os);
 			}
-			print_metric_end(config);
+			print_metric_end(config, &os);
 		}
 		break;
 	case AGGR_NONE:
-- 
2.38.1.584.g0f3c55d4c2-goog

Re: [PATCH 01/15] perf stat: Fix cgroup display in JSON output

[Ian Rogers]

On Wed, Nov 23, 2022 at 10:02 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> It missed the 'else' keyword after checking json output mode.
>
> Fixes: 41cb875242e7 ("perf stat: Split print_cgroup() function")
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  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 f5501760ff2e..46e90f0bb423 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -175,7 +175,7 @@ static void print_cgroup(struct perf_stat_config *config, struct cgroup *cgrp)
>
>                 if (config->json_output)
>                         print_cgroup_json(config, cgrp_name);
> -               if (config->csv_output)
> +               else if (config->csv_output)
>                         print_cgroup_csv(config, cgrp_name);
>                 else
>                         print_cgroup_std(config, cgrp_name);
> --
> 2.38.1.584.g0f3c55d4c2-goog
>

Re: [PATCH 02/15] perf stat: Move summary prefix printing logic in CSV output

[Ian Rogers]

On Wed, Nov 23, 2022 at 10:02 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> It matches to the prefix (interval timestamp), so better to have them together.
> No functional change intended.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/util/stat-display.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index 46e90f0bb423..d86f2f8e020d 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -713,11 +713,6 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
>                 nl = config->metric_only ? new_line_metric : new_line_std;
>         }
>
> -       if (!config->no_csv_summary && config->csv_output &&
> -           config->summary && !config->interval && !config->metric_only) {
> -               fprintf(config->output, "%16s%s", "summary", config->csv_sep);
> -       }
> -
>         if (run == 0 || ena == 0 || counter->counts->scaled == -1) {
>                 if (config->metric_only) {
>                         pm(config, &os, NULL, "", "", 0);
> @@ -828,8 +823,13 @@ static void print_counter_aggrdata(struct perf_stat_config *config,
>         ena = aggr->counts.ena;
>         run = aggr->counts.run;
>
> -       if (prefix && !metric_only)
> -               fprintf(output, "%s", prefix);
> +       if (!metric_only) {
> +               if (prefix)
> +                       fprintf(output, "%s", prefix);
> +               else if (config->summary && config->csv_output &&
> +                        !config->no_csv_summary && !config->interval)
> +                       fprintf(output, "%16s%s", "summary", config->csv_sep);
> +       }
>
>         uval = val * counter->scale;
>
> --
> 2.38.1.584.g0f3c55d4c2-goog
>

Re: [PATCH 03/15] perf stat: Do not align time prefix in CSV output

[Ian Rogers]

On Wed, Nov 23, 2022 at 10:02 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> We don't care about the alignment in the CSV output as it's intended for machine
> processing.  Let's get rid of it to make the output more compact.
>
> Before:
>   # perf stat -a --summary -I 1 -x, true
>        0.001149309,219.20,msec,cpu-clock,219322251,100.00,219.200,CPUs utilized
>        0.001149309,144,,context-switches,219241902,100.00,656.935,/sec
>        0.001149309,38,,cpu-migrations,219173705,100.00,173.358,/sec
>        0.001149309,61,,page-faults,219093635,100.00,278.285,/sec
>        0.001149309,10679310,,cycles,218746228,100.00,0.049,GHz
>        0.001149309,6288296,,instructions,218589869,100.00,0.59,insn per cycle
>        0.001149309,1386904,,branches,218428851,100.00,6.327,M/sec
>        0.001149309,56863,,branch-misses,218219951,100.00,4.10,of all branches
>            summary,219.20,msec,cpu-clock,219322251,100.00,20.025,CPUs utilized
>            summary,144,,context-switches,219241902,100.00,656.935,/sec
>            summary,38,,cpu-migrations,219173705,100.00,173.358,/sec
>            summary,61,,page-faults,219093635,100.00,278.285,/sec
>            summary,10679310,,cycles,218746228,100.00,0.049,GHz
>            summary,6288296,,instructions,218589869,100.00,0.59,insn per cycle
>            summary,1386904,,branches,218428851,100.00,6.327,M/sec
>            summary,56863,,branch-misses,218219951,100.00,4.10,of all branches
>
> After:
>   0.001148449,224.75,msec,cpu-clock,224870589,100.00,224.747,CPUs utilized
>   0.001148449,176,,context-switches,224775564,100.00,783.103,/sec
>   0.001148449,38,,cpu-migrations,224707428,100.00,169.079,/sec
>   0.001148449,61,,page-faults,224629326,100.00,271.416,/sec
>   0.001148449,12172071,,cycles,224266368,100.00,0.054,GHz
>   0.001148449,6901907,,instructions,224108764,100.00,0.57,insn per cycle
>   0.001148449,1515655,,branches,223946693,100.00,6.744,M/sec
>   0.001148449,70027,,branch-misses,223735385,100.00,4.62,of all branches
>   summary,224.75,msec,cpu-clock,224870589,100.00,21.066,CPUs utilized
>   summary,176,,context-switches,224775564,100.00,783.103,/sec
>   summary,38,,cpu-migrations,224707428,100.00,169.079,/sec
>   summary,61,,page-faults,224629326,100.00,271.416,/sec
>   summary,12172071,,cycles,224266368,100.00,0.054,GHz
>   summary,6901907,,instructions,224108764,100.00,0.57,insn per cycle
>   summary,1515655,,branches,223946693,100.00,6.744,M/sec
>   summary,70027,,branch-misses,223735385,100.00,4.62,of all branches
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/util/stat-display.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index d86f2f8e020d..15c88b9b5aa3 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -828,7 +828,7 @@ static void print_counter_aggrdata(struct perf_stat_config *config,
>                         fprintf(output, "%s", prefix);
>                 else if (config->summary && config->csv_output &&
>                          !config->no_csv_summary && !config->interval)
> -                       fprintf(output, "%16s%s", "summary", config->csv_sep);
> +                       fprintf(output, "%s%s", "summary", config->csv_sep);
>         }
>
>         uval = val * counter->scale;
> @@ -1078,9 +1078,12 @@ static void prepare_interval(struct perf_stat_config *config,
>         if (config->iostat_run)
>                 return;
>
> -       if (!config->json_output)
> -               sprintf(prefix, "%6lu.%09lu%s", (unsigned long) ts->tv_sec,
> +       if (config->csv_output)
> +               sprintf(prefix, "%lu.%09lu%s", (unsigned long) ts->tv_sec,
>                                  ts->tv_nsec, config->csv_sep);
> +       else if (!config->json_output)
> +               sprintf(prefix, "%6lu.%09lu ", (unsigned long) ts->tv_sec,
> +                                ts->tv_nsec);
>         else if (!config->metric_only)
>                 sprintf(prefix, "{\"interval\" : %lu.%09lu, ", (unsigned long)
>                                  ts->tv_sec, ts->tv_nsec);
> --
> 2.38.1.584.g0f3c55d4c2-goog
>

Re: [PATCH 04/15] perf stat: Use scnprintf() in prepare_interval()

[Ian Rogers]

On Wed, Nov 23, 2022 at 10:02 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> It should not use sprintf() anymore.  Let's pass the buffer size and use the
> safer scnprintf() instead.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/util/stat-display.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index 15c88b9b5aa3..744b7a40f59a 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -1073,23 +1073,23 @@ static void print_metric_headers(struct perf_stat_config *config,
>  }
>
>  static void prepare_interval(struct perf_stat_config *config,
> -                            char *prefix, struct timespec *ts)
> +                            char *prefix, size_t len, struct timespec *ts)
>  {
>         if (config->iostat_run)
>                 return;
>
>         if (config->csv_output)
> -               sprintf(prefix, "%lu.%09lu%s", (unsigned long) ts->tv_sec,
> -                                ts->tv_nsec, config->csv_sep);
> +               scnprintf(prefix, len, "%lu.%09lu%s",
> +                         (unsigned long) ts->tv_sec, ts->tv_nsec, config->csv_sep);
>         else if (!config->json_output)
> -               sprintf(prefix, "%6lu.%09lu ", (unsigned long) ts->tv_sec,
> -                                ts->tv_nsec);
> +               scnprintf(prefix, len, "%6lu.%09lu ",
> +                         (unsigned long) ts->tv_sec, ts->tv_nsec);
>         else if (!config->metric_only)
> -               sprintf(prefix, "{\"interval\" : %lu.%09lu, ", (unsigned long)
> -                                ts->tv_sec, ts->tv_nsec);
> +               scnprintf(prefix, len, "{\"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);
> +               scnprintf(prefix, len, "{\"interval\" : %lu.%09lu}",
> +                         (unsigned long) ts->tv_sec, ts->tv_nsec);
>  }
>
>  static void print_header_interval_std(struct perf_stat_config *config,
> @@ -1390,7 +1390,7 @@ void evlist__print_counters(struct evlist *evlist, struct perf_stat_config *conf
>
>         if (interval) {
>                 prefix = buf;
> -               prepare_interval(config, prefix, ts);
> +               prepare_interval(config, buf, sizeof(buf), ts);
>         }
>
>         print_header(config, _target, evlist, argc, argv);
> --
> 2.38.1.584.g0f3c55d4c2-goog
>

Re: [PATCH 05/15] perf stat: Remove prefix argument in print_metric_headers()

[Ian Rogers]

On Wed, Nov 23, 2022 at 10:02 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> It always passes a whitespace to the function, thus we can just add it to the
> function body.  Furthermore, it's only used in the normal output mode.
>
> Well, actually CSV used it but it doesn't need to since we don't care about the
> indentation or alignment in the CSV output.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/util/stat-display.c | 26 ++++++++++----------------
>  1 file changed, 10 insertions(+), 16 deletions(-)
>
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index 744b7a40f59a..deed6ccf072f 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -996,10 +996,9 @@ static void print_no_aggr_metric(struct perf_stat_config *config,
>  }
>
>  static void print_metric_headers_std(struct perf_stat_config *config,
> -                                    const char *prefix, bool no_indent)
> +                                    bool no_indent)
>  {
> -       if (prefix)
> -               fprintf(config->output, "%s", prefix);
> +       fputc(' ', config->output);
>
>         if (!no_indent) {
>                 int len = aggr_header_lens[config->aggr_mode];
> @@ -1012,11 +1011,8 @@ static void print_metric_headers_std(struct perf_stat_config *config,
>  }
>
>  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)
> @@ -1024,7 +1020,6 @@ static void print_metric_headers_csv(struct perf_stat_config *config,
>  }
>
>  static void print_metric_headers_json(struct perf_stat_config *config,
> -                                     const char *prefix __maybe_unused,
>                                       bool no_indent __maybe_unused)
>  {
>         if (config->interval)
> @@ -1032,8 +1027,7 @@ static void print_metric_headers_json(struct perf_stat_config *config,
>  }
>
>  static void print_metric_headers(struct perf_stat_config *config,
> -                                struct evlist *evlist,
> -                                const char *prefix, bool no_indent)
> +                                struct evlist *evlist, bool no_indent)
>  {
>         struct evsel *counter;
>         struct outstate os = {
> @@ -1047,11 +1041,11 @@ static void print_metric_headers(struct perf_stat_config *config,
>         };
>
>         if (config->json_output)
> -               print_metric_headers_json(config, prefix, no_indent);
> +               print_metric_headers_json(config, no_indent);
>         else if (config->csv_output)
> -               print_metric_headers_csv(config, prefix, no_indent);
> +               print_metric_headers_csv(config, no_indent);
>         else
> -               print_metric_headers_std(config, prefix, no_indent);
> +               print_metric_headers_std(config, no_indent);
>
>         if (config->iostat_run)
>                 iostat_print_header_prefix(config);
> @@ -1132,7 +1126,7 @@ static void print_header_interval_std(struct perf_stat_config *config,
>         }
>
>         if (config->metric_only)
> -               print_metric_headers(config, evlist, " ", true);
> +               print_metric_headers(config, evlist, true);
>         else
>                 fprintf(output, " %*s %*s events\n",
>                         COUNTS_LEN, "counts", config->unit_width, "unit");
> @@ -1168,7 +1162,7 @@ static void print_header_std(struct perf_stat_config *config,
>         fprintf(output, ":\n\n");
>
>         if (config->metric_only)
> -               print_metric_headers(config, evlist, " ", false);
> +               print_metric_headers(config, evlist, false);
>  }
>
>  static void print_header_csv(struct perf_stat_config *config,
> @@ -1178,7 +1172,7 @@ static void print_header_csv(struct perf_stat_config *config,
>                              const char **argv __maybe_unused)
>  {
>         if (config->metric_only)
> -               print_metric_headers(config, evlist, " ", true);
> +               print_metric_headers(config, evlist, true);
>  }
>  static void print_header_json(struct perf_stat_config *config,
>                               struct target *_target __maybe_unused,
> @@ -1187,7 +1181,7 @@ static void print_header_json(struct perf_stat_config *config,
>                               const char **argv __maybe_unused)
>  {
>         if (config->metric_only)
> -               print_metric_headers(config, evlist, " ", true);
> +               print_metric_headers(config, evlist, true);
>  }
>
>  static void print_header(struct perf_stat_config *config,
> --
> 2.38.1.584.g0f3c55d4c2-goog
>

Re: [PATCH 06/15] perf stat: Remove metric_only argument in print_counter_aggrdata()

[Ian Rogers]

On Wed, Nov 23, 2022 at 10:02 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> It already passes the stat_config argument, then it can find the value in the
> config.  No need to pass it separately.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/util/stat-display.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index deed6ccf072f..b8432c0a0ec3 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -804,7 +804,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)
> +                                  char *prefix)
>  {
>         FILE *output = config->output;
>         u64 ena, run, val;
> @@ -813,6 +813,7 @@ static void print_counter_aggrdata(struct perf_stat_config *config,
>         struct perf_stat_aggr *aggr = &ps->aggr[s];
>         struct aggr_cpu_id id = config->aggr_map->map[s];
>         double avg = aggr->counts.val;
> +       bool metric_only = config->metric_only;
>
>         if (counter->supported && aggr->nr == 0)
>                 return;
> @@ -875,7 +876,6 @@ static void print_aggr(struct perf_stat_config *config,
>                        struct evlist *evlist,
>                        char *prefix)
>  {
> -       bool metric_only = config->metric_only;
>         struct evsel *counter;
>         int s;
>
> @@ -893,8 +893,7 @@ static void print_aggr(struct perf_stat_config *config,
>                         if (counter->merged_stat)
>                                 continue;
>
> -                       print_counter_aggrdata(config, counter, s, prefix,
> -                                              metric_only);
> +                       print_counter_aggrdata(config, counter, s, prefix);
>                 }
>                 print_metric_end(config);
>         }
> @@ -904,7 +903,6 @@ 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;
> @@ -928,8 +926,7 @@ static void print_aggr_cgroup(struct perf_stat_config *config,
>                                 if (counter->cgrp != cgrp)
>                                         continue;
>
> -                               print_counter_aggrdata(config, counter, s, prefix,
> -                                                      metric_only);
> +                               print_counter_aggrdata(config, counter, s, prefix);
>                         }
>                         print_metric_end(config);
>                 }
> @@ -939,7 +936,6 @@ static void print_aggr_cgroup(struct perf_stat_config *config,
>  static void print_counter(struct perf_stat_config *config,
>                           struct evsel *counter, char *prefix)
>  {
> -       bool metric_only = config->metric_only;
>         int s;
>
>         /* AGGR_THREAD doesn't have config->aggr_get_id */
> @@ -950,8 +946,7 @@ 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);
> +               print_counter_aggrdata(config, counter, s, prefix);
>         }
>  }
>
> @@ -1339,7 +1334,7 @@ static void print_percore(struct perf_stat_config *config,
>                 if (found)
>                         continue;
>
> -               print_counter_aggrdata(config, counter, s, prefix, metric_only);
> +               print_counter_aggrdata(config, counter, s, prefix);
>
>                 core_map->map[c++] = core_id;
>         }
> --
> 2.38.1.584.g0f3c55d4c2-goog
>

Re: [PATCH 07/15] perf stat: Pass const char *prefix to display routines

[Ian Rogers]

On Wed, Nov 23, 2022 at 10:02 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> This is a minor cleanup and preparation for the later change.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/util/iostat.h       |  2 +-
>  tools/perf/util/stat-display.c | 18 +++++++++---------
>  2 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/tools/perf/util/iostat.h b/tools/perf/util/iostat.h
> index 23c1c46a331a..c22688f87cb2 100644
> --- a/tools/perf/util/iostat.h
> +++ b/tools/perf/util/iostat.h
> @@ -28,7 +28,7 @@ enum iostat_mode_t {
>
>  extern enum iostat_mode_t iostat_mode;
>
> -typedef void (*iostat_print_counter_t)(struct perf_stat_config *, struct evsel *, char *);
> +typedef void (*iostat_print_counter_t)(struct perf_stat_config *, struct evsel *, const char *);
>
>  int iostat_prepare(struct evlist *evlist, struct perf_stat_config *config);
>  int iostat_parse(const struct option *opt, const char *str,
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index b8432c0a0ec3..d2894a519d61 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -675,7 +675,7 @@ static bool is_mixed_hw_group(struct evsel *counter)
>
>  static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int nr,
>                      struct evsel *counter, double uval,
> -                    char *prefix, u64 run, u64 ena, double noise,
> +                    const char *prefix, u64 run, u64 ena, double noise,
>                      struct runtime_stat *st, int map_idx)
>  {
>         struct perf_stat_output_ctx out;
> @@ -804,7 +804,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)
> +                                  const char *prefix)
>  {
>         FILE *output = config->output;
>         u64 ena, run, val;
> @@ -843,7 +843,7 @@ 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,
> +                              const char *prefix, int aggr_idx,
>                                struct cgroup *cgrp)
>  {
>         struct perf_stat_aggr *aggr;
> @@ -874,7 +874,7 @@ static void print_metric_end(struct perf_stat_config *config)
>
>  static void print_aggr(struct perf_stat_config *config,
>                        struct evlist *evlist,
> -                      char *prefix)
> +                      const char *prefix)
>  {
>         struct evsel *counter;
>         int s;
> @@ -901,7 +901,7 @@ static void print_aggr(struct perf_stat_config *config,
>
>  static void print_aggr_cgroup(struct perf_stat_config *config,
>                               struct evlist *evlist,
> -                             char *prefix)
> +                             const char *prefix)
>  {
>         struct evsel *counter, *evsel;
>         struct cgroup *cgrp = NULL;
> @@ -934,7 +934,7 @@ static void print_aggr_cgroup(struct perf_stat_config *config,
>  }
>
>  static void print_counter(struct perf_stat_config *config,
> -                         struct evsel *counter, char *prefix)
> +                         struct evsel *counter, const char *prefix)
>  {
>         int s;
>
> @@ -952,7 +952,7 @@ static void print_counter(struct perf_stat_config *config,
>
>  static void print_no_aggr_metric(struct perf_stat_config *config,
>                                  struct evlist *evlist,
> -                                char *prefix)
> +                                const char *prefix)
>  {
>         int all_idx;
>         struct perf_cpu cpu;
> @@ -1301,7 +1301,7 @@ static void print_footer(struct perf_stat_config *config)
>  }
>
>  static void print_percore(struct perf_stat_config *config,
> -                         struct evsel *counter, char *prefix)
> +                         struct evsel *counter, const char *prefix)
>  {
>         bool metric_only = config->metric_only;
>         FILE *output = config->output;
> @@ -1345,7 +1345,7 @@ static void print_percore(struct perf_stat_config *config,
>  }
>
>  static void print_cgroup_counter(struct perf_stat_config *config, struct evlist *evlist,
> -                                char *prefix)
> +                                const char *prefix)
>  {
>         struct cgroup *cgrp = NULL;
>         struct evsel *counter;
> --
> 2.38.1.584.g0f3c55d4c2-goog
>

Re: [PATCH 08/15] perf stat: Use struct outstate in evlist__print_counters()

[Ian Rogers]

On Wed, Nov 23, 2022 at 10:02 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> This is a preparation for the later cleanup.  No functional changes
> intended.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/util/stat-display.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index d2894a519d61..70aebf359e16 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -1372,13 +1372,16 @@ void evlist__print_counters(struct evlist *evlist, struct perf_stat_config *conf
>         bool metric_only = config->metric_only;
>         int interval = config->interval;
>         struct evsel *counter;
> -       char buf[64], *prefix = NULL;
> +       char buf[64];
> +       struct outstate os = {
> +               .fh = config->output,
> +       };
>
>         if (config->iostat_run)
>                 evlist->selected = evlist__first(evlist);
>
>         if (interval) {
> -               prefix = buf;
> +               os.prefix = buf;
>                 prepare_interval(config, buf, sizeof(buf), ts);
>         }
>
> @@ -1390,35 +1393,35 @@ void evlist__print_counters(struct evlist *evlist, struct perf_stat_config *conf
>         case AGGR_SOCKET:
>         case AGGR_NODE:
>                 if (config->cgroup_list)
> -                       print_aggr_cgroup(config, evlist, prefix);
> +                       print_aggr_cgroup(config, evlist, os.prefix);
>                 else
> -                       print_aggr(config, evlist, prefix);
> +                       print_aggr(config, evlist, os.prefix);
>                 break;
>         case AGGR_THREAD:
>         case AGGR_GLOBAL:
>                 if (config->iostat_run) {
> -                       iostat_print_counters(evlist, config, ts, prefix = buf,
> +                       iostat_print_counters(evlist, config, ts, buf,
>                                               print_counter);
>                 } else if (config->cgroup_list) {
> -                       print_cgroup_counter(config, evlist, prefix);
> +                       print_cgroup_counter(config, evlist, os.prefix);
>                 } else {
> -                       print_metric_begin(config, evlist, prefix,
> +                       print_metric_begin(config, evlist, os.prefix,
>                                            /*aggr_idx=*/0, /*cgrp=*/NULL);
>                         evlist__for_each_entry(evlist, counter) {
> -                               print_counter(config, counter, prefix);
> +                               print_counter(config, counter, os.prefix);
>                         }
>                         print_metric_end(config);
>                 }
>                 break;
>         case AGGR_NONE:
>                 if (metric_only)
> -                       print_no_aggr_metric(config, evlist, prefix);
> +                       print_no_aggr_metric(config, evlist, os.prefix);
>                 else {
>                         evlist__for_each_entry(evlist, counter) {
>                                 if (counter->percore)
> -                                       print_percore(config, counter, prefix);
> +                                       print_percore(config, counter, os.prefix);
>                                 else
> -                                       print_counter(config, counter, prefix);
> +                                       print_counter(config, counter, os.prefix);
>                         }
>                 }
>                 break;
> --
> 2.38.1.584.g0f3c55d4c2-goog
>

Re: [PATCH 09/15] perf stat: Pass struct outstate to print_metric_begin()

[Ian Rogers]

On Wed, Nov 23, 2022 at 10:02 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> It passes prefix and cgroup pointers but the outstate already has them.
> Let's pass the outstate pointer instead.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/util/stat-display.c | 50 +++++++++++++++++++---------------
>  1 file changed, 28 insertions(+), 22 deletions(-)
>
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index 70aebf359e16..3ed63061d6f8 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -843,8 +843,7 @@ static void print_counter_aggrdata(struct perf_stat_config *config,
>
>  static void print_metric_begin(struct perf_stat_config *config,
>                                struct evlist *evlist,
> -                              const char *prefix, int aggr_idx,
> -                              struct cgroup *cgrp)
> +                              struct outstate *os, int aggr_idx)
>  {
>         struct perf_stat_aggr *aggr;
>         struct aggr_cpu_id id;
> @@ -853,15 +852,15 @@ static void print_metric_begin(struct perf_stat_config *config,
>         if (!config->metric_only)
>                 return;
>
> -       if (prefix)
> -               fprintf(config->output, "%s", prefix);
> +       if (os->prefix)
> +               fprintf(config->output, "%s", os->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);
>
> -       print_cgroup(config, cgrp);
> +       print_cgroup(config, os->cgrp);
>  }
>
>  static void print_metric_end(struct perf_stat_config *config)
> @@ -877,6 +876,9 @@ static void print_aggr(struct perf_stat_config *config,
>                        const char *prefix)
>  {
>         struct evsel *counter;
> +       struct outstate os = {
> +               .prefix = prefix,
> +       };
>         int s;
>
>         if (!config->aggr_map || !config->aggr_get_id)
> @@ -887,7 +889,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, /*cgrp=*/NULL);
> +               print_metric_begin(config, evlist, &os, s);
>
>                 evlist__for_each_entry(evlist, counter) {
>                         if (counter->merged_stat)
> @@ -904,26 +906,28 @@ static void print_aggr_cgroup(struct perf_stat_config *config,
>                               const char *prefix)
>  {
>         struct evsel *counter, *evsel;
> -       struct cgroup *cgrp = NULL;
> +       struct outstate os = {
> +               .prefix = prefix,
> +       };
>         int s;
>
>         if (!config->aggr_map || !config->aggr_get_id)
>                 return;
>
>         evlist__for_each_entry(evlist, evsel) {
> -               if (cgrp == evsel->cgrp)
> +               if (os.cgrp == evsel->cgrp)
>                         continue;
>
> -               cgrp = evsel->cgrp;
> +               os.cgrp = evsel->cgrp;
>
>                 for (s = 0; s < config->aggr_map->nr; s++) {
> -                       print_metric_begin(config, evlist, prefix, s, cgrp);
> +                       print_metric_begin(config, evlist, &os, s);
>
>                         evlist__for_each_entry(evlist, counter) {
>                                 if (counter->merged_stat)
>                                         continue;
>
> -                               if (counter->cgrp != cgrp)
> +                               if (counter->cgrp != os.cgrp)
>                                         continue;
>
>                                 print_counter_aggrdata(config, counter, s, prefix);
> @@ -956,6 +960,9 @@ static void print_no_aggr_metric(struct perf_stat_config *config,
>  {
>         int all_idx;
>         struct perf_cpu cpu;
> +       struct outstate os = {
> +               .prefix = prefix,
> +       };
>
>         perf_cpu_map__for_each_cpu(cpu, all_idx, evlist->core.user_requested_cpus) {
>                 struct evsel *counter;
> @@ -973,8 +980,7 @@ 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, /*cgrp=*/NULL);
> +                               print_metric_begin(config, evlist, &os, counter_idx);
>                                 first = false;
>                         }
>                         val = ps->aggr[counter_idx].counts.val;
> @@ -1347,22 +1353,23 @@ static void print_percore(struct perf_stat_config *config,
>  static void print_cgroup_counter(struct perf_stat_config *config, struct evlist *evlist,
>                                  const char *prefix)
>  {
> -       struct cgroup *cgrp = NULL;
>         struct evsel *counter;
> +       struct outstate os = {
> +               .prefix = prefix,
> +       };
>
>         evlist__for_each_entry(evlist, counter) {
> -               if (cgrp != counter->cgrp) {
> -                       if (cgrp != NULL)
> +               if (os.cgrp != counter->cgrp) {
> +                       if (os.cgrp != NULL)
>                                 print_metric_end(config);
>
> -                       cgrp = counter->cgrp;
> -                       print_metric_begin(config, evlist, prefix,
> -                                          /*aggr_idx=*/0, cgrp);
> +                       os.cgrp = counter->cgrp;
> +                       print_metric_begin(config, evlist, &os, /*aggr_idx=*/0);
>                 }
>
>                 print_counter(config, counter, prefix);
>         }
> -       if (cgrp)
> +       if (os.cgrp)
>                 print_metric_end(config);
>  }
>
> @@ -1405,8 +1412,7 @@ void evlist__print_counters(struct evlist *evlist, struct perf_stat_config *conf
>                 } else if (config->cgroup_list) {
>                         print_cgroup_counter(config, evlist, os.prefix);
>                 } else {
> -                       print_metric_begin(config, evlist, os.prefix,
> -                                          /*aggr_idx=*/0, /*cgrp=*/NULL);
> +                       print_metric_begin(config, evlist, &os, /*aggr_idx=*/0);
>                         evlist__for_each_entry(evlist, counter) {
>                                 print_counter(config, counter, os.prefix);
>                         }
> --
> 2.38.1.584.g0f3c55d4c2-goog
>

Re: [PATCH 10/15] perf stat: Pass struct outstate to printout()

[Ian Rogers]

On Wed, Nov 23, 2022 at 10:02 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> The printout() takes a lot of arguments and sets an outstate with the
> value.  Instead, we can fill the outstate first and then pass it to
> reduce the number of arguments.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/util/stat-display.c | 38 ++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 20 deletions(-)
>
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index 3ed63061d6f8..dd190f71e933 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -673,22 +673,15 @@ static bool is_mixed_hw_group(struct evsel *counter)
>         return false;
>  }
>
> -static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int nr,
> -                    struct evsel *counter, double uval,
> -                    const char *prefix, u64 run, u64 ena, double noise,
> +static void printout(struct perf_stat_config *config, struct outstate *os,
> +                    double uval, u64 run, u64 ena, double noise,
>                      struct runtime_stat *st, int map_idx)
>  {
>         struct perf_stat_output_ctx out;
> -       struct outstate os = {
> -               .fh = config->output,
> -               .prefix = prefix ? prefix : "",
> -               .id = id,
> -               .nr = nr,
> -               .evsel = counter,
> -       };
>         print_metric_t pm;
>         new_line_t nl;
>         bool ok = true;
> +       struct evsel *counter = os->evsel;
>
>         if (config->csv_output) {
>                 static const int aggr_fields[AGGR_MAX] = {
> @@ -704,7 +697,7 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
>
>                 pm = config->metric_only ? print_metric_only_csv : print_metric_csv;
>                 nl = config->metric_only ? new_line_metric : new_line_csv;
> -               os.nfields = 3 + aggr_fields[config->aggr_mode] + (counter->cgrp ? 1 : 0);
> +               os->nfields = 3 + aggr_fields[config->aggr_mode] + (counter->cgrp ? 1 : 0);
>         } else if (config->json_output) {
>                 pm = config->metric_only ? print_metric_only_json : print_metric_json;
>                 nl = config->metric_only ? new_line_metric : new_line_json;
> @@ -715,7 +708,7 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
>
>         if (run == 0 || ena == 0 || counter->counts->scaled == -1) {
>                 if (config->metric_only) {
> -                       pm(config, &os, NULL, "", "", 0);
> +                       pm(config, os, NULL, "", "", 0);
>                         return;
>                 }
>
> @@ -732,11 +725,11 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
>
>         out.print_metric = pm;
>         out.new_line = nl;
> -       out.ctx = &os;
> +       out.ctx = os;
>         out.force_header = false;
>
>         if (!config->metric_only) {
> -               abs_printout(config, id, nr, counter, uval, ok);
> +               abs_printout(config, os->id, os->nr, counter, uval, ok);
>
>                 print_noise(config, counter, noise, /*before_metric=*/true);
>                 print_running(config, run, ena, /*before_metric=*/true);
> @@ -814,6 +807,13 @@ static void print_counter_aggrdata(struct perf_stat_config *config,
>         struct aggr_cpu_id id = config->aggr_map->map[s];
>         double avg = aggr->counts.val;
>         bool metric_only = config->metric_only;
> +       struct outstate os = {
> +               .fh = config->output,
> +               .prefix = prefix ? prefix : "",
> +               .id = id,
> +               .nr = aggr->nr,
> +               .evsel = counter,
> +       };
>
>         if (counter->supported && aggr->nr == 0)
>                 return;
> @@ -834,8 +834,7 @@ static void print_counter_aggrdata(struct perf_stat_config *config,
>
>         uval = val * counter->scale;
>
> -       printout(config, id, aggr->nr, counter, uval,
> -                prefix, run, ena, avg, &rt_stat, s);
> +       printout(config, &os, uval, run, ena, avg, &rt_stat, s);
>
>         if (!metric_only)
>                 fputc('\n', output);
> @@ -971,14 +970,14 @@ static void print_no_aggr_metric(struct perf_stat_config *config,
>                 evlist__for_each_entry(evlist, counter) {
>                         u64 ena, run, val;
>                         double uval;
> -                       struct aggr_cpu_id id;
>                         struct perf_stat_evsel *ps = counter->stats;
>                         int counter_idx = perf_cpu_map__idx(evsel__cpus(counter), cpu);
>
>                         if (counter_idx < 0)
>                                 continue;
>
> -                       id = aggr_cpu_id__cpu(cpu, /*data=*/NULL);
> +                       os.evsel = counter;
> +                       os.id = aggr_cpu_id__cpu(cpu, /*data=*/NULL);
>                         if (first) {
>                                 print_metric_begin(config, evlist, &os, counter_idx);
>                                 first = false;
> @@ -988,8 +987,7 @@ static void print_no_aggr_metric(struct perf_stat_config *config,
>                         run = ps->aggr[counter_idx].counts.run;
>
>                         uval = val * counter->scale;
> -                       printout(config, id, 0, counter, uval, prefix,
> -                                run, ena, 1.0, &rt_stat, counter_idx);
> +                       printout(config, &os, uval, run, ena, 1.0, &rt_stat, counter_idx);
>                 }
>                 if (!first)
>                         print_metric_end(config);
> --
> 2.38.1.584.g0f3c55d4c2-goog
>

Re: [PATCH 11/15] perf stat: Do not pass runtime_stat to printout()

[Ian Rogers]

On Wed, Nov 23, 2022 at 10:02 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> It always passes a pointer to rt_stat as it's the only one.  Let's not
> pass it and directly refer it in the printout().
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/util/stat-display.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index dd190f71e933..cdf4ca7f6e3a 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -674,8 +674,7 @@ static bool is_mixed_hw_group(struct evsel *counter)
>  }
>
>  static void printout(struct perf_stat_config *config, struct outstate *os,
> -                    double uval, u64 run, u64 ena, double noise,
> -                    struct runtime_stat *st, int map_idx)
> +                    double uval, u64 run, u64 ena, double noise, int map_idx)
>  {
>         struct perf_stat_output_ctx out;
>         print_metric_t pm;
> @@ -737,7 +736,7 @@ static void printout(struct perf_stat_config *config, struct outstate *os,
>
>         if (ok) {
>                 perf_stat__print_shadow_stats(config, counter, uval, map_idx,
> -                                             &out, &config->metric_events, st);
> +                                             &out, &config->metric_events, &rt_stat);
>         } else {
>                 pm(config, &os, /*color=*/NULL, /*format=*/NULL, /*unit=*/"", /*val=*/0);
>         }
> @@ -834,7 +833,7 @@ static void print_counter_aggrdata(struct perf_stat_config *config,
>
>         uval = val * counter->scale;
>
> -       printout(config, &os, uval, run, ena, avg, &rt_stat, s);
> +       printout(config, &os, uval, run, ena, avg, s);
>
>         if (!metric_only)
>                 fputc('\n', output);
> @@ -987,7 +986,7 @@ static void print_no_aggr_metric(struct perf_stat_config *config,
>                         run = ps->aggr[counter_idx].counts.run;
>
>                         uval = val * counter->scale;
> -                       printout(config, &os, uval, run, ena, 1.0, &rt_stat, counter_idx);
> +                       printout(config, &os, uval, run, ena, 1.0, counter_idx);
>                 }
>                 if (!first)
>                         print_metric_end(config);
> --
> 2.38.1.584.g0f3c55d4c2-goog
>

Re: [PATCH 12/15] perf stat: Pass through struct outstate

[Ian Rogers]

On Wed, Nov 23, 2022 at 10:02 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Now most of the print functions take a pointer to the struct outstate.
> We have one in the evlist__print_counters() and pass it through the
> child functions.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/arch/x86/util/iostat.c |   4 +-
>  tools/perf/util/iostat.c          |   3 +-
>  tools/perf/util/iostat.h          |   4 +-
>  tools/perf/util/stat-display.c    | 102 +++++++++++++-----------------
>  4 files changed, 50 insertions(+), 63 deletions(-)
>
> diff --git a/tools/perf/arch/x86/util/iostat.c b/tools/perf/arch/x86/util/iostat.c
> index 404de795ec0b..7eb0a7b00b95 100644
> --- a/tools/perf/arch/x86/util/iostat.c
> +++ b/tools/perf/arch/x86/util/iostat.c
> @@ -449,7 +449,7 @@ void iostat_print_metric(struct perf_stat_config *config, struct evsel *evsel,
>
>  void iostat_print_counters(struct evlist *evlist,
>                            struct perf_stat_config *config, struct timespec *ts,
> -                          char *prefix, iostat_print_counter_t print_cnt_cb)
> +                          char *prefix, iostat_print_counter_t print_cnt_cb, void *arg)
>  {
>         void *perf_device = NULL;
>         struct evsel *counter = evlist__first(evlist);
> @@ -464,7 +464,7 @@ void iostat_print_counters(struct evlist *evlist,
>                         iostat_prefix(evlist, config, prefix, ts);
>                         fprintf(config->output, "\n%s", prefix);
>                 }
> -               print_cnt_cb(config, counter, prefix);
> +               print_cnt_cb(config, counter, arg);
>         }
>         fputc('\n', config->output);
>  }
> diff --git a/tools/perf/util/iostat.c b/tools/perf/util/iostat.c
> index 57dd49da28fe..b770bd473af7 100644
> --- a/tools/perf/util/iostat.c
> +++ b/tools/perf/util/iostat.c
> @@ -48,6 +48,7 @@ __weak void iostat_print_counters(struct evlist *evlist __maybe_unused,
>                                   struct perf_stat_config *config __maybe_unused,
>                                   struct timespec *ts __maybe_unused,
>                                   char *prefix __maybe_unused,
> -                                 iostat_print_counter_t print_cnt_cb __maybe_unused)
> +                                 iostat_print_counter_t print_cnt_cb __maybe_unused,
> +                                 void *arg __maybe_unused)
>  {
>  }
> diff --git a/tools/perf/util/iostat.h b/tools/perf/util/iostat.h
> index c22688f87cb2..a4e7299c5c2f 100644
> --- a/tools/perf/util/iostat.h
> +++ b/tools/perf/util/iostat.h
> @@ -28,7 +28,7 @@ enum iostat_mode_t {
>
>  extern enum iostat_mode_t iostat_mode;
>
> -typedef void (*iostat_print_counter_t)(struct perf_stat_config *, struct evsel *, const char *);
> +typedef void (*iostat_print_counter_t)(struct perf_stat_config *, struct evsel *, void *);
>
>  int iostat_prepare(struct evlist *evlist, struct perf_stat_config *config);
>  int iostat_parse(const struct option *opt, const char *str,
> @@ -42,6 +42,6 @@ void iostat_print_metric(struct perf_stat_config *config, struct evsel *evsel,
>                          struct perf_stat_output_ctx *out);
>  void iostat_print_counters(struct evlist *evlist,
>                            struct perf_stat_config *config, struct timespec *ts,
> -                          char *prefix, iostat_print_counter_t print_cnt_cb);
> +                          char *prefix, iostat_print_counter_t print_cnt_cb, void *arg);
>
>  #endif /* _IOSTAT_H */
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index cdf4ca7f6e3a..335627e8542d 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -796,7 +796,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,
> -                                  const char *prefix)
> +                                  struct outstate *os)
>  {
>         FILE *output = config->output;
>         u64 ena, run, val;
> @@ -806,13 +806,10 @@ static void print_counter_aggrdata(struct perf_stat_config *config,
>         struct aggr_cpu_id id = config->aggr_map->map[s];
>         double avg = aggr->counts.val;
>         bool metric_only = config->metric_only;
> -       struct outstate os = {
> -               .fh = config->output,
> -               .prefix = prefix ? prefix : "",
> -               .id = id,
> -               .nr = aggr->nr,
> -               .evsel = counter,
> -       };
> +
> +       os->id = id;
> +       os->nr = aggr->nr;
> +       os->evsel = counter;
>
>         if (counter->supported && aggr->nr == 0)
>                 return;
> @@ -824,8 +821,8 @@ static void print_counter_aggrdata(struct perf_stat_config *config,
>         run = aggr->counts.run;
>
>         if (!metric_only) {
> -               if (prefix)
> -                       fprintf(output, "%s", prefix);
> +               if (os->prefix)
> +                       fprintf(output, "%s", os->prefix);
>                 else if (config->summary && config->csv_output &&
>                          !config->no_csv_summary && !config->interval)
>                         fprintf(output, "%s%s", "summary", config->csv_sep);
> @@ -833,7 +830,7 @@ static void print_counter_aggrdata(struct perf_stat_config *config,
>
>         uval = val * counter->scale;
>
> -       printout(config, &os, uval, run, ena, avg, s);
> +       printout(config, os, uval, run, ena, avg, s);
>
>         if (!metric_only)
>                 fputc('\n', output);
> @@ -871,12 +868,9 @@ static void print_metric_end(struct perf_stat_config *config)
>
>  static void print_aggr(struct perf_stat_config *config,
>                        struct evlist *evlist,
> -                      const char *prefix)
> +                      struct outstate *os)
>  {
>         struct evsel *counter;
> -       struct outstate os = {
> -               .prefix = prefix,
> -       };
>         int s;
>
>         if (!config->aggr_map || !config->aggr_get_id)
> @@ -887,13 +881,13 @@ 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, &os, s);
> +               print_metric_begin(config, evlist, os, s);
>
>                 evlist__for_each_entry(evlist, counter) {
>                         if (counter->merged_stat)
>                                 continue;
>
> -                       print_counter_aggrdata(config, counter, s, prefix);
> +                       print_counter_aggrdata(config, counter, s, os);
>                 }
>                 print_metric_end(config);
>         }
> @@ -901,34 +895,31 @@ static void print_aggr(struct perf_stat_config *config,
>
>  static void print_aggr_cgroup(struct perf_stat_config *config,
>                               struct evlist *evlist,
> -                             const char *prefix)
> +                             struct outstate *os)
>  {
>         struct evsel *counter, *evsel;
> -       struct outstate os = {
> -               .prefix = prefix,
> -       };
>         int s;
>
>         if (!config->aggr_map || !config->aggr_get_id)
>                 return;
>
>         evlist__for_each_entry(evlist, evsel) {
> -               if (os.cgrp == evsel->cgrp)
> +               if (os->cgrp == evsel->cgrp)
>                         continue;
>
> -               os.cgrp = evsel->cgrp;
> +               os->cgrp = evsel->cgrp;
>
>                 for (s = 0; s < config->aggr_map->nr; s++) {
> -                       print_metric_begin(config, evlist, &os, s);
> +                       print_metric_begin(config, evlist, os, s);
>
>                         evlist__for_each_entry(evlist, counter) {
>                                 if (counter->merged_stat)
>                                         continue;
>
> -                               if (counter->cgrp != os.cgrp)
> +                               if (counter->cgrp != os->cgrp)
>                                         continue;
>
> -                               print_counter_aggrdata(config, counter, s, prefix);
> +                               print_counter_aggrdata(config, counter, s, os);
>                         }
>                         print_metric_end(config);
>                 }
> @@ -936,7 +927,7 @@ static void print_aggr_cgroup(struct perf_stat_config *config,
>  }
>
>  static void print_counter(struct perf_stat_config *config,
> -                         struct evsel *counter, const char *prefix)
> +                         struct evsel *counter, struct outstate *os)
>  {
>         int s;
>
> @@ -948,19 +939,16 @@ 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);
> +               print_counter_aggrdata(config, counter, s, os);
>         }
>  }
>
>  static void print_no_aggr_metric(struct perf_stat_config *config,
>                                  struct evlist *evlist,
> -                                const char *prefix)
> +                                struct outstate *os)
>  {
>         int all_idx;
>         struct perf_cpu cpu;
> -       struct outstate os = {
> -               .prefix = prefix,
> -       };
>
>         perf_cpu_map__for_each_cpu(cpu, all_idx, evlist->core.user_requested_cpus) {
>                 struct evsel *counter;
> @@ -975,10 +963,10 @@ static void print_no_aggr_metric(struct perf_stat_config *config,
>                         if (counter_idx < 0)
>                                 continue;
>
> -                       os.evsel = counter;
> -                       os.id = aggr_cpu_id__cpu(cpu, /*data=*/NULL);
> +                       os->evsel = counter;
> +                       os->id = aggr_cpu_id__cpu(cpu, /*data=*/NULL);
>                         if (first) {
> -                               print_metric_begin(config, evlist, &os, counter_idx);
> +                               print_metric_begin(config, evlist, os, counter_idx);
>                                 first = false;
>                         }
>                         val = ps->aggr[counter_idx].counts.val;
> @@ -986,7 +974,7 @@ static void print_no_aggr_metric(struct perf_stat_config *config,
>                         run = ps->aggr[counter_idx].counts.run;
>
>                         uval = val * counter->scale;
> -                       printout(config, &os, uval, run, ena, 1.0, counter_idx);
> +                       printout(config, os, uval, run, ena, 1.0, counter_idx);
>                 }
>                 if (!first)
>                         print_metric_end(config);
> @@ -1304,7 +1292,7 @@ static void print_footer(struct perf_stat_config *config)
>  }
>
>  static void print_percore(struct perf_stat_config *config,
> -                         struct evsel *counter, const char *prefix)
> +                         struct evsel *counter, struct outstate *os)
>  {
>         bool metric_only = config->metric_only;
>         FILE *output = config->output;
> @@ -1315,7 +1303,7 @@ static void print_percore(struct perf_stat_config *config,
>                 return;
>
>         if (config->percore_show_thread)
> -               return print_counter(config, counter, prefix);
> +               return print_counter(config, counter, os);
>
>         core_map = cpu_aggr_map__empty_new(config->aggr_map->nr);
>         if (core_map == NULL) {
> @@ -1337,7 +1325,7 @@ static void print_percore(struct perf_stat_config *config,
>                 if (found)
>                         continue;
>
> -               print_counter_aggrdata(config, counter, s, prefix);
> +               print_counter_aggrdata(config, counter, s, os);
>
>                 core_map->map[c++] = core_id;
>         }
> @@ -1348,30 +1336,28 @@ static void print_percore(struct perf_stat_config *config,
>  }
>
>  static void print_cgroup_counter(struct perf_stat_config *config, struct evlist *evlist,
> -                                const char *prefix)
> +                                struct outstate *os)
>  {
>         struct evsel *counter;
> -       struct outstate os = {
> -               .prefix = prefix,
> -       };
>
>         evlist__for_each_entry(evlist, counter) {
> -               if (os.cgrp != counter->cgrp) {
> -                       if (os.cgrp != NULL)
> +               if (os->cgrp != counter->cgrp) {
> +                       if (os->cgrp != NULL)
>                                 print_metric_end(config);
>
> -                       os.cgrp = counter->cgrp;
> -                       print_metric_begin(config, evlist, &os, /*aggr_idx=*/0);
> +                       os->cgrp = counter->cgrp;
> +                       print_metric_begin(config, evlist, os, /*aggr_idx=*/0);
>                 }
>
> -               print_counter(config, counter, prefix);
> +               print_counter(config, counter, os);
>         }
> -       if (os.cgrp)
> +       if (os->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)
> +                           struct target *_target, struct timespec *ts,
> +                           int argc, const char **argv)
>  {
>         bool metric_only = config->metric_only;
>         int interval = config->interval;
> @@ -1397,34 +1383,34 @@ void evlist__print_counters(struct evlist *evlist, struct perf_stat_config *conf
>         case AGGR_SOCKET:
>         case AGGR_NODE:
>                 if (config->cgroup_list)
> -                       print_aggr_cgroup(config, evlist, os.prefix);
> +                       print_aggr_cgroup(config, evlist, &os);
>                 else
> -                       print_aggr(config, evlist, os.prefix);
> +                       print_aggr(config, evlist, &os);
>                 break;
>         case AGGR_THREAD:
>         case AGGR_GLOBAL:
>                 if (config->iostat_run) {
>                         iostat_print_counters(evlist, config, ts, buf,
> -                                             print_counter);
> +                                             (iostat_print_counter_t)print_counter, &os);
>                 } else if (config->cgroup_list) {
> -                       print_cgroup_counter(config, evlist, os.prefix);
> +                       print_cgroup_counter(config, evlist, &os);
>                 } else {
>                         print_metric_begin(config, evlist, &os, /*aggr_idx=*/0);
>                         evlist__for_each_entry(evlist, counter) {
> -                               print_counter(config, counter, os.prefix);
> +                               print_counter(config, counter, &os);
>                         }
>                         print_metric_end(config);
>                 }
>                 break;
>         case AGGR_NONE:
>                 if (metric_only)
> -                       print_no_aggr_metric(config, evlist, os.prefix);
> +                       print_no_aggr_metric(config, evlist, &os);
>                 else {
>                         evlist__for_each_entry(evlist, counter) {
>                                 if (counter->percore)
> -                                       print_percore(config, counter, os.prefix);
> +                                       print_percore(config, counter, &os);
>                                 else
> -                                       print_counter(config, counter, os.prefix);
> +                                       print_counter(config, counter, &os);
>                         }
>                 }
>                 break;
> --
> 2.38.1.584.g0f3c55d4c2-goog
>

Re: [PATCH 13/15] perf stat: Fix JSON output in metric-only mode

[Ian Rogers]

On Wed, Nov 23, 2022 at 10:02 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> It generated a broken JSON output when aggregation mode or cgroup is
> used with --metric-only option.  Also get rid of the header line and
> make the output single line for each entry.
>
> It needs to know whether the current metric is the first one or not.
> So add 'first' field in the outstate and mark it false after printing.
>
> Before:
>   # perf stat -a -j --metric-only true
>   {"unit" : "GHz"}{"unit" : "insn per cycle"}{"unit" : "branch-misses of all branches"}
>   {{"metric-value" : "0.797"}{"metric-value" : "1.65"}{"metric-value" : "0.89"}
>    ^
>
>   # perf stat -a -j --metric-only --per-socket true
>   {"unit" : "GHz"}{"unit" : "insn per cycle"}{"unit" : "branch-misses of all branches"}
>   {"socket" : "S0", "aggregate-number" : 8, {"metric-value" : "0.295"}{"metric-value" : "1.88"}{"metric-value" : "0.64"}
>                                            ^
>
> After:
>   # perf stat -a -j --metric-only true
>   {"GHz" : "0.990", "insn per cycle" : "2.06", "branch-misses of all branches" : "0.59"}
>
>   # perf stat -a -j --metric-only --per-socket true
>   {"socket" : "S0", "aggregate-number" : 8, "GHz" : "0.439", "insn per cycle" : "2.14", "branch-misses of all branches" : "0.51"}
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/util/stat-display.c | 42 +++++++++++++++++++---------------
>  1 file changed, 24 insertions(+), 18 deletions(-)
>
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index 335627e8542d..43640115454c 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -279,9 +279,6 @@ static void print_aggr_id_json(struct perf_stat_config *config,
>  {
>         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, ",
> @@ -335,6 +332,7 @@ static void aggr_printout(struct perf_stat_config *config,
>  struct outstate {
>         FILE *fh;
>         bool newline;
> +       bool first;
>         const char *prefix;
>         int  nfields;
>         int  nr;
> @@ -491,6 +489,7 @@ static void print_metric_only(struct perf_stat_config *config,
>
>         color_snprintf(str, sizeof(str), color ?: "", fmt, val);
>         fprintf(out, "%*s ", mlen, str);
> +       os->first = false;
>  }
>
>  static void print_metric_only_csv(struct perf_stat_config *config __maybe_unused,
> @@ -512,6 +511,7 @@ static void print_metric_only_csv(struct perf_stat_config *config __maybe_unused
>                 ends++;
>         *ends = 0;
>         fprintf(out, "%s%s", vals, config->csv_sep);
> +       os->first = false;
>  }
>
>  static void print_metric_only_json(struct perf_stat_config *config __maybe_unused,
> @@ -532,7 +532,8 @@ static void print_metric_only_json(struct perf_stat_config *config __maybe_unuse
>         while (isdigit(*ends) || *ends == '.')
>                 ends++;
>         *ends = 0;
> -       fprintf(out, "{\"metric-value\" : \"%s\"}", vals);
> +       fprintf(out, "%s\"%s\" : \"%s\"", os->first ? "" : ", ", unit, vals);
> +       os->first = false;
>  }
>
>  static void new_line_metric(struct perf_stat_config *config __maybe_unused,
> @@ -561,7 +562,7 @@ static void print_metric_header(struct perf_stat_config *config,
>         unit = fixunit(tbuf, os->evsel, unit);
>
>         if (config->json_output)
> -               fprintf(os->fh, "{\"unit\" : \"%s\"}", unit);
> +               return;
>         else if (config->csv_output)
>                 fprintf(os->fh, "%s%s", unit, config->csv_sep);
>         else
> @@ -821,6 +822,8 @@ static void print_counter_aggrdata(struct perf_stat_config *config,
>         run = aggr->counts.run;
>
>         if (!metric_only) {
> +               if (config->json_output)
> +                       fputc('{', output);
>                 if (os->prefix)
>                         fprintf(output, "%s", os->prefix);
>                 else if (config->summary && config->csv_output &&
> @@ -844,9 +847,12 @@ static void print_metric_begin(struct perf_stat_config *config,
>         struct aggr_cpu_id id;
>         struct evsel *evsel;
>
> +       os->first = true;
>         if (!config->metric_only)
>                 return;
>
> +       if (config->json_output)
> +               fputc('{', config->output);
>         if (os->prefix)
>                 fprintf(config->output, "%s", os->prefix);
>
> @@ -855,7 +861,7 @@ static void print_metric_begin(struct perf_stat_config *config,
>         aggr = &evsel->stats->aggr[aggr_idx];
>         aggr_printout(config, evsel, id, aggr->nr);
>
> -       print_cgroup(config, os->cgrp);
> +       print_cgroup(config, os->cgrp ? : evsel->cgrp);
>  }
>
>  static void print_metric_end(struct perf_stat_config *config)
> @@ -863,6 +869,8 @@ static void print_metric_end(struct perf_stat_config *config)
>         if (!config->metric_only)
>                 return;
>
> +       if (config->json_output)
> +               fputc('}', config->output);
>         fputc('\n', config->output);
>  }
>
> @@ -1005,11 +1013,9 @@ static void print_metric_headers_csv(struct perf_stat_config *config,
>                 fputs(aggr_header_csv[config->aggr_mode], config->output);
>  }
>
> -static void print_metric_headers_json(struct perf_stat_config *config,
> +static void print_metric_headers_json(struct perf_stat_config *config __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,
> @@ -1049,7 +1055,9 @@ static void print_metric_headers(struct perf_stat_config *config,
>                                               &config->metric_events,
>                                               &rt_stat);
>         }
> -       fputc('\n', config->output);
> +
> +       if (!config->json_output)
> +               fputc('\n', config->output);
>  }
>
>  static void prepare_interval(struct perf_stat_config *config,
> @@ -1058,17 +1066,14 @@ static void prepare_interval(struct perf_stat_config *config,
>         if (config->iostat_run)
>                 return;
>
> -       if (config->csv_output)
> +       if (config->json_output)
> +               scnprintf(prefix, len, "\"interval\" : %lu.%09lu, ",
> +                         (unsigned long) ts->tv_sec, ts->tv_nsec);
> +       else if (config->csv_output)
>                 scnprintf(prefix, len, "%lu.%09lu%s",
>                           (unsigned long) ts->tv_sec, ts->tv_nsec, config->csv_sep);
> -       else if (!config->json_output)
> -               scnprintf(prefix, len, "%6lu.%09lu ",
> -                         (unsigned long) ts->tv_sec, ts->tv_nsec);
> -       else if (!config->metric_only)
> -               scnprintf(prefix, len, "{\"interval\" : %lu.%09lu, ",
> -                         (unsigned long) ts->tv_sec, ts->tv_nsec);
>         else
> -               scnprintf(prefix, len, "{\"interval\" : %lu.%09lu}",
> +               scnprintf(prefix, len, "%6lu.%09lu ",
>                           (unsigned long) ts->tv_sec, ts->tv_nsec);
>  }
>
> @@ -1365,6 +1370,7 @@ void evlist__print_counters(struct evlist *evlist, struct perf_stat_config *conf
>         char buf[64];
>         struct outstate os = {
>                 .fh = config->output,
> +               .first = true,
>         };
>
>         if (config->iostat_run)
> --
> 2.38.1.584.g0f3c55d4c2-goog
>

Re: [PATCH 14/15] perf stat: Rename "aggregate-number" to "cpu-count" in JSON

[Ian Rogers]

On Wed, Nov 23, 2022 at 10:02 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> As the JSON output has been broken for a little while, I guess there are
> not many users.  Let's rename the field to more intuitive one. :)

I'm not sure cpu-count is accurate. For example, an uncore counter in
a dual socket machine may have a CPU mask of "0, 36", ie one event per
socket. The aggregate-number in this case I believe is 2.

Thanks,
Ian

> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/stat-display.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index 43640115454c..7a39a1a7261d 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -281,19 +281,19 @@ static void print_aggr_id_json(struct perf_stat_config *config,
>
>         switch (config->aggr_mode) {
>         case AGGR_CORE:
> -               fprintf(output, "\"core\" : \"S%d-D%d-C%d\", \"aggregate-number\" : %d, ",
> +               fprintf(output, "\"core\" : \"S%d-D%d-C%d\", \"cpu-count\" : %d, ",
>                         id.socket, id.die, id.core, nr);
>                 break;
>         case AGGR_DIE:
> -               fprintf(output, "\"die\" : \"S%d-D%d\", \"aggregate-number\" : %d, ",
> +               fprintf(output, "\"die\" : \"S%d-D%d\", \"cpu-count\" : %d, ",
>                         id.socket, id.die, nr);
>                 break;
>         case AGGR_SOCKET:
> -               fprintf(output, "\"socket\" : \"S%d\", \"aggregate-number\" : %d, ",
> +               fprintf(output, "\"socket\" : \"S%d\", \"cpu-count\" : %d, ",
>                         id.socket, nr);
>                 break;
>         case AGGR_NODE:
> -               fprintf(output, "\"node\" : \"N%d\", \"aggregate-number\" : %d, ",
> +               fprintf(output, "\"node\" : \"N%d\", \"cpu-count\" : %d, ",
>                         id.node, nr);
>                 break;
>         case AGGR_NONE:
> --
> 2.38.1.584.g0f3c55d4c2-goog
>

Re: [PATCH 15/15] perf stat: Tidy up JSON metric-only output when no metrics

[Ian Rogers]

On Wed, Nov 23, 2022 at 10:02 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> It printed empty strings for each metric.  I guess it's needed for CSV
> output to match the column number.  We could just ignore the empty
> metrics in JSON but it ended up with a broken JSON object with a
> trailing comma.
>
> So I added a dummy '"metric-value" : "none"' part.  To do that, it
> needs to pass struct outstate to print_metric_end() to check if any
> metric value is printed or not.
>
> Before:
>   # perf stat -aj --metric-only --per-socket --for-each-cgroup system.slice true
>   {"socket" : "S0", "cpu-count" : 8, "cgroup" : "system.slice", "" : "", "" : "", "" : "", "" : "", "" : "", "" : "", "" : "", "" : ""}
>
> After:
>   # perf stat -aj --metric-only --per-socket --for-each-cgroup system.slice true
>   {"socket" : "S0", "cpu-count" : 8, "cgroup" : "system.slice", "metric-value" : "none"}
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/util/stat-display.c | 27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index 7a39a1a7261d..847acdb5dc40 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -532,6 +532,8 @@ static void print_metric_only_json(struct perf_stat_config *config __maybe_unuse
>         while (isdigit(*ends) || *ends == '.')
>                 ends++;
>         *ends = 0;
> +       if (!unit[0] || !vals[0])
> +               return;
>         fprintf(out, "%s\"%s\" : \"%s\"", os->first ? "" : ", ", unit, vals);
>         os->first = false;
>  }
> @@ -864,14 +866,19 @@ static void print_metric_begin(struct perf_stat_config *config,
>         print_cgroup(config, os->cgrp ? : evsel->cgrp);
>  }
>
> -static void print_metric_end(struct perf_stat_config *config)
> +static void print_metric_end(struct perf_stat_config *config, struct outstate *os)
>  {
> +       FILE *output = config->output;
> +
>         if (!config->metric_only)
>                 return;
>
> -       if (config->json_output)
> -               fputc('}', config->output);
> -       fputc('\n', config->output);
> +       if (config->json_output) {
> +               if (os->first)
> +                       fputs("\"metric-value\" : \"none\"", output);
> +               fputc('}', output);
> +       }
> +       fputc('\n', output);
>  }
>
>  static void print_aggr(struct perf_stat_config *config,
> @@ -897,7 +904,7 @@ static void print_aggr(struct perf_stat_config *config,
>
>                         print_counter_aggrdata(config, counter, s, os);
>                 }
> -               print_metric_end(config);
> +               print_metric_end(config, os);
>         }
>  }
>
> @@ -929,7 +936,7 @@ static void print_aggr_cgroup(struct perf_stat_config *config,
>
>                                 print_counter_aggrdata(config, counter, s, os);
>                         }
> -                       print_metric_end(config);
> +                       print_metric_end(config, os);
>                 }
>         }
>  }
> @@ -985,7 +992,7 @@ static void print_no_aggr_metric(struct perf_stat_config *config,
>                         printout(config, os, uval, run, ena, 1.0, counter_idx);
>                 }
>                 if (!first)
> -                       print_metric_end(config);
> +                       print_metric_end(config, os);
>         }
>  }
>
> @@ -1348,7 +1355,7 @@ static void print_cgroup_counter(struct perf_stat_config *config, struct evlist
>         evlist__for_each_entry(evlist, counter) {
>                 if (os->cgrp != counter->cgrp) {
>                         if (os->cgrp != NULL)
> -                               print_metric_end(config);
> +                               print_metric_end(config, os);
>
>                         os->cgrp = counter->cgrp;
>                         print_metric_begin(config, evlist, os, /*aggr_idx=*/0);
> @@ -1357,7 +1364,7 @@ static void print_cgroup_counter(struct perf_stat_config *config, struct evlist
>                 print_counter(config, counter, os);
>         }
>         if (os->cgrp)
> -               print_metric_end(config);
> +               print_metric_end(config, os);
>  }
>
>  void evlist__print_counters(struct evlist *evlist, struct perf_stat_config *config,
> @@ -1405,7 +1412,7 @@ void evlist__print_counters(struct evlist *evlist, struct perf_stat_config *conf
>                         evlist__for_each_entry(evlist, counter) {
>                                 print_counter(config, counter, &os);
>                         }
> -                       print_metric_end(config);
> +                       print_metric_end(config, &os);
>                 }
>                 break;
>         case AGGR_NONE:
> --
> 2.38.1.584.g0f3c55d4c2-goog
>

Re: [PATCH 01/15] perf stat: Fix cgroup display in JSON output

[Arnaldo Carvalho de Melo]

Em Wed, Nov 23, 2022 at 03:20:12PM -0800, Ian Rogers escreveu:
> On Wed, Nov 23, 2022 at 10:02 AM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > It missed the 'else' keyword after checking json output mode.
> >
> > Fixes: 41cb875242e7 ("perf stat: Split print_cgroup() function")
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> 
> Acked-by: Ian Rogers <irogers@google.com>

Thanks, applied the series.

- Arnaldo

Re: [PATCH 14/15] perf stat: Rename "aggregate-number" to "cpu-count" in JSON

[Namhyung Kim]

Hi Ian,

On Wed, Nov 23, 2022 at 3:31 PM Ian Rogers <irogers@google.com> wrote:
>
> On Wed, Nov 23, 2022 at 10:02 AM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > As the JSON output has been broken for a little while, I guess there are
> > not many users.  Let's rename the field to more intuitive one. :)
>
> I'm not sure cpu-count is accurate. For example, an uncore counter in
> a dual socket machine may have a CPU mask of "0, 36", ie one event per
> socket. The aggregate-number in this case I believe is 2.

You're right.  In case of uncore events, it can be confusing.  But in some
sense it could be thought as cpu count as well since it aggregates the
result from two cpus anyway. :)

Note that the aggregate-number (or cpu-count) is only printed if users
requested one of aggregation options like --per-socket or --per-core.
In your example, then it could print 1 for each socket.

But I think uncore events are different from core events, and hopefully
they have separate instances for different sockets or something already.
That means it doesn't need to use those aggregation options for them.

Also the CSV output uses "cpus" for the same information.  It'd be nice
we could have consistency.

Thanks,
Namhyung

Re: [PATCH 14/15] perf stat: Rename "aggregate-number" to "cpu-count" in JSON

[Ian Rogers]

On Thu, Nov 24, 2022 at 11:51 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Hi Ian,
>
> On Wed, Nov 23, 2022 at 3:31 PM Ian Rogers <irogers@google.com> wrote:
> >
> > On Wed, Nov 23, 2022 at 10:02 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > >
> > > As the JSON output has been broken for a little while, I guess there are
> > > not many users.  Let's rename the field to more intuitive one. :)
> >
> > I'm not sure cpu-count is accurate. For example, an uncore counter in
> > a dual socket machine may have a CPU mask of "0, 36", ie one event per
> > socket. The aggregate-number in this case I believe is 2.
>
> You're right.  In case of uncore events, it can be confusing.  But in some
> sense it could be thought as cpu count as well since it aggregates the
> result from two cpus anyway. :)
>
> Note that the aggregate-number (or cpu-count) is only printed if users
> requested one of aggregation options like --per-socket or --per-core.
> In your example, then it could print 1 for each socket.
>
> But I think uncore events are different from core events, and hopefully
> they have separate instances for different sockets or something already.
> That means it doesn't need to use those aggregation options for them.
>
> Also the CSV output uses "cpus" for the same information.  It'd be nice
> we could have consistency.

So in the original patch from Claire she'd passed the name "number"
through to the json from the stat code. Having an integer called
"number" isn't exactly intention revealing - thank you for your clean
up work! :-) I switched "number" to be "aggregate number" as the
number comes from the "data" aggregated and the code refers to it as
aggregate data. I think aggregate-number is more consistent with the
code, and cpu-count would look strange in the uncore case above where
the number of CPUs (really hyperthreads) is 72. Perhaps we should also
be outputting the aggregation mode with the number. Anyway, I think
for the patch series I'd prefer we skipped this one and kept the rest.

Thanks,
Ian
> Namhyung

Re: [PATCH 14/15] perf stat: Rename "aggregate-number" to "cpu-count" in JSON

[Namhyung Kim]

On Sat, Nov 26, 2022 at 7:14 PM Ian Rogers <irogers@google.com> wrote:
>
> On Thu, Nov 24, 2022 at 11:51 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > Hi Ian,
> >
> > On Wed, Nov 23, 2022 at 3:31 PM Ian Rogers <irogers@google.com> wrote:
> > >
> > > On Wed, Nov 23, 2022 at 10:02 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > > >
> > > > As the JSON output has been broken for a little while, I guess there are
> > > > not many users.  Let's rename the field to more intuitive one. :)
> > >
> > > I'm not sure cpu-count is accurate. For example, an uncore counter in
> > > a dual socket machine may have a CPU mask of "0, 36", ie one event per
> > > socket. The aggregate-number in this case I believe is 2.
> >
> > You're right.  In case of uncore events, it can be confusing.  But in some
> > sense it could be thought as cpu count as well since it aggregates the
> > result from two cpus anyway. :)
> >
> > Note that the aggregate-number (or cpu-count) is only printed if users
> > requested one of aggregation options like --per-socket or --per-core.
> > In your example, then it could print 1 for each socket.
> >
> > But I think uncore events are different from core events, and hopefully
> > they have separate instances for different sockets or something already.
> > That means it doesn't need to use those aggregation options for them.
> >
> > Also the CSV output uses "cpus" for the same information.  It'd be nice
> > we could have consistency.
>
> So in the original patch from Claire she'd passed the name "number"
> through to the json from the stat code. Having an integer called
> "number" isn't exactly intention revealing - thank you for your clean
> up work! :-) I switched "number" to be "aggregate number" as the
> number comes from the "data" aggregated and the code refers to it as
> aggregate data. I think aggregate-number is more consistent with the
> code, and cpu-count would look strange in the uncore case above where
> the number of CPUs (really hyperthreads) is 72. Perhaps we should also
> be outputting the aggregation mode with the number. Anyway, I think
> for the patch series I'd prefer we skipped this one and kept the rest.

Right, I think we need a more general term to include non-cpu events.
But it seems Arnaldo already merged it.

Arnaldo, do you want me to send a revert?

Thanks,
Namhyung

Re: [PATCH 14/15] perf stat: Rename "aggregate-number" to "cpu-count" in JSON

[Ian Rogers]

On Tue, Nov 29, 2022 at 2:46 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Sat, Nov 26, 2022 at 7:14 PM Ian Rogers <irogers@google.com> wrote:
> >
> > On Thu, Nov 24, 2022 at 11:51 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > >
> > > Hi Ian,
> > >
> > > On Wed, Nov 23, 2022 at 3:31 PM Ian Rogers <irogers@google.com> wrote:
> > > >
> > > > On Wed, Nov 23, 2022 at 10:02 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > > > >
> > > > > As the JSON output has been broken for a little while, I guess there are
> > > > > not many users.  Let's rename the field to more intuitive one. :)
> > > >
> > > > I'm not sure cpu-count is accurate. For example, an uncore counter in
> > > > a dual socket machine may have a CPU mask of "0, 36", ie one event per
> > > > socket. The aggregate-number in this case I believe is 2.
> > >
> > > You're right.  In case of uncore events, it can be confusing.  But in some
> > > sense it could be thought as cpu count as well since it aggregates the
> > > result from two cpus anyway. :)
> > >
> > > Note that the aggregate-number (or cpu-count) is only printed if users
> > > requested one of aggregation options like --per-socket or --per-core.
> > > In your example, then it could print 1 for each socket.
> > >
> > > But I think uncore events are different from core events, and hopefully
> > > they have separate instances for different sockets or something already.
> > > That means it doesn't need to use those aggregation options for them.
> > >
> > > Also the CSV output uses "cpus" for the same information.  It'd be nice
> > > we could have consistency.
> >
> > So in the original patch from Claire she'd passed the name "number"
> > through to the json from the stat code. Having an integer called
> > "number" isn't exactly intention revealing - thank you for your clean
> > up work! :-) I switched "number" to be "aggregate number" as the
> > number comes from the "data" aggregated and the code refers to it as
> > aggregate data. I think aggregate-number is more consistent with the
> > code, and cpu-count would look strange in the uncore case above where
> > the number of CPUs (really hyperthreads) is 72. Perhaps we should also
> > be outputting the aggregation mode with the number. Anyway, I think
> > for the patch series I'd prefer we skipped this one and kept the rest.
>
> Right, I think we need a more general term to include non-cpu events.
> But it seems Arnaldo already merged it.
>
> Arnaldo, do you want me to send a revert?
>
> Thanks,
> Namhyung

This is also breaking the json output test:

$ perf test -vv 89
89: perf stat JSON output linter                                    :
--- start ---
test child forked, pid 2116261
Checking json output: no args [Success]
Checking json output: system wide [Success]
Checking json output: interval [Success]
Checking json output: event [Success]
Checking json output: per thread [Success]
Checking json output: per node Test failed for input:
{"node" : "N0", "cpu-count" : 16, "counter-value" : "32.468431",
"unit" : "msec", "event" : "cpu-clo
ck", "event-runtime" : 32498339, "pcnt-running" : 100.00,
"metric-value" : 19.450525, "metric-unit"
: "CPUs utilized"}

{"node" : "N0", "cpu-count" : 16, "counter-value" : "52.000000",
"unit" : "", "event" : "context-swi
tches", "event-runtime" : 32471361, "pcnt-running" : 100.00,
"metric-value" : 1.601556, "metric-unit
" : "K/sec"}

{"node" : "N0", "cpu-count" : 16, "counter-value" : "16.000000",
"unit" : "", "event" : "cpu-migrati
ons", "event-runtime" : 32469950, "pcnt-running" : 100.00,
"metric-value" : 492.786362, "metric-unit
" : "/sec"}

{"node" : "N0", "cpu-count" : 16, "counter-value" : "57.000000",
"unit" : "", "event" : "page-faults
", "event-runtime" : 32474408, "pcnt-running" : 100.00, "metric-value"
: 1.755551, "metric-unit" : "
K/sec"}

{"node" : "N0", "cpu-count" : 16, "counter-value" : "2958499.000000",
"unit" : "", "event" : "cycles
", "event-runtime" : 32411643, "pcnt-running" : 100.00, "metric-value"
: 0.091119, "metric-unit" : "
GHz"}

{"node" : "N0", "cpu-count" : 16, "counter-value" : "3175893.000000",
"unit" : "", "event" : "instru
ctions", "event-runtime" : 32403573, "pcnt-running" : 100.00,
"metric-value" : 1.073481, "metric-uni
t" : "insn per cycle"}

{"node" : "N0", "cpu-count" : 16, "counter-value" : "688120.000000",
"unit" : "", "event" : "branche
s", "event-runtime" : 32391536, "pcnt-running" : 100.00,
"metric-value" : 21.193509, "metric-unit" :
"M/sec"}

{"node" : "N0", "cpu-count" : 16, "counter-value" : "17584.000000",
"unit" : "", "event" : "branch-m
isses", "event-runtime" : 32371972, "pcnt-running" : 100.00,
"metric-value" : 2.555368, "metric-unit
" : "of all branches"}

{"node" : "N0", "cpu-count" : 16, "counter-value" : "14377890.000000",
"unit" : "", "event" : "slots
", "event-runtime" : 32350026, "pcnt-running" : 100.00, "metric-value"
: 442.826757, "metric-unit" :
"M/sec"}

{"node" : "N0", "cpu-count" : 16, "counter-value" : "3380921.000000",
"unit" : "", "event" : "topdow
n-retiring", "event-runtime" : 32350026, "pcnt-running" : 100.00,
"metric-value" : 23.514767, "metri
c-unit" : "Retiring"}

{"node" : "N0", "cpu-count" : 16, "counter-value" : "1444174.000000",
"unit" : "", "event" : "topdow
n-bad-spec", "event-runtime" : 32350026, "pcnt-running" : 100.00,
"metric-value" : 10.044427, "metri
c-unit" : "Bad Speculation"}

{"node" : "N0", "cpu-count" : 16, "counter-value" : "5899393.000000",
"unit" : "", "event" : "topdow
n-fe-bound", "event-runtime" : 32350026, "pcnt-running" : 100.00,
"metric-value" : 41.031084, "metri
c-unit" : "Frontend Bound"}

{"node" : "N0", "cpu-count" : 16, "counter-value" : "3653375.000000",
"unit" : "", "event" : "topdow
n-be-bound", "event-runtime" : 32350026, "pcnt-running" : 100.00,
"metric-value" : 25.409722, "metri
c-unit" : "Backend Bound"}

Traceback (most recent call last):
 File "/home/irogers/kernel.org/./tools/perf/tests/shell/lib/perf_json_output_lint.py",
line 93, in
<module>
   check_json_output(expected_items)
 File "/home/irogers/kernel.org/./tools/perf/tests/shell/lib/perf_json_output_lint.py",
line 78, in
check_json_output
   raise RuntimeError(f'Unexpected key: key={key} value={value}')
RuntimeError: Unexpected key: key=cpu-count value=16
test child finished with -1
---- end ----
perf stat JSON output linter: FAILED!

Thanks,
Ian

Re: [PATCH 05/15] perf stat: Remove prefix argument in print_metric_headers()

[Ian Rogers]

On Wed, Nov 23, 2022 at 3:23 PM Ian Rogers <irogers@google.com> wrote:
>
> On Wed, Nov 23, 2022 at 10:02 AM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > It always passes a whitespace to the function, thus we can just add it to the
> > function body.  Furthermore, it's only used in the normal output mode.
> >
> > Well, actually CSV used it but it doesn't need to since we don't care about the
> > indentation or alignment in the CSV output.
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>
> Acked-by: Ian Rogers <irogers@google.com>
>
> Thanks,
> Ian

I suspect this may be responsible for a metric segv that I'm now
seeing in Arnaldo's tree:

$ gdb --args perf stat -M Backend true
...
Performance counter stats for 'true':

        4,712,355      TOPDOWN.SLOTS                    #     17.3 %
tma_core_bound

Program received signal SIGSEGV, Segmentation fault.
__strlen_evex () at ../sysdeps/x86_64/multiarch/strlen-evex.S:77
77      ../sysdeps/x86_64/multiarch/strlen-evex.S: No such file or directory.
(gdb) bt
#0  __strlen_evex () at ../sysdeps/x86_64/multiarch/strlen-evex.S:77
#1  0x00007ffff74749a5 in __GI__IO_fputs (str=0x0, fp=0x7ffff75f5680
<_IO_2_1_stderr_>)
   at ./libio/iofputs.c:33
#2  0x0000555555779f28 in do_new_line_std (config=0x555555e077c0
<stat_config>, os=0x7fffffffbf10)
   at util/stat-display.c:356
#3  0x000055555577a081 in print_metric_std (config=0x555555e077c0
<stat_config>,
   ctx=0x7fffffffbf10, color=0x0, fmt=0x5555558b77b5 "%8.1f",
   unit=0x7fffffffbb10 "%  tma_memory_bound", val=13.165355724442199)
at util/stat-display.c:380
#4  0x00005555557768b6 in generic_metric (config=0x555555e077c0 <stat_config>,
   metric_expr=0x55555593d5b7 "((CYCLE_ACTIVITY.STALLS_MEM_ANY +
EXE_ACTIVITY.BOUND_ON_STORES) / (C
YCLE_ACTIVITY.STALLS_TOTAL + (EXE_ACTIVITY.1_PORTS_UTIL + tma_retiring
* EXE_ACTIVITY.2_PORTS_UTIL)
+ EXE_ACTIVITY.BOUND_ON_STORES))"..., metric_events=0x555555f334e0,
metric_refs=0x555555ec81d0,
   name=0x555555f32e80 "TOPDOWN.SLOTS", metric_name=0x555555f26c80
"tma_memory_bound",
   metric_unit=0x55555593d5b1 "100%", runtime=0, map_idx=0,
out=0x7fffffffbd90,
   st=0x555555e9e620 <rt_stat>) at util/stat-shadow.c:934
#5  0x0000555555778cac in perf_stat__print_shadow_stats
(config=0x555555e077c0 <stat_config>,
   evsel=0x555555f289d0, avg=4712355, map_idx=0, out=0x7fffffffbd90,
   metric_events=0x555555e078e8 <stat_config+296>, st=0x555555e9e620 <rt_stat>)
   at util/stat-shadow.c:1329
#6  0x000055555577b6a0 in printout (config=0x555555e077c0
<stat_config>, os=0x7fffffffbf10,
   uval=4712355, run=325322, ena=325322, noise=4712355, map_idx=0) at
util/stat-display.c:741
#7  0x000055555577bc74 in print_counter_aggrdata
(config=0x555555e077c0 <stat_config>,
   counter=0x555555f289d0, s=0, os=0x7fffffffbf10) at util/stat-display.c:838
#8  0x000055555577c1d8 in print_counter (config=0x555555e077c0 <stat_config>,
   counter=0x555555f289d0, os=0x7fffffffbf10) at util/stat-display.c:957
#9  0x000055555577dba0 in evlist__print_counters (evlist=0x555555ec3610,
   config=0x555555e077c0 <stat_config>, _target=0x555555e01c80
<target>, ts=0x0, argc=1,
   argv=0x7fffffffe450) at util/stat-display.c:1413
#10 0x00005555555fc821 in print_counters (ts=0x0, argc=1, argv=0x7fffffffe450)
   at builtin-stat.c:1040
#11 0x000055555560091a in cmd_stat (argc=1, argv=0x7fffffffe450) at
builtin-stat.c:2665
#12 0x00005555556b1eea in run_builtin (p=0x555555e11f70
<commands+336>, argc=4,
   argv=0x7fffffffe450) at perf.c:322
#13 0x00005555556b2181 in handle_internal_command (argc=4,
argv=0x7fffffffe450) at perf.c:376
#14 0x00005555556b22d7 in run_argv (argcp=0x7fffffffe27c,
argv=0x7fffffffe270) at perf.c:420
#15 0x00005555556b26ef in main (argc=4, argv=0x7fffffffe450) at perf.c:550
(gdb)

Thanks,
Ian

> > ---
> >  tools/perf/util/stat-display.c | 26 ++++++++++----------------
> >  1 file changed, 10 insertions(+), 16 deletions(-)
> >
> > diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> > index 744b7a40f59a..deed6ccf072f 100644
> > --- a/tools/perf/util/stat-display.c
> > +++ b/tools/perf/util/stat-display.c
> > @@ -996,10 +996,9 @@ static void print_no_aggr_metric(struct perf_stat_config *config,
> >  }
> >
> >  static void print_metric_headers_std(struct perf_stat_config *config,
> > -                                    const char *prefix, bool no_indent)
> > +                                    bool no_indent)
> >  {
> > -       if (prefix)
> > -               fprintf(config->output, "%s", prefix);
> > +       fputc(' ', config->output);
> >
> >         if (!no_indent) {
> >                 int len = aggr_header_lens[config->aggr_mode];
> > @@ -1012,11 +1011,8 @@ static void print_metric_headers_std(struct perf_stat_config *config,
> >  }
> >
> >  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)
> > @@ -1024,7 +1020,6 @@ static void print_metric_headers_csv(struct perf_stat_config *config,
> >  }
> >
> >  static void print_metric_headers_json(struct perf_stat_config *config,
> > -                                     const char *prefix __maybe_unused,
> >                                       bool no_indent __maybe_unused)
> >  {
> >         if (config->interval)
> > @@ -1032,8 +1027,7 @@ static void print_metric_headers_json(struct perf_stat_config *config,
> >  }
> >
> >  static void print_metric_headers(struct perf_stat_config *config,
> > -                                struct evlist *evlist,
> > -                                const char *prefix, bool no_indent)
> > +                                struct evlist *evlist, bool no_indent)
> >  {
> >         struct evsel *counter;
> >         struct outstate os = {
> > @@ -1047,11 +1041,11 @@ static void print_metric_headers(struct perf_stat_config *config,
> >         };
> >
> >         if (config->json_output)
> > -               print_metric_headers_json(config, prefix, no_indent);
> > +               print_metric_headers_json(config, no_indent);
> >         else if (config->csv_output)
> > -               print_metric_headers_csv(config, prefix, no_indent);
> > +               print_metric_headers_csv(config, no_indent);
> >         else
> > -               print_metric_headers_std(config, prefix, no_indent);
> > +               print_metric_headers_std(config, no_indent);
> >
> >         if (config->iostat_run)
> >                 iostat_print_header_prefix(config);
> > @@ -1132,7 +1126,7 @@ static void print_header_interval_std(struct perf_stat_config *config,
> >         }
> >
> >         if (config->metric_only)
> > -               print_metric_headers(config, evlist, " ", true);
> > +               print_metric_headers(config, evlist, true);
> >         else
> >                 fprintf(output, " %*s %*s events\n",
> >                         COUNTS_LEN, "counts", config->unit_width, "unit");
> > @@ -1168,7 +1162,7 @@ static void print_header_std(struct perf_stat_config *config,
> >         fprintf(output, ":\n\n");
> >
> >         if (config->metric_only)
> > -               print_metric_headers(config, evlist, " ", false);
> > +               print_metric_headers(config, evlist, false);
> >  }
> >
> >  static void print_header_csv(struct perf_stat_config *config,
> > @@ -1178,7 +1172,7 @@ static void print_header_csv(struct perf_stat_config *config,
> >                              const char **argv __maybe_unused)
> >  {
> >         if (config->metric_only)
> > -               print_metric_headers(config, evlist, " ", true);
> > +               print_metric_headers(config, evlist, true);
> >  }
> >  static void print_header_json(struct perf_stat_config *config,
> >                               struct target *_target __maybe_unused,
> > @@ -1187,7 +1181,7 @@ static void print_header_json(struct perf_stat_config *config,
> >                               const char **argv __maybe_unused)
> >  {
> >         if (config->metric_only)
> > -               print_metric_headers(config, evlist, " ", true);
> > +               print_metric_headers(config, evlist, true);
> >  }
> >
> >  static void print_header(struct perf_stat_config *config,
> > --
> > 2.38.1.584.g0f3c55d4c2-goog
> >

Re: [PATCH 05/15] perf stat: Remove prefix argument in print_metric_headers()

[Ian Rogers]

On Tue, Nov 29, 2022 at 9:09 PM Ian Rogers <irogers@google.com> wrote:
>
> On Wed, Nov 23, 2022 at 3:23 PM Ian Rogers <irogers@google.com> wrote:
> >
> > On Wed, Nov 23, 2022 at 10:02 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > >
> > > It always passes a whitespace to the function, thus we can just add it to the
> > > function body.  Furthermore, it's only used in the normal output mode.
> > >
> > > Well, actually CSV used it but it doesn't need to since we don't care about the
> > > indentation or alignment in the CSV output.
> > >
> > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> >
> > Acked-by: Ian Rogers <irogers@google.com>
> >
> > Thanks,
> > Ian
>
> I suspect this may be responsible for a metric segv that I'm now
> seeing in Arnaldo's tree:
>
> $ gdb --args perf stat -M Backend true
> ...
> Performance counter stats for 'true':
>
>         4,712,355      TOPDOWN.SLOTS                    #     17.3 %
> tma_core_bound
>
> Program received signal SIGSEGV, Segmentation fault.
> __strlen_evex () at ../sysdeps/x86_64/multiarch/strlen-evex.S:77
> 77      ../sysdeps/x86_64/multiarch/strlen-evex.S: No such file or directory.
> (gdb) bt
> #0  __strlen_evex () at ../sysdeps/x86_64/multiarch/strlen-evex.S:77
> #1  0x00007ffff74749a5 in __GI__IO_fputs (str=0x0, fp=0x7ffff75f5680
> <_IO_2_1_stderr_>)
>    at ./libio/iofputs.c:33
> #2  0x0000555555779f28 in do_new_line_std (config=0x555555e077c0
> <stat_config>, os=0x7fffffffbf10)
>    at util/stat-display.c:356
> #3  0x000055555577a081 in print_metric_std (config=0x555555e077c0
> <stat_config>,
>    ctx=0x7fffffffbf10, color=0x0, fmt=0x5555558b77b5 "%8.1f",
>    unit=0x7fffffffbb10 "%  tma_memory_bound", val=13.165355724442199)
> at util/stat-display.c:380
> #4  0x00005555557768b6 in generic_metric (config=0x555555e077c0 <stat_config>,
>    metric_expr=0x55555593d5b7 "((CYCLE_ACTIVITY.STALLS_MEM_ANY +
> EXE_ACTIVITY.BOUND_ON_STORES) / (C
> YCLE_ACTIVITY.STALLS_TOTAL + (EXE_ACTIVITY.1_PORTS_UTIL + tma_retiring
> * EXE_ACTIVITY.2_PORTS_UTIL)
> + EXE_ACTIVITY.BOUND_ON_STORES))"..., metric_events=0x555555f334e0,
> metric_refs=0x555555ec81d0,
>    name=0x555555f32e80 "TOPDOWN.SLOTS", metric_name=0x555555f26c80
> "tma_memory_bound",
>    metric_unit=0x55555593d5b1 "100%", runtime=0, map_idx=0,
> out=0x7fffffffbd90,
>    st=0x555555e9e620 <rt_stat>) at util/stat-shadow.c:934
> #5  0x0000555555778cac in perf_stat__print_shadow_stats
> (config=0x555555e077c0 <stat_config>,
>    evsel=0x555555f289d0, avg=4712355, map_idx=0, out=0x7fffffffbd90,
>    metric_events=0x555555e078e8 <stat_config+296>, st=0x555555e9e620 <rt_stat>)
>    at util/stat-shadow.c:1329
> #6  0x000055555577b6a0 in printout (config=0x555555e077c0
> <stat_config>, os=0x7fffffffbf10,
>    uval=4712355, run=325322, ena=325322, noise=4712355, map_idx=0) at
> util/stat-display.c:741
> #7  0x000055555577bc74 in print_counter_aggrdata
> (config=0x555555e077c0 <stat_config>,
>    counter=0x555555f289d0, s=0, os=0x7fffffffbf10) at util/stat-display.c:838
> #8  0x000055555577c1d8 in print_counter (config=0x555555e077c0 <stat_config>,
>    counter=0x555555f289d0, os=0x7fffffffbf10) at util/stat-display.c:957
> #9  0x000055555577dba0 in evlist__print_counters (evlist=0x555555ec3610,
>    config=0x555555e077c0 <stat_config>, _target=0x555555e01c80
> <target>, ts=0x0, argc=1,
>    argv=0x7fffffffe450) at util/stat-display.c:1413
> #10 0x00005555555fc821 in print_counters (ts=0x0, argc=1, argv=0x7fffffffe450)
>    at builtin-stat.c:1040
> #11 0x000055555560091a in cmd_stat (argc=1, argv=0x7fffffffe450) at
> builtin-stat.c:2665
> #12 0x00005555556b1eea in run_builtin (p=0x555555e11f70
> <commands+336>, argc=4,
>    argv=0x7fffffffe450) at perf.c:322
> #13 0x00005555556b2181 in handle_internal_command (argc=4,
> argv=0x7fffffffe450) at perf.c:376
> #14 0x00005555556b22d7 in run_argv (argcp=0x7fffffffe27c,
> argv=0x7fffffffe270) at perf.c:420
> #15 0x00005555556b26ef in main (argc=4, argv=0x7fffffffe450) at perf.c:550
> (gdb)
>
> Thanks,
> Ian

More specifically, I think os->prefix needs testing for NULL:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/stat-display.c?h=perf/core#n356
so:
fputs(os->prefix, os->fh);
should be:
if (os->prefix)
  fputs(os->prefix, os->fh);

Thanks,
Ian

> > > ---
> > >  tools/perf/util/stat-display.c | 26 ++++++++++----------------
> > >  1 file changed, 10 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> > > index 744b7a40f59a..deed6ccf072f 100644
> > > --- a/tools/perf/util/stat-display.c
> > > +++ b/tools/perf/util/stat-display.c
> > > @@ -996,10 +996,9 @@ static void print_no_aggr_metric(struct perf_stat_config *config,
> > >  }
> > >
> > >  static void print_metric_headers_std(struct perf_stat_config *config,
> > > -                                    const char *prefix, bool no_indent)
> > > +                                    bool no_indent)
> > >  {
> > > -       if (prefix)
> > > -               fprintf(config->output, "%s", prefix);
> > > +       fputc(' ', config->output);
> > >
> > >         if (!no_indent) {
> > >                 int len = aggr_header_lens[config->aggr_mode];
> > > @@ -1012,11 +1011,8 @@ static void print_metric_headers_std(struct perf_stat_config *config,
> > >  }
> > >
> > >  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)
> > > @@ -1024,7 +1020,6 @@ static void print_metric_headers_csv(struct perf_stat_config *config,
> > >  }
> > >
> > >  static void print_metric_headers_json(struct perf_stat_config *config,
> > > -                                     const char *prefix __maybe_unused,
> > >                                       bool no_indent __maybe_unused)
> > >  {
> > >         if (config->interval)
> > > @@ -1032,8 +1027,7 @@ static void print_metric_headers_json(struct perf_stat_config *config,
> > >  }
> > >
> > >  static void print_metric_headers(struct perf_stat_config *config,
> > > -                                struct evlist *evlist,
> > > -                                const char *prefix, bool no_indent)
> > > +                                struct evlist *evlist, bool no_indent)
> > >  {
> > >         struct evsel *counter;
> > >         struct outstate os = {
> > > @@ -1047,11 +1041,11 @@ static void print_metric_headers(struct perf_stat_config *config,
> > >         };
> > >
> > >         if (config->json_output)
> > > -               print_metric_headers_json(config, prefix, no_indent);
> > > +               print_metric_headers_json(config, no_indent);
> > >         else if (config->csv_output)
> > > -               print_metric_headers_csv(config, prefix, no_indent);
> > > +               print_metric_headers_csv(config, no_indent);
> > >         else
> > > -               print_metric_headers_std(config, prefix, no_indent);
> > > +               print_metric_headers_std(config, no_indent);
> > >
> > >         if (config->iostat_run)
> > >                 iostat_print_header_prefix(config);
> > > @@ -1132,7 +1126,7 @@ static void print_header_interval_std(struct perf_stat_config *config,
> > >         }
> > >
> > >         if (config->metric_only)
> > > -               print_metric_headers(config, evlist, " ", true);
> > > +               print_metric_headers(config, evlist, true);
> > >         else
> > >                 fprintf(output, " %*s %*s events\n",
> > >                         COUNTS_LEN, "counts", config->unit_width, "unit");
> > > @@ -1168,7 +1162,7 @@ static void print_header_std(struct perf_stat_config *config,
> > >         fprintf(output, ":\n\n");
> > >
> > >         if (config->metric_only)
> > > -               print_metric_headers(config, evlist, " ", false);
> > > +               print_metric_headers(config, evlist, false);
> > >  }
> > >
> > >  static void print_header_csv(struct perf_stat_config *config,
> > > @@ -1178,7 +1172,7 @@ static void print_header_csv(struct perf_stat_config *config,
> > >                              const char **argv __maybe_unused)
> > >  {
> > >         if (config->metric_only)
> > > -               print_metric_headers(config, evlist, " ", true);
> > > +               print_metric_headers(config, evlist, true);
> > >  }
> > >  static void print_header_json(struct perf_stat_config *config,
> > >                               struct target *_target __maybe_unused,
> > > @@ -1187,7 +1181,7 @@ static void print_header_json(struct perf_stat_config *config,
> > >                               const char **argv __maybe_unused)
> > >  {
> > >         if (config->metric_only)
> > > -               print_metric_headers(config, evlist, " ", true);
> > > +               print_metric_headers(config, evlist, true);
> > >  }
> > >
> > >  static void print_header(struct perf_stat_config *config,
> > > --
> > > 2.38.1.584.g0f3c55d4c2-goog
> > >

Re: [PATCH 05/15] perf stat: Remove prefix argument in print_metric_headers()

[Namhyung Kim]

On Tue, Nov 29, 2022 at 9:13 PM Ian Rogers <irogers@google.com> wrote:
>
> On Tue, Nov 29, 2022 at 9:09 PM Ian Rogers <irogers@google.com> wrote:
> >
> > On Wed, Nov 23, 2022 at 3:23 PM Ian Rogers <irogers@google.com> wrote:
> > >
> > > On Wed, Nov 23, 2022 at 10:02 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > > >
> > > > It always passes a whitespace to the function, thus we can just add it to the
> > > > function body.  Furthermore, it's only used in the normal output mode.
> > > >
> > > > Well, actually CSV used it but it doesn't need to since we don't care about the
> > > > indentation or alignment in the CSV output.
> > > >
> > > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > >
> > > Acked-by: Ian Rogers <irogers@google.com>
> > >
> > > Thanks,
> > > Ian
> >
> > I suspect this may be responsible for a metric segv that I'm now
> > seeing in Arnaldo's tree:
> >
> > $ gdb --args perf stat -M Backend true
> > ...
> > Performance counter stats for 'true':
> >
> >         4,712,355      TOPDOWN.SLOTS                    #     17.3 %
> > tma_core_bound
> >
> > Program received signal SIGSEGV, Segmentation fault.
> > __strlen_evex () at ../sysdeps/x86_64/multiarch/strlen-evex.S:77
> > 77      ../sysdeps/x86_64/multiarch/strlen-evex.S: No such file or directory.
> > (gdb) bt
> > #0  __strlen_evex () at ../sysdeps/x86_64/multiarch/strlen-evex.S:77
> > #1  0x00007ffff74749a5 in __GI__IO_fputs (str=0x0, fp=0x7ffff75f5680
> > <_IO_2_1_stderr_>)
> >    at ./libio/iofputs.c:33
> > #2  0x0000555555779f28 in do_new_line_std (config=0x555555e077c0
> > <stat_config>, os=0x7fffffffbf10)
> >    at util/stat-display.c:356
> > #3  0x000055555577a081 in print_metric_std (config=0x555555e077c0
> > <stat_config>,
> >    ctx=0x7fffffffbf10, color=0x0, fmt=0x5555558b77b5 "%8.1f",
> >    unit=0x7fffffffbb10 "%  tma_memory_bound", val=13.165355724442199)
> > at util/stat-display.c:380
> > #4  0x00005555557768b6 in generic_metric (config=0x555555e077c0 <stat_config>,
> >    metric_expr=0x55555593d5b7 "((CYCLE_ACTIVITY.STALLS_MEM_ANY +
> > EXE_ACTIVITY.BOUND_ON_STORES) / (C
> > YCLE_ACTIVITY.STALLS_TOTAL + (EXE_ACTIVITY.1_PORTS_UTIL + tma_retiring
> > * EXE_ACTIVITY.2_PORTS_UTIL)
> > + EXE_ACTIVITY.BOUND_ON_STORES))"..., metric_events=0x555555f334e0,
> > metric_refs=0x555555ec81d0,
> >    name=0x555555f32e80 "TOPDOWN.SLOTS", metric_name=0x555555f26c80
> > "tma_memory_bound",
> >    metric_unit=0x55555593d5b1 "100%", runtime=0, map_idx=0,
> > out=0x7fffffffbd90,
> >    st=0x555555e9e620 <rt_stat>) at util/stat-shadow.c:934
> > #5  0x0000555555778cac in perf_stat__print_shadow_stats
> > (config=0x555555e077c0 <stat_config>,
> >    evsel=0x555555f289d0, avg=4712355, map_idx=0, out=0x7fffffffbd90,
> >    metric_events=0x555555e078e8 <stat_config+296>, st=0x555555e9e620 <rt_stat>)
> >    at util/stat-shadow.c:1329
> > #6  0x000055555577b6a0 in printout (config=0x555555e077c0
> > <stat_config>, os=0x7fffffffbf10,
> >    uval=4712355, run=325322, ena=325322, noise=4712355, map_idx=0) at
> > util/stat-display.c:741
> > #7  0x000055555577bc74 in print_counter_aggrdata
> > (config=0x555555e077c0 <stat_config>,
> >    counter=0x555555f289d0, s=0, os=0x7fffffffbf10) at util/stat-display.c:838
> > #8  0x000055555577c1d8 in print_counter (config=0x555555e077c0 <stat_config>,
> >    counter=0x555555f289d0, os=0x7fffffffbf10) at util/stat-display.c:957
> > #9  0x000055555577dba0 in evlist__print_counters (evlist=0x555555ec3610,
> >    config=0x555555e077c0 <stat_config>, _target=0x555555e01c80
> > <target>, ts=0x0, argc=1,
> >    argv=0x7fffffffe450) at util/stat-display.c:1413
> > #10 0x00005555555fc821 in print_counters (ts=0x0, argc=1, argv=0x7fffffffe450)
> >    at builtin-stat.c:1040
> > #11 0x000055555560091a in cmd_stat (argc=1, argv=0x7fffffffe450) at
> > builtin-stat.c:2665
> > #12 0x00005555556b1eea in run_builtin (p=0x555555e11f70
> > <commands+336>, argc=4,
> >    argv=0x7fffffffe450) at perf.c:322
> > #13 0x00005555556b2181 in handle_internal_command (argc=4,
> > argv=0x7fffffffe450) at perf.c:376
> > #14 0x00005555556b22d7 in run_argv (argcp=0x7fffffffe27c,
> > argv=0x7fffffffe270) at perf.c:420
> > #15 0x00005555556b26ef in main (argc=4, argv=0x7fffffffe450) at perf.c:550
> > (gdb)
> >
> > Thanks,
> > Ian
>
> More specifically, I think os->prefix needs testing for NULL:
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/stat-display.c?h=perf/core#n356
> so:
> fputs(os->prefix, os->fh);
> should be:
> if (os->prefix)
>   fputs(os->prefix, os->fh);

Yep, could you please send it as a formal patch?

Thanks,
Namhyung

Re: [PATCH 05/15] perf stat: Remove prefix argument in print_metric_headers()

[Arnaldo Carvalho de Melo]

Em Tue, Nov 29, 2022 at 09:13:11PM -0800, Ian Rogers escreveu:
> On Tue, Nov 29, 2022 at 9:09 PM Ian Rogers <irogers@google.com> wrote:
> >
> > On Wed, Nov 23, 2022 at 3:23 PM Ian Rogers <irogers@google.com> wrote:
> > >
> > > On Wed, Nov 23, 2022 at 10:02 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > > >
> > > > It always passes a whitespace to the function, thus we can just add it to the
> > > > function body.  Furthermore, it's only used in the normal output mode.
> > > >
> > > > Well, actually CSV used it but it doesn't need to since we don't care about the
> > > > indentation or alignment in the CSV output.
> > > >
> > > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > >
> > > Acked-by: Ian Rogers <irogers@google.com>
> > >
> > > Thanks,
> > > Ian
> >
> > I suspect this may be responsible for a metric segv that I'm now
> > seeing in Arnaldo's tree:
> >
> > $ gdb --args perf stat -M Backend true
> > ...
> > Performance counter stats for 'true':
> >
> >         4,712,355      TOPDOWN.SLOTS                    #     17.3 %
> > tma_core_bound
> >
> > Program received signal SIGSEGV, Segmentation fault.
> > __strlen_evex () at ../sysdeps/x86_64/multiarch/strlen-evex.S:77
> > 77      ../sysdeps/x86_64/multiarch/strlen-evex.S: No such file or directory.
> > (gdb) bt
> > #0  __strlen_evex () at ../sysdeps/x86_64/multiarch/strlen-evex.S:77
> > #1  0x00007ffff74749a5 in __GI__IO_fputs (str=0x0, fp=0x7ffff75f5680
> > <_IO_2_1_stderr_>)
> >    at ./libio/iofputs.c:33
> > #2  0x0000555555779f28 in do_new_line_std (config=0x555555e077c0
> > <stat_config>, os=0x7fffffffbf10)
> >    at util/stat-display.c:356
> > #3  0x000055555577a081 in print_metric_std (config=0x555555e077c0
> > <stat_config>,
> >    ctx=0x7fffffffbf10, color=0x0, fmt=0x5555558b77b5 "%8.1f",
> >    unit=0x7fffffffbb10 "%  tma_memory_bound", val=13.165355724442199)
> > at util/stat-display.c:380
> > #4  0x00005555557768b6 in generic_metric (config=0x555555e077c0 <stat_config>,
> >    metric_expr=0x55555593d5b7 "((CYCLE_ACTIVITY.STALLS_MEM_ANY +
> > EXE_ACTIVITY.BOUND_ON_STORES) / (C
> > YCLE_ACTIVITY.STALLS_TOTAL + (EXE_ACTIVITY.1_PORTS_UTIL + tma_retiring
> > * EXE_ACTIVITY.2_PORTS_UTIL)
> > + EXE_ACTIVITY.BOUND_ON_STORES))"..., metric_events=0x555555f334e0,
> > metric_refs=0x555555ec81d0,
> >    name=0x555555f32e80 "TOPDOWN.SLOTS", metric_name=0x555555f26c80
> > "tma_memory_bound",
> >    metric_unit=0x55555593d5b1 "100%", runtime=0, map_idx=0,
> > out=0x7fffffffbd90,
> >    st=0x555555e9e620 <rt_stat>) at util/stat-shadow.c:934
> > #5  0x0000555555778cac in perf_stat__print_shadow_stats
> > (config=0x555555e077c0 <stat_config>,
> >    evsel=0x555555f289d0, avg=4712355, map_idx=0, out=0x7fffffffbd90,
> >    metric_events=0x555555e078e8 <stat_config+296>, st=0x555555e9e620 <rt_stat>)
> >    at util/stat-shadow.c:1329
> > #6  0x000055555577b6a0 in printout (config=0x555555e077c0
> > <stat_config>, os=0x7fffffffbf10,
> >    uval=4712355, run=325322, ena=325322, noise=4712355, map_idx=0) at
> > util/stat-display.c:741
> > #7  0x000055555577bc74 in print_counter_aggrdata
> > (config=0x555555e077c0 <stat_config>,
> >    counter=0x555555f289d0, s=0, os=0x7fffffffbf10) at util/stat-display.c:838
> > #8  0x000055555577c1d8 in print_counter (config=0x555555e077c0 <stat_config>,
> >    counter=0x555555f289d0, os=0x7fffffffbf10) at util/stat-display.c:957
> > #9  0x000055555577dba0 in evlist__print_counters (evlist=0x555555ec3610,
> >    config=0x555555e077c0 <stat_config>, _target=0x555555e01c80
> > <target>, ts=0x0, argc=1,
> >    argv=0x7fffffffe450) at util/stat-display.c:1413
> > #10 0x00005555555fc821 in print_counters (ts=0x0, argc=1, argv=0x7fffffffe450)
> >    at builtin-stat.c:1040
> > #11 0x000055555560091a in cmd_stat (argc=1, argv=0x7fffffffe450) at
> > builtin-stat.c:2665
> > #12 0x00005555556b1eea in run_builtin (p=0x555555e11f70
> > <commands+336>, argc=4,
> >    argv=0x7fffffffe450) at perf.c:322
> > #13 0x00005555556b2181 in handle_internal_command (argc=4,
> > argv=0x7fffffffe450) at perf.c:376
> > #14 0x00005555556b22d7 in run_argv (argcp=0x7fffffffe27c,
> > argv=0x7fffffffe270) at perf.c:420
> > #15 0x00005555556b26ef in main (argc=4, argv=0x7fffffffe450) at perf.c:550
> > (gdb)
> >
> > Thanks,
> > Ian
> 
> More specifically, I think os->prefix needs testing for NULL:
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/stat-display.c?h=perf/core#n356
> so:
> fputs(os->prefix, os->fh);
> should be:
> if (os->prefix)
>   fputs(os->prefix, os->fh);

Going thru the messages, for now I just added the test.

- Arnaldo
 
> Thanks,
> Ian
> 
> > > > ---
> > > >  tools/perf/util/stat-display.c | 26 ++++++++++----------------
> > > >  1 file changed, 10 insertions(+), 16 deletions(-)
> > > >
> > > > diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> > > > index 744b7a40f59a..deed6ccf072f 100644
> > > > --- a/tools/perf/util/stat-display.c
> > > > +++ b/tools/perf/util/stat-display.c
> > > > @@ -996,10 +996,9 @@ static void print_no_aggr_metric(struct perf_stat_config *config,
> > > >  }
> > > >
> > > >  static void print_metric_headers_std(struct perf_stat_config *config,
> > > > -                                    const char *prefix, bool no_indent)
> > > > +                                    bool no_indent)
> > > >  {
> > > > -       if (prefix)
> > > > -               fprintf(config->output, "%s", prefix);
> > > > +       fputc(' ', config->output);
> > > >
> > > >         if (!no_indent) {
> > > >                 int len = aggr_header_lens[config->aggr_mode];
> > > > @@ -1012,11 +1011,8 @@ static void print_metric_headers_std(struct perf_stat_config *config,
> > > >  }
> > > >
> > > >  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)
> > > > @@ -1024,7 +1020,6 @@ static void print_metric_headers_csv(struct perf_stat_config *config,
> > > >  }
> > > >
> > > >  static void print_metric_headers_json(struct perf_stat_config *config,
> > > > -                                     const char *prefix __maybe_unused,
> > > >                                       bool no_indent __maybe_unused)
> > > >  {
> > > >         if (config->interval)
> > > > @@ -1032,8 +1027,7 @@ static void print_metric_headers_json(struct perf_stat_config *config,
> > > >  }
> > > >
> > > >  static void print_metric_headers(struct perf_stat_config *config,
> > > > -                                struct evlist *evlist,
> > > > -                                const char *prefix, bool no_indent)
> > > > +                                struct evlist *evlist, bool no_indent)
> > > >  {
> > > >         struct evsel *counter;
> > > >         struct outstate os = {
> > > > @@ -1047,11 +1041,11 @@ static void print_metric_headers(struct perf_stat_config *config,
> > > >         };
> > > >
> > > >         if (config->json_output)
> > > > -               print_metric_headers_json(config, prefix, no_indent);
> > > > +               print_metric_headers_json(config, no_indent);
> > > >         else if (config->csv_output)
> > > > -               print_metric_headers_csv(config, prefix, no_indent);
> > > > +               print_metric_headers_csv(config, no_indent);
> > > >         else
> > > > -               print_metric_headers_std(config, prefix, no_indent);
> > > > +               print_metric_headers_std(config, no_indent);
> > > >
> > > >         if (config->iostat_run)
> > > >                 iostat_print_header_prefix(config);
> > > > @@ -1132,7 +1126,7 @@ static void print_header_interval_std(struct perf_stat_config *config,
> > > >         }
> > > >
> > > >         if (config->metric_only)
> > > > -               print_metric_headers(config, evlist, " ", true);
> > > > +               print_metric_headers(config, evlist, true);
> > > >         else
> > > >                 fprintf(output, " %*s %*s events\n",
> > > >                         COUNTS_LEN, "counts", config->unit_width, "unit");
> > > > @@ -1168,7 +1162,7 @@ static void print_header_std(struct perf_stat_config *config,
> > > >         fprintf(output, ":\n\n");
> > > >
> > > >         if (config->metric_only)
> > > > -               print_metric_headers(config, evlist, " ", false);
> > > > +               print_metric_headers(config, evlist, false);
> > > >  }
> > > >
> > > >  static void print_header_csv(struct perf_stat_config *config,
> > > > @@ -1178,7 +1172,7 @@ static void print_header_csv(struct perf_stat_config *config,
> > > >                              const char **argv __maybe_unused)
> > > >  {
> > > >         if (config->metric_only)
> > > > -               print_metric_headers(config, evlist, " ", true);
> > > > +               print_metric_headers(config, evlist, true);
> > > >  }
> > > >  static void print_header_json(struct perf_stat_config *config,
> > > >                               struct target *_target __maybe_unused,
> > > > @@ -1187,7 +1181,7 @@ static void print_header_json(struct perf_stat_config *config,
> > > >                               const char **argv __maybe_unused)
> > > >  {
> > > >         if (config->metric_only)
> > > > -               print_metric_headers(config, evlist, " ", true);
> > > > +               print_metric_headers(config, evlist, true);
> > > >  }
> > > >
> > > >  static void print_header(struct perf_stat_config *config,
> > > > --
> > > > 2.38.1.584.g0f3c55d4c2-goog
> > > >

-- 

- Arnaldo

Re: [PATCH 05/15] perf stat: Remove prefix argument in print_metric_headers()

[Arnaldo Carvalho de Melo]

Em Mon, Dec 05, 2022 at 09:22:07AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Nov 29, 2022 at 09:13:11PM -0800, Ian Rogers escreveu:
> > More specifically, I think os->prefix needs testing for NULL:
> > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/stat-display.c?h=perf/core#n356
> > so:
> > fputs(os->prefix, os->fh);
> > should be:
> > if (os->prefix)
> >   fputs(os->prefix, os->fh);
 
> Going thru the messages, for now I just added the test.

So I added this, as the patch introducing the problem was already in
acme/perf/core, please see if this is acceptable.

- Arnaldo

commit e21eb4d4fbd298a94ac200e2b6c3bea431e8cbca
Author: Ian Rogers <irogers@google.com>
Date:   Mon Dec 5 09:34:04 2022 -0300

    perf stat: Check existence of os->prefix, fixing a segfault
    
    We need to check if we have a OS prefix, otherwise we stumble on a
    metric segv that I'm now seeing in Arnaldo's tree:
    
      $ gdb --args perf stat -M Backend true
      ...
      Performance counter stats for 'true':
    
              4,712,355      TOPDOWN.SLOTS                    #     17.3 % tma_core_bound
    
      Program received signal SIGSEGV, Segmentation fault.
      __strlen_evex () at ../sysdeps/x86_64/multiarch/strlen-evex.S:77
      77      ../sysdeps/x86_64/multiarch/strlen-evex.S: No such file or directory.
      (gdb) bt
      #0  __strlen_evex () at ../sysdeps/x86_64/multiarch/strlen-evex.S:77
      #1  0x00007ffff74749a5 in __GI__IO_fputs (str=0x0, fp=0x7ffff75f5680 <_IO_2_1_stderr_>)
      #2  0x0000555555779f28 in do_new_line_std (config=0x555555e077c0 <stat_config>, os=0x7fffffffbf10) at util/stat-display.c:356
      #3  0x000055555577a081 in print_metric_std (config=0x555555e077c0 <stat_config>, ctx=0x7fffffffbf10, color=0x0, fmt=0x5555558b77b5 "%8.1f", unit=0x7fffffffbb10 "%  tma_memory_bound", val=13.165355724442199) at util/stat-display.c:380
      #4  0x00005555557768b6 in generic_metric (config=0x555555e077c0 <stat_config>, metric_expr=0x55555593d5b7 "((CYCLE_ACTIVITY.STALLS_MEM_ANY + EXE_ACTIVITY.BOUND_ON_STORES) / (CYCLE_ACTIVITY.STALLS_TOTAL + (EXE_ACTIVITY.1_PORTS_UTIL + tma_retiring * EXE_ACTIVITY.2_PORTS_UTIL) + EXE_ACTIVITY.BOUND_ON_STORES))"..., metric_events=0x555555f334e0, metric_refs=0x555555ec81d0, name=0x555555f32e80 "TOPDOWN.SLOTS", metric_name=0x555555f26c80 "tma_memory_bound", metric_unit=0x55555593d5b1 "100%", runtime=0, map_idx=0, out=0x7fffffffbd90, st=0x555555e9e620 <rt_stat>) at util/stat-shadow.c:934
      #5  0x0000555555778cac in perf_stat__print_shadow_stats (config=0x555555e077c0 <stat_config>, evsel=0x555555f289d0, avg=4712355, map_idx=0, out=0x7fffffffbd90, metric_events=0x555555e078e8 <stat_config+296>, st=0x555555e9e620 <rt_stat>) at util/stat-shadow.c:1329
      #6  0x000055555577b6a0 in printout (config=0x555555e077c0 <stat_config>, os=0x7fffffffbf10, uval=4712355, run=325322, ena=325322, noise=4712355, map_idx=0) at util/stat-display.c:741
      #7  0x000055555577bc74 in print_counter_aggrdata (config=0x555555e077c0 <stat_config>, counter=0x555555f289d0, s=0, os=0x7fffffffbf10) at util/stat-display.c:838
      #8  0x000055555577c1d8 in print_counter (config=0x555555e077c0 <stat_config>, counter=0x555555f289d0, os=0x7fffffffbf10) at util/stat-display.c:957
      #9  0x000055555577dba0 in evlist__print_counters (evlist=0x555555ec3610, config=0x555555e077c0 <stat_config>, _target=0x555555e01c80 <target>, ts=0x0, argc=1, argv=0x7fffffffe450) at util/stat-display.c:1413
      #10 0x00005555555fc821 in print_counters (ts=0x0, argc=1, argv=0x7fffffffe450) at builtin-stat.c:1040
      #11 0x000055555560091a in cmd_stat (argc=1, argv=0x7fffffffe450) at builtin-stat.c:2665
      #12 0x00005555556b1eea in run_builtin (p=0x555555e11f70 <commands+336>, argc=4, argv=0x7fffffffe450) at perf.c:322
      #13 0x00005555556b2181 in handle_internal_command (argc=4, argv=0x7fffffffe450) at perf.c:376
      #14 0x00005555556b22d7 in run_argv (argcp=0x7fffffffe27c, argv=0x7fffffffe270) at perf.c:420
      #15 0x00005555556b26ef in main (argc=4, argv=0x7fffffffe450) at perf.c:550
      (gdb)
    
    Fixes: f123b2d84ecec9a3 ("perf stat: Remove prefix argument in print_metric_headers()")
    Acked-by: Namhyung Kim <namhyung@kernel.org>
    Cc: Adrian Hunter <adrian.hunter@intel.com>
    Cc: Athira Jajeev <atrajeev@linux.vnet.ibm.com>
    Cc: Ingo Molnar <mingo@kernel.org>
    Cc: James Clark <james.clark@arm.com>
    Cc: Jiri Olsa <jolsa@kernel.org>
    Cc: Kan Liang <kan.liang@linux.intel.com>
    Cc: Namhyung Kim <namhyung@kernel.org>
    Cc: Peter Zijlstra <peterz@infradead.org>
    Cc: Xing Zhengjun <zhengjun.xing@linux.intel.com>
    Link: http://lore.kernel.org/lkml/CAP-5=fUOjSM5HajU9TCD6prY39LbX4OQbkEbtKPPGRBPBN=_VQ@mail.gmail.com
    Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index f1ee4b052198690f..9b7772e6abf6538f 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -353,7 +353,8 @@ static void do_new_line_std(struct perf_stat_config *config,
 			    struct outstate *os)
 {
 	fputc('\n', os->fh);
-	fputs(os->prefix, os->fh);
+	if (os->prefix)
+		fputs(os->prefix, os->fh);
 	aggr_printout(config, os->evsel, os->id, os->nr);
 	if (config->aggr_mode == AGGR_NONE)
 		fprintf(os->fh, "        ");

Re: [PATCH 13/15] perf stat: Fix JSON output in metric-only mode

[Namhyung Kim]

Hello,

On Thu, Dec 1, 2022 at 4:56 PM Wang, Weilin <weilin.wang@intel.com> wrote:
>
> Hi Namhyung,
>
>
>
> I encountered an issue with the metric json output, which I think might be related to this commit.
>
>
>
> This is one of the commands I tried:
>
>
>
> ./perf stat -M memory_bandwidth_total,memory_bandwidth_read,memory_bandwidth_write -a -j -- sleep 1
>
>
>
> Output before this commit:
>
> ====================================================
>
> {"counter-value" : "1117309.000000", "unit" : "", "event" : "UNC_M_CAS_COUNT.RD", "event-runtime" : 8017286783, "pcnt-running" : 100.00, "metric-value" : 124.961911, "metric-unit" : "MB/s  memory_bandwidth_read"} {"metric-value" : 237.237504, "metric-unit" : "MB/s  memory_bandwidth_total"} {"counter-value" : "1004052.000000", "unit" : "", "event" : "UNC_M_CAS_COUNT.WR", "event-runtime" : 8017286783, "pcnt-running" : 100.00, "metric-value" : 112.275593, "metric-unit" : "MB/s  memory_bandwidth_write"} {"counter-value" : "1002140370.000000", "unit" : "ns", "event" : "duration_time", "event-runtime" : 1002140370, "pcnt-running" : 100.00, "metric-value" : 0.000000, "metric-unit" : "(null)"}
>
> ====================================================
>
>
>
> Output after this commit:
>
> ====================================================
>
> {"counter-value" : "1374098.000000", "unit" : "", "event" : "UNC_M_CAS_COUNT.RD", "event-runtime" : 8016491003, "pcnt-running" : 100.00, "metric-value" : 153.660738, "metric-unit" : "MB/s  memory_bandwidth_read"} "metric-value" : 300.338968, "metric-unit" : "MB/s  memory_bandwidth_total"} {"counter-value" : "1311710.000000", "unit" : "", "event" : "UNC_M_CAS_COUNT.WR", "event-runtime" : 8016491003, "pcnt-running" : 100.00, "metric-value" : 146.678229, "metric-unit" : "MB/s  memory_bandwidth_write"} {"counter-value" : "1002037512.000000", "unit" : "ns", "event" : "duration_time", "event-runtime" : 1002037512, "pcnt-running" : 100.00, "metric-value" : 0.000000, "metric-unit" : "(null)"}
>
> ====================================================
>
>
>
> The second set of output is missing a "{" on the second line. Looks like it happens when I collect two related metrics together. Another example is:
>
>
>
> ./perf stat -M memory_bandwidth_total,memory_bandwidth_read,memory_bandwidth_write,L3_Cache_Fill_BW_1T,L3_Cache_Fill_BW -a -j -- sleep 1
>
>
>
> {"counter-value" : "944204.000000", "unit" : "", "event" : "UNC_M_CAS_COUNT.RD", "event-runtime" : 8033937053, "pcnt-running" : 100.00, "metric-value" : 105.369994, "metric-unit" : "MB/s  memory_bandwidth_read"} "metric-value" : 210.592807, "metric-unit" : "MB/s  memory_bandwidth_total"} {"counter-value" : "942679.000000", "unit" : "", "event" : "UNC_M_CAS_COUNT.WR", "event-runtime" : 8033937053, "pcnt-running" : 100.00, "metric-value" : 105.222813, "metric-unit" : "MB/s  memory_bandwidth_write"} {"counter-value" : "1004471458.000000", "unit" : "ns", "event" : "duration_time", "event-runtime" : 1004471458, "pcnt-running" : 100.00, "metric-value" : 0.000000, "metric-unit" : "(null)"} {"counter-value" : "184675.000000", "unit" : "", "event" : "LONGEST_LAT_CACHE.MISS", "event-runtime" : 88387319917, "pcnt-running" : 100.00, "metric-value" : 0.011767, "metric-unit" : "L3_Cache_Fill_BW_1T"} "metric-value" : 0.011767, "metric-unit" : "L3_Cache_Fill_BW"} {"counter-value" : "1004471458.000000", "unit" : "ns", "event" : "duration_time", "event-runtime" : 1004471458, "pcnt-running" : 100.00, "metric-value" : 0.000000, "metric-unit" : "(null)"}

Thanks for your report.  It seems it misses the open bracket when the metric
produces more than one values.  I think new_line_json() should have it.
Will send the fix soon.

Thanks,
Namhyung