[PATCH v1 0/3] Parse event sort/regroup fixes

[PATCH v1 0/3] Parse event sort/regroup fixes

[Ian Rogers]

Patch 1, fix:
perf stat -e cycles,slots,topdown-fe-bound
so that cycles isn't made a group leader (failure caused by PMUs
matching). Previously this event list would fail so not necessarily a
regression over previous perf release.

Patch 2, when regrouping events the leader needs to be updated due to
sorting. This fix causes larger event groups that then regress at
least the tigerlake metric test as the kernel PMU driver fails to
break the weak groups. This is a fix for a bug but the consequence of
fixing the bug is to make a passing test fail due to the kernel PMU
driver.

Patch 3, don't alter the list position of events without a group if
they don't need forcing into a group. Analysis showed this was the
cause of the issue reported by Andi Kleen:
https://lore.kernel.org/linux-perf-users/ZLBgbHkbrfGygM%2Fu@tassilo/

Due to the test regression in patch 2, follow up patches may be
necessary for Icelake+ Intel vendor metrics to add METRIC_NO_GROUP to
avoid the kernel PMU driver issue.

Ian Rogers (3):
  perf parse-events: Extra care around force grouped events
  perf parse-events: When fixing group leaders always set the leader
  perf parse-events: Only move force grouped evsels when sorting

 tools/perf/util/parse-events.c | 58 +++++++++++++++++++++-------------
 1 file changed, 36 insertions(+), 22 deletions(-)

-- 
2.41.0.487.g6d72f3e995-goog

[PATCH v1 1/3] perf parse-events: Extra care around force grouped events

[Ian Rogers]

Perf metric (topdown) events on Intel Icelake+ machines require a
group, however, they may be next to events that don't require a group.
Consider:
cycles,slots,topdown-fe-bound

The cycles event needn't be grouped but slots and topdown-fe-bound
need grouping. Prior to this change, as slots and topdown-fe-bound
need a group forcing and all events share the same PMU, slots and
topdown-fe-bound would be forced into a group with cycles. This is a
bug on two fronts, cycles wasn't supposed to be grouped and cycles
can't be a group leader with a perf metric event.

This change adds recognition that cycles isn't force grouped and so it
shouldn't be force grouped with slots and topdown-fe-bound.

Fixes: a90cc5a9eeab ("perf evsel: Don't let evsel__group_pmu_name() traverse unsorted group")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/parse-events.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 5dcfbf316bf6..f10760ac1781 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -2141,7 +2141,7 @@ static int parse_events__sort_events_and_fix_groups(struct list_head *list)
 	int idx = 0, unsorted_idx = -1;
 	struct evsel *pos, *cur_leader = NULL;
 	struct perf_evsel *cur_leaders_grp = NULL;
-	bool idx_changed = false;
+	bool idx_changed = false, cur_leader_force_grouped = false;
 	int orig_num_leaders = 0, num_leaders = 0;
 	int ret;
 
@@ -2182,7 +2182,7 @@ static int parse_events__sort_events_and_fix_groups(struct list_head *list)
 		const struct evsel *pos_leader = evsel__leader(pos);
 		const char *pos_pmu_name = pos->group_pmu_name;
 		const char *cur_leader_pmu_name, *pos_leader_pmu_name;
-		bool force_grouped = arch_evsel__must_be_in_group(pos);
+		bool pos_force_grouped = arch_evsel__must_be_in_group(pos);
 
 		/* Reset index and nr_members. */
 		if (pos->core.idx != idx)
@@ -2198,7 +2198,8 @@ static int parse_events__sort_events_and_fix_groups(struct list_head *list)
 			cur_leader = pos;
 
 		cur_leader_pmu_name = cur_leader->group_pmu_name;
-		if ((cur_leaders_grp != pos->core.leader && !force_grouped) ||
+		if ((cur_leaders_grp != pos->core.leader &&
+		     (!pos_force_grouped || !cur_leader_force_grouped)) ||
 		    strcmp(cur_leader_pmu_name, pos_pmu_name)) {
 			/* Event is for a different group/PMU than last. */
 			cur_leader = pos;
@@ -2208,9 +2209,14 @@ static int parse_events__sort_events_and_fix_groups(struct list_head *list)
 			 * group.
 			 */
 			cur_leaders_grp = pos->core.leader;
+			/*
+			 * Avoid forcing events into groups with events that
+			 * don't need to be in the group.
+			 */
+			cur_leader_force_grouped = pos_force_grouped;
 		}
 		pos_leader_pmu_name = pos_leader->group_pmu_name;
-		if (strcmp(pos_leader_pmu_name, pos_pmu_name) || force_grouped) {
+		if (strcmp(pos_leader_pmu_name, pos_pmu_name) || pos_force_grouped) {
 			/*
 			 * Event's PMU differs from its leader's. Groups can't
 			 * span PMUs, so update leader from the group/PMU
-- 
2.41.0.487.g6d72f3e995-goog

[PATCH v1 2/3] perf parse-events: When fixing group leaders always set the leader

[Ian Rogers]

The evsel grouping fix iterates over evsels tracking the leader group
and the current position's group, updating the current position's
leader if an evsel is being forced into a group or groups
changed. However, groups changing isn't a sufficient condition as
sorting may have reordered events and the leader may no longer come
first. For this reason update all leaders whenever they disagree.

This change breaks certain Icelake+ metrics due to bugs in the
kernel. For example, tma_l3_bound with threshold enabled tries to
program the events:

{topdown-retiring,slots,CYCLE_ACTIVITY.STALLS_L2_MISS,topdown-fe-bound,EXE_ACTIVITY.BOUND_ON_STORES,EXE_ACTIVITY.1_PORTS_UTIL,topdown-be-bound,cpu/INT_MISC.RECOVERY_CYCLES,cmask=1,edge/,CYCLE_ACTIVITY.STALLS_L3_MISS,CPU_CLK_UNHALTED.THREAD,CYCLE_ACTIVITY.STALLS_MEM_ANY,EXE_ACTIVITY.2_PORTS_UTIL,CYCLE_ACTIVITY.STALLS_TOTAL,topdown-bad-spec}:W

fixing the perf metric event order gives:

{slots,topdown-retiring,topdown-fe-bound,topdown-be-bound,topdown-bad-spec,CYCLE_ACTIVITY.STALLS_L2_MISS,EXE_ACTIVITY.BOUND_ON_STORES,EXE_ACTIVITY.1_PORTS_UTIL,cpu/INT_MISC.RECOVERY_CYCLES,cmask=1,edge/,CYCLE_ACTIVITY.STALLS_L3_MISS,CPU_CLK_UNHALTED.THREAD,CYCLE_ACTIVITY.STALLS_MEM_ANY,EXE_ACTIVITY.2_PORTS_UTIL,CYCLE_ACTIVITY.STALLS_TOTAL}:W

Both of these return "<not counted>" for all events, whilst they work
with the group removed respecting that the perf metric events must
still be grouped. A vendor events update will need to add
METRIC_NO_GROUP to these metrics to workaround the kernel PMU driver
issue.

Fixes: a90cc5a9eeab ("perf evsel: Don't let evsel__group_pmu_name() traverse unsorted group")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/parse-events.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index f10760ac1781..4a36ce60c7dd 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -2181,7 +2181,7 @@ static int parse_events__sort_events_and_fix_groups(struct list_head *list)
 	list_for_each_entry(pos, list, core.node) {
 		const struct evsel *pos_leader = evsel__leader(pos);
 		const char *pos_pmu_name = pos->group_pmu_name;
-		const char *cur_leader_pmu_name, *pos_leader_pmu_name;
+		const char *cur_leader_pmu_name;
 		bool pos_force_grouped = arch_evsel__must_be_in_group(pos);
 
 		/* Reset index and nr_members. */
@@ -2215,13 +2215,8 @@ static int parse_events__sort_events_and_fix_groups(struct list_head *list)
 			 */
 			cur_leader_force_grouped = pos_force_grouped;
 		}
-		pos_leader_pmu_name = pos_leader->group_pmu_name;
-		if (strcmp(pos_leader_pmu_name, pos_pmu_name) || pos_force_grouped) {
-			/*
-			 * Event's PMU differs from its leader's. Groups can't
-			 * span PMUs, so update leader from the group/PMU
-			 * tracker.
-			 */
+		if (pos_leader != cur_leader) {
+			/* The leader changed so update it. */
 			evsel__set_leader(pos, cur_leader);
 		}
 	}
-- 
2.41.0.487.g6d72f3e995-goog

[PATCH v1 3/3] perf parse-events: Only move force grouped evsels when sorting

[Ian Rogers]

Prior to this change, events without a group would be sorted as if
they were from the location of the first event without a group. For
example instructions and cycles are without a group:

instructions,{imc_free_running/data_read/,imc_free_running/data_write/},cycles

parse events would create an eventual evlist like:

instructions,cycles,{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/}

This is done so that perf metric events, that must always be in a
group, will be adjacent and so can be forced into a group.

This change modifies the sorting so that only force grouped events,
like perf metrics, are sorted and all other events keep their position
with respect to groups in the evlist. The location of the force
grouped event is chosen to match the first force grouped event.

For architectures without force grouped events, ie anything not Intel
Icelake or newer, this should mean sorting and fixing doesn't modify
the event positions except when fixing the grouping for PMUs of things
like uncore events.

Reported-by: Andi Kleen <ak@linux.intel.com>
Fixes: 347c2f0a0988 ("perf parse-events: Sort and group parsed events")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/parse-events.c | 39 ++++++++++++++++++++++------------
 1 file changed, 26 insertions(+), 13 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 4a36ce60c7dd..e63fc40efea5 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -2092,16 +2092,16 @@ __weak int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
 	return lhs->core.idx - rhs->core.idx;
 }
 
-static int evlist__cmp(void *state, const struct list_head *l, const struct list_head *r)
+static int evlist__cmp(void *_fg_idx, 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;
+	int *force_grouped_idx = _fg_idx;
+	int lhs_sort_idx, rhs_sort_idx, ret;
 	const char *lhs_pmu_name, *rhs_pmu_name;
-	bool lhs_has_group = false, rhs_has_group = false;
+	bool lhs_has_group, rhs_has_group;
 
 	/*
 	 * First sort by grouping/leader. Read the leader idx only if the evsel
@@ -2113,15 +2113,25 @@ static int evlist__cmp(void *state, const struct list_head *l, const struct list
 	 */
 	if (lhs_core->leader != lhs_core || lhs_core->nr_members > 1) {
 		lhs_has_group = true;
-		lhs_leader_idx = lhs_core->leader->idx;
+		lhs_sort_idx = lhs_core->leader->idx;
+	} else {
+		lhs_has_group = false;
+		lhs_sort_idx = *force_grouped_idx != -1 && arch_evsel__must_be_in_group(lhs)
+			? *force_grouped_idx
+			: lhs_core->idx;
 	}
 	if (rhs_core->leader != rhs_core || rhs_core->nr_members > 1) {
 		rhs_has_group = true;
-		rhs_leader_idx = rhs_core->leader->idx;
+		rhs_sort_idx = rhs_core->leader->idx;
+	} else {
+		rhs_has_group = false;
+		rhs_sort_idx = *force_grouped_idx != -1 && arch_evsel__must_be_in_group(rhs)
+			? *force_grouped_idx
+			: rhs_core->idx;
 	}
 
-	if (lhs_leader_idx != rhs_leader_idx)
-		return lhs_leader_idx - rhs_leader_idx;
+	if (lhs_sort_idx != rhs_sort_idx)
+		return lhs_sort_idx - rhs_sort_idx;
 
 	/* Group by PMU if there is a group. Groups can't span PMUs. */
 	if (lhs_has_group && rhs_has_group) {
@@ -2138,7 +2148,7 @@ static int evlist__cmp(void *state, const struct list_head *l, const struct list
 
 static int parse_events__sort_events_and_fix_groups(struct list_head *list)
 {
-	int idx = 0, unsorted_idx = -1;
+	int idx = 0, force_grouped_idx = -1;
 	struct evsel *pos, *cur_leader = NULL;
 	struct perf_evsel *cur_leaders_grp = NULL;
 	bool idx_changed = false, cur_leader_force_grouped = false;
@@ -2166,12 +2176,14 @@ static int parse_events__sort_events_and_fix_groups(struct list_head *list)
 		 */
 		pos->core.idx = idx++;
 
-		if (unsorted_idx == -1 && pos == pos_leader && pos->core.nr_members < 2)
-			unsorted_idx = pos->core.idx;
+		/* Remember an index to sort all forced grouped events together to. */
+		if (force_grouped_idx == -1 && pos == pos_leader && pos->core.nr_members < 2 &&
+		    arch_evsel__must_be_in_group(pos))
+			force_grouped_idx = pos->core.idx;
 	}
 
 	/* Sort events. */
-	list_sort(&unsorted_idx, list, evlist__cmp);
+	list_sort(&force_grouped_idx, list, evlist__cmp);
 
 	/*
 	 * Recompute groups, splitting for PMUs and adding groups for events
@@ -2182,7 +2194,8 @@ static int parse_events__sort_events_and_fix_groups(struct list_head *list)
 		const struct evsel *pos_leader = evsel__leader(pos);
 		const char *pos_pmu_name = pos->group_pmu_name;
 		const char *cur_leader_pmu_name;
-		bool pos_force_grouped = arch_evsel__must_be_in_group(pos);
+		bool pos_force_grouped = force_grouped_idx != -1 &&
+			arch_evsel__must_be_in_group(pos);
 
 		/* Reset index and nr_members. */
 		if (pos->core.idx != idx)
-- 
2.41.0.487.g6d72f3e995-goog

Re: [PATCH v1 0/3] Parse event sort/regroup fixes

[Arnaldo Carvalho de Melo]

Em Tue, Jul 18, 2023 at 05:18:33PM -0700, Ian Rogers escreveu:
> Patch 1, fix:
> perf stat -e cycles,slots,topdown-fe-bound
> so that cycles isn't made a group leader (failure caused by PMUs
> matching). Previously this event list would fail so not necessarily a
> regression over previous perf release.
> 
> Patch 2, when regrouping events the leader needs to be updated due to
> sorting. This fix causes larger event groups that then regress at
> least the tigerlake metric test as the kernel PMU driver fails to
> break the weak groups. This is a fix for a bug but the consequence of
> fixing the bug is to make a passing test fail due to the kernel PMU
> driver.
> 
> Patch 3, don't alter the list position of events without a group if
> they don't need forcing into a group. Analysis showed this was the
> cause of the issue reported by Andi Kleen:
> https://lore.kernel.org/linux-perf-users/ZLBgbHkbrfGygM%2Fu@tassilo/

Andi,

	Can you please check these patches and provide a Tested-by?

Thanks,

- Arnaldo
 
> Due to the test regression in patch 2, follow up patches may be
> necessary for Icelake+ Intel vendor metrics to add METRIC_NO_GROUP to
> avoid the kernel PMU driver issue.
> 
> Ian Rogers (3):
>   perf parse-events: Extra care around force grouped events
>   perf parse-events: When fixing group leaders always set the leader
>   perf parse-events: Only move force grouped evsels when sorting
> 
>  tools/perf/util/parse-events.c | 58 +++++++++++++++++++++-------------
>  1 file changed, 36 insertions(+), 22 deletions(-)
> 
> -- 
> 2.41.0.487.g6d72f3e995-goog

Re: [PATCH v1 0/3] Parse event sort/regroup fixes

[Ian Rogers]

On Wed, Jul 19, 2023 at 7:49 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Tue, Jul 18, 2023 at 05:18:33PM -0700, Ian Rogers escreveu:
> > Patch 1, fix:
> > perf stat -e cycles,slots,topdown-fe-bound
> > so that cycles isn't made a group leader (failure caused by PMUs
> > matching). Previously this event list would fail so not necessarily a
> > regression over previous perf release.
> >
> > Patch 2, when regrouping events the leader needs to be updated due to
> > sorting. This fix causes larger event groups that then regress at
> > least the tigerlake metric test as the kernel PMU driver fails to
> > break the weak groups. This is a fix for a bug but the consequence of
> > fixing the bug is to make a passing test fail due to the kernel PMU
> > driver.
> >
> > Patch 3, don't alter the list position of events without a group if
> > they don't need forcing into a group. Analysis showed this was the
> > cause of the issue reported by Andi Kleen:
> > https://lore.kernel.org/linux-perf-users/ZLBgbHkbrfGygM%2Fu@tassilo/
>
> Andi,
>
>         Can you please check these patches and provide a Tested-by?

I think we should be aiming to get these fixes/changes into Linux 6.5
and it's a shame this didn't happen last week. Feedback appreciated.

Thanks,
Ian

> Thanks,
>
> - Arnaldo
>
> > Due to the test regression in patch 2, follow up patches may be
> > necessary for Icelake+ Intel vendor metrics to add METRIC_NO_GROUP to
> > avoid the kernel PMU driver issue.
> >
> > Ian Rogers (3):
> >   perf parse-events: Extra care around force grouped events
> >   perf parse-events: When fixing group leaders always set the leader
> >   perf parse-events: Only move force grouped evsels when sorting
> >
> >  tools/perf/util/parse-events.c | 58 +++++++++++++++++++++-------------
> >  1 file changed, 36 insertions(+), 22 deletions(-)
> >
> > --
> > 2.41.0.487.g6d72f3e995-goog

Re: [PATCH v1 0/3] Parse event sort/regroup fixes

[Andi Kleen]

> > Andi,
> >
> >         Can you please check these patches and provide a Tested-by?
> 
> I think we should be aiming to get these fixes/changes into Linux 6.5
> and it's a shame this didn't happen last week. Feedback appreciated.

Sorry was on vacation. Will test right now.

-Andi

Re: [PATCH v1 0/3] Parse event sort/regroup fixes

[Andi Kleen]

> > Andi,
> >
> >         Can you please check these patches and provide a Tested-by?
> 
> I think we should be aiming to get these fixes/changes into Linux 6.5
> and it's a shame this didn't happen last week. Feedback appreciated.

Sorry for the delay, I was finally able to test them now and they
resolve my issues. Thanks

Tested-by: Andi Kleen <ak@linux.intel.com>

-Andi

Re: [PATCH v1 0/3] Parse event sort/regroup fixes

[Andi Kleen]

What's the status of this patch kit ? I don't see it in perf-tools-next

It should be applied to any branches that have the original problem.

Thanks,
-Andi

Re: [PATCH v1 0/3] Parse event sort/regroup fixes

[Andi Kleen]

On Wed, Jul 19, 2023 at 11:49:47AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Jul 18, 2023 at 05:18:33PM -0700, Ian Rogers escreveu:
> > Patch 1, fix:
> > perf stat -e cycles,slots,topdown-fe-bound
> > so that cycles isn't made a group leader (failure caused by PMUs
> > matching). Previously this event list would fail so not necessarily a
> > regression over previous perf release.
> > 
> > Patch 2, when regrouping events the leader needs to be updated due to
> > sorting. This fix causes larger event groups that then regress at
> > least the tigerlake metric test as the kernel PMU driver fails to
> > break the weak groups. This is a fix for a bug but the consequence of
> > fixing the bug is to make a passing test fail due to the kernel PMU
> > driver.
> > 
> > Patch 3, don't alter the list position of events without a group if
> > they don't need forcing into a group. Analysis showed this was the
> > cause of the issue reported by Andi Kleen:
> > https://lore.kernel.org/linux-perf-users/ZLBgbHkbrfGygM%2Fu@tassilo/
> 
> Andi,
> 
> 	Can you please check these patches and provide a Tested-by?

Tested-by: Andi Kleen <ak@linux.intel.com>

-Andi