All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/6] perf stat: Introduce iiostat mode to provide I/O performance metrics
@ 2020-12-23 13:03 Alexander Antonov
  2020-12-23 13:03 ` [PATCH v2 1/6] perf stat: Add AGGR_IIO_STACK mode Alexander Antonov
                   ` (5 more replies)
  0 siblings, 6 replies; 19+ messages in thread
From: Alexander Antonov @ 2020-12-23 13:03 UTC (permalink / raw)
  To: acme
  Cc: linux-kernel, jolsa, ak, alexander.shishkin, mark.rutland,
	namhyung, irogers, mingo, peterz, alexander.antonov

The previous version can be found at:
v1: https://lkml.kernel.org/r/20201210090340.14358-1-alexander.antonov@linux.intel.com

Changes in this revision are:
v1 -> v2:
  1. Using 'perf iiostat' subcommand instead of 'perf stat --iiostat':
    - Added perf-iiostat.sh script to use short command
    - Updated manual pages to get help for 'perf iiostat'
    - Added 'perf-iiostat' to perf's gitignore file

Mode is intended to provide four I/O performance metrics in MB per each
IIO stack:
 - Inbound Read:   I/O devices below IIO stack read from the host memory
 - Inbound Write:  I/O devices below IIO stack write to the host memory
 - Outbound Read:  CPU reads from I/O devices below IIO stack
 - Outbound Write: CPU writes to I/O devices below IIO stack

Each metric requiries only one IIO event which increments at every 4B
transfer in corresponding direction. The formulas to compute metrics
are generic:
    #EventCount * 4B / (1024 * 1024)

Note: iiostat introduces new perf data aggregation mode - per I/O stack
hence -e and -M options are not supported.

Usage examples:

1. List all IIO stacks (example for 2-S platform):
   $ perf iiostat show
   S0-uncore_iio_0<0000:00>
   S1-uncore_iio_0<0000:80>
   S0-uncore_iio_1<0000:17>
   S1-uncore_iio_1<0000:85>
   S0-uncore_iio_2<0000:3a>
   S1-uncore_iio_2<0000:ae>
   S0-uncore_iio_3<0000:5d>
   S1-uncore_iio_3<0000:d7>

2. Collect metrics for all I/O stacks:
   $ perf iiostat -- dd if=/dev/zero of=/dev/nvme0n1 bs=1M oflag=direct
   357708+0 records in
   357707+0 records out
   375083606016 bytes (375 GB, 349 GiB) copied, 215.974 s, 1.7 GB/s

    Performance counter stats for 'system wide':

      port             Inbound Read(MB)    Inbound Write(MB)    Outbound Read(MB)   Outbound Write(MB) 
   0000:00                    1                    0                    2                    3 
   0000:80                    0                    0                    0                    0 
   0000:17               352552                   43                    0                   21 
   0000:85                    0                    0                    0                    0 
   0000:3a                    3                    0                    0                    0 
   0000:ae                    0                    0                    0                    0 
   0000:5d                    0                    0                    0                    0 
   0000:d7                    0                    0                    0                    0

3. Collect metrics for comma separated list of I/O stacks:
   $ perf iiostat 0000:17,0:3a -- dd if=/dev/zero of=/dev/nvme0n1 bs=1M oflag=direct
   357708+0 records in
   357707+0 records out
   375083606016 bytes (375 GB, 349 GiB) copied, 197.08 s, 1.9 GB/s

    Performance counter stats for 'system wide':

      port             Inbound Read(MB)    Inbound Write(MB)    Outbound Read(MB)   Outbound Write(MB) 
   0000:17               358559                   44                    0                   22 
   0000:3a                    3                    2                    0                    0 

        197.081983474 seconds time elapsed


Alexander Antonov (6):
  perf stat: Add AGGR_IIO_STACK mode
  perf evsel: Introduce an observed performance device
  perf stat: Basic support for iiostat in perf
  perf stat: Helper functions for IIO stacks list in iiostat mode
  perf stat: Enable iiostat mode for x86 platforms
  perf: Update .gitignore file

 tools/perf/.gitignore                         |   1 +
 tools/perf/Documentation/perf-iiostat.txt     |  89 ++++
 tools/perf/Makefile.perf                      |   5 +-
 tools/perf/arch/x86/util/Build                |   1 +
 tools/perf/arch/x86/util/iiostat.c            | 462 ++++++++++++++++++
 tools/perf/builtin-stat.c                     |  40 +-
 tools/perf/command-list.txt                   |   1 +
 tools/perf/perf-iiostat.sh                    |  12 +
 tools/perf/util/evsel.h                       |   1 +
 tools/perf/util/iiostat.h                     |  33 ++
 .../scripting-engines/trace-event-python.c    |   2 +-
 tools/perf/util/stat-display.c                |  51 +-
 tools/perf/util/stat-shadow.c                 |  11 +-
 tools/perf/util/stat.c                        |   3 +-
 tools/perf/util/stat.h                        |   2 +
 15 files changed, 704 insertions(+), 10 deletions(-)
 create mode 100644 tools/perf/Documentation/perf-iiostat.txt
 create mode 100644 tools/perf/arch/x86/util/iiostat.c
 create mode 100644 tools/perf/perf-iiostat.sh
 create mode 100644 tools/perf/util/iiostat.h


base-commit: 644bf4b0f7acde641d3db200b4db66977e96c3bd
-- 
2.19.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v2 1/6] perf stat: Add AGGR_IIO_STACK mode
  2020-12-23 13:03 [PATCH v2 0/6] perf stat: Introduce iiostat mode to provide I/O performance metrics Alexander Antonov
@ 2020-12-23 13:03 ` Alexander Antonov
  2020-12-23 13:03 ` [PATCH v2 2/6] perf evsel: Introduce an observed performance device Alexander Antonov
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 19+ messages in thread
From: Alexander Antonov @ 2020-12-23 13:03 UTC (permalink / raw)
  To: acme
  Cc: linux-kernel, jolsa, ak, alexander.shishkin, mark.rutland,
	namhyung, irogers, mingo, peterz, alexander.antonov

Adding AGGR_IIO_STACK mode to be able to distinguish aggr_mode
for IIO stacks in following patches.

Signed-off-by: Alexander Antonov <alexander.antonov@linux.intel.com>
---
 tools/perf/builtin-stat.c                           |  7 +++++--
 .../util/scripting-engines/trace-event-python.c     |  2 +-
 tools/perf/util/stat-display.c                      | 13 +++++++++++--
 tools/perf/util/stat.c                              |  3 ++-
 tools/perf/util/stat.h                              |  1 +
 5 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index f15b2f8aa14d..72f9d0aa3f96 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -913,7 +913,7 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
 		init_stats(&walltime_nsecs_stats);
 		update_stats(&walltime_nsecs_stats, t1 - t0);
 
-		if (stat_config.aggr_mode == AGGR_GLOBAL)
+		if (stat_config.aggr_mode == AGGR_GLOBAL || stat_config.aggr_mode == AGGR_IIO_STACK)
 			perf_evlist__save_aggr_prev_raw_counts(evsel_list);
 
 		perf_evlist__copy_prev_raw_counts(evsel_list);
@@ -1309,6 +1309,7 @@ static int perf_stat_init_aggr_mode(void)
 		break;
 	case AGGR_GLOBAL:
 	case AGGR_THREAD:
+	case AGGR_IIO_STACK:
 	case AGGR_UNSET:
 	default:
 		break;
@@ -1499,6 +1500,7 @@ static int perf_stat_init_aggr_mode_file(struct perf_stat *st)
 	case AGGR_NONE:
 	case AGGR_GLOBAL:
 	case AGGR_THREAD:
+	case AGGR_IIO_STACK:
 	case AGGR_UNSET:
 	default:
 		break;
@@ -2216,7 +2218,8 @@ int cmd_stat(int argc, const char **argv)
 	 * --per-thread is aggregated per thread, we dont mix it with cpu mode
 	 */
 	if (((stat_config.aggr_mode != AGGR_GLOBAL &&
-	      stat_config.aggr_mode != AGGR_THREAD) || nr_cgroups) &&
+	      stat_config.aggr_mode != AGGR_THREAD &&
+	      stat_config.aggr_mode != AGGR_IIO_STACK) || nr_cgroups) &&
 	    !target__has_cpu(&target)) {
 		fprintf(stderr, "both cgroup and no-aggregation "
 			"modes only available in system-wide mode\n");
diff --git a/tools/perf/util/scripting-engines/trace-event-python.c b/tools/perf/util/scripting-engines/trace-event-python.c
index c83c2c6564e0..e8b472faeae4 100644
--- a/tools/perf/util/scripting-engines/trace-event-python.c
+++ b/tools/perf/util/scripting-engines/trace-event-python.c
@@ -1401,7 +1401,7 @@ static void python_process_stat(struct perf_stat_config *config,
 	struct perf_cpu_map *cpus = counter->core.cpus;
 	int cpu, thread;
 
-	if (config->aggr_mode == AGGR_GLOBAL) {
+	if (config->aggr_mode == AGGR_GLOBAL || config->aggr_mode == AGGR_IIO_STACK) {
 		process_stat(counter, -1, -1, tstamp,
 			     &counter->counts->aggr);
 		return;
diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index 4b57c0c07632..3bfcdb80443a 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -133,6 +133,7 @@ static void aggr_printout(struct perf_stat_config *config,
 			config->csv_sep);
 		break;
 	case AGGR_GLOBAL:
+	case AGGR_IIO_STACK:
 	case AGGR_UNSET:
 	default:
 		break;
@@ -330,7 +331,7 @@ static int first_shadow_cpu(struct perf_stat_config *config,
 	if (config->aggr_mode == AGGR_NONE)
 		return id;
 
-	if (config->aggr_mode == AGGR_GLOBAL)
+	if (config->aggr_mode == AGGR_GLOBAL || config->aggr_mode == AGGR_IIO_STACK)
 		return 0;
 
 	for (i = 0; i < evsel__nr_cpus(evsel); i++) {
@@ -424,6 +425,7 @@ static void printout(struct perf_stat_config *config, int id, int nr,
 	if (config->csv_output && !config->metric_only) {
 		static int aggr_fields[] = {
 			[AGGR_GLOBAL] = 0,
+			[AGGR_IIO_STACK] = 0,
 			[AGGR_THREAD] = 1,
 			[AGGR_NONE] = 1,
 			[AGGR_SOCKET] = 2,
@@ -906,6 +908,7 @@ static int aggr_header_lens[] = {
 	[AGGR_NONE] = 6,
 	[AGGR_THREAD] = 24,
 	[AGGR_GLOBAL] = 0,
+	[AGGR_IIO_STACK] = 0,
 };
 
 static const char *aggr_header_csv[] = {
@@ -914,7 +917,8 @@ static const char *aggr_header_csv[] = {
 	[AGGR_SOCKET] 	= 	"socket,cpus",
 	[AGGR_NONE] 	= 	"cpu,",
 	[AGGR_THREAD] 	= 	"comm-pid,",
-	[AGGR_GLOBAL] 	=	""
+	[AGGR_GLOBAL]	=	"",
+	[AGGR_IIO_STACK] =	"port,"
 };
 
 static void print_metric_headers(struct perf_stat_config *config,
@@ -1001,6 +1005,9 @@ static void print_interval(struct perf_stat_config *config,
 			if (!metric_only)
 				fprintf(output, "                  counts %*s events\n", unit_width, "unit");
 			break;
+		case AGGR_IIO_STACK:
+			fprintf(output, "#           time    port        ");
+			break;
 		case AGGR_GLOBAL:
 		default:
 			fprintf(output, "#           time");
@@ -1246,6 +1253,8 @@ perf_evlist__print_counters(struct evlist *evlist,
 			}
 		}
 		break;
+	case AGGR_IIO_STACK:
+		break;
 	case AGGR_UNSET:
 	default:
 		break;
diff --git a/tools/perf/util/stat.c b/tools/perf/util/stat.c
index bd0decd6d753..6a655f909587 100644
--- a/tools/perf/util/stat.c
+++ b/tools/perf/util/stat.c
@@ -363,6 +363,7 @@ process_counter_values(struct perf_stat_config *config, struct evsel *evsel,
 		}
 		break;
 	case AGGR_GLOBAL:
+	case AGGR_IIO_STACK:
 		aggr->val += count->val;
 		aggr->ena += count->ena;
 		aggr->run += count->run;
@@ -424,7 +425,7 @@ int perf_stat_process_counter(struct perf_stat_config *config,
 	if (ret)
 		return ret;
 
-	if (config->aggr_mode != AGGR_GLOBAL)
+	if (config->aggr_mode != AGGR_GLOBAL && config->aggr_mode != AGGR_IIO_STACK)
 		return 0;
 
 	if (!counter->snapshot)
diff --git a/tools/perf/util/stat.h b/tools/perf/util/stat.h
index 05adf8165025..d053ebd44ae3 100644
--- a/tools/perf/util/stat.h
+++ b/tools/perf/util/stat.h
@@ -50,6 +50,7 @@ enum aggr_mode {
 	AGGR_DIE,
 	AGGR_CORE,
 	AGGR_THREAD,
+	AGGR_IIO_STACK,
 	AGGR_UNSET,
 	AGGR_NODE,
 };
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v2 2/6] perf evsel: Introduce an observed performance device
  2020-12-23 13:03 [PATCH v2 0/6] perf stat: Introduce iiostat mode to provide I/O performance metrics Alexander Antonov
  2020-12-23 13:03 ` [PATCH v2 1/6] perf stat: Add AGGR_IIO_STACK mode Alexander Antonov
@ 2020-12-23 13:03 ` Alexander Antonov
  2021-01-06  8:44   ` Namhyung Kim
  2020-12-23 13:03 ` [PATCH v2 3/6] perf stat: Basic support for iiostat in perf Alexander Antonov
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 19+ messages in thread
From: Alexander Antonov @ 2020-12-23 13:03 UTC (permalink / raw)
  To: acme
  Cc: linux-kernel, jolsa, ak, alexander.shishkin, mark.rutland,
	namhyung, irogers, mingo, peterz, alexander.antonov

Adding evsel::perf_device void pointer.

For performance monitoring purposes, an evsel can have a related device.
These changes allow to attribute, for example, I/O performance metrics
to IIO stack.

Signed-off-by: Alexander Antonov <alexander.antonov@linux.intel.com>
---
 tools/perf/util/evsel.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 79a860d8e3ee..c346920f477a 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -127,6 +127,7 @@ struct evsel {
 	 * See also evsel__has_callchain().
 	 */
 	__u64			synth_sample_type;
+	void			*perf_device;
 };
 
 struct perf_missing_features {
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v2 3/6] perf stat: Basic support for iiostat in perf
  2020-12-23 13:03 [PATCH v2 0/6] perf stat: Introduce iiostat mode to provide I/O performance metrics Alexander Antonov
  2020-12-23 13:03 ` [PATCH v2 1/6] perf stat: Add AGGR_IIO_STACK mode Alexander Antonov
  2020-12-23 13:03 ` [PATCH v2 2/6] perf evsel: Introduce an observed performance device Alexander Antonov
@ 2020-12-23 13:03 ` Alexander Antonov
  2021-01-06  8:56   ` Namhyung Kim
  2020-12-23 13:03 ` [PATCH v2 4/6] perf stat: Helper functions for IIO stacks list in iiostat mode Alexander Antonov
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 19+ messages in thread
From: Alexander Antonov @ 2020-12-23 13:03 UTC (permalink / raw)
  To: acme
  Cc: linux-kernel, jolsa, ak, alexander.shishkin, mark.rutland,
	namhyung, irogers, mingo, peterz, alexander.antonov

Add basic flow for a new iiostat mode in perf. Mode is intended to
provide four I/O performance metrics per each IIO stack: Inbound Read,
Inbound Write, Outbound Read, Outbound Write.

The actual code to compute the metrics and attribute it to
evsel::perf_device is in follow-on patches.

Signed-off-by: Alexander Antonov <alexander.antonov@linux.intel.com>
---
 tools/perf/builtin-stat.c      | 33 ++++++++++++++++++++++++++++-
 tools/perf/util/iiostat.h      | 33 +++++++++++++++++++++++++++++
 tools/perf/util/stat-display.c | 38 +++++++++++++++++++++++++++++++++-
 tools/perf/util/stat-shadow.c  | 11 +++++++++-
 tools/perf/util/stat.h         |  1 +
 5 files changed, 113 insertions(+), 3 deletions(-)
 create mode 100644 tools/perf/util/iiostat.h

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 72f9d0aa3f96..14c3da136927 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -67,6 +67,7 @@
 #include "util/top.h"
 #include "util/affinity.h"
 #include "util/pfm.h"
+#include "util/iiostat.h"
 #include "asm/bug.h"
 
 #include <linux/time64.h>
@@ -198,7 +199,8 @@ static struct perf_stat_config stat_config = {
 	.walltime_nsecs_stats	= &walltime_nsecs_stats,
 	.big_num		= true,
 	.ctl_fd			= -1,
-	.ctl_fd_ack		= -1
+	.ctl_fd_ack		= -1,
+	.iiostat_run		= false,
 };
 
 static bool cpus_map_matched(struct evsel *a, struct evsel *b)
@@ -1073,6 +1075,14 @@ static int parse_stat_cgroups(const struct option *opt,
 	return parse_cgroups(opt, str, unset);
 }
 
+__weak int iiostat_parse(const struct option *opt __maybe_unused,
+			 const char *str __maybe_unused,
+			 int unset __maybe_unused)
+{
+	pr_err("iiostat mode is not supported\n");
+	return -1;
+}
+
 static struct option stat_options[] = {
 	OPT_BOOLEAN('T', "transaction", &transaction_run,
 		    "hardware transaction statistics"),
@@ -1185,6 +1195,8 @@ static struct option stat_options[] = {
 		     "\t\t\t  Optionally send control command completion ('ack\\n') to ack-fd descriptor.\n"
 		     "\t\t\t  Alternatively, ctl-fifo / ack-fifo will be opened and used as ctl-fd / ack-fd.",
 		      parse_control_option),
+	OPT_CALLBACK_OPTARG(0, "iiostat", &evsel_list, &stat_config, "root port",
+			    "measure PCIe metrics per IIO stack", iiostat_parse),
 	OPT_END()
 };
 
@@ -1509,6 +1521,12 @@ static int perf_stat_init_aggr_mode_file(struct perf_stat *st)
 	return 0;
 }
 
+__weak int iiostat_show_root_ports(struct evlist *evlist __maybe_unused,
+				   struct perf_stat_config *config __maybe_unused)
+{
+	return 0;
+}
+
 /*
  * Add default attributes, if there were no attributes specified or
  * if -d/--detailed, -d -d or -d -d -d is used:
@@ -2054,6 +2072,10 @@ static void setup_system_wide(int forks)
 	}
 }
 
+__weak void iiostat_delete_root_ports(struct evlist *evlist __maybe_unused)
+{
+}
+
 int cmd_stat(int argc, const char **argv)
 {
 	const char * const stat_usage[] = {
@@ -2230,6 +2252,12 @@ int cmd_stat(int argc, const char **argv)
 		goto out;
 	}
 
+	if (stat_config.iiostat_run) {
+		status = iiostat_show_root_ports(evsel_list, &stat_config);
+		if (status || !stat_config.iiostat_run)
+			goto out;
+	}
+
 	if (add_default_attributes())
 		goto out;
 
@@ -2406,6 +2434,9 @@ int cmd_stat(int argc, const char **argv)
 	perf_stat__exit_aggr_mode();
 	perf_evlist__free_stats(evsel_list);
 out:
+	if (stat_config.iiostat_run)
+		iiostat_delete_root_ports(evsel_list);
+
 	zfree(&stat_config.walltime_run);
 
 	if (smi_cost && smi_reset)
diff --git a/tools/perf/util/iiostat.h b/tools/perf/util/iiostat.h
new file mode 100644
index 000000000000..8d4226df9975
--- /dev/null
+++ b/tools/perf/util/iiostat.h
@@ -0,0 +1,33 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * perf iiostat
+ *
+ * Copyright (C) 2020, Intel Corporation
+ *
+ * Authors: Alexander Antonov <alexander.antonov@linux.intel.com>
+ */
+
+#ifndef _IIOSTAT_H
+#define _IIOSTAT_H
+
+#include <subcmd/parse-options.h>
+#include "util/stat.h"
+#include "util/parse-events.h"
+#include "util/evlist.h"
+
+struct option;
+struct perf_stat_config;
+struct evlist;
+struct timespec;
+
+int iiostat_parse(const struct option *opt, const char *str,
+		  int unset __maybe_unused);
+void iiostat_prefix(struct perf_stat_config *config, struct evlist *evlist,
+		    char *prefix, struct timespec *ts);
+void iiostat_print_metric(struct perf_stat_config *config, struct evsel *evsel,
+			  struct perf_stat_output_ctx *out);
+int iiostat_show_root_ports(struct evlist *evlist,
+			    struct perf_stat_config *config);
+void iiostat_delete_root_ports(struct evlist *evlist);
+
+#endif /* _IIOSTAT_H */
diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
index 3bfcdb80443a..9eb8484e8b90 100644
--- a/tools/perf/util/stat-display.c
+++ b/tools/perf/util/stat-display.c
@@ -17,6 +17,7 @@
 #include "cgroup.h"
 #include <api/fs/fs.h>
 #include "util.h"
+#include "iiostat.h"
 
 #define CNTR_NOT_SUPPORTED	"<not supported>"
 #define CNTR_NOT_COUNTED	"<not counted>"
@@ -310,6 +311,12 @@ static void print_metric_header(struct perf_stat_config *config,
 	struct outstate *os = ctx;
 	char tbuf[1024];
 
+	/* In case of iiostat, print metric header for first perf_device only */
+	if (os->evsel->perf_device && os->evsel->evlist->selected->perf_device &&
+	    config->iiostat_run &&
+	    os->evsel->perf_device != os->evsel->evlist->selected->perf_device)
+		return;
+
 	if (!valid_only_metric(unit))
 		return;
 	unit = fixunit(tbuf, os->evsel, unit);
@@ -942,6 +949,8 @@ static void print_metric_headers(struct perf_stat_config *config,
 			fputs("time,", config->output);
 		fputs(aggr_header_csv[config->aggr_mode], config->output);
 	}
+	if (config->iiostat_run && !config->interval && !config->csv_output)
+		fprintf(config->output, "   port         ");
 
 	/* Print metrics headers only */
 	evlist__for_each_entry(evlist, counter) {
@@ -959,6 +968,13 @@ static void print_metric_headers(struct perf_stat_config *config,
 	fputc('\n', config->output);
 }
 
+__weak void iiostat_prefix(struct perf_stat_config *config __maybe_unused,
+			    struct evlist *evlist __maybe_unused,
+			    char *prefix __maybe_unused,
+			    struct timespec *ts __maybe_unused)
+{
+}
+
 static void print_interval(struct perf_stat_config *config,
 			   struct evlist *evlist,
 			   char *prefix, struct timespec *ts)
@@ -971,7 +987,8 @@ static void print_interval(struct perf_stat_config *config,
 	if (config->interval_clear)
 		puts(CONSOLE_CLEAR);
 
-	sprintf(prefix, "%6lu.%09lu%s", ts->tv_sec, ts->tv_nsec, config->csv_sep);
+	if (!config->iiostat_run)
+		sprintf(prefix, "%6lu.%09lu%s", ts->tv_sec, ts->tv_nsec, config->csv_sep);
 
 	if ((num_print_interval == 0 && !config->csv_output) || config->interval_clear) {
 		switch (config->aggr_mode) {
@@ -1205,6 +1222,10 @@ perf_evlist__print_counters(struct evlist *evlist,
 	int interval = config->interval;
 	struct evsel *counter;
 	char buf[64], *prefix = NULL;
+	void *device = NULL;
+
+	if (config->iiostat_run)
+		evlist->selected = evlist__first(evlist);
 
 	if (interval)
 		print_interval(config, evlist, prefix = buf, ts);
@@ -1254,6 +1275,21 @@ perf_evlist__print_counters(struct evlist *evlist,
 		}
 		break;
 	case AGGR_IIO_STACK:
+		counter = evlist__first(evlist);
+		perf_evlist__set_selected(evlist, counter);
+		iiostat_prefix(config, evlist, prefix = buf, ts);
+		fprintf(config->output, "%s", prefix);
+		evlist__for_each_entry(evlist, counter) {
+			device = evlist->selected->perf_device;
+			if (device && device != counter->perf_device) {
+				perf_evlist__set_selected(evlist, counter);
+				iiostat_prefix(config, evlist, prefix, ts);
+				fprintf(config->output, "\n%s", prefix);
+			}
+			print_counter_aggr(config, counter, prefix);
+			if ((counter->idx + 1) == evlist->core.nr_entries)
+				fputc('\n', config->output);
+		}
 		break;
 	case AGGR_UNSET:
 	default:
diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
index 901265127e36..c5851ceb4c6b 100644
--- a/tools/perf/util/stat-shadow.c
+++ b/tools/perf/util/stat-shadow.c
@@ -9,6 +9,7 @@
 #include "expr.h"
 #include "metricgroup.h"
 #include <linux/zalloc.h>
+#include "iiostat.h"
 
 /*
  * AGGR_GLOBAL: Use CPU 0
@@ -919,6 +920,12 @@ double test_generic_metric(struct metric_expr *mexp, int cpu, struct runtime_sta
 	return ratio;
 }
 
+__weak void iiostat_print_metric(struct perf_stat_config *config __maybe_unused,
+				  struct evsel *evsel __maybe_unused,
+				  struct perf_stat_output_ctx *out __maybe_unused)
+{
+}
+
 void perf_stat__print_shadow_stats(struct perf_stat_config *config,
 				   struct evsel *evsel,
 				   double avg, int cpu,
@@ -934,7 +941,9 @@ void perf_stat__print_shadow_stats(struct perf_stat_config *config,
 	struct metric_event *me;
 	int num = 1;
 
-	if (evsel__match(evsel, HARDWARE, HW_INSTRUCTIONS)) {
+	if (config->iiostat_run) {
+		iiostat_print_metric(config, evsel, out);
+	} else if (evsel__match(evsel, HARDWARE, HW_INSTRUCTIONS)) {
 		total = runtime_stat_avg(st, STAT_CYCLES, ctx, cpu);
 
 		if (total) {
diff --git a/tools/perf/util/stat.h b/tools/perf/util/stat.h
index d053ebd44ae3..a072dfe3dbbf 100644
--- a/tools/perf/util/stat.h
+++ b/tools/perf/util/stat.h
@@ -118,6 +118,7 @@ struct perf_stat_config {
 	bool			 walltime_run_table;
 	bool			 all_kernel;
 	bool			 all_user;
+	bool			 iiostat_run;
 	bool			 percore_show_thread;
 	bool			 summary;
 	bool			 metric_no_group;
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v2 4/6] perf stat: Helper functions for IIO stacks list in iiostat mode
  2020-12-23 13:03 [PATCH v2 0/6] perf stat: Introduce iiostat mode to provide I/O performance metrics Alexander Antonov
                   ` (2 preceding siblings ...)
  2020-12-23 13:03 ` [PATCH v2 3/6] perf stat: Basic support for iiostat in perf Alexander Antonov
@ 2020-12-23 13:03 ` Alexander Antonov
  2020-12-23 13:03 ` [PATCH v2 5/6] perf stat: Enable iiostat mode for x86 platforms Alexander Antonov
  2020-12-23 13:03 ` [PATCH v2 6/6] perf: Update .gitignore file Alexander Antonov
  5 siblings, 0 replies; 19+ messages in thread
From: Alexander Antonov @ 2020-12-23 13:03 UTC (permalink / raw)
  To: acme
  Cc: linux-kernel, jolsa, ak, alexander.shishkin, mark.rutland,
	namhyung, irogers, mingo, peterz, alexander.antonov

Introduce helper functions to control IIO stacks list.
These helpers will be used in the follow-up patch.

Signed-off-by: Alexander Antonov <alexander.antonov@linux.intel.com>
---
 tools/perf/arch/x86/util/iiostat.c | 125 +++++++++++++++++++++++++++++
 1 file changed, 125 insertions(+)
 create mode 100644 tools/perf/arch/x86/util/iiostat.c

diff --git a/tools/perf/arch/x86/util/iiostat.c b/tools/perf/arch/x86/util/iiostat.c
new file mode 100644
index 000000000000..98b9707b4827
--- /dev/null
+++ b/tools/perf/arch/x86/util/iiostat.c
@@ -0,0 +1,125 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * perf iiostat
+ *
+ * Copyright (C) 2020, Intel Corporation
+ *
+ * Authors: Alexander Antonov <alexander.antonov@linux.intel.com>
+ */
+
+#include <api/fs/fs.h>
+#include <linux/kernel.h>
+#include <linux/err.h>
+#include <limits.h>
+#include <stdio.h>
+#include <string.h>
+#include <errno.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <dirent.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <regex.h>
+#include "util/cpumap.h"
+#include "util/debug.h"
+#include "util/iiostat.h"
+#include "util/counts.h"
+#include "path.h"
+
+struct iio_root_port {
+	u32 domain;
+	u8 bus;
+	u8 die;
+	u8 pmu_idx;
+	int idx;
+};
+
+struct iio_root_ports_list {
+	struct iio_root_port **rps;
+	int nr_entries;
+};
+
+static void iio_root_port_show(FILE *output, const struct iio_root_port * const rp)
+{
+	if (output && rp)
+		fprintf(output, "S%d-uncore_iio_%d<%04x:%02x>\n",
+			rp->die, rp->pmu_idx, rp->domain, rp->bus);
+}
+
+static struct iio_root_port *iio_root_port_new(u32 domain, u8 bus, u8 die, u8 pmu_idx)
+{
+	struct iio_root_port *p = calloc(1, sizeof(*p));
+
+	if (p) {
+		p->domain = domain;
+		p->bus = bus;
+		p->die = die;
+		p->pmu_idx = pmu_idx;
+	}
+	return p;
+}
+
+static struct iio_root_ports_list *iio_root_ports_list_new(void)
+{
+	struct iio_root_ports_list *list = calloc(1, sizeof(*list));
+
+	if (list) {
+		list->rps = calloc(1, sizeof(struct iio_root_port *));
+		if (!list->rps) {
+			free(list);
+			list = NULL;
+		}
+	}
+
+	return list;
+}
+
+static void iio_root_ports_list_free(struct iio_root_ports_list *list)
+{
+	int idx;
+
+	if (list) {
+		for (idx = 0; idx < list->nr_entries; idx++)
+			free(list->rps[idx]);
+		free(list->rps);
+		free(list);
+	}
+}
+
+static
+struct iio_root_port *iio_root_port_find_by_notation(const struct iio_root_ports_list * const list,
+						     u32 domain, u8 bus)
+{
+	int idx;
+	struct iio_root_port *rp;
+
+	if (list) {
+		for (idx = 0; idx < list->nr_entries; idx++) {
+			rp = list->rps[idx];
+			if (rp && rp->domain == domain && rp->bus == bus)
+				return rp;
+		}
+	}
+	return NULL;
+}
+
+static int iio_root_ports_list_insert(struct iio_root_ports_list *list,
+				      struct iio_root_port * const rp)
+{
+	struct iio_root_port **tmp_buf;
+
+	if (list && rp) {
+		rp->idx = list->nr_entries++;
+		/* One more for NULL.*/
+		tmp_buf = realloc(list->rps, (list->nr_entries + 1) * sizeof(*list->rps));
+		if (!tmp_buf) {
+			pr_err("Failed to realloc memory\n");
+			return -ENOMEM;
+		}
+		tmp_buf[rp->idx] = rp;
+		tmp_buf[list->nr_entries] = NULL;
+		list->rps = tmp_buf;
+	}
+	return 0;
+}
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v2 5/6] perf stat: Enable iiostat mode for x86 platforms
  2020-12-23 13:03 [PATCH v2 0/6] perf stat: Introduce iiostat mode to provide I/O performance metrics Alexander Antonov
                   ` (3 preceding siblings ...)
  2020-12-23 13:03 ` [PATCH v2 4/6] perf stat: Helper functions for IIO stacks list in iiostat mode Alexander Antonov
@ 2020-12-23 13:03 ` Alexander Antonov
  2021-01-06  9:02   ` Namhyung Kim
  2020-12-23 13:03 ` [PATCH v2 6/6] perf: Update .gitignore file Alexander Antonov
  5 siblings, 1 reply; 19+ messages in thread
From: Alexander Antonov @ 2020-12-23 13:03 UTC (permalink / raw)
  To: acme
  Cc: linux-kernel, jolsa, ak, alexander.shishkin, mark.rutland,
	namhyung, irogers, mingo, peterz, alexander.antonov

This functionality is based on recently introduced sysfs attributes
for Intel® Xeon® Scalable processor family (code name Skylake-SP):
Commit bb42b3d39781 ("perf/x86/intel/uncore: Expose an Uncore unit to
IIO PMON mapping")

Mode is intended to provide four I/O performance metrics in MB per each
IIO stack:
 - Inbound Read: I/O devices below IIO stack read from the host memory
 - Inbound Write: I/O devices below IIO stack write to the host memory
 - Outbound Read: CPU reads from I/O devices below IIO stack
 - Outbound Write: CPU writes to I/O devices below IIO stack

Each metric requiries only one IIO event which increments at every 4B
transfer in corresponding direction. The formulas to compute metrics
are generic:
    #EventCount * 4B / (1024 * 1024)

Signed-off-by: Alexander Antonov <alexander.antonov@linux.intel.com>
---
 tools/perf/Documentation/perf-iiostat.txt |  89 ++++++
 tools/perf/Makefile.perf                  |   5 +-
 tools/perf/arch/x86/util/Build            |   1 +
 tools/perf/arch/x86/util/iiostat.c        | 337 ++++++++++++++++++++++
 tools/perf/command-list.txt               |   1 +
 tools/perf/perf-iiostat.sh                |  12 +
 6 files changed, 444 insertions(+), 1 deletion(-)
 create mode 100644 tools/perf/Documentation/perf-iiostat.txt
 create mode 100644 tools/perf/perf-iiostat.sh

diff --git a/tools/perf/Documentation/perf-iiostat.txt b/tools/perf/Documentation/perf-iiostat.txt
new file mode 100644
index 000000000000..38b5697b0d85
--- /dev/null
+++ b/tools/perf/Documentation/perf-iiostat.txt
@@ -0,0 +1,89 @@
+perf-iiostat(1)
+===============
+
+NAME
+----
+perf-iiostat - Show I/O performance metrics
+
+SYNOPSIS
+--------
+[verse]
+'perf iiostat' show
+'perf iiostat' <ports> -- <command> [<options>]
+
+DESCRIPTION
+-----------
+Mode is intended to provide four I/O performance metrics per each IIO
+stack (PCIe root port):
+
+- Inbound Read   - I/O devices below IIO stack read from the host memory, in MB
+
+- Inbound Write  - I/O devices below IIO stack write to the host memory, in MB
+
+- Outbound Read  - CPU reads from I/O devices below IIO stack, in MB
+
+- Outbound Write - CPU writes to I/O devices below IIO stack, in MB
+
+OPTIONS
+-------
+<command>...::
+	Any command you can specify in a shell.
+
+show::
+	List all IIO stacks.
+
+<ports>::
+	Select the root ports for monitoring. Comma-separated list is supported.
+
+EXAMPLES
+--------
+
+1. List all IIO stacks (example for 2-S platform):
+
+   $ perf iiostat show
+   S0-uncore_iio_0<0000:00>
+   S1-uncore_iio_0<0000:80>
+   S0-uncore_iio_1<0000:17>
+   S1-uncore_iio_1<0000:85>
+   S0-uncore_iio_2<0000:3a>
+   S1-uncore_iio_2<0000:ae>
+   S0-uncore_iio_3<0000:5d>
+   S1-uncore_iio_3<0000:d7>
+
+2. Collect metrics for all I/O stacks:
+
+   $ perf iiostat -- dd if=/dev/zero of=/dev/nvme0n1 bs=1M oflag=direct
+   357708+0 records in
+   357707+0 records out
+   375083606016 bytes (375 GB, 349 GiB) copied, 215.974 s, 1.7 GB/s
+
+    Performance counter stats for 'system wide':
+
+      port             Inbound Read(MB)    Inbound Write(MB)    Outbound Read(MB)   Outbound Write(MB) 
+   0000:00                    1                    0                    2                    3 
+   0000:80                    0                    0                    0                    0 
+   0000:17               352552                   43                    0                   21 
+   0000:85                    0                    0                    0                    0 
+   0000:3a                    3                    0                    0                    0 
+   0000:ae                    0                    0                    0                    0 
+   0000:5d                    0                    0                    0                    0 
+   0000:d7                    0                    0                    0                    0
+
+3. Collect metrics for comma-separated list of I/O stacks:
+
+   $ perf iiostat 0000:17,0:3a -- dd if=/dev/zero of=/dev/nvme0n1 bs=1M oflag=direct
+   357708+0 records in
+   357707+0 records out
+   375083606016 bytes (375 GB, 349 GiB) copied, 197.08 s, 1.9 GB/s
+
+    Performance counter stats for 'system wide':
+
+      port             Inbound Read(MB)    Inbound Write(MB)    Outbound Read(MB)   Outbound Write(MB) 
+   0000:17               358559                   44                    0                   22 
+   0000:3a                    3                    2                    0                    0 
+
+        197.081983474 seconds time elapsed
+
+SEE ALSO
+--------
+linkperf:perf-stat[1]
\ No newline at end of file
diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
index 7ce3f2e8b9c7..c16c14a304a9 100644
--- a/tools/perf/Makefile.perf
+++ b/tools/perf/Makefile.perf
@@ -280,6 +280,7 @@ SCRIPT_SH =
 
 SCRIPT_SH += perf-archive.sh
 SCRIPT_SH += perf-with-kcore.sh
+SCRIPT_SH += perf-iiostat.sh
 
 grep-libs = $(filter -l%,$(1))
 strip-libs = $(filter-out -l%,$(1))
@@ -944,6 +945,8 @@ endif
 		$(INSTALL) $(OUTPUT)perf-archive -t '$(DESTDIR_SQ)$(perfexec_instdir_SQ)'
 	$(call QUIET_INSTALL, perf-with-kcore) \
 		$(INSTALL) $(OUTPUT)perf-with-kcore -t '$(DESTDIR_SQ)$(perfexec_instdir_SQ)'
+	$(call QUIET_INSTALL, perf-iiostat) \
+		$(INSTALL) $(OUTPUT)perf-iiostat -t '$(DESTDIR_SQ)$(perfexec_instdir_SQ)'
 ifndef NO_LIBAUDIT
 	$(call QUIET_INSTALL, strace/groups) \
 		$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(STRACE_GROUPS_INSTDIR_SQ)'; \
@@ -1009,7 +1012,7 @@ python-clean:
 	$(python-clean)
 
 clean:: $(LIBTRACEEVENT)-clean $(LIBAPI)-clean $(LIBBPF)-clean $(LIBSUBCMD)-clean $(LIBPERF)-clean config-clean fixdep-clean python-clean
-	$(call QUIET_CLEAN, core-objs)  $(RM) $(LIBPERF_A) $(OUTPUT)perf-archive $(OUTPUT)perf-with-kcore $(LANG_BINDINGS)
+	$(call QUIET_CLEAN, core-objs)  $(RM) $(LIBPERF_A) $(OUTPUT)perf-archive $(OUTPUT)perf-with-kcore $(OUTPUT)perf-iiostat $(LANG_BINDINGS)
 	$(Q)find $(if $(OUTPUT),$(OUTPUT),.) -name '*.o' -delete -o -name '\.*.cmd' -delete -o -name '\.*.d' -delete
 	$(Q)$(RM) $(OUTPUT).config-detected
 	$(call QUIET_CLEAN, core-progs) $(RM) $(ALL_PROGRAMS) perf perf-read-vdso32 perf-read-vdsox32 $(OUTPUT)pmu-events/jevents $(OUTPUT)$(LIBJVMTI).so
diff --git a/tools/perf/arch/x86/util/Build b/tools/perf/arch/x86/util/Build
index 347c39b960eb..6fa275d3d897 100644
--- a/tools/perf/arch/x86/util/Build
+++ b/tools/perf/arch/x86/util/Build
@@ -6,6 +6,7 @@ perf-y += perf_regs.o
 perf-y += topdown.o
 perf-y += machine.o
 perf-y += event.o
+perf-y += iiostat.o
 
 perf-$(CONFIG_DWARF) += dwarf-regs.o
 perf-$(CONFIG_BPF_PROLOGUE) += dwarf-regs.o
diff --git a/tools/perf/arch/x86/util/iiostat.c b/tools/perf/arch/x86/util/iiostat.c
index 98b9707b4827..de8690dbb8b0 100644
--- a/tools/perf/arch/x86/util/iiostat.c
+++ b/tools/perf/arch/x86/util/iiostat.c
@@ -27,6 +27,44 @@
 #include "util/counts.h"
 #include "path.h"
 
+#ifndef MAX_PATH
+#define MAX_PATH 1024
+#endif
+
+#define UNCORE_IIO_PMU_PATH	"devices/uncore_iio_%d"
+#define SYSFS_UNCORE_PMU_PATH	"%s/"UNCORE_IIO_PMU_PATH
+#define PLATFORM_MAPPING_PATH	UNCORE_IIO_PMU_PATH"/die%d"
+
+enum iiostat_mode_t {
+	IIOSTAT_NONE		= -1,
+	IIOSTAT_RUN		= 0,
+	IIOSTAT_SHOW		= 1
+};
+
+static enum iiostat_mode_t iiostat_mode = IIOSTAT_NONE;
+
+/*
+ * Each metric requiries only one IIO event which increments at every 4B transfer
+ * in corresponding direction. The formulas to compute metrics are generic:
+ *     #EventCount * 4B / (1024 * 1024)
+ */
+static const char * const iiostat_metrics[] = {
+	"Inbound Read(MB)",
+	"Inbound Write(MB)",
+	"Outbound Read(MB)",
+	"Outbound Write(MB)",
+};
+
+static inline int iiostat_metrics_count(void)
+{
+	return sizeof(iiostat_metrics) / sizeof(char *);
+}
+
+static const char *iiostat_metric_by_idx(int idx)
+{
+	return *(iiostat_metrics + idx % iiostat_metrics_count());
+}
+
 struct iio_root_port {
 	u32 domain;
 	u8 bus;
@@ -123,3 +161,302 @@ static int iio_root_ports_list_insert(struct iio_root_ports_list *list,
 	}
 	return 0;
 }
+
+static int uncore_pmu_iio_platform_mapping(u8 pmu_idx, struct iio_root_ports_list * const list)
+{
+	char *buf;
+	char path[MAX_PATH];
+	u32 domain;
+	u8 bus;
+	struct iio_root_port *rp;
+	size_t size;
+	int ret;
+
+	for (int die = 0; die < cpu__max_node(); die++) {
+		scnprintf(path, MAX_PATH, PLATFORM_MAPPING_PATH, pmu_idx, die);
+		if (sysfs__read_str(path, &buf, &size) < 0) {
+			if (pmu_idx)
+				goto out;
+			pr_err("Mode iiostat is not supported\n");
+			return -1;
+		}
+		ret = sscanf(buf, "%04x:%02hhx", &domain, &bus);
+		free(buf);
+		if (ret != 2) {
+			pr_err("Invalid mapping data: iio_%d; die%d\n", pmu_idx, die);
+			return -1;
+		}
+		rp = iio_root_port_new(domain, bus, die, pmu_idx);
+		if (!rp || iio_root_ports_list_insert(list, rp)) {
+			free(rp);
+			return -ENOMEM;
+		}
+	}
+out:
+	return 0;
+}
+
+static u8 iio_pmu_count(void)
+{
+	u8 pmu_idx = 0;
+	char path[MAX_PATH];
+	const char *sysfs = sysfs__mountpoint();
+
+	if (sysfs) {
+		for (;; pmu_idx++) {
+			snprintf(path, sizeof(path), SYSFS_UNCORE_PMU_PATH,
+				 sysfs, pmu_idx);
+			if (access(path, F_OK) != 0)
+				break;
+		}
+	}
+	return pmu_idx;
+}
+
+static int iio_root_ports_scan(struct iio_root_ports_list **list)
+{
+	int ret = -ENOMEM;
+	struct iio_root_ports_list *tmp_list;
+	u8 pmu_count = iio_pmu_count();
+
+	if (!pmu_count) {
+		pr_err("Unsupported uncore pmu configuration\n");
+		return -1;
+	}
+
+	tmp_list = iio_root_ports_list_new();
+	if (!tmp_list)
+		goto err;
+
+	for (u8 pmu_idx = 0; pmu_idx < pmu_count; pmu_idx++) {
+		ret = uncore_pmu_iio_platform_mapping(pmu_idx, tmp_list);
+		if (ret)
+			break;
+	}
+err:
+	if (!ret)
+		*list = tmp_list;
+	else
+		iio_root_ports_list_free(tmp_list);
+
+	return ret;
+}
+
+static int iio_root_port_parse_str(u32 *domain, u8 *bus, char *str)
+{
+	int ret;
+	regex_t regex;
+	/*
+	 * Expected format domain:bus:
+	 * Valid domain range [0:ffff]
+	 * Valid bus range [0:ff]
+	 * Example: 0000:af, 0:3d, 01:7
+	 */
+	regcomp(&regex, "^([a-f0-9A-F]{1,}):([a-f0-9A-F]{1,2})$", REG_EXTENDED);
+	ret = regexec(&regex, str, 0, NULL, 0);
+	if (ret || sscanf(str, "%08x:%02hhx", domain, bus) != 2)
+		pr_warning("Unrecognized root port format: %s\n"
+			   "Please use the following format:\n"
+			   "\t [domain]:[bus]\n"
+			   "\t for example: 0000:3d\n", str);
+
+	regfree(&regex);
+	return ret;
+}
+
+static int iio_root_ports_list_filter(struct iio_root_ports_list **list,
+				      const char *filter)
+{
+	char *tok, *tmp, *filter_copy = NULL;
+	struct iio_root_port *rp;
+	u32 domain;
+	u8 bus;
+	int ret = -ENOMEM;
+	struct iio_root_ports_list *tmp_list = iio_root_ports_list_new();
+
+	if (!tmp_list)
+		goto err;
+
+	filter_copy = strdup(filter);
+	if (!filter_copy)
+		goto err;
+
+	for (tok = strtok_r(filter_copy, ",", &tmp); tok; tok = strtok_r(NULL, ",", &tmp)) {
+		if (!iio_root_port_parse_str(&domain, &bus, tok)) {
+			rp = iio_root_port_find_by_notation(*list, domain, bus);
+			if (rp) {
+				(*list)->rps[rp->idx] = NULL;
+				ret = iio_root_ports_list_insert(tmp_list, rp);
+				if (ret) {
+					free(rp);
+					goto err;
+				}
+			} else if (!iio_root_port_find_by_notation(tmp_list, domain, bus))
+				pr_warning("Root port %04x:%02x were not found\n", domain, bus);
+		}
+	}
+
+	if (tmp_list->nr_entries == 0) {
+		pr_err("Requested root ports were not found\n");
+		ret = -EINVAL;
+	}
+err:
+	iio_root_ports_list_free(*list);
+	if (ret)
+		iio_root_ports_list_free(tmp_list);
+	else
+		*list = tmp_list;
+
+	free(filter_copy);
+	return ret;
+}
+
+static int iiostat_event_group(struct evlist *evl, struct iio_root_ports_list *list)
+{
+	int ret;
+	struct iio_root_port **rp;
+	const char *iiostat_cmd_template =
+		"{uncore_iio_%x/event=0x83,umask=0x04,ch_mask=0xF,fc_mask=0x07/,\
+		uncore_iio_%x/event=0x83,umask=0x01,ch_mask=0xF,fc_mask=0x07/,\
+		uncore_iio_%x/event=0xc0,umask=0x04,ch_mask=0xF,fc_mask=0x07/,\
+		uncore_iio_%x/event=0xc0,umask=0x01,ch_mask=0xF,fc_mask=0x07/}";
+	const int len_template = strlen(iiostat_cmd_template) + 1;
+	struct evsel *evsel = NULL;
+	int metrics_count = iiostat_metrics_count();
+	char *iiostat_cmd = calloc(len_template, 1);
+
+	if (!iiostat_cmd)
+		return -ENOMEM;
+
+	for (rp = list->rps; *rp; rp++) {
+		sprintf(iiostat_cmd, iiostat_cmd_template,
+			(*rp)->pmu_idx, (*rp)->pmu_idx, (*rp)->pmu_idx, (*rp)->pmu_idx);
+		ret = parse_events(evl, iiostat_cmd, NULL);
+		if (ret)
+			goto err;
+	}
+
+	evlist__for_each_entry(evl, evsel) {
+		evsel->perf_device = list->rps[evsel->idx / metrics_count];
+	}
+	list->nr_entries = 0;
+err:
+	iio_root_ports_list_free(list);
+	free(iiostat_cmd);
+	return ret;
+}
+
+int iiostat_parse(const struct option *opt, const char *str,
+		  int unset __maybe_unused)
+{
+	int ret;
+	struct iio_root_ports_list *list;
+	struct evlist *evl = *(struct evlist **)opt->value;
+	struct perf_stat_config *config = (struct perf_stat_config *)opt->data;
+
+	if (evl->core.nr_entries > 0) {
+		pr_err("Unsupported event configuration\n");
+		return -1;
+	}
+	config->metric_only = true;
+	config->aggr_mode = AGGR_IIO_STACK;
+	config->iiostat_run = true;
+	ret = iio_root_ports_scan(&list);
+	if (ret)
+		return ret;
+
+	if (!str) {
+		iiostat_mode = IIOSTAT_RUN;
+	} else if (!strcmp(str, "show")) {
+		iiostat_mode = IIOSTAT_SHOW;
+	} else {
+		iiostat_mode = IIOSTAT_RUN;
+		ret = iio_root_ports_list_filter(&list, str);
+		if (ret)
+			return ret;
+	}
+	return iiostat_event_group(evl, list);
+}
+
+void iiostat_prefix(struct perf_stat_config *config,
+		    struct evlist *evlist,
+		    char *prefix, struct timespec *ts)
+{
+	struct iio_root_port *rp = evlist->selected->perf_device;
+
+	if (rp) {
+		if (ts)
+			sprintf(prefix, "%6lu.%09lu%s%04x:%02x%s",
+					ts->tv_sec, ts->tv_nsec,
+					config->csv_sep, rp->domain, rp->bus,
+					config->csv_sep);
+		else
+			sprintf(prefix, "%04x:%02x%s", rp->domain, rp->bus,
+				config->csv_sep);
+	}
+}
+
+void iiostat_print_metric(struct perf_stat_config *config, struct evsel *evsel,
+			  struct perf_stat_output_ctx *out)
+{
+	double iiostat_value = 0;
+	u64 prev_count_val = 0;
+	const char *iiostat_metric = iiostat_metric_by_idx(evsel->idx);
+	u8 die = ((struct iio_root_port *)evsel->perf_device)->die;
+	struct perf_counts_values *count = perf_counts(evsel->counts, die, 0);
+
+	if (count->run && count->ena) {
+		if (evsel->prev_raw_counts && !out->force_header) {
+			struct perf_counts_values *prev_count = perf_counts(evsel->prev_raw_counts, die, 0);
+
+			prev_count_val = prev_count->val;
+			prev_count->val = count->val;
+		}
+		iiostat_value = (count->val - prev_count_val) / ((double) count->run / count->ena);
+	}
+	out->print_metric(config, out->ctx, NULL, "%8.0f", iiostat_metric,
+			  iiostat_value / (256 * 1024));
+}
+
+int iiostat_show_root_ports(struct evlist *evlist, struct perf_stat_config *config)
+{
+	struct evsel *evsel;
+	struct iio_root_port *rp = NULL;
+	bool is_show_mode = (iiostat_mode == IIOSTAT_SHOW);
+
+	if (config->aggr_mode != AGGR_IIO_STACK) {
+		pr_err("Unsupported event configuration\n");
+		return -1;
+	}
+
+	if (is_show_mode || verbose) {
+		evlist__for_each_entry(evlist, evsel) {
+			if (!evsel->perf_device) {
+				pr_err("Unsupported event configuration\n");
+				return -1;
+			}
+			if (rp != evsel->perf_device) {
+				rp = evsel->perf_device;
+				iio_root_port_show(config->output, rp);
+			}
+		}
+	}
+	/* Stop iiostat in show mode*/
+	config->iiostat_run = !is_show_mode;
+	if (is_show_mode)
+		iiostat_delete_root_ports(evlist);
+	return 0;
+}
+
+void iiostat_delete_root_ports(struct evlist *evlist)
+{
+	struct evsel *evsel;
+	struct iio_root_port *rp = NULL;
+
+	evlist__for_each_entry(evlist, evsel) {
+		if (rp != evsel->perf_device) {
+			rp = evsel->perf_device;
+			free(evsel->perf_device);
+		}
+	}
+}
diff --git a/tools/perf/command-list.txt b/tools/perf/command-list.txt
index bc6c585f74fc..77e4c9410b0e 100644
--- a/tools/perf/command-list.txt
+++ b/tools/perf/command-list.txt
@@ -14,6 +14,7 @@ perf-config			mainporcelain common
 perf-evlist			mainporcelain common
 perf-ftrace			mainporcelain common
 perf-inject			mainporcelain common
+perf-iiostat			mainporcelain common
 perf-kallsyms			mainporcelain common
 perf-kmem			mainporcelain common
 perf-kvm			mainporcelain common
diff --git a/tools/perf/perf-iiostat.sh b/tools/perf/perf-iiostat.sh
new file mode 100644
index 000000000000..2c5168d2550b
--- /dev/null
+++ b/tools/perf/perf-iiostat.sh
@@ -0,0 +1,12 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# perf iiostat
+# Alexander Antonov <alexander.antonov@linux.intel.com>
+
+if [[ "$1" == "show" ]] || [[ "$1" =~ ([a-f0-9A-F]{1,}):([a-f0-9A-F]{1,2})(,)? ]]; then
+        DELIMITER="="
+else
+        DELIMITER=" "
+fi
+
+perf stat --iiostat$DELIMITER$*
\ No newline at end of file
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v2 6/6] perf: Update .gitignore file
  2020-12-23 13:03 [PATCH v2 0/6] perf stat: Introduce iiostat mode to provide I/O performance metrics Alexander Antonov
                   ` (4 preceding siblings ...)
  2020-12-23 13:03 ` [PATCH v2 5/6] perf stat: Enable iiostat mode for x86 platforms Alexander Antonov
@ 2020-12-23 13:03 ` Alexander Antonov
  5 siblings, 0 replies; 19+ messages in thread
From: Alexander Antonov @ 2020-12-23 13:03 UTC (permalink / raw)
  To: acme
  Cc: linux-kernel, jolsa, ak, alexander.shishkin, mark.rutland,
	namhyung, irogers, mingo, peterz, alexander.antonov

After a "make -C tools/perf", git reports the following untracked file:
perf-iiostat

Add this generated file to perf's .gitignore file.

Signed-off-by: Alexander Antonov <alexander.antonov@linux.intel.com>
---
 tools/perf/.gitignore | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/.gitignore b/tools/perf/.gitignore
index f3f84781fd74..ab826736e677 100644
--- a/tools/perf/.gitignore
+++ b/tools/perf/.gitignore
@@ -20,6 +20,7 @@ perf.data.old
 output.svg
 perf-archive
 perf-with-kcore
+perf-iiostat
 tags
 TAGS
 cscope*
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 2/6] perf evsel: Introduce an observed performance device
  2020-12-23 13:03 ` [PATCH v2 2/6] perf evsel: Introduce an observed performance device Alexander Antonov
@ 2021-01-06  8:44   ` Namhyung Kim
  2021-01-13 11:13     ` Alexander Antonov
  0 siblings, 1 reply; 19+ messages in thread
From: Namhyung Kim @ 2021-01-06  8:44 UTC (permalink / raw)
  To: Alexander Antonov
  Cc: Arnaldo Carvalho de Melo, linux-kernel, Jiri Olsa, Andi Kleen,
	Alexander Shishkin, Mark Rutland, Ian Rogers, Ingo Molnar,
	Peter Zijlstra

Hi,

On Wed, Dec 23, 2020 at 10:03 PM Alexander Antonov
<alexander.antonov@linux.intel.com> wrote:
>
> Adding evsel::perf_device void pointer.
>
> For performance monitoring purposes, an evsel can have a related device.
> These changes allow to attribute, for example, I/O performance metrics
> to IIO stack.
>
> Signed-off-by: Alexander Antonov <alexander.antonov@linux.intel.com>
> ---
>  tools/perf/util/evsel.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> index 79a860d8e3ee..c346920f477a 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -127,6 +127,7 @@ struct evsel {
>          * See also evsel__has_callchain().
>          */
>         __u64                   synth_sample_type;
> +       void                    *perf_device;

Maybe we can use the existing 'priv' field.

Thanks,
Namhyung


>  };
>
>  struct perf_missing_features {
> --
> 2.19.1
>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 3/6] perf stat: Basic support for iiostat in perf
  2020-12-23 13:03 ` [PATCH v2 3/6] perf stat: Basic support for iiostat in perf Alexander Antonov
@ 2021-01-06  8:56   ` Namhyung Kim
  2021-01-13 11:34     ` Alexander Antonov
  0 siblings, 1 reply; 19+ messages in thread
From: Namhyung Kim @ 2021-01-06  8:56 UTC (permalink / raw)
  To: Alexander Antonov
  Cc: Arnaldo Carvalho de Melo, linux-kernel, Jiri Olsa, Andi Kleen,
	Alexander Shishkin, Mark Rutland, Ian Rogers, Ingo Molnar,
	Peter Zijlstra

On Wed, Dec 23, 2020 at 10:03 PM Alexander Antonov
<alexander.antonov@linux.intel.com> wrote:
>
> Add basic flow for a new iiostat mode in perf. Mode is intended to
> provide four I/O performance metrics per each IIO stack: Inbound Read,
> Inbound Write, Outbound Read, Outbound Write.

It seems like a generic analysis and other archs can extend it later..
Then we can make it a bit more general.. at least, names? :)

>
> The actual code to compute the metrics and attribute it to
> evsel::perf_device is in follow-on patches.
>
> Signed-off-by: Alexander Antonov <alexander.antonov@linux.intel.com>
> ---
>  tools/perf/builtin-stat.c      | 33 ++++++++++++++++++++++++++++-
>  tools/perf/util/iiostat.h      | 33 +++++++++++++++++++++++++++++
>  tools/perf/util/stat-display.c | 38 +++++++++++++++++++++++++++++++++-
>  tools/perf/util/stat-shadow.c  | 11 +++++++++-
>  tools/perf/util/stat.h         |  1 +
>  5 files changed, 113 insertions(+), 3 deletions(-)
>  create mode 100644 tools/perf/util/iiostat.h
>
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 72f9d0aa3f96..14c3da136927 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -67,6 +67,7 @@
>  #include "util/top.h"
>  #include "util/affinity.h"
>  #include "util/pfm.h"
> +#include "util/iiostat.h"
>  #include "asm/bug.h"
>
>  #include <linux/time64.h>
> @@ -198,7 +199,8 @@ static struct perf_stat_config stat_config = {
>         .walltime_nsecs_stats   = &walltime_nsecs_stats,
>         .big_num                = true,
>         .ctl_fd                 = -1,
> -       .ctl_fd_ack             = -1
> +       .ctl_fd_ack             = -1,
> +       .iiostat_run            = false,
>  };
>
>  static bool cpus_map_matched(struct evsel *a, struct evsel *b)
> @@ -1073,6 +1075,14 @@ static int parse_stat_cgroups(const struct option *opt,
>         return parse_cgroups(opt, str, unset);
>  }
>
> +__weak int iiostat_parse(const struct option *opt __maybe_unused,
> +                        const char *str __maybe_unused,
> +                        int unset __maybe_unused)
> +{
> +       pr_err("iiostat mode is not supported\n");
> +       return -1;
> +}
> +
>  static struct option stat_options[] = {
>         OPT_BOOLEAN('T', "transaction", &transaction_run,
>                     "hardware transaction statistics"),
> @@ -1185,6 +1195,8 @@ static struct option stat_options[] = {
>                      "\t\t\t  Optionally send control command completion ('ack\\n') to ack-fd descriptor.\n"
>                      "\t\t\t  Alternatively, ctl-fifo / ack-fifo will be opened and used as ctl-fd / ack-fd.",
>                       parse_control_option),
> +       OPT_CALLBACK_OPTARG(0, "iiostat", &evsel_list, &stat_config, "root port",
> +                           "measure PCIe metrics per IIO stack", iiostat_parse),
>         OPT_END()
>  };
>
> @@ -1509,6 +1521,12 @@ static int perf_stat_init_aggr_mode_file(struct perf_stat *st)
>         return 0;
>  }
>
> +__weak int iiostat_show_root_ports(struct evlist *evlist __maybe_unused,
> +                                  struct perf_stat_config *config __maybe_unused)
> +{
> +       return 0;
> +}

I think it's too specific, maybe iiostat_prepare() ?

> +
>  /*
>   * Add default attributes, if there were no attributes specified or
>   * if -d/--detailed, -d -d or -d -d -d is used:
> @@ -2054,6 +2072,10 @@ static void setup_system_wide(int forks)
>         }
>  }
>
> +__weak void iiostat_delete_root_ports(struct evlist *evlist __maybe_unused)
> +{
> +}

Same here..

> +
>  int cmd_stat(int argc, const char **argv)
>  {
>         const char * const stat_usage[] = {
> @@ -2230,6 +2252,12 @@ int cmd_stat(int argc, const char **argv)
>                 goto out;
>         }
>
> +       if (stat_config.iiostat_run) {
> +               status = iiostat_show_root_ports(evsel_list, &stat_config);
> +               if (status || !stat_config.iiostat_run)
> +                       goto out;
> +       }
> +
>         if (add_default_attributes())
>                 goto out;
>
> @@ -2406,6 +2434,9 @@ int cmd_stat(int argc, const char **argv)
>         perf_stat__exit_aggr_mode();
>         perf_evlist__free_stats(evsel_list);
>  out:
> +       if (stat_config.iiostat_run)
> +               iiostat_delete_root_ports(evsel_list);
> +
>         zfree(&stat_config.walltime_run);
>
>         if (smi_cost && smi_reset)
> diff --git a/tools/perf/util/iiostat.h b/tools/perf/util/iiostat.h
> new file mode 100644
> index 000000000000..8d4226df9975
> --- /dev/null
> +++ b/tools/perf/util/iiostat.h
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * perf iiostat
> + *
> + * Copyright (C) 2020, Intel Corporation
> + *
> + * Authors: Alexander Antonov <alexander.antonov@linux.intel.com>
> + */
> +
> +#ifndef _IIOSTAT_H
> +#define _IIOSTAT_H
> +
> +#include <subcmd/parse-options.h>
> +#include "util/stat.h"
> +#include "util/parse-events.h"
> +#include "util/evlist.h"
> +
> +struct option;
> +struct perf_stat_config;
> +struct evlist;
> +struct timespec;
> +
> +int iiostat_parse(const struct option *opt, const char *str,
> +                 int unset __maybe_unused);
> +void iiostat_prefix(struct perf_stat_config *config, struct evlist *evlist,
> +                   char *prefix, struct timespec *ts);
> +void iiostat_print_metric(struct perf_stat_config *config, struct evsel *evsel,
> +                         struct perf_stat_output_ctx *out);
> +int iiostat_show_root_ports(struct evlist *evlist,
> +                           struct perf_stat_config *config);
> +void iiostat_delete_root_ports(struct evlist *evlist);
> +
> +#endif /* _IIOSTAT_H */
> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> index 3bfcdb80443a..9eb8484e8b90 100644
> --- a/tools/perf/util/stat-display.c
> +++ b/tools/perf/util/stat-display.c
> @@ -17,6 +17,7 @@
>  #include "cgroup.h"
>  #include <api/fs/fs.h>
>  #include "util.h"
> +#include "iiostat.h"
>
>  #define CNTR_NOT_SUPPORTED     "<not supported>"
>  #define CNTR_NOT_COUNTED       "<not counted>"
> @@ -310,6 +311,12 @@ static void print_metric_header(struct perf_stat_config *config,
>         struct outstate *os = ctx;
>         char tbuf[1024];
>
> +       /* In case of iiostat, print metric header for first perf_device only */
> +       if (os->evsel->perf_device && os->evsel->evlist->selected->perf_device &&
> +           config->iiostat_run &&

When is the perf_device set?  Is it possible to be NULL in the iiostat mode?

Thanks,
Namhyung


> +           os->evsel->perf_device != os->evsel->evlist->selected->perf_device)
> +               return;
> +
>         if (!valid_only_metric(unit))
>                 return;
>         unit = fixunit(tbuf, os->evsel, unit);

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 5/6] perf stat: Enable iiostat mode for x86 platforms
  2020-12-23 13:03 ` [PATCH v2 5/6] perf stat: Enable iiostat mode for x86 platforms Alexander Antonov
@ 2021-01-06  9:02   ` Namhyung Kim
  2021-01-13 12:08     ` Alexander Antonov
  0 siblings, 1 reply; 19+ messages in thread
From: Namhyung Kim @ 2021-01-06  9:02 UTC (permalink / raw)
  To: Alexander Antonov
  Cc: Arnaldo Carvalho de Melo, linux-kernel, Jiri Olsa, Andi Kleen,
	Alexander Shishkin, Mark Rutland, Ian Rogers, Ingo Molnar,
	Peter Zijlstra

On Wed, Dec 23, 2020 at 10:03 PM Alexander Antonov
<alexander.antonov@linux.intel.com> wrote:
>
> This functionality is based on recently introduced sysfs attributes
> for Intel® Xeon® Scalable processor family (code name Skylake-SP):
> Commit bb42b3d39781 ("perf/x86/intel/uncore: Expose an Uncore unit to
> IIO PMON mapping")
>
> Mode is intended to provide four I/O performance metrics in MB per each
> IIO stack:
>  - Inbound Read: I/O devices below IIO stack read from the host memory
>  - Inbound Write: I/O devices below IIO stack write to the host memory
>  - Outbound Read: CPU reads from I/O devices below IIO stack
>  - Outbound Write: CPU writes to I/O devices below IIO stack
>
> Each metric requiries only one IIO event which increments at every 4B
> transfer in corresponding direction. The formulas to compute metrics
> are generic:
>     #EventCount * 4B / (1024 * 1024)

Hmm.. maybe we can do this with JSON metrics, no?

>
> Signed-off-by: Alexander Antonov <alexander.antonov@linux.intel.com>
> ---
>  tools/perf/Documentation/perf-iiostat.txt |  89 ++++++
>  tools/perf/Makefile.perf                  |   5 +-
>  tools/perf/arch/x86/util/Build            |   1 +
>  tools/perf/arch/x86/util/iiostat.c        | 337 ++++++++++++++++++++++
>  tools/perf/command-list.txt               |   1 +
>  tools/perf/perf-iiostat.sh                |  12 +
>  6 files changed, 444 insertions(+), 1 deletion(-)
>  create mode 100644 tools/perf/Documentation/perf-iiostat.txt
>  create mode 100644 tools/perf/perf-iiostat.sh
>
> diff --git a/tools/perf/Documentation/perf-iiostat.txt b/tools/perf/Documentation/perf-iiostat.txt
> new file mode 100644
> index 000000000000..38b5697b0d85
> --- /dev/null
> +++ b/tools/perf/Documentation/perf-iiostat.txt
> @@ -0,0 +1,89 @@
> +perf-iiostat(1)
> +===============
> +
> +NAME
> +----
> +perf-iiostat - Show I/O performance metrics
> +
> +SYNOPSIS
> +--------
> +[verse]
> +'perf iiostat' show
> +'perf iiostat' <ports> -- <command> [<options>]
> +
> +DESCRIPTION
> +-----------
> +Mode is intended to provide four I/O performance metrics per each IIO
> +stack (PCIe root port):
> +
> +- Inbound Read   - I/O devices below IIO stack read from the host memory, in MB
> +
> +- Inbound Write  - I/O devices below IIO stack write to the host memory, in MB
> +
> +- Outbound Read  - CPU reads from I/O devices below IIO stack, in MB
> +
> +- Outbound Write - CPU writes to I/O devices below IIO stack, in MB
> +
> +OPTIONS
> +-------
> +<command>...::
> +       Any command you can specify in a shell.
> +
> +show::
> +       List all IIO stacks.

I'd prefer 'list' for this, but not a strong opinion..

> +
> +<ports>::
> +       Select the root ports for monitoring. Comma-separated list is supported.
> +
> +EXAMPLES
> +--------
> +
> +1. List all IIO stacks (example for 2-S platform):
> +
> +   $ perf iiostat show
> +   S0-uncore_iio_0<0000:00>
> +   S1-uncore_iio_0<0000:80>
> +   S0-uncore_iio_1<0000:17>
> +   S1-uncore_iio_1<0000:85>
> +   S0-uncore_iio_2<0000:3a>
> +   S1-uncore_iio_2<0000:ae>
> +   S0-uncore_iio_3<0000:5d>
> +   S1-uncore_iio_3<0000:d7>
> +
> +2. Collect metrics for all I/O stacks:
> +
> +   $ perf iiostat -- dd if=/dev/zero of=/dev/nvme0n1 bs=1M oflag=direct
> +   357708+0 records in
> +   357707+0 records out
> +   375083606016 bytes (375 GB, 349 GiB) copied, 215.974 s, 1.7 GB/s
> +
> +    Performance counter stats for 'system wide':
> +
> +      port             Inbound Read(MB)    Inbound Write(MB)    Outbound Read(MB)   Outbound Write(MB)
> +   0000:00                    1                    0                    2                    3
> +   0000:80                    0                    0                    0                    0
> +   0000:17               352552                   43                    0                   21
> +   0000:85                    0                    0                    0                    0
> +   0000:3a                    3                    0                    0                    0
> +   0000:ae                    0                    0                    0                    0
> +   0000:5d                    0                    0                    0                    0
> +   0000:d7                    0                    0                    0                    0
> +
> +3. Collect metrics for comma-separated list of I/O stacks:
> +
> +   $ perf iiostat 0000:17,0:3a -- dd if=/dev/zero of=/dev/nvme0n1 bs=1M oflag=direct
> +   357708+0 records in
> +   357707+0 records out
> +   375083606016 bytes (375 GB, 349 GiB) copied, 197.08 s, 1.9 GB/s
> +
> +    Performance counter stats for 'system wide':
> +
> +      port             Inbound Read(MB)    Inbound Write(MB)    Outbound Read(MB)   Outbound Write(MB)
> +   0000:17               358559                   44                    0                   22
> +   0000:3a                    3                    2                    0                    0
> +
> +        197.081983474 seconds time elapsed
> +
> +SEE ALSO
> +--------
> +linkperf:perf-stat[1]
> \ No newline at end of file

[SNIP]
> diff --git a/tools/perf/perf-iiostat.sh b/tools/perf/perf-iiostat.sh
> new file mode 100644
> index 000000000000..2c5168d2550b
> --- /dev/null
> +++ b/tools/perf/perf-iiostat.sh
> @@ -0,0 +1,12 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# perf iiostat
> +# Alexander Antonov <alexander.antonov@linux.intel.com>
> +
> +if [[ "$1" == "show" ]] || [[ "$1" =~ ([a-f0-9A-F]{1,}):([a-f0-9A-F]{1,2})(,)? ]]; then
> +        DELIMITER="="
> +else
> +        DELIMITER=" "
> +fi
> +
> +perf stat --iiostat$DELIMITER$*

Why is this needed?

Thanks,
Namhyung


> \ No newline at end of file
> --
> 2.19.1
>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 2/6] perf evsel: Introduce an observed performance device
  2021-01-06  8:44   ` Namhyung Kim
@ 2021-01-13 11:13     ` Alexander Antonov
  0 siblings, 0 replies; 19+ messages in thread
From: Alexander Antonov @ 2021-01-13 11:13 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, linux-kernel, Jiri Olsa, Andi Kleen,
	Alexander Shishkin, Mark Rutland, Ian Rogers, Ingo Molnar,
	Peter Zijlstra


On 1/6/2021 11:44 AM, Namhyung Kim wrote:
> Hi,
>
> On Wed, Dec 23, 2020 at 10:03 PM Alexander Antonov
> <alexander.antonov@linux.intel.com> wrote:
>> Adding evsel::perf_device void pointer.
>>
>> For performance monitoring purposes, an evsel can have a related device.
>> These changes allow to attribute, for example, I/O performance metrics
>> to IIO stack.
>>
>> Signed-off-by: Alexander Antonov <alexander.antonov@linux.intel.com>
>> ---
>>   tools/perf/util/evsel.h | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
>> index 79a860d8e3ee..c346920f477a 100644
>> --- a/tools/perf/util/evsel.h
>> +++ b/tools/perf/util/evsel.h
>> @@ -127,6 +127,7 @@ struct evsel {
>>           * See also evsel__has_callchain().
>>           */
>>          __u64                   synth_sample_type;
>> +       void                    *perf_device;
> Maybe we can use the existing 'priv' field.
>
> Thanks,
> Namhyung

Hello Namhyung,

Looks like the 'priv' field isn't used in this case. I suppose it can be
re-used in iiostat mode.

Thanks,
Alexander
>
>>   };
>>
>>   struct perf_missing_features {
>> --
>> 2.19.1
>>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 3/6] perf stat: Basic support for iiostat in perf
  2021-01-06  8:56   ` Namhyung Kim
@ 2021-01-13 11:34     ` Alexander Antonov
  2021-01-14  3:34       ` Namhyung Kim
  0 siblings, 1 reply; 19+ messages in thread
From: Alexander Antonov @ 2021-01-13 11:34 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, linux-kernel, Jiri Olsa, Andi Kleen,
	Alexander Shishkin, Mark Rutland, Ian Rogers, Ingo Molnar,
	Peter Zijlstra


On 1/6/2021 11:56 AM, Namhyung Kim wrote:
> On Wed, Dec 23, 2020 at 10:03 PM Alexander Antonov
> <alexander.antonov@linux.intel.com> wrote:
>> Add basic flow for a new iiostat mode in perf. Mode is intended to
>> provide four I/O performance metrics per each IIO stack: Inbound Read,
>> Inbound Write, Outbound Read, Outbound Write.
> It seems like a generic analysis and other archs can extend it later..
> Then we can make it a bit more general.. at least, names? :)
I'm not sure that I fully understand you. Do you mean to rename metrics?
The mode is intended to provide PCIe metrics which are appliable for 
other archs
as well.
Actually, I suppose we can rename 'iiostat' to 'pciestat' or something 
like this
to make it a bit more general because the name 'IIO' (Integrated I/O 
stack) is
Intel specific and it can be named in different way on other platforms. 
In this
case the code has to be updated in the same way as well.
>
>> The actual code to compute the metrics and attribute it to
>> evsel::perf_device is in follow-on patches.
>>
>> Signed-off-by: Alexander Antonov <alexander.antonov@linux.intel.com>
>> ---
>>   tools/perf/builtin-stat.c      | 33 ++++++++++++++++++++++++++++-
>>   tools/perf/util/iiostat.h      | 33 +++++++++++++++++++++++++++++
>>   tools/perf/util/stat-display.c | 38 +++++++++++++++++++++++++++++++++-
>>   tools/perf/util/stat-shadow.c  | 11 +++++++++-
>>   tools/perf/util/stat.h         |  1 +
>>   5 files changed, 113 insertions(+), 3 deletions(-)
>>   create mode 100644 tools/perf/util/iiostat.h
>>
>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
>> index 72f9d0aa3f96..14c3da136927 100644
>> --- a/tools/perf/builtin-stat.c
>> +++ b/tools/perf/builtin-stat.c
>> @@ -67,6 +67,7 @@
>>   #include "util/top.h"
>>   #include "util/affinity.h"
>>   #include "util/pfm.h"
>> +#include "util/iiostat.h"
>>   #include "asm/bug.h"
>>
>>   #include <linux/time64.h>
>> @@ -198,7 +199,8 @@ static struct perf_stat_config stat_config = {
>>          .walltime_nsecs_stats   = &walltime_nsecs_stats,
>>          .big_num                = true,
>>          .ctl_fd                 = -1,
>> -       .ctl_fd_ack             = -1
>> +       .ctl_fd_ack             = -1,
>> +       .iiostat_run            = false,
>>   };
>>
>>   static bool cpus_map_matched(struct evsel *a, struct evsel *b)
>> @@ -1073,6 +1075,14 @@ static int parse_stat_cgroups(const struct option *opt,
>>          return parse_cgroups(opt, str, unset);
>>   }
>>
>> +__weak int iiostat_parse(const struct option *opt __maybe_unused,
>> +                        const char *str __maybe_unused,
>> +                        int unset __maybe_unused)
>> +{
>> +       pr_err("iiostat mode is not supported\n");
>> +       return -1;
>> +}
>> +
>>   static struct option stat_options[] = {
>>          OPT_BOOLEAN('T', "transaction", &transaction_run,
>>                      "hardware transaction statistics"),
>> @@ -1185,6 +1195,8 @@ static struct option stat_options[] = {
>>                       "\t\t\t  Optionally send control command completion ('ack\\n') to ack-fd descriptor.\n"
>>                       "\t\t\t  Alternatively, ctl-fifo / ack-fifo will be opened and used as ctl-fd / ack-fd.",
>>                        parse_control_option),
>> +       OPT_CALLBACK_OPTARG(0, "iiostat", &evsel_list, &stat_config, "root port",
>> +                           "measure PCIe metrics per IIO stack", iiostat_parse),
>>          OPT_END()
>>   };
>>
>> @@ -1509,6 +1521,12 @@ static int perf_stat_init_aggr_mode_file(struct perf_stat *st)
>>          return 0;
>>   }
>>
>> +__weak int iiostat_show_root_ports(struct evlist *evlist __maybe_unused,
>> +                                  struct perf_stat_config *config __maybe_unused)
>> +{
>> +       return 0;
>> +}
> I think it's too specific, maybe iiostat_prepare() ?
What do you think about iiostat_show_root_ports() -> iiostat_show()?
>
>> +
>>   /*
>>    * Add default attributes, if there were no attributes specified or
>>    * if -d/--detailed, -d -d or -d -d -d is used:
>> @@ -2054,6 +2072,10 @@ static void setup_system_wide(int forks)
>>          }
>>   }
>>
>> +__weak void iiostat_delete_root_ports(struct evlist *evlist __maybe_unused)
>> +{
>> +}
> Same here..
I suggest to rename iiostat_delete_root_ports() -> iiostat_release().
What do you think?
>
>> +
>>   int cmd_stat(int argc, const char **argv)
>>   {
>>          const char * const stat_usage[] = {
>> @@ -2230,6 +2252,12 @@ int cmd_stat(int argc, const char **argv)
>>                  goto out;
>>          }
>>
>> +       if (stat_config.iiostat_run) {
>> +               status = iiostat_show_root_ports(evsel_list, &stat_config);
>> +               if (status || !stat_config.iiostat_run)
>> +                       goto out;
>> +       }
>> +
>>          if (add_default_attributes())
>>                  goto out;
>>
>> @@ -2406,6 +2434,9 @@ int cmd_stat(int argc, const char **argv)
>>          perf_stat__exit_aggr_mode();
>>          perf_evlist__free_stats(evsel_list);
>>   out:
>> +       if (stat_config.iiostat_run)
>> +               iiostat_delete_root_ports(evsel_list);
>> +
>>          zfree(&stat_config.walltime_run);
>>
>>          if (smi_cost && smi_reset)
>> diff --git a/tools/perf/util/iiostat.h b/tools/perf/util/iiostat.h
>> new file mode 100644
>> index 000000000000..8d4226df9975
>> --- /dev/null
>> +++ b/tools/perf/util/iiostat.h
>> @@ -0,0 +1,33 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * perf iiostat
>> + *
>> + * Copyright (C) 2020, Intel Corporation
>> + *
>> + * Authors: Alexander Antonov <alexander.antonov@linux.intel.com>
>> + */
>> +
>> +#ifndef _IIOSTAT_H
>> +#define _IIOSTAT_H
>> +
>> +#include <subcmd/parse-options.h>
>> +#include "util/stat.h"
>> +#include "util/parse-events.h"
>> +#include "util/evlist.h"
>> +
>> +struct option;
>> +struct perf_stat_config;
>> +struct evlist;
>> +struct timespec;
>> +
>> +int iiostat_parse(const struct option *opt, const char *str,
>> +                 int unset __maybe_unused);
>> +void iiostat_prefix(struct perf_stat_config *config, struct evlist *evlist,
>> +                   char *prefix, struct timespec *ts);
>> +void iiostat_print_metric(struct perf_stat_config *config, struct evsel *evsel,
>> +                         struct perf_stat_output_ctx *out);
>> +int iiostat_show_root_ports(struct evlist *evlist,
>> +                           struct perf_stat_config *config);
>> +void iiostat_delete_root_ports(struct evlist *evlist);
>> +
>> +#endif /* _IIOSTAT_H */
>> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
>> index 3bfcdb80443a..9eb8484e8b90 100644
>> --- a/tools/perf/util/stat-display.c
>> +++ b/tools/perf/util/stat-display.c
>> @@ -17,6 +17,7 @@
>>   #include "cgroup.h"
>>   #include <api/fs/fs.h>
>>   #include "util.h"
>> +#include "iiostat.h"
>>
>>   #define CNTR_NOT_SUPPORTED     "<not supported>"
>>   #define CNTR_NOT_COUNTED       "<not counted>"
>> @@ -310,6 +311,12 @@ static void print_metric_header(struct perf_stat_config *config,
>>          struct outstate *os = ctx;
>>          char tbuf[1024];
>>
>> +       /* In case of iiostat, print metric header for first perf_device only */
>> +       if (os->evsel->perf_device && os->evsel->evlist->selected->perf_device &&
>> +           config->iiostat_run &&
> When is the perf_device set?  Is it possible to be NULL in the iiostat mode?
>
> Thanks,
> Namhyung
>
The perf_device field is initialized inside iiostat.c::iiostat_event_group()
and it cannot be NULL.
The idea is to attribute events to PCIe ports through perf_device field.

Thanks,
Alexander
>> +           os->evsel->perf_device != os->evsel->evlist->selected->perf_device)
>> +               return;
>> +
>>          if (!valid_only_metric(unit))
>>                  return;
>>          unit = fixunit(tbuf, os->evsel, unit);

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 5/6] perf stat: Enable iiostat mode for x86 platforms
  2021-01-06  9:02   ` Namhyung Kim
@ 2021-01-13 12:08     ` Alexander Antonov
  2021-01-14  3:39       ` Namhyung Kim
  0 siblings, 1 reply; 19+ messages in thread
From: Alexander Antonov @ 2021-01-13 12:08 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, linux-kernel, Jiri Olsa, Andi Kleen,
	Alexander Shishkin, Mark Rutland, Ian Rogers, Ingo Molnar,
	Peter Zijlstra


On 1/6/2021 12:02 PM, Namhyung Kim wrote:
> On Wed, Dec 23, 2020 at 10:03 PM Alexander Antonov
> <alexander.antonov@linux.intel.com> wrote:
>> This functionality is based on recently introduced sysfs attributes
>> for Intel® Xeon® Scalable processor family (code name Skylake-SP):
>> Commit bb42b3d39781 ("perf/x86/intel/uncore: Expose an Uncore unit to
>> IIO PMON mapping")
>>
>> Mode is intended to provide four I/O performance metrics in MB per each
>> IIO stack:
>>   - Inbound Read: I/O devices below IIO stack read from the host memory
>>   - Inbound Write: I/O devices below IIO stack write to the host memory
>>   - Outbound Read: CPU reads from I/O devices below IIO stack
>>   - Outbound Write: CPU writes to I/O devices below IIO stack
>>
>> Each metric requiries only one IIO event which increments at every 4B
>> transfer in corresponding direction. The formulas to compute metrics
>> are generic:
>>      #EventCount * 4B / (1024 * 1024)
> Hmm.. maybe we can do this with JSON metrics, no?
Do you mean to add metrics to *-metrics.json file?
Looks like it's possible but in this case JSON file should be updated 
for each
new enabled platform and calculations will be the same.
I would prefer to leave it as is because perf will work without changing of
userspace part once IIO sysfs attributes are added for new platforms.
>
>> Signed-off-by: Alexander Antonov <alexander.antonov@linux.intel.com>
>> ---
>>   tools/perf/Documentation/perf-iiostat.txt |  89 ++++++
>>   tools/perf/Makefile.perf                  |   5 +-
>>   tools/perf/arch/x86/util/Build            |   1 +
>>   tools/perf/arch/x86/util/iiostat.c        | 337 ++++++++++++++++++++++
>>   tools/perf/command-list.txt               |   1 +
>>   tools/perf/perf-iiostat.sh                |  12 +
>>   6 files changed, 444 insertions(+), 1 deletion(-)
>>   create mode 100644 tools/perf/Documentation/perf-iiostat.txt
>>   create mode 100644 tools/perf/perf-iiostat.sh
>>
>> diff --git a/tools/perf/Documentation/perf-iiostat.txt b/tools/perf/Documentation/perf-iiostat.txt
>> new file mode 100644
>> index 000000000000..38b5697b0d85
>> --- /dev/null
>> +++ b/tools/perf/Documentation/perf-iiostat.txt
>> @@ -0,0 +1,89 @@
>> +perf-iiostat(1)
>> +===============
>> +
>> +NAME
>> +----
>> +perf-iiostat - Show I/O performance metrics
>> +
>> +SYNOPSIS
>> +--------
>> +[verse]
>> +'perf iiostat' show
>> +'perf iiostat' <ports> -- <command> [<options>]
>> +
>> +DESCRIPTION
>> +-----------
>> +Mode is intended to provide four I/O performance metrics per each IIO
>> +stack (PCIe root port):
>> +
>> +- Inbound Read   - I/O devices below IIO stack read from the host memory, in MB
>> +
>> +- Inbound Write  - I/O devices below IIO stack write to the host memory, in MB
>> +
>> +- Outbound Read  - CPU reads from I/O devices below IIO stack, in MB
>> +
>> +- Outbound Write - CPU writes to I/O devices below IIO stack, in MB
>> +
>> +OPTIONS
>> +-------
>> +<command>...::
>> +       Any command you can specify in a shell.
>> +
>> +show::
>> +       List all IIO stacks.
> I'd prefer 'list' for this, but not a strong opinion..
The 'list' is fine for me as well.
>
>> +
>> +<ports>::
>> +       Select the root ports for monitoring. Comma-separated list is supported.
>> +
>> +EXAMPLES
>> +--------
>> +
>> +1. List all IIO stacks (example for 2-S platform):
>> +
>> +   $ perf iiostat show
>> +   S0-uncore_iio_0<0000:00>
>> +   S1-uncore_iio_0<0000:80>
>> +   S0-uncore_iio_1<0000:17>
>> +   S1-uncore_iio_1<0000:85>
>> +   S0-uncore_iio_2<0000:3a>
>> +   S1-uncore_iio_2<0000:ae>
>> +   S0-uncore_iio_3<0000:5d>
>> +   S1-uncore_iio_3<0000:d7>
>> +
>> +2. Collect metrics for all I/O stacks:
>> +
>> +   $ perf iiostat -- dd if=/dev/zero of=/dev/nvme0n1 bs=1M oflag=direct
>> +   357708+0 records in
>> +   357707+0 records out
>> +   375083606016 bytes (375 GB, 349 GiB) copied, 215.974 s, 1.7 GB/s
>> +
>> +    Performance counter stats for 'system wide':
>> +
>> +      port             Inbound Read(MB)    Inbound Write(MB)    Outbound Read(MB)   Outbound Write(MB)
>> +   0000:00                    1                    0                    2                    3
>> +   0000:80                    0                    0                    0                    0
>> +   0000:17               352552                   43                    0                   21
>> +   0000:85                    0                    0                    0                    0
>> +   0000:3a                    3                    0                    0                    0
>> +   0000:ae                    0                    0                    0                    0
>> +   0000:5d                    0                    0                    0                    0
>> +   0000:d7                    0                    0                    0                    0
>> +
>> +3. Collect metrics for comma-separated list of I/O stacks:
>> +
>> +   $ perf iiostat 0000:17,0:3a -- dd if=/dev/zero of=/dev/nvme0n1 bs=1M oflag=direct
>> +   357708+0 records in
>> +   357707+0 records out
>> +   375083606016 bytes (375 GB, 349 GiB) copied, 197.08 s, 1.9 GB/s
>> +
>> +    Performance counter stats for 'system wide':
>> +
>> +      port             Inbound Read(MB)    Inbound Write(MB)    Outbound Read(MB)   Outbound Write(MB)
>> +   0000:17               358559                   44                    0                   22
>> +   0000:3a                    3                    2                    0                    0
>> +
>> +        197.081983474 seconds time elapsed
>> +
>> +SEE ALSO
>> +--------
>> +linkperf:perf-stat[1]
>> \ No newline at end of file
> [SNIP]
>> diff --git a/tools/perf/perf-iiostat.sh b/tools/perf/perf-iiostat.sh
>> new file mode 100644
>> index 000000000000..2c5168d2550b
>> --- /dev/null
>> +++ b/tools/perf/perf-iiostat.sh
>> @@ -0,0 +1,12 @@
>> +#!/bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
>> +# perf iiostat
>> +# Alexander Antonov <alexander.antonov@linux.intel.com>
>> +
>> +if [[ "$1" == "show" ]] || [[ "$1" =~ ([a-f0-9A-F]{1,}):([a-f0-9A-F]{1,2})(,)? ]]; then
>> +        DELIMITER="="
>> +else
>> +        DELIMITER=" "
>> +fi
>> +
>> +perf stat --iiostat$DELIMITER$*
> Why is this needed?
>
> Thanks,
> Namhyung
Arnaldo raised question relates to format of 'perf stat --iiostat' 
subcommand
and explained how it can be changed to 'perf iiostat' through the aliases
mechanism in perf.

Thank you,
Alexander
>
>> \ No newline at end of file
>> --
>> 2.19.1
>>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 3/6] perf stat: Basic support for iiostat in perf
  2021-01-13 11:34     ` Alexander Antonov
@ 2021-01-14  3:34       ` Namhyung Kim
  2021-01-14 16:30         ` Alexander Antonov
  0 siblings, 1 reply; 19+ messages in thread
From: Namhyung Kim @ 2021-01-14  3:34 UTC (permalink / raw)
  To: Alexander Antonov
  Cc: Arnaldo Carvalho de Melo, linux-kernel, Jiri Olsa, Andi Kleen,
	Alexander Shishkin, Mark Rutland, Ian Rogers, Ingo Molnar,
	Peter Zijlstra

Hello,

On Wed, Jan 13, 2021 at 8:34 PM Alexander Antonov
<alexander.antonov@linux.intel.com> wrote:
>
>
> On 1/6/2021 11:56 AM, Namhyung Kim wrote:
> > On Wed, Dec 23, 2020 at 10:03 PM Alexander Antonov
> > <alexander.antonov@linux.intel.com> wrote:
> >> Add basic flow for a new iiostat mode in perf. Mode is intended to
> >> provide four I/O performance metrics per each IIO stack: Inbound Read,
> >> Inbound Write, Outbound Read, Outbound Write.
> > It seems like a generic analysis and other archs can extend it later..
> > Then we can make it a bit more general.. at least, names? :)
> I'm not sure that I fully understand you. Do you mean to rename metrics?
> The mode is intended to provide PCIe metrics which are appliable for
> other archs
> as well.
> Actually, I suppose we can rename 'iiostat' to 'pciestat' or something
> like this
> to make it a bit more general because the name 'IIO' (Integrated I/O
> stack) is
> Intel specific and it can be named in different way on other platforms.
> In this
> case the code has to be updated in the same way as well.

Maybe just 'iostat' ?

> >
> >> The actual code to compute the metrics and attribute it to
> >> evsel::perf_device is in follow-on patches.
> >>
> >> Signed-off-by: Alexander Antonov <alexander.antonov@linux.intel.com>
> >> ---
> >>   tools/perf/builtin-stat.c      | 33 ++++++++++++++++++++++++++++-
> >>   tools/perf/util/iiostat.h      | 33 +++++++++++++++++++++++++++++
> >>   tools/perf/util/stat-display.c | 38 +++++++++++++++++++++++++++++++++-
> >>   tools/perf/util/stat-shadow.c  | 11 +++++++++-
> >>   tools/perf/util/stat.h         |  1 +
> >>   5 files changed, 113 insertions(+), 3 deletions(-)
> >>   create mode 100644 tools/perf/util/iiostat.h
> >>
> >> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> >> index 72f9d0aa3f96..14c3da136927 100644
> >> --- a/tools/perf/builtin-stat.c
> >> +++ b/tools/perf/builtin-stat.c
> >> @@ -67,6 +67,7 @@
> >>   #include "util/top.h"
> >>   #include "util/affinity.h"
> >>   #include "util/pfm.h"
> >> +#include "util/iiostat.h"
> >>   #include "asm/bug.h"
> >>
> >>   #include <linux/time64.h>
> >> @@ -198,7 +199,8 @@ static struct perf_stat_config stat_config = {
> >>          .walltime_nsecs_stats   = &walltime_nsecs_stats,
> >>          .big_num                = true,
> >>          .ctl_fd                 = -1,
> >> -       .ctl_fd_ack             = -1
> >> +       .ctl_fd_ack             = -1,
> >> +       .iiostat_run            = false,
> >>   };
> >>
> >>   static bool cpus_map_matched(struct evsel *a, struct evsel *b)
> >> @@ -1073,6 +1075,14 @@ static int parse_stat_cgroups(const struct option *opt,
> >>          return parse_cgroups(opt, str, unset);
> >>   }
> >>
> >> +__weak int iiostat_parse(const struct option *opt __maybe_unused,
> >> +                        const char *str __maybe_unused,
> >> +                        int unset __maybe_unused)
> >> +{
> >> +       pr_err("iiostat mode is not supported\n");
> >> +       return -1;
> >> +}
> >> +
> >>   static struct option stat_options[] = {
> >>          OPT_BOOLEAN('T', "transaction", &transaction_run,
> >>                      "hardware transaction statistics"),
> >> @@ -1185,6 +1195,8 @@ static struct option stat_options[] = {
> >>                       "\t\t\t  Optionally send control command completion ('ack\\n') to ack-fd descriptor.\n"
> >>                       "\t\t\t  Alternatively, ctl-fifo / ack-fifo will be opened and used as ctl-fd / ack-fd.",
> >>                        parse_control_option),
> >> +       OPT_CALLBACK_OPTARG(0, "iiostat", &evsel_list, &stat_config, "root port",
> >> +                           "measure PCIe metrics per IIO stack", iiostat_parse),
> >>          OPT_END()
> >>   };
> >>
> >> @@ -1509,6 +1521,12 @@ static int perf_stat_init_aggr_mode_file(struct perf_stat *st)
> >>          return 0;
> >>   }
> >>
> >> +__weak int iiostat_show_root_ports(struct evlist *evlist __maybe_unused,
> >> +                                  struct perf_stat_config *config __maybe_unused)
> >> +{
> >> +       return 0;
> >> +}
> > I think it's too specific, maybe iiostat_prepare() ?
> What do you think about iiostat_show_root_ports() -> iiostat_show()?

I'm ok with it, I thought it needs some initialization work there.

> >
> >> +
> >>   /*
> >>    * Add default attributes, if there were no attributes specified or
> >>    * if -d/--detailed, -d -d or -d -d -d is used:
> >> @@ -2054,6 +2072,10 @@ static void setup_system_wide(int forks)
> >>          }
> >>   }
> >>
> >> +__weak void iiostat_delete_root_ports(struct evlist *evlist __maybe_unused)
> >> +{
> >> +}
> > Same here..
> I suggest to rename iiostat_delete_root_ports() -> iiostat_release().
> What do you think?

Looks good.

> >
> >> +
> >>   int cmd_stat(int argc, const char **argv)
> >>   {
> >>          const char * const stat_usage[] = {
> >> @@ -2230,6 +2252,12 @@ int cmd_stat(int argc, const char **argv)
> >>                  goto out;
> >>          }
> >>
> >> +       if (stat_config.iiostat_run) {
> >> +               status = iiostat_show_root_ports(evsel_list, &stat_config);
> >> +               if (status || !stat_config.iiostat_run)
> >> +                       goto out;
> >> +       }
> >> +
> >>          if (add_default_attributes())
> >>                  goto out;
> >>

[SNIP]
> >> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
> >> index 3bfcdb80443a..9eb8484e8b90 100644
> >> --- a/tools/perf/util/stat-display.c
> >> +++ b/tools/perf/util/stat-display.c
> >> @@ -17,6 +17,7 @@
> >>   #include "cgroup.h"
> >>   #include <api/fs/fs.h>
> >>   #include "util.h"
> >> +#include "iiostat.h"
> >>
> >>   #define CNTR_NOT_SUPPORTED     "<not supported>"
> >>   #define CNTR_NOT_COUNTED       "<not counted>"
> >> @@ -310,6 +311,12 @@ static void print_metric_header(struct perf_stat_config *config,
> >>          struct outstate *os = ctx;
> >>          char tbuf[1024];
> >>
> >> +       /* In case of iiostat, print metric header for first perf_device only */
> >> +       if (os->evsel->perf_device && os->evsel->evlist->selected->perf_device &&
> >> +           config->iiostat_run &&
> > When is the perf_device set?  Is it possible to be NULL in the iiostat mode?
> >
> The perf_device field is initialized inside iiostat.c::iiostat_event_group()
> and it cannot be NULL.
> The idea is to attribute events to PCIe ports through perf_device field.
>

If it's guaranteed non-NULL, we can check config->iiostat_run only and make
the condition simpler.

Thanks,
Namhyung



> >> +           os->evsel->perf_device != os->evsel->evlist->selected->perf_device)
> >> +               return;
> >> +
> >>          if (!valid_only_metric(unit))
> >>                  return;
> >>          unit = fixunit(tbuf, os->evsel, unit);

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 5/6] perf stat: Enable iiostat mode for x86 platforms
  2021-01-13 12:08     ` Alexander Antonov
@ 2021-01-14  3:39       ` Namhyung Kim
  2021-01-14 16:41         ` Alexander Antonov
  0 siblings, 1 reply; 19+ messages in thread
From: Namhyung Kim @ 2021-01-14  3:39 UTC (permalink / raw)
  To: Alexander Antonov
  Cc: Arnaldo Carvalho de Melo, linux-kernel, Jiri Olsa, Andi Kleen,
	Alexander Shishkin, Mark Rutland, Ian Rogers, Ingo Molnar,
	Peter Zijlstra

On Wed, Jan 13, 2021 at 9:08 PM Alexander Antonov
<alexander.antonov@linux.intel.com> wrote:
>
>
> On 1/6/2021 12:02 PM, Namhyung Kim wrote:
> > On Wed, Dec 23, 2020 at 10:03 PM Alexander Antonov
> > <alexander.antonov@linux.intel.com> wrote:
> >> This functionality is based on recently introduced sysfs attributes
> >> for Intel® Xeon® Scalable processor family (code name Skylake-SP):
> >> Commit bb42b3d39781 ("perf/x86/intel/uncore: Expose an Uncore unit to
> >> IIO PMON mapping")
> >>
> >> Mode is intended to provide four I/O performance metrics in MB per each
> >> IIO stack:
> >>   - Inbound Read: I/O devices below IIO stack read from the host memory
> >>   - Inbound Write: I/O devices below IIO stack write to the host memory
> >>   - Outbound Read: CPU reads from I/O devices below IIO stack
> >>   - Outbound Write: CPU writes to I/O devices below IIO stack
> >>
> >> Each metric requiries only one IIO event which increments at every 4B
> >> transfer in corresponding direction. The formulas to compute metrics
> >> are generic:
> >>      #EventCount * 4B / (1024 * 1024)
> > Hmm.. maybe we can do this with JSON metrics, no?
> Do you mean to add metrics to *-metrics.json file?
> Looks like it's possible but in this case JSON file should be updated
> for each
> new enabled platform and calculations will be the same.
> I would prefer to leave it as is because perf will work without changing of
> userspace part once IIO sysfs attributes are added for new platforms.

OK.

> >
> >> Signed-off-by: Alexander Antonov <alexander.antonov@linux.intel.com>
> >> ---

[SNIP]
> >> diff --git a/tools/perf/perf-iiostat.sh b/tools/perf/perf-iiostat.sh
> >> new file mode 100644
> >> index 000000000000..2c5168d2550b
> >> --- /dev/null
> >> +++ b/tools/perf/perf-iiostat.sh
> >> @@ -0,0 +1,12 @@
> >> +#!/bin/bash
> >> +# SPDX-License-Identifier: GPL-2.0
> >> +# perf iiostat
> >> +# Alexander Antonov <alexander.antonov@linux.intel.com>
> >> +
> >> +if [[ "$1" == "show" ]] || [[ "$1" =~ ([a-f0-9A-F]{1,}):([a-f0-9A-F]{1,2})(,)? ]]; then
> >> +        DELIMITER="="
> >> +else
> >> +        DELIMITER=" "
> >> +fi
> >> +
> >> +perf stat --iiostat$DELIMITER$*
> > Why is this needed?
> >
> > Thanks,
> > Namhyung
> Arnaldo raised question relates to format of 'perf stat --iiostat'
> subcommand
> and explained how it can be changed to 'perf iiostat' through the aliases
> mechanism in perf.

Yeah, I know that.  What I'm asking is the DELIMITER part.

Thanks,
Namhyung

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 3/6] perf stat: Basic support for iiostat in perf
  2021-01-14  3:34       ` Namhyung Kim
@ 2021-01-14 16:30         ` Alexander Antonov
  0 siblings, 0 replies; 19+ messages in thread
From: Alexander Antonov @ 2021-01-14 16:30 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, linux-kernel, Jiri Olsa, Andi Kleen,
	Alexander Shishkin, Mark Rutland, Ian Rogers, Ingo Molnar,
	Peter Zijlstra


On 1/14/2021 6:34 AM, Namhyung Kim wrote:
> Hello,
>
> On Wed, Jan 13, 2021 at 8:34 PM Alexander Antonov
> <alexander.antonov@linux.intel.com> wrote:
>>
>> On 1/6/2021 11:56 AM, Namhyung Kim wrote:
>>> On Wed, Dec 23, 2020 at 10:03 PM Alexander Antonov
>>> <alexander.antonov@linux.intel.com> wrote:
>>>> Add basic flow for a new iiostat mode in perf. Mode is intended to
>>>> provide four I/O performance metrics per each IIO stack: Inbound Read,
>>>> Inbound Write, Outbound Read, Outbound Write.
>>> It seems like a generic analysis and other archs can extend it later..
>>> Then we can make it a bit more general.. at least, names? :)
>> I'm not sure that I fully understand you. Do you mean to rename metrics?
>> The mode is intended to provide PCIe metrics which are appliable for
>> other archs
>> as well.
>> Actually, I suppose we can rename 'iiostat' to 'pciestat' or something
>> like this
>> to make it a bit more general because the name 'IIO' (Integrated I/O
>> stack) is
>> Intel specific and it can be named in different way on other platforms.
>> In this
>> case the code has to be updated in the same way as well.
> Maybe just 'iostat' ?
Yeah, it looks better :)

>
>>>> The actual code to compute the metrics and attribute it to
>>>> evsel::perf_device is in follow-on patches.
>>>>
>>>> Signed-off-by: Alexander Antonov <alexander.antonov@linux.intel.com>
>>>> ---
>>>>    tools/perf/builtin-stat.c      | 33 ++++++++++++++++++++++++++++-
>>>>    tools/perf/util/iiostat.h      | 33 +++++++++++++++++++++++++++++
>>>>    tools/perf/util/stat-display.c | 38 +++++++++++++++++++++++++++++++++-
>>>>    tools/perf/util/stat-shadow.c  | 11 +++++++++-
>>>>    tools/perf/util/stat.h         |  1 +
>>>>    5 files changed, 113 insertions(+), 3 deletions(-)
>>>>    create mode 100644 tools/perf/util/iiostat.h
>>>>
>>>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
>>>> index 72f9d0aa3f96..14c3da136927 100644
>>>> --- a/tools/perf/builtin-stat.c
>>>> +++ b/tools/perf/builtin-stat.c
>>>> @@ -67,6 +67,7 @@
>>>>    #include "util/top.h"
>>>>    #include "util/affinity.h"
>>>>    #include "util/pfm.h"
>>>> +#include "util/iiostat.h"
>>>>    #include "asm/bug.h"
>>>>
>>>>    #include <linux/time64.h>
>>>> @@ -198,7 +199,8 @@ static struct perf_stat_config stat_config = {
>>>>           .walltime_nsecs_stats   = &walltime_nsecs_stats,
>>>>           .big_num                = true,
>>>>           .ctl_fd                 = -1,
>>>> -       .ctl_fd_ack             = -1
>>>> +       .ctl_fd_ack             = -1,
>>>> +       .iiostat_run            = false,
>>>>    };
>>>>
>>>>    static bool cpus_map_matched(struct evsel *a, struct evsel *b)
>>>> @@ -1073,6 +1075,14 @@ static int parse_stat_cgroups(const struct option *opt,
>>>>           return parse_cgroups(opt, str, unset);
>>>>    }
>>>>
>>>> +__weak int iiostat_parse(const struct option *opt __maybe_unused,
>>>> +                        const char *str __maybe_unused,
>>>> +                        int unset __maybe_unused)
>>>> +{
>>>> +       pr_err("iiostat mode is not supported\n");
>>>> +       return -1;
>>>> +}
>>>> +
>>>>    static struct option stat_options[] = {
>>>>           OPT_BOOLEAN('T', "transaction", &transaction_run,
>>>>                       "hardware transaction statistics"),
>>>> @@ -1185,6 +1195,8 @@ static struct option stat_options[] = {
>>>>                        "\t\t\t  Optionally send control command completion ('ack\\n') to ack-fd descriptor.\n"
>>>>                        "\t\t\t  Alternatively, ctl-fifo / ack-fifo will be opened and used as ctl-fd / ack-fd.",
>>>>                         parse_control_option),
>>>> +       OPT_CALLBACK_OPTARG(0, "iiostat", &evsel_list, &stat_config, "root port",
>>>> +                           "measure PCIe metrics per IIO stack", iiostat_parse),
>>>>           OPT_END()
>>>>    };
>>>>
>>>> @@ -1509,6 +1521,12 @@ static int perf_stat_init_aggr_mode_file(struct perf_stat *st)
>>>>           return 0;
>>>>    }
>>>>
>>>> +__weak int iiostat_show_root_ports(struct evlist *evlist __maybe_unused,
>>>> +                                  struct perf_stat_config *config __maybe_unused)
>>>> +{
>>>> +       return 0;
>>>> +}
>>> I think it's too specific, maybe iiostat_prepare() ?
>> What do you think about iiostat_show_root_ports() -> iiostat_show()?
> I'm ok with it, I thought it needs some initialization work there.
>
>>>> +
>>>>    /*
>>>>     * Add default attributes, if there were no attributes specified or
>>>>     * if -d/--detailed, -d -d or -d -d -d is used:
>>>> @@ -2054,6 +2072,10 @@ static void setup_system_wide(int forks)
>>>>           }
>>>>    }
>>>>
>>>> +__weak void iiostat_delete_root_ports(struct evlist *evlist __maybe_unused)
>>>> +{
>>>> +}
>>> Same here..
>> I suggest to rename iiostat_delete_root_ports() -> iiostat_release().
>> What do you think?
> Looks good.
>
>>>> +
>>>>    int cmd_stat(int argc, const char **argv)
>>>>    {
>>>>           const char * const stat_usage[] = {
>>>> @@ -2230,6 +2252,12 @@ int cmd_stat(int argc, const char **argv)
>>>>                   goto out;
>>>>           }
>>>>
>>>> +       if (stat_config.iiostat_run) {
>>>> +               status = iiostat_show_root_ports(evsel_list, &stat_config);
>>>> +               if (status || !stat_config.iiostat_run)
>>>> +                       goto out;
>>>> +       }
>>>> +
>>>>           if (add_default_attributes())
>>>>                   goto out;
>>>>
> [SNIP]
>>>> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
>>>> index 3bfcdb80443a..9eb8484e8b90 100644
>>>> --- a/tools/perf/util/stat-display.c
>>>> +++ b/tools/perf/util/stat-display.c
>>>> @@ -17,6 +17,7 @@
>>>>    #include "cgroup.h"
>>>>    #include <api/fs/fs.h>
>>>>    #include "util.h"
>>>> +#include "iiostat.h"
>>>>
>>>>    #define CNTR_NOT_SUPPORTED     "<not supported>"
>>>>    #define CNTR_NOT_COUNTED       "<not counted>"
>>>> @@ -310,6 +311,12 @@ static void print_metric_header(struct perf_stat_config *config,
>>>>           struct outstate *os = ctx;
>>>>           char tbuf[1024];
>>>>
>>>> +       /* In case of iiostat, print metric header for first perf_device only */
>>>> +       if (os->evsel->perf_device && os->evsel->evlist->selected->perf_device &&
>>>> +           config->iiostat_run &&
>>> When is the perf_device set?  Is it possible to be NULL in the iiostat mode?
>>>
>> The perf_device field is initialized inside iiostat.c::iiostat_event_group()
>> and it cannot be NULL.
>> The idea is to attribute events to PCIe ports through perf_device field.
>>
> If it's guaranteed non-NULL, we can check config->iiostat_run only and make
> the condition simpler.
>
> Thanks,
> Namhyung
>
I will update it in the next version of patchset.

Thanks,
Alexander
>
>>>> +           os->evsel->perf_device != os->evsel->evlist->selected->perf_device)
>>>> +               return;
>>>> +
>>>>           if (!valid_only_metric(unit))
>>>>                   return;
>>>>           unit = fixunit(tbuf, os->evsel, unit);

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 5/6] perf stat: Enable iiostat mode for x86 platforms
  2021-01-14  3:39       ` Namhyung Kim
@ 2021-01-14 16:41         ` Alexander Antonov
  2021-01-15  7:33           ` Namhyung Kim
  0 siblings, 1 reply; 19+ messages in thread
From: Alexander Antonov @ 2021-01-14 16:41 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, linux-kernel, Jiri Olsa, Andi Kleen,
	Alexander Shishkin, Mark Rutland, Ian Rogers, Ingo Molnar,
	Peter Zijlstra


On 1/14/2021 6:39 AM, Namhyung Kim wrote:
> On Wed, Jan 13, 2021 at 9:08 PM Alexander Antonov
> <alexander.antonov@linux.intel.com> wrote:
>>
>> On 1/6/2021 12:02 PM, Namhyung Kim wrote:
>>> On Wed, Dec 23, 2020 at 10:03 PM Alexander Antonov
>>> <alexander.antonov@linux.intel.com> wrote:
>>>> This functionality is based on recently introduced sysfs attributes
>>>> for Intel® Xeon® Scalable processor family (code name Skylake-SP):
>>>> Commit bb42b3d39781 ("perf/x86/intel/uncore: Expose an Uncore unit to
>>>> IIO PMON mapping")
>>>>
>>>> Mode is intended to provide four I/O performance metrics in MB per each
>>>> IIO stack:
>>>>    - Inbound Read: I/O devices below IIO stack read from the host memory
>>>>    - Inbound Write: I/O devices below IIO stack write to the host memory
>>>>    - Outbound Read: CPU reads from I/O devices below IIO stack
>>>>    - Outbound Write: CPU writes to I/O devices below IIO stack
>>>>
>>>> Each metric requiries only one IIO event which increments at every 4B
>>>> transfer in corresponding direction. The formulas to compute metrics
>>>> are generic:
>>>>       #EventCount * 4B / (1024 * 1024)
>>> Hmm.. maybe we can do this with JSON metrics, no?
>> Do you mean to add metrics to *-metrics.json file?
>> Looks like it's possible but in this case JSON file should be updated
>> for each
>> new enabled platform and calculations will be the same.
>> I would prefer to leave it as is because perf will work without changing of
>> userspace part once IIO sysfs attributes are added for new platforms.
> OK.
>
>>>> Signed-off-by: Alexander Antonov <alexander.antonov@linux.intel.com>
>>>> ---
> [SNIP]
>>>> diff --git a/tools/perf/perf-iiostat.sh b/tools/perf/perf-iiostat.sh
>>>> new file mode 100644
>>>> index 000000000000..2c5168d2550b
>>>> --- /dev/null
>>>> +++ b/tools/perf/perf-iiostat.sh
>>>> @@ -0,0 +1,12 @@
>>>> +#!/bin/bash
>>>> +# SPDX-License-Identifier: GPL-2.0
>>>> +# perf iiostat
>>>> +# Alexander Antonov <alexander.antonov@linux.intel.com>
>>>> +
>>>> +if [[ "$1" == "show" ]] || [[ "$1" =~ ([a-f0-9A-F]{1,}):([a-f0-9A-F]{1,2})(,)? ]]; then
>>>> +        DELIMITER="="
>>>> +else
>>>> +        DELIMITER=" "
>>>> +fi
>>>> +
>>>> +perf stat --iiostat$DELIMITER$*
>>> Why is this needed?
>>>
>>> Thanks,
>>> Namhyung
>> Arnaldo raised question relates to format of 'perf stat --iiostat'
>> subcommand
>> and explained how it can be changed to 'perf iiostat' through the aliases
>> mechanism in perf.
> Yeah, I know that.  What I'm asking is the DELIMITER part.
>
> Thanks,
> Namhyung
I'm using DELIMITER to resolve two different cases for format of iiostat 
command:
The first one is the command with an option for iiostat mode, for example:
'perf iiostat show' which should be converted to 'perf stat 
--iiostat=show' or
'perf iiostat 0000:ae,0000:5d' to 'perf stat --iiostat=0000:ae,0000:5d'.
The second is the command without any option for iiostat: 'perf iiostat 
-I 1000'
should be converted to 'perf stat --iiostat -I 1000'.

Thanks,
Alexander

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 5/6] perf stat: Enable iiostat mode for x86 platforms
  2021-01-14 16:41         ` Alexander Antonov
@ 2021-01-15  7:33           ` Namhyung Kim
  2021-01-15 14:34             ` Alexander Antonov
  0 siblings, 1 reply; 19+ messages in thread
From: Namhyung Kim @ 2021-01-15  7:33 UTC (permalink / raw)
  To: Alexander Antonov
  Cc: Arnaldo Carvalho de Melo, linux-kernel, Jiri Olsa, Andi Kleen,
	Alexander Shishkin, Mark Rutland, Ian Rogers, Ingo Molnar,
	Peter Zijlstra

On Fri, Jan 15, 2021 at 1:41 AM Alexander Antonov
<alexander.antonov@linux.intel.com> wrote:
> On 1/14/2021 6:39 AM, Namhyung Kim wrote:
> > On Wed, Jan 13, 2021 at 9:08 PM Alexander Antonov
> > <alexander.antonov@linux.intel.com> wrote:
> >>
> >> On 1/6/2021 12:02 PM, Namhyung Kim wrote:
> >>> On Wed, Dec 23, 2020 at 10:03 PM Alexander Antonov
> >>>> diff --git a/tools/perf/perf-iiostat.sh b/tools/perf/perf-iiostat.sh
> >>>> new file mode 100644
> >>>> index 000000000000..2c5168d2550b
> >>>> --- /dev/null
> >>>> +++ b/tools/perf/perf-iiostat.sh
> >>>> @@ -0,0 +1,12 @@
> >>>> +#!/bin/bash
> >>>> +# SPDX-License-Identifier: GPL-2.0
> >>>> +# perf iiostat
> >>>> +# Alexander Antonov <alexander.antonov@linux.intel.com>
> >>>> +
> >>>> +if [[ "$1" == "show" ]] || [[ "$1" =~ ([a-f0-9A-F]{1,}):([a-f0-9A-F]{1,2})(,)? ]]; then
> >>>> +        DELIMITER="="
> >>>> +else
> >>>> +        DELIMITER=" "
> >>>> +fi
> >>>> +
> >>>> +perf stat --iiostat$DELIMITER$*
> >>> Why is this needed?
> >>>
> >>> Thanks,
> >>> Namhyung
> >> Arnaldo raised question relates to format of 'perf stat --iiostat'
> >> subcommand
> >> and explained how it can be changed to 'perf iiostat' through the aliases
> >> mechanism in perf.
> > Yeah, I know that.  What I'm asking is the DELIMITER part.
> >
> > Thanks,
> > Namhyung
> I'm using DELIMITER to resolve two different cases for format of iiostat
> command:
> The first one is the command with an option for iiostat mode, for example:
> 'perf iiostat show' which should be converted to 'perf stat
> --iiostat=show' or
> 'perf iiostat 0000:ae,0000:5d' to 'perf stat --iiostat=0000:ae,0000:5d'.
> The second is the command without any option for iiostat: 'perf iiostat
> -I 1000'
> should be converted to 'perf stat --iiostat -I 1000'.

Can't we simply use a whitespace ?

Thanks,
Namhyung

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 5/6] perf stat: Enable iiostat mode for x86 platforms
  2021-01-15  7:33           ` Namhyung Kim
@ 2021-01-15 14:34             ` Alexander Antonov
  0 siblings, 0 replies; 19+ messages in thread
From: Alexander Antonov @ 2021-01-15 14:34 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Arnaldo Carvalho de Melo, linux-kernel, Jiri Olsa, Andi Kleen,
	Alexander Shishkin, Mark Rutland, Ian Rogers, Ingo Molnar,
	Peter Zijlstra


On 1/15/2021 10:33 AM, Namhyung Kim wrote:
> On Fri, Jan 15, 2021 at 1:41 AM Alexander Antonov
> <alexander.antonov@linux.intel.com> wrote:
>> On 1/14/2021 6:39 AM, Namhyung Kim wrote:
>>> On Wed, Jan 13, 2021 at 9:08 PM Alexander Antonov
>>> <alexander.antonov@linux.intel.com> wrote:
>>>> On 1/6/2021 12:02 PM, Namhyung Kim wrote:
>>>>> On Wed, Dec 23, 2020 at 10:03 PM Alexander Antonov
>>>>>> diff --git a/tools/perf/perf-iiostat.sh b/tools/perf/perf-iiostat.sh
>>>>>> new file mode 100644
>>>>>> index 000000000000..2c5168d2550b
>>>>>> --- /dev/null
>>>>>> +++ b/tools/perf/perf-iiostat.sh
>>>>>> @@ -0,0 +1,12 @@
>>>>>> +#!/bin/bash
>>>>>> +# SPDX-License-Identifier: GPL-2.0
>>>>>> +# perf iiostat
>>>>>> +# Alexander Antonov <alexander.antonov@linux.intel.com>
>>>>>> +
>>>>>> +if [[ "$1" == "show" ]] || [[ "$1" =~ ([a-f0-9A-F]{1,}):([a-f0-9A-F]{1,2})(,)? ]]; then
>>>>>> +        DELIMITER="="
>>>>>> +else
>>>>>> +        DELIMITER=" "
>>>>>> +fi
>>>>>> +
>>>>>> +perf stat --iiostat$DELIMITER$*
>>>>> Why is this needed?
>>>>>
>>>>> Thanks,
>>>>> Namhyung
>>>> Arnaldo raised question relates to format of 'perf stat --iiostat'
>>>> subcommand
>>>> and explained how it can be changed to 'perf iiostat' through the aliases
>>>> mechanism in perf.
>>> Yeah, I know that.  What I'm asking is the DELIMITER part.
>>>
>>> Thanks,
>>> Namhyung
>> I'm using DELIMITER to resolve two different cases for format of iiostat
>> command:
>> The first one is the command with an option for iiostat mode, for example:
>> 'perf iiostat show' which should be converted to 'perf stat
>> --iiostat=show' or
>> 'perf iiostat 0000:ae,0000:5d' to 'perf stat --iiostat=0000:ae,0000:5d'.
>> The second is the command without any option for iiostat: 'perf iiostat
>> -I 1000'
>> should be converted to 'perf stat --iiostat -I 1000'.
> Can't we simply use a whitespace ?
We need to use the equal sign to pass arguments to iiostat mode.

Thanks,
Alexander

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2021-01-15 14:37 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-23 13:03 [PATCH v2 0/6] perf stat: Introduce iiostat mode to provide I/O performance metrics Alexander Antonov
2020-12-23 13:03 ` [PATCH v2 1/6] perf stat: Add AGGR_IIO_STACK mode Alexander Antonov
2020-12-23 13:03 ` [PATCH v2 2/6] perf evsel: Introduce an observed performance device Alexander Antonov
2021-01-06  8:44   ` Namhyung Kim
2021-01-13 11:13     ` Alexander Antonov
2020-12-23 13:03 ` [PATCH v2 3/6] perf stat: Basic support for iiostat in perf Alexander Antonov
2021-01-06  8:56   ` Namhyung Kim
2021-01-13 11:34     ` Alexander Antonov
2021-01-14  3:34       ` Namhyung Kim
2021-01-14 16:30         ` Alexander Antonov
2020-12-23 13:03 ` [PATCH v2 4/6] perf stat: Helper functions for IIO stacks list in iiostat mode Alexander Antonov
2020-12-23 13:03 ` [PATCH v2 5/6] perf stat: Enable iiostat mode for x86 platforms Alexander Antonov
2021-01-06  9:02   ` Namhyung Kim
2021-01-13 12:08     ` Alexander Antonov
2021-01-14  3:39       ` Namhyung Kim
2021-01-14 16:41         ` Alexander Antonov
2021-01-15  7:33           ` Namhyung Kim
2021-01-15 14:34             ` Alexander Antonov
2020-12-23 13:03 ` [PATCH v2 6/6] perf: Update .gitignore file Alexander Antonov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.