Hello,
I'm working on cleaning up the perf stat code and found a number of issues.
Before moving on to the real changes, I'd like to fix those first. I'll
think about how to test this properly.
changes in v2)
* move applied patches to the beginning (Arnaldo)
* make it error to use --interval-clear with other output (Ian)
* add two more patches for CSV output
* add Acked-by from Ian
You can get it from 'perf/stat-cleanup-v2' branch in
git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
Thanks,
Namhyung
Namhyung Kim (11):
perf stat: Fix crash with --per-node --metric-only in CSV mode
perf stat: Move common code in print_metric_headers()
perf stat: Fix --metric-only --json output
perf stat: Increase metric length to align outputs
perf stat: Clear screen only if output file is a tty
perf stat: Do not indent headers for JSON
perf stat: Add header for interval in JSON output
perf stat: Fix condition in print_interval()
perf stat: Consolidate condition to print metrics
perf stat: Fix summary output in CSV with --metric-only
perf stat: Add missing separator in the CSV header
tools/perf/builtin-stat.c | 8 ++++
tools/perf/util/stat-display.c | 68 ++++++++++++++++------------------
2 files changed, 40 insertions(+), 36 deletions(-)
base-commit: 816815b852216f3aa3a43e2ce91c5510927cd61b
--
2.38.1.493.g58b659f92b-goog
It should have a comma after 'cpus' for socket and die aggregation mode.
The output of the following command shows the issue.
$ sudo perf stat -a --per-socket -x, --metric-only -I1 true
Before:
+--- here
V
time,socket,cpusGhz,insn per cycle,branch-misses of all branches,
0.000908461,S0,8,0.950,1.65,1.21,
After:
time,socket,cpus,GHz,insn per cycle,branch-misses of all branches,
0.000683094,S0,8,0.593,2.00,0.60,
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/perf/util/stat-display.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index 96bb7a42fd41..a316807255cd 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -828,8 +828,8 @@ static int aggr_header_lens[] = {
static const char *aggr_header_csv[] = {
[AGGR_CORE] = "core,cpus,",
- [AGGR_DIE] = "die,cpus",
- [AGGR_SOCKET] = "socket,cpus",
+ [AGGR_DIE] = "die,cpus,",
+ [AGGR_SOCKET] = "socket,cpus,",
[AGGR_NONE] = "cpu,",
[AGGR_THREAD] = "comm-pid,",
[AGGR_NODE] = "node,",
--
2.38.1.493.g58b659f92b-goog
The following command will get segfault due to missing aggr_header_csv
for AGGR_NODE:
$ sudo perf stat -a --per-node -x, --metric-only true
Fixes: 86895b480a2f ("perf stat: Add --per-node agregation support")
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/perf/util/stat-display.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index 657434cd29ee..ea41e6308c50 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -534,7 +534,7 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
[AGGR_CORE] = 2,
[AGGR_THREAD] = 1,
[AGGR_UNSET] = 0,
- [AGGR_NODE] = 0,
+ [AGGR_NODE] = 1,
};
pm = config->metric_only ? print_metric_only_csv : print_metric_csv;
@@ -819,6 +819,7 @@ static int aggr_header_lens[] = {
[AGGR_SOCKET] = 12,
[AGGR_NONE] = 6,
[AGGR_THREAD] = 24,
+ [AGGR_NODE] = 6,
[AGGR_GLOBAL] = 0,
};
@@ -828,6 +829,7 @@ static const char *aggr_header_csv[] = {
[AGGR_SOCKET] = "socket,cpus",
[AGGR_NONE] = "cpu,",
[AGGR_THREAD] = "comm-pid,",
+ [AGGR_NODE] = "node,",
[AGGR_GLOBAL] = ""
};
--
2.38.1.493.g58b659f92b-goog
It should not print "summary" for each event when --metric-only is set.
Before:
$ sudo perf stat -a --per-socket --summary -x, --metric-only true
time,socket,cpusGhz,insn per cycle,branch-misses of all branches,
0.000709079,S0,8,0.893,2.40,0.45,
S0,8, summary, summary, summary, summary, summary,0.893, summary,2.40, summary, summary,0.45,
After:
$ sudo perf stat -a --per-socket --summary -x, --metric-only true
time,socket,cpusGHz,insn per cycle,branch-misses of all branches,
0.000882297,S0,8,0.598,1.64,0.64,
summary,S0,8,0.598,1.64,0.64,
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/perf/util/stat-display.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index ce81798b5864..96bb7a42fd41 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -549,7 +549,7 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
}
if (!config->no_csv_summary && config->csv_output &&
- config->summary && !config->interval) {
+ config->summary && !config->interval && !config->metric_only) {
fprintf(config->output, "%16s%s", "summary", config->csv_sep);
}
@@ -732,8 +732,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++) {
- if (prefix && metric_only)
- fprintf(output, "%s", prefix);
+ if (metric_only) {
+ if (prefix)
+ fprintf(output, "%s", prefix);
+ else if (config->summary && !config->no_csv_summary &&
+ config->csv_output && !config->interval)
+ fprintf(output, "%16s%s", "summary", config->csv_sep);
+ }
first = true;
evlist__for_each_entry(evlist, counter) {
--
2.38.1.493.g58b659f92b-goog
Currently it prints all metric headers for JSON output. But actually it
skips some metrics with valid_only_metric(). So the output looks like:
$ perf stat --metric-only --json true
{"unit" : "CPUs utilized", "unit" : "/sec", "unit" : "/sec", "unit" : "/sec", "unit" : "GHz", "unit" : "insn per cycle", "unit" : "/sec", "unit" : "branch-misses of all branches"}
{"metric-value" : "3.861"}{"metric-value" : "0.79"}{"metric-value" : "3.04"}
As you can see there are 8 units in the header but only 3 metric-values
are there. It should skip the unused headers as well. Also each unit
should be printed as a separate object like metric values.
With this patch:
$ perf stat --metric-only --json true
{"unit" : "GHz"}{"unit" : "insn per cycle"}{"unit" : "branch-misses of all branches"}
{"metric-value" : "4.166"}{"metric-value" : "0.73"}{"metric-value" : "2.96"}
Fixes: df936cadfb58 ("perf stat: Add JSON output option")
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/perf/util/stat-display.c | 22 +++-------------------
1 file changed, 3 insertions(+), 19 deletions(-)
diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index c7b3a1e10263..96ad0c71adc2 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -430,12 +430,12 @@ static void print_metric_header(struct perf_stat_config *config,
os->evsel->priv != os->evsel->evlist->selected->priv)
return;
- if (!valid_only_metric(unit) && !config->json_output)
+ if (!valid_only_metric(unit))
return;
unit = fixunit(tbuf, os->evsel, unit);
if (config->json_output)
- fprintf(os->fh, "\"unit\" : \"%s\"", unit);
+ fprintf(os->fh, "{\"unit\" : \"%s\"}", unit);
else if (config->csv_output)
fprintf(os->fh, "%s%s", unit, config->csv_sep);
else
@@ -847,10 +847,6 @@ static void print_metric_headers(struct perf_stat_config *config,
.new_line = new_line_metric,
.force_header = true,
};
- bool first = true;
-
- if (config->json_output && !config->interval)
- fprintf(config->output, "{");
if (prefix && !config->json_output)
fprintf(config->output, "%s", prefix);
@@ -871,18 +867,12 @@ static void print_metric_headers(struct perf_stat_config *config,
evlist__for_each_entry(evlist, counter) {
os.evsel = counter;
- if (!first && config->json_output)
- fprintf(config->output, ", ");
- first = false;
-
perf_stat__print_shadow_stats(config, counter, 0,
0,
&out,
&config->metric_events,
&rt_stat);
}
- if (config->json_output)
- fprintf(config->output, "}");
fputc('\n', config->output);
}
@@ -954,14 +944,8 @@ static void print_interval(struct perf_stat_config *config,
}
}
- if ((num_print_interval == 0 || config->interval_clear)
- && metric_only && !config->json_output)
+ if ((num_print_interval == 0 || config->interval_clear) && metric_only)
print_metric_headers(config, evlist, " ", true);
- if ((num_print_interval == 0 || config->interval_clear)
- && metric_only && config->json_output) {
- fprintf(output, "{");
- print_metric_headers(config, evlist, " ", true);
- }
if (++num_print_interval == 25)
num_print_interval = 0;
}
--
2.38.1.493.g58b659f92b-goog
The --interval-clear option makes perf stat to clear the terminal at
each interval. But it doesn't need to clear the screen when it saves
to a file. Make it fail when it's enabled with the output options.
$ perf stat -I 1 --interval-clear -o myfile true
--interval-clear does not work with output
Usage: perf stat [<options>] [<command>]
-o, --output <file> output file name
--log-fd <n> log output to fd, instead of stderr
--interval-clear clear screen in between new interval
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/perf/builtin-stat.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index d5e1670bca20..1d79801f4e84 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -2403,6 +2403,14 @@ int cmd_stat(int argc, const char **argv)
}
}
+ if (stat_config.interval_clear && !isatty(fileno(output))) {
+ fprintf(stderr, "--interval-clear does not work with output\n");
+ parse_options_usage(stat_usage, stat_options, "o", 1);
+ parse_options_usage(NULL, stat_options, "log-fd", 0);
+ parse_options_usage(NULL, stat_options, "interval-clear", 0);
+ return -1;
+ }
+
stat_config.output = output;
/*
--
2.38.1.493.g58b659f92b-goog
The num_print_interval and config->interval_clear should be checked
together like other places like later in the function. Otherwise,
the --interval-clear option could print the headers for the CSV or
JSON output unnecessarily.
Acked-by: Ian Rogers <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/perf/util/stat-display.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index 03d58277e8d6..5f4fb0bd4037 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -902,8 +902,8 @@ static void print_interval(struct perf_stat_config *config,
sprintf(prefix, "{\"interval\" : %lu.%09lu}", (unsigned long)
ts->tv_sec, ts->tv_nsec);
- if ((num_print_interval == 0 && !config->csv_output && !config->json_output)
- || config->interval_clear) {
+ if ((num_print_interval == 0 || config->interval_clear) &&
+ !config->csv_output && !config->json_output) {
switch (config->aggr_mode) {
case AGGR_NODE:
fprintf(output, "# time node cpus");
--
2.38.1.493.g58b659f92b-goog
On Fri, Nov 11, 2022 at 7:22 PM Namhyung Kim <[email protected]> wrote:
>
> Currently it prints all metric headers for JSON output. But actually it
> skips some metrics with valid_only_metric(). So the output looks like:
>
> $ perf stat --metric-only --json true
> {"unit" : "CPUs utilized", "unit" : "/sec", "unit" : "/sec", "unit" : "/sec", "unit" : "GHz", "unit" : "insn per cycle", "unit" : "/sec", "unit" : "branch-misses of all branches"}
> {"metric-value" : "3.861"}{"metric-value" : "0.79"}{"metric-value" : "3.04"}
>
> As you can see there are 8 units in the header but only 3 metric-values
> are there. It should skip the unused headers as well. Also each unit
> should be printed as a separate object like metric values.
>
> With this patch:
>
> $ perf stat --metric-only --json true
> {"unit" : "GHz"}{"unit" : "insn per cycle"}{"unit" : "branch-misses of all branches"}
> {"metric-value" : "4.166"}{"metric-value" : "0.73"}{"metric-value" : "2.96"}
>
> Fixes: df936cadfb58 ("perf stat: Add JSON output option")
> Signed-off-by: Namhyung Kim <[email protected]>
Acked-by: Ian Rogers <[email protected]>
Thanks,
Ian
> ---
> tools/perf/util/stat-display.c | 22 +++-------------------
> 1 file changed, 3 insertions(+), 19 deletions(-)
>
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index c7b3a1e10263..96ad0c71adc2 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -430,12 +430,12 @@ static void print_metric_header(struct perf_stat_config *config,
> os->evsel->priv != os->evsel->evlist->selected->priv)
> return;
>
> - if (!valid_only_metric(unit) && !config->json_output)
> + if (!valid_only_metric(unit))
> return;
> unit = fixunit(tbuf, os->evsel, unit);
>
> if (config->json_output)
> - fprintf(os->fh, "\"unit\" : \"%s\"", unit);
> + fprintf(os->fh, "{\"unit\" : \"%s\"}", unit);
> else if (config->csv_output)
> fprintf(os->fh, "%s%s", unit, config->csv_sep);
> else
> @@ -847,10 +847,6 @@ static void print_metric_headers(struct perf_stat_config *config,
> .new_line = new_line_metric,
> .force_header = true,
> };
> - bool first = true;
> -
> - if (config->json_output && !config->interval)
> - fprintf(config->output, "{");
>
> if (prefix && !config->json_output)
> fprintf(config->output, "%s", prefix);
> @@ -871,18 +867,12 @@ static void print_metric_headers(struct perf_stat_config *config,
> evlist__for_each_entry(evlist, counter) {
> os.evsel = counter;
>
> - if (!first && config->json_output)
> - fprintf(config->output, ", ");
> - first = false;
> -
> perf_stat__print_shadow_stats(config, counter, 0,
> 0,
> &out,
> &config->metric_events,
> &rt_stat);
> }
> - if (config->json_output)
> - fprintf(config->output, "}");
> fputc('\n', config->output);
> }
>
> @@ -954,14 +944,8 @@ static void print_interval(struct perf_stat_config *config,
> }
> }
>
> - if ((num_print_interval == 0 || config->interval_clear)
> - && metric_only && !config->json_output)
> + if ((num_print_interval == 0 || config->interval_clear) && metric_only)
> print_metric_headers(config, evlist, " ", true);
> - if ((num_print_interval == 0 || config->interval_clear)
> - && metric_only && config->json_output) {
> - fprintf(output, "{");
> - print_metric_headers(config, evlist, " ", true);
> - }
> if (++num_print_interval == 25)
> num_print_interval = 0;
> }
> --
> 2.38.1.493.g58b659f92b-goog
>
On Fri, Nov 11, 2022 at 7:22 PM Namhyung Kim <[email protected]> wrote:
>
> The --interval-clear option makes perf stat to clear the terminal at
> each interval. But it doesn't need to clear the screen when it saves
> to a file. Make it fail when it's enabled with the output options.
>
> $ perf stat -I 1 --interval-clear -o myfile true
> --interval-clear does not work with output
>
> Usage: perf stat [<options>] [<command>]
>
> -o, --output <file> output file name
> --log-fd <n> log output to fd, instead of stderr
> --interval-clear clear screen in between new interval
>
> Signed-off-by: Namhyung Kim <[email protected]>
Acked-by: Ian Rogers <[email protected]>
Thanks,
Ian
> ---
> tools/perf/builtin-stat.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index d5e1670bca20..1d79801f4e84 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -2403,6 +2403,14 @@ int cmd_stat(int argc, const char **argv)
> }
> }
>
> + if (stat_config.interval_clear && !isatty(fileno(output))) {
> + fprintf(stderr, "--interval-clear does not work with output\n");
> + parse_options_usage(stat_usage, stat_options, "o", 1);
> + parse_options_usage(NULL, stat_options, "log-fd", 0);
> + parse_options_usage(NULL, stat_options, "interval-clear", 0);
> + return -1;
> + }
> +
> stat_config.output = output;
>
> /*
> --
> 2.38.1.493.g58b659f92b-goog
>
On Fri, Nov 11, 2022 at 7:22 PM Namhyung Kim <[email protected]> wrote:
>
> The following command will get segfault due to missing aggr_header_csv
> for AGGR_NODE:
>
> $ sudo perf stat -a --per-node -x, --metric-only true
>
> Fixes: 86895b480a2f ("perf stat: Add --per-node agregation support")
> Signed-off-by: Namhyung Kim <[email protected]>
Acked-by: Ian Rogers <[email protected]>
Thanks,
Ian
> ---
> tools/perf/util/stat-display.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index 657434cd29ee..ea41e6308c50 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -534,7 +534,7 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
> [AGGR_CORE] = 2,
> [AGGR_THREAD] = 1,
> [AGGR_UNSET] = 0,
> - [AGGR_NODE] = 0,
> + [AGGR_NODE] = 1,
> };
>
> pm = config->metric_only ? print_metric_only_csv : print_metric_csv;
> @@ -819,6 +819,7 @@ static int aggr_header_lens[] = {
> [AGGR_SOCKET] = 12,
> [AGGR_NONE] = 6,
> [AGGR_THREAD] = 24,
> + [AGGR_NODE] = 6,
> [AGGR_GLOBAL] = 0,
> };
>
> @@ -828,6 +829,7 @@ static const char *aggr_header_csv[] = {
> [AGGR_SOCKET] = "socket,cpus",
> [AGGR_NONE] = "cpu,",
> [AGGR_THREAD] = "comm-pid,",
> + [AGGR_NODE] = "node,",
> [AGGR_GLOBAL] = ""
> };
>
> --
> 2.38.1.493.g58b659f92b-goog
>
On Fri, Nov 11, 2022 at 7:23 PM Namhyung Kim <[email protected]> wrote:
>
> It should not print "summary" for each event when --metric-only is set.
>
> Before:
> $ sudo perf stat -a --per-socket --summary -x, --metric-only true
> time,socket,cpusGhz,insn per cycle,branch-misses of all branches,
> 0.000709079,S0,8,0.893,2.40,0.45,
> S0,8, summary, summary, summary, summary, summary,0.893, summary,2.40, summary, summary,0.45,
>
> After:
> $ sudo perf stat -a --per-socket --summary -x, --metric-only true
> time,socket,cpusGHz,insn per cycle,branch-misses of all branches,
> 0.000882297,S0,8,0.598,1.64,0.64,
> summary,S0,8,0.598,1.64,0.64,
>
> Signed-off-by: Namhyung Kim <[email protected]>
Acked-by: Ian Rogers <[email protected]>
Thanks,
Ian
> ---
> tools/perf/util/stat-display.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index ce81798b5864..96bb7a42fd41 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -549,7 +549,7 @@ static void printout(struct perf_stat_config *config, struct aggr_cpu_id id, int
> }
>
> if (!config->no_csv_summary && config->csv_output &&
> - config->summary && !config->interval) {
> + config->summary && !config->interval && !config->metric_only) {
> fprintf(config->output, "%16s%s", "summary", config->csv_sep);
> }
>
> @@ -732,8 +732,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++) {
> - if (prefix && metric_only)
> - fprintf(output, "%s", prefix);
> + if (metric_only) {
> + if (prefix)
> + fprintf(output, "%s", prefix);
> + else if (config->summary && !config->no_csv_summary &&
> + config->csv_output && !config->interval)
> + fprintf(output, "%16s%s", "summary", config->csv_sep);
> + }
>
> first = true;
> evlist__for_each_entry(evlist, counter) {
> --
> 2.38.1.493.g58b659f92b-goog
>
On Fri, Nov 11, 2022 at 7:23 PM Namhyung Kim <[email protected]> wrote:
>
> It should have a comma after 'cpus' for socket and die aggregation mode.
> The output of the following command shows the issue.
>
> $ sudo perf stat -a --per-socket -x, --metric-only -I1 true
>
> Before:
> +--- here
> V
> time,socket,cpusGhz,insn per cycle,branch-misses of all branches,
> 0.000908461,S0,8,0.950,1.65,1.21,
>
> After:
> time,socket,cpus,GHz,insn per cycle,branch-misses of all branches,
> 0.000683094,S0,8,0.593,2.00,0.60,
>
> Signed-off-by: Namhyung Kim <[email protected]>
Acked-by: Ian Rogers <[email protected]>
Thanks,
Ian
> ---
> tools/perf/util/stat-display.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index 96bb7a42fd41..a316807255cd 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -828,8 +828,8 @@ static int aggr_header_lens[] = {
>
> static const char *aggr_header_csv[] = {
> [AGGR_CORE] = "core,cpus,",
> - [AGGR_DIE] = "die,cpus",
> - [AGGR_SOCKET] = "socket,cpus",
> + [AGGR_DIE] = "die,cpus,",
> + [AGGR_SOCKET] = "socket,cpus,",
> [AGGR_NONE] = "cpu,",
> [AGGR_THREAD] = "comm-pid,",
> [AGGR_NODE] = "node,",
> --
> 2.38.1.493.g58b659f92b-goog
>