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

[Namhyung Kim <namhyung@kernel.org>]

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@kernel.org> wrote:
> >
> > Hi Ian,
> >
> > On Thu, Jun 27, 2024 at 1:48 PM Ian Rogers <irogers@google.com> wrote:
> > >
> > > On Thu, Jun 27, 2024 at 1:03 PM Namhyung Kim <namhyung@kernel.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@kernel.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