perf stat: fix csv mode column output for non-cgroup events

When using the -x option, perf stat prints csv-style output with one event per line. For each event, it prints the count, the unit, the event name, the cgroup, and a bunch of other event specific fields (such as insn per cycles).

When you use csv-style mode, you expect a normalized output where each event is printed with the same number of fields regardless of what it is so it can easily be imported into a spreadsheet or parsed. For instance, if an event does not have a unit, then print an empty field for it. Although this approach was implemented for the unit, it was not for the cgroup. When mixing cgroup and non-cgroup events, then non-cgroup events would not show an empty field, instead the next field was printed, make columns not line up correctly.

This patch fixes the cgroup output issues by forcing an empty field for non-cgroup events as soon as one event has cgroup.

Before:
<not counted> @ @cycles @foo @ 0 @100.00@@
2531614 @ @cycles @[email protected]@ @

foo cgroup lines up with time_running!

After:
<not counted> @ @cycles @foo @0 @100.00@@
2594834 @ @cycles @ @5287372 @100.00@@

Fields line up.

Signed-off-by Stephane Eranian <[email protected]>

---
 tools/perf/util/stat-display.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

tools/perf/util/stat-display.c +11 -6

index e7b4c44ebb62..665ee374fc01 100644
@@ -59,6 +59,15 @@ static void print_noise(struct perf_stat_config *config,
        print_noise_pct(config, stddev_stats(&ps->res_stats[0]), avg);
 }
 
+static void print_cgroup(struct perf_stat_config *config, struct perf_evsel *evsel)
+{
+       if (nr_cgroups) {
+               const char *cgrp_name = evsel->cgrp ? evsel->cgrp->name  : "";
+               fprintf(config->output, "%s%s", config->csv_sep, cgrp_name);
+       }
+}
+
+
 static void aggr_printout(struct perf_stat_config *config,
                          struct perf_evsel *evsel, int id, int nr)
 {

@@ -336,8 +345,7 @@ static void abs_printout(struct perf_stat_config *config,
 
        fprintf(output, "%-*s", config->csv_output ? 0 : 25, perf_evsel__name(evsel));
 
-       if (evsel->cgrp)
-               fprintf(output, "%s%s", config->csv_sep, evsel->cgrp->name);
+       print_cgroup(config, evsel);
 }
 
 static bool is_mixed_hw_group(struct perf_evsel *counter)

@@ -431,9 +439,7 @@ static void printout(struct perf_stat_config *config, int id, int nr,
                        config->csv_output ? 0 : -25,
                        perf_evsel__name(counter));
 
-               if (counter->cgrp)
-                       fprintf(config->output, "%s%s",
-                               config->csv_sep, counter->cgrp->name);
+               print_cgroup(config, counter);
 
                if (!config->csv_output)
                        pm(config, &os, NULL, NULL, "", 0);
--

2.7.4

When using the -x option, perf stat prints csv-style output with one event per line. For each event, it prints the count, the unit, the event name, the cgroup, and a bunch of other event specific fields (such as insn per cycles).

When you use csv-style mode, you expect a normalized output where each event is printed with the same number of fields regardless of what it is so it can easily be imported into a spreadsheet or parsed. For instance, if an event does not have a unit, then print an empty field for it. Although this approach was implemented for the unit, it was not for the cgroup. When mixing cgroup and non-cgroup events, then non-cgroup events would not show an empty field, instead the next field was printed, make columns not line up correctly.

This patch fixes the cgroup output issues by forcing an empty field for non-cgroup events as soon as one event has cgroup.

Before:
<not counted> @ @cycles @foo @ 0 @100.00@@
2531614 @ @cycles @[email protected]@ @

foo cgroup lines up with time_running!

After:
<not counted> @ @cycles @foo @0 @100.00@@
2594834 @ @cycles @ @5287372 @100.00@@

Fields line up.

Signed-off-by Stephane Eranian <[email protected]>

---
 tools/perf/util/stat-display.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

tools/perf/util/stat-display.c +11 -5

index e7b4c44ebb62..665ee374fc01 100644
@@ -59,6 +59,15 @@ static void print_noise(struct perf_stat_config *config,
 	print_noise_pct(config, stddev_stats(&ps->res_stats[0]), avg);
 }
 
+static void print_cgroup(struct perf_stat_config *config, struct perf_evsel *evsel)
+{
+	if (nr_cgroups) {
+		const char *cgrp_name = evsel->cgrp ? evsel->cgrp->name  : "";
+		fprintf(config->output, "%s%s", config->csv_sep, cgrp_name);
+	}
+}
+
+
 static void aggr_printout(struct perf_stat_config *config,
 			  struct perf_evsel *evsel, int id, int nr)
 {

@@ -336,8 +345,7 @@ static void abs_printout(struct perf_stat_config *config,
 
 	fprintf(output, "%-*s", config->csv_output ? 0 : 25, perf_evsel__name(evsel));
 
-	if (evsel->cgrp)
-		fprintf(output, "%s%s", config->csv_sep, evsel->cgrp->name);
+	print_cgroup(config, evsel);
 }
 
 static bool is_mixed_hw_group(struct perf_evsel *counter)

@@ -431,9 +439,7 @@ static void printout(struct perf_stat_config *config, int id, int nr,
 			config->csv_output ? 0 : -25,
 			perf_evsel__name(counter));
 
-		if (counter->cgrp)
-			fprintf(config->output, "%s%s",
-				config->csv_sep, counter->cgrp->name);
+		print_cgroup(config, counter);
 
 		if (!config->csv_output)
 			pm(config, &os, NULL, NULL, "", 0);

Signature · 1 line

2.7.4

Em Wed, Nov 07, 2018 at 02:50:45AM -0800, Stephane Eranian escreveu:

Quoted 14 lines

When using the -x option, perf stat prints csv-style output with one event per line. For each event, it prints the count, the unit, the event name, the cgroup, and a bunch of other event specific fields (such as insn per cycles).

When you use csv-style mode, you expect a normalized output where each event is printed with the same number of fields regardless of what it is so it can easily be imported into a spreadsheet or parsed. For instance, if an event does not have a unit, then print an empty field for it. Although this approach was implemented for the unit, it was not for the cgroup. When mixing cgroup and non-cgroup events, then non-cgroup events would not show an empty field, instead the next field was printed, make columns not line up correctly.

Quoted 2 lines

This patch fixes the cgroup output issues by forcing an empty field for non-cgroup events as soon as one event has cgroup.

Looks sane, that nr_cgroups global variable at some point has to go to the evlist, but that is not introduced by this patch, Jiri, are you ok with it as well?

- Arnaldo

Quoted 60 lines

Before:
<not counted> @ @cycles @foo @ 0 @100.00@@
2531614 @ @cycles @[email protected]@ @

foo cgroup lines up with time_running!

After:
<not counted> @ @cycles @foo @0 @100.00@@
2594834 @ @cycles @ @5287372 @100.00@@

Fields line up.

Signed-off-by Stephane Eranian <[email protected]>

---
 tools/perf/util/stat-display.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

tools/perf/util/stat-display.c +11 -5

index e7b4c44ebb62..665ee374fc01 100644
@@ -59,6 +59,15 @@ static void print_noise(struct perf_stat_config *config,
 	print_noise_pct(config, stddev_stats(&ps->res_stats[0]), avg);
 }
 
+static void print_cgroup(struct perf_stat_config *config, struct perf_evsel *evsel)
+{
+	if (nr_cgroups) {
+		const char *cgrp_name = evsel->cgrp ? evsel->cgrp->name  : "";
+		fprintf(config->output, "%s%s", config->csv_sep, cgrp_name);
+	}
+}
+
+
 static void aggr_printout(struct perf_stat_config *config,
 			  struct perf_evsel *evsel, int id, int nr)
 {

@@ -336,8 +345,7 @@ static void abs_printout(struct perf_stat_config *config,
 
 	fprintf(output, "%-*s", config->csv_output ? 0 : 25, perf_evsel__name(evsel));
 
-	if (evsel->cgrp)
-		fprintf(output, "%s%s", config->csv_sep, evsel->cgrp->name);
+	print_cgroup(config, evsel);
 }
 
 static bool is_mixed_hw_group(struct perf_evsel *counter)

@@ -431,9 +439,7 @@ static void printout(struct perf_stat_config *config, int id, int nr,
 			config->csv_output ? 0 : -25,
 			perf_evsel__name(counter));
 
-		if (counter->cgrp)
-			fprintf(config->output, "%s%s",
-				config->csv_sep, counter->cgrp->name);
+		print_cgroup(config, counter);
 
 		if (!config->csv_output)
 			pm(config, &os, NULL, NULL, "", 0);

Signature · 1 line

2.7.4

Quoted 18 lines

Em Wed, Nov 07, 2018 at 02:50:45AM -0800, Stephane Eranian escreveu:

Quoted 16 lines

When using the -x option, perf stat prints csv-style output with one event per line. For each event, it prints the count, the unit, the event name, the cgroup, and a bunch of other event specific fields (such as insn per cycles).

When you use csv-style mode, you expect a normalized output where each event is printed with the same number of fields regardless of what it is so it can easily be imported into a spreadsheet or parsed. For instance, if an event does not have a unit, then print an empty field for it. Although this approach was implemented for the unit, it was not for the cgroup. When mixing cgroup and non-cgroup events, then non-cgroup events would not show an empty field, instead the next field was printed, make columns not line up correctly.

This patch fixes the cgroup output issues by forcing an empty field for non-cgroup events as soon as one event has cgroup.

Quoted 3 lines

Looks sane, that nr_cgroups global variable at some point has to go to the evlist, but that is not introduced by this patch, Jiri, are you ok with it as well?

yep, looks good, sry I missed it

Acked-by Jiri Olsa <[email protected]>