[PATCH 1/3 v2] perf alias: Remove trailing newline when reading sysfs files

[PATCH 1/3 v2] perf alias: Remove trailing newline when reading sysfs files

[Thomas Richter]

Remove a trailing newline when reading sysfs file contents
such as /sys/devices/cpum_cf/events/TX_NC_TEND.
This shows when verbose option -v is used.

Output before:
tx_nc_tend -> 'cpum_cf'/'event=0x008d
'/

Output after:
tx_nc_tend -> 'cpum_cf'/'event=0x8d'/

Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
Reviewed-by: Hendrik Brueckner <brueckner@linux.ibm.com>
Cc: Jiri Olsa <jolsa@redhat.com>
---
 tools/perf/util/pmu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 7878934ebb23..6d2012405f2b 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -303,6 +303,9 @@ static int perf_pmu__new_alias(struct list_head *list, char *dir, char *name, FI
 
 	buf[ret] = 0;
 
+	/* Remove trailing newline from sysfs file */
+	rtrim(buf);
+
 	return __perf_pmu__new_alias(list, dir, name, NULL, buf, NULL, NULL, NULL,
 				     NULL, NULL, NULL);
 }
-- 
2.14.3

[PATCH 2/3 v2] perf alias: Rebuild alias expression string to make it comparable

[Thomas Richter]

PMU alias definitions in sysfs files may have spaces, newlines
and numbers with leading zeroes. Some alias definitions may
also appear in JSON files without spaces, etc.

Scan alias definitions and remove leading zeroes, spaces,
newlines, etc and rebuild string to make alias->str member
comparable.

s390 for example  has terms specified as
event=0x0091 (read from files ../<PMU>/events/<FILE>
and terms specified as event=0x91 (read from JSON files).

Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
Reviewed-by: Hendrik Brueckner <brueckner@linux.ibm.com>
Cc: Jiri Olsa <jolsa@redhat.com>
---
 tools/perf/util/pmu.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 6d2012405f2b..208e427dc038 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -241,9 +241,11 @@ static int __perf_pmu__new_alias(struct list_head *list, char *dir, char *name,
 				 char *metric_expr,
 				 char *metric_name)
 {
+	struct parse_events_term *term;
 	struct perf_pmu_alias *alias;
 	int ret;
 	int num;
+	char newval[256];
 
 	alias = malloc(sizeof(*alias));
 	if (!alias)
@@ -262,6 +264,27 @@ static int __perf_pmu__new_alias(struct list_head *list, char *dir, char *name,
 		return ret;
 	}
 
+	/* Scan event and remove leading zeroes, spaces, newlines, some
+	 * platforms have terms specified as
+	 * event=0x0091 (read from files ../<PMU>/events/<FILE>
+	 * and terms specified as event=0x91 (read from JSON files).
+	 *
+	 * Rebuild string to make alias->str member comparable.
+	 */
+	memset(newval, 0, sizeof(newval));
+	ret = 0;
+	list_for_each_entry(term, &alias->terms, list) {
+		if (ret)
+			ret += scnprintf(newval + ret, sizeof(newval) - ret,
+					 ",");
+		if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM)
+			ret += scnprintf(newval + ret, sizeof(newval) - ret,
+					 "%s=%#x", term->config, term->val.num);
+		else if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR)
+			ret += scnprintf(newval + ret, sizeof(newval) - ret,
+					 "%s=%s", term->config, term->val.str);
+	}
+
 	alias->name = strdup(name);
 	if (dir) {
 		/*
@@ -285,7 +308,7 @@ static int __perf_pmu__new_alias(struct list_head *list, char *dir, char *name,
 		snprintf(alias->unit, sizeof(alias->unit), "%s", unit);
 	}
 	alias->per_pkg = perpkg && sscanf(perpkg, "%d", &num) == 1 && num == 1;
-	alias->str = strdup(val);
+	alias->str = strdup(newval);
 
 	list_add_tail(&alias->list, list);
 
-- 
2.14.3

[PATCH 3/3 v2] perf stat: Remove duplicate event counting

[Thomas Richter]

Perf stat shows a mismatch in perf stat regarding counter names
on s390:

Run command
   [root@s35lp76 perf]# ./perf stat -e tx_nc_tend  -v --
                ~/mytesttx 1 >/tmp/111
   tx_nc_tend: 1 573146 573146
   tx_nc_tend: 1 573146 573146

   Performance counter stats for '/root/mytesttx 1':

                 3      tx_nc_tend

       0.001037252 seconds time elapsed

   [root@s35lp76 perf]#

shows transaction counter tx_nc_tend with value 3 but it
was triggered only once as seen by the output of mytesttx.

When looking up the event name tx_nc_tend the following
function sequence is called:

parse_events_multi_pmu_add()
+--> perf_pmu__scan() being called with NULL argument
     +--> pmu_read_sysfs() scans directory ../devices/ for
                           all PMUs
          +--> perf_pmu__find() tries to find a PMU in the
                           global pmu list.
               +--> pmu_lookup() called to read all file
                                 entries when not in global
                                 list.

pmu_lookup() causes the issue. It calls
+---> pmu_aliases() to read all the entries in the PMU directory.
                    On s390 this is named
                    /sys/devices/cpum_cf/events.
      +--> pmu_aliases_parse() reads all files and creates an
                       alias for each file name.

                       So we end up with first entry created by
                       reading the sysfs file
                       [root@s35lp76 perf]# cat /sys/devices/cpum_cf
                                                /events/TX_NC_TEND
                       event=0x008d
                       [root@s35lp76 perf]#

                       Debug output shows this entry
                       tx_nc_tend -> 'cpum_cf'/'event=0x008d
                       '/
                       After all files in this directory have been
                       read and aliases created this function is called:
      +--> pmu_add_cpu_aliases()
                       This function looks up the CPU tables
                       created by the json files.
                       With json files for s390 now available all
                       the aliases are added to
                       the PMU alias list a second time.
                       The second entry is added by
                       reading the json file converted by jevent
                       resulting in file pmu-events/pmu-events.c:

                       {
                         .name = "tx_nc_tend",
                         .event = "event=0x8d",
                         .desc = "Unit: cpum_cf Completed TEND \
                                  instructions \
                                  in non-constrained TX mode",
                         .topic = "extended",
                         .long_desc = "A TEND instruction has \
                                       completed  in a \
                                       non-constrained \
                                       transactional-execution mode",
                         .pmu = "cpum_cf",
                        },

                        Debug output shows this entry
                        tx_nc_tend -> 'cpum_cf'/'event=0x8d'/

Function pmu_aliases_parse() and pmu_add_cpu_aliases()
both use __perf_pmu__new_alias() to add an alias to the
PMU alias list. There is no check if an alias already exist

So we end up with 2 entries for tx_nc_tend in the
PMU alias list.

Having set up the PMU alias list for this PMU now
parse_events_multi_add_pmu() reads the complete alias
list and adds each alias with parse_events_add_pmu()
to the global perfev_list.  This causes the alias to
be added multiple times to the event list.

Fix this by making __perf_pmu__new_alias()
to merge alias definitions if an alias is already on the
alias list.
Also print a debug message when the alias has mismatches
in some fields.

Output before:
[root@s35lp76 perf]# ./perf stat -e tx_nc_tend  -v \
                      -- ~/mytesttx 1 >/tmp/111
tx_nc_tend: 1 551446 551446

 Performance counter stats for '/root/mytesttx 1':

                 3      tx_nc_tend

       0.000961134 seconds time elapsed

[root@s35lp76 perf]#

Output after:
[root@s35lp76 perf]#  ./perf stat -e tx_nc_tend  -v \
                      -- ~/mytesttx 1 >/tmp/111
tx_nc_tend: 1 551446 551446

 Performance counter stats for '/root/mytesttx 1':

                 1      tx_nc_tend

       0.000961134 seconds time elapsed

[root@s35lp76 perf]#

Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
Reviewed-by: Hendrik Brueckner <brueckner@linux.ibm.com>
Cc: Jiri Olsa <jolsa@redhat.com>
---
 tools/perf/util/pmu.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 70 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 208e427dc038..afd68524ffa9 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -234,6 +234,74 @@ static int perf_pmu__parse_snapshot(struct perf_pmu_alias *alias,
 	return 0;
 }
 
+static void perf_pmu_assign_str(char *name, const char *field, char **old_str,
+				char **new_str)
+{
+	if (!*old_str)
+		goto set_new;
+
+	if (*new_str) {	/* Have new string, check with old */
+		if (strcasecmp(*old_str, *new_str))
+			pr_debug("alias %s differs in field '%s'\n",
+				 name, field);
+		zfree(old_str);
+	} else		/* Nothing new --> keep old string */
+		return;
+set_new:
+	*old_str = *new_str;
+	*new_str = NULL;
+}
+
+static void perf_pmu_update_alias(struct perf_pmu_alias *old,
+				  struct perf_pmu_alias *newalias)
+{
+	perf_pmu_assign_str(old->name, "desc", &old->desc, &newalias->desc);
+	perf_pmu_assign_str(old->name, "long_desc", &old->long_desc,
+			    &newalias->long_desc);
+	perf_pmu_assign_str(old->name, "topic", &old->topic, &newalias->topic);
+	perf_pmu_assign_str(old->name, "metric_expr", &old->metric_expr,
+			    &newalias->metric_expr);
+	perf_pmu_assign_str(old->name, "metric_name", &old->metric_name,
+			    &newalias->metric_name);
+	perf_pmu_assign_str(old->name, "value", &old->str, &newalias->str);
+	old->scale = newalias->scale;
+	old->per_pkg = newalias->per_pkg;
+	old->snapshot = newalias->snapshot;
+	memcpy(old->unit, newalias->unit, sizeof(old->unit));
+}
+
+/* Delete an alias entry. */
+static void perf_pmu_free_alias(struct perf_pmu_alias *newalias)
+{
+	zfree(&newalias->name);
+	zfree(&newalias->desc);
+	zfree(&newalias->long_desc);
+	zfree(&newalias->topic);
+	zfree(&newalias->str);
+	zfree(&newalias->metric_expr);
+	zfree(&newalias->metric_name);
+	parse_events_terms__purge(&newalias->terms);
+	free(newalias);
+}
+
+/* Merge an alias, search in alias list. If this name is already
+ * present merge both of them to combine all information.
+ */
+static bool perf_pmu_merge_alias(struct perf_pmu_alias *newalias,
+				 struct list_head *alist)
+{
+	struct perf_pmu_alias *a;
+
+	list_for_each_entry(a, alist, list) {
+		if (!strcasecmp(newalias->name, a->name)) {
+			perf_pmu_update_alias(a, newalias);
+			perf_pmu_free_alias(newalias);
+			return true;
+		}
+	}
+	return false;
+}
+
 static int __perf_pmu__new_alias(struct list_head *list, char *dir, char *name,
 				 char *desc, char *val,
 				 char *long_desc, char *topic,
@@ -310,7 +378,8 @@ static int __perf_pmu__new_alias(struct list_head *list, char *dir, char *name,
 	alias->per_pkg = perpkg && sscanf(perpkg, "%d", &num) == 1 && num == 1;
 	alias->str = strdup(newval);
 
-	list_add_tail(&alias->list, list);
+	if (!perf_pmu_merge_alias(alias, list))
+		list_add_tail(&alias->list, list);
 
 	return 0;
 }
-- 
2.14.3

Re: [PATCH 1/3 v2] perf alias: Remove trailing newline when reading sysfs files

[Arnaldo Carvalho de Melo]

Em Fri, Jun 15, 2018 at 12:11:03PM +0200, Thomas Richter escreveu:
> Remove a trailing newline when reading sysfs file contents
> such as /sys/devices/cpum_cf/events/TX_NC_TEND.
> This shows when verbose option -v is used.
> 
> Output before:
> tx_nc_tend -> 'cpum_cf'/'event=0x008d
> '/

Thanks, applied.

- Arnaldo

Re: [PATCH 2/3 v2] perf alias: Rebuild alias expression string to make it comparable

[Arnaldo Carvalho de Melo]

Em Fri, Jun 15, 2018 at 12:11:04PM +0200, Thomas Richter escreveu:
> PMU alias definitions in sysfs files may have spaces, newlines
> and numbers with leading zeroes. Some alias definitions may
> also appear in JSON files without spaces, etc.
> 
> Scan alias definitions and remove leading zeroes, spaces,
> newlines, etc and rebuild string to make alias->str member
> comparable.
> 
> s390 for example  has terms specified as
> event=0x0091 (read from files ../<PMU>/events/<FILE>
> and terms specified as event=0x91 (read from JSON files).

Applying, I think Jiri agreed with this one in the v1 post, right?

- Arnaldo