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
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 <[email protected]>
Fixes: 347c2f0a0988 ("perf parse-events: Sort and group parsed events")
Signed-off-by: Ian Rogers <[email protected]>
---
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
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
On Wed, Jul 19, 2023 at 7:49 AM Arnaldo Carvalho de Melo
<[email protected]> 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
> > 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
> > 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 <[email protected]>
-Andi
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 <[email protected]>
-Andi
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