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] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAM9d7cibrW=K=GZB_zsURB1Ff6Eok7qy3rVt83VVM6pVhv+Agg@mail.gmail.com>
Date: Thu, 27 Jun 2024 15:23:47 -0700
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

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.

> I'm somewhat surprised that we have no metric tests in the stat output
> tests like tools/perf/tests/shell/stat+csv_output.sh. Perhaps we can
> add the following:
> ```
> diff --git a/tools/perf/tests/shell/lib/stat_output.sh
> b/tools/perf/tests/shell/lib/stat_output.sh
> index 9a176ceae4a3..a920b2d78abb 100644
> --- a/tools/perf/tests/shell/lib/stat_output.sh
> +++ b/tools/perf/tests/shell/lib/stat_output.sh
> @@ -148,6 +148,14 @@ check_per_socket()
>        echo "[Success]"
> }
>
> +check_metric_only()
> +{
> +        echo -n "Checking $1 output: metric only "
> +        perf stat --metric-only $2 -e instructions,cycles true
> +        commachecker --metric-only
> +        echo "[Success]"
> +}
> +
> # The perf stat options for per-socket, per-core, per-die
> # and -A ( no_aggr mode ) uses the info fetched from this
> # directory: "/sys/devices/system/cpu/cpu*/topology". For
> diff --git a/tools/perf/tests/shell/stat+csv_output.sh
> b/tools/perf/tests/shell/stat+csv_output.sh
> index fc2d8cc6e5e0..d6807dbab931 100755
> --- a/tools/perf/tests/shell/stat+csv_output.sh
> +++ b/tools/perf/tests/shell/stat+csv_output.sh
> @@ -44,6 +44,7 @@ function commachecker()
>        ;; "--per-die")         exp=8
>        ;; "--per-cluster")     exp=8
>        ;; "--per-cache")       exp=8
> +        ;; "--metric-only")     exp=2
>        esac
>
>        while read line
> @@ -83,6 +84,7 @@ then
>        check_per_cluster "CSV" "$perf_cmd"
>        check_per_die "CSV" "$perf_cmd"
>        check_per_socket "CSV" "$perf_cmd"
> +        check_metric_only "CSV" "$perf_cmd"
> else
>        echo "[Skip] Skipping tests for system_wide_no_aggr, per_core,
> per_die and per_socket since
> socket id exposed via topology is invalid"
> fi
> ```
> It is using the hard coded metrics and it looks like the header
> printing for that is broken, but this is so often the case for stat
> output :-(

Right, I also noticed something in the header.  One more work
item to the list.

Anyway, I'll add it to the test case!

Thanks,
Namhyung

>
> > +       if (aggr_header_num[config->aggr_mode] == 1) {
> > +               fprintf(config->output, aggr_header_csv[config->aggr_mode],
> > +                       config->csv_sep);
> > +       } else if (aggr_header_num[config->aggr_mode] == 2) {
> > +               fprintf(config->output, aggr_header_csv[config->aggr_mode],
> > +                       config->csv_sep, config->csv_sep);
> > +       } else {
> >                 fputs(aggr_header_csv[config->aggr_mode], config->output);
> > +       }
> >  }
> >
> >  static void print_metric_headers_json(struct perf_stat_config *config __maybe_unused,
> > --
> > 2.45.2.803.g4e1b14247a-goog
> >

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ