| 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
| ||
|
Message-ID: <CAP-5=fVmybd=WTKS3kvsF+VU_Lrke9hEvNyBtunAHBw-6ViZig@mail.gmail.com>
Date: Mon, 24 Feb 2025 10:50:24 -0800
From: Ian Rogers <irogers@...gle.com>
To: Namhyung Kim <namhyung@...nel.org>
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 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?
Thanks,
Ian
> > 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