From: Ian Rogers <irogers@google.com>
To: Namhyung Kim <namhyung@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>,
Jiri Olsa <jolsa@kernel.org>, Ingo Molnar <mingo@kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
LKML <linux-kernel@vger.kernel.org>,
Adrian Hunter <adrian.hunter@intel.com>,
linux-perf-users@vger.kernel.org,
Kan Liang <kan.liang@linux.intel.com>,
Zhengjun Xing <zhengjun.xing@linux.intel.com>,
James Clark <james.clark@arm.com>,
Athira Jajeev <atrajeev@linux.vnet.ibm.com>
Subject: Re: [PATCH 05/15] perf stat: Remove prefix argument in print_metric_headers()
Date: Tue, 29 Nov 2022 21:13:11 -0800 [thread overview]
Message-ID: <CAP-5=fUOjSM5HajU9TCD6prY39LbX4OQbkEbtKPPGRBPBN=_VQ@mail.gmail.com> (raw)
In-Reply-To: <CAP-5=fXsWfOmavOAmxsUcX+g3cve2r20JmseDF7Bn1jO6tPX-Q@mail.gmail.com>
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
> > >
next prev parent reply other threads:[~2022-11-30 5:14 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-23 18:01 [PATCHSET 00/15] perf stat: Improve perf stat output (v2) Namhyung Kim
2022-11-23 18:01 ` [PATCH 01/15] perf stat: Fix cgroup display in JSON output Namhyung Kim
2022-11-23 23:20 ` Ian Rogers
2022-11-24 12:45 ` Arnaldo Carvalho de Melo
2022-11-23 18:01 ` [PATCH 02/15] perf stat: Move summary prefix printing logic in CSV output Namhyung Kim
2022-11-23 23:20 ` Ian Rogers
2022-11-23 18:01 ` [PATCH 03/15] perf stat: Do not align time prefix " Namhyung Kim
2022-11-23 23:21 ` Ian Rogers
2022-11-23 18:01 ` [PATCH 04/15] perf stat: Use scnprintf() in prepare_interval() Namhyung Kim
2022-11-23 23:22 ` Ian Rogers
2022-11-23 18:01 ` [PATCH 05/15] perf stat: Remove prefix argument in print_metric_headers() Namhyung Kim
2022-11-23 23:23 ` Ian Rogers
2022-11-30 5:09 ` Ian Rogers
2022-11-30 5:13 ` Ian Rogers [this message]
2022-11-30 21:21 ` Namhyung Kim
2022-12-05 12:22 ` Arnaldo Carvalho de Melo
2022-12-05 12:40 ` Arnaldo Carvalho de Melo
2022-11-23 18:01 ` [PATCH 06/15] perf stat: Remove metric_only argument in print_counter_aggrdata() Namhyung Kim
2022-11-23 23:23 ` Ian Rogers
2022-11-23 18:02 ` [PATCH 07/15] perf stat: Pass const char *prefix to display routines Namhyung Kim
2022-11-23 23:24 ` Ian Rogers
2022-11-23 18:02 ` [PATCH 08/15] perf stat: Use struct outstate in evlist__print_counters() Namhyung Kim
2022-11-23 23:24 ` Ian Rogers
2022-11-23 18:02 ` [PATCH 09/15] perf stat: Pass struct outstate to print_metric_begin() Namhyung Kim
2022-11-23 23:25 ` Ian Rogers
2022-11-23 18:02 ` [PATCH 10/15] perf stat: Pass struct outstate to printout() Namhyung Kim
2022-11-23 23:26 ` Ian Rogers
2022-11-23 18:02 ` [PATCH 11/15] perf stat: Do not pass runtime_stat " Namhyung Kim
2022-11-23 23:27 ` Ian Rogers
2022-11-23 18:02 ` [PATCH 12/15] perf stat: Pass through struct outstate Namhyung Kim
2022-11-23 23:27 ` Ian Rogers
2022-11-23 18:02 ` [PATCH 13/15] perf stat: Fix JSON output in metric-only mode Namhyung Kim
2022-11-23 23:28 ` Ian Rogers
2022-11-23 18:02 ` [PATCH 14/15] perf stat: Rename "aggregate-number" to "cpu-count" in JSON Namhyung Kim
2022-11-23 23:30 ` Ian Rogers
2022-11-25 7:50 ` Namhyung Kim
2022-11-27 3:14 ` Ian Rogers
2022-11-29 22:45 ` Namhyung Kim
2022-11-30 5:01 ` Ian Rogers
2022-11-23 18:02 ` [PATCH 15/15] perf stat: Tidy up JSON metric-only output when no metrics Namhyung Kim
2022-11-23 23:31 ` Ian Rogers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAP-5=fUOjSM5HajU9TCD6prY39LbX4OQbkEbtKPPGRBPBN=_VQ@mail.gmail.com' \
--to=irogers@google.com \
--cc=acme@kernel.org \
--cc=adrian.hunter@intel.com \
--cc=atrajeev@linux.vnet.ibm.com \
--cc=james.clark@arm.com \
--cc=jolsa@kernel.org \
--cc=kan.liang@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mingo@kernel.org \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.org \
--cc=zhengjun.xing@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).