| 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
| ||
|
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