[PATCH 1/5] perf pmu: Fix core PMU alias list for X86 platform

[PATCH 1/5] perf pmu: Fix core PMU alias list for X86 platform

[kan.liang]

From: Kan Liang <kan.liang@linux.intel.com>

When counting uncore event with alias, core event is mistakenly
involved, for example:

  perf stat --no-merge -e "unc_m_cas_count.all" -C0  sleep 1

  Performance counter stats for 'CPU(s) 0':

                 0      unc_m_cas_count.all [uncore_imc_4]
                 0      unc_m_cas_count.all [uncore_imc_2]
                 0      unc_m_cas_count.all [uncore_imc_0]
           153,640      unc_m_cas_count.all [cpu]
                 0      unc_m_cas_count.all [uncore_imc_5]
            25,026      unc_m_cas_count.all [uncore_imc_3]
                 0      unc_m_cas_count.all [uncore_imc_1]

       1.001447890 seconds time elapsed

The reason is that current implementation doesn't check PMU name of a
event when adding its alias into the alias list for core PMU. The
uncore event aliases are mistakenly added.

This bug was introduced in:
  commit 14b22ae028de ("perf pmu: Add helper function is_pmu_core to
  detect PMU CORE devices")

Checking the PMU name for all PMUs on X86 and other architectures except
ARM.
There is no behavior change for ARM.

Fixes: 14b22ae028de ("perf pmu: Add helper function is_pmu_core to detect PMU CORE devices")
Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
---
 tools/perf/util/pmu.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 064bdcb..0df2a02 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -539,9 +539,10 @@ static bool pmu_is_uncore(const char *name)
 
 /*
  *  PMU CORE devices have different name other than cpu in sysfs on some
- *  platforms. looking for possible sysfs files to identify as core device.
+ *  platforms.
+ *  Looking for possible sysfs files to identify the arm core device.
  */
-static int is_pmu_core(const char *name)
+static int is_arm_pmu_core(const char *name)
 {
 	struct stat st;
 	char path[PATH_MAX];
@@ -550,12 +551,6 @@ static int is_pmu_core(const char *name)
 	if (!sysfs)
 		return 0;
 
-	/* Look for cpu sysfs (x86 and others) */
-	scnprintf(path, PATH_MAX, "%s/bus/event_source/devices/cpu", sysfs);
-	if ((stat(path, &st) == 0) &&
-			(strncmp(name, "cpu", strlen("cpu")) == 0))
-		return 1;
-
 	/* Look for cpu sysfs (specific to arm) */
 	scnprintf(path, PATH_MAX, "%s/bus/event_source/devices/%s/cpus",
 				sysfs, name);
@@ -662,6 +657,7 @@ static void pmu_add_cpu_aliases(struct list_head *head, struct perf_pmu *pmu)
 	struct pmu_events_map *map;
 	struct pmu_event *pe;
 	const char *name = pmu->name;
+	const char *pname;
 
 	map = perf_pmu__find_map(pmu);
 	if (!map)
@@ -680,11 +676,9 @@ static void pmu_add_cpu_aliases(struct list_head *head, struct perf_pmu *pmu)
 			break;
 		}
 
-		if (!is_pmu_core(name)) {
-			/* check for uncore devices */
-			if (pe->pmu == NULL)
-				continue;
-			if (strncmp(pe->pmu, name, strlen(pe->pmu)))
+		if (!is_arm_pmu_core(name)) {
+			pname = pe->pmu ? pe->pmu : "cpu";
+			if (strncmp(pname, name, strlen(pname)))
 				continue;
 		}
 
-- 
2.7.4

[PATCH 2/5] perf stat: Print out hint for mixed PMU group error

[kan.liang]

From: Kan Liang <kan.liang@linux.intel.com>

Perf doesn't support mixed events from different PMUs (except software
event) in a group. For this case, only "<not counted>" or "<not
supported>" are printed out. There is no hint which guides users to fix
the issue.

Checking the PMU type of events to determine if they are from the same
PMU. There may be false alarm for the checking. E.g. the core PMU has
different PMU type. But it should not happen often.
The false alarm can also be tolerated, because:
- It only happens on error path.
- It just provides a possible solution for the issue.

Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
---
 tools/perf/builtin-stat.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 147a27e..30e6b37 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -172,6 +172,7 @@ static bool			interval_count;
 static const char		*output_name;
 static int			output_fd;
 static int			print_free_counters_hint;
+static int			print_mixed_hw_group_error;
 
 struct perf_stat {
 	bool			 record;
@@ -1126,6 +1127,30 @@ static void abs_printout(int id, int nr, struct perf_evsel *evsel, double avg)
 		fprintf(output, "%s%s", csv_sep, evsel->cgrp->name);
 }
 
+static bool is_mixed_hw_group(struct perf_evsel *counter)
+{
+	struct perf_evlist *evlist = counter->evlist;
+	u32 pmu_type = counter->attr.type;
+	struct perf_evsel *pos;
+
+	if (counter->nr_members < 2)
+		return false;
+
+	evlist__for_each_entry(evlist, pos) {
+		/* software events can be part of any hardware group */
+		if (pos->attr.type == PERF_TYPE_SOFTWARE)
+			continue;
+		if (pmu_type == PERF_TYPE_SOFTWARE) {
+			pmu_type = pos->attr.type;
+			continue;
+		}
+		if (pmu_type != pos->attr.type)
+			return true;
+	}
+
+	return false;
+}
+
 static void printout(int id, int nr, struct perf_evsel *counter, double uval,
 		     char *prefix, u64 run, u64 ena, double noise,
 		     struct runtime_stat *st)
@@ -1178,8 +1203,11 @@ static void printout(int id, int nr, struct perf_evsel *counter, double uval,
 			counter->supported ? CNTR_NOT_COUNTED : CNTR_NOT_SUPPORTED,
 			csv_sep);
 
-		if (counter->supported)
+		if (counter->supported) {
 			print_free_counters_hint = 1;
+			if (is_mixed_hw_group(counter))
+				print_mixed_hw_group_error = 1;
+		}
 
 		fprintf(stat_config.output, "%-*s%s",
 			csv_output ? 0 : unit_width,
@@ -1757,6 +1785,11 @@ static void print_footer(void)
 "	echo 0 > /proc/sys/kernel/nmi_watchdog\n"
 "	perf stat ...\n"
 "	echo 1 > /proc/sys/kernel/nmi_watchdog\n");
+
+	if (print_mixed_hw_group_error)
+		fprintf(output,
+			"The events in group usually have to be from "
+			"the same PMU. Try reorganizing the group.\n");
 }
 
 static void print_counters(struct timespec *ts, int argc, const char **argv)
-- 
2.7.4

[PATCH 3/5] perf evsel: Only fall back group read for leader

[kan.liang]

From: Kan Liang <kan.liang@linux.intel.com>

Perf doesn't support mixed events from different PMUs (except software
event) in a group. The perf stat should output <not counted>/<not
supported> for all events, but it doesn't. For example,

  perf stat -e '{cycles,uncore_imc_5/umask=0xF,event=0x4/,instructions}'
       <not counted>      cycles
       <not supported>    uncore_imc_5/umask=0xF,event=0x4/
           1,024,300      instructions

If perf fails to open an event, it doesn't error out directly. It will
disable some features and retry, until the event is opened or all
features are disabled. The disabled features will not be re-enabled. The
group read is one of these features.
For the example as above, the IMC event and the leader event "cycles"
are from different PMUs. Opening the IMC event must fail. The group read
feature must be disabled for IMC event and the followed event
"instructions". The "instructions" event has the same PMU as the leader
"cycles". It can be opened successfully. Since the group read feature
has been disabled, the "instructions" event will be read as a single
event, which definitely has a value.

The group read fallback is still useful for the case which kernel
doesn't support group read. It is good enough to be handled only by the
leader.
For the fallback request from members, it must be caused by an error. The
fallback only breaks the semantics of group.
Limit the group read fallback only for the leader.

Fixes:  82bf311e15d2 ("perf stat: Use group read for event groups")
Reported-by: Andi Kleen <ak@linux.intel.com>
Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
---
 tools/perf/util/evsel.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 1ac8d92..391ef7c 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -1922,7 +1922,8 @@ int perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
 		goto fallback_missing_features;
 	} else if (!perf_missing_features.group_read &&
 		    evsel->attr.inherit &&
-		   (evsel->attr.read_format & PERF_FORMAT_GROUP)) {
+		   (evsel->attr.read_format & PERF_FORMAT_GROUP) &&
+		   perf_evsel__is_group_leader(evsel)) {
 		perf_missing_features.group_read = true;
 		pr_debug2("switching off group read\n");
 		goto fallback_missing_features;
-- 
2.7.4

[PATCH 4/5] perf parse-events: Specially handle uncore event alias in small groups

[kan.liang]

From: Kan Liang <kan.liang@linux.intel.com>

Perf stat doesn't count the uncore event aliases from the same uncore
block in a group, for example:

  perf stat -e '{unc_m_cas_count.all,unc_m_clockticks}' -a -I 1000
  #           time             counts unit events
       1.000447342      <not counted>      unc_m_cas_count.all
       1.000447342      <not counted>      unc_m_clockticks
       2.000740654      <not counted>      unc_m_cas_count.all
       2.000740654      <not counted>      unc_m_clockticks

The output is very misleading. It gives a wrong impression that the
uncore event doesn't work.

An uncore block could be composed by several PMUs. An uncore event alias
is a joint name which means the same event runs on all PMUs of a block.
Perf doesn't support mixed events from different PMUs in the same group.
It is wrong to put uncore event aliases in a big group.

The right way is to split the big group into multiple small groups which
only include the events from the same PMU.
Only uncore event aliases from the same uncore block should be specially
handled here. It doesn't make sense to mix the uncore events with other
uncore events from different blocks or even core events in a group.

With the patch:
  #           time             counts unit events
     1.001557653            140,833      unc_m_cas_count.all
     1.001557653      1,330,231,332      unc_m_clockticks
     2.002709483             85,007      unc_m_cas_count.all
     2.002709483      1,429,494,563      unc_m_clockticks

Reported-by: Andi Kleen <ak@linux.intel.com>
Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
---
 tools/perf/util/evsel.h        |  1 +
 tools/perf/util/parse-events.c | 94 ++++++++++++++++++++++++++++++++++++++++--
 tools/perf/util/parse-events.h |  7 +++-
 tools/perf/util/parse-events.y |  8 ++--
 4 files changed, 101 insertions(+), 9 deletions(-)

diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index d3ee3af..b8a1055 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -126,6 +126,7 @@ struct perf_evsel {
 	bool			precise_max;
 	bool			ignore_missing_thread;
 	bool			forced_leader;
+	bool			use_uncore_alias;
 	/* parse modifier helper */
 	int			exclude_GH;
 	int			nr_members;
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 2fb0272..7a836fe 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1219,13 +1219,16 @@ int parse_events_add_numeric(struct parse_events_state *parse_state,
 
 int parse_events_add_pmu(struct parse_events_state *parse_state,
 			 struct list_head *list, char *name,
-			 struct list_head *head_config, bool auto_merge_stats)
+			 struct list_head *head_config,
+			 bool auto_merge_stats,
+			 bool use_alias)
 {
 	struct perf_event_attr attr;
 	struct perf_pmu_info info;
 	struct perf_pmu *pmu;
 	struct perf_evsel *evsel;
 	struct parse_events_error *err = parse_state->error;
+	bool use_uncore_alias;
 	LIST_HEAD(config_terms);
 
 	pmu = perf_pmu__find(name);
@@ -1244,11 +1247,14 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
 		memset(&attr, 0, sizeof(attr));
 	}
 
+	use_uncore_alias = (pmu->is_uncore && use_alias);
+
 	if (!head_config) {
 		attr.type = pmu->type;
 		evsel = __add_event(list, &parse_state->idx, &attr, NULL, pmu, NULL, auto_merge_stats);
 		if (evsel) {
 			evsel->pmu_name = name;
+			evsel->use_uncore_alias = use_uncore_alias;
 			return 0;
 		} else {
 			return -ENOMEM;
@@ -1282,6 +1288,7 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
 		evsel->metric_expr = info.metric_expr;
 		evsel->metric_name = info.metric_name;
 		evsel->pmu_name = name;
+		evsel->use_uncore_alias = use_uncore_alias;
 	}
 
 	return evsel ? 0 : -ENOMEM;
@@ -1317,7 +1324,7 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 				list_add_tail(&term->list, head);
 
 				if (!parse_events_add_pmu(parse_state, list,
-							  pmu->name, head, true)) {
+							  pmu->name, head, true, true)) {
 					pr_debug("%s -> %s/%s/\n", str,
 						 pmu->name, alias->str);
 					ok++;
@@ -1339,7 +1346,85 @@ int parse_events__modifier_group(struct list_head *list,
 	return parse_events__modifier_event(list, event_mod, true);
 }
 
-void parse_events__set_leader(char *name, struct list_head *list)
+/*
+ * Check if the two uncore PMUs are from the same uncore block
+ * The format of the uncore PMU name is uncore_#blockname_#pmuidx
+ */
+static bool is_same_uncore_block(const char *pmu_name_a, const char *pmu_name_b)
+{
+	char *end_a, *end_b;
+
+	end_a = strrchr(pmu_name_a, '_');
+	end_b = strrchr(pmu_name_b, '_');
+
+	if (!end_a || !end_b)
+		return false;
+
+	if ((end_a - pmu_name_a) != (end_b - pmu_name_b))
+		return false;
+
+	return (0 == strncmp(pmu_name_a, pmu_name_b, end_a - pmu_name_a));
+}
+
+static int
+parse_events__set_leader_for_uncore_aliase(char *name, struct list_head *list,
+					   struct parse_events_state *parse_state)
+{
+	struct perf_evsel *evsel, *leader;
+	uintptr_t *leaders;
+	int i = 0, nr_pmu = 0, total_members, ret = 0;
+
+	leader = list_entry(list->next, struct perf_evsel, node);
+	evsel = list_entry(list->prev, struct perf_evsel, node);
+	total_members = evsel->idx - leader->idx + 1;
+
+	leaders = calloc(total_members, sizeof(uintptr_t));
+	if (!leaders)
+		return ret;
+
+	 __evlist__for_each_entry(list, evsel) {
+
+		/* Only split the uncore group which members use alias */
+		if (!evsel->use_uncore_alias)
+			goto out;
+
+		/* The events must be from the same uncore block */
+		if (!is_same_uncore_block(leader->pmu_name, evsel->pmu_name))
+			goto out;
+
+		if (!strcmp(leader->name, evsel->name))
+			leaders[nr_pmu++] = (uintptr_t) evsel;
+	}
+
+	/* only one event alias */
+	if (nr_pmu == total_members) {
+		parse_state->nr_groups--;
+		goto handled;
+	}
+
+	__evlist__for_each_entry(list, evsel) {
+		if (i >= nr_pmu)
+			i = 0;
+		evsel->leader = (struct perf_evsel *) leaders[i++];
+	}
+
+	for (i = 0; i < nr_pmu; i++) {
+		evsel = (struct perf_evsel *) leaders[i];
+		evsel->nr_members = total_members / nr_pmu;
+		evsel->group_name = name ? strdup(name) : NULL;
+	}
+
+	parse_state->nr_groups += nr_pmu - 1;
+
+handled:
+	ret = 1;
+out:
+	free(leaders);
+	return ret;
+}
+
+void parse_events__set_leader(char *name, struct list_head *list,
+			      struct parse_events_state *parse_state)
 {
 	struct perf_evsel *leader;
 
@@ -1348,6 +1433,9 @@ void parse_events__set_leader(char *name, struct list_head *list)
 		return;
 	}
 
+	if (parse_events__set_leader_for_uncore_aliase(name, list, parse_state))
+		return;
+
 	__perf_evlist__set_leader(list);
 	leader = list_entry(list->next, struct perf_evsel, node);
 	leader->group_name = name ? strdup(name) : NULL;
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 5015cfd..4473dac 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -167,7 +167,9 @@ int parse_events_add_breakpoint(struct list_head *list, int *idx,
 				void *ptr, char *type, u64 len);
 int parse_events_add_pmu(struct parse_events_state *parse_state,
 			 struct list_head *list, char *name,
-			 struct list_head *head_config, bool auto_merge_stats);
+			 struct list_head *head_config,
+			 bool auto_merge_stats,
+			 bool use_alias);
 
 int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 			       char *str,
@@ -178,7 +180,8 @@ int parse_events_copy_term_list(struct list_head *old,
 
 enum perf_pmu_event_symbol_type
 perf_pmu__parse_check(const char *name);
-void parse_events__set_leader(char *name, struct list_head *list);
+void parse_events__set_leader(char *name, struct list_head *list,
+			      struct parse_events_state *parse_state);
 void parse_events_update_lists(struct list_head *list_event,
 			       struct list_head *list_all);
 void parse_events_evlist_error(struct parse_events_state *parse_state,
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 7afeb80..e37608a 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -161,7 +161,7 @@ PE_NAME '{' events '}'
 	struct list_head *list = $3;
 
 	inc_group_count(list, _parse_state);
-	parse_events__set_leader($1, list);
+	parse_events__set_leader($1, list, _parse_state);
 	$$ = list;
 }
 |
@@ -170,7 +170,7 @@ PE_NAME '{' events '}'
 	struct list_head *list = $2;
 
 	inc_group_count(list, _parse_state);
-	parse_events__set_leader(NULL, list);
+	parse_events__set_leader(NULL, list, _parse_state);
 	$$ = list;
 }
 
@@ -232,7 +232,7 @@ PE_NAME opt_event_config
 		YYABORT;
 
 	ALLOC_LIST(list);
-	if (parse_events_add_pmu(_parse_state, list, $1, $2, false)) {
+	if (parse_events_add_pmu(_parse_state, list, $1, $2, false, false)) {
 		struct perf_pmu *pmu = NULL;
 		int ok = 0;
 		char *pattern;
@@ -251,7 +251,7 @@ PE_NAME opt_event_config
 					free(pattern);
 					YYABORT;
 				}
-				if (!parse_events_add_pmu(_parse_state, list, pmu->name, terms, true))
+				if (!parse_events_add_pmu(_parse_state, list, pmu->name, terms, true, false))
 					ok++;
 				parse_events_terms__delete(terms);
 			}
-- 
2.7.4

[PATCH 5/5] perf stat: Fix duplicate PMU name for interval print

[kan.liang]

From: Kan Liang <kan.liang@linux.intel.com>

PMU name is printed repeatedly for interval print, for example:

  perf stat --no-merge -e 'unc_m_clockticks' -a -I 1000
  #           time             counts unit events
     1.001053069        243,702,144      unc_m_clockticks [uncore_imc_4]
     1.001053069        244,268,304      unc_m_clockticks [uncore_imc_2]
     1.001053069        244,427,386      unc_m_clockticks [uncore_imc_0]
     1.001053069        244,583,760      unc_m_clockticks [uncore_imc_5]
     1.001053069        244,738,971      unc_m_clockticks [uncore_imc_3]
     1.001053069        244,880,309      unc_m_clockticks [uncore_imc_1]
     2.002024821        240,818,200      unc_m_clockticks [uncore_imc_4]
[uncore_imc_4]
     2.002024821        240,767,812      unc_m_clockticks [uncore_imc_2]
[uncore_imc_2]
     2.002024821        240,764,215      unc_m_clockticks [uncore_imc_0]
[uncore_imc_0]
     2.002024821        240,759,504      unc_m_clockticks [uncore_imc_5]
[uncore_imc_5]
     2.002024821        240,755,992      unc_m_clockticks [uncore_imc_3]
[uncore_imc_3]
     2.002024821        240,750,403      unc_m_clockticks [uncore_imc_1]
[uncore_imc_1]

For each print, the PMU name is unconditionally appended to the
counter->name.
Need to check the counter->name first. If the PMU name is already
appended, do nothing.

Fixes: 8c5421c016a4 ("perf pmu: Display pmu name when printing unmerged events in stat")
Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
---
 tools/perf/builtin-stat.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 30e6b37..e22c213 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -1296,6 +1296,8 @@ static void uniquify_event_name(struct perf_evsel *counter)
 			counter->name = new_name;
 		}
 	} else {
+		if (strstr(counter->name, counter->pmu_name))
+			return;
 		if (asprintf(&new_name,
 			     "%s [%s]", counter->name, counter->pmu_name) > 0) {
 			free(counter->name);
-- 
2.7.4

Re: [PATCH 5/5] perf stat: Fix duplicate PMU name for interval print

[Arnaldo Carvalho de Melo]

Em Tue, Apr 24, 2018 at 11:20:14AM -0700, kan.liang@linux.intel.com escreveu:
> From: Kan Liang <kan.liang@linux.intel.com>
> 
> PMU name is printed repeatedly for interval print, for example:
> 
>   perf stat --no-merge -e 'unc_m_clockticks' -a -I 1000
>   #           time             counts unit events
>      1.001053069        243,702,144      unc_m_clockticks [uncore_imc_4]
>      1.001053069        244,268,304      unc_m_clockticks [uncore_imc_2]
>      1.001053069        244,427,386      unc_m_clockticks [uncore_imc_0]
>      1.001053069        244,583,760      unc_m_clockticks [uncore_imc_5]
>      1.001053069        244,738,971      unc_m_clockticks [uncore_imc_3]
>      1.001053069        244,880,309      unc_m_clockticks [uncore_imc_1]
>      2.002024821        240,818,200      unc_m_clockticks [uncore_imc_4] [uncore_imc_4]
>      2.002024821        240,767,812      unc_m_clockticks [uncore_imc_2] [uncore_imc_2]
>      2.002024821        240,764,215      unc_m_clockticks [uncore_imc_0] [uncore_imc_0]
>      2.002024821        240,759,504      unc_m_clockticks [uncore_imc_5] [uncore_imc_5]
>      2.002024821        240,755,992      unc_m_clockticks [uncore_imc_3] [uncore_imc_3]
>      2.002024821        240,750,403      unc_m_clockticks [uncore_imc_1] [uncore_imc_1]
> 
> For each print, the PMU name is unconditionally appended to the
> counter->name.
> Need to check the counter->name first. If the PMU name is already
> appended, do nothing.
> 
> Fixes: 8c5421c016a4 ("perf pmu: Display pmu name when printing unmerged events in stat")
> +++ b/tools/perf/builtin-stat.c
> @@ -1296,6 +1296,8 @@ static void uniquify_event_name(struct perf_evsel *counter)
>  			counter->name = new_name;
>  		}
>  	} else {
> +		if (strstr(counter->name, counter->pmu_name))
> +			return;
>  		if (asprintf(&new_name,
>  			     "%s [%s]", counter->name, counter->pmu_name) > 0) {
>  			free(counter->name);

Humm, do you have any problem with the patch below instead?

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 2137c7d11767..8518342c5466 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -1261,7 +1261,8 @@ static void uniquify_event_name(struct perf_evsel *counter)
 	char *new_name;
 	char *config;
 
-	if (!counter->pmu_name || !strncmp(counter->name, counter->pmu_name,
+	if (counter->uniquified_name ||
+	    !counter->pmu_name || !strncmp(counter->name, counter->pmu_name,
 					   strlen(counter->pmu_name)))
 		return;
 
@@ -1279,6 +1280,8 @@ static void uniquify_event_name(struct perf_evsel *counter)
 			counter->name = new_name;
 		}
 	}
+
+	counter->uniquified_name = true;
 }
 
 static void collect_all_aliases(struct perf_evsel *counter,
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index d3ee3af618ef..92ec009a292d 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -115,6 +115,7 @@ struct perf_evsel {
 	unsigned int		sample_size;
 	int			id_pos;
 	int			is_pos;
+	bool			uniquified_name;
 	bool			snapshot;
 	bool 			supported;
 	bool 			needs_swap;

Re: [PATCH 4/5] perf parse-events: Specially handle uncore event alias in small groups

[Arnaldo Carvalho de Melo]

Em Tue, Apr 24, 2018 at 11:20:13AM -0700, kan.liang@linux.intel.com escreveu:
> From: Kan Liang <kan.liang@linux.intel.com>
> 
> Perf stat doesn't count the uncore event aliases from the same uncore
> block in a group, for example:

This one is not applying to acme/perf/urgent, all the rest I got merged
there, the last one with that change from using strstr() to a new bool
in perf_evsel for the uniquifying operation having being performed.

- Arnaldo
 
>   perf stat -e '{unc_m_cas_count.all,unc_m_clockticks}' -a -I 1000
>   #           time             counts unit events
>        1.000447342      <not counted>      unc_m_cas_count.all
>        1.000447342      <not counted>      unc_m_clockticks
>        2.000740654      <not counted>      unc_m_cas_count.all
>        2.000740654      <not counted>      unc_m_clockticks
> 
> The output is very misleading. It gives a wrong impression that the
> uncore event doesn't work.
> 
> An uncore block could be composed by several PMUs. An uncore event alias
> is a joint name which means the same event runs on all PMUs of a block.
> Perf doesn't support mixed events from different PMUs in the same group.
> It is wrong to put uncore event aliases in a big group.
> 
> The right way is to split the big group into multiple small groups which
> only include the events from the same PMU.
> Only uncore event aliases from the same uncore block should be specially
> handled here. It doesn't make sense to mix the uncore events with other
> uncore events from different blocks or even core events in a group.
> 
> With the patch:
>   #           time             counts unit events
>      1.001557653            140,833      unc_m_cas_count.all
>      1.001557653      1,330,231,332      unc_m_clockticks
>      2.002709483             85,007      unc_m_cas_count.all
>      2.002709483      1,429,494,563      unc_m_clockticks
> 
> Reported-by: Andi Kleen <ak@linux.intel.com>
> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
> ---
>  tools/perf/util/evsel.h        |  1 +
>  tools/perf/util/parse-events.c | 94 ++++++++++++++++++++++++++++++++++++++++--
>  tools/perf/util/parse-events.h |  7 +++-
>  tools/perf/util/parse-events.y |  8 ++--
>  4 files changed, 101 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> index d3ee3af..b8a1055 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -126,6 +126,7 @@ struct perf_evsel {
>  	bool			precise_max;
>  	bool			ignore_missing_thread;
>  	bool			forced_leader;
> +	bool			use_uncore_alias;
>  	/* parse modifier helper */
>  	int			exclude_GH;
>  	int			nr_members;
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 2fb0272..7a836fe 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -1219,13 +1219,16 @@ int parse_events_add_numeric(struct parse_events_state *parse_state,
>  
>  int parse_events_add_pmu(struct parse_events_state *parse_state,
>  			 struct list_head *list, char *name,
> -			 struct list_head *head_config, bool auto_merge_stats)
> +			 struct list_head *head_config,
> +			 bool auto_merge_stats,
> +			 bool use_alias)
>  {
>  	struct perf_event_attr attr;
>  	struct perf_pmu_info info;
>  	struct perf_pmu *pmu;
>  	struct perf_evsel *evsel;
>  	struct parse_events_error *err = parse_state->error;
> +	bool use_uncore_alias;
>  	LIST_HEAD(config_terms);
>  
>  	pmu = perf_pmu__find(name);
> @@ -1244,11 +1247,14 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
>  		memset(&attr, 0, sizeof(attr));
>  	}
>  
> +	use_uncore_alias = (pmu->is_uncore && use_alias);
> +
>  	if (!head_config) {
>  		attr.type = pmu->type;
>  		evsel = __add_event(list, &parse_state->idx, &attr, NULL, pmu, NULL, auto_merge_stats);
>  		if (evsel) {
>  			evsel->pmu_name = name;
> +			evsel->use_uncore_alias = use_uncore_alias;
>  			return 0;
>  		} else {
>  			return -ENOMEM;
> @@ -1282,6 +1288,7 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
>  		evsel->metric_expr = info.metric_expr;
>  		evsel->metric_name = info.metric_name;
>  		evsel->pmu_name = name;
> +		evsel->use_uncore_alias = use_uncore_alias;
>  	}
>  
>  	return evsel ? 0 : -ENOMEM;
> @@ -1317,7 +1324,7 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
>  				list_add_tail(&term->list, head);
>  
>  				if (!parse_events_add_pmu(parse_state, list,
> -							  pmu->name, head, true)) {
> +							  pmu->name, head, true, true)) {
>  					pr_debug("%s -> %s/%s/\n", str,
>  						 pmu->name, alias->str);
>  					ok++;
> @@ -1339,7 +1346,85 @@ int parse_events__modifier_group(struct list_head *list,
>  	return parse_events__modifier_event(list, event_mod, true);
>  }
>  
> -void parse_events__set_leader(char *name, struct list_head *list)
> +/*
> + * Check if the two uncore PMUs are from the same uncore block
> + * The format of the uncore PMU name is uncore_#blockname_#pmuidx
> + */
> +static bool is_same_uncore_block(const char *pmu_name_a, const char *pmu_name_b)
> +{
> +	char *end_a, *end_b;
> +
> +	end_a = strrchr(pmu_name_a, '_');
> +	end_b = strrchr(pmu_name_b, '_');
> +
> +	if (!end_a || !end_b)
> +		return false;
> +
> +	if ((end_a - pmu_name_a) != (end_b - pmu_name_b))
> +		return false;
> +
> +	return (0 == strncmp(pmu_name_a, pmu_name_b, end_a - pmu_name_a));
> +}
> +
> +static int
> +parse_events__set_leader_for_uncore_aliase(char *name, struct list_head *list,
> +					   struct parse_events_state *parse_state)
> +{
> +	struct perf_evsel *evsel, *leader;
> +	uintptr_t *leaders;
> +	int i = 0, nr_pmu = 0, total_members, ret = 0;
> +
> +	leader = list_entry(list->next, struct perf_evsel, node);
> +	evsel = list_entry(list->prev, struct perf_evsel, node);
> +	total_members = evsel->idx - leader->idx + 1;
> +
> +	leaders = calloc(total_members, sizeof(uintptr_t));
> +	if (!leaders)
> +		return ret;
> +
> +	 __evlist__for_each_entry(list, evsel) {
> +
> +		/* Only split the uncore group which members use alias */
> +		if (!evsel->use_uncore_alias)
> +			goto out;
> +
> +		/* The events must be from the same uncore block */
> +		if (!is_same_uncore_block(leader->pmu_name, evsel->pmu_name))
> +			goto out;
> +
> +		if (!strcmp(leader->name, evsel->name))
> +			leaders[nr_pmu++] = (uintptr_t) evsel;
> +	}
> +
> +	/* only one event alias */
> +	if (nr_pmu == total_members) {
> +		parse_state->nr_groups--;
> +		goto handled;
> +	}
> +
> +	__evlist__for_each_entry(list, evsel) {
> +		if (i >= nr_pmu)
> +			i = 0;
> +		evsel->leader = (struct perf_evsel *) leaders[i++];
> +	}
> +
> +	for (i = 0; i < nr_pmu; i++) {
> +		evsel = (struct perf_evsel *) leaders[i];
> +		evsel->nr_members = total_members / nr_pmu;
> +		evsel->group_name = name ? strdup(name) : NULL;
> +	}
> +
> +	parse_state->nr_groups += nr_pmu - 1;
> +
> +handled:
> +	ret = 1;
> +out:
> +	free(leaders);
> +	return ret;
> +}
> +
> +void parse_events__set_leader(char *name, struct list_head *list,
> +			      struct parse_events_state *parse_state)
>  {
>  	struct perf_evsel *leader;
>  
> @@ -1348,6 +1433,9 @@ void parse_events__set_leader(char *name, struct list_head *list)
>  		return;
>  	}
>  
> +	if (parse_events__set_leader_for_uncore_aliase(name, list, parse_state))
> +		return;
> +
>  	__perf_evlist__set_leader(list);
>  	leader = list_entry(list->next, struct perf_evsel, node);
>  	leader->group_name = name ? strdup(name) : NULL;
> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index 5015cfd..4473dac 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -167,7 +167,9 @@ int parse_events_add_breakpoint(struct list_head *list, int *idx,
>  				void *ptr, char *type, u64 len);
>  int parse_events_add_pmu(struct parse_events_state *parse_state,
>  			 struct list_head *list, char *name,
> -			 struct list_head *head_config, bool auto_merge_stats);
> +			 struct list_head *head_config,
> +			 bool auto_merge_stats,
> +			 bool use_alias);
>  
>  int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
>  			       char *str,
> @@ -178,7 +180,8 @@ int parse_events_copy_term_list(struct list_head *old,
>  
>  enum perf_pmu_event_symbol_type
>  perf_pmu__parse_check(const char *name);
> -void parse_events__set_leader(char *name, struct list_head *list);
> +void parse_events__set_leader(char *name, struct list_head *list,
> +			      struct parse_events_state *parse_state);
>  void parse_events_update_lists(struct list_head *list_event,
>  			       struct list_head *list_all);
>  void parse_events_evlist_error(struct parse_events_state *parse_state,
> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> index 7afeb80..e37608a 100644
> --- a/tools/perf/util/parse-events.y
> +++ b/tools/perf/util/parse-events.y
> @@ -161,7 +161,7 @@ PE_NAME '{' events '}'
>  	struct list_head *list = $3;
>  
>  	inc_group_count(list, _parse_state);
> -	parse_events__set_leader($1, list);
> +	parse_events__set_leader($1, list, _parse_state);
>  	$$ = list;
>  }
>  |
> @@ -170,7 +170,7 @@ PE_NAME '{' events '}'
>  	struct list_head *list = $2;
>  
>  	inc_group_count(list, _parse_state);
> -	parse_events__set_leader(NULL, list);
> +	parse_events__set_leader(NULL, list, _parse_state);
>  	$$ = list;
>  }
>  
> @@ -232,7 +232,7 @@ PE_NAME opt_event_config
>  		YYABORT;
>  
>  	ALLOC_LIST(list);
> -	if (parse_events_add_pmu(_parse_state, list, $1, $2, false)) {
> +	if (parse_events_add_pmu(_parse_state, list, $1, $2, false, false)) {
>  		struct perf_pmu *pmu = NULL;
>  		int ok = 0;
>  		char *pattern;
> @@ -251,7 +251,7 @@ PE_NAME opt_event_config
>  					free(pattern);
>  					YYABORT;
>  				}
> -				if (!parse_events_add_pmu(_parse_state, list, pmu->name, terms, true))
> +				if (!parse_events_add_pmu(_parse_state, list, pmu->name, terms, true, false))
>  					ok++;
>  				parse_events_terms__delete(terms);
>  			}
> -- 
> 2.7.4

Re: [PATCH 5/5] perf stat: Fix duplicate PMU name for interval print

[Liang, Kan]

On 4/24/2018 2:53 PM, Arnaldo Carvalho de Melo wrote:
> Em Tue, Apr 24, 2018 at 11:20:14AM -0700, kan.liang@linux.intel.com escreveu:
>> From: Kan Liang <kan.liang@linux.intel.com>
>>
>> PMU name is printed repeatedly for interval print, for example:
>>
>>    perf stat --no-merge -e 'unc_m_clockticks' -a -I 1000
>>    #           time             counts unit events
>>       1.001053069        243,702,144      unc_m_clockticks [uncore_imc_4]
>>       1.001053069        244,268,304      unc_m_clockticks [uncore_imc_2]
>>       1.001053069        244,427,386      unc_m_clockticks [uncore_imc_0]
>>       1.001053069        244,583,760      unc_m_clockticks [uncore_imc_5]
>>       1.001053069        244,738,971      unc_m_clockticks [uncore_imc_3]
>>       1.001053069        244,880,309      unc_m_clockticks [uncore_imc_1]
>>       2.002024821        240,818,200      unc_m_clockticks [uncore_imc_4] [uncore_imc_4]
>>       2.002024821        240,767,812      unc_m_clockticks [uncore_imc_2] [uncore_imc_2]
>>       2.002024821        240,764,215      unc_m_clockticks [uncore_imc_0] [uncore_imc_0]
>>       2.002024821        240,759,504      unc_m_clockticks [uncore_imc_5] [uncore_imc_5]
>>       2.002024821        240,755,992      unc_m_clockticks [uncore_imc_3] [uncore_imc_3]
>>       2.002024821        240,750,403      unc_m_clockticks [uncore_imc_1] [uncore_imc_1]
>>
>> For each print, the PMU name is unconditionally appended to the
>> counter->name.
>> Need to check the counter->name first. If the PMU name is already
>> appended, do nothing.
>>
>> Fixes: 8c5421c016a4 ("perf pmu: Display pmu name when printing unmerged events in stat")
>> +++ b/tools/perf/builtin-stat.c
>> @@ -1296,6 +1296,8 @@ static void uniquify_event_name(struct perf_evsel *counter)
>>   			counter->name = new_name;
>>   		}
>>   	} else {
>> +		if (strstr(counter->name, counter->pmu_name))
>> +			return;
>>   		if (asprintf(&new_name,
>>   			     "%s [%s]", counter->name, counter->pmu_name) > 0) {
>>   			free(counter->name);
> 
> Humm, do you have any problem with the patch below instead?

No. The patch as below looks good to me.

Thanks,
Kan

> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 2137c7d11767..8518342c5466 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -1261,7 +1261,8 @@ static void uniquify_event_name(struct perf_evsel *counter)
>   	char *new_name;
>   	char *config;
>   
> -	if (!counter->pmu_name || !strncmp(counter->name, counter->pmu_name,
> +	if (counter->uniquified_name ||
> +	    !counter->pmu_name || !strncmp(counter->name, counter->pmu_name,
>   					   strlen(counter->pmu_name)))
>   		return;
>   
> @@ -1279,6 +1280,8 @@ static void uniquify_event_name(struct perf_evsel *counter)
>   			counter->name = new_name;
>   		}
>   	}
> +
> +	counter->uniquified_name = true;
>   }
>   
>   static void collect_all_aliases(struct perf_evsel *counter,
> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> index d3ee3af618ef..92ec009a292d 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -115,6 +115,7 @@ struct perf_evsel {
>   	unsigned int		sample_size;
>   	int			id_pos;
>   	int			is_pos;
> +	bool			uniquified_name;
>   	bool			snapshot;
>   	bool 			supported;
>   	bool 			needs_swap;
> 

Re: [PATCH 4/5] perf parse-events: Specially handle uncore event alias in small groups

[Liang, Kan]

On 4/24/2018 3:17 PM, Arnaldo Carvalho de Melo wrote:
> Em Tue, Apr 24, 2018 at 11:20:13AM -0700, kan.liang@linux.intel.com escreveu:
>> From: Kan Liang <kan.liang@linux.intel.com>
>>
>> Perf stat doesn't count the uncore event aliases from the same uncore
>> block in a group, for example:
> 
> This one is not applying to acme/perf/urgent, all the rest I got merged
> there, the last one with that change from using strstr() to a new bool
> in perf_evsel for the uniquifying operation having being performed.
>

Sure. Thank you for letting me know.

Thanks,
Kan

> - Arnaldo
>   
>>    perf stat -e '{unc_m_cas_count.all,unc_m_clockticks}' -a -I 1000
>>    #           time             counts unit events
>>         1.000447342      <not counted>      unc_m_cas_count.all
>>         1.000447342      <not counted>      unc_m_clockticks
>>         2.000740654      <not counted>      unc_m_cas_count.all
>>         2.000740654      <not counted>      unc_m_clockticks
>>
>> The output is very misleading. It gives a wrong impression that the
>> uncore event doesn't work.
>>
>> An uncore block could be composed by several PMUs. An uncore event alias
>> is a joint name which means the same event runs on all PMUs of a block.
>> Perf doesn't support mixed events from different PMUs in the same group.
>> It is wrong to put uncore event aliases in a big group.
>>
>> The right way is to split the big group into multiple small groups which
>> only include the events from the same PMU.
>> Only uncore event aliases from the same uncore block should be specially
>> handled here. It doesn't make sense to mix the uncore events with other
>> uncore events from different blocks or even core events in a group.
>>
>> With the patch:
>>    #           time             counts unit events
>>       1.001557653            140,833      unc_m_cas_count.all
>>       1.001557653      1,330,231,332      unc_m_clockticks
>>       2.002709483             85,007      unc_m_cas_count.all
>>       2.002709483      1,429,494,563      unc_m_clockticks
>>
>> Reported-by: Andi Kleen <ak@linux.intel.com>
>> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
>> ---
>>   tools/perf/util/evsel.h        |  1 +
>>   tools/perf/util/parse-events.c | 94 ++++++++++++++++++++++++++++++++++++++++--
>>   tools/perf/util/parse-events.h |  7 +++-
>>   tools/perf/util/parse-events.y |  8 ++--
>>   4 files changed, 101 insertions(+), 9 deletions(-)
>>
>> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
>> index d3ee3af..b8a1055 100644
>> --- a/tools/perf/util/evsel.h
>> +++ b/tools/perf/util/evsel.h
>> @@ -126,6 +126,7 @@ struct perf_evsel {
>>   	bool			precise_max;
>>   	bool			ignore_missing_thread;
>>   	bool			forced_leader;
>> +	bool			use_uncore_alias;
>>   	/* parse modifier helper */
>>   	int			exclude_GH;
>>   	int			nr_members;
>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
>> index 2fb0272..7a836fe 100644
>> --- a/tools/perf/util/parse-events.c
>> +++ b/tools/perf/util/parse-events.c
>> @@ -1219,13 +1219,16 @@ int parse_events_add_numeric(struct parse_events_state *parse_state,
>>   
>>   int parse_events_add_pmu(struct parse_events_state *parse_state,
>>   			 struct list_head *list, char *name,
>> -			 struct list_head *head_config, bool auto_merge_stats)
>> +			 struct list_head *head_config,
>> +			 bool auto_merge_stats,
>> +			 bool use_alias)
>>   {
>>   	struct perf_event_attr attr;
>>   	struct perf_pmu_info info;
>>   	struct perf_pmu *pmu;
>>   	struct perf_evsel *evsel;
>>   	struct parse_events_error *err = parse_state->error;
>> +	bool use_uncore_alias;
>>   	LIST_HEAD(config_terms);
>>   
>>   	pmu = perf_pmu__find(name);
>> @@ -1244,11 +1247,14 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
>>   		memset(&attr, 0, sizeof(attr));
>>   	}
>>   
>> +	use_uncore_alias = (pmu->is_uncore && use_alias);
>> +
>>   	if (!head_config) {
>>   		attr.type = pmu->type;
>>   		evsel = __add_event(list, &parse_state->idx, &attr, NULL, pmu, NULL, auto_merge_stats);
>>   		if (evsel) {
>>   			evsel->pmu_name = name;
>> +			evsel->use_uncore_alias = use_uncore_alias;
>>   			return 0;
>>   		} else {
>>   			return -ENOMEM;
>> @@ -1282,6 +1288,7 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
>>   		evsel->metric_expr = info.metric_expr;
>>   		evsel->metric_name = info.metric_name;
>>   		evsel->pmu_name = name;
>> +		evsel->use_uncore_alias = use_uncore_alias;
>>   	}
>>   
>>   	return evsel ? 0 : -ENOMEM;
>> @@ -1317,7 +1324,7 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
>>   				list_add_tail(&term->list, head);
>>   
>>   				if (!parse_events_add_pmu(parse_state, list,
>> -							  pmu->name, head, true)) {
>> +							  pmu->name, head, true, true)) {
>>   					pr_debug("%s -> %s/%s/\n", str,
>>   						 pmu->name, alias->str);
>>   					ok++;
>> @@ -1339,7 +1346,85 @@ int parse_events__modifier_group(struct list_head *list,
>>   	return parse_events__modifier_event(list, event_mod, true);
>>   }
>>   
>> -void parse_events__set_leader(char *name, struct list_head *list)
>> +/*
>> + * Check if the two uncore PMUs are from the same uncore block
>> + * The format of the uncore PMU name is uncore_#blockname_#pmuidx
>> + */
>> +static bool is_same_uncore_block(const char *pmu_name_a, const char *pmu_name_b)
>> +{
>> +	char *end_a, *end_b;
>> +
>> +	end_a = strrchr(pmu_name_a, '_');
>> +	end_b = strrchr(pmu_name_b, '_');
>> +
>> +	if (!end_a || !end_b)
>> +		return false;
>> +
>> +	if ((end_a - pmu_name_a) != (end_b - pmu_name_b))
>> +		return false;
>> +
>> +	return (0 == strncmp(pmu_name_a, pmu_name_b, end_a - pmu_name_a));
>> +}
>> +
>> +static int
>> +parse_events__set_leader_for_uncore_aliase(char *name, struct list_head *list,
>> +					   struct parse_events_state *parse_state)
>> +{
>> +	struct perf_evsel *evsel, *leader;
>> +	uintptr_t *leaders;
>> +	int i = 0, nr_pmu = 0, total_members, ret = 0;
>> +
>> +	leader = list_entry(list->next, struct perf_evsel, node);
>> +	evsel = list_entry(list->prev, struct perf_evsel, node);
>> +	total_members = evsel->idx - leader->idx + 1;
>> +
>> +	leaders = calloc(total_members, sizeof(uintptr_t));
>> +	if (!leaders)
>> +		return ret;
>> +
>> +	 __evlist__for_each_entry(list, evsel) {
>> +
>> +		/* Only split the uncore group which members use alias */
>> +		if (!evsel->use_uncore_alias)
>> +			goto out;
>> +
>> +		/* The events must be from the same uncore block */
>> +		if (!is_same_uncore_block(leader->pmu_name, evsel->pmu_name))
>> +			goto out;
>> +
>> +		if (!strcmp(leader->name, evsel->name))
>> +			leaders[nr_pmu++] = (uintptr_t) evsel;
>> +	}
>> +
>> +	/* only one event alias */
>> +	if (nr_pmu == total_members) {
>> +		parse_state->nr_groups--;
>> +		goto handled;
>> +	}
>> +
>> +	__evlist__for_each_entry(list, evsel) {
>> +		if (i >= nr_pmu)
>> +			i = 0;
>> +		evsel->leader = (struct perf_evsel *) leaders[i++];
>> +	}
>> +
>> +	for (i = 0; i < nr_pmu; i++) {
>> +		evsel = (struct perf_evsel *) leaders[i];
>> +		evsel->nr_members = total_members / nr_pmu;
>> +		evsel->group_name = name ? strdup(name) : NULL;
>> +	}
>> +
>> +	parse_state->nr_groups += nr_pmu - 1;
>> +
>> +handled:
>> +	ret = 1;
>> +out:
>> +	free(leaders);
>> +	return ret;
>> +}
>> +
>> +void parse_events__set_leader(char *name, struct list_head *list,
>> +			      struct parse_events_state *parse_state)
>>   {
>>   	struct perf_evsel *leader;
>>   
>> @@ -1348,6 +1433,9 @@ void parse_events__set_leader(char *name, struct list_head *list)
>>   		return;
>>   	}
>>   
>> +	if (parse_events__set_leader_for_uncore_aliase(name, list, parse_state))
>> +		return;
>> +
>>   	__perf_evlist__set_leader(list);
>>   	leader = list_entry(list->next, struct perf_evsel, node);
>>   	leader->group_name = name ? strdup(name) : NULL;
>> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
>> index 5015cfd..4473dac 100644
>> --- a/tools/perf/util/parse-events.h
>> +++ b/tools/perf/util/parse-events.h
>> @@ -167,7 +167,9 @@ int parse_events_add_breakpoint(struct list_head *list, int *idx,
>>   				void *ptr, char *type, u64 len);
>>   int parse_events_add_pmu(struct parse_events_state *parse_state,
>>   			 struct list_head *list, char *name,
>> -			 struct list_head *head_config, bool auto_merge_stats);
>> +			 struct list_head *head_config,
>> +			 bool auto_merge_stats,
>> +			 bool use_alias);
>>   
>>   int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
>>   			       char *str,
>> @@ -178,7 +180,8 @@ int parse_events_copy_term_list(struct list_head *old,
>>   
>>   enum perf_pmu_event_symbol_type
>>   perf_pmu__parse_check(const char *name);
>> -void parse_events__set_leader(char *name, struct list_head *list);
>> +void parse_events__set_leader(char *name, struct list_head *list,
>> +			      struct parse_events_state *parse_state);
>>   void parse_events_update_lists(struct list_head *list_event,
>>   			       struct list_head *list_all);
>>   void parse_events_evlist_error(struct parse_events_state *parse_state,
>> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
>> index 7afeb80..e37608a 100644
>> --- a/tools/perf/util/parse-events.y
>> +++ b/tools/perf/util/parse-events.y
>> @@ -161,7 +161,7 @@ PE_NAME '{' events '}'
>>   	struct list_head *list = $3;
>>   
>>   	inc_group_count(list, _parse_state);
>> -	parse_events__set_leader($1, list);
>> +	parse_events__set_leader($1, list, _parse_state);
>>   	$$ = list;
>>   }
>>   |
>> @@ -170,7 +170,7 @@ PE_NAME '{' events '}'
>>   	struct list_head *list = $2;
>>   
>>   	inc_group_count(list, _parse_state);
>> -	parse_events__set_leader(NULL, list);
>> +	parse_events__set_leader(NULL, list, _parse_state);
>>   	$$ = list;
>>   }
>>   
>> @@ -232,7 +232,7 @@ PE_NAME opt_event_config
>>   		YYABORT;
>>   
>>   	ALLOC_LIST(list);
>> -	if (parse_events_add_pmu(_parse_state, list, $1, $2, false)) {
>> +	if (parse_events_add_pmu(_parse_state, list, $1, $2, false, false)) {
>>   		struct perf_pmu *pmu = NULL;
>>   		int ok = 0;
>>   		char *pattern;
>> @@ -251,7 +251,7 @@ PE_NAME opt_event_config
>>   					free(pattern);
>>   					YYABORT;
>>   				}
>> -				if (!parse_events_add_pmu(_parse_state, list, pmu->name, terms, true))
>> +				if (!parse_events_add_pmu(_parse_state, list, pmu->name, terms, true, false))
>>   					ok++;
>>   				parse_events_terms__delete(terms);
>>   			}
>> -- 
>> 2.7.4

Re: [PATCH 5/5] perf stat: Fix duplicate PMU name for interval print

[Arnaldo Carvalho de Melo]

Em Tue, Apr 24, 2018 at 03:18:34PM -0400, Liang, Kan escreveu:
> On 4/24/2018 2:53 PM, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Apr 24, 2018 at 11:20:14AM -0700, kan.liang@linux.intel.com escreveu:
> > > +		if (strstr(counter->name, counter->pmu_name))
> > > +			return;
> > >   		if (asprintf(&new_name,
> > >   			     "%s [%s]", counter->name, counter->pmu_name) > 0) {
> > >   			free(counter->name);

> > Humm, do you have any problem with the patch below instead?
 
> No. The patch as below looks good to me.

Thanks for checking,

- Arnaldo

> > +++ b/tools/perf/builtin-stat.c
> > @@ -1261,7 +1261,8 @@ static void uniquify_event_name(struct perf_evsel *counter)
> > -	if (!counter->pmu_name || !strncmp(counter->name, counter->pmu_name,
> > +	if (counter->uniquified_name ||
> > +	    !counter->pmu_name || !strncmp(counter->name, counter->pmu_name,
> >   					   strlen(counter->pmu_name)))
> > @@ -1279,6 +1280,8 @@ static void uniquify_event_name(struct perf_evsel *counter)
> >   	}
> > +
> > +	counter->uniquified_name = true;
> > +++ b/tools/perf/util/evsel.h
> > @@ -115,6 +115,7 @@ struct perf_evsel {
> >   	unsigned int		sample_size;
> >   	int			id_pos;
> >   	int			is_pos;
> > +	bool			uniquified_name;

Re: [PATCH 4/5] perf parse-events: Specially handle uncore event alias in small groups

[Arnaldo Carvalho de Melo]

Em Tue, Apr 24, 2018 at 03:23:06PM -0400, Liang, Kan escreveu:
> On 4/24/2018 3:17 PM, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Apr 24, 2018 at 11:20:13AM -0700, kan.liang@linux.intel.com escreveu:
> > > From: Kan Liang <kan.liang@linux.intel.com>
> > > 
> > > Perf stat doesn't count the uncore event aliases from the same uncore
> > > block in a group, for example:
> > 
> > This one is not applying to acme/perf/urgent, all the rest I got merged
> > there, the last one with that change from using strstr() to a new bool
> > in perf_evsel for the uniquifying operation having being performed.
> 
> Sure. Thank you for letting me know.

Just pushed what I have there,

- Arnaldo

Re: [PATCH 4/5] perf parse-events: Specially handle uncore event alias in small groups

[Liang, Kan]

On 4/24/2018 3:29 PM, Arnaldo Carvalho de Melo wrote:
> Em Tue, Apr 24, 2018 at 03:23:06PM -0400, Liang, Kan escreveu:
>> On 4/24/2018 3:17 PM, Arnaldo Carvalho de Melo wrote:
>>> Em Tue, Apr 24, 2018 at 11:20:13AM -0700, kan.liang@linux.intel.com escreveu:
>>>> From: Kan Liang <kan.liang@linux.intel.com>
>>>>
>>>> Perf stat doesn't count the uncore event aliases from the same uncore
>>>> block in a group, for example:
>>>
>>> This one is not applying to acme/perf/urgent, all the rest I got merged
>>> there, the last one with that change from using strstr() to a new bool
>>> in perf_evsel for the uniquifying operation having being performed.
>>
>> Sure. Thank you for letting me know.
> 
> Just pushed what I have there,
> 

Thanks Arnaldo.

How about this one?
Will it be applied to acme/perf/core? Or should I resend it for wider 
review?

Thanks,
Kan

Re: [PATCH 4/5] perf parse-events: Specially handle uncore event alias in small groups

[Arnaldo Carvalho de Melo]

Em Wed, Apr 25, 2018 at 08:27:31AM -0400, Liang, Kan escreveu:
> 
> 
> On 4/24/2018 3:29 PM, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Apr 24, 2018 at 03:23:06PM -0400, Liang, Kan escreveu:
> > > On 4/24/2018 3:17 PM, Arnaldo Carvalho de Melo wrote:
> > > > Em Tue, Apr 24, 2018 at 11:20:13AM -0700, kan.liang@linux.intel.com escreveu:
> > > > > From: Kan Liang <kan.liang@linux.intel.com>
> > > > > 
> > > > > Perf stat doesn't count the uncore event aliases from the same uncore
> > > > > block in a group, for example:
> > > > 
> > > > This one is not applying to acme/perf/urgent, all the rest I got merged
> > > > there, the last one with that change from using strstr() to a new bool
> > > > in perf_evsel for the uniquifying operation having being performed.
> > > 
> > > Sure. Thank you for letting me know.
> > 
> > Just pushed what I have there,
> > 
> 
> Thanks Arnaldo.
> 
> How about this one?
> Will it be applied to acme/perf/core? Or should I resend it for wider
> review?

On acme/perf/urgent, please check

[acme@seventh perf]$ patch -p1 < /wb/1.patch 
patching file tools/perf/util/evsel.h
Hunk #1 succeeded at 127 (offset 1 line).
patching file tools/perf/util/parse-events.c
patching file tools/perf/util/parse-events.h
patching file tools/perf/util/parse-events.y
Hunk #3 FAILED at 232.
1 out of 4 hunks FAILED -- saving rejects to file tools/perf/util/parse-events.y.rej
[acme@seventh perf]$ cat tools/perf/util/parse-events.y.rej
--- tools/perf/util/parse-events.y
+++ tools/perf/util/parse-events.y
@@ -232,7 +232,7 @@ PE_NAME opt_event_config
 		YYABORT;
 
 	ALLOC_LIST(list);
-	if (parse_events_add_pmu(_parse_state, list, $1, $2, false)) {
+	if (parse_events_add_pmu(_parse_state, list, $1, $2, false, false)) {
 		struct perf_pmu *pmu = NULL;
 		int ok = 0;
 		char *pattern;
[acme@seventh perf]$

Re: [PATCH 4/5] perf parse-events: Specially handle uncore event alias in small groups

[Liang, Kan]

On 4/25/2018 8:59 AM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Apr 25, 2018 at 08:27:31AM -0400, Liang, Kan escreveu:
>>
>>
>> On 4/24/2018 3:29 PM, Arnaldo Carvalho de Melo wrote:
>>> Em Tue, Apr 24, 2018 at 03:23:06PM -0400, Liang, Kan escreveu:
>>>> On 4/24/2018 3:17 PM, Arnaldo Carvalho de Melo wrote:
>>>>> Em Tue, Apr 24, 2018 at 11:20:13AM -0700, kan.liang@linux.intel.com escreveu:
>>>>>> From: Kan Liang <kan.liang@linux.intel.com>
>>>>>>
>>>>>> Perf stat doesn't count the uncore event aliases from the same uncore
>>>>>> block in a group, for example:
>>>>>
>>>>> This one is not applying to acme/perf/urgent, all the rest I got merged
>>>>> there, the last one with that change from using strstr() to a new bool
>>>>> in perf_evsel for the uniquifying operation having being performed.
>>>>
>>>> Sure. Thank you for letting me know.
>>>
>>> Just pushed what I have there,
>>>
>>
>> Thanks Arnaldo.
>>
>> How about this one?
>> Will it be applied to acme/perf/core? Or should I resend it for wider
>> review?
> 
> On acme/perf/urgent, please check

I will re-base the patch and send V2.

Thanks,
Kan

> 
> [acme@seventh perf]$ patch -p1 < /wb/1.patch
> patching file tools/perf/util/evsel.h
> Hunk #1 succeeded at 127 (offset 1 line).
> patching file tools/perf/util/parse-events.c
> patching file tools/perf/util/parse-events.h
> patching file tools/perf/util/parse-events.y
> Hunk #3 FAILED at 232.
> 1 out of 4 hunks FAILED -- saving rejects to file tools/perf/util/parse-events.y.rej
> [acme@seventh perf]$ cat tools/perf/util/parse-events.y.rej
> --- tools/perf/util/parse-events.y
> +++ tools/perf/util/parse-events.y
> @@ -232,7 +232,7 @@ PE_NAME opt_event_config
>   		YYABORT;
>   
>   	ALLOC_LIST(list);
> -	if (parse_events_add_pmu(_parse_state, list, $1, $2, false)) {
> +	if (parse_events_add_pmu(_parse_state, list, $1, $2, false, false)) {
>   		struct perf_pmu *pmu = NULL;
>   		int ok = 0;
>   		char *pattern;
> [acme@seventh perf]$
> 

[tip:perf/urgent] perf pmu: Fix core PMU alias list for X86 platform

[tip-bot for Kan Liang]

Commit-ID:  292c34c10249c64a70def442f0d977bf9d466ed7
Gitweb:     https://git.kernel.org/tip/292c34c10249c64a70def442f0d977bf9d466ed7
Author:     Kan Liang <kan.liang@linux.intel.com>
AuthorDate: Tue, 24 Apr 2018 11:20:10 -0700
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 24 Apr 2018 16:02:29 -0300

perf pmu: Fix core PMU alias list for X86 platform

When counting uncore event with alias, core event is mistakenly
involved, for example:

  perf stat --no-merge -e "unc_m_cas_count.all" -C0  sleep 1

  Performance counter stats for 'CPU(s) 0':

                 0      unc_m_cas_count.all [uncore_imc_4]
                 0      unc_m_cas_count.all [uncore_imc_2]
                 0      unc_m_cas_count.all [uncore_imc_0]
           153,640      unc_m_cas_count.all [cpu]
                 0      unc_m_cas_count.all [uncore_imc_5]
            25,026      unc_m_cas_count.all [uncore_imc_3]
                 0      unc_m_cas_count.all [uncore_imc_1]

       1.001447890 seconds time elapsed

The reason is that current implementation doesn't check PMU name of a
event when adding its alias into the alias list for core PMU. The
uncore event aliases are mistakenly added.

This bug was introduced in:
  commit 14b22ae028de ("perf pmu: Add helper function is_pmu_core to
  detect PMU CORE devices")

Checking the PMU name for all PMUs on X86 and other architectures except
ARM.
There is no behavior change for ARM.

Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Agustin Vega-Frias <agustinv@codeaurora.org>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Ganapatrao Kulkarni <ganapatrao.kulkarni@cavium.com>
Cc: Jin Yao <yao.jin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Shaokun Zhang <zhangshaokun@hisilicon.com>
Cc: Will Deacon <will.deacon@arm.com>
Fixes: 14b22ae028de ("perf pmu: Add helper function is_pmu_core to detect PMU CORE devices")
Link: http://lkml.kernel.org/r/1524594014-79243-1-git-send-email-kan.liang@linux.intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/pmu.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index af4bedf4cf98..d2fb597c9a8c 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -539,9 +539,10 @@ static bool pmu_is_uncore(const char *name)
 
 /*
  *  PMU CORE devices have different name other than cpu in sysfs on some
- *  platforms. looking for possible sysfs files to identify as core device.
+ *  platforms.
+ *  Looking for possible sysfs files to identify the arm core device.
  */
-static int is_pmu_core(const char *name)
+static int is_arm_pmu_core(const char *name)
 {
 	struct stat st;
 	char path[PATH_MAX];
@@ -550,12 +551,6 @@ static int is_pmu_core(const char *name)
 	if (!sysfs)
 		return 0;
 
-	/* Look for cpu sysfs (x86 and others) */
-	scnprintf(path, PATH_MAX, "%s/bus/event_source/devices/cpu", sysfs);
-	if ((stat(path, &st) == 0) &&
-			(strncmp(name, "cpu", strlen("cpu")) == 0))
-		return 1;
-
 	/* Look for cpu sysfs (specific to arm) */
 	scnprintf(path, PATH_MAX, "%s/bus/event_source/devices/%s/cpus",
 				sysfs, name);
@@ -668,6 +663,7 @@ static void pmu_add_cpu_aliases(struct list_head *head, struct perf_pmu *pmu)
 	struct pmu_events_map *map;
 	struct pmu_event *pe;
 	const char *name = pmu->name;
+	const char *pname;
 
 	map = perf_pmu__find_map(pmu);
 	if (!map)
@@ -686,11 +682,9 @@ static void pmu_add_cpu_aliases(struct list_head *head, struct perf_pmu *pmu)
 			break;
 		}
 
-		if (!is_pmu_core(name)) {
-			/* check for uncore devices */
-			if (pe->pmu == NULL)
-				continue;
-			if (strncmp(pe->pmu, name, strlen(pe->pmu)))
+		if (!is_arm_pmu_core(name)) {
+			pname = pe->pmu ? pe->pmu : "cpu";
+			if (strncmp(pname, name, strlen(pname)))
 				continue;
 		}
 

[tip:perf/urgent] perf stat: Print out hint for mixed PMU group error

[tip-bot for Kan Liang]

Commit-ID:  30060eaed769039c6e523b9d159f2b2858fa8907
Gitweb:     https://git.kernel.org/tip/30060eaed769039c6e523b9d159f2b2858fa8907
Author:     Kan Liang <kan.liang@linux.intel.com>
AuthorDate: Tue, 24 Apr 2018 11:20:11 -0700
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 24 Apr 2018 16:11:59 -0300

perf stat: Print out hint for mixed PMU group error

Perf doesn't support mixed events from different PMUs (except software
event) in a group. For this case, only "<not counted>" or "<not
supported>" are printed out. There is no hint which guides users to fix
the issue.

Checking the PMU type of events to determine if they are from the same
PMU. There may be false alarm for the checking. E.g. the core PMU has
different PMU type. But it should not happen often.

The false alarm can also be tolerated, because:

- It only happens on error path.
- It just provides a possible solution for the issue.

Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
Cc: Agustin Vega-Frias <agustinv@codeaurora.org>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Ganapatrao Kulkarni <ganapatrao.kulkarni@cavium.com>
Cc: Jin Yao <yao.jin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Shaokun Zhang <zhangshaokun@hisilicon.com>
Cc: Will Deacon <will.deacon@arm.com>
Link: http://lkml.kernel.org/r/1524594014-79243-2-git-send-email-kan.liang@linux.intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-stat.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 147a27e8c937..30e6b374e095 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -172,6 +172,7 @@ static bool			interval_count;
 static const char		*output_name;
 static int			output_fd;
 static int			print_free_counters_hint;
+static int			print_mixed_hw_group_error;
 
 struct perf_stat {
 	bool			 record;
@@ -1126,6 +1127,30 @@ static void abs_printout(int id, int nr, struct perf_evsel *evsel, double avg)
 		fprintf(output, "%s%s", csv_sep, evsel->cgrp->name);
 }
 
+static bool is_mixed_hw_group(struct perf_evsel *counter)
+{
+	struct perf_evlist *evlist = counter->evlist;
+	u32 pmu_type = counter->attr.type;
+	struct perf_evsel *pos;
+
+	if (counter->nr_members < 2)
+		return false;
+
+	evlist__for_each_entry(evlist, pos) {
+		/* software events can be part of any hardware group */
+		if (pos->attr.type == PERF_TYPE_SOFTWARE)
+			continue;
+		if (pmu_type == PERF_TYPE_SOFTWARE) {
+			pmu_type = pos->attr.type;
+			continue;
+		}
+		if (pmu_type != pos->attr.type)
+			return true;
+	}
+
+	return false;
+}
+
 static void printout(int id, int nr, struct perf_evsel *counter, double uval,
 		     char *prefix, u64 run, u64 ena, double noise,
 		     struct runtime_stat *st)
@@ -1178,8 +1203,11 @@ static void printout(int id, int nr, struct perf_evsel *counter, double uval,
 			counter->supported ? CNTR_NOT_COUNTED : CNTR_NOT_SUPPORTED,
 			csv_sep);
 
-		if (counter->supported)
+		if (counter->supported) {
 			print_free_counters_hint = 1;
+			if (is_mixed_hw_group(counter))
+				print_mixed_hw_group_error = 1;
+		}
 
 		fprintf(stat_config.output, "%-*s%s",
 			csv_output ? 0 : unit_width,
@@ -1757,6 +1785,11 @@ static void print_footer(void)
 "	echo 0 > /proc/sys/kernel/nmi_watchdog\n"
 "	perf stat ...\n"
 "	echo 1 > /proc/sys/kernel/nmi_watchdog\n");
+
+	if (print_mixed_hw_group_error)
+		fprintf(output,
+			"The events in group usually have to be from "
+			"the same PMU. Try reorganizing the group.\n");
 }
 
 static void print_counters(struct timespec *ts, int argc, const char **argv)

[tip:perf/urgent] perf evsel: Only fall back group read for leader

[tip-bot for Kan Liang]

Commit-ID:  121f325f34caf9a7654ec8a50e20942ed9d6dafc
Gitweb:     https://git.kernel.org/tip/121f325f34caf9a7654ec8a50e20942ed9d6dafc
Author:     Kan Liang <kan.liang@linux.intel.com>
AuthorDate: Tue, 24 Apr 2018 11:20:12 -0700
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 24 Apr 2018 16:11:59 -0300

perf evsel: Only fall back group read for leader

Perf doesn't support mixed events from different PMUs (except software
event) in a group. The perf stat should output <not counted>/<not
supported> for all events, but it doesn't. For example,

  perf stat -e '{cycles,uncore_imc_5/umask=0xF,event=0x4/,instructions}'
       <not counted>      cycles
       <not supported>    uncore_imc_5/umask=0xF,event=0x4/
           1,024,300      instructions

If perf fails to open an event, it doesn't error out directly. It will
disable some features and retry, until the event is opened or all
features are disabled. The disabled features will not be re-enabled. The
group read is one of these features.

For the example as above, the IMC event and the leader event "cycles"
are from different PMUs. Opening the IMC event must fail. The group read
feature must be disabled for IMC event and the followed event
"instructions". The "instructions" event has the same PMU as the leader
"cycles". It can be opened successfully. Since the group read feature
has been disabled, the "instructions" event will be read as a single
event, which definitely has a value.

The group read fallback is still useful for the case which kernel
doesn't support group read. It is good enough to be handled only by the
leader.

For the fallback request from members, it must be caused by an error.
The fallback only breaks the semantics of group.  Limit the group read
fallback only for the leader.

Committer testing:

On a broadwell t450s notebook:

Before:

  # perf stat -e '{cycles,unc_cbo_cache_lookup.read_i,instructions}' sleep 1

  Performance counter stats for 'sleep 1':

     <not counted>      cycles
   <not supported>      unc_cbo_cache_lookup.read_i
           818,206      instructions

       1.003170887 seconds time elapsed

  Some events weren't counted. Try disabling the NMI watchdog:
	echo 0 > /proc/sys/kernel/nmi_watchdog
	perf stat ...
	echo 1 > /proc/sys/kernel/nmi_watchdog

After:

  # perf stat -e '{cycles,unc_cbo_cache_lookup.read_i,instructions}' sleep 1

  Performance counter stats for 'sleep 1':

     <not counted>      cycles
   <not supported>      unc_cbo_cache_lookup.read_i
     <not counted>      instructions

       1.001380511 seconds time elapsed

  Some events weren't counted. Try disabling the NMI watchdog:
	echo 0 > /proc/sys/kernel/nmi_watchdog
	perf stat ...
	echo 1 > /proc/sys/kernel/nmi_watchdog
  #

Reported-by: Andi Kleen <ak@linux.intel.com>
Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Agustin Vega-Frias <agustinv@codeaurora.org>
Cc: Ganapatrao Kulkarni <ganapatrao.kulkarni@cavium.com>
Cc: Jin Yao <yao.jin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Shaokun Zhang <zhangshaokun@hisilicon.com>
Cc: Will Deacon <will.deacon@arm.com>
Fixes:  82bf311e15d2 ("perf stat: Use group read for event groups")
Link: http://lkml.kernel.org/r/1524594014-79243-3-git-send-email-kan.liang@linux.intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/evsel.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 26bdeecc0452..4cd2cf93f726 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -1925,7 +1925,8 @@ try_fallback:
 		goto fallback_missing_features;
 	} else if (!perf_missing_features.group_read &&
 		    evsel->attr.inherit &&
-		   (evsel->attr.read_format & PERF_FORMAT_GROUP)) {
+		   (evsel->attr.read_format & PERF_FORMAT_GROUP) &&
+		   perf_evsel__is_group_leader(evsel)) {
 		perf_missing_features.group_read = true;
 		pr_debug2("switching off group read\n");
 		goto fallback_missing_features;

[tip:perf/urgent] perf stat: Fix duplicate PMU name for interval print

[tip-bot for Kan Liang]

Commit-ID:  80ee8c588afde077cb0439e15129579a267916c4
Gitweb:     https://git.kernel.org/tip/80ee8c588afde077cb0439e15129579a267916c4
Author:     Kan Liang <kan.liang@linux.intel.com>
AuthorDate: Tue, 24 Apr 2018 11:20:14 -0700
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 24 Apr 2018 16:12:00 -0300

perf stat: Fix duplicate PMU name for interval print

PMU name is printed repeatedly for interval print, for example:

  perf stat --no-merge -e 'unc_m_clockticks' -a -I 1000
  #           time             counts unit events
     1.001053069        243,702,144      unc_m_clockticks [uncore_imc_4]
     1.001053069        244,268,304      unc_m_clockticks [uncore_imc_2]
     1.001053069        244,427,386      unc_m_clockticks [uncore_imc_0]
     1.001053069        244,583,760      unc_m_clockticks [uncore_imc_5]
     1.001053069        244,738,971      unc_m_clockticks [uncore_imc_3]
     1.001053069        244,880,309      unc_m_clockticks [uncore_imc_1]
     2.002024821        240,818,200      unc_m_clockticks [uncore_imc_4] [uncore_imc_4]
     2.002024821        240,767,812      unc_m_clockticks [uncore_imc_2] [uncore_imc_2]
     2.002024821        240,764,215      unc_m_clockticks [uncore_imc_0] [uncore_imc_0]
     2.002024821        240,759,504      unc_m_clockticks [uncore_imc_5] [uncore_imc_5]
     2.002024821        240,755,992      unc_m_clockticks [uncore_imc_3] [uncore_imc_3]
     2.002024821        240,750,403      unc_m_clockticks [uncore_imc_1] [uncore_imc_1]

For each print, the PMU name is unconditionally appended to the
counter->name.

Need to check the counter->name first. If the PMU name is already
appended, do nothing.

Committer notes:

Add and use perf_evsel->uniquified_name bool instead of doing the more
expensive strstr(event->name, pmu->name).

Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Agustin Vega-Frias <agustinv@codeaurora.org>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Ganapatrao Kulkarni <ganapatrao.kulkarni@cavium.com>
Cc: Jin Yao <yao.jin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Shaokun Zhang <zhangshaokun@hisilicon.com>
Cc: Will Deacon <will.deacon@arm.com>
Fixes: 8c5421c016a4 ("perf pmu: Display pmu name when printing unmerged events in stat")
Link: http://lkml.kernel.org/r/1524594014-79243-5-git-send-email-kan.liang@linux.intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-stat.c | 5 ++++-
 tools/perf/util/evsel.h   | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 30e6b374e095..f17dc601b0f3 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -1284,7 +1284,8 @@ static void uniquify_event_name(struct perf_evsel *counter)
 	char *new_name;
 	char *config;
 
-	if (!counter->pmu_name || !strncmp(counter->name, counter->pmu_name,
+	if (counter->uniquified_name ||
+	    !counter->pmu_name || !strncmp(counter->name, counter->pmu_name,
 					   strlen(counter->pmu_name)))
 		return;
 
@@ -1302,6 +1303,8 @@ static void uniquify_event_name(struct perf_evsel *counter)
 			counter->name = new_name;
 		}
 	}
+
+	counter->uniquified_name = true;
 }
 
 static void collect_all_aliases(struct perf_evsel *counter,
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index d3ee3af618ef..92ec009a292d 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -115,6 +115,7 @@ struct perf_evsel {
 	unsigned int		sample_size;
 	int			id_pos;
 	int			is_pos;
+	bool			uniquified_name;
 	bool			snapshot;
 	bool 			supported;
 	bool 			needs_swap;