| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <f7e4f7e8-748c-4ec7-9088-0e844392c11a@linux.intel.com>
Date: Thu, 6 Mar 2025 17:17:01 +0800
From: "Mi, Dapeng" <dapeng1.mi@...ux.intel.com>
To: Ian Rogers <irogers@...gle.com>, Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>, Arnaldo Carvalho de Melo <acme@...nel.org>,
Namhyung Kim <namhyung@...nel.org>, Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Jiri Olsa <jolsa@...nel.org>, Adrian Hunter <adrian.hunter@...el.com>,
Kan Liang <kan.liang@...ux.intel.com>, James Clark <james.clark@...aro.org>,
Dominique Martinet <asmadeus@...ewreck.org>, Andi Kleen
<ak@...ux.intel.com>, linux-perf-users@...r.kernel.org,
linux-kernel@...r.kernel.org, Thomas Falcon <thomas.falcon@...el.com>
Subject: Re: [PATCH v1 2/2] perf parse-events: Corrections to topdown sorting
On 3/5/2025 4:37 PM, Ian Rogers wrote:
> In the case of '{instructions,slots},faults,topdown-retiring' the
> first event that must be grouped, slots, is ignored causing the
> topdown-retiring event not to be adjacent to the group it needs to be
> inserted into. Don't ignore the group members when computing the
> force_grouped_index.
>
> Make the force_grouped_index be for the leader of the group it is
> within and always use it first rather than a group leader index so
> that topdown events may be sorted from one group into another.
>
> Reported-by: Dapeng Mi <dapeng1.mi@...ux.intel.com>
> Closes: https://lore.kernel.org/lkml/20250224083306.71813-2-dapeng1.mi@linux.intel.com/
> Signed-off-by: Ian Rogers <irogers@...gle.com>
> ---
> tools/perf/util/parse-events.c | 54 ++++++++++++++++++----------------
> 1 file changed, 28 insertions(+), 26 deletions(-)
>
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 35e48fe56dfa..cf32abc496e9 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -1983,31 +1983,30 @@ static int evlist__cmp(void *_fg_idx, const struct list_head *l, const struct li
> bool lhs_has_group, rhs_has_group;
>
> /*
> - * First sort by grouping/leader. Read the leader idx only if the evsel
> - * is part of a group, by default ungrouped events will be sorted
> - * relative to grouped events based on where the first ungrouped event
> - * occurs. If both events don't have a group we want to fall-through to
> - * the arch specific sorting, that can reorder and fix things like
> - * Intel's topdown events.
> + * Get the indexes of the 2 events to sort. If the events are
> + * in groups then the leader's index is used otherwise the
> + * event's index is used. Events in the same group will be
> + * sorted by PMU name. An index may be forced for events that
> + * must be in the same group, namely Intel topdown events.
> + * When everything is identical arch specific sorting is used,
> + * that can reorder and fix things like Intel's topdown
> + * events.
> */
> - if (lhs_core->leader != lhs_core || lhs_core->nr_members > 1) {
> - lhs_has_group = true;
> + lhs_has_group = lhs_core->leader != lhs_core || lhs_core->nr_members > 1;
> + if (*force_grouped_idx != -1 && arch_evsel__must_be_in_group(lhs))
> + lhs_sort_idx = *force_grouped_idx;
> + else if (lhs_has_group)
> 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;
> + else
> + lhs_sort_idx = lhs_core->idx;
> + rhs_has_group = rhs_core->leader != rhs_core || rhs_core->nr_members > 1;
> +
> + if (*force_grouped_idx != -1 && arch_evsel__must_be_in_group(rhs))
> + rhs_sort_idx = *force_grouped_idx;
> + else if (rhs_has_group)
> 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;
> - }
> + else
> + rhs_sort_idx = rhs_core->idx;
>
> if (lhs_sort_idx != rhs_sort_idx)
> return lhs_sort_idx - rhs_sort_idx;
> @@ -2055,10 +2054,13 @@ static int parse_events__sort_events_and_fix_groups(struct list_head *list)
> */
> pos->core.idx = 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;
> + /*
> + * Remember an index to sort all forced grouped events
> + * together to. Use the group leader as some events
> + * must appear first within the group.
> + */
> + if (force_grouped_idx == -1 && arch_evsel__must_be_in_group(pos))
> + force_grouped_idx = pos_leader->core.idx;
> }
>
> /* Sort events. */
Hi Ian,
With this fix, this topdown metrics sequence
"{instructions,slots},faults,topdown-retiring" indeed works on non-hybrid
platform, like SPR, but it still fails on hybrid platform.
Here is the result on Intel LNL platform.
./perf stat -e "{instructions,slots},faults,topdown-retiring" true
WARNING: events were regrouped to match PMUs
Performance counter stats for 'true':
*<not supported> * cpu_core/topdown-retiring/u
146,710 instructions:u
<not counted>
cpu_core/slots/u (0.00%)
<not counted>
instructions:u (0.00%)
49 faults:u
195,855 cpu_atom/topdown-retiring/u
0.001367139 seconds time elapsed
0.001402000 seconds user
0.000000000 seconds sys
the "cpu_core/topdown-retiring/" event is incorrectly moved to the head and
becomes the group leader.
To thoroughly fix this issue on hybrid platform, we need an extra below
change.
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 91c2b2e2c6bd..1f7772d4db6e 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -2006,7 +2006,7 @@ static int evlist__cmp(void *_fg_idx, const struct
list_head *l, const struct li
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) {
+ if (lhs_has_group || rhs_has_group) {
lhs_pmu_name = lhs->group_pmu_name;
rhs_pmu_name = rhs->group_pmu_name;
ret = strcmp(lhs_pmu_name, rhs_pmu_name);
Besides, since we support this new topdown events sequence regroup, the
comments and tests are need to be updated accordingly.
diff --git a/tools/perf/arch/x86/util/evlist.c
b/tools/perf/arch/x86/util/evlist.c
index 447a734e591c..8d7a7c4acd4b 100644
--- a/tools/perf/arch/x86/util/evlist.c
+++ b/tools/perf/arch/x86/util/evlist.c
@@ -39,28 +39,21 @@ int arch_evlist__cmp(const struct evsel *lhs, const
struct evsel *rhs)
* 26,319,024 slots
* 2,427,791 instructions
* 2,683,508 topdown-retiring
- *
- * If slots event and topdown metrics events are not in same group, the
- * topdown metrics events must be first event after the slots event
group,
- * otherwise topdown metrics events can't be regrouped correctly, e.g.
- *
- * a. perf stat -e "{instructions,slots},cycles,topdown-retiring"
-C0 sleep 1
+ * e. slots event and metrics event are in a group and not adjacent
+ * perf stat -e "{instructions,slots},cycles,topdown-retiring"
-C0 sleep 1
* WARNING: events were regrouped to match PMUs
- * Performance counter stats for 'CPU(s) 0':
- * 17,923,134 slots
- * 2,154,855 instructions
- * 3,015,058 cycles
- * <not supported> topdown-retiring
- *
- * If slots event and topdown metrics events are in two groups, the
group which
- * has topdown metrics events must contain only the topdown metrics
event,
- * otherwise topdown metrics event can't be regrouped correctly as
well, e.g.
- *
- * a. perf stat -e "{instructions,slots},{topdown-retiring,cycles}"
-C0 sleep 1
+ * Performance counter stats for 'true':
+ * 78,452,058 slots
+ * 10,767,929 topdown-retiring
+ * 9,438,226 instructions
+ * 13,080,988 cycles
+ * f. slots event and metrics event are in two groups and not adjacent
+ * perf stat -e "{instructions,slots},{cycles,topdown-retiring}"
-C0 sleep 1
* WARNING: events were regrouped to match PMUs
- * Error:
- * The sys_perf_event_open() syscall returned with 22 (Invalid
argument) for
- * event (topdown-retiring)
+ * 68,433,522 slots
+ * 8,856,102 topdown-retiring
+ * 7,791,494 instructions
+ * 11,469,513 cycles
*/
if (topdown_sys_has_perf_metrics() &&
(arch_evsel__must_be_in_group(lhs) ||
arch_evsel__must_be_in_group(rhs))) {
diff --git a/tools/perf/tests/shell/stat.sh b/tools/perf/tests/shell/stat.sh
index 68323d636fb7..a1b847c16f07 100755
--- a/tools/perf/tests/shell/stat.sh
+++ b/tools/perf/tests/shell/stat.sh
@@ -97,6 +97,18 @@ test_topdown_groups() {
err=1
return
fi
+ if perf stat -e '{instructions,slots},cycles,topdown-retiring' true 2>&1
| grep -E -q "<not supported>"
+ then
+ echo "Topdown event group test [Failed non-adjacent topdown metrics
group not move into slots group]"
+ err=1
+ return
+ fi
+ if perf stat -e '{instructions,slots},{cycles,topdown-retiring}' true
2>&1 | grep -E -q "<not supported>"
+ then
+ echo "Topdown event group test [Failed non-adjacent topdown metrics
group not merge into slots group]"
+ err=1
+ return
+ fi
if perf stat -e '{instructions,r400,r8000}' true 2>&1 | grep -E -q "<not
supported>"
then
echo "Topdown event group test [Failed raw format slots not reordered
first]"
Thanks,
Dapeng Mi
Powered by blists - more mailing lists