[PATCH v1 00/10] Better fixes for grouping of events

[PATCH v1 00/10] Better fixes for grouping of events

[Ian Rogers]

The rules for grouping events has grown more complex. Topdown events
must be grouped, but flags like --metric-no-group and flags on metrics
don't respect this. Uncore events may be expanded using wild cards for
PMU names, but then the events need reordering so the group members
are adjacent. Rather than fixing metrics, this change fixes the main
event parsing code to first sort and then regroup evsels.

As this is shared functionality changes to it should cause
concern. The change is done with the intent of simplifying and making
more robust the grouping logic, examples are given. If additional
changes are necessary, they are most likely necessary to the
evsel__pmu_name logic as the code avoids breaking groups that are on
the same PMU. The pmu_name is tweaked in the case of software and aux
events, that use groups in a slightly different manner to conventional
events.

The code was manually tested as well as passing perf test on a Intel
tigerlake CPU with intel-pt.

Ian Rogers (10):
  libperf evlist: Avoid a use of evsel idx
  perf stat: Don't remove all grouped events when CPU maps disagree
  perf record: Early auxtrace initialization before event parsing
  perf stat: Modify the group test
  perf evsel: Limit in group test to CPUs
  perf evsel: Allow const evsel for certain accesses
  perf evsel: Add function to compute pmu_name
  perf parse-events: Pass ownership of the group name
  perf parse-events: Sort and group parsed events
  perf evsel: Remove use_uncore_alias

 tools/lib/perf/evlist.c             |  13 +-
 tools/perf/arch/x86/util/auxtrace.c |  17 +-
 tools/perf/arch/x86/util/evlist.c   |  39 ++---
 tools/perf/arch/x86/util/evsel.c    |   3 +
 tools/perf/builtin-record.c         |   6 +
 tools/perf/builtin-stat.c           |  24 ++-
 tools/perf/util/auxtrace.h          |   2 +
 tools/perf/util/evlist.h            |   2 +-
 tools/perf/util/evsel.c             |  27 ++-
 tools/perf/util/evsel.h             |   8 +-
 tools/perf/util/parse-events.c      | 253 +++++++++++++---------------
 tools/perf/util/parse-events.h      |   6 +-
 tools/perf/util/parse-events.y      |  17 +-
 tools/perf/util/pmu.c               |   6 +-
 tools/perf/util/python.c            |   2 +-
 tools/perf/util/stat-shadow.c       |   2 +-
 16 files changed, 228 insertions(+), 199 deletions(-)

-- 
2.39.2.722.g9855ee24e9-goog

[PATCH v1 01/10] libperf evlist: Avoid a use of evsel idx

[Ian Rogers]

Setting the leader iterates the list, so rather than use idx (which
may be changed through list reordering) just count the elements and
set afterwards.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/lib/perf/evlist.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/tools/lib/perf/evlist.c b/tools/lib/perf/evlist.c
index 61b637f29b82..2d6121e89ccb 100644
--- a/tools/lib/perf/evlist.c
+++ b/tools/lib/perf/evlist.c
@@ -687,15 +687,14 @@ perf_evlist__next_mmap(struct perf_evlist *evlist, struct perf_mmap *map,
 
 void __perf_evlist__set_leader(struct list_head *list, struct perf_evsel *leader)
 {
-	struct perf_evsel *first, *last, *evsel;
-
-	first = list_first_entry(list, struct perf_evsel, node);
-	last = list_last_entry(list, struct perf_evsel, node);
-
-	leader->nr_members = last->idx - first->idx + 1;
+	struct perf_evsel *evsel;
+	int n = 0;
 
-	__perf_evlist__for_each_entry(list, evsel)
+	__perf_evlist__for_each_entry(list, evsel) {
 		evsel->leader = leader;
+		n++;
+	}
+	leader->nr_members = n;
 }
 
 void perf_evlist__set_leader(struct perf_evlist *evlist)
-- 
2.39.2.722.g9855ee24e9-goog

[PATCH v1 02/10] perf stat: Don't remove all grouped events when CPU maps disagree

[Ian Rogers]

If the events in an evlist's CPU map differ then the entire group is
removed. For example:

```
$ perf stat -e '{imc_free_running/data_read/,imc_free_running/data_write/,cs}' -a sleep 1
WARNING: grouped events cpus do not match, disabling group:
  anon group { imc_free_running/data_read/, imc_free_running/data_write/, cs }
```

Change the behavior so that just the events not matching the leader
are removed. So in the example above, just 'cs' will be removed.

Modify the warning so that it is produced once for each group, rather
than once for the entire evlist. Shrink the scope and size of the
warning text buffer.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/builtin-stat.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index d70b1ec88594..5c12ae5efce5 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -181,14 +181,13 @@ static bool cpus_map_matched(struct evsel *a, struct evsel *b)
 
 static void evlist__check_cpu_maps(struct evlist *evlist)
 {
-	struct evsel *evsel, *pos, *leader;
-	char buf[1024];
+	struct evsel *evsel, *warned_leader = NULL;
 
 	if (evlist__has_hybrid(evlist))
 		evlist__warn_hybrid_group(evlist);
 
 	evlist__for_each_entry(evlist, evsel) {
-		leader = evsel__leader(evsel);
+		struct evsel *leader = evsel__leader(evsel);
 
 		/* Check that leader matches cpus with each member. */
 		if (leader == evsel)
@@ -197,19 +196,26 @@ static void evlist__check_cpu_maps(struct evlist *evlist)
 			continue;
 
 		/* If there's mismatch disable the group and warn user. */
-		WARN_ONCE(1, "WARNING: grouped events cpus do not match, disabling group:\n");
-		evsel__group_desc(leader, buf, sizeof(buf));
-		pr_warning("  %s\n", buf);
-
+		if (warned_leader != leader) {
+			char buf[200];
+
+			pr_warning("WARNING: grouped events cpus do not match.\n"
+				"Events with CPUs not matching the leader will "
+				"be removed from the group.\n");
+			evsel__group_desc(leader, buf, sizeof(buf));
+			pr_warning("  %s\n", buf);
+			warned_leader = leader;
+		}
 		if (verbose > 0) {
+			char buf[200];
+
 			cpu_map__snprint(leader->core.cpus, buf, sizeof(buf));
 			pr_warning("     %s: %s\n", leader->name, buf);
 			cpu_map__snprint(evsel->core.cpus, buf, sizeof(buf));
 			pr_warning("     %s: %s\n", evsel->name, buf);
 		}
 
-		for_each_group_evsel(pos, leader)
-			evsel__remove_from_group(pos, leader);
+		evsel__remove_from_group(evsel, leader);
 	}
 }
 
-- 
2.39.2.722.g9855ee24e9-goog

[PATCH v1 03/10] perf record: Early auxtrace initialization before event parsing

[Ian Rogers]

This allows event parsing to use the evsel__is_aux_event function,
which is important when determining event grouping.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/arch/x86/util/auxtrace.c | 17 +++++++++++++----
 tools/perf/builtin-record.c         |  6 ++++++
 tools/perf/util/auxtrace.h          |  2 ++
 3 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/tools/perf/arch/x86/util/auxtrace.c b/tools/perf/arch/x86/util/auxtrace.c
index 3da506e13f49..de1e4842ea2e 100644
--- a/tools/perf/arch/x86/util/auxtrace.c
+++ b/tools/perf/arch/x86/util/auxtrace.c
@@ -15,6 +15,19 @@
 #include "../../../util/intel-bts.h"
 #include "../../../util/evlist.h"
 
+void auxtrace__early_init(void)
+{
+	struct perf_pmu *intel_pt_pmu;
+	struct perf_pmu *intel_bts_pmu;
+
+	intel_pt_pmu = perf_pmu__find(INTEL_PT_PMU_NAME);
+	if (intel_pt_pmu)
+		intel_pt_pmu->auxtrace = true;
+	intel_bts_pmu = perf_pmu__find(INTEL_BTS_PMU_NAME);
+	if (intel_bts_pmu)
+		intel_bts_pmu->auxtrace = true;
+}
+
 static
 struct auxtrace_record *auxtrace_record__init_intel(struct evlist *evlist,
 						    int *err)
@@ -26,11 +39,7 @@ struct auxtrace_record *auxtrace_record__init_intel(struct evlist *evlist,
 	bool found_bts = false;
 
 	intel_pt_pmu = perf_pmu__find(INTEL_PT_PMU_NAME);
-	if (intel_pt_pmu)
-		intel_pt_pmu->auxtrace = true;
 	intel_bts_pmu = perf_pmu__find(INTEL_BTS_PMU_NAME);
-	if (intel_bts_pmu)
-		intel_bts_pmu->auxtrace = true;
 
 	evlist__for_each_entry(evlist, evsel) {
 		if (intel_pt_pmu && evsel->core.attr.type == intel_pt_pmu->type)
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 8374117e66f6..a0870c076dc0 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -3940,6 +3940,10 @@ static int record__init_thread_masks(struct record *rec)
 	return ret;
 }
 
+__weak void auxtrace__early_init(void)
+{
+}
+
 int cmd_record(int argc, const char **argv)
 {
 	int err;
@@ -3985,6 +3989,8 @@ int cmd_record(int argc, const char **argv)
 	if (err)
 		return err;
 
+	auxtrace__early_init();
+
 	argc = parse_options(argc, argv, record_options, record_usage,
 			    PARSE_OPT_STOP_AT_NON_OPTION);
 	if (quiet)
diff --git a/tools/perf/util/auxtrace.h b/tools/perf/util/auxtrace.h
index 29eb82dff574..49a86aa6ac94 100644
--- a/tools/perf/util/auxtrace.h
+++ b/tools/perf/util/auxtrace.h
@@ -457,6 +457,8 @@ struct addr_filters {
 
 struct auxtrace_cache;
 
+void auxtrace__early_init(void);
+
 #ifdef HAVE_AUXTRACE_SUPPORT
 
 u64 compat_auxtrace_mmap__read_head(struct auxtrace_mmap *mm);
-- 
2.39.2.722.g9855ee24e9-goog

[PATCH v1 04/10] perf stat: Modify the group test

[Ian Rogers]

Previously nr_members would be 0 for an event with no group. The
previous change made that count 1, the event is its own leader without
a group. Make the find_stat logic consistent with this, an improvement
suggested by Namhyung Kim.

Suggested-by: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/stat-shadow.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
index ef85f1ae1ab2..eeccab6751d7 100644
--- a/tools/perf/util/stat-shadow.c
+++ b/tools/perf/util/stat-shadow.c
@@ -163,7 +163,7 @@ static double find_stat(const struct evsel *evsel, int aggr_idx, enum stat_type
 			continue;
 
 		/* Ignore evsels that are part of different groups. */
-		if (evsel->core.leader->nr_members &&
+		if (evsel->core.leader->nr_members > 1 &&
 		    evsel->core.leader != cur->core.leader)
 			continue;
 		/* Ignore evsels with mismatched modifiers. */
-- 
2.39.2.722.g9855ee24e9-goog

[PATCH v1 05/10] perf evsel: Limit in group test to CPUs

[Ian Rogers]

Don't just match on the event name, restict based on the PMU too.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/arch/x86/util/evsel.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/arch/x86/util/evsel.c b/tools/perf/arch/x86/util/evsel.c
index ea3972d785d1..580b0a172136 100644
--- a/tools/perf/arch/x86/util/evsel.c
+++ b/tools/perf/arch/x86/util/evsel.c
@@ -61,6 +61,9 @@ bool arch_evsel__must_be_in_group(const struct evsel *evsel)
 	if (!evsel__sys_has_perf_metrics(evsel))
 		return false;
 
+	if (evsel->pmu_name && strncmp(evsel->pmu_name, "cpu", 3))
+		return false;
+
 	return evsel->name &&
 		(strcasestr(evsel->name, "slots") ||
 		 strcasestr(evsel->name, "topdown"));
-- 
2.39.2.722.g9855ee24e9-goog

[PATCH v1 06/10] perf evsel: Allow const evsel for certain accesses

[Ian Rogers]

List sorting, added later to evlist, passes const elements requiring
helper functions to also be const. Make the argument to
evsel__find_pmu, evsel__is_aux_event and evsel__leader const.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/evsel.c  | 2 +-
 tools/perf/util/evsel.h  | 6 +++---
 tools/perf/util/pmu.c    | 6 +++---
 tools/perf/util/python.c | 2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 51e8ce6edddc..2dc2c24252bb 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -3139,7 +3139,7 @@ bool evsel__is_hybrid(const struct evsel *evsel)
 	return evsel->pmu_name && perf_pmu__is_hybrid(evsel->pmu_name);
 }
 
-struct evsel *evsel__leader(struct evsel *evsel)
+struct evsel *evsel__leader(const struct evsel *evsel)
 {
 	return container_of(evsel->core.leader, struct evsel, core);
 }
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 814a49ebb7e3..676c499323e9 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -212,8 +212,8 @@ int evsel__object_config(size_t object_size,
 			 int (*init)(struct evsel *evsel),
 			 void (*fini)(struct evsel *evsel));
 
-struct perf_pmu *evsel__find_pmu(struct evsel *evsel);
-bool evsel__is_aux_event(struct evsel *evsel);
+struct perf_pmu *evsel__find_pmu(const struct evsel *evsel);
+bool evsel__is_aux_event(const struct evsel *evsel);
 
 struct evsel *evsel__new_idx(struct perf_event_attr *attr, int idx);
 
@@ -505,7 +505,7 @@ int evsel__store_ids(struct evsel *evsel, struct evlist *evlist);
 
 void evsel__zero_per_pkg(struct evsel *evsel);
 bool evsel__is_hybrid(const struct evsel *evsel);
-struct evsel *evsel__leader(struct evsel *evsel);
+struct evsel *evsel__leader(const struct evsel *evsel);
 bool evsel__has_leader(struct evsel *evsel, struct evsel *leader);
 bool evsel__is_leader(struct evsel *evsel);
 void evsel__set_leader(struct evsel *evsel, struct evsel *leader);
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 43b6182d96b7..45d9b8e28e16 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -988,7 +988,7 @@ struct perf_pmu *perf_pmu__scan(struct perf_pmu *pmu)
 	return NULL;
 }
 
-struct perf_pmu *evsel__find_pmu(struct evsel *evsel)
+struct perf_pmu *evsel__find_pmu(const struct evsel *evsel)
 {
 	struct perf_pmu *pmu = NULL;
 
@@ -1000,11 +1000,11 @@ struct perf_pmu *evsel__find_pmu(struct evsel *evsel)
 			break;
 	}
 
-	evsel->pmu = pmu;
+	((struct evsel *)evsel)->pmu = pmu;
 	return pmu;
 }
 
-bool evsel__is_aux_event(struct evsel *evsel)
+bool evsel__is_aux_event(const struct evsel *evsel)
 {
 	struct perf_pmu *pmu = evsel__find_pmu(evsel);
 
diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
index 42e8b813d010..ab48ffbb6448 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -83,7 +83,7 @@ const char *perf_env__arch(struct perf_env *env __maybe_unused)
  * far, for the perf python binding known usecases, revisit if this become
  * necessary.
  */
-struct perf_pmu *evsel__find_pmu(struct evsel *evsel __maybe_unused)
+struct perf_pmu *evsel__find_pmu(const struct evsel *evsel __maybe_unused)
 {
 	return NULL;
 }
-- 
2.39.2.722.g9855ee24e9-goog

[PATCH v1 07/10] perf evsel: Add function to compute pmu_name

[Ian Rogers]

The computed pmu_name respects software events and aux event groups,
such that the pmu_name is changed to be that of the aux event leader
or group leader for software events. This is done as a later change
will split events that are in different PMUs into different groups.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/evsel.c | 24 ++++++++++++++++++++++++
 tools/perf/util/evsel.h |  1 +
 2 files changed, 25 insertions(+)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 2dc2c24252bb..9c6b486f8bd4 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -821,6 +821,30 @@ const char *evsel__name(struct evsel *evsel)
 	return "unknown";
 }
 
+const char *evsel__pmu_name(const struct evsel *evsel)
+{
+	const struct evsel *leader;
+
+	/* If the pmu_name is set use it. pmu_name isn't set for CPU and software events. */
+	if (evsel->pmu_name)
+		return evsel->pmu_name;
+	/*
+	 * Software events may be in a group with other uncore PMU events. Use
+	 * the pmu_name of the group leader to avoid breaking the software event
+	 * out of the group.
+	 *
+	 * Aux event leaders, like intel_pt, expect a group with events from
+	 * other PMUs, so substitute the AUX event's PMU in this case.
+	 */
+	leader  = evsel__leader(evsel);
+	if ((evsel->core.attr.type == PERF_TYPE_SOFTWARE || evsel__is_aux_event(leader)) &&
+	    leader->pmu_name) {
+		return leader->pmu_name;
+	}
+
+	return "cpu";
+}
+
 const char *evsel__metric_id(const struct evsel *evsel)
 {
 	if (evsel->metric_id)
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 676c499323e9..72121194d3b1 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -280,6 +280,7 @@ int arch_evsel__hw_name(struct evsel *evsel, char *bf, size_t size);
 
 int __evsel__hw_cache_type_op_res_name(u8 type, u8 op, u8 result, char *bf, size_t size);
 const char *evsel__name(struct evsel *evsel);
+const char *evsel__pmu_name(const struct evsel *evsel);
 const char *evsel__metric_id(const struct evsel *evsel);
 
 static inline bool evsel__is_tool(const struct evsel *evsel)
-- 
2.39.2.722.g9855ee24e9-goog

[PATCH v1 08/10] perf parse-events: Pass ownership of the group name

[Ian Rogers]

Pass ownership of the group name rather than copying and freeing the
original. This saves a memory allocation and copy.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/parse-events.c | 3 ++-
 tools/perf/util/parse-events.y | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 0336ff27c15f..1be454697d57 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1761,6 +1761,7 @@ parse_events__set_leader_for_uncore_aliase(char *name, struct list_head *list,
 
 handled:
 	ret = 1;
+	free(name);
 out:
 	free(leaders);
 	return ret;
@@ -1786,7 +1787,7 @@ void parse_events__set_leader(char *name, struct list_head *list,
 
 	leader = arch_evlist__leader(list);
 	__perf_evlist__set_leader(list, &leader->core);
-	leader->group_name = name ? strdup(name) : NULL;
+	leader->group_name = name;
 	list_move(&leader->core.node, list);
 }
 
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index be8c51770051..541b8dde2063 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -202,8 +202,8 @@ PE_NAME '{' events '}'
 	struct list_head *list = $3;
 
 	inc_group_count(list, _parse_state);
+	/* Takes ownership of $1. */
 	parse_events__set_leader($1, list, _parse_state);
-	free($1);
 	$$ = list;
 }
 |
-- 
2.39.2.722.g9855ee24e9-goog

[PATCH v1 09/10] perf parse-events: Sort and group parsed events

[Ian Rogers]

This change is intended to be a no-op for most current cases, the
default sort order is the order the events were parsed. Where it
varies is in how groups are handled. Previously an uncore and core
event that are grouped would most often cause the group to be removed:

```
$ perf stat -e '{instructions,uncore_imc_free_running_0/data_total/}' -a sleep 1
WARNING: grouped events cpus do not match, disabling group:
  anon group { instructions, uncore_imc_free_running_0/data_total/ }
...
```

However, when wildcards are used the events should be re-sorted and
re-grouped in parse_events__set_leader, but this currently fails for
simple examples:

```
$ perf stat -e '{uncore_imc_free_running/data_read/,uncore_imc_free_running/data_write/}' -a sleep 1

 Performance counter stats for 'system wide':

     <not counted> MiB  uncore_imc_free_running/data_read/
     <not counted> MiB  uncore_imc_free_running/data_write/

       1.000996992 seconds time elapsed
```

A futher failure mode, fixed in this patch, is to force topdown events
into a group.

This change moves sorting the evsels in the evlist after parsing. It
requires parsing to set up groups. First the evsels are sorted
respecting the existing groupings and parse order, but also reordering
to ensure evsels of the same PMU and group appear together. So that
software and aux events respect groups, their pmu_name is taken from
the group leader. The sorting is done with list_sort removing a memory
allocation.

After sorting a pass is done to correct the group leaders and for
topdown events ensuring they have a group leader.

This fixes the problems seen before:

```
$ perf stat -e '{uncore_imc_free_running/data_read/,uncore_imc_free_running/data_write/}' -a sleep 1

 Performance counter stats for 'system wide':

            727.42 MiB  uncore_imc_free_running/data_read/
             81.84 MiB  uncore_imc_free_running/data_write/

       1.000948615 seconds time elapsed
```

As well as making groups not fail for cases like:

```
$ perf stat -e '{imc_free_running_0/data_total/,imc_free_running_1/data_total/}' -a sleep 1

 Performance counter stats for 'system wide':

            256.47 MiB  imc_free_running_0/data_total/
            256.48 MiB  imc_free_running_1/data_total/

       1.001165442 seconds time elapsed
```

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/arch/x86/util/evlist.c |  39 ++---
 tools/perf/util/evlist.h          |   2 +-
 tools/perf/util/parse-events.c    | 240 +++++++++++++++---------------
 tools/perf/util/parse-events.h    |   3 +-
 tools/perf/util/parse-events.y    |   4 +-
 5 files changed, 136 insertions(+), 152 deletions(-)

diff --git a/tools/perf/arch/x86/util/evlist.c b/tools/perf/arch/x86/util/evlist.c
index 8a7ae4162563..d4193479a364 100644
--- a/tools/perf/arch/x86/util/evlist.c
+++ b/tools/perf/arch/x86/util/evlist.c
@@ -65,29 +65,22 @@ int arch_evlist__add_default_attrs(struct evlist *evlist,
 	return ___evlist__add_default_attrs(evlist, attrs, nr_attrs);
 }
 
-struct evsel *arch_evlist__leader(struct list_head *list)
+int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
 {
-	struct evsel *evsel, *first, *slots = NULL;
-	bool has_topdown = false;
-
-	first = list_first_entry(list, struct evsel, core.node);
-
-	if (!topdown_sys_has_perf_metrics())
-		return first;
-
-	/* If there is a slots event and a topdown event then the slots event comes first. */
-	__evlist__for_each_entry(list, evsel) {
-		if (evsel->pmu_name && !strncmp(evsel->pmu_name, "cpu", 3) && evsel->name) {
-			if (strcasestr(evsel->name, "slots")) {
-				slots = evsel;
-				if (slots == first)
-					return first;
-			}
-			if (strcasestr(evsel->name, "topdown"))
-				has_topdown = true;
-			if (slots && has_topdown)
-				return slots;
-		}
+	if (topdown_sys_has_perf_metrics() &&
+	    (!lhs->pmu_name || !strncmp(lhs->pmu_name, "cpu", 3))) {
+		/* Ensure the topdown slots comes first. */
+		if (strcasestr(lhs->name, "slots"))
+			return -1;
+		if (strcasestr(rhs->name, "slots"))
+			return 1;
+		/* Followed by topdown events. */
+		if (strcasestr(lhs->name, "topdown") && !strcasestr(rhs->name, "topdown"))
+			return -1;
+		if (!strcasestr(lhs->name, "topdown") && strcasestr(rhs->name, "topdown"))
+			return 1;
 	}
-	return first;
+
+	/* Default ordering by insertion index. */
+	return lhs->core.idx - rhs->core.idx;
 }
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index 01fa9d592c5a..d89d8f92802b 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -119,7 +119,7 @@ int arch_evlist__add_default_attrs(struct evlist *evlist,
 #define evlist__add_default_attrs(evlist, array) \
 	arch_evlist__add_default_attrs(evlist, array, ARRAY_SIZE(array))
 
-struct evsel *arch_evlist__leader(struct list_head *list);
+int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs);
 
 int evlist__add_dummy(struct evlist *evlist);
 struct evsel *evlist__add_aux_dummy(struct evlist *evlist, bool system_wide);
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 1be454697d57..d6eb0a54dd2d 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <linux/hw_breakpoint.h>
 #include <linux/err.h>
+#include <linux/list_sort.h>
 #include <linux/zalloc.h>
 #include <dirent.h>
 #include <errno.h>
@@ -1655,125 +1656,7 @@ int parse_events__modifier_group(struct list_head *list,
 	return parse_events__modifier_event(list, event_mod, true);
 }
 
-/*
- * 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 (strncmp(pmu_name_a, pmu_name_b, end_a - pmu_name_a) == 0);
-}
-
-static int
-parse_events__set_leader_for_uncore_aliase(char *name, struct list_head *list,
-					   struct parse_events_state *parse_state)
-{
-	struct evsel *evsel, *leader;
-	uintptr_t *leaders;
-	bool is_leader = true;
-	int i, nr_pmu = 0, total_members, ret = 0;
-
-	leader = list_first_entry(list, struct evsel, core.node);
-	evsel = list_last_entry(list, struct evsel, core.node);
-	total_members = evsel->core.idx - leader->core.idx + 1;
-
-	leaders = calloc(total_members, sizeof(uintptr_t));
-	if (WARN_ON(!leaders))
-		return 0;
-
-	/*
-	 * Going through the whole group and doing sanity check.
-	 * All members must use alias, and be from the same uncore block.
-	 * Also, storing the leader events in an array.
-	 */
-	__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 (!is_leader)
-			continue;
-		/*
-		 * If the event's PMU name starts to repeat, it must be a new
-		 * event. That can be used to distinguish the leader from
-		 * other members, even they have the same event name.
-		 */
-		if ((leader != evsel) &&
-		    !strcmp(leader->pmu_name, evsel->pmu_name)) {
-			is_leader = false;
-			continue;
-		}
-
-		/* Store the leader event for each PMU */
-		leaders[nr_pmu++] = (uintptr_t) evsel;
-	}
-
-	/* only one event alias */
-	if (nr_pmu == total_members) {
-		parse_state->nr_groups--;
-		goto handled;
-	}
-
-	/*
-	 * 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. The big group has to be split into multiple small groups
-	 * which only include the events from the same PMU.
-	 *
-	 * Here the uncore event aliases must be from the same uncore block.
-	 * The number of PMUs must be same for each alias. The number of new
-	 * small groups equals to the number of PMUs.
-	 * Setting the leader event for corresponding members in each group.
-	 */
-	i = 0;
-	__evlist__for_each_entry(list, evsel) {
-		if (i >= nr_pmu)
-			i = 0;
-		evsel__set_leader(evsel, (struct evsel *) leaders[i++]);
-	}
-
-	/* The number of members and group name are same for each group */
-	for (i = 0; i < nr_pmu; i++) {
-		evsel = (struct evsel *) leaders[i];
-		evsel->core.nr_members = total_members / nr_pmu;
-		evsel->group_name = name ? strdup(name) : NULL;
-	}
-
-	/* Take the new small groups into account */
-	parse_state->nr_groups += nr_pmu - 1;
-
-handled:
-	ret = 1;
-	free(name);
-out:
-	free(leaders);
-	return ret;
-}
-
-__weak struct evsel *arch_evlist__leader(struct list_head *list)
-{
-	return list_first_entry(list, struct evsel, core.node);
-}
-
-void parse_events__set_leader(char *name, struct list_head *list,
-			      struct parse_events_state *parse_state)
+void parse_events__set_leader(char *name, struct list_head *list)
 {
 	struct evsel *leader;
 
@@ -1782,13 +1665,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;
-
-	leader = arch_evlist__leader(list);
+	leader = list_first_entry(list, struct evsel, core.node);
 	__perf_evlist__set_leader(list, &leader->core);
 	leader->group_name = name;
-	list_move(&leader->core.node, list);
 }
 
 /* list_event is assumed to point to malloc'ed memory */
@@ -2245,6 +2124,117 @@ static int parse_events__with_hybrid_pmu(struct parse_events_state *parse_state,
 	return ret;
 }
 
+__weak int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
+{
+	/* Order by insertion index. */
+	return lhs->core.idx - rhs->core.idx;
+}
+
+static int evlist__cmp(void *state, const struct list_head *l, const struct list_head *r)
+{
+	const struct perf_evsel *lhs_core = container_of(l, struct perf_evsel, node);
+	const struct evsel *lhs = container_of(lhs_core, struct evsel, core);
+	const struct perf_evsel *rhs_core = container_of(r, struct perf_evsel, node);
+	const struct evsel *rhs = container_of(rhs_core, struct evsel, core);
+	int *leader_idx = state;
+	int lhs_leader_idx = *leader_idx, rhs_leader_idx = *leader_idx, ret;
+	const char *lhs_pmu_name, *rhs_pmu_name;
+
+	/*
+	 * First sort by grouping/leader. Read the leader idx only if the evsel
+	 * is part of a group, as -1 indicates no group.
+	 */
+	if (lhs_core->leader != lhs_core || lhs_core->nr_members > 1)
+		lhs_leader_idx = lhs_core->leader->idx;
+	if (rhs_core->leader != rhs_core || rhs_core->nr_members > 1)
+		rhs_leader_idx = rhs_core->leader->idx;
+
+	if (lhs_leader_idx != rhs_leader_idx)
+		return lhs_leader_idx - rhs_leader_idx;
+
+	/* Group by PMU. Groups can't span PMUs. */
+	lhs_pmu_name = evsel__pmu_name(lhs);
+	rhs_pmu_name = evsel__pmu_name(rhs);
+	ret = strcmp(lhs_pmu_name, rhs_pmu_name);
+	if (ret)
+		return ret;
+
+	/* Architecture specific sorting. */
+	return arch_evlist__cmp(lhs, rhs);
+}
+
+static void parse_events__sort_events_and_fix_groups(struct list_head *list)
+{
+	int idx = -1;
+	struct evsel *pos, *cur_leader = NULL;
+	struct perf_evsel *cur_leaders_grp = NULL;
+
+	/*
+	 * Compute index to insert ungrouped events at. Place them where the
+	 * first ungrouped event appears.
+	 */
+	list_for_each_entry(pos, list, core.node) {
+		const struct evsel *pos_leader = evsel__leader(pos);
+
+		if (pos != pos_leader || pos->core.nr_members > 1)
+			continue;
+
+		idx = pos->core.idx;
+		break;
+	}
+
+	/* Sort events. */
+	list_sort(&idx, list, evlist__cmp);
+
+	/*
+	 * Recompute groups, splitting for PMUs and adding groups for events
+	 * that require them.
+	 */
+	idx = 0;
+	list_for_each_entry(pos, list, core.node) {
+		const struct evsel *pos_leader = evsel__leader(pos);
+		const char *pos_pmu_name = evsel__pmu_name(pos);
+		const char *cur_leader_pmu_name, *pos_leader_pmu_name;
+		bool force_grouped = arch_evsel__must_be_in_group(pos);
+
+		/* Reset index and nr_members. */
+		pos->core.idx = idx++;
+		pos->core.nr_members = 0;
+
+		/*
+		 * Set the group leader respecting the given groupings and that
+		 * groups can't span PMUs.
+		 */
+		if (!cur_leader)
+			cur_leader = pos;
+
+		cur_leader_pmu_name = evsel__pmu_name(cur_leader);
+		if ((cur_leaders_grp != pos->core.leader && !force_grouped) ||
+		    strcmp(cur_leader_pmu_name, pos_pmu_name)) {
+			/* Event is for a different group/PMU than last. */
+			cur_leader = pos;
+			/*
+			 * Remember the leader's group before it is overwritten,
+			 * so that later events match as being in the same
+			 * group.
+			 */
+			cur_leaders_grp = pos->core.leader;
+		}
+		pos_leader_pmu_name = evsel__pmu_name(pos_leader);
+		if (strcmp(pos_leader_pmu_name, pos_pmu_name) || force_grouped) {
+			/*
+			 * Event's PMU differs from its leader's. Groups can't
+			 * span PMUs, so update leader from the group/PMU
+			 * tracker.
+			 */
+			evsel__set_leader(pos, cur_leader);
+		}
+	}
+	list_for_each_entry(pos, list, core.node) {
+		pos->core.leader->nr_members++;
+	}
+}
+
 int __parse_events(struct evlist *evlist, const char *str,
 		   struct parse_events_error *err, struct perf_pmu *fake_pmu)
 {
@@ -2266,6 +2256,8 @@ int __parse_events(struct evlist *evlist, const char *str,
 		return -1;
 	}
 
+	parse_events__sort_events_and_fix_groups(&parse_state.list);
+
 	/*
 	 * Add list to the evlist even with errors to allow callers to clean up.
 	 */
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 428e72eaafcc..22fc11b0bd59 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -200,8 +200,7 @@ 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,
-			      struct parse_events_state *parse_state);
+void parse_events__set_leader(char *name, struct list_head *list);
 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 541b8dde2063..90d12f2bc8be 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -203,7 +203,7 @@ PE_NAME '{' events '}'
 
 	inc_group_count(list, _parse_state);
 	/* Takes ownership of $1. */
-	parse_events__set_leader($1, list, _parse_state);
+	parse_events__set_leader($1, list);
 	$$ = list;
 }
 |
@@ -212,7 +212,7 @@ PE_NAME '{' events '}'
 	struct list_head *list = $2;
 
 	inc_group_count(list, _parse_state);
-	parse_events__set_leader(NULL, list, _parse_state);
+	parse_events__set_leader(NULL, list);
 	$$ = list;
 }
 
-- 
2.39.2.722.g9855ee24e9-goog

[PATCH v1 10/10] perf evsel: Remove use_uncore_alias

[Ian Rogers]

This flag used to be used when regrouping uncore events in particular
due to wildcard matches. This is now handled by sorting evlist and so
the flag is redundant.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/evsel.c        |  1 -
 tools/perf/util/evsel.h        |  1 -
 tools/perf/util/parse-events.c | 12 +++---------
 tools/perf/util/parse-events.h |  3 +--
 tools/perf/util/parse-events.y | 11 +++++++----
 5 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 9c6b486f8bd4..5446128be03b 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -458,7 +458,6 @@ struct evsel *evsel__clone(struct evsel *orig)
 	evsel->per_pkg = orig->per_pkg;
 	evsel->percore = orig->percore;
 	evsel->precise_max = orig->precise_max;
-	evsel->use_uncore_alias = orig->use_uncore_alias;
 	evsel->is_libpfm_event = orig->is_libpfm_event;
 
 	evsel->exclude_GH = orig->exclude_GH;
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 72121194d3b1..9a8d08fcad1c 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -89,7 +89,6 @@ struct evsel {
 		bool			per_pkg;
 		bool			percore;
 		bool			precise_max;
-		bool			use_uncore_alias;
 		bool			is_libpfm_event;
 		bool			auto_merge_stats;
 		bool			collect_stat;
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index d6eb0a54dd2d..ac7709c1c5b7 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1445,15 +1445,13 @@ static int parse_events__inside_hybrid_pmu(struct parse_events_state *parse_stat
 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,
-			 bool use_alias)
+			 bool auto_merge_stats)
 {
 	struct perf_event_attr attr;
 	struct perf_pmu_info info;
 	struct perf_pmu *pmu;
 	struct evsel *evsel;
 	struct parse_events_error *err = parse_state->error;
-	bool use_uncore_alias;
 	LIST_HEAD(config_terms);
 
 	pmu = parse_state->fake_pmu ?: perf_pmu__find(name);
@@ -1488,8 +1486,6 @@ 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,
@@ -1499,7 +1495,6 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
 				    /*cpu_list=*/NULL);
 		if (evsel) {
 			evsel->pmu_name = name ? strdup(name) : NULL;
-			evsel->use_uncore_alias = use_uncore_alias;
 			return 0;
 		} else {
 			return -ENOMEM;
@@ -1560,7 +1555,6 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
 		evsel->use_config_name = true;
 
 	evsel->pmu_name = name ? strdup(name) : NULL;
-	evsel->use_uncore_alias = use_uncore_alias;
 	evsel->percore = config_term_percore(&evsel->config_terms);
 
 	if (parse_state->fake_pmu)
@@ -1622,7 +1616,7 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 				parse_events_copy_term_list(head, &orig_head);
 				if (!parse_events_add_pmu(parse_state, list,
 							  pmu->name, orig_head,
-							  true, true)) {
+							  /*auto_merge_stats=*/true)) {
 					pr_debug("%s -> %s/%s/\n", str,
 						 pmu->name, alias->str);
 					ok++;
@@ -1634,7 +1628,7 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 
 	if (parse_state->fake_pmu) {
 		if (!parse_events_add_pmu(parse_state, list, str, head,
-					  true, true)) {
+					  /*auto_merge_stats=*/true)) {
 			pr_debug("%s -> %s/%s/\n", str, "fake_pmu", str);
 			ok++;
 		}
diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
index 22fc11b0bd59..fdac44dc696b 100644
--- a/tools/perf/util/parse-events.h
+++ b/tools/perf/util/parse-events.h
@@ -183,8 +183,7 @@ int parse_events_add_breakpoint(struct list_head *list, int *idx,
 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,
-			 bool use_alias);
+			 bool auto_merge_stats);
 
 struct evsel *parse_events__add_event(int idx, struct perf_event_attr *attr,
 				      const char *name, const char *metric_id,
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 90d12f2bc8be..f1b153c72d67 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -313,7 +313,7 @@ event_pmu_name opt_pmu_config
 	list = alloc_list();
 	if (!list)
 		CLEANUP_YYABORT;
-	if (parse_events_add_pmu(_parse_state, list, $1, $2, false, false)) {
+	if (parse_events_add_pmu(_parse_state, list, $1, $2, /*auto_merge_stats=*/false)) {
 		struct perf_pmu *pmu = NULL;
 		int ok = 0;
 
@@ -330,8 +330,10 @@ event_pmu_name opt_pmu_config
 			    !perf_pmu__match(pattern, pmu->alias_name, $1)) {
 				if (parse_events_copy_term_list(orig_terms, &terms))
 					CLEANUP_YYABORT;
-				if (!parse_events_add_pmu(_parse_state, list, pmu->name, terms, true, false))
+				if (!parse_events_add_pmu(_parse_state, list, pmu->name, terms,
+							  /*auto_merge_stats=*/true)) {
 					ok++;
+				}
 				parse_events_terms__delete(terms);
 			}
 		}
@@ -407,7 +409,8 @@ PE_PMU_EVENT_FAKE sep_dc
 	if (!list)
 		YYABORT;
 
-	err = parse_events_add_pmu(_parse_state, list, $1, NULL, false, false);
+	err = parse_events_add_pmu(_parse_state, list, $1, /*head_config=*/NULL,
+				   /*auto_merge_stats=*/false);
 	free($1);
 	if (err < 0) {
 		free(list);
@@ -425,7 +428,7 @@ PE_PMU_EVENT_FAKE opt_pmu_config
 	if (!list)
 		YYABORT;
 
-	err = parse_events_add_pmu(_parse_state, list, $1, $2, false, false);
+	err = parse_events_add_pmu(_parse_state, list, $1, $2, /*auto_merge_stats=*/false);
 	free($1);
 	parse_events_terms__delete($2);
 	if (err < 0) {
-- 
2.39.2.722.g9855ee24e9-goog

Re: [PATCH v1 03/10] perf record: Early auxtrace initialization before event parsing

[Arnaldo Carvalho de Melo]

Em Wed, Mar 01, 2023 at 08:12:04PM -0800, Ian Rogers escreveu:
> This allows event parsing to use the evsel__is_aux_event function,
> which is important when determining event grouping.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/arch/x86/util/auxtrace.c | 17 +++++++++++++----
>  tools/perf/builtin-record.c         |  6 ++++++
>  tools/perf/util/auxtrace.h          |  2 ++
>  3 files changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/arch/x86/util/auxtrace.c b/tools/perf/arch/x86/util/auxtrace.c
> index 3da506e13f49..de1e4842ea2e 100644
> --- a/tools/perf/arch/x86/util/auxtrace.c
> +++ b/tools/perf/arch/x86/util/auxtrace.c
> @@ -15,6 +15,19 @@
>  #include "../../../util/intel-bts.h"
>  #include "../../../util/evlist.h"
>  
> +void auxtrace__early_init(void)
> +{
> +	struct perf_pmu *intel_pt_pmu;
> +	struct perf_pmu *intel_bts_pmu;
> +
> +	intel_pt_pmu = perf_pmu__find(INTEL_PT_PMU_NAME);
> +	if (intel_pt_pmu)
> +		intel_pt_pmu->auxtrace = true;
> +	intel_bts_pmu = perf_pmu__find(INTEL_BTS_PMU_NAME);
> +	if (intel_bts_pmu)
> +		intel_bts_pmu->auxtrace = true;
> +}
> +
>  static
>  struct auxtrace_record *auxtrace_record__init_intel(struct evlist *evlist,
>  						    int *err)
> @@ -26,11 +39,7 @@ struct auxtrace_record *auxtrace_record__init_intel(struct evlist *evlist,
>  	bool found_bts = false;
>  
>  	intel_pt_pmu = perf_pmu__find(INTEL_PT_PMU_NAME);
> -	if (intel_pt_pmu)
> -		intel_pt_pmu->auxtrace = true;

In this case, can't we do it as:

	if (intel_pt_pmu == NULL && intel_bts_pmu == NULL)
		auxtrace__early_init();

To avoid possibly doing the finds again?

- Arnaldo

>  	intel_bts_pmu = perf_pmu__find(INTEL_BTS_PMU_NAME);
> -	if (intel_bts_pmu)
> -		intel_bts_pmu->auxtrace = true;
>  
>  	evlist__for_each_entry(evlist, evsel) {
>  		if (intel_pt_pmu && evsel->core.attr.type == intel_pt_pmu->type)
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 8374117e66f6..a0870c076dc0 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -3940,6 +3940,10 @@ static int record__init_thread_masks(struct record *rec)
>  	return ret;
>  }
>  
> +__weak void auxtrace__early_init(void)
> +{
> +}
> +
>  int cmd_record(int argc, const char **argv)
>  {
>  	int err;
> @@ -3985,6 +3989,8 @@ int cmd_record(int argc, const char **argv)
>  	if (err)
>  		return err;
>  
> +	auxtrace__early_init();
> +
>  	argc = parse_options(argc, argv, record_options, record_usage,
>  			    PARSE_OPT_STOP_AT_NON_OPTION);
>  	if (quiet)
> diff --git a/tools/perf/util/auxtrace.h b/tools/perf/util/auxtrace.h
> index 29eb82dff574..49a86aa6ac94 100644
> --- a/tools/perf/util/auxtrace.h
> +++ b/tools/perf/util/auxtrace.h
> @@ -457,6 +457,8 @@ struct addr_filters {
>  
>  struct auxtrace_cache;
>  
> +void auxtrace__early_init(void);
> +
>  #ifdef HAVE_AUXTRACE_SUPPORT
>  
>  u64 compat_auxtrace_mmap__read_head(struct auxtrace_mmap *mm);
> -- 
> 2.39.2.722.g9855ee24e9-goog
> 

-- 

- Arnaldo

Re: [PATCH v1 01/10] libperf evlist: Avoid a use of evsel idx

[Arnaldo Carvalho de Melo]

Em Wed, Mar 01, 2023 at 08:12:02PM -0800, Ian Rogers escreveu:
> Setting the leader iterates the list, so rather than use idx (which
> may be changed through list reordering) just count the elements and
> set afterwards.

Looks ok
 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/lib/perf/evlist.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/lib/perf/evlist.c b/tools/lib/perf/evlist.c
> index 61b637f29b82..2d6121e89ccb 100644
> --- a/tools/lib/perf/evlist.c
> +++ b/tools/lib/perf/evlist.c
> @@ -687,15 +687,14 @@ perf_evlist__next_mmap(struct perf_evlist *evlist, struct perf_mmap *map,
>  
>  void __perf_evlist__set_leader(struct list_head *list, struct perf_evsel *leader)
>  {
> -	struct perf_evsel *first, *last, *evsel;
> -
> -	first = list_first_entry(list, struct perf_evsel, node);
> -	last = list_last_entry(list, struct perf_evsel, node);
> -
> -	leader->nr_members = last->idx - first->idx + 1;
> +	struct perf_evsel *evsel;
> +	int n = 0;
>  
> -	__perf_evlist__for_each_entry(list, evsel)
> +	__perf_evlist__for_each_entry(list, evsel) {
>  		evsel->leader = leader;
> +		n++;
> +	}
> +	leader->nr_members = n;
>  }
>  
>  void perf_evlist__set_leader(struct perf_evlist *evlist)
> -- 
> 2.39.2.722.g9855ee24e9-goog
> 

-- 

- Arnaldo

Re: [PATCH v1 02/10] perf stat: Don't remove all grouped events when CPU maps disagree

[Arnaldo Carvalho de Melo]

Em Wed, Mar 01, 2023 at 08:12:03PM -0800, Ian Rogers escreveu:
> If the events in an evlist's CPU map differ then the entire group is
> removed. For example:
> 
> ```
> $ perf stat -e '{imc_free_running/data_read/,imc_free_running/data_write/,cs}' -a sleep 1
> WARNING: grouped events cpus do not match, disabling group:
>   anon group { imc_free_running/data_read/, imc_free_running/data_write/, cs }
> ```
> 
> Change the behavior so that just the events not matching the leader
> are removed. So in the example above, just 'cs' will be removed.

Its a change in behaviour but a good one, I think, Jiri?

- Arnaldo
 
> Modify the warning so that it is produced once for each group, rather
> than once for the entire evlist. Shrink the scope and size of the
> warning text buffer.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/builtin-stat.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index d70b1ec88594..5c12ae5efce5 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -181,14 +181,13 @@ static bool cpus_map_matched(struct evsel *a, struct evsel *b)
>  
>  static void evlist__check_cpu_maps(struct evlist *evlist)
>  {
> -	struct evsel *evsel, *pos, *leader;
> -	char buf[1024];
> +	struct evsel *evsel, *warned_leader = NULL;
>  
>  	if (evlist__has_hybrid(evlist))
>  		evlist__warn_hybrid_group(evlist);
>  
>  	evlist__for_each_entry(evlist, evsel) {
> -		leader = evsel__leader(evsel);
> +		struct evsel *leader = evsel__leader(evsel);
>  
>  		/* Check that leader matches cpus with each member. */
>  		if (leader == evsel)
> @@ -197,19 +196,26 @@ static void evlist__check_cpu_maps(struct evlist *evlist)
>  			continue;
>  
>  		/* If there's mismatch disable the group and warn user. */
> -		WARN_ONCE(1, "WARNING: grouped events cpus do not match, disabling group:\n");
> -		evsel__group_desc(leader, buf, sizeof(buf));
> -		pr_warning("  %s\n", buf);
> -
> +		if (warned_leader != leader) {
> +			char buf[200];
> +
> +			pr_warning("WARNING: grouped events cpus do not match.\n"
> +				"Events with CPUs not matching the leader will "
> +				"be removed from the group.\n");
> +			evsel__group_desc(leader, buf, sizeof(buf));
> +			pr_warning("  %s\n", buf);
> +			warned_leader = leader;
> +		}
>  		if (verbose > 0) {
> +			char buf[200];
> +
>  			cpu_map__snprint(leader->core.cpus, buf, sizeof(buf));
>  			pr_warning("     %s: %s\n", leader->name, buf);
>  			cpu_map__snprint(evsel->core.cpus, buf, sizeof(buf));
>  			pr_warning("     %s: %s\n", evsel->name, buf);
>  		}
>  
> -		for_each_group_evsel(pos, leader)
> -			evsel__remove_from_group(pos, leader);
> +		evsel__remove_from_group(evsel, leader);
>  	}
>  }
>  
> -- 
> 2.39.2.722.g9855ee24e9-goog
> 

-- 

- Arnaldo

Re: [PATCH v1 04/10] perf stat: Modify the group test

[Arnaldo Carvalho de Melo]

Em Wed, Mar 01, 2023 at 08:12:05PM -0800, Ian Rogers escreveu:
> Previously nr_members would be 0 for an event with no group. The
> previous change made that count 1, the event is its own leader without
> a group. Make the find_stat logic consistent with this, an improvement
> suggested by Namhyung Kim.

Is this the only place where this change in behaviour needs to be taken
into account?

- Arnaldo
 
> Suggested-by: Namhyung Kim <namhyung@kernel.org>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/stat-shadow.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
> index ef85f1ae1ab2..eeccab6751d7 100644
> --- a/tools/perf/util/stat-shadow.c
> +++ b/tools/perf/util/stat-shadow.c
> @@ -163,7 +163,7 @@ static double find_stat(const struct evsel *evsel, int aggr_idx, enum stat_type
>  			continue;
>  
>  		/* Ignore evsels that are part of different groups. */
> -		if (evsel->core.leader->nr_members &&
> +		if (evsel->core.leader->nr_members > 1 &&
>  		    evsel->core.leader != cur->core.leader)
>  			continue;
>  		/* Ignore evsels with mismatched modifiers. */
> -- 
> 2.39.2.722.g9855ee24e9-goog
> 

-- 

- Arnaldo

Re: [PATCH v1 05/10] perf evsel: Limit in group test to CPUs

[Arnaldo Carvalho de Melo]

Em Wed, Mar 01, 2023 at 08:12:06PM -0800, Ian Rogers escreveu:
> Don't just match on the event name, restict based on the PMU too.

restrict.

Can you please expand a bit this explanation?

- Arnaldo
 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/arch/x86/util/evsel.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tools/perf/arch/x86/util/evsel.c b/tools/perf/arch/x86/util/evsel.c
> index ea3972d785d1..580b0a172136 100644
> --- a/tools/perf/arch/x86/util/evsel.c
> +++ b/tools/perf/arch/x86/util/evsel.c
> @@ -61,6 +61,9 @@ bool arch_evsel__must_be_in_group(const struct evsel *evsel)
>  	if (!evsel__sys_has_perf_metrics(evsel))
>  		return false;
>  
> +	if (evsel->pmu_name && strncmp(evsel->pmu_name, "cpu", 3))
> +		return false;
> +
>  	return evsel->name &&
>  		(strcasestr(evsel->name, "slots") ||
>  		 strcasestr(evsel->name, "topdown"));
> -- 
> 2.39.2.722.g9855ee24e9-goog
> 

-- 

- Arnaldo

Re: [PATCH v1 06/10] perf evsel: Allow const evsel for certain accesses

[Arnaldo Carvalho de Melo]

Em Wed, Mar 01, 2023 at 08:12:07PM -0800, Ian Rogers escreveu:
> List sorting, added later to evlist, passes const elements requiring
> helper functions to also be const. Make the argument to
> evsel__find_pmu, evsel__is_aux_event and evsel__leader const.

But then you have to de-constify it at some point :-\

- Arnaldo
 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/evsel.c  | 2 +-
>  tools/perf/util/evsel.h  | 6 +++---
>  tools/perf/util/pmu.c    | 6 +++---
>  tools/perf/util/python.c | 2 +-
>  4 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 51e8ce6edddc..2dc2c24252bb 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -3139,7 +3139,7 @@ bool evsel__is_hybrid(const struct evsel *evsel)
>  	return evsel->pmu_name && perf_pmu__is_hybrid(evsel->pmu_name);
>  }
>  
> -struct evsel *evsel__leader(struct evsel *evsel)
> +struct evsel *evsel__leader(const struct evsel *evsel)
>  {
>  	return container_of(evsel->core.leader, struct evsel, core);
>  }
> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> index 814a49ebb7e3..676c499323e9 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -212,8 +212,8 @@ int evsel__object_config(size_t object_size,
>  			 int (*init)(struct evsel *evsel),
>  			 void (*fini)(struct evsel *evsel));
>  
> -struct perf_pmu *evsel__find_pmu(struct evsel *evsel);
> -bool evsel__is_aux_event(struct evsel *evsel);
> +struct perf_pmu *evsel__find_pmu(const struct evsel *evsel);
> +bool evsel__is_aux_event(const struct evsel *evsel);
>  
>  struct evsel *evsel__new_idx(struct perf_event_attr *attr, int idx);
>  
> @@ -505,7 +505,7 @@ int evsel__store_ids(struct evsel *evsel, struct evlist *evlist);
>  
>  void evsel__zero_per_pkg(struct evsel *evsel);
>  bool evsel__is_hybrid(const struct evsel *evsel);
> -struct evsel *evsel__leader(struct evsel *evsel);
> +struct evsel *evsel__leader(const struct evsel *evsel);
>  bool evsel__has_leader(struct evsel *evsel, struct evsel *leader);
>  bool evsel__is_leader(struct evsel *evsel);
>  void evsel__set_leader(struct evsel *evsel, struct evsel *leader);
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 43b6182d96b7..45d9b8e28e16 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -988,7 +988,7 @@ struct perf_pmu *perf_pmu__scan(struct perf_pmu *pmu)
>  	return NULL;
>  }
>  
> -struct perf_pmu *evsel__find_pmu(struct evsel *evsel)
> +struct perf_pmu *evsel__find_pmu(const struct evsel *evsel)
>  {
>  	struct perf_pmu *pmu = NULL;
>  
> @@ -1000,11 +1000,11 @@ struct perf_pmu *evsel__find_pmu(struct evsel *evsel)
>  			break;
>  	}
>  
> -	evsel->pmu = pmu;
> +	((struct evsel *)evsel)->pmu = pmu;

Hu

>  	return pmu;
>  }
>  
> -bool evsel__is_aux_event(struct evsel *evsel)
> +bool evsel__is_aux_event(const struct evsel *evsel)
>  {
>  	struct perf_pmu *pmu = evsel__find_pmu(evsel);
>  
> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index 42e8b813d010..ab48ffbb6448 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c
> @@ -83,7 +83,7 @@ const char *perf_env__arch(struct perf_env *env __maybe_unused)
>   * far, for the perf python binding known usecases, revisit if this become
>   * necessary.
>   */
> -struct perf_pmu *evsel__find_pmu(struct evsel *evsel __maybe_unused)
> +struct perf_pmu *evsel__find_pmu(const struct evsel *evsel __maybe_unused)
>  {
>  	return NULL;
>  }
> -- 
> 2.39.2.722.g9855ee24e9-goog
> 

-- 

- Arnaldo

Re: [PATCH v1 07/10] perf evsel: Add function to compute pmu_name

[Arnaldo Carvalho de Melo]

Em Wed, Mar 01, 2023 at 08:12:08PM -0800, Ian Rogers escreveu:
> The computed pmu_name respects software events and aux event groups,
> such that the pmu_name is changed to be that of the aux event leader
> or group leader for software events. This is done as a later change
> will split events that are in different PMUs into different groups.

Adrian, can you please take a look and provide an Ack or Reviewed-by?

- Arnaldo
 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/evsel.c | 24 ++++++++++++++++++++++++
>  tools/perf/util/evsel.h |  1 +
>  2 files changed, 25 insertions(+)
> 
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 2dc2c24252bb..9c6b486f8bd4 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -821,6 +821,30 @@ const char *evsel__name(struct evsel *evsel)
>  	return "unknown";
>  }
>  
> +const char *evsel__pmu_name(const struct evsel *evsel)
> +{
> +	const struct evsel *leader;
> +
> +	/* If the pmu_name is set use it. pmu_name isn't set for CPU and software events. */
> +	if (evsel->pmu_name)
> +		return evsel->pmu_name;
> +	/*
> +	 * Software events may be in a group with other uncore PMU events. Use
> +	 * the pmu_name of the group leader to avoid breaking the software event
> +	 * out of the group.
> +	 *
> +	 * Aux event leaders, like intel_pt, expect a group with events from
> +	 * other PMUs, so substitute the AUX event's PMU in this case.
> +	 */
> +	leader  = evsel__leader(evsel);
> +	if ((evsel->core.attr.type == PERF_TYPE_SOFTWARE || evsel__is_aux_event(leader)) &&
> +	    leader->pmu_name) {
> +		return leader->pmu_name;
> +	}
> +
> +	return "cpu";
> +}
> +
>  const char *evsel__metric_id(const struct evsel *evsel)
>  {
>  	if (evsel->metric_id)
> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> index 676c499323e9..72121194d3b1 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -280,6 +280,7 @@ int arch_evsel__hw_name(struct evsel *evsel, char *bf, size_t size);
>  
>  int __evsel__hw_cache_type_op_res_name(u8 type, u8 op, u8 result, char *bf, size_t size);
>  const char *evsel__name(struct evsel *evsel);
> +const char *evsel__pmu_name(const struct evsel *evsel);
>  const char *evsel__metric_id(const struct evsel *evsel);
>  
>  static inline bool evsel__is_tool(const struct evsel *evsel)
> -- 
> 2.39.2.722.g9855ee24e9-goog
> 

-- 

- Arnaldo

Re: [PATCH v1 08/10] perf parse-events: Pass ownership of the group name

[Arnaldo Carvalho de Melo]

Em Wed, Mar 01, 2023 at 08:12:09PM -0800, Ian Rogers escreveu:
> Pass ownership of the group name rather than copying and freeing the
> original. This saves a memory allocation and copy.

Looks ok.

- Arnaldo
 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/parse-events.c | 3 ++-
>  tools/perf/util/parse-events.y | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 0336ff27c15f..1be454697d57 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -1761,6 +1761,7 @@ parse_events__set_leader_for_uncore_aliase(char *name, struct list_head *list,
>  
>  handled:
>  	ret = 1;
> +	free(name);
>  out:
>  	free(leaders);
>  	return ret;
> @@ -1786,7 +1787,7 @@ void parse_events__set_leader(char *name, struct list_head *list,
>  
>  	leader = arch_evlist__leader(list);
>  	__perf_evlist__set_leader(list, &leader->core);
> -	leader->group_name = name ? strdup(name) : NULL;
> +	leader->group_name = name;
>  	list_move(&leader->core.node, list);
>  }
>  
> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> index be8c51770051..541b8dde2063 100644
> --- a/tools/perf/util/parse-events.y
> +++ b/tools/perf/util/parse-events.y
> @@ -202,8 +202,8 @@ PE_NAME '{' events '}'
>  	struct list_head *list = $3;
>  
>  	inc_group_count(list, _parse_state);
> +	/* Takes ownership of $1. */
>  	parse_events__set_leader($1, list, _parse_state);
> -	free($1);
>  	$$ = list;
>  }
>  |
> -- 
> 2.39.2.722.g9855ee24e9-goog
> 

-- 

- Arnaldo

Re: [PATCH v1 09/10] perf parse-events: Sort and group parsed events

[Arnaldo Carvalho de Melo]

Em Wed, Mar 01, 2023 at 08:12:10PM -0800, Ian Rogers escreveu:
> This change is intended to be a no-op for most current cases, the
> default sort order is the order the events were parsed. Where it
> varies is in how groups are handled. Previously an uncore and core
> event that are grouped would most often cause the group to be removed:

And shouldn't that continue to be the case? I.e. the user asks for
events to be in a group, but that can't be performed, so the tool fails
and states that those events can't be in a group, looks sensible, no?

Perhaps the tool should continue to fail but provide an alternative
command line where multiple groups are specified, the user gets educated
and uses that instead? Probably the logic would be similar to what you
seem to be doing here?

- Arnaldo

> 
> ```
> $ perf stat -e '{instructions,uncore_imc_free_running_0/data_total/}' -a sleep 1
> WARNING: grouped events cpus do not match, disabling group:
>   anon group { instructions, uncore_imc_free_running_0/data_total/ }
> ...
> ```
> 
> However, when wildcards are used the events should be re-sorted and
> re-grouped in parse_events__set_leader, but this currently fails for
> simple examples:
> 
> ```
> $ perf stat -e '{uncore_imc_free_running/data_read/,uncore_imc_free_running/data_write/}' -a sleep 1

Next time can you state what is the system where this can be tested?
Saves a bit of time looking for where to test this.
 
>  Performance counter stats for 'system wide':
> 
>      <not counted> MiB  uncore_imc_free_running/data_read/
>      <not counted> MiB  uncore_imc_free_running/data_write/
> 
>        1.000996992 seconds time elapsed
> ```
> 
> A futher failure mode, fixed in this patch, is to force topdown events

further

> into a group.
> 
> This change moves sorting the evsels in the evlist after parsing. It
> requires parsing to set up groups. First the evsels are sorted
> respecting the existing groupings and parse order, but also reordering
> to ensure evsels of the same PMU and group appear together. So that
> software and aux events respect groups, their pmu_name is taken from
> the group leader. The sorting is done with list_sort removing a memory
> allocation.
> 
> After sorting a pass is done to correct the group leaders and for
> topdown events ensuring they have a group leader.
> 
> This fixes the problems seen before:
> 
> ```
> $ perf stat -e '{uncore_imc_free_running/data_read/,uncore_imc_free_running/data_write/}' -a sleep 1
> 
>  Performance counter stats for 'system wide':
> 
>             727.42 MiB  uncore_imc_free_running/data_read/
>              81.84 MiB  uncore_imc_free_running/data_write/
> 
>        1.000948615 seconds time elapsed
> ```
> 
> As well as making groups not fail for cases like:
> 
> ```
> $ perf stat -e '{imc_free_running_0/data_total/,imc_free_running_1/data_total/}' -a sleep 1
> 
>  Performance counter stats for 'system wide':
> 
>             256.47 MiB  imc_free_running_0/data_total/
>             256.48 MiB  imc_free_running_1/data_total/
> 
>        1.001165442 seconds time elapsed
> ```
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/arch/x86/util/evlist.c |  39 ++---
>  tools/perf/util/evlist.h          |   2 +-
>  tools/perf/util/parse-events.c    | 240 +++++++++++++++---------------
>  tools/perf/util/parse-events.h    |   3 +-
>  tools/perf/util/parse-events.y    |   4 +-
>  5 files changed, 136 insertions(+), 152 deletions(-)
> 
> diff --git a/tools/perf/arch/x86/util/evlist.c b/tools/perf/arch/x86/util/evlist.c
> index 8a7ae4162563..d4193479a364 100644
> --- a/tools/perf/arch/x86/util/evlist.c
> +++ b/tools/perf/arch/x86/util/evlist.c
> @@ -65,29 +65,22 @@ int arch_evlist__add_default_attrs(struct evlist *evlist,
>  	return ___evlist__add_default_attrs(evlist, attrs, nr_attrs);
>  }
>  
> -struct evsel *arch_evlist__leader(struct list_head *list)
> +int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
>  {
> -	struct evsel *evsel, *first, *slots = NULL;
> -	bool has_topdown = false;
> -
> -	first = list_first_entry(list, struct evsel, core.node);
> -
> -	if (!topdown_sys_has_perf_metrics())
> -		return first;
> -
> -	/* If there is a slots event and a topdown event then the slots event comes first. */
> -	__evlist__for_each_entry(list, evsel) {
> -		if (evsel->pmu_name && !strncmp(evsel->pmu_name, "cpu", 3) && evsel->name) {
> -			if (strcasestr(evsel->name, "slots")) {
> -				slots = evsel;
> -				if (slots == first)
> -					return first;
> -			}
> -			if (strcasestr(evsel->name, "topdown"))
> -				has_topdown = true;
> -			if (slots && has_topdown)
> -				return slots;
> -		}
> +	if (topdown_sys_has_perf_metrics() &&
> +	    (!lhs->pmu_name || !strncmp(lhs->pmu_name, "cpu", 3))) {
> +		/* Ensure the topdown slots comes first. */
> +		if (strcasestr(lhs->name, "slots"))
> +			return -1;
> +		if (strcasestr(rhs->name, "slots"))
> +			return 1;
> +		/* Followed by topdown events. */
> +		if (strcasestr(lhs->name, "topdown") && !strcasestr(rhs->name, "topdown"))
> +			return -1;
> +		if (!strcasestr(lhs->name, "topdown") && strcasestr(rhs->name, "topdown"))
> +			return 1;
>  	}
> -	return first;
> +
> +	/* Default ordering by insertion index. */
> +	return lhs->core.idx - rhs->core.idx;
>  }
> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
> index 01fa9d592c5a..d89d8f92802b 100644
> --- a/tools/perf/util/evlist.h
> +++ b/tools/perf/util/evlist.h
> @@ -119,7 +119,7 @@ int arch_evlist__add_default_attrs(struct evlist *evlist,
>  #define evlist__add_default_attrs(evlist, array) \
>  	arch_evlist__add_default_attrs(evlist, array, ARRAY_SIZE(array))
>  
> -struct evsel *arch_evlist__leader(struct list_head *list);
> +int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs);
>  
>  int evlist__add_dummy(struct evlist *evlist);
>  struct evsel *evlist__add_aux_dummy(struct evlist *evlist, bool system_wide);
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 1be454697d57..d6eb0a54dd2d 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include <linux/hw_breakpoint.h>
>  #include <linux/err.h>
> +#include <linux/list_sort.h>
>  #include <linux/zalloc.h>
>  #include <dirent.h>
>  #include <errno.h>
> @@ -1655,125 +1656,7 @@ int parse_events__modifier_group(struct list_head *list,
>  	return parse_events__modifier_event(list, event_mod, true);
>  }
>  
> -/*
> - * 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 (strncmp(pmu_name_a, pmu_name_b, end_a - pmu_name_a) == 0);
> -}
> -
> -static int
> -parse_events__set_leader_for_uncore_aliase(char *name, struct list_head *list,
> -					   struct parse_events_state *parse_state)
> -{
> -	struct evsel *evsel, *leader;
> -	uintptr_t *leaders;
> -	bool is_leader = true;
> -	int i, nr_pmu = 0, total_members, ret = 0;
> -
> -	leader = list_first_entry(list, struct evsel, core.node);
> -	evsel = list_last_entry(list, struct evsel, core.node);
> -	total_members = evsel->core.idx - leader->core.idx + 1;
> -
> -	leaders = calloc(total_members, sizeof(uintptr_t));
> -	if (WARN_ON(!leaders))
> -		return 0;
> -
> -	/*
> -	 * Going through the whole group and doing sanity check.
> -	 * All members must use alias, and be from the same uncore block.
> -	 * Also, storing the leader events in an array.
> -	 */
> -	__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 (!is_leader)
> -			continue;
> -		/*
> -		 * If the event's PMU name starts to repeat, it must be a new
> -		 * event. That can be used to distinguish the leader from
> -		 * other members, even they have the same event name.
> -		 */
> -		if ((leader != evsel) &&
> -		    !strcmp(leader->pmu_name, evsel->pmu_name)) {
> -			is_leader = false;
> -			continue;
> -		}
> -
> -		/* Store the leader event for each PMU */
> -		leaders[nr_pmu++] = (uintptr_t) evsel;
> -	}
> -
> -	/* only one event alias */
> -	if (nr_pmu == total_members) {
> -		parse_state->nr_groups--;
> -		goto handled;
> -	}
> -
> -	/*
> -	 * 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. The big group has to be split into multiple small groups
> -	 * which only include the events from the same PMU.
> -	 *
> -	 * Here the uncore event aliases must be from the same uncore block.
> -	 * The number of PMUs must be same for each alias. The number of new
> -	 * small groups equals to the number of PMUs.
> -	 * Setting the leader event for corresponding members in each group.
> -	 */
> -	i = 0;
> -	__evlist__for_each_entry(list, evsel) {
> -		if (i >= nr_pmu)
> -			i = 0;
> -		evsel__set_leader(evsel, (struct evsel *) leaders[i++]);
> -	}
> -
> -	/* The number of members and group name are same for each group */
> -	for (i = 0; i < nr_pmu; i++) {
> -		evsel = (struct evsel *) leaders[i];
> -		evsel->core.nr_members = total_members / nr_pmu;
> -		evsel->group_name = name ? strdup(name) : NULL;
> -	}
> -
> -	/* Take the new small groups into account */
> -	parse_state->nr_groups += nr_pmu - 1;
> -
> -handled:
> -	ret = 1;
> -	free(name);
> -out:
> -	free(leaders);
> -	return ret;
> -}
> -
> -__weak struct evsel *arch_evlist__leader(struct list_head *list)
> -{
> -	return list_first_entry(list, struct evsel, core.node);
> -}
> -
> -void parse_events__set_leader(char *name, struct list_head *list,
> -			      struct parse_events_state *parse_state)
> +void parse_events__set_leader(char *name, struct list_head *list)
>  {
>  	struct evsel *leader;
>  
> @@ -1782,13 +1665,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;
> -
> -	leader = arch_evlist__leader(list);
> +	leader = list_first_entry(list, struct evsel, core.node);
>  	__perf_evlist__set_leader(list, &leader->core);
>  	leader->group_name = name;
> -	list_move(&leader->core.node, list);
>  }
>  
>  /* list_event is assumed to point to malloc'ed memory */
> @@ -2245,6 +2124,117 @@ static int parse_events__with_hybrid_pmu(struct parse_events_state *parse_state,
>  	return ret;
>  }
>  
> +__weak int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
> +{
> +	/* Order by insertion index. */
> +	return lhs->core.idx - rhs->core.idx;
> +}
> +
> +static int evlist__cmp(void *state, const struct list_head *l, const struct list_head *r)
> +{
> +	const struct perf_evsel *lhs_core = container_of(l, struct perf_evsel, node);
> +	const struct evsel *lhs = container_of(lhs_core, struct evsel, core);
> +	const struct perf_evsel *rhs_core = container_of(r, struct perf_evsel, node);
> +	const struct evsel *rhs = container_of(rhs_core, struct evsel, core);
> +	int *leader_idx = state;
> +	int lhs_leader_idx = *leader_idx, rhs_leader_idx = *leader_idx, ret;
> +	const char *lhs_pmu_name, *rhs_pmu_name;
> +
> +	/*
> +	 * First sort by grouping/leader. Read the leader idx only if the evsel
> +	 * is part of a group, as -1 indicates no group.
> +	 */
> +	if (lhs_core->leader != lhs_core || lhs_core->nr_members > 1)
> +		lhs_leader_idx = lhs_core->leader->idx;
> +	if (rhs_core->leader != rhs_core || rhs_core->nr_members > 1)
> +		rhs_leader_idx = rhs_core->leader->idx;
> +
> +	if (lhs_leader_idx != rhs_leader_idx)
> +		return lhs_leader_idx - rhs_leader_idx;
> +
> +	/* Group by PMU. Groups can't span PMUs. */
> +	lhs_pmu_name = evsel__pmu_name(lhs);
> +	rhs_pmu_name = evsel__pmu_name(rhs);
> +	ret = strcmp(lhs_pmu_name, rhs_pmu_name);
> +	if (ret)
> +		return ret;
> +
> +	/* Architecture specific sorting. */
> +	return arch_evlist__cmp(lhs, rhs);
> +}
> +
> +static void parse_events__sort_events_and_fix_groups(struct list_head *list)
> +{
> +	int idx = -1;
> +	struct evsel *pos, *cur_leader = NULL;
> +	struct perf_evsel *cur_leaders_grp = NULL;
> +
> +	/*
> +	 * Compute index to insert ungrouped events at. Place them where the
> +	 * first ungrouped event appears.
> +	 */
> +	list_for_each_entry(pos, list, core.node) {
> +		const struct evsel *pos_leader = evsel__leader(pos);
> +
> +		if (pos != pos_leader || pos->core.nr_members > 1)
> +			continue;
> +
> +		idx = pos->core.idx;
> +		break;
> +	}
> +
> +	/* Sort events. */
> +	list_sort(&idx, list, evlist__cmp);
> +
> +	/*
> +	 * Recompute groups, splitting for PMUs and adding groups for events
> +	 * that require them.
> +	 */
> +	idx = 0;
> +	list_for_each_entry(pos, list, core.node) {
> +		const struct evsel *pos_leader = evsel__leader(pos);
> +		const char *pos_pmu_name = evsel__pmu_name(pos);
> +		const char *cur_leader_pmu_name, *pos_leader_pmu_name;
> +		bool force_grouped = arch_evsel__must_be_in_group(pos);
> +
> +		/* Reset index and nr_members. */
> +		pos->core.idx = idx++;
> +		pos->core.nr_members = 0;
> +
> +		/*
> +		 * Set the group leader respecting the given groupings and that
> +		 * groups can't span PMUs.
> +		 */
> +		if (!cur_leader)
> +			cur_leader = pos;
> +
> +		cur_leader_pmu_name = evsel__pmu_name(cur_leader);
> +		if ((cur_leaders_grp != pos->core.leader && !force_grouped) ||
> +		    strcmp(cur_leader_pmu_name, pos_pmu_name)) {
> +			/* Event is for a different group/PMU than last. */
> +			cur_leader = pos;
> +			/*
> +			 * Remember the leader's group before it is overwritten,
> +			 * so that later events match as being in the same
> +			 * group.
> +			 */
> +			cur_leaders_grp = pos->core.leader;
> +		}
> +		pos_leader_pmu_name = evsel__pmu_name(pos_leader);
> +		if (strcmp(pos_leader_pmu_name, pos_pmu_name) || force_grouped) {
> +			/*
> +			 * Event's PMU differs from its leader's. Groups can't
> +			 * span PMUs, so update leader from the group/PMU
> +			 * tracker.
> +			 */
> +			evsel__set_leader(pos, cur_leader);
> +		}
> +	}
> +	list_for_each_entry(pos, list, core.node) {
> +		pos->core.leader->nr_members++;
> +	}
> +}
> +
>  int __parse_events(struct evlist *evlist, const char *str,
>  		   struct parse_events_error *err, struct perf_pmu *fake_pmu)
>  {
> @@ -2266,6 +2256,8 @@ int __parse_events(struct evlist *evlist, const char *str,
>  		return -1;
>  	}
>  
> +	parse_events__sort_events_and_fix_groups(&parse_state.list);
> +
>  	/*
>  	 * Add list to the evlist even with errors to allow callers to clean up.
>  	 */
> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index 428e72eaafcc..22fc11b0bd59 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -200,8 +200,7 @@ 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,
> -			      struct parse_events_state *parse_state);
> +void parse_events__set_leader(char *name, struct list_head *list);
>  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 541b8dde2063..90d12f2bc8be 100644
> --- a/tools/perf/util/parse-events.y
> +++ b/tools/perf/util/parse-events.y
> @@ -203,7 +203,7 @@ PE_NAME '{' events '}'
>  
>  	inc_group_count(list, _parse_state);
>  	/* Takes ownership of $1. */
> -	parse_events__set_leader($1, list, _parse_state);
> +	parse_events__set_leader($1, list);
>  	$$ = list;
>  }
>  |
> @@ -212,7 +212,7 @@ PE_NAME '{' events '}'
>  	struct list_head *list = $2;
>  
>  	inc_group_count(list, _parse_state);
> -	parse_events__set_leader(NULL, list, _parse_state);
> +	parse_events__set_leader(NULL, list);
>  	$$ = list;
>  }
>  
> -- 
> 2.39.2.722.g9855ee24e9-goog
> 

-- 

- Arnaldo

Re: [PATCH v1 05/10] perf evsel: Limit in group test to CPUs

[Liang, Kan]

On 2023-03-01 11:12 p.m., Ian Rogers wrote:
> Don't just match on the event name, restict based on the PMU too.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/arch/x86/util/evsel.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tools/perf/arch/x86/util/evsel.c b/tools/perf/arch/x86/util/evsel.c
> index ea3972d785d1..580b0a172136 100644
> --- a/tools/perf/arch/x86/util/evsel.c
> +++ b/tools/perf/arch/x86/util/evsel.c
> @@ -61,6 +61,9 @@ bool arch_evsel__must_be_in_group(const struct evsel *evsel)
>  	if (!evsel__sys_has_perf_metrics(evsel))
>  		return false;
>  
> +	if (evsel->pmu_name && strncmp(evsel->pmu_name, "cpu", 3))
> +		return false;

I'm not sure why we want to check the pmu name. It seems better to move
it into evsel__sys_has_perf_metrics(), since perf_metrics is a core PMU
only feature.

I think the strncmp(evsel->pmu_name, "cpu", 3) is to check whether it is
a core PMU. It is also used in other places. I think it's better to
factor out it, e.g., arch_evsel__is_core_pmu(). It will deliver a clear
message of what we are doing here.

Thanks,
Kan
> +
>  	return evsel->name &&
>  		(strcasestr(evsel->name, "slots") ||
>  		 strcasestr(evsel->name, "topdown"));

Re: [PATCH v1 03/10] perf record: Early auxtrace initialization before event parsing

[Ian Rogers]

On Thu, Mar 2, 2023 at 6:32 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>
> Em Wed, Mar 01, 2023 at 08:12:04PM -0800, Ian Rogers escreveu:
> > This allows event parsing to use the evsel__is_aux_event function,
> > which is important when determining event grouping.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/arch/x86/util/auxtrace.c | 17 +++++++++++++----
> >  tools/perf/builtin-record.c         |  6 ++++++
> >  tools/perf/util/auxtrace.h          |  2 ++
> >  3 files changed, 21 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/perf/arch/x86/util/auxtrace.c b/tools/perf/arch/x86/util/auxtrace.c
> > index 3da506e13f49..de1e4842ea2e 100644
> > --- a/tools/perf/arch/x86/util/auxtrace.c
> > +++ b/tools/perf/arch/x86/util/auxtrace.c
> > @@ -15,6 +15,19 @@
> >  #include "../../../util/intel-bts.h"
> >  #include "../../../util/evlist.h"
> >
> > +void auxtrace__early_init(void)
> > +{
> > +     struct perf_pmu *intel_pt_pmu;
> > +     struct perf_pmu *intel_bts_pmu;
> > +
> > +     intel_pt_pmu = perf_pmu__find(INTEL_PT_PMU_NAME);
> > +     if (intel_pt_pmu)
> > +             intel_pt_pmu->auxtrace = true;
> > +     intel_bts_pmu = perf_pmu__find(INTEL_BTS_PMU_NAME);
> > +     if (intel_bts_pmu)
> > +             intel_bts_pmu->auxtrace = true;
> > +}
> > +
> >  static
> >  struct auxtrace_record *auxtrace_record__init_intel(struct evlist *evlist,
> >                                                   int *err)
> > @@ -26,11 +39,7 @@ struct auxtrace_record *auxtrace_record__init_intel(struct evlist *evlist,
> >       bool found_bts = false;
> >
> >       intel_pt_pmu = perf_pmu__find(INTEL_PT_PMU_NAME);
> > -     if (intel_pt_pmu)
> > -             intel_pt_pmu->auxtrace = true;
>
> In this case, can't we do it as:
>
>         if (intel_pt_pmu == NULL && intel_bts_pmu == NULL)
>                 auxtrace__early_init();
>
> To avoid possibly doing the finds again?
>
> - Arnaldo

So this code is called after parse events and has the evlist. The
early init code is called before parse events so that the aux trace
PMUs are flagged for the sake of the is_aux call in evsel__pmu_name,
as aux events must be grouped with their different PMU type leader. It
won't be possible to combine the calls but I plan to look at
restructuring how PMUs are accessed so that searches are less
necessary.

Thanks,
Ian

> >       intel_bts_pmu = perf_pmu__find(INTEL_BTS_PMU_NAME);
> > -     if (intel_bts_pmu)
> > -             intel_bts_pmu->auxtrace = true;
> >
> >       evlist__for_each_entry(evlist, evsel) {
> >               if (intel_pt_pmu && evsel->core.attr.type == intel_pt_pmu->type)
> > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> > index 8374117e66f6..a0870c076dc0 100644
> > --- a/tools/perf/builtin-record.c
> > +++ b/tools/perf/builtin-record.c
> > @@ -3940,6 +3940,10 @@ static int record__init_thread_masks(struct record *rec)
> >       return ret;
> >  }
> >
> > +__weak void auxtrace__early_init(void)
> > +{
> > +}
> > +
> >  int cmd_record(int argc, const char **argv)
> >  {
> >       int err;
> > @@ -3985,6 +3989,8 @@ int cmd_record(int argc, const char **argv)
> >       if (err)
> >               return err;
> >
> > +     auxtrace__early_init();
> > +
> >       argc = parse_options(argc, argv, record_options, record_usage,
> >                           PARSE_OPT_STOP_AT_NON_OPTION);
> >       if (quiet)
> > diff --git a/tools/perf/util/auxtrace.h b/tools/perf/util/auxtrace.h
> > index 29eb82dff574..49a86aa6ac94 100644
> > --- a/tools/perf/util/auxtrace.h
> > +++ b/tools/perf/util/auxtrace.h
> > @@ -457,6 +457,8 @@ struct addr_filters {
> >
> >  struct auxtrace_cache;
> >
> > +void auxtrace__early_init(void);
> > +
> >  #ifdef HAVE_AUXTRACE_SUPPORT
> >
> >  u64 compat_auxtrace_mmap__read_head(struct auxtrace_mmap *mm);
> > --
> > 2.39.2.722.g9855ee24e9-goog
> >
>
> --
>
> - Arnaldo

Re: [PATCH v1 04/10] perf stat: Modify the group test

[Ian Rogers]

On Thu, Mar 2, 2023 at 6:34 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>
> Em Wed, Mar 01, 2023 at 08:12:05PM -0800, Ian Rogers escreveu:
> > Previously nr_members would be 0 for an event with no group. The
> > previous change made that count 1, the event is its own leader without
> > a group. Make the find_stat logic consistent with this, an improvement
> > suggested by Namhyung Kim.
>
> Is this the only place where this change in behaviour needs to be taken
> into account?
>
> - Arnaldo

Actually, I reordered the patches and so the review comment is off.
The nr_members change is in the sorting patch 9. I'll fix up the
comment. I did look for other uses and didn't spot any. I also think
we can add some kind of helper. The current evsel__is_leader functions
are weird. When thinking about the helper I couldn't think of a good
name as I want groups greater than size 1. I'll tweak this.

Thanks,
Ian

> > Suggested-by: Namhyung Kim <namhyung@kernel.org>
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/stat-shadow.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/util/stat-shadow.c b/tools/perf/util/stat-shadow.c
> > index ef85f1ae1ab2..eeccab6751d7 100644
> > --- a/tools/perf/util/stat-shadow.c
> > +++ b/tools/perf/util/stat-shadow.c
> > @@ -163,7 +163,7 @@ static double find_stat(const struct evsel *evsel, int aggr_idx, enum stat_type
> >                       continue;
> >
> >               /* Ignore evsels that are part of different groups. */
> > -             if (evsel->core.leader->nr_members &&
> > +             if (evsel->core.leader->nr_members > 1 &&
> >                   evsel->core.leader != cur->core.leader)
> >                       continue;
> >               /* Ignore evsels with mismatched modifiers. */
> > --
> > 2.39.2.722.g9855ee24e9-goog
> >
>
> --
>
> - Arnaldo

Re: [PATCH v1 07/10] perf evsel: Add function to compute pmu_name

[Ian Rogers]

On Thu, Mar 2, 2023 at 6:39 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>
> Em Wed, Mar 01, 2023 at 08:12:08PM -0800, Ian Rogers escreveu:
> > The computed pmu_name respects software events and aux event groups,
> > such that the pmu_name is changed to be that of the aux event leader
> > or group leader for software events. This is done as a later change
> > will split events that are in different PMUs into different groups.
>
> Adrian, can you please take a look and provide an Ack or Reviewed-by?
>
> - Arnaldo

Adrian's input would be greatly appreciated! The handling of aux
events/grouping was done to make sure the "Miscellaneous Intel PT
testing" was passing, so there is some confidence it is correct :-)

Thanks,
Ian

> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/evsel.c | 24 ++++++++++++++++++++++++
> >  tools/perf/util/evsel.h |  1 +
> >  2 files changed, 25 insertions(+)
> >
> > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > index 2dc2c24252bb..9c6b486f8bd4 100644
> > --- a/tools/perf/util/evsel.c
> > +++ b/tools/perf/util/evsel.c
> > @@ -821,6 +821,30 @@ const char *evsel__name(struct evsel *evsel)
> >       return "unknown";
> >  }
> >
> > +const char *evsel__pmu_name(const struct evsel *evsel)
> > +{
> > +     const struct evsel *leader;
> > +
> > +     /* If the pmu_name is set use it. pmu_name isn't set for CPU and software events. */
> > +     if (evsel->pmu_name)
> > +             return evsel->pmu_name;
> > +     /*
> > +      * Software events may be in a group with other uncore PMU events. Use
> > +      * the pmu_name of the group leader to avoid breaking the software event
> > +      * out of the group.
> > +      *
> > +      * Aux event leaders, like intel_pt, expect a group with events from
> > +      * other PMUs, so substitute the AUX event's PMU in this case.
> > +      */
> > +     leader  = evsel__leader(evsel);
> > +     if ((evsel->core.attr.type == PERF_TYPE_SOFTWARE || evsel__is_aux_event(leader)) &&
> > +         leader->pmu_name) {
> > +             return leader->pmu_name;
> > +     }
> > +
> > +     return "cpu";
> > +}
> > +
> >  const char *evsel__metric_id(const struct evsel *evsel)
> >  {
> >       if (evsel->metric_id)
> > diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> > index 676c499323e9..72121194d3b1 100644
> > --- a/tools/perf/util/evsel.h
> > +++ b/tools/perf/util/evsel.h
> > @@ -280,6 +280,7 @@ int arch_evsel__hw_name(struct evsel *evsel, char *bf, size_t size);
> >
> >  int __evsel__hw_cache_type_op_res_name(u8 type, u8 op, u8 result, char *bf, size_t size);
> >  const char *evsel__name(struct evsel *evsel);
> > +const char *evsel__pmu_name(const struct evsel *evsel);
> >  const char *evsel__metric_id(const struct evsel *evsel);
> >
> >  static inline bool evsel__is_tool(const struct evsel *evsel)
> > --
> > 2.39.2.722.g9855ee24e9-goog
> >
>
> --
>
> - Arnaldo

Re: [PATCH v1 09/10] perf parse-events: Sort and group parsed events

[Ian Rogers]

On Thu, Mar 2, 2023 at 6:51 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>
> Em Wed, Mar 01, 2023 at 08:12:10PM -0800, Ian Rogers escreveu:
> > This change is intended to be a no-op for most current cases, the
> > default sort order is the order the events were parsed. Where it
> > varies is in how groups are handled. Previously an uncore and core
> > event that are grouped would most often cause the group to be removed:
>
> And shouldn't that continue to be the case? I.e. the user asks for
> events to be in a group, but that can't be performed, so the tool fails
> and states that those events can't be in a group, looks sensible, no?

I agree with you in principle but it is tricky. Consider:
$ perf stat -e '{imc_free_running/data_read/,imc_free_running/data_write/}'
-a sleep 1

On my Tigerlake laptop there is no imc_free_running PMU, but there are
uncore_imc_free_running_0 and uncore_imc_free_running_1. So when you
parse the expression above we generate a list of the following events:

uncore_imc_free_running_0/data_read/
uncore_imc_free_running_1/data_read/
uncore_imc_free_running_0/data_write/
uncore_imc_free_running_1/data_write/

which wouldn't work as a grouping as there are two PMUs. The groupings
we want are:
{uncore_imc_free_running_0/data_read/,uncore_imc_free_running_0/data_write/}
{uncore_imc_free_running_1/data_read/,uncore_imc_free_running_1/data_write/}

Similarly for metrics we place all events in a weak group, attempt the
perf_event_open and break the group if that fails. We are running into
issues with this:
1) topdown events must always be in groups
2) the fake PMU in the kernel, used to see whether the scheduling for
the weak group can work, fails to consider errata like two counters
being used on Sandybridge if SMT is enabled due to "leaky" counters

So the vendor updates in:
https://lore.kernel.org/lkml/20230219092848.639226-1-irogers@google.com/
brought in additional flags, but now those flags break things like
topdown events being in a group. For example, the Tigerlake
tma_dram_bound metric doesn't group events, there are more events than
there are counters, but has topdown events which must be in a group.
If that metric leans on the weak group to fail then that's prone to
issues like we see with sandybridge where the group opening succeeds
but you get no counts. If we don't group the events we at least need
to group the topdown events, which is what is fixed here.

Metrics may also do things like counts of X per kilo-instruction. If
we imagine there is an uncore PMU with data_read and data_write, we
could have a metric like:
(uncore_imc@data_read@ + uncore_imc@data_write@) / instructions
It likely makes sense to group the uncore events but we can't group
instructions due to the different PMU. The current behavior is to just
remove all groups.

> Perhaps the tool should continue to fail but provide an alternative
> command line where multiple groups are specified, the user gets educated
> and uses that instead? Probably the logic would be similar to what you
> seem to be doing here?

I think we can pass a flag to say that the event parsing is parsing a
command line. I think we can warn when the groupings change, but as
can be seen in the wildcard uncore case, we expect the groupings to
change. This change will force topdown events into a group, we could
warn for this but the old behavior was just an error and it seems more
friendly just to correct it. In the case of software and aux events,
we expect the groups to be mixed. This means detecting the exact
conditions to warn about and what the corrected user command line
should be are tricky.

The approach taken here is to generalize what we do for uncore
wildcard matching and swap an array sort for a list sort. It seems
likely that this problem will get worse in the future as the layers of
interconnect produce more cases where you want to wildcard some but
not all PMUs. Listing out all PMUs and events is cumbersome, so
wildcards and regular expressions seem useful.

I think when we have examples of useful warnings we should add those
into the code, but we need to take care we don't flag wildcard and
metric groups as being erroneous. With the change as is, we correct
groups but still warn for things like mismatched CPU masks on events.

Thanks,
Ian

> - Arnaldo
>
> >
> > ```
> > $ perf stat -e '{instructions,uncore_imc_free_running_0/data_total/}' -a sleep 1
> > WARNING: grouped events cpus do not match, disabling group:
> >   anon group { instructions, uncore_imc_free_running_0/data_total/ }
> > ...
> > ```
> >
> > However, when wildcards are used the events should be re-sorted and
> > re-grouped in parse_events__set_leader, but this currently fails for
> > simple examples:
> >
> > ```
> > $ perf stat -e '{uncore_imc_free_running/data_read/,uncore_imc_free_running/data_write/}' -a sleep 1
>
> Next time can you state what is the system where this can be tested?
> Saves a bit of time looking for where to test this.
>
> >  Performance counter stats for 'system wide':
> >
> >      <not counted> MiB  uncore_imc_free_running/data_read/
> >      <not counted> MiB  uncore_imc_free_running/data_write/
> >
> >        1.000996992 seconds time elapsed
> > ```
> >
> > A futher failure mode, fixed in this patch, is to force topdown events
>
> further
>
> > into a group.
> >
> > This change moves sorting the evsels in the evlist after parsing. It
> > requires parsing to set up groups. First the evsels are sorted
> > respecting the existing groupings and parse order, but also reordering
> > to ensure evsels of the same PMU and group appear together. So that
> > software and aux events respect groups, their pmu_name is taken from
> > the group leader. The sorting is done with list_sort removing a memory
> > allocation.
> >
> > After sorting a pass is done to correct the group leaders and for
> > topdown events ensuring they have a group leader.
> >
> > This fixes the problems seen before:
> >
> > ```
> > $ perf stat -e '{uncore_imc_free_running/data_read/,uncore_imc_free_running/data_write/}' -a sleep 1
> >
> >  Performance counter stats for 'system wide':
> >
> >             727.42 MiB  uncore_imc_free_running/data_read/
> >              81.84 MiB  uncore_imc_free_running/data_write/
> >
> >        1.000948615 seconds time elapsed
> > ```
> >
> > As well as making groups not fail for cases like:
> >
> > ```
> > $ perf stat -e '{imc_free_running_0/data_total/,imc_free_running_1/data_total/}' -a sleep 1
> >
> >  Performance counter stats for 'system wide':
> >
> >             256.47 MiB  imc_free_running_0/data_total/
> >             256.48 MiB  imc_free_running_1/data_total/
> >
> >        1.001165442 seconds time elapsed
> > ```
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/arch/x86/util/evlist.c |  39 ++---
> >  tools/perf/util/evlist.h          |   2 +-
> >  tools/perf/util/parse-events.c    | 240 +++++++++++++++---------------
> >  tools/perf/util/parse-events.h    |   3 +-
> >  tools/perf/util/parse-events.y    |   4 +-
> >  5 files changed, 136 insertions(+), 152 deletions(-)
> >
> > diff --git a/tools/perf/arch/x86/util/evlist.c b/tools/perf/arch/x86/util/evlist.c
> > index 8a7ae4162563..d4193479a364 100644
> > --- a/tools/perf/arch/x86/util/evlist.c
> > +++ b/tools/perf/arch/x86/util/evlist.c
> > @@ -65,29 +65,22 @@ int arch_evlist__add_default_attrs(struct evlist *evlist,
> >       return ___evlist__add_default_attrs(evlist, attrs, nr_attrs);
> >  }
> >
> > -struct evsel *arch_evlist__leader(struct list_head *list)
> > +int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
> >  {
> > -     struct evsel *evsel, *first, *slots = NULL;
> > -     bool has_topdown = false;
> > -
> > -     first = list_first_entry(list, struct evsel, core.node);
> > -
> > -     if (!topdown_sys_has_perf_metrics())
> > -             return first;
> > -
> > -     /* If there is a slots event and a topdown event then the slots event comes first. */
> > -     __evlist__for_each_entry(list, evsel) {
> > -             if (evsel->pmu_name && !strncmp(evsel->pmu_name, "cpu", 3) && evsel->name) {
> > -                     if (strcasestr(evsel->name, "slots")) {
> > -                             slots = evsel;
> > -                             if (slots == first)
> > -                                     return first;
> > -                     }
> > -                     if (strcasestr(evsel->name, "topdown"))
> > -                             has_topdown = true;
> > -                     if (slots && has_topdown)
> > -                             return slots;
> > -             }
> > +     if (topdown_sys_has_perf_metrics() &&
> > +         (!lhs->pmu_name || !strncmp(lhs->pmu_name, "cpu", 3))) {
> > +             /* Ensure the topdown slots comes first. */
> > +             if (strcasestr(lhs->name, "slots"))
> > +                     return -1;
> > +             if (strcasestr(rhs->name, "slots"))
> > +                     return 1;
> > +             /* Followed by topdown events. */
> > +             if (strcasestr(lhs->name, "topdown") && !strcasestr(rhs->name, "topdown"))
> > +                     return -1;
> > +             if (!strcasestr(lhs->name, "topdown") && strcasestr(rhs->name, "topdown"))
> > +                     return 1;
> >       }
> > -     return first;
> > +
> > +     /* Default ordering by insertion index. */
> > +     return lhs->core.idx - rhs->core.idx;
> >  }
> > diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
> > index 01fa9d592c5a..d89d8f92802b 100644
> > --- a/tools/perf/util/evlist.h
> > +++ b/tools/perf/util/evlist.h
> > @@ -119,7 +119,7 @@ int arch_evlist__add_default_attrs(struct evlist *evlist,
> >  #define evlist__add_default_attrs(evlist, array) \
> >       arch_evlist__add_default_attrs(evlist, array, ARRAY_SIZE(array))
> >
> > -struct evsel *arch_evlist__leader(struct list_head *list);
> > +int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs);
> >
> >  int evlist__add_dummy(struct evlist *evlist);
> >  struct evsel *evlist__add_aux_dummy(struct evlist *evlist, bool system_wide);
> > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > index 1be454697d57..d6eb0a54dd2d 100644
> > --- a/tools/perf/util/parse-events.c
> > +++ b/tools/perf/util/parse-events.c
> > @@ -1,6 +1,7 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >  #include <linux/hw_breakpoint.h>
> >  #include <linux/err.h>
> > +#include <linux/list_sort.h>
> >  #include <linux/zalloc.h>
> >  #include <dirent.h>
> >  #include <errno.h>
> > @@ -1655,125 +1656,7 @@ int parse_events__modifier_group(struct list_head *list,
> >       return parse_events__modifier_event(list, event_mod, true);
> >  }
> >
> > -/*
> > - * 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 (strncmp(pmu_name_a, pmu_name_b, end_a - pmu_name_a) == 0);
> > -}
> > -
> > -static int
> > -parse_events__set_leader_for_uncore_aliase(char *name, struct list_head *list,
> > -                                        struct parse_events_state *parse_state)
> > -{
> > -     struct evsel *evsel, *leader;
> > -     uintptr_t *leaders;
> > -     bool is_leader = true;
> > -     int i, nr_pmu = 0, total_members, ret = 0;
> > -
> > -     leader = list_first_entry(list, struct evsel, core.node);
> > -     evsel = list_last_entry(list, struct evsel, core.node);
> > -     total_members = evsel->core.idx - leader->core.idx + 1;
> > -
> > -     leaders = calloc(total_members, sizeof(uintptr_t));
> > -     if (WARN_ON(!leaders))
> > -             return 0;
> > -
> > -     /*
> > -      * Going through the whole group and doing sanity check.
> > -      * All members must use alias, and be from the same uncore block.
> > -      * Also, storing the leader events in an array.
> > -      */
> > -     __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 (!is_leader)
> > -                     continue;
> > -             /*
> > -              * If the event's PMU name starts to repeat, it must be a new
> > -              * event. That can be used to distinguish the leader from
> > -              * other members, even they have the same event name.
> > -              */
> > -             if ((leader != evsel) &&
> > -                 !strcmp(leader->pmu_name, evsel->pmu_name)) {
> > -                     is_leader = false;
> > -                     continue;
> > -             }
> > -
> > -             /* Store the leader event for each PMU */
> > -             leaders[nr_pmu++] = (uintptr_t) evsel;
> > -     }
> > -
> > -     /* only one event alias */
> > -     if (nr_pmu == total_members) {
> > -             parse_state->nr_groups--;
> > -             goto handled;
> > -     }
> > -
> > -     /*
> > -      * 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. The big group has to be split into multiple small groups
> > -      * which only include the events from the same PMU.
> > -      *
> > -      * Here the uncore event aliases must be from the same uncore block.
> > -      * The number of PMUs must be same for each alias. The number of new
> > -      * small groups equals to the number of PMUs.
> > -      * Setting the leader event for corresponding members in each group.
> > -      */
> > -     i = 0;
> > -     __evlist__for_each_entry(list, evsel) {
> > -             if (i >= nr_pmu)
> > -                     i = 0;
> > -             evsel__set_leader(evsel, (struct evsel *) leaders[i++]);
> > -     }
> > -
> > -     /* The number of members and group name are same for each group */
> > -     for (i = 0; i < nr_pmu; i++) {
> > -             evsel = (struct evsel *) leaders[i];
> > -             evsel->core.nr_members = total_members / nr_pmu;
> > -             evsel->group_name = name ? strdup(name) : NULL;
> > -     }
> > -
> > -     /* Take the new small groups into account */
> > -     parse_state->nr_groups += nr_pmu - 1;
> > -
> > -handled:
> > -     ret = 1;
> > -     free(name);
> > -out:
> > -     free(leaders);
> > -     return ret;
> > -}
> > -
> > -__weak struct evsel *arch_evlist__leader(struct list_head *list)
> > -{
> > -     return list_first_entry(list, struct evsel, core.node);
> > -}
> > -
> > -void parse_events__set_leader(char *name, struct list_head *list,
> > -                           struct parse_events_state *parse_state)
> > +void parse_events__set_leader(char *name, struct list_head *list)
> >  {
> >       struct evsel *leader;
> >
> > @@ -1782,13 +1665,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;
> > -
> > -     leader = arch_evlist__leader(list);
> > +     leader = list_first_entry(list, struct evsel, core.node);
> >       __perf_evlist__set_leader(list, &leader->core);
> >       leader->group_name = name;
> > -     list_move(&leader->core.node, list);
> >  }
> >
> >  /* list_event is assumed to point to malloc'ed memory */
> > @@ -2245,6 +2124,117 @@ static int parse_events__with_hybrid_pmu(struct parse_events_state *parse_state,
> >       return ret;
> >  }
> >
> > +__weak int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
> > +{
> > +     /* Order by insertion index. */
> > +     return lhs->core.idx - rhs->core.idx;
> > +}
> > +
> > +static int evlist__cmp(void *state, const struct list_head *l, const struct list_head *r)
> > +{
> > +     const struct perf_evsel *lhs_core = container_of(l, struct perf_evsel, node);
> > +     const struct evsel *lhs = container_of(lhs_core, struct evsel, core);
> > +     const struct perf_evsel *rhs_core = container_of(r, struct perf_evsel, node);
> > +     const struct evsel *rhs = container_of(rhs_core, struct evsel, core);
> > +     int *leader_idx = state;
> > +     int lhs_leader_idx = *leader_idx, rhs_leader_idx = *leader_idx, ret;
> > +     const char *lhs_pmu_name, *rhs_pmu_name;
> > +
> > +     /*
> > +      * First sort by grouping/leader. Read the leader idx only if the evsel
> > +      * is part of a group, as -1 indicates no group.
> > +      */
> > +     if (lhs_core->leader != lhs_core || lhs_core->nr_members > 1)
> > +             lhs_leader_idx = lhs_core->leader->idx;
> > +     if (rhs_core->leader != rhs_core || rhs_core->nr_members > 1)
> > +             rhs_leader_idx = rhs_core->leader->idx;
> > +
> > +     if (lhs_leader_idx != rhs_leader_idx)
> > +             return lhs_leader_idx - rhs_leader_idx;
> > +
> > +     /* Group by PMU. Groups can't span PMUs. */
> > +     lhs_pmu_name = evsel__pmu_name(lhs);
> > +     rhs_pmu_name = evsel__pmu_name(rhs);
> > +     ret = strcmp(lhs_pmu_name, rhs_pmu_name);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Architecture specific sorting. */
> > +     return arch_evlist__cmp(lhs, rhs);
> > +}
> > +
> > +static void parse_events__sort_events_and_fix_groups(struct list_head *list)
> > +{
> > +     int idx = -1;
> > +     struct evsel *pos, *cur_leader = NULL;
> > +     struct perf_evsel *cur_leaders_grp = NULL;
> > +
> > +     /*
> > +      * Compute index to insert ungrouped events at. Place them where the
> > +      * first ungrouped event appears.
> > +      */
> > +     list_for_each_entry(pos, list, core.node) {
> > +             const struct evsel *pos_leader = evsel__leader(pos);
> > +
> > +             if (pos != pos_leader || pos->core.nr_members > 1)
> > +                     continue;
> > +
> > +             idx = pos->core.idx;
> > +             break;
> > +     }
> > +
> > +     /* Sort events. */
> > +     list_sort(&idx, list, evlist__cmp);
> > +
> > +     /*
> > +      * Recompute groups, splitting for PMUs and adding groups for events
> > +      * that require them.
> > +      */
> > +     idx = 0;
> > +     list_for_each_entry(pos, list, core.node) {
> > +             const struct evsel *pos_leader = evsel__leader(pos);
> > +             const char *pos_pmu_name = evsel__pmu_name(pos);
> > +             const char *cur_leader_pmu_name, *pos_leader_pmu_name;
> > +             bool force_grouped = arch_evsel__must_be_in_group(pos);
> > +
> > +             /* Reset index and nr_members. */
> > +             pos->core.idx = idx++;
> > +             pos->core.nr_members = 0;
> > +
> > +             /*
> > +              * Set the group leader respecting the given groupings and that
> > +              * groups can't span PMUs.
> > +              */
> > +             if (!cur_leader)
> > +                     cur_leader = pos;
> > +
> > +             cur_leader_pmu_name = evsel__pmu_name(cur_leader);
> > +             if ((cur_leaders_grp != pos->core.leader && !force_grouped) ||
> > +                 strcmp(cur_leader_pmu_name, pos_pmu_name)) {
> > +                     /* Event is for a different group/PMU than last. */
> > +                     cur_leader = pos;
> > +                     /*
> > +                      * Remember the leader's group before it is overwritten,
> > +                      * so that later events match as being in the same
> > +                      * group.
> > +                      */
> > +                     cur_leaders_grp = pos->core.leader;
> > +             }
> > +             pos_leader_pmu_name = evsel__pmu_name(pos_leader);
> > +             if (strcmp(pos_leader_pmu_name, pos_pmu_name) || force_grouped) {
> > +                     /*
> > +                      * Event's PMU differs from its leader's. Groups can't
> > +                      * span PMUs, so update leader from the group/PMU
> > +                      * tracker.
> > +                      */
> > +                     evsel__set_leader(pos, cur_leader);
> > +             }
> > +     }
> > +     list_for_each_entry(pos, list, core.node) {
> > +             pos->core.leader->nr_members++;
> > +     }
> > +}
> > +
> >  int __parse_events(struct evlist *evlist, const char *str,
> >                  struct parse_events_error *err, struct perf_pmu *fake_pmu)
> >  {
> > @@ -2266,6 +2256,8 @@ int __parse_events(struct evlist *evlist, const char *str,
> >               return -1;
> >       }
> >
> > +     parse_events__sort_events_and_fix_groups(&parse_state.list);
> > +
> >       /*
> >        * Add list to the evlist even with errors to allow callers to clean up.
> >        */
> > diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> > index 428e72eaafcc..22fc11b0bd59 100644
> > --- a/tools/perf/util/parse-events.h
> > +++ b/tools/perf/util/parse-events.h
> > @@ -200,8 +200,7 @@ 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,
> > -                           struct parse_events_state *parse_state);
> > +void parse_events__set_leader(char *name, struct list_head *list);
> >  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 541b8dde2063..90d12f2bc8be 100644
> > --- a/tools/perf/util/parse-events.y
> > +++ b/tools/perf/util/parse-events.y
> > @@ -203,7 +203,7 @@ PE_NAME '{' events '}'
> >
> >       inc_group_count(list, _parse_state);
> >       /* Takes ownership of $1. */
> > -     parse_events__set_leader($1, list, _parse_state);
> > +     parse_events__set_leader($1, list);
> >       $$ = list;
> >  }
> >  |
> > @@ -212,7 +212,7 @@ PE_NAME '{' events '}'
> >       struct list_head *list = $2;
> >
> >       inc_group_count(list, _parse_state);
> > -     parse_events__set_leader(NULL, list, _parse_state);
> > +     parse_events__set_leader(NULL, list);
> >       $$ = list;
> >  }
> >
> > --
> > 2.39.2.722.g9855ee24e9-goog
> >
>
> --
>
> - Arnaldo

Re: [PATCH v1 05/10] perf evsel: Limit in group test to CPUs

[Ian Rogers]

On Thu, Mar 2, 2023 at 7:24 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
>
>
> On 2023-03-01 11:12 p.m., Ian Rogers wrote:
> > Don't just match on the event name, restict based on the PMU too.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/arch/x86/util/evsel.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/tools/perf/arch/x86/util/evsel.c b/tools/perf/arch/x86/util/evsel.c
> > index ea3972d785d1..580b0a172136 100644
> > --- a/tools/perf/arch/x86/util/evsel.c
> > +++ b/tools/perf/arch/x86/util/evsel.c
> > @@ -61,6 +61,9 @@ bool arch_evsel__must_be_in_group(const struct evsel *evsel)
> >       if (!evsel__sys_has_perf_metrics(evsel))
> >               return false;
> >
> > +     if (evsel->pmu_name && strncmp(evsel->pmu_name, "cpu", 3))
> > +             return false;
>
> I'm not sure why we want to check the pmu name. It seems better to move
> it into evsel__sys_has_perf_metrics(), since perf_metrics is a core PMU
> only feature.
>
> I think the strncmp(evsel->pmu_name, "cpu", 3) is to check whether it is
> a core PMU. It is also used in other places. I think it's better to
> factor out it, e.g., arch_evsel__is_core_pmu(). It will deliver a clear
> message of what we are doing here.
>
> Thanks,
> Kan

I missed the behavior of evsel__sys_has_perf_metrics and think we can
just drop this change. We should probably rename
evsel__sys_has_perf_metrics perhaps something like
arch_evsel__pmu_has_topdown_events.

Thanks,
Ian

> > +
> >       return evsel->name &&
> >               (strcasestr(evsel->name, "slots") ||
> >                strcasestr(evsel->name, "topdown"));

Re: [PATCH v1 05/10] perf evsel: Limit in group test to CPUs

[Liang, Kan]

On 2023-03-02 2:38 p.m., Ian Rogers wrote:
> On Thu, Mar 2, 2023 at 7:24 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>>
>>
>>
>> On 2023-03-01 11:12 p.m., Ian Rogers wrote:
>>> Don't just match on the event name, restict based on the PMU too.
>>>
>>> Signed-off-by: Ian Rogers <irogers@google.com>
>>> ---
>>>  tools/perf/arch/x86/util/evsel.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/tools/perf/arch/x86/util/evsel.c b/tools/perf/arch/x86/util/evsel.c
>>> index ea3972d785d1..580b0a172136 100644
>>> --- a/tools/perf/arch/x86/util/evsel.c
>>> +++ b/tools/perf/arch/x86/util/evsel.c
>>> @@ -61,6 +61,9 @@ bool arch_evsel__must_be_in_group(const struct evsel *evsel)
>>>       if (!evsel__sys_has_perf_metrics(evsel))
>>>               return false;
>>>
>>> +     if (evsel->pmu_name && strncmp(evsel->pmu_name, "cpu", 3))
>>> +             return false;
>>
>> I'm not sure why we want to check the pmu name. It seems better to move
>> it into evsel__sys_has_perf_metrics(), since perf_metrics is a core PMU
>> only feature.
>>
>> I think the strncmp(evsel->pmu_name, "cpu", 3) is to check whether it is
>> a core PMU. It is also used in other places. I think it's better to
>> factor out it, e.g., arch_evsel__is_core_pmu(). It will deliver a clear
>> message of what we are doing here.
>>
>> Thanks,
>> Kan
> 
> I missed the behavior of evsel__sys_has_perf_metrics and think we can
> just drop this change.

Yes, dropping the change is also OK for me.

> We should probably rename
> evsel__sys_has_perf_metrics perhaps something like
> arch_evsel__pmu_has_topdown_events.

The topdown is a tricky feature. For the big core, to support the
topdown events, we have a dedicated perf metrics register, which has to
be grouped with the fixed counter 3. That brings all of these troubles.
Sorry for that.
For the atom, the topdown events are still supported, but with GP
counters. There is no perf metrics register at all.

The evsel__sys_has_perf_metrics() is used to check whether the perf
metrics register is supported. If so, we have to specially handle it.
It's not to check whether the topdown events are supported. So I think
it's better to keep the perf_metrics name, rather than topdown_events.

Thanks,
Kan
> 
> Thanks,
> Ian
> 
>>> +
>>>       return evsel->name &&
>>>               (strcasestr(evsel->name, "slots") ||
>>>                strcasestr(evsel->name, "topdown"));