[PATCH 1/2] perf x86/topdown: Fix topdown leader sampling test error on hybrid

[PATCH 1/2] perf x86/topdown: Fix topdown leader sampling test error on hybrid

[Dapeng Mi]

When running topdown leader smapling test on Intel hybrid platforms,
such as LNL/ARL, we see the below error.

Topdown leader sampling test
Topdown leader sampling [Failed topdown events not reordered correctly]

It indciates the below command fails.

perf record -o "${perfdata}" -e "{instructions,slots,topdown-retiring}:S" true

The root cause is that perf tool creats a perf event for each PMU type
if it can create.

As for this command, there would be 5 perf events created,
cpu_atom/instructions/,cpu_atom/topdown_retiring/,
cpu_core/slots/,cpu_core/instructions/,cpu_core/topdown-retiring/

For these 5 events, the 2 cpu_atom events are in a group and the other 3
cpu_core events are in another group.

When arch_topdown_sample_read() traverses all these 5 events, events
cpu_atom/instructions/ and cpu_core/slots/ don't have a same group
leade, and then return false directly and lead to cpu_core/slots/ event
is used to sample and this is not allowed by PMU driver.

It's a overkill to return false directly if "evsel->core.leader !=
 leader->core.leader" since there could be multiple groups in the event
list.

Just "continue" instead of "return false" to fix this issue.

Fixes: 1e53e9d1787b ("perf x86/topdown: Correct leader selection with sample_read enabled")
Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
---
 tools/perf/arch/x86/util/topdown.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/arch/x86/util/topdown.c b/tools/perf/arch/x86/util/topdown.c
index f63747d0abdf..d1c654839049 100644
--- a/tools/perf/arch/x86/util/topdown.c
+++ b/tools/perf/arch/x86/util/topdown.c
@@ -81,7 +81,7 @@ bool arch_topdown_sample_read(struct evsel *leader)
 	 */
 	evlist__for_each_entry(leader->evlist, evsel) {
 		if (evsel->core.leader != leader->core.leader)
-			return false;
+			continue;
 		if (evsel != leader && arch_is_topdown_metrics(evsel))
 			return true;
 	}

base-commit: 4bac7fb5862740087825eda3ed6168e91da8b7e6
-- 
2.40.1

[PATCH 2/2] perf tools/tests: Fix topdown groups test on hybrid platforms

[Dapeng Mi]

When running topdown groups test on hybrid platforms like LNL/ARL, we
see the following 2 commands fail.

perf stat $cputype -e '{instructions,slots},topdown-retiring' true
perf stat $cputype -e '{instructions,slots},{topdown-retiring}' true

Take the 1st command as an example, 5 events are created on hybrid
platform. They are cpu_atom/instructions/, cpu_core/instructions/,
cpu_core/slots/, cpu_atom/topdown-retiring/ and
cpu_core/topdown-retiring/ events. The former 3 events are in a group
and the latter 2 topdown-retiring events are independent events.

As the limitation of current implementation, the
cpu_core/topdown-retiring/ event can't be moved into previous group as
long as there are other events before it. That's the reason why we see
the failure.

Thus add "--cputype core" option to limit only P-core events are tested.

Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
---
 tools/perf/arch/x86/util/evlist.c | 26 +++++++++++++++++++++++---
 tools/perf/tests/shell/stat.sh    | 20 ++++++++++++++++++--
 2 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/tools/perf/arch/x86/util/evlist.c b/tools/perf/arch/x86/util/evlist.c
index 447a734e591c..0a71ba975871 100644
--- a/tools/perf/arch/x86/util/evlist.c
+++ b/tools/perf/arch/x86/util/evlist.c
@@ -9,7 +9,7 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
 {
 	/*
 	 * Currently the following topdown events sequence are supported to
-	 * move and regroup correctly.
+	 * move and regroup correctly on non-hybrid platforms.
 	 *
 	 * a. all events in a group
 	 *    perf stat -e "{instructions,topdown-retiring,slots}" -C0 sleep 1
@@ -44,7 +44,7 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
 	 * 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. 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
@@ -56,11 +56,31 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
 	 * 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
+	 * f. perf stat -e "{instructions,slots},{topdown-retiring,cycles}" -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)
+	 *
+	 * For hybrid platforms, the sequences 'c' and 'd' are not supported as well
+	 * besides above sequences 'e' and 'f'.
+	 *
+	 *    perf stat -e "{instructions,slots},topdown-retiring" -C0 sleep 1
+	 *    perf stat -e "{instructions,slots},{topdown-retiring}" -C0 sleep 1
+	 *
+	 * On hybrid platforms each event would create an instance on all types of PMU
+	 * if the event can be supported by the PMU, i.e., the "topdown-retiring" event
+	 * would create two instances on hybrid platforms with P-cores and E-cores,
+	 * "cpu_core/topdown-retiring/" and "cpu_atom/topdown_retiring".
+	 *
+	 * Take the first command as an example, the events list would be converted to
+	 * below list in fact.
+	 *
+	 * "{cpu_atom/instructions/,cpu_core/instructions/,cpu_core/slots/},
+	 *  cpu_atom/topdown-retiring/,cpu_core/topdown-retiring/"
+	 *
+	 * This is actually same with event list in case 'e', "cpu_core/topdown-retiring/"
+	 * event can't be moved into previous events group.
 	 */
 	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..cdfe27c25528 100755
--- a/tools/perf/tests/shell/stat.sh
+++ b/tools/perf/tests/shell/stat.sh
@@ -5,6 +5,16 @@
 set -e
 
 err=0
+is_hybrid=false
+
+check_hybrid_platform() {
+  pmus=$(ls /sys/bus/event_source/devices/*/cpus 2>/dev/null | wc -l)
+  if [ "$pmus" -gt 1 ]
+  then
+    is_hybrid=true
+  fi
+}
+
 test_default_stat() {
   echo "Basic stat command test"
   if ! perf stat true 2>&1 | grep -E -q "Performance counter stats for 'true':"
@@ -62,6 +72,11 @@ test_topdown_groups() {
   # Topdown events must be grouped with the slots event first. Test that
   # parse-events reorders this.
   echo "Topdown event group test"
+  cputype=""
+  if $is_hybrid
+  then
+    cputype="--cputype core"
+  fi
   if ! perf stat -e '{slots,topdown-retiring}' true > /dev/null 2>&1
   then
     echo "Topdown event group test [Skipped event parsing failed]"
@@ -85,13 +100,13 @@ test_topdown_groups() {
     err=1
     return
   fi
-  if perf stat -e '{instructions,slots},topdown-retiring' true 2>&1 | grep -E -q "<not supported>"
+  if perf stat $cputype -e '{instructions,slots},topdown-retiring' true 2>&1 | grep -E -q "<not supported>"
   then
     echo "Topdown event group test [Failed topdown metrics event not move into slots group]"
     err=1
     return
   fi
-  if perf stat -e '{instructions,slots},{topdown-retiring}' true 2>&1 | grep -E -q "<not supported>"
+  if perf stat $cputype -e '{instructions,slots},{topdown-retiring}' true 2>&1 | grep -E -q "<not supported>"
   then
     echo "Topdown event group test [Failed topdown metrics group not merge into slots group]"
     err=1
@@ -200,6 +215,7 @@ test_hybrid() {
   echo "hybrid test [Success]"
 }
 
+check_hybrid_platform
 test_default_stat
 test_stat_record_report
 test_stat_record_script
-- 
2.40.1

Re: [PATCH 2/2] perf tools/tests: Fix topdown groups test on hybrid platforms

[Falcon, Thomas]

On Mon, 2025-02-24 at 08:33 +0000, Dapeng Mi wrote:
> When running topdown groups test on hybrid platforms like LNL/ARL, we
> see the following 2 commands fail.
> 
> perf stat $cputype -e '{instructions,slots},topdown-retiring' true
> perf stat $cputype -e '{instructions,slots},{topdown-retiring}' true
> 
> Take the 1st command as an example, 5 events are created on hybrid
> platform. They are cpu_atom/instructions/, cpu_core/instructions/,
> cpu_core/slots/, cpu_atom/topdown-retiring/ and
> cpu_core/topdown-retiring/ events. The former 3 events are in a group
> and the latter 2 topdown-retiring events are independent events.
> 
> As the limitation of current implementation, the
> cpu_core/topdown-retiring/ event can't be moved into previous group
> as
> long as there are other events before it. That's the reason why we
> see
> the failure.
> 
> Thus add "--cputype core" option to limit only P-core events are
> tested.
> 
> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>

Tested-by: Thomas Falcon <thomas.falcon@intel.com>

> ---
>  tools/perf/arch/x86/util/evlist.c | 26 +++++++++++++++++++++++---
>  tools/perf/tests/shell/stat.sh    | 20 ++++++++++++++++++--
>  2 files changed, 41 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/arch/x86/util/evlist.c
> b/tools/perf/arch/x86/util/evlist.c
> index 447a734e591c..0a71ba975871 100644
> --- a/tools/perf/arch/x86/util/evlist.c
> +++ b/tools/perf/arch/x86/util/evlist.c
> @@ -9,7 +9,7 @@ int arch_evlist__cmp(const struct evsel *lhs, const
> struct evsel *rhs)
>  {
>  	/*
>  	 * Currently the following topdown events sequence are
> supported to
> -	 * move and regroup correctly.
> +	 * move and regroup correctly on non-hybrid platforms.
>  	 *
>  	 * a. all events in a group
>  	 *    perf stat -e "{instructions,topdown-retiring,slots}" -
> C0 sleep 1
> @@ -44,7 +44,7 @@ int arch_evlist__cmp(const struct evsel *lhs, const
> struct evsel *rhs)
>  	 * 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. 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
> @@ -56,11 +56,31 @@ int arch_evlist__cmp(const struct evsel *lhs,
> const struct evsel *rhs)
>  	 * 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
> +	 * f. perf stat -e "{instructions,slots},{topdown-
> retiring,cycles}" -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)
> +	 *
> +	 * For hybrid platforms, the sequences 'c' and 'd' are not
> supported as well
> +	 * besides above sequences 'e' and 'f'.
> +	 *
> +	 *    perf stat -e "{instructions,slots},topdown-retiring" -
> C0 sleep 1
> +	 *    perf stat -e "{instructions,slots},{topdown-retiring}"
> -C0 sleep 1
> +	 *
> +	 * On hybrid platforms each event would create an instance
> on all types of PMU
> +	 * if the event can be supported by the PMU, i.e., the
> "topdown-retiring" event
> +	 * would create two instances on hybrid platforms with P-
> cores and E-cores,
> +	 * "cpu_core/topdown-retiring/" and
> "cpu_atom/topdown_retiring".
> +	 *
> +	 * Take the first command as an example, the events list
> would be converted to
> +	 * below list in fact.
> +	 *
> +	 *
> "{cpu_atom/instructions/,cpu_core/instructions/,cpu_core/slots/},
> +	 *  cpu_atom/topdown-retiring/,cpu_core/topdown-retiring/"
> +	 *
> +	 * This is actually same with event list in case 'e',
> "cpu_core/topdown-retiring/"
> +	 * event can't be moved into previous events group.
>  	 */
>  	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..cdfe27c25528 100755
> --- a/tools/perf/tests/shell/stat.sh
> +++ b/tools/perf/tests/shell/stat.sh
> @@ -5,6 +5,16 @@
>  set -e
>  
>  err=0
> +is_hybrid=false
> +
> +check_hybrid_platform() {
> +  pmus=$(ls /sys/bus/event_source/devices/*/cpus 2>/dev/null | wc -
> l)
> +  if [ "$pmus" -gt 1 ]
> +  then
> +    is_hybrid=true
> +  fi
> +}
> +
>  test_default_stat() {
>    echo "Basic stat command test"
>    if ! perf stat true 2>&1 | grep -E -q "Performance counter stats
> for 'true':"
> @@ -62,6 +72,11 @@ test_topdown_groups() {
>    # Topdown events must be grouped with the slots event first. Test
> that
>    # parse-events reorders this.
>    echo "Topdown event group test"
> +  cputype=""
> +  if $is_hybrid
> +  then
> +    cputype="--cputype core"
> +  fi
>    if ! perf stat -e '{slots,topdown-retiring}' true > /dev/null 2>&1
>    then
>      echo "Topdown event group test [Skipped event parsing failed]"
> @@ -85,13 +100,13 @@ test_topdown_groups() {
>      err=1
>      return
>    fi
> -  if perf stat -e '{instructions,slots},topdown-retiring' true 2>&1
> | grep -E -q "<not supported>"
> +  if perf stat $cputype -e '{instructions,slots},topdown-retiring'
> true 2>&1 | grep -E -q "<not supported>"
>    then
>      echo "Topdown event group test [Failed topdown metrics event not
> move into slots group]"
>      err=1
>      return
>    fi
> -  if perf stat -e '{instructions,slots},{topdown-retiring}' true
> 2>&1 | grep -E -q "<not supported>"
> +  if perf stat $cputype -e '{instructions,slots},{topdown-retiring}'
> true 2>&1 | grep -E -q "<not supported>"
>    then
>      echo "Topdown event group test [Failed topdown metrics group not
> merge into slots group]"
>      err=1
> @@ -200,6 +215,7 @@ test_hybrid() {
>    echo "hybrid test [Success]"
>  }
>  
> +check_hybrid_platform
>  test_default_stat
>  test_stat_record_report
>  test_stat_record_script

Re: [PATCH 2/2] perf tools/tests: Fix topdown groups test on hybrid platforms

[Ian Rogers]

On Sun, Feb 23, 2025 at 5:43 PM Dapeng Mi <dapeng1.mi@linux.intel.com> wrote:
>
> When running topdown groups test on hybrid platforms like LNL/ARL, we
> see the following 2 commands fail.
>
> perf stat $cputype -e '{instructions,slots},topdown-retiring' true
> perf stat $cputype -e '{instructions,slots},{topdown-retiring}' true
>
> Take the 1st command as an example, 5 events are created on hybrid
> platform. They are cpu_atom/instructions/, cpu_core/instructions/,
> cpu_core/slots/, cpu_atom/topdown-retiring/ and
> cpu_core/topdown-retiring/ events. The former 3 events are in a group
> and the latter 2 topdown-retiring events are independent events.
>
> As the limitation of current implementation, the
> cpu_core/topdown-retiring/ event can't be moved into previous group as
> long as there are other events before it. That's the reason why we see
> the failure.
>
> Thus add "--cputype core" option to limit only P-core events are tested.
>
> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
> ---
>  tools/perf/arch/x86/util/evlist.c | 26 +++++++++++++++++++++++---
>  tools/perf/tests/shell/stat.sh    | 20 ++++++++++++++++++--
>  2 files changed, 41 insertions(+), 5 deletions(-)
>
> diff --git a/tools/perf/arch/x86/util/evlist.c b/tools/perf/arch/x86/util/evlist.c
> index 447a734e591c..0a71ba975871 100644
> --- a/tools/perf/arch/x86/util/evlist.c
> +++ b/tools/perf/arch/x86/util/evlist.c
> @@ -9,7 +9,7 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
>  {
>         /*
>          * Currently the following topdown events sequence are supported to
> -        * move and regroup correctly.
> +        * move and regroup correctly on non-hybrid platforms.
>          *
>          * a. all events in a group
>          *    perf stat -e "{instructions,topdown-retiring,slots}" -C0 sleep 1
> @@ -44,7 +44,7 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
>          * 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. 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
> @@ -56,11 +56,31 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
>          * 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
> +        * f. perf stat -e "{instructions,slots},{topdown-retiring,cycles}" -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)
> +        *
> +        * For hybrid platforms, the sequences 'c' and 'd' are not supported as well
> +        * besides above sequences 'e' and 'f'.
> +        *
> +        *    perf stat -e "{instructions,slots},topdown-retiring" -C0 sleep 1
> +        *    perf stat -e "{instructions,slots},{topdown-retiring}" -C0 sleep 1
> +        *
> +        * On hybrid platforms each event would create an instance on all types of PMU
> +        * if the event can be supported by the PMU, i.e., the "topdown-retiring" event
> +        * would create two instances on hybrid platforms with P-cores and E-cores,
> +        * "cpu_core/topdown-retiring/" and "cpu_atom/topdown_retiring".
> +        *
> +        * Take the first command as an example, the events list would be converted to
> +        * below list in fact.
> +        *
> +        * "{cpu_atom/instructions/,cpu_core/instructions/,cpu_core/slots/},
> +        *  cpu_atom/topdown-retiring/,cpu_core/topdown-retiring/"
> +        *
> +        * This is actually same with event list in case 'e', "cpu_core/topdown-retiring/"
> +        * event can't be moved into previous events group.
>          */
>         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..cdfe27c25528 100755
> --- a/tools/perf/tests/shell/stat.sh
> +++ b/tools/perf/tests/shell/stat.sh
> @@ -5,6 +5,16 @@
>  set -e
>
>  err=0
> +is_hybrid=false
> +
> +check_hybrid_platform() {
> +  pmus=$(ls /sys/bus/event_source/devices/*/cpus 2>/dev/null | wc -l)
> +  if [ "$pmus" -gt 1 ]
> +  then
> +    is_hybrid=true
> +  fi
> +}
> +
>  test_default_stat() {
>    echo "Basic stat command test"
>    if ! perf stat true 2>&1 | grep -E -q "Performance counter stats for 'true':"
> @@ -62,6 +72,11 @@ test_topdown_groups() {
>    # Topdown events must be grouped with the slots event first. Test that
>    # parse-events reorders this.
>    echo "Topdown event group test"
> +  cputype=""
> +  if $is_hybrid
> +  then
> +    cputype="--cputype core"
> +  fi
>    if ! perf stat -e '{slots,topdown-retiring}' true > /dev/null 2>&1
>    then
>      echo "Topdown event group test [Skipped event parsing failed]"
> @@ -85,13 +100,13 @@ test_topdown_groups() {
>      err=1
>      return
>    fi
> -  if perf stat -e '{instructions,slots},topdown-retiring' true 2>&1 | grep -E -q "<not supported>"
> +  if perf stat $cputype -e '{instructions,slots},topdown-retiring' true 2>&1 | grep -E -q "<not supported>"

So I can make this test fail on non-hybrid with:
```
$ perf stat -e '{instructions,slots},faults,topdown-retiring' true
WARNING: events were regrouped to match PMUs

Performance counter stats for 'true':

        5,312,770      slots
        1,078,401      instructions
               56      faults
  <not supported>      topdown-retiring
```
The issue is that the slots isn't a group leader, so the "force group
index" we try to insert must be grouped events into is miscalculated
to be topdown-retiring. The following fixes this:
```
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 35e48fe56dfa..68ddc335cde4 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -2056,8 +2056,7 @@ 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))
+               if (force_grouped_idx == -1 &&
arch_evsel__must_be_in_group(pos))
                        force_grouped_idx = pos->core.idx;
        }
```

>    then
>      echo "Topdown event group test [Failed topdown metrics event not move into slots group]"
>      err=1
>      return
>    fi
> -  if perf stat -e '{instructions,slots},{topdown-retiring}' true 2>&1 | grep -E -q "<not supported>"
> +  if perf stat $cputype -e '{instructions,slots},{topdown-retiring}' true 2>&1 | grep -E -q "<not supported>"

So this is trickier as '{topdown-retiring}' ends up looking as a 2
event group. I'll post a proposed fix.

Thanks,
Ian

>    then
>      echo "Topdown event group test [Failed topdown metrics group not merge into slots group]"
>      err=1
> @@ -200,6 +215,7 @@ test_hybrid() {
>    echo "hybrid test [Success]"
>  }
>
> +check_hybrid_platform
>  test_default_stat
>  test_stat_record_report
>  test_stat_record_script
> --
> 2.40.1
>

Re: [PATCH 2/2] perf tools/tests: Fix topdown groups test on hybrid platforms

[Ian Rogers]

On Tue, Mar 4, 2025 at 10:07 PM Ian Rogers <irogers@google.com> wrote:
>
> On Sun, Feb 23, 2025 at 5:43 PM Dapeng Mi <dapeng1.mi@linux.intel.com> wrote:
> >
> > When running topdown groups test on hybrid platforms like LNL/ARL, we
> > see the following 2 commands fail.
> >
> > perf stat $cputype -e '{instructions,slots},topdown-retiring' true
> > perf stat $cputype -e '{instructions,slots},{topdown-retiring}' true
> >
> > Take the 1st command as an example, 5 events are created on hybrid
> > platform. They are cpu_atom/instructions/, cpu_core/instructions/,
> > cpu_core/slots/, cpu_atom/topdown-retiring/ and
> > cpu_core/topdown-retiring/ events. The former 3 events are in a group
> > and the latter 2 topdown-retiring events are independent events.
> >
> > As the limitation of current implementation, the
> > cpu_core/topdown-retiring/ event can't be moved into previous group as
> > long as there are other events before it. That's the reason why we see
> > the failure.
> >
> > Thus add "--cputype core" option to limit only P-core events are tested.
> >
> > Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
> > ---
> >  tools/perf/arch/x86/util/evlist.c | 26 +++++++++++++++++++++++---
> >  tools/perf/tests/shell/stat.sh    | 20 ++++++++++++++++++--
> >  2 files changed, 41 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/perf/arch/x86/util/evlist.c b/tools/perf/arch/x86/util/evlist.c
> > index 447a734e591c..0a71ba975871 100644
> > --- a/tools/perf/arch/x86/util/evlist.c
> > +++ b/tools/perf/arch/x86/util/evlist.c
> > @@ -9,7 +9,7 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
> >  {
> >         /*
> >          * Currently the following topdown events sequence are supported to
> > -        * move and regroup correctly.
> > +        * move and regroup correctly on non-hybrid platforms.
> >          *
> >          * a. all events in a group
> >          *    perf stat -e "{instructions,topdown-retiring,slots}" -C0 sleep 1
> > @@ -44,7 +44,7 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
> >          * 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. 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
> > @@ -56,11 +56,31 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
> >          * 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
> > +        * f. perf stat -e "{instructions,slots},{topdown-retiring,cycles}" -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)
> > +        *
> > +        * For hybrid platforms, the sequences 'c' and 'd' are not supported as well
> > +        * besides above sequences 'e' and 'f'.
> > +        *
> > +        *    perf stat -e "{instructions,slots},topdown-retiring" -C0 sleep 1
> > +        *    perf stat -e "{instructions,slots},{topdown-retiring}" -C0 sleep 1
> > +        *
> > +        * On hybrid platforms each event would create an instance on all types of PMU
> > +        * if the event can be supported by the PMU, i.e., the "topdown-retiring" event
> > +        * would create two instances on hybrid platforms with P-cores and E-cores,
> > +        * "cpu_core/topdown-retiring/" and "cpu_atom/topdown_retiring".
> > +        *
> > +        * Take the first command as an example, the events list would be converted to
> > +        * below list in fact.
> > +        *
> > +        * "{cpu_atom/instructions/,cpu_core/instructions/,cpu_core/slots/},
> > +        *  cpu_atom/topdown-retiring/,cpu_core/topdown-retiring/"
> > +        *
> > +        * This is actually same with event list in case 'e', "cpu_core/topdown-retiring/"
> > +        * event can't be moved into previous events group.
> >          */
> >         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..cdfe27c25528 100755
> > --- a/tools/perf/tests/shell/stat.sh
> > +++ b/tools/perf/tests/shell/stat.sh
> > @@ -5,6 +5,16 @@
> >  set -e
> >
> >  err=0
> > +is_hybrid=false
> > +
> > +check_hybrid_platform() {
> > +  pmus=$(ls /sys/bus/event_source/devices/*/cpus 2>/dev/null | wc -l)
> > +  if [ "$pmus" -gt 1 ]
> > +  then
> > +    is_hybrid=true
> > +  fi
> > +}
> > +
> >  test_default_stat() {
> >    echo "Basic stat command test"
> >    if ! perf stat true 2>&1 | grep -E -q "Performance counter stats for 'true':"
> > @@ -62,6 +72,11 @@ test_topdown_groups() {
> >    # Topdown events must be grouped with the slots event first. Test that
> >    # parse-events reorders this.
> >    echo "Topdown event group test"
> > +  cputype=""
> > +  if $is_hybrid
> > +  then
> > +    cputype="--cputype core"
> > +  fi
> >    if ! perf stat -e '{slots,topdown-retiring}' true > /dev/null 2>&1
> >    then
> >      echo "Topdown event group test [Skipped event parsing failed]"
> > @@ -85,13 +100,13 @@ test_topdown_groups() {
> >      err=1
> >      return
> >    fi
> > -  if perf stat -e '{instructions,slots},topdown-retiring' true 2>&1 | grep -E -q "<not supported>"
> > +  if perf stat $cputype -e '{instructions,slots},topdown-retiring' true 2>&1 | grep -E -q "<not supported>"
>
> So I can make this test fail on non-hybrid with:
> ```
> $ perf stat -e '{instructions,slots},faults,topdown-retiring' true
> WARNING: events were regrouped to match PMUs
>
> Performance counter stats for 'true':
>
>         5,312,770      slots
>         1,078,401      instructions
>                56      faults
>   <not supported>      topdown-retiring
> ```
> The issue is that the slots isn't a group leader, so the "force group
> index" we try to insert must be grouped events into is miscalculated
> to be topdown-retiring. The following fixes this:
> ```
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 35e48fe56dfa..68ddc335cde4 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -2056,8 +2056,7 @@ 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))
> +               if (force_grouped_idx == -1 &&
> arch_evsel__must_be_in_group(pos))
>                         force_grouped_idx = pos->core.idx;
>         }
> ```
>
> >    then
> >      echo "Topdown event group test [Failed topdown metrics event not move into slots group]"
> >      err=1
> >      return
> >    fi
> > -  if perf stat -e '{instructions,slots},{topdown-retiring}' true 2>&1 | grep -E -q "<not supported>"
> > +  if perf stat $cputype -e '{instructions,slots},{topdown-retiring}' true 2>&1 | grep -E -q "<not supported>"
>
> So this is trickier as '{topdown-retiring}' ends up looking as a 2
> event group. I'll post a proposed fix.

Patch here:
https://lore.kernel.org/lkml/20250305083735.393333-2-irogers@google.com/

Thanks,
Ian

>
> >    then
> >      echo "Topdown event group test [Failed topdown metrics group not merge into slots group]"
> >      err=1
> > @@ -200,6 +215,7 @@ test_hybrid() {
> >    echo "hybrid test [Success]"
> >  }
> >
> > +check_hybrid_platform
> >  test_default_stat
> >  test_stat_record_report
> >  test_stat_record_script
> > --
> > 2.40.1
> >

Re: [PATCH 1/2] perf x86/topdown: Fix topdown leader sampling test error on hybrid

[Falcon, Thomas]

On Mon, 2025-02-24 at 08:33 +0000, Dapeng Mi wrote:
> When running topdown leader smapling test on Intel hybrid platforms,
> such as LNL/ARL, we see the below error.
> 
> Topdown leader sampling test
> Topdown leader sampling [Failed topdown events not reordered
> correctly]
> 
> It indciates the below command fails.
> 
> perf record -o "${perfdata}" -e "{instructions,slots,topdown-
> retiring}:S" true
> 
> The root cause is that perf tool creats a perf event for each PMU
> type
> if it can create.
> 
> As for this command, there would be 5 perf events created,
> cpu_atom/instructions/,cpu_atom/topdown_retiring/,
> cpu_core/slots/,cpu_core/instructions/,cpu_core/topdown-retiring/
> 
> For these 5 events, the 2 cpu_atom events are in a group and the
> other 3
> cpu_core events are in another group.
> 
> When arch_topdown_sample_read() traverses all these 5 events, events
> cpu_atom/instructions/ and cpu_core/slots/ don't have a same group
> leade, and then return false directly and lead to cpu_core/slots/
> event
> is used to sample and this is not allowed by PMU driver.
> 
> It's a overkill to return false directly if "evsel->core.leader !=
>  leader->core.leader" since there could be multiple groups in the
> event
> list.
> 
> Just "continue" instead of "return false" to fix this issue.
> 
> Fixes: 1e53e9d1787b ("perf x86/topdown: Correct leader selection with
> sample_read enabled")
> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>

Tested-by: Thomas Falcon <thomas.falcon@intel.com>

> ---
>  tools/perf/arch/x86/util/topdown.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/arch/x86/util/topdown.c
> b/tools/perf/arch/x86/util/topdown.c
> index f63747d0abdf..d1c654839049 100644
> --- a/tools/perf/arch/x86/util/topdown.c
> +++ b/tools/perf/arch/x86/util/topdown.c
> @@ -81,7 +81,7 @@ bool arch_topdown_sample_read(struct evsel *leader)
>  	 */
>  	evlist__for_each_entry(leader->evlist, evsel) {
>  		if (evsel->core.leader != leader->core.leader)
> -			return false;
> +			continue;
>  		if (evsel != leader &&
> arch_is_topdown_metrics(evsel))
>  			return true;
>  	}
> 
> base-commit: 4bac7fb5862740087825eda3ed6168e91da8b7e6

Re: [PATCH 1/2] perf x86/topdown: Fix topdown leader sampling test error on hybrid

[Ian Rogers]

On Tue, Mar 4, 2025 at 8:49 AM Falcon, Thomas <thomas.falcon@intel.com> wrote:
>
> On Mon, 2025-02-24 at 08:33 +0000, Dapeng Mi wrote:
> > When running topdown leader smapling test on Intel hybrid platforms,
> > such as LNL/ARL, we see the below error.
> >
> > Topdown leader sampling test
> > Topdown leader sampling [Failed topdown events not reordered
> > correctly]
> >
> > It indciates the below command fails.
> >
> > perf record -o "${perfdata}" -e "{instructions,slots,topdown-
> > retiring}:S" true
> >
> > The root cause is that perf tool creats a perf event for each PMU
> > type
> > if it can create.
> >
> > As for this command, there would be 5 perf events created,
> > cpu_atom/instructions/,cpu_atom/topdown_retiring/,
> > cpu_core/slots/,cpu_core/instructions/,cpu_core/topdown-retiring/
> >
> > For these 5 events, the 2 cpu_atom events are in a group and the
> > other 3
> > cpu_core events are in another group.
> >
> > When arch_topdown_sample_read() traverses all these 5 events, events
> > cpu_atom/instructions/ and cpu_core/slots/ don't have a same group
> > leade, and then return false directly and lead to cpu_core/slots/
> > event
> > is used to sample and this is not allowed by PMU driver.
> >
> > It's a overkill to return false directly if "evsel->core.leader !=
> >  leader->core.leader" since there could be multiple groups in the
> > event
> > list.
> >
> > Just "continue" instead of "return false" to fix this issue.
> >
> > Fixes: 1e53e9d1787b ("perf x86/topdown: Correct leader selection with
> > sample_read enabled")
> > Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
>
> Tested-by: Thomas Falcon <thomas.falcon@intel.com>

On an Alderlake:
Tested-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> > ---
> >  tools/perf/arch/x86/util/topdown.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/arch/x86/util/topdown.c
> > b/tools/perf/arch/x86/util/topdown.c
> > index f63747d0abdf..d1c654839049 100644
> > --- a/tools/perf/arch/x86/util/topdown.c
> > +++ b/tools/perf/arch/x86/util/topdown.c
> > @@ -81,7 +81,7 @@ bool arch_topdown_sample_read(struct evsel *leader)
> >        */
> >       evlist__for_each_entry(leader->evlist, evsel) {
> >               if (evsel->core.leader != leader->core.leader)
> > -                     return false;
> > +                     continue;
> >               if (evsel != leader &&
> > arch_is_topdown_metrics(evsel))
> >                       return true;
> >       }
> >
> > base-commit: 4bac7fb5862740087825eda3ed6168e91da8b7e6
>