[PATCH] perf stat: Add support for s390 transaction counters

[PATCH] perf stat: Add support for s390 transaction counters

[Thomas Richter]

This patch introduces support for s390 transaction counters
displayed with command 'perf stat -T -- sleep 2'

Right now there is only hard coded support for x86.

This patch introduces architecture specfic counter
tables for x86 and s390. The architecture is queried
and the event string for transaction counters is constructed
depending on the architecture and the CPU measurement
facility counter list.

Output Before:
[root@s35lp76 perf]# perf stat -T -- sleep 1
Cannot set up transaction events
[root@s35lp76 perf]#

Output after:
[root@s35lp76 perf]# ./perf stat -T -- sleep 1

 Performance counter stats for 'sleep 1':

   0.939985      task-clock (msec)         #    0.001 CPUs utilized
  2,557,145      instructions              #    0.53  insn per cycle
  4,785,929      cycles                    #    5.091 GHz
          0      cpum_cf/TX_C_TABORT_NO_SPECIAL/ #    0.000 K/sec
          0      cpum_cf/TX_C_TABORT_SPECIAL/ #    0.000 K/sec
          0      cpum_cf/TX_C_TEND/        #    0.000 K/sec
          0      cpum_cf/TX_NC_TABORT/     #    0.000 K/sec
          0      cpum_cf/TX_NC_TEND/       #    0.000 K/sec

  1.001934710 seconds time elapsed

[root@s35lp76 perf]#

Output on x86 is unchanged.

Signed-off-by: Thomas Richter <tmricht@linux.vnet.ibm.com>
Reviewed--by: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>
---
 tools/perf/builtin-stat.c | 162 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 135 insertions(+), 27 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 86c8c8a9229c..75b4f1c9cd78 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -95,22 +95,8 @@ static const char *transaction_attrs = {
 	"task-clock,"
 	"{"
 	"instructions,"
-	"cycles,"
-	"cpu/cycles-t/,"
-	"cpu/tx-start/,"
-	"cpu/el-start/,"
-	"cpu/cycles-ct/"
-	"}"
-};
-
-/* More limited version when the CPU does not have all events. */
-static const char * transaction_limited_attrs = {
-	"task-clock,"
-	"{"
-	"instructions,"
-	"cycles,"
-	"cpu/cycles-t/,"
-	"cpu/tx-start/"
+	"cycles"
+	"%s"
 	"}"
 };
 
@@ -2149,13 +2135,123 @@ __weak void arch_topdown_group_warn(void)
 {
 }
 
+struct pmu_tx_events {			/* Define transaction counters */
+	const char *pmu;		/* PMU name */
+	const char *name;		/* Counter name */
+};
+
+static struct pmu_tx_events x86_tx_events[] = {/* x86 transaction counters */
+	{
+		.pmu = "cpu",
+		.name = "cycles-t",
+	},
+	{
+		.pmu = "cpu",
+		.name = "tx-start",
+	},
+	{
+		.pmu = "cpu",
+		.name = "el-start",
+	},
+	{
+		.pmu = "cpu",
+		.name = "cycles-ct",
+	},
+	{
+		.pmu = 0
+	}
+};
+
+static struct pmu_tx_events s390_tx_events[] = {/* s390 transaction counters */
+	{
+		.pmu = "cpum_cf",
+		.name = "TX_C_TABORT_NO_SPECIAL",
+	},
+	{
+		.pmu = "cpum_cf",
+		.name = "TX_C_TABORT_SPECIAL",
+	},
+	{
+		.pmu = "cpum_cf",
+		.name = "TX_C_TEND",
+	},
+	{
+		.pmu = "cpum_cf",
+		.name = "TX_NC_TABORT",
+	},
+	{
+		.pmu = "cpum_cf",
+		.name = "TX_NC_TEND",
+	},
+	{
+		.pmu = 0
+	}
+};
+
+struct arch_pmu_tx_events {
+	const char *archname;		/* Architecture name */
+	struct pmu_tx_events *tx;	/* Architecture specific counters */
+} arch_pmu_tx_events[] = {
+	{
+		.archname = "s390",
+		.tx = s390_tx_events
+	},
+	{
+		.archname = "x86",
+		.tx = x86_tx_events
+	},
+	{
+		.archname = 0
+	}
+};
+
+static struct pmu_tx_events *pmu_tx_find_arch(const char *name)
+{
+	struct arch_pmu_tx_events *p = arch_pmu_tx_events;
+
+	for (; p->archname; ++p)
+		if (!strcmp(name, p->archname))
+			return p->tx;
+	return NULL;
+}
+
+/* Search the list of transaction events and test if they are valid for
+ * this PMU. The events are named cpu/cycles-t/ or cpum_cf/TX_NC_TEND/
+ * that is the PMU name followed by the event name surrounded by slashes.
+ *
+ * The function returns a string which contains the tested (and existing)
+ * PMU transaction events. The caller must free this string.
+ *
+ * If no PMU transaction events are found, a NULL pointer is returned.
+ */
+static char *build_tx_string(const char *fmt, struct pmu_tx_events *txp)
+{
+	struct pmu_tx_events *p = txp;
+	char buffer[512], *result;
+	int rc, i = 0;
+
+	memset(buffer, 0, sizeof(buffer));
+	for (p = txp; p->pmu; ++p) {
+		if (pmu_have_event(p->pmu, p->name)) {
+			rc = snprintf(buffer + i, sizeof(buffer) - i, ",%s/%s/",
+				      p->pmu, p->name);
+			i += rc;
+			if (i >= (int)sizeof(buffer))
+				break;
+		}
+	}
+	if (i)			/* Transaction counters found */
+		return asprintf(&result, fmt, buffer) < 0 ? NULL : result;
+	return NULL;
+}
+
 /*
  * Add default attributes, if there were no attributes specified or
  * if -d/--detailed, -d -d or -d -d -d is used:
  */
 static int add_default_attributes(void)
 {
-	int err;
+	int err = -1;
 	struct perf_event_attr default_attrs0[] = {
 
   { .type = PERF_TYPE_SOFTWARE, .config = PERF_COUNT_SW_TASK_CLOCK		},
@@ -2274,20 +2370,32 @@ static int add_default_attributes(void)
 		return 0;
 
 	if (transaction_run) {
-		struct parse_events_error errinfo;
+		struct parse_events_error errinfo = { .str = NULL };
+		char *tx_str;
+		struct pmu_tx_events *tx_archp;
 
-		if (pmu_have_event("cpu", "cycles-ct") &&
-		    pmu_have_event("cpu", "el-start"))
-			err = parse_events(evsel_list, transaction_attrs,
-					   &errinfo);
-		else
-			err = parse_events(evsel_list,
-					   transaction_limited_attrs,
-					   &errinfo);
-		if (err) {
+		/* Find architecture transaction counter string table */
+		tx_archp = pmu_tx_find_arch(perf_env__arch(NULL));
+		if (!tx_archp) {
+			fprintf(stderr, "Cannot set up transaction events\n");
+			return -1;
+		}
+
+		/* Build architecture transaction counter string */
+		tx_str = build_tx_string(transaction_attrs, tx_archp);
+		if (!tx_str) {
 			fprintf(stderr, "Cannot set up transaction events\n");
 			return -1;
 		}
+
+		err = parse_events(evsel_list, tx_str, &errinfo);
+		free(tx_str);
+		if (err) {
+			fprintf(stderr, "Cannot set up transaction events:%s\n",
+				errinfo.str);
+			free(errinfo.str);
+			return -1;
+		}
 		return 0;
 	}
 
-- 
2.14.3

[PATCH] perf stat: Make function perf_stat_evsel_id_init static

[Thomas Richter]

Function perf_stat_evsel_id_init() has global linkage
but is only used in util/stat.c. Make it static.

Signed-off-by: Thomas Richter <tmricht@linux.vnet.ibm.com>
---
 tools/perf/util/stat.c | 2 +-
 tools/perf/util/stat.h | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/tools/perf/util/stat.c b/tools/perf/util/stat.c
index 32235657c1ac..a0061e0b0fad 100644
--- a/tools/perf/util/stat.c
+++ b/tools/perf/util/stat.c
@@ -92,7 +92,7 @@ static const char *id_str[PERF_STAT_EVSEL_ID__MAX] = {
 };
 #undef ID
 
-void perf_stat_evsel_id_init(struct perf_evsel *evsel)
+static void perf_stat_evsel_id_init(struct perf_evsel *evsel)
 {
 	struct perf_stat_evsel *ps = evsel->stats;
 	int i;
diff --git a/tools/perf/util/stat.h b/tools/perf/util/stat.h
index dbc6f7134f61..679994a5cd07 100644
--- a/tools/perf/util/stat.h
+++ b/tools/perf/util/stat.h
@@ -126,8 +126,6 @@ bool __perf_evsel_stat__is(struct perf_evsel *evsel,
 #define perf_stat_evsel__is(evsel, id) \
 	__perf_evsel_stat__is(evsel, PERF_STAT_EVSEL_ID__ ## id)
 
-void perf_stat_evsel_id_init(struct perf_evsel *evsel);
-
 extern struct runtime_stat rt_stat;
 extern struct stats walltime_nsecs_stats;
 
-- 
2.14.3

Re: [PATCH] perf stat: Add support for s390 transaction counters

[Arnaldo Carvalho de Melo]

Em Mon, Mar 12, 2018 at 11:38:06AM +0100, Thomas Richter escreveu:
> This patch introduces support for s390 transaction counters
> displayed with command 'perf stat -T -- sleep 2'
> 
> Right now there is only hard coded support for x86.
> 
> This patch introduces architecture specfic counter
> tables for x86 and s390. The architecture is queried
> and the event string for transaction counters is constructed
> depending on the architecture and the CPU measurement
> facility counter list.
> 
> Output Before:
> [root@s35lp76 perf]# perf stat -T -- sleep 1
> Cannot set up transaction events
> [root@s35lp76 perf]#
> 
> Output after:
> [root@s35lp76 perf]# ./perf stat -T -- sleep 1
> 
>  Performance counter stats for 'sleep 1':
> 
>    0.939985      task-clock (msec)         #    0.001 CPUs utilized
>   2,557,145      instructions              #    0.53  insn per cycle
>   4,785,929      cycles                    #    5.091 GHz
>           0      cpum_cf/TX_C_TABORT_NO_SPECIAL/ #    0.000 K/sec
>           0      cpum_cf/TX_C_TABORT_SPECIAL/ #    0.000 K/sec
>           0      cpum_cf/TX_C_TEND/        #    0.000 K/sec
>           0      cpum_cf/TX_NC_TABORT/     #    0.000 K/sec
>           0      cpum_cf/TX_NC_TEND/       #    0.000 K/sec
> 
>   1.001934710 seconds time elapsed
> 
> [root@s35lp76 perf]#
> 
> Output on x86 is unchanged.

Please break this patch in multiple ones, for instance, that part that
converts the hardcoded string to a table with events, etc, should be
done first, just for x86, then when you add the s/390 support that patch
will have mostly + lines, i.e. will be _just_ for s/390.

I.e. the logic that discovers which events should be used instead of
trying out of two possible sets ("full featured" and that limited one)
is an improvement that works for x86 or any other arch where there are
processors with different sets of transaction counters.

See other comments below.
 
> Signed-off-by: Thomas Richter <tmricht@linux.vnet.ibm.com>
> Reviewed--by: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>
> ---
>  tools/perf/builtin-stat.c | 162 ++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 135 insertions(+), 27 deletions(-)
> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 86c8c8a9229c..75b4f1c9cd78 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -95,22 +95,8 @@ static const char *transaction_attrs = {
>  	"task-clock,"
>  	"{"
>  	"instructions,"
> -	"cycles,"
> -	"cpu/cycles-t/,"
> -	"cpu/tx-start/,"
> -	"cpu/el-start/,"
> -	"cpu/cycles-ct/"
> -	"}"
> -};
> -
> -/* More limited version when the CPU does not have all events. */
> -static const char * transaction_limited_attrs = {
> -	"task-clock,"
> -	"{"
> -	"instructions,"
> -	"cycles,"
> -	"cpu/cycles-t/,"
> -	"cpu/tx-start/"
> +	"cycles"
> +	"%s"
>  	"}"
>  };
>  
> @@ -2149,13 +2135,123 @@ __weak void arch_topdown_group_warn(void)
>  {
>  }
>  
> +struct pmu_tx_events {			/* Define transaction counters */
> +	const char *pmu;		/* PMU name */
> +	const char *name;		/* Counter name */
> +};
> +
> +static struct pmu_tx_events x86_tx_events[] = {/* x86 transaction counters */
> +	{
> +		.pmu = "cpu",
> +		.name = "cycles-t",
> +	},
> +	{
> +		.pmu = "cpu",
> +		.name = "tx-start",
> +	},
> +	{
> +		.pmu = "cpu",
> +		.name = "el-start",
> +	},
> +	{
> +		.pmu = "cpu",
> +		.name = "cycles-ct",
> +	},
> +	{
> +		.pmu = 0
> +	}
> +};
> +
> +static struct pmu_tx_events s390_tx_events[] = {/* s390 transaction counters */
> +	{
> +		.pmu = "cpum_cf",
> +		.name = "TX_C_TABORT_NO_SPECIAL",
> +	},
> +	{
> +		.pmu = "cpum_cf",
> +		.name = "TX_C_TABORT_SPECIAL",
> +	},
> +	{
> +		.pmu = "cpum_cf",
> +		.name = "TX_C_TEND",
> +	},
> +	{
> +		.pmu = "cpum_cf",
> +		.name = "TX_NC_TABORT",
> +	},
> +	{
> +		.pmu = "cpum_cf",
> +		.name = "TX_NC_TEND",
> +	},
> +	{
> +		.pmu = 0
> +	}
> +};
> +
> +struct arch_pmu_tx_events {
> +	const char *archname;		/* Architecture name */
> +	struct pmu_tx_events *tx;	/* Architecture specific counters */
> +} arch_pmu_tx_events[] = {
> +	{
> +		.archname = "s390",
> +		.tx = s390_tx_events
> +	},
> +	{
> +		.archname = "x86",
> +		.tx = x86_tx_events
> +	},
> +	{
> +		.archname = 0
> +	}
> +};
> +
> +static struct pmu_tx_events *pmu_tx_find_arch(const char *name)
> +{
> +	struct arch_pmu_tx_events *p = arch_pmu_tx_events;
> +
> +	for (; p->archname; ++p)
> +		if (!strcmp(name, p->archname))
> +			return p->tx;
> +	return NULL;
> +}
> +
> +/* Search the list of transaction events and test if they are valid for
> + * this PMU. The events are named cpu/cycles-t/ or cpum_cf/TX_NC_TEND/
> + * that is the PMU name followed by the event name surrounded by slashes.
> + *
> + * The function returns a string which contains the tested (and existing)
> + * PMU transaction events. The caller must free this string.
> + *
> + * If no PMU transaction events are found, a NULL pointer is returned.
> + */
> +static char *build_tx_string(const char *fmt, struct pmu_tx_events *txp)
> +{
> +	struct pmu_tx_events *p = txp;
> +	char buffer[512], *result;
> +	int rc, i = 0;
> +
> +	memset(buffer, 0, sizeof(buffer));
> +	for (p = txp; p->pmu; ++p) {
> +		if (pmu_have_event(p->pmu, p->name)) {
> +			rc = snprintf(buffer + i, sizeof(buffer) - i, ",%s/%s/",
> +				      p->pmu, p->name);

Please use scnprintf(), we've been using it instead of snprintf due to
this in its man page:

RETURN VALUE
       The  functions snprintf() and vsnprintf() do not write more than size bytes (including the terminating null byte ('\0')).  If
       the output was truncated due to this limit, then the return value is the number of characters (excluding the terminating null
       byte)  which would have been written to the final string if enough space had been available.  Thus, a return value of size or
       more means that the output was truncated.

---

It doesn't return the number of characters printed, but the number of
characters that would be printed if there was space, so that you can
check for truncation.

> +			i += rc;
> +			if (i >= (int)sizeof(buffer))
> +				break;
> +		}
> +	}
> +	if (i)			/* Transaction counters found */
> +		return asprintf(&result, fmt, buffer) < 0 ? NULL : result;
> +	return NULL;
> +}
> +
>  /*
>   * Add default attributes, if there were no attributes specified or
>   * if -d/--detailed, -d -d or -d -d -d is used:
>   */
>  static int add_default_attributes(void)
>  {
> -	int err;
> +	int err = -1;
>  	struct perf_event_attr default_attrs0[] = {
>  
>    { .type = PERF_TYPE_SOFTWARE, .config = PERF_COUNT_SW_TASK_CLOCK		},
> @@ -2274,20 +2370,32 @@ static int add_default_attributes(void)
>  		return 0;
>  
>  	if (transaction_run) {
> -		struct parse_events_error errinfo;
> +		struct parse_events_error errinfo = { .str = NULL };
> +		char *tx_str;
> +		struct pmu_tx_events *tx_archp;
>  
> -		if (pmu_have_event("cpu", "cycles-ct") &&
> -		    pmu_have_event("cpu", "el-start"))
> -			err = parse_events(evsel_list, transaction_attrs,
> -					   &errinfo);
> -		else
> -			err = parse_events(evsel_list,
> -					   transaction_limited_attrs,
> -					   &errinfo);
> -		if (err) {
> +		/* Find architecture transaction counter string table */
> +		tx_archp = pmu_tx_find_arch(perf_env__arch(NULL));
> +		if (!tx_archp) {
> +			fprintf(stderr, "Cannot set up transaction events\n");
> +			return -1;
> +		}
> +
> +		/* Build architecture transaction counter string */
> +		tx_str = build_tx_string(transaction_attrs, tx_archp);
> +		if (!tx_str) {
>  			fprintf(stderr, "Cannot set up transaction events\n");
>  			return -1;
>  		}
> +
> +		err = parse_events(evsel_list, tx_str, &errinfo);
> +		free(tx_str);
> +		if (err) {
> +			fprintf(stderr, "Cannot set up transaction events:%s\n",
> +				errinfo.str);

Can't we use the parse_events_print_error() function here? Or why is it
better to do it this way (using errinfo.str)?

> +			free(errinfo.str);
> +			return -1;
> +		}
>  		return 0;
>  	}
>  
> -- 
> 2.14.3

Re: [PATCH] perf stat: Make function perf_stat_evsel_id_init static

[Arnaldo Carvalho de Melo]

Em Mon, Mar 12, 2018 at 11:38:07AM +0100, Thomas Richter escreveu:
> Function perf_stat_evsel_id_init() has global linkage
> but is only used in util/stat.c. Make it static.
> 
> Signed-off-by: Thomas Richter <tmricht@linux.vnet.ibm.com>

Thanks, applied.

- Arnaldo

Re: [PATCH] perf stat: Add support for s390 transaction counters

[Andi Kleen]

Thomas Richter <tmricht@linux.vnet.ibm.com> writes:

> Right now there is only hard coded support for x86.

That's not true. There is support for generic transaction events in perf.

As far as I can tell your events would map 1:1 to the generic tx-* events.

-Andi

Re: [PATCH] perf stat: Add support for s390 transaction counters

[Thomas-Mich Richter]

On 03/13/2018 04:23 AM, Andi Kleen wrote:
> Thomas Richter <tmricht@linux.vnet.ibm.com> writes:
> 
>> Right now there is only hard coded support for x86.
> 
> That's not true. There is support for generic transaction events in perf.
> 
> As far as I can tell your events would map 1:1 to the generic tx-* events.
> 
> -Andi
> 

I might be wrong, but when I look at function add_default_attributes()
in file buildin-stat.c the string variables transaction_attrs
and transaction_limited_attrs are used when flag T is specified on command line:

/* Default events used for perf stat -T */
static const char *transaction_attrs = {
        "task-clock,"
        "{"
        "instructions,"
        "cycles,"
        "cpu/cycles-t/,"
        "cpu/tx-start/,"
        "cpu/el-start/,"
        "cpu/cycles-ct/"
        "}"
};

These PMU events show up on my x86 notebook but no on the s390.
That's why I came to this conclusion. I have not tried other architectures.
-- 
Thomas Richter, Dept 3303, IBM LTC Boeblingen Germany
--
Vorsitzende des Aufsichtsrats: Martina Koederitz 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH] perf stat: Add support for s390 transaction counters

[Arnaldo Carvalho de Melo]

Em Wed, Mar 14, 2018 at 09:34:48AM +0100, Thomas-Mich Richter escreveu:
> On 03/13/2018 04:23 AM, Andi Kleen wrote:
> > Thomas Richter <tmricht@linux.vnet.ibm.com> writes:

> >> Right now there is only hard coded support for x86.

> > That's not true. There is support for generic transaction events in perf.

> > As far as I can tell your events would map 1:1 to the generic tx-* events.
 
> I might be wrong, but when I look at function add_default_attributes()
> in file buildin-stat.c the string variables transaction_attrs
> and transaction_limited_attrs are used when flag T is specified on command line:
 
> /* Default events used for perf stat -T */
> static const char *transaction_attrs = {
>         "task-clock,"
>         "{"
>         "instructions,"
>         "cycles,"
>         "cpu/cycles-t/,"
>         "cpu/tx-start/,"
>         "cpu/el-start/,"
>         "cpu/cycles-ct/"
>         "}"
> };
 
> These PMU events show up on my x86 notebook but no on the s390.
> That's why I came to this conclusion. I have not tried other architectures.

So, I think Andi is saying that the s/390 kernel should map the generic
transaction events (cpu/cycles-t/, cpu/tx-start/, etc) to the events you
want to make 'perf stat -T' use, that way 'perf stat' doesn't have to be
changed, it will continue asking for the generic transaction event names
and then the kernel does the translation.

Just like we map "cycles" to different underlying events in different
architectures, right?

- Arnaldo

Re: [PATCH] perf stat: Add support for s390 transaction counters

[Thomas-Mich Richter]

On 03/14/2018 02:18 PM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Mar 14, 2018 at 09:34:48AM +0100, Thomas-Mich Richter escreveu:
>> On 03/13/2018 04:23 AM, Andi Kleen wrote:
>>> Thomas Richter <tmricht@linux.vnet.ibm.com> writes:
> 
>>>> Right now there is only hard coded support for x86.
> 
>>> That's not true. There is support for generic transaction events in perf.
> 
>>> As far as I can tell your events would map 1:1 to the generic tx-* events.
> 
>> I might be wrong, but when I look at function add_default_attributes()
>> in file buildin-stat.c the string variables transaction_attrs
>> and transaction_limited_attrs are used when flag T is specified on command line:
> 
>> /* Default events used for perf stat -T */
>> static const char *transaction_attrs = {
>>         "task-clock,"
>>         "{"
>>         "instructions,"
>>         "cycles,"
>>         "cpu/cycles-t/,"
>>         "cpu/tx-start/,"
>>         "cpu/el-start/,"
>>         "cpu/cycles-ct/"
>>         "}"
>> };
> 
>> These PMU events show up on my x86 notebook but no on the s390.
>> That's why I came to this conclusion. I have not tried other architectures.
> 
> So, I think Andi is saying that the s/390 kernel should map the generic
> transaction events (cpu/cycles-t/, cpu/tx-start/, etc) to the events you
> want to make 'perf stat -T' use, that way 'perf stat' doesn't have to be
> changed, it will continue asking for the generic transaction event names
> and then the kernel does the translation.
> 
> Just like we map "cycles" to different underlying events in different
> architectures, right?
> 
> - Arnaldo

S390 has no support for Elision and uses transaction begin/end/abort
instructions. The CPU measurement counter facility provides counters for
transaction end and transaction abort.

This means s390 counter facility device driver in arch/s390/kernel/perf_cpum_cf.c
could support the tx_abort and tx_commit symbolic counter names.

I have used this table (taken from arch/x86/events/intel/core.c) as giudeline:
/* Haswell special events */
EVENT_ATTR_STR(tx-start,        tx_start,       "event=0xc9,umask=0x1");
EVENT_ATTR_STR(tx-commit,       tx_commit,      "event=0xc9,umask=0x2");
EVENT_ATTR_STR(tx-abort,        tx_abort,       "event=0xc9,umask=0x4");
EVENT_ATTR_STR(tx-capacity,     tx_capacity,    "event=0x54,umask=0x2");
EVENT_ATTR_STR(tx-conflict,     tx_conflict,    "event=0x54,umask=0x1");
EVENT_ATTR_STR(el-start,        el_start,       "event=0xc8,umask=0x1");
EVENT_ATTR_STR(el-commit,       el_commit,      "event=0xc8,umask=0x2");
EVENT_ATTR_STR(el-abort,        el_abort,       "event=0xc8,umask=0x4");
EVENT_ATTR_STR(el-capacity,     el_capacity,    "event=0x54,umask=0x2");
EVENT_ATTR_STR(el-conflict,     el_conflict,    "event=0x54,umask=0x1");
EVENT_ATTR_STR(cycles-t,        cycles_t,       "event=0x3c,in_tx=1");
EVENT_ATTR_STR(cycles-ct,       cycles_ct,      "event=0x3c,in_tx=1,in_tx_cp=1");


So s390 can only support tx_commit and tx-abort symbolic names.
None of them show up in the transactions_attrs and transaction_limited_attrs
string variables used in builtin-stat.c file.

That is the reason why I introduced the patch set v2.
-- 
Thomas Richter, Dept 3303, IBM LTC Boeblingen Germany
--
Vorsitzende des Aufsichtsrats: Martina Koederitz 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH] perf stat: Add support for s390 transaction counters

[Andi Kleen]

> S390 has no support for Elision and uses transaction begin/end/abort
> instructions. The CPU measurement counter facility provides counters for
> transaction end and transaction abort.

You don't need to implement the el-* events.

> I have used this table (taken from arch/x86/events/intel/core.c) as giudeline:
> /* Haswell special events */
> EVENT_ATTR_STR(tx-start,        tx_start,       "event=0xc9,umask=0x1");
> EVENT_ATTR_STR(tx-commit,       tx_commit,      "event=0xc9,umask=0x2");
> EVENT_ATTR_STR(tx-abort,        tx_abort,       "event=0xc9,umask=0x4");
> EVENT_ATTR_STR(tx-capacity,     tx_capacity,    "event=0x54,umask=0x2");
> EVENT_ATTR_STR(tx-conflict,     tx_conflict,    "event=0x54,umask=0x1");
> EVENT_ATTR_STR(el-start,        el_start,       "event=0xc8,umask=0x1");
> EVENT_ATTR_STR(el-commit,       el_commit,      "event=0xc8,umask=0x2");
> EVENT_ATTR_STR(el-abort,        el_abort,       "event=0xc8,umask=0x4");
> EVENT_ATTR_STR(el-capacity,     el_capacity,    "event=0x54,umask=0x2");
> EVENT_ATTR_STR(el-conflict,     el_conflict,    "event=0x54,umask=0x1");
> EVENT_ATTR_STR(cycles-t,        cycles_t,       "event=0x3c,in_tx=1");
> EVENT_ATTR_STR(cycles-ct,       cycles_ct,      "event=0x3c,in_tx=1,in_tx_cp=1");
> 
> 
> So s390 can only support tx_commit and tx-abort symbolic names.
> None of them show up in the transactions_attrs and transaction_limited_attrs
> string variables used in builtin-stat.c file.

We could change perf stat to fall back to only tx commit and tx abort.
We already did that for one limited case.

-Andi

Re: [PATCH] perf stat: Add support for s390 transaction counters

[Hendrik Brueckner]

Hi,

On Wed, Mar 14, 2018 at 08:43:17AM -0700, Andi Kleen wrote:
> > S390 has no support for Elision and uses transaction begin/end/abort
> > instructions. The CPU measurement counter facility provides counters for
> > transaction end and transaction abort.
> 
> You don't need to implement the el-* events.
> 
> > I have used this table (taken from arch/x86/events/intel/core.c) as giudeline:
> > /* Haswell special events */
> > EVENT_ATTR_STR(tx-start,        tx_start,       "event=0xc9,umask=0x1");
> > EVENT_ATTR_STR(tx-commit,       tx_commit,      "event=0xc9,umask=0x2");
> > EVENT_ATTR_STR(tx-abort,        tx_abort,       "event=0xc9,umask=0x4");
> > EVENT_ATTR_STR(tx-capacity,     tx_capacity,    "event=0x54,umask=0x2");
> > EVENT_ATTR_STR(tx-conflict,     tx_conflict,    "event=0x54,umask=0x1");
> > EVENT_ATTR_STR(el-start,        el_start,       "event=0xc8,umask=0x1");
> > EVENT_ATTR_STR(el-commit,       el_commit,      "event=0xc8,umask=0x2");
> > EVENT_ATTR_STR(el-abort,        el_abort,       "event=0xc8,umask=0x4");
> > EVENT_ATTR_STR(el-capacity,     el_capacity,    "event=0x54,umask=0x2");
> > EVENT_ATTR_STR(el-conflict,     el_conflict,    "event=0x54,umask=0x1");
> > EVENT_ATTR_STR(cycles-t,        cycles_t,       "event=0x3c,in_tx=1");
> > EVENT_ATTR_STR(cycles-ct,       cycles_ct,      "event=0x3c,in_tx=1,in_tx_cp=1");
> > 
> > 
> > So s390 can only support tx_commit and tx-abort symbolic names.

In detail, for s390 we have:

cpum_cf/TX_C_TABORT_NO_SPECIAL/
cpum_cf/TX_C_TABORT_SPECIAL/
cpum_cf/TX_C_TEND/
cpum_cf/TX_NC_TABORT/
cpum_cf/TX_NC_TEND/

The mapping of the above is not that easy. As s390 have counters for
non-constraint (TC_NC_*) and contraint (TX_C_*) transaction commits (TEND)
and aborts (TABORTs).

> We could change perf stat to fall back to only tx commit and tx abort.
> We already did that for one limited case.

Displaying these different types for s390 is important from my point of
view.  Of course, I could create a mapping of TX_NC_TABORT/TX_NC_TEND
to tx-commit/tx-abort.  The remaining events would still appear to be specific
to the cpum_cf.

So I would propose to go with adding the cpum_cf/ specific ones first.
If necessary, they could go into the perf/arch/s390/ directory and included
in builtin-stat.  I put a todo on my list to provide at least a
tx-commit/abort for the nonconstraint transactions. (The other would still be
specific).

Kind regards,
  Hendrik

-- 
Hendrik Brueckner
brueckner@linux.vnet.ibm.com      | IBM Deutschland Research & Development GmbH
Linux on z Systems Development    | Schoenaicher Str. 220, 71032 Boeblingen


IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH] perf stat: Add support for s390 transaction counters

[Andi Kleen]

> Displaying these different types for s390 is important from my point of
> view.  Of course, I could create a mapping of TX_NC_TABORT/TX_NC_TEND
> to tx-commit/tx-abort.  The remaining events would still appear to be specific
> to the cpum_cf.

If you want more generic metrics you can just use the new json generic metrics /
metric group support in the eventlist. In fact if we did it today I wouldn't
add perf stat -T at all, but just use that.

-Andi

[tip:perf/core] perf stat: Make function perf_stat_evsel_id_init static

[tip-bot for Thomas Richter]

Commit-ID:  26e4711fc8352252f474a02429d7495652c4aef7
Gitweb:     https://git.kernel.org/tip/26e4711fc8352252f474a02429d7495652c4aef7
Author:     Thomas Richter <tmricht@linux.vnet.ibm.com>
AuthorDate: Mon, 12 Mar 2018 11:38:07 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Fri, 16 Mar 2018 13:56:17 -0300

perf stat: Make function perf_stat_evsel_id_init static

Function perf_stat_evsel_id_init() has global linkage but is only used
in util/stat.c. Make it static.

Signed-off-by: Thomas Richter <tmricht@linux.vnet.ibm.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Link: http://lkml.kernel.org/r/20180312103807.45069-2-tmricht@linux.vnet.ibm.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/stat.c | 2 +-
 tools/perf/util/stat.h | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/tools/perf/util/stat.c b/tools/perf/util/stat.c
index 32235657c1ac..a0061e0b0fad 100644
--- a/tools/perf/util/stat.c
+++ b/tools/perf/util/stat.c
@@ -92,7 +92,7 @@ static const char *id_str[PERF_STAT_EVSEL_ID__MAX] = {
 };
 #undef ID
 
-void perf_stat_evsel_id_init(struct perf_evsel *evsel)
+static void perf_stat_evsel_id_init(struct perf_evsel *evsel)
 {
 	struct perf_stat_evsel *ps = evsel->stats;
 	int i;
diff --git a/tools/perf/util/stat.h b/tools/perf/util/stat.h
index 2f44e386a0e8..8f56ba4fd258 100644
--- a/tools/perf/util/stat.h
+++ b/tools/perf/util/stat.h
@@ -128,8 +128,6 @@ bool __perf_evsel_stat__is(struct perf_evsel *evsel,
 #define perf_stat_evsel__is(evsel, id) \
 	__perf_evsel_stat__is(evsel, PERF_STAT_EVSEL_ID__ ## id)
 
-void perf_stat_evsel_id_init(struct perf_evsel *evsel);
-
 extern struct runtime_stat rt_stat;
 extern struct stats walltime_nsecs_stats;