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

[PATCH 1/3] 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 show 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>
---
 tools/perf/util/pmu.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 7878934ebb23..26c79a9c4142 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -294,7 +294,7 @@ static int __perf_pmu__new_alias(struct list_head *list, char *dir, char *name,
 
 static int perf_pmu__new_alias(struct list_head *list, char *dir, char *name, FILE *file)
 {
-	char buf[256];
+	char *cp, buf[256];
 	int ret;
 
 	ret = fread(buf, 1, sizeof(buf), file);
@@ -303,6 +303,11 @@ static int perf_pmu__new_alias(struct list_head *list, char *dir, char *name, FI
 
 	buf[ret] = 0;
 
+	/* Remove trailing newline from sysfs file */
+	cp = strrchr(buf, '\n');
+	if (cp)
+		*cp = '\0';
+
 	return __perf_pmu__new_alias(list, dir, name, NULL, buf, NULL, NULL, NULL,
 				     NULL, NULL, NULL);
 }
-- 
2.14.3

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

[Thomas Richter]

PMU alias definitions in sysfs files may have spaces, newlines
and number with leading zeroes. Same 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>
---
 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 26c79a9c4142..da8f243743d3 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] 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>
---
 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 da8f243743d3..a266da1db139 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] perf alias: Remove trailing newline when reading sysfs files

[Paul Clarke]

On 06/14/2018 06:48 AM, Thomas Richter wrote:
> Remove a trailing newline when reading sysfs file contents
> such as /sys/devices/cpum_cf/events/TX_NC_TEND.
> This show 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>
> ---
>  tools/perf/util/pmu.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 7878934ebb23..26c79a9c4142 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -294,7 +294,7 @@ static int __perf_pmu__new_alias(struct list_head *list, char *dir, char *name,
> 
>  static int perf_pmu__new_alias(struct list_head *list, char *dir, char *name, FILE *file)
>  {
> -	char buf[256];
> +	char *cp, buf[256];
>  	int ret;
> 
>  	ret = fread(buf, 1, sizeof(buf), file);
> @@ -303,6 +303,11 @@ static int perf_pmu__new_alias(struct list_head *list, char *dir, char *name, FI
> 
>  	buf[ret] = 0;
> 
> +	/* Remove trailing newline from sysfs file */
> +	cp = strrchr(buf, '\n');
> +	if (cp)
> +		*cp = '\0';

A nit, perhaps, but this will search backwards through the entire string if a newline is not found, which is the most common case, I presume.  Would it be more efficient to just look at the last character?  Something like:
i = strlen(buf);
if (i > 0 && buf[i-1] == '\n')
  buf[i-1] = '\0';

> +
>  	return __perf_pmu__new_alias(list, dir, name, NULL, buf, NULL, NULL, NULL,
>  				     NULL, NULL, NULL);
>  }
> 

PC

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

[Paul Clarke]

On 06/14/2018 06:48 AM, Thomas Richter wrote:
> PMU alias definitions in sysfs files may have spaces, newlines
> and number with leading zeroes. Same 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>
> ---
>  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 26c79a9c4142..da8f243743d3 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];

How was 256 chosen?

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

If we exceed 256, we just suddenly terminate the rebuilding without reporting any issues.

> +	}
> +
>  	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);
> 

PC

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

[Arnaldo Carvalho de Melo]

Em Thu, Jun 14, 2018 at 01:48:43PM +0200, Thomas Richter escreveu:
> Remove a trailing newline when reading sysfs file contents
<SNIP>
> +	/* Remove trailing newline from sysfs file */
> +	cp = strrchr(buf, '\n');
> +	if (cp)
> +		*cp = '\0';
> +

We have rtrim()

- Arnaldo

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

[Thomas-Mich Richter]

On 06/14/2018 03:17 PM, Paul Clarke wrote:
> On 06/14/2018 06:48 AM, Thomas Richter wrote:
>> Remove a trailing newline when reading sysfs file contents
>> such as /sys/devices/cpum_cf/events/TX_NC_TEND.
>> This show 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>
>> ---
>>  tools/perf/util/pmu.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
>> index 7878934ebb23..26c79a9c4142 100644
>> --- a/tools/perf/util/pmu.c
>> +++ b/tools/perf/util/pmu.c
>> @@ -294,7 +294,7 @@ static int __perf_pmu__new_alias(struct list_head *list, char *dir, char *name,
>>
>>  static int perf_pmu__new_alias(struct list_head *list, char *dir, char *name, FILE *file)
>>  {
>> -	char buf[256];
>> +	char *cp, buf[256];
>>  	int ret;
>>
>>  	ret = fread(buf, 1, sizeof(buf), file);
>> @@ -303,6 +303,11 @@ static int perf_pmu__new_alias(struct list_head *list, char *dir, char *name, FI
>>
>>  	buf[ret] = 0;
>>
>> +	/* Remove trailing newline from sysfs file */
>> +	cp = strrchr(buf, '\n');
>> +	if (cp)
>> +		*cp = '\0';
> 
> A nit, perhaps, but this will search backwards through the entire string if a newline is not found, which is the most common case, I presume.  Would it be more efficient to just look at the last character?  Something like:
> i = strlen(buf);
> if (i > 0 && buf[i-1] == '\n')
>   buf[i-1] = '\0';
> 
>> +
>>  	return __perf_pmu__new_alias(list, dir, name, NULL, buf, NULL, NULL, NULL,
>>  				     NULL, NULL, NULL);
>>  }
>>
> 
> PC
> 

Arnaldo suggested rtrim() which I will use.
I agree that your approach is a bit faster...



-- 
Thomas Richter, Dept 3303, IBM s390 Linux Development, Boeblingen, Germany
--
Vorsitzende des Aufsichtsrats: Martina Koederitz 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294

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

[Thomas-Mich Richter]

On 06/14/2018 03:53 PM, Paul Clarke wrote:
> On 06/14/2018 06:48 AM, Thomas Richter wrote:
>> PMU alias definitions in sysfs files may have spaces, newlines
>> and number with leading zeroes. Same 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>
>> ---
>>  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 26c79a9c4142..da8f243743d3 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];
> 
> How was 256 chosen?

I copied this from function perf_pmu__new_alias() which also uses:

char buf[256];

Looking a the sysfs file contents this seems to be sufficient.
This number comes from commit long time ago.

> 
>>
>>  	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);
> 
> If we exceed 256, we just suddenly terminate the rebuilding without reporting any issues.

Correct the string would be truncated, but see above, it would not have been read correctly
anyway.

> 
>> +	}
>> +
>>  	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);
>>
> 
> PC
> 

-- 
Thomas Richter, Dept 3303, IBM s390 Linux Development, Boeblingen, Germany
--
Vorsitzende des Aufsichtsrats: Martina Koederitz 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294

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

[Jiri Olsa]

On Thu, Jun 14, 2018 at 08:53:14AM -0500, Paul Clarke wrote:

SNIP

> > +		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);
> 
> If we exceed 256, we just suddenly terminate the rebuilding without reporting any issues.
> 
> > +	}
> > +
> >  	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);

hum, how is newval different from val? AFAICS it's the same

jirka

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

[Jiri Olsa]

On Thu, Jun 14, 2018 at 01:48:45PM +0200, Thomas Richter wrote:

SNIP

> +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;
> +}

ok, I like your change better.. we can rebuilt it to use
rb tree later when we have this fixed

thanks,
jirka

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

[Thomas-Mich Richter]

On 06/15/2018 10:21 AM, Jiri Olsa wrote:
> On Thu, Jun 14, 2018 at 01:48:45PM +0200, Thomas Richter wrote:
> 
> SNIP
> 
>> +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;
>> +}
> 
> ok, I like your change better.. we can rebuilt it to use
> rb tree later when we have this fixed
> 
> thanks,
> jirka
> 

Thanks for the review. I will resend the patch set later today when I have added
Arnaldo's comments.
After that we add your rb tree stuff.

-- 
Thomas Richter, Dept 3303, IBM s390 Linux Development, Boeblingen, Germany
--
Vorsitzende des Aufsichtsrats: Martina Koederitz 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294

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

[Thomas-Mich Richter]

On 06/15/2018 10:12 AM, Jiri Olsa wrote:
> On Thu, Jun 14, 2018 at 08:53:14AM -0500, Paul Clarke wrote:
> 
> SNIP
> 
>>> +		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);
>>
>> If we exceed 256, we just suddenly terminate the rebuilding without reporting any issues.
>>
>>> +	}
>>> +
>>>  	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);
> 
> hum, how is newval different from val? AFAICS it's the same
> 

Not really, depends on the platform, here is some debug output from s390:
root@s35lp76 perf]# ./perf stat -e tx_nc_tend -- ~/mytesttx 1 >/tmp/111

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

                 1      tx_nc_tend                                                  

       0.001050150 seconds time elapsed

[root@s35lp76 perf]# fgrep -i tx_nc_tend /tmp/111
__perf_pmu__new_alias TX_NC_TEND val:event=0x008d newval:event=0x8d
__perf_pmu__new_alias tx_nc_tend val:event=0x8d newval:event=0x8d
TX_NC_TEND	1 rc 8
[root@s35lp76 perf]# 

On s390 the events in the PMU sysfs file are printed with leading zeroes.
This means the strcmp() of alias->str differs, but the contents is logically
identical. (Same of some files contains spaces).

Thats why I do the rewrite of val into newval.

The alias name does not match too, but we use strcasecmp() to ignore case.

Hope this helps.
-- 
Thomas Richter, Dept 3303, IBM s390 Linux Development, Boeblingen, Germany
--
Vorsitzende des Aufsichtsrats: Martina Koederitz 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294

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

[David Laight]

From: Paul Clarke
> Sent: 14 June 2018 14:18
...
> > +	/* Remove trailing newline from sysfs file */
> > +	cp = strrchr(buf, '\n');
> > +	if (cp)
> > +		*cp = '\0';
> 
> A nit, perhaps, but this will search backwards through the entire string if a newline is not found,
> which is the most common case, I presume.  Would it be more efficient to just look at the last
> character?  Something like:
> i = strlen(buf);
> if (i > 0 && buf[i-1] == '\n')
>   buf[i-1] = '\0';

Worse it will do horrid things of the output has multiple lines of text
without a newline at the end.

Both strlen() and strrhr() need to scan the entire string.
However since strlen is only looking for one value it should be much
more efficient - especially on 64bit systems where shifts and bit
operations can be used to find the 64bit word containing the first
zero byte.

I suspect rtrim() has an extra data cache miss.

	David

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

[Jiri Olsa]

On Fri, Jun 15, 2018 at 11:09:05AM +0200, Thomas-Mich Richter wrote:
> On 06/15/2018 10:12 AM, Jiri Olsa wrote:
> > On Thu, Jun 14, 2018 at 08:53:14AM -0500, Paul Clarke wrote:
> > 
> > SNIP
> > 
> >>> +		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);
> >>
> >> If we exceed 256, we just suddenly terminate the rebuilding without reporting any issues.
> >>
> >>> +	}
> >>> +
> >>>  	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);
> > 
> > hum, how is newval different from val? AFAICS it's the same
> > 
> 
> Not really, depends on the platform, here is some debug output from s390:
> root@s35lp76 perf]# ./perf stat -e tx_nc_tend -- ~/mytesttx 1 >/tmp/111
> 
>  Performance counter stats for '/root/mytesttx 1':
> 
>                  1      tx_nc_tend                                                  
> 
>        0.001050150 seconds time elapsed
> 
> [root@s35lp76 perf]# fgrep -i tx_nc_tend /tmp/111
> __perf_pmu__new_alias TX_NC_TEND val:event=0x008d newval:event=0x8d
> __perf_pmu__new_alias tx_nc_tend val:event=0x8d newval:event=0x8d
> TX_NC_TEND	1 rc 8
> [root@s35lp76 perf]# 
> 
> On s390 the events in the PMU sysfs file are printed with leading zeroes.
> This means the strcmp() of alias->str differs, but the contents is logically
> identical. (Same of some files contains spaces).
> 
> Thats why I do the rewrite of val into newval.
> 
> The alias name does not match too, but we use strcasecmp() to ignore case.
> 
> Hope this helps.

yep.. should have read the full change log ;-) thanks

jirka

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

[Arnaldo Carvalho de Melo]

Em Fri, Jun 15, 2018 at 10:21:24AM +0200, Jiri Olsa escreveu:
> On Thu, Jun 14, 2018 at 01:48:45PM +0200, Thomas Richter wrote:
> 
> SNIP
> 
> > +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;
> > +}
> 
> ok, I like your change better.. we can rebuilt it to use
> rb tree later when we have this fixed

Ok, can I take this as an Acked-by or Reviewed-by?

- Arnaldo

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

[Jiri Olsa]

On Tue, Jun 19, 2018 at 12:17:06PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Jun 15, 2018 at 10:21:24AM +0200, Jiri Olsa escreveu:
> > On Thu, Jun 14, 2018 at 01:48:45PM +0200, Thomas Richter wrote:
> > 
> > SNIP
> > 
> > > +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;
> > > +}
> > 
> > ok, I like your change better.. we can rebuilt it to use
> > rb tree later when we have this fixed
> 
> Ok, can I take this as an Acked-by or Reviewed-by?

yes

jirka