linux-kernel - Re: [PATCH 1/2] perf stat: Use field separator in the metric header

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Z7zUDxho5vLa_2fF@google.com>
Date: Mon, 24 Feb 2025 12:18:23 -0800
From: Namhyung Kim <namhyung@...nel.org>
To: Ian Rogers <irogers@...gle.com>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>,
	Kan Liang <kan.liang@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>,
	Adrian Hunter <adrian.hunter@...el.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...nel.org>, LKML <linux-kernel@...r.kernel.org>,
	linux-perf-users@...r.kernel.org
Subject: Re: [PATCH 1/2] perf stat: Use field separator in the metric header

On Mon, Feb 24, 2025 at 10:50:24AM -0800, Ian Rogers wrote:
> On Thu, Jun 27, 2024 at 3:24 PM Namhyung Kim <namhyung@...nel.org> wrote:
> >
> > Hi Ian,
> >
> > On Thu, Jun 27, 2024 at 1:48 PM Ian Rogers <irogers@...gle.com> wrote:
> > >
> > > On Thu, Jun 27, 2024 at 1:03 PM Namhyung Kim <namhyung@...nel.org> wrote:
> > > >
> > > > It didn't use the passed field separator (using -x option) when it
> > > > prints the metric headers and always put "," between the fields.
> > > >
> > > > Before:
> > > >   $ sudo ./perf stat -a -x : --per-core -M tma_core_bound --metric-only true
> > > >   core,cpus,%  tma_core_bound:     <<<--- here: "core,cpus," but ":" expected
> > > >   S0-D0-C0:2:10.5:
> > > >   S0-D0-C1:2:14.8:
> > > >   S0-D0-C2:2:9.9:
> > > >   S0-D0-C3:2:13.2:
> > > >
> > > > After:
> > > >   $ sudo ./perf stat -a -x : --per-core -M tma_core_bound --metric-only true
> > > >   core:cpus:%  tma_core_bound:
> > > >   S0-D0-C0:2:10.5:
> > > >   S0-D0-C1:2:15.0:
> > > >   S0-D0-C2:2:16.5:
> > > >   S0-D0-C3:2:12.5:
> > > >
> > > > Signed-off-by: Namhyung Kim <namhyung@...nel.org>
> > > > ---
> > > >  tools/perf/util/stat-display.c | 37 ++++++++++++++++++++++++++--------
> > > >  1 file changed, 29 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> > > > index 91d2f7f65df7..e8673c9f6b49 100644
> > > > --- a/tools/perf/util/stat-display.c
> > > > +++ b/tools/perf/util/stat-display.c
> > > > @@ -47,16 +47,27 @@ static int aggr_header_lens[] = {
> > > >  };
> > > >
> > > >  static const char *aggr_header_csv[] = {
> > > > -       [AGGR_CORE]     =       "core,cpus,",
> > > > -       [AGGR_CACHE]    =       "cache,cpus,",
> > > > -       [AGGR_DIE]      =       "die,cpus,",
> > > > -       [AGGR_SOCKET]   =       "socket,cpus,",
> > > > -       [AGGR_NONE]     =       "cpu,",
> > > > -       [AGGR_THREAD]   =       "comm-pid,",
> > > > -       [AGGR_NODE]     =       "node,",
> > > > +       [AGGR_CORE]     =       "core%scpus%s",
> > > > +       [AGGR_CACHE]    =       "cache%scpus%s",
> > > > +       [AGGR_DIE]      =       "die%scpus%s",
> > > > +       [AGGR_SOCKET]   =       "socket%scpus%s",
> > > > +       [AGGR_NONE]     =       "cpu%s",
> > > > +       [AGGR_THREAD]   =       "comm-pid%s",
> > > > +       [AGGR_NODE]     =       "node%s",
> > > >         [AGGR_GLOBAL]   =       ""
> > > >  };
> > > >
> > > > +static int aggr_header_num[] = {
> > > > +       [AGGR_CORE]     =       2,
> > > > +       [AGGR_CACHE]    =       2,
> > > > +       [AGGR_DIE]      =       2,
> > > > +       [AGGR_SOCKET]   =       2,
> > > > +       [AGGR_NONE]     =       1,
> > > > +       [AGGR_THREAD]   =       1,
> > > > +       [AGGR_NODE]     =       1,
> > > > +       [AGGR_GLOBAL]   =       0,
> > > > +};
> > > > +
> > > >  static const char *aggr_header_std[] = {
> > > >         [AGGR_CORE]     =       "core",
> > > >         [AGGR_CACHE]    =       "cache",
> > > > @@ -1185,8 +1196,18 @@ static void print_metric_headers_csv(struct perf_stat_config *config,
> > > >  {
> > > >         if (config->interval)
> > > >                 fputs("time,", config->output);
> > > > -       if (!config->iostat_run)
> > > > +       if (config->iostat_run)
> > > > +               return;
> > > > +
> > >
> > > Having a static count of commas seems somewhat error prone, perhaps:
> > > ```
> > > const char *header = aggr_header_csv[config->aggr_mode];
> > > if (config->csv_sep == ',' || !strchr(header, ',')) {
> > >   fputs(config->output, header);
> > > } else {
> > >   char *tmp = strdup(header);
> > >   char *p = tmp;
> > >    while (p && *p) {
> > >       if (p == ',')
> > >         *p = config->csv_sep;
> > >      p++;
> > >    }
> > >   fputs(config->output, tmp);
> > >   free(tmp);
> > > }
> > > ```
> >
> > Looks good.  But I think we should handle longer separators like -x ":::".
> > Will do in v2.
> 
> Hi Namhyung,
> 
> It looks like this has been forgotten. Did you have a v2?

It's merged to v6.11, please see

https://lore.kernel.org/linux-perf-users/171994580797.2905908.17252651084023923233.b4-ty@kernel.org/

Thanks,
Namhyung