[Patch v3 0/5] Bug fixes on topdown events reordering

[Patch v3 0/5] Bug fixes on topdown events reordering

[Dapeng Mi]

Changes:
v2 -> v3:
  * Refine arch_is_topdown_metrics() (Kan Liang)
  * Only move topdown metric events forward when they are not in the
    group with previous event (Ian)
  * Check if topdown events is supported before test topown events
    leader sampling (Kan Liang)

History:
  v1: https://lore.kernel.org/all/20240702224037.343958-1-dapeng1.mi@linux.intel.com/
  v2: https://lore.kernel.org/all/20240708144204.839486-1-dapeng1.mi@linux.intel.com/


Dapeng Mi (5):
  perf x86/topdown: Complete topdown slots/metrics events check
  perf x86/topdown: Correct leader selection with sample_read enabled
  perf x86/topdown: Don't move topdown metric events in group
  perf tests: Add leader sampling test in record tests
  perf tests: Add topdown events counting and sampling tests

 tools/perf/arch/x86/util/evlist.c  | 13 +++++--
 tools/perf/arch/x86/util/evsel.c   |  3 +-
 tools/perf/arch/x86/util/topdown.c | 62 +++++++++++++++++++++++++++++-
 tools/perf/arch/x86/util/topdown.h |  2 +
 tools/perf/tests/shell/record.sh   | 45 ++++++++++++++++++++++
 tools/perf/tests/shell/stat.sh     |  6 +++
 6 files changed, 124 insertions(+), 7 deletions(-)


base-commit: 73e931504f8e0d42978bfcda37b323dbbd1afc08
-- 
2.40.1

[Patch v3 1/5] perf x86/topdown: Complete topdown slots/metrics events check

[Dapeng Mi]

It's not complete to check whether an event is a topdown slots or
topdown metrics event by only comparing the event name since user
may assign the event by RAW format, e.g.

perf stat -e '{instructions,cpu/r400/,cpu/r8300/}' sleep 1

 Performance counter stats for 'sleep 1':

     <not counted>      instructions
     <not counted>      cpu/r400/
   <not supported>      cpu/r8300/

       1.002917796 seconds time elapsed

       0.002955000 seconds user
       0.000000000 seconds sys

The RAW format slots and topdown-be-bound events are not recognized and
not regroup the events, and eventually cause error.

Thus add two helpers arch_is_topdown_slots()/arch_is_topdown_metrics()
to detect whether an event is topdown slots/metrics event by comparing
the event config directly, and use these two helpers to replace the
original event name comparisons.

Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
---
 tools/perf/arch/x86/util/evlist.c  |  8 ++---
 tools/perf/arch/x86/util/evsel.c   |  3 +-
 tools/perf/arch/x86/util/topdown.c | 48 +++++++++++++++++++++++++++++-
 tools/perf/arch/x86/util/topdown.h |  2 ++
 4 files changed, 55 insertions(+), 6 deletions(-)

diff --git a/tools/perf/arch/x86/util/evlist.c b/tools/perf/arch/x86/util/evlist.c
index b1ce0c52d88d..332e8907f43e 100644
--- a/tools/perf/arch/x86/util/evlist.c
+++ b/tools/perf/arch/x86/util/evlist.c
@@ -78,14 +78,14 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
 	if (topdown_sys_has_perf_metrics() &&
 	    (arch_evsel__must_be_in_group(lhs) || arch_evsel__must_be_in_group(rhs))) {
 		/* Ensure the topdown slots comes first. */
-		if (strcasestr(lhs->name, "slots") && !strcasestr(lhs->name, "uops_retired.slots"))
+		if (arch_is_topdown_slots(lhs))
 			return -1;
-		if (strcasestr(rhs->name, "slots") && !strcasestr(rhs->name, "uops_retired.slots"))
+		if (arch_is_topdown_slots(rhs))
 			return 1;
 		/* Followed by topdown events. */
-		if (strcasestr(lhs->name, "topdown") && !strcasestr(rhs->name, "topdown"))
+		if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
 			return -1;
-		if (!strcasestr(lhs->name, "topdown") && strcasestr(rhs->name, "topdown"))
+		if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs))
 			return 1;
 	}
 
diff --git a/tools/perf/arch/x86/util/evsel.c b/tools/perf/arch/x86/util/evsel.c
index 090d0f371891..181f2ba0bb2a 100644
--- a/tools/perf/arch/x86/util/evsel.c
+++ b/tools/perf/arch/x86/util/evsel.c
@@ -6,6 +6,7 @@
 #include "util/pmu.h"
 #include "util/pmus.h"
 #include "linux/string.h"
+#include "topdown.h"
 #include "evsel.h"
 #include "util/debug.h"
 #include "env.h"
@@ -44,7 +45,7 @@ bool arch_evsel__must_be_in_group(const struct evsel *evsel)
 	    strcasestr(evsel->name, "uops_retired.slots"))
 		return false;
 
-	return strcasestr(evsel->name, "topdown") || strcasestr(evsel->name, "slots");
+	return arch_is_topdown_metrics(evsel) || arch_is_topdown_slots(evsel);
 }
 
 int arch_evsel__hw_name(struct evsel *evsel, char *bf, size_t size)
diff --git a/tools/perf/arch/x86/util/topdown.c b/tools/perf/arch/x86/util/topdown.c
index 3f9a267d4501..49f25d67ed77 100644
--- a/tools/perf/arch/x86/util/topdown.c
+++ b/tools/perf/arch/x86/util/topdown.c
@@ -32,6 +32,52 @@ bool topdown_sys_has_perf_metrics(void)
 }
 
 #define TOPDOWN_SLOTS		0x0400
+bool arch_is_topdown_slots(const struct evsel *evsel)
+{
+	if (evsel->core.attr.config == TOPDOWN_SLOTS)
+		return true;
+
+	return false;
+}
+
+static int compare_topdown_event(void *vstate, struct pmu_event_info *info)
+{
+	int *config = vstate;
+	int event = 0;
+	int umask = 0;
+	char *str;
+
+	if (!strcasestr(info->name, "topdown"))
+		return 0;
+
+	str = strcasestr(info->str, "event=");
+	if (str)
+		sscanf(str, "event=%x", &event);
+
+	str = strcasestr(info->str, "umask=");
+	if (str)
+		sscanf(str, "umask=%x", &umask);
+
+	if (event == 0 && *config == (event | umask << 8))
+		return 1;
+
+	return 0;
+}
+
+bool arch_is_topdown_metrics(const struct evsel *evsel)
+{
+	struct perf_pmu *pmu = evsel__find_pmu(evsel);
+	int config = evsel->core.attr.config;
+
+	if (!pmu || !pmu->is_core)
+		return false;
+
+	if (perf_pmu__for_each_event(pmu, false, &config,
+				     compare_topdown_event))
+		return true;
+
+	return false;
+}
 
 /*
  * Check whether a topdown group supports sample-read.
@@ -44,7 +90,7 @@ bool arch_topdown_sample_read(struct evsel *leader)
 	if (!evsel__sys_has_perf_metrics(leader))
 		return false;
 
-	if (leader->core.attr.config == TOPDOWN_SLOTS)
+	if (arch_is_topdown_slots(leader))
 		return true;
 
 	return false;
diff --git a/tools/perf/arch/x86/util/topdown.h b/tools/perf/arch/x86/util/topdown.h
index 46bf9273e572..1bae9b1822d7 100644
--- a/tools/perf/arch/x86/util/topdown.h
+++ b/tools/perf/arch/x86/util/topdown.h
@@ -3,5 +3,7 @@
 #define _TOPDOWN_H 1
 
 bool topdown_sys_has_perf_metrics(void);
+bool arch_is_topdown_slots(const struct evsel *evsel);
+bool arch_is_topdown_metrics(const struct evsel *evsel);
 
 #endif
-- 
2.40.1

[Patch v3 2/5] perf x86/topdown: Correct leader selection with sample_read enabled

[Dapeng Mi]

Addresses an issue where, in the absence of a topdown metrics event
within a sampling group, the slots event was incorrectly bypassed as
the sampling leader when sample_read was enabled.

perf record -e '{slots,branches}:S' -c 10000 -vv sleep 1

In this case, the slots event should be sampled as leader but the
branches event is sampled in fact like the verbose output shows.

perf_event_attr:
  type                             4 (cpu)
  size                             168
  config                           0x400 (slots)
  sample_type                      IP|TID|TIME|READ|CPU|IDENTIFIER
  read_format                      ID|GROUP|LOST
  disabled                         1
  sample_id_all                    1
  exclude_guest                    1
------------------------------------------------------------
sys_perf_event_open: pid -1  cpu 0  group_fd -1  flags 0x8 = 5
------------------------------------------------------------
perf_event_attr:
  type                             0 (PERF_TYPE_HARDWARE)
  size                             168
  config                           0x4 (PERF_COUNT_HW_BRANCH_INSTRUCTIONS)
  { sample_period, sample_freq }   10000
  sample_type                      IP|TID|TIME|READ|CPU|IDENTIFIER
  read_format                      ID|GROUP|LOST
  sample_id_all                    1
  exclude_guest                    1

The sample period of slots event instead of branches event is reset to
0.

This fix ensures the slots event remains the leader under these
conditions.

Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
---
 tools/perf/arch/x86/util/topdown.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/tools/perf/arch/x86/util/topdown.c b/tools/perf/arch/x86/util/topdown.c
index 49f25d67ed77..857e00cf579f 100644
--- a/tools/perf/arch/x86/util/topdown.c
+++ b/tools/perf/arch/x86/util/topdown.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 #include "api/fs/fs.h"
 #include "util/evsel.h"
+#include "util/evlist.h"
 #include "util/pmu.h"
 #include "util/pmus.h"
 #include "util/topdown.h"
@@ -87,11 +88,22 @@ bool arch_is_topdown_metrics(const struct evsel *evsel)
  */
 bool arch_topdown_sample_read(struct evsel *leader)
 {
+	struct evsel *evsel;
+
 	if (!evsel__sys_has_perf_metrics(leader))
 		return false;
 
-	if (arch_is_topdown_slots(leader))
-		return true;
+	if (!arch_is_topdown_slots(leader))
+		return false;
+
+	/*
+	 * If slots event as leader event but no topdown metric events
+	 * in group, slots event should still sample as leader.
+	 */
+	evlist__for_each_entry(leader->evlist, evsel) {
+		if (evsel != leader && arch_is_topdown_metrics(evsel))
+			return true;
+	}
 
 	return false;
 }
-- 
2.40.1

[Patch v3 3/5] perf x86/topdown: Don't move topdown metric events in group

[Dapeng Mi]

when running below perf command, we say error is reported.

perf record -e "{slots,instructions,topdown-retiring}:S" -vv -C0 sleep 1

------------------------------------------------------------
perf_event_attr:
  type                             4 (cpu)
  size                             168
  config                           0x400 (slots)
  sample_type                      IP|TID|TIME|READ|CPU|PERIOD|IDENTIFIER
  read_format                      ID|GROUP|LOST
  disabled                         1
  sample_id_all                    1
  exclude_guest                    1
------------------------------------------------------------
sys_perf_event_open: pid -1  cpu 0  group_fd -1  flags 0x8 = 5
------------------------------------------------------------
perf_event_attr:
  type                             4 (cpu)
  size                             168
  config                           0x8000 (topdown-retiring)
  { sample_period, sample_freq }   4000
  sample_type                      IP|TID|TIME|READ|CPU|PERIOD|IDENTIFIER
  read_format                      ID|GROUP|LOST
  freq                             1
  sample_id_all                    1
  exclude_guest                    1
------------------------------------------------------------
sys_perf_event_open: pid -1  cpu 0  group_fd 5  flags 0x8
sys_perf_event_open failed, error -22

Error:
The sys_perf_event_open() syscall returned with 22 (Invalid argument) for event (topdown-retiring).

The reason of error is that the events are regrouped and
topdown-retiring event is moved to closely after the slots event and
topdown-retiring event needs to do the sampling, but Intel PMU driver
doesn't support to sample topdown metrics events.

For topdown metrics events, it just requires to be in a group which has
slots event as leader. It doesn't require topdown metrics event must be
closely after slots event. Thus it's a overkill to move topdown metrics
event closely after slots event in events regrouping and furtherly cause
the above issue.

Thus don't move topdown metrics events forward if they are already in a
group.

Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
---
 tools/perf/arch/x86/util/evlist.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/perf/arch/x86/util/evlist.c b/tools/perf/arch/x86/util/evlist.c
index 332e8907f43e..6ae044f21843 100644
--- a/tools/perf/arch/x86/util/evlist.c
+++ b/tools/perf/arch/x86/util/evlist.c
@@ -85,7 +85,12 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
 		/* Followed by topdown events. */
 		if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
 			return -1;
-		if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs))
+		/*
+		 * Move topdown events forward only when topdown events
+		 * are not in same group with previous event.
+		 */
+		if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs) &&
+		    lhs->core.leader != rhs->core.leader)
 			return 1;
 	}
 
-- 
2.40.1

[Patch v3 4/5] perf tests: Add leader sampling test in record tests

[Dapeng Mi]

Add leader sampling test to validate event counts are captured into
record and the count value is consistent.

Suggested-by: Kan Liang <kan.liang@linux.intel.com>
Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
---
 tools/perf/tests/shell/record.sh | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/tools/perf/tests/shell/record.sh b/tools/perf/tests/shell/record.sh
index 3d1a7759a7b2..8e3e66780fed 100755
--- a/tools/perf/tests/shell/record.sh
+++ b/tools/perf/tests/shell/record.sh
@@ -17,6 +17,7 @@ skip_test_missing_symbol ${testsym}
 
 err=0
 perfdata=$(mktemp /tmp/__perf_test.perf.data.XXXXX)
+script_output=$(mktemp /tmp/__perf_test.perf.data.XXXXX.script)
 testprog="perf test -w thloop"
 cpu_pmu_dir="/sys/bus/event_source/devices/cpu*"
 br_cntr_file="/caps/branch_counter_nr"
@@ -190,11 +191,38 @@ test_branch_counter() {
   echo "Basic branch counter test [Success]"
 }
 
+test_leader_sampling() {
+  echo "Basic leader sampling test"
+  if ! perf record -o "${perfdata}" -e "{branches,branches}:Su" perf test -w brstack 2> /dev/null
+  then
+    echo "Leader sampling [Failed record]"
+    err=1
+    return
+  fi
+  index=0
+  perf script -i "${perfdata}" > $script_output
+  while IFS= read -r line
+  do
+    # Check if the two branches counts are equal in each record
+    branches=$(echo $line | awk '{for(i=1;i<=NF;i++) if($i=="branches:") print $(i-1)}')
+    if [ $(($index%2)) -ne 0 ] && [ ${branches}x != ${prev_branches}x ]
+    then
+      echo "Leader sampling [Failed inconsistent branches count]"
+      err=1
+      return
+    fi
+    index=$(($index+1))
+    prev_branches=$branches
+  done < $script_output
+  echo "Basic leader sampling test [Success]"
+}
+
 test_per_thread
 test_register_capture
 test_system_wide
 test_workload
 test_branch_counter
+test_leader_sampling
 
 cleanup
 exit $err
-- 
2.40.1

[Patch v3 5/5] perf tests: Add topdown events counting and sampling tests

[Dapeng Mi]

Add counting and leader sampling tests to verify topdown events including
raw format can be reordered correctly.

Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
---
 tools/perf/tests/shell/record.sh | 17 +++++++++++++++++
 tools/perf/tests/shell/stat.sh   |  6 ++++++
 2 files changed, 23 insertions(+)

diff --git a/tools/perf/tests/shell/record.sh b/tools/perf/tests/shell/record.sh
index 8e3e66780fed..9d0c43427811 100755
--- a/tools/perf/tests/shell/record.sh
+++ b/tools/perf/tests/shell/record.sh
@@ -217,12 +217,29 @@ test_leader_sampling() {
   echo "Basic leader sampling test [Success]"
 }
 
+test_topdown_leader_sampling() {
+  echo "Topdown leader sampling test"
+  if ! perf stat -e "{slots,topdown-retiring}" true 2> /dev/null
+  then
+    echo "Topdown leader sampling [Skipped event parsing failed]"
+    return
+  fi
+  if ! perf record -o "${perfdata}" -e "{instructions,slots,topdown-retiring}:S" true 2> /dev/null
+  then
+    echo "Topdown leader sampling [Failed topdown events not reordered correctly]"
+    err=1
+    return
+  fi
+  echo "Topdown leader sampling test [Success]"
+}
+
 test_per_thread
 test_register_capture
 test_system_wide
 test_workload
 test_branch_counter
 test_leader_sampling
+test_topdown_leader_sampling
 
 cleanup
 exit $err
diff --git a/tools/perf/tests/shell/stat.sh b/tools/perf/tests/shell/stat.sh
index 3f1e67795490..092a7a2abcf8 100755
--- a/tools/perf/tests/shell/stat.sh
+++ b/tools/perf/tests/shell/stat.sh
@@ -79,6 +79,12 @@ test_topdown_groups() {
     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]"
+    err=1
+    return
+  fi
   echo "Topdown event group test [Success]"
 }
 
-- 
2.40.1

Re: [Patch v3 0/5] Bug fixes on topdown events reordering

[Mi, Dapeng]

Hi Ian,

Kindly ping, any comments here? Thanks.

On 7/13/2024 1:03 AM, Dapeng Mi wrote:
> Changes:
> v2 -> v3:
>   * Refine arch_is_topdown_metrics() (Kan Liang)
>   * Only move topdown metric events forward when they are not in the
>     group with previous event (Ian)
>   * Check if topdown events is supported before test topown events
>     leader sampling (Kan Liang)
>
> History:
>   v1: https://lore.kernel.org/all/20240702224037.343958-1-dapeng1.mi@linux.intel.com/
>   v2: https://lore.kernel.org/all/20240708144204.839486-1-dapeng1.mi@linux.intel.com/
>
>
> Dapeng Mi (5):
>   perf x86/topdown: Complete topdown slots/metrics events check
>   perf x86/topdown: Correct leader selection with sample_read enabled
>   perf x86/topdown: Don't move topdown metric events in group
>   perf tests: Add leader sampling test in record tests
>   perf tests: Add topdown events counting and sampling tests
>
>  tools/perf/arch/x86/util/evlist.c  | 13 +++++--
>  tools/perf/arch/x86/util/evsel.c   |  3 +-
>  tools/perf/arch/x86/util/topdown.c | 62 +++++++++++++++++++++++++++++-
>  tools/perf/arch/x86/util/topdown.h |  2 +
>  tools/perf/tests/shell/record.sh   | 45 ++++++++++++++++++++++
>  tools/perf/tests/shell/stat.sh     |  6 +++
>  6 files changed, 124 insertions(+), 7 deletions(-)
>
>
> base-commit: 73e931504f8e0d42978bfcda37b323dbbd1afc08

Re: [Patch v3 1/5] perf x86/topdown: Complete topdown slots/metrics events check

[Arnaldo Carvalho de Melo]

On Fri, Jul 12, 2024 at 05:03:35PM +0000, Dapeng Mi wrote:
> It's not complete to check whether an event is a topdown slots or
> topdown metrics event by only comparing the event name since user
> may assign the event by RAW format, e.g.
> 
> perf stat -e '{instructions,cpu/r400/,cpu/r8300/}' sleep 1
> 
>  Performance counter stats for 'sleep 1':
> 
>      <not counted>      instructions
>      <not counted>      cpu/r400/
>    <not supported>      cpu/r8300/
> 
>        1.002917796 seconds time elapsed
> 
>        0.002955000 seconds user
>        0.000000000 seconds sys
> 
> The RAW format slots and topdown-be-bound events are not recognized and
> not regroup the events, and eventually cause error.
> 
> Thus add two helpers arch_is_topdown_slots()/arch_is_topdown_metrics()
> to detect whether an event is topdown slots/metrics event by comparing
> the event config directly, and use these two helpers to replace the
> original event name comparisons.

Looks ok, I made a comment below, please take a look
 
> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
> ---
>  tools/perf/arch/x86/util/evlist.c  |  8 ++---
>  tools/perf/arch/x86/util/evsel.c   |  3 +-
>  tools/perf/arch/x86/util/topdown.c | 48 +++++++++++++++++++++++++++++-
>  tools/perf/arch/x86/util/topdown.h |  2 ++
>  4 files changed, 55 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/perf/arch/x86/util/evlist.c b/tools/perf/arch/x86/util/evlist.c
> index b1ce0c52d88d..332e8907f43e 100644
> --- a/tools/perf/arch/x86/util/evlist.c
> +++ b/tools/perf/arch/x86/util/evlist.c
> @@ -78,14 +78,14 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
>  	if (topdown_sys_has_perf_metrics() &&
>  	    (arch_evsel__must_be_in_group(lhs) || arch_evsel__must_be_in_group(rhs))) {
>  		/* Ensure the topdown slots comes first. */
> -		if (strcasestr(lhs->name, "slots") && !strcasestr(lhs->name, "uops_retired.slots"))
> +		if (arch_is_topdown_slots(lhs))
>  			return -1;
> -		if (strcasestr(rhs->name, "slots") && !strcasestr(rhs->name, "uops_retired.slots"))
> +		if (arch_is_topdown_slots(rhs))
>  			return 1;
>  		/* Followed by topdown events. */
> -		if (strcasestr(lhs->name, "topdown") && !strcasestr(rhs->name, "topdown"))
> +		if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
>  			return -1;
> -		if (!strcasestr(lhs->name, "topdown") && strcasestr(rhs->name, "topdown"))
> +		if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs))
>  			return 1;
>  	}
>  
> diff --git a/tools/perf/arch/x86/util/evsel.c b/tools/perf/arch/x86/util/evsel.c
> index 090d0f371891..181f2ba0bb2a 100644
> --- a/tools/perf/arch/x86/util/evsel.c
> +++ b/tools/perf/arch/x86/util/evsel.c
> @@ -6,6 +6,7 @@
>  #include "util/pmu.h"
>  #include "util/pmus.h"
>  #include "linux/string.h"
> +#include "topdown.h"
>  #include "evsel.h"
>  #include "util/debug.h"
>  #include "env.h"
> @@ -44,7 +45,7 @@ bool arch_evsel__must_be_in_group(const struct evsel *evsel)
>  	    strcasestr(evsel->name, "uops_retired.slots"))
>  		return false;
>  
> -	return strcasestr(evsel->name, "topdown") || strcasestr(evsel->name, "slots");
> +	return arch_is_topdown_metrics(evsel) || arch_is_topdown_slots(evsel);
>  }
>  
>  int arch_evsel__hw_name(struct evsel *evsel, char *bf, size_t size)
> diff --git a/tools/perf/arch/x86/util/topdown.c b/tools/perf/arch/x86/util/topdown.c
> index 3f9a267d4501..49f25d67ed77 100644
> --- a/tools/perf/arch/x86/util/topdown.c
> +++ b/tools/perf/arch/x86/util/topdown.c
> @@ -32,6 +32,52 @@ bool topdown_sys_has_perf_metrics(void)
>  }
>  
>  #define TOPDOWN_SLOTS		0x0400
> +bool arch_is_topdown_slots(const struct evsel *evsel)
> +{
> +	if (evsel->core.attr.config == TOPDOWN_SLOTS)
> +		return true;
> +
> +	return false;
> +}
> +
> +static int compare_topdown_event(void *vstate, struct pmu_event_info *info)
> +{
> +	int *config = vstate;
> +	int event = 0;
> +	int umask = 0;
> +	char *str;
> +
> +	if (!strcasestr(info->name, "topdown"))
> +		return 0;
> +
> +	str = strcasestr(info->str, "event=");
> +	if (str)
> +		sscanf(str, "event=%x", &event);
> +
> +	str = strcasestr(info->str, "umask=");
> +	if (str)
> +		sscanf(str, "umask=%x", &umask);
> +
> +	if (event == 0 && *config == (event | umask << 8))
> +		return 1;
> +
> +	return 0;
> +}
> +
> +bool arch_is_topdown_metrics(const struct evsel *evsel)
> +{
> +	struct perf_pmu *pmu = evsel__find_pmu(evsel);
> +	int config = evsel->core.attr.config;

Humm, can we cache this information? I.e. have some evsel->is_topdown:1
bit to avoid having to traverse all events if we call this multiple
times for the same evsel? 

- Arnaldo

> +	if (!pmu || !pmu->is_core)
> +		return false;
> +
> +	if (perf_pmu__for_each_event(pmu, false, &config,
> +				     compare_topdown_event))
> +		return true;
> +
> +	return false;
> +}
>  
>  /*
>   * Check whether a topdown group supports sample-read.
> @@ -44,7 +90,7 @@ bool arch_topdown_sample_read(struct evsel *leader)
>  	if (!evsel__sys_has_perf_metrics(leader))
>  		return false;
>  
> -	if (leader->core.attr.config == TOPDOWN_SLOTS)
> +	if (arch_is_topdown_slots(leader))
>  		return true;
>  
>  	return false;
> diff --git a/tools/perf/arch/x86/util/topdown.h b/tools/perf/arch/x86/util/topdown.h
> index 46bf9273e572..1bae9b1822d7 100644
> --- a/tools/perf/arch/x86/util/topdown.h
> +++ b/tools/perf/arch/x86/util/topdown.h
> @@ -3,5 +3,7 @@
>  #define _TOPDOWN_H 1
>  
>  bool topdown_sys_has_perf_metrics(void);
> +bool arch_is_topdown_slots(const struct evsel *evsel);
> +bool arch_is_topdown_metrics(const struct evsel *evsel);
>  
>  #endif
> -- 
> 2.40.1
> 

Re: [Patch v3 2/5] perf x86/topdown: Correct leader selection with sample_read enabled

[Arnaldo Carvalho de Melo]

On Fri, Jul 12, 2024 at 05:03:36PM +0000, Dapeng Mi wrote:
> Addresses an issue where, in the absence of a topdown metrics event
> within a sampling group, the slots event was incorrectly bypassed as
> the sampling leader when sample_read was enabled.
> 
> perf record -e '{slots,branches}:S' -c 10000 -vv sleep 1

Kan, can you please take a look at this patch and the others in this
series?

Thanks,

- Arnaldo
 
> In this case, the slots event should be sampled as leader but the
> branches event is sampled in fact like the verbose output shows.
> 
> perf_event_attr:
>   type                             4 (cpu)
>   size                             168
>   config                           0x400 (slots)
>   sample_type                      IP|TID|TIME|READ|CPU|IDENTIFIER
>   read_format                      ID|GROUP|LOST
>   disabled                         1
>   sample_id_all                    1
>   exclude_guest                    1
> ------------------------------------------------------------
> sys_perf_event_open: pid -1  cpu 0  group_fd -1  flags 0x8 = 5
> ------------------------------------------------------------
> perf_event_attr:
>   type                             0 (PERF_TYPE_HARDWARE)
>   size                             168
>   config                           0x4 (PERF_COUNT_HW_BRANCH_INSTRUCTIONS)
>   { sample_period, sample_freq }   10000
>   sample_type                      IP|TID|TIME|READ|CPU|IDENTIFIER
>   read_format                      ID|GROUP|LOST
>   sample_id_all                    1
>   exclude_guest                    1
> 
> The sample period of slots event instead of branches event is reset to
> 0.
> 
> This fix ensures the slots event remains the leader under these
> conditions.
> 
> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
> ---
>  tools/perf/arch/x86/util/topdown.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/arch/x86/util/topdown.c b/tools/perf/arch/x86/util/topdown.c
> index 49f25d67ed77..857e00cf579f 100644
> --- a/tools/perf/arch/x86/util/topdown.c
> +++ b/tools/perf/arch/x86/util/topdown.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include "api/fs/fs.h"
>  #include "util/evsel.h"
> +#include "util/evlist.h"
>  #include "util/pmu.h"
>  #include "util/pmus.h"
>  #include "util/topdown.h"
> @@ -87,11 +88,22 @@ bool arch_is_topdown_metrics(const struct evsel *evsel)
>   */
>  bool arch_topdown_sample_read(struct evsel *leader)
>  {
> +	struct evsel *evsel;
> +
>  	if (!evsel__sys_has_perf_metrics(leader))
>  		return false;
>  
> -	if (arch_is_topdown_slots(leader))
> -		return true;
> +	if (!arch_is_topdown_slots(leader))
> +		return false;
> +
> +	/*
> +	 * If slots event as leader event but no topdown metric events
> +	 * in group, slots event should still sample as leader.
> +	 */
> +	evlist__for_each_entry(leader->evlist, evsel) {
> +		if (evsel != leader && arch_is_topdown_metrics(evsel))
> +			return true;
> +	}
>  
>  	return false;
>  }
> -- 
> 2.40.1

Re: [Patch v3 2/5] perf x86/topdown: Correct leader selection with sample_read enabled

[Liang, Kan]

On 2024-08-12 9:42 a.m., Arnaldo Carvalho de Melo wrote:
> On Fri, Jul 12, 2024 at 05:03:36PM +0000, Dapeng Mi wrote:
>> Addresses an issue where, in the absence of a topdown metrics event
>> within a sampling group, the slots event was incorrectly bypassed as
>> the sampling leader when sample_read was enabled.
>>
>> perf record -e '{slots,branches}:S' -c 10000 -vv sleep 1
> 
> Kan, can you please take a look at this patch and the others in this
> series?

Sure.

Thanks,
Kan

> 
> Thanks,
> 
> - Arnaldo
>  
>> In this case, the slots event should be sampled as leader but the
>> branches event is sampled in fact like the verbose output shows.
>>
>> perf_event_attr:
>>   type                             4 (cpu)
>>   size                             168
>>   config                           0x400 (slots)
>>   sample_type                      IP|TID|TIME|READ|CPU|IDENTIFIER
>>   read_format                      ID|GROUP|LOST
>>   disabled                         1
>>   sample_id_all                    1
>>   exclude_guest                    1
>> ------------------------------------------------------------
>> sys_perf_event_open: pid -1  cpu 0  group_fd -1  flags 0x8 = 5
>> ------------------------------------------------------------
>> perf_event_attr:
>>   type                             0 (PERF_TYPE_HARDWARE)
>>   size                             168
>>   config                           0x4 (PERF_COUNT_HW_BRANCH_INSTRUCTIONS)
>>   { sample_period, sample_freq }   10000
>>   sample_type                      IP|TID|TIME|READ|CPU|IDENTIFIER
>>   read_format                      ID|GROUP|LOST
>>   sample_id_all                    1
>>   exclude_guest                    1
>>
>> The sample period of slots event instead of branches event is reset to
>> 0.
>>
>> This fix ensures the slots event remains the leader under these
>> conditions.
>>
>> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
>> ---
>>  tools/perf/arch/x86/util/topdown.c | 16 ++++++++++++++--
>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/perf/arch/x86/util/topdown.c b/tools/perf/arch/x86/util/topdown.c
>> index 49f25d67ed77..857e00cf579f 100644
>> --- a/tools/perf/arch/x86/util/topdown.c
>> +++ b/tools/perf/arch/x86/util/topdown.c
>> @@ -1,6 +1,7 @@
>>  // SPDX-License-Identifier: GPL-2.0
>>  #include "api/fs/fs.h"
>>  #include "util/evsel.h"
>> +#include "util/evlist.h"
>>  #include "util/pmu.h"
>>  #include "util/pmus.h"
>>  #include "util/topdown.h"
>> @@ -87,11 +88,22 @@ bool arch_is_topdown_metrics(const struct evsel *evsel)
>>   */
>>  bool arch_topdown_sample_read(struct evsel *leader)
>>  {
>> +	struct evsel *evsel;
>> +
>>  	if (!evsel__sys_has_perf_metrics(leader))
>>  		return false;
>>  
>> -	if (arch_is_topdown_slots(leader))
>> -		return true;
>> +	if (!arch_is_topdown_slots(leader))
>> +		return false;
>> +
>> +	/*
>> +	 * If slots event as leader event but no topdown metric events
>> +	 * in group, slots event should still sample as leader.
>> +	 */
>> +	evlist__for_each_entry(leader->evlist, evsel) {
>> +		if (evsel != leader && arch_is_topdown_metrics(evsel))
>> +			return true;
>> +	}
>>  
>>  	return false;
>>  }
>> -- 
>> 2.40.1
> 

Re: [Patch v3 2/5] perf x86/topdown: Correct leader selection with sample_read enabled

[Liang, Kan]

On 2024-07-12 1:03 p.m., Dapeng Mi wrote:
> Addresses an issue where, in the absence of a topdown metrics event
> within a sampling group, the slots event was incorrectly bypassed as
> the sampling leader when sample_read was enabled.
> 
> perf record -e '{slots,branches}:S' -c 10000 -vv sleep 1
> 
> In this case, the slots event should be sampled as leader but the
> branches event is sampled in fact like the verbose output shows.
> 
> perf_event_attr:
>   type                             4 (cpu)
>   size                             168
>   config                           0x400 (slots)
>   sample_type                      IP|TID|TIME|READ|CPU|IDENTIFIER
>   read_format                      ID|GROUP|LOST
>   disabled                         1
>   sample_id_all                    1
>   exclude_guest                    1
> ------------------------------------------------------------
> sys_perf_event_open: pid -1  cpu 0  group_fd -1  flags 0x8 = 5
> ------------------------------------------------------------
> perf_event_attr:
>   type                             0 (PERF_TYPE_HARDWARE)
>   size                             168
>   config                           0x4 (PERF_COUNT_HW_BRANCH_INSTRUCTIONS)
>   { sample_period, sample_freq }   10000
>   sample_type                      IP|TID|TIME|READ|CPU|IDENTIFIER
>   read_format                      ID|GROUP|LOST
>   sample_id_all                    1
>   exclude_guest                    1
> 
> The sample period of slots event instead of branches event is reset to
> 0.
> 
> This fix ensures the slots event remains the leader under these
> conditions.
> 
> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
> ---
>  tools/perf/arch/x86/util/topdown.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/arch/x86/util/topdown.c b/tools/perf/arch/x86/util/topdown.c
> index 49f25d67ed77..857e00cf579f 100644
> --- a/tools/perf/arch/x86/util/topdown.c
> +++ b/tools/perf/arch/x86/util/topdown.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include "api/fs/fs.h"
>  #include "util/evsel.h"
> +#include "util/evlist.h"
>  #include "util/pmu.h"
>  #include "util/pmus.h"
>  #include "util/topdown.h"
> @@ -87,11 +88,22 @@ bool arch_is_topdown_metrics(const struct evsel *evsel)
>   */
>  bool arch_topdown_sample_read(struct evsel *leader)
>  {
> +	struct evsel *evsel;
> +
>  	if (!evsel__sys_has_perf_metrics(leader))
>  		return false;
>  
> -	if (arch_is_topdown_slots(leader))
> -		return true;
> +	if (!arch_is_topdown_slots(leader))
> +		return false;
> +
> +	/*
> +	 * If slots event as leader event but no topdown metric events
> +	 * in group, slots event should still sample as leader.
> +	 */
> +	evlist__for_each_entry(leader->evlist, evsel) {
		evsel = leader->leader;
		evlist__for_each_entry_continue(leader->evlist, evsel)
			if (evsel->leader != leader->leader)
				return false;

Maybe we should limit the check in a group, rather than the entire
evlist. Something as above (not tested)?

Thanks,
Kan
> +		if (evsel != leader && arch_is_topdown_metrics(evsel))
> +			return true;
> +	}
>  
>  	return false;
>  }

Re: [Patch v3 3/5] perf x86/topdown: Don't move topdown metric events in group

[Liang, Kan]

On 2024-07-12 1:03 p.m., Dapeng Mi wrote:
> when running below perf command, we say error is reported.
> 
> perf record -e "{slots,instructions,topdown-retiring}:S" -vv -C0 sleep 1
> 
> ------------------------------------------------------------
> perf_event_attr:
>   type                             4 (cpu)
>   size                             168
>   config                           0x400 (slots)
>   sample_type                      IP|TID|TIME|READ|CPU|PERIOD|IDENTIFIER
>   read_format                      ID|GROUP|LOST
>   disabled                         1
>   sample_id_all                    1
>   exclude_guest                    1
> ------------------------------------------------------------
> sys_perf_event_open: pid -1  cpu 0  group_fd -1  flags 0x8 = 5
> ------------------------------------------------------------
> perf_event_attr:
>   type                             4 (cpu)
>   size                             168
>   config                           0x8000 (topdown-retiring)
>   { sample_period, sample_freq }   4000
>   sample_type                      IP|TID|TIME|READ|CPU|PERIOD|IDENTIFIER
>   read_format                      ID|GROUP|LOST
>   freq                             1
>   sample_id_all                    1
>   exclude_guest                    1
> ------------------------------------------------------------
> sys_perf_event_open: pid -1  cpu 0  group_fd 5  flags 0x8
> sys_perf_event_open failed, error -22
> 
> Error:
> The sys_perf_event_open() syscall returned with 22 (Invalid argument) for event (topdown-retiring).
> 
> The reason of error is that the events are regrouped and
> topdown-retiring event is moved to closely after the slots event and
> topdown-retiring event needs to do the sampling, but Intel PMU driver
> doesn't support to sample topdown metrics events.
> 
> For topdown metrics events, it just requires to be in a group which has
> slots event as leader. It doesn't require topdown metrics event must be
> closely after slots event. Thus it's a overkill to move topdown metrics
> event closely after slots event in events regrouping and furtherly cause
> the above issue.
> 
> Thus don't move topdown metrics events forward if they are already in a
> group.
> 
> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
> ---
>  tools/perf/arch/x86/util/evlist.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/arch/x86/util/evlist.c b/tools/perf/arch/x86/util/evlist.c
> index 332e8907f43e..6ae044f21843 100644
> --- a/tools/perf/arch/x86/util/evlist.c
> +++ b/tools/perf/arch/x86/util/evlist.c
> @@ -85,7 +85,12 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
>  		/* Followed by topdown events. */
>  		if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
>  			return -1;
> -		if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs))
> +		/*
> +		 * Move topdown events forward only when topdown events
> +		 * are not in same group with previous event.
> +		 */

Do you mean this case?

perf stat -e '{slots,branches},topdown-retiring' -C0 sleep 1
WARNING: events were regrouped to match PMUs

 Performance counter stats for 'CPU(s) 0':

        22,568,316      slots
           569,904      branches
         3,805,637      topdown-retiring


But if I add one more event before topdown-retiring, it seems break again.

perf stat -e '{slots,branches},cycles,topdown-retiring' -C0 sleep 1

 Performance counter stats for 'CPU(s) 0':

        25,218,108      slots
           647,598      branches
         4,345,121      cycles
   <not supported>      topdown-retiring

I'm not asking to support all the above cases. I just try to understand
which cases you plan to support.

Can you please add some comments or update the document to clearly show
which format is supported, which format will be automatically adjusted
by the tool, and which format will be error out?

We should also need test cases for all the supported formats, not just
the standard one.

Thanks,
Kan

> +		if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs) &&
> +		    lhs->core.leader != rhs->core.leader)
>  			return 1;
>  	}
>  

Re: [Patch v3 1/5] perf x86/topdown: Complete topdown slots/metrics events check

[Mi, Dapeng]

On 8/12/2024 9:41 PM, Arnaldo Carvalho de Melo wrote:
> On Fri, Jul 12, 2024 at 05:03:35PM +0000, Dapeng Mi wrote:
>> It's not complete to check whether an event is a topdown slots or
>> topdown metrics event by only comparing the event name since user
>> may assign the event by RAW format, e.g.
>>
>> perf stat -e '{instructions,cpu/r400/,cpu/r8300/}' sleep 1
>>
>>  Performance counter stats for 'sleep 1':
>>
>>      <not counted>      instructions
>>      <not counted>      cpu/r400/
>>    <not supported>      cpu/r8300/
>>
>>        1.002917796 seconds time elapsed
>>
>>        0.002955000 seconds user
>>        0.000000000 seconds sys
>>
>> The RAW format slots and topdown-be-bound events are not recognized and
>> not regroup the events, and eventually cause error.
>>
>> Thus add two helpers arch_is_topdown_slots()/arch_is_topdown_metrics()
>> to detect whether an event is topdown slots/metrics event by comparing
>> the event config directly, and use these two helpers to replace the
>> original event name comparisons.
> Looks ok, I made a comment below, please take a look
>  
>> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
>> ---
>>  tools/perf/arch/x86/util/evlist.c  |  8 ++---
>>  tools/perf/arch/x86/util/evsel.c   |  3 +-
>>  tools/perf/arch/x86/util/topdown.c | 48 +++++++++++++++++++++++++++++-
>>  tools/perf/arch/x86/util/topdown.h |  2 ++
>>  4 files changed, 55 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/perf/arch/x86/util/evlist.c b/tools/perf/arch/x86/util/evlist.c
>> index b1ce0c52d88d..332e8907f43e 100644
>> --- a/tools/perf/arch/x86/util/evlist.c
>> +++ b/tools/perf/arch/x86/util/evlist.c
>> @@ -78,14 +78,14 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
>>  	if (topdown_sys_has_perf_metrics() &&
>>  	    (arch_evsel__must_be_in_group(lhs) || arch_evsel__must_be_in_group(rhs))) {
>>  		/* Ensure the topdown slots comes first. */
>> -		if (strcasestr(lhs->name, "slots") && !strcasestr(lhs->name, "uops_retired.slots"))
>> +		if (arch_is_topdown_slots(lhs))
>>  			return -1;
>> -		if (strcasestr(rhs->name, "slots") && !strcasestr(rhs->name, "uops_retired.slots"))
>> +		if (arch_is_topdown_slots(rhs))
>>  			return 1;
>>  		/* Followed by topdown events. */
>> -		if (strcasestr(lhs->name, "topdown") && !strcasestr(rhs->name, "topdown"))
>> +		if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
>>  			return -1;
>> -		if (!strcasestr(lhs->name, "topdown") && strcasestr(rhs->name, "topdown"))
>> +		if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs))
>>  			return 1;
>>  	}
>>  
>> diff --git a/tools/perf/arch/x86/util/evsel.c b/tools/perf/arch/x86/util/evsel.c
>> index 090d0f371891..181f2ba0bb2a 100644
>> --- a/tools/perf/arch/x86/util/evsel.c
>> +++ b/tools/perf/arch/x86/util/evsel.c
>> @@ -6,6 +6,7 @@
>>  #include "util/pmu.h"
>>  #include "util/pmus.h"
>>  #include "linux/string.h"
>> +#include "topdown.h"
>>  #include "evsel.h"
>>  #include "util/debug.h"
>>  #include "env.h"
>> @@ -44,7 +45,7 @@ bool arch_evsel__must_be_in_group(const struct evsel *evsel)
>>  	    strcasestr(evsel->name, "uops_retired.slots"))
>>  		return false;
>>  
>> -	return strcasestr(evsel->name, "topdown") || strcasestr(evsel->name, "slots");
>> +	return arch_is_topdown_metrics(evsel) || arch_is_topdown_slots(evsel);
>>  }
>>  
>>  int arch_evsel__hw_name(struct evsel *evsel, char *bf, size_t size)
>> diff --git a/tools/perf/arch/x86/util/topdown.c b/tools/perf/arch/x86/util/topdown.c
>> index 3f9a267d4501..49f25d67ed77 100644
>> --- a/tools/perf/arch/x86/util/topdown.c
>> +++ b/tools/perf/arch/x86/util/topdown.c
>> @@ -32,6 +32,52 @@ bool topdown_sys_has_perf_metrics(void)
>>  }
>>  
>>  #define TOPDOWN_SLOTS		0x0400
>> +bool arch_is_topdown_slots(const struct evsel *evsel)
>> +{
>> +	if (evsel->core.attr.config == TOPDOWN_SLOTS)
>> +		return true;
>> +
>> +	return false;
>> +}
>> +
>> +static int compare_topdown_event(void *vstate, struct pmu_event_info *info)
>> +{
>> +	int *config = vstate;
>> +	int event = 0;
>> +	int umask = 0;
>> +	char *str;
>> +
>> +	if (!strcasestr(info->name, "topdown"))
>> +		return 0;
>> +
>> +	str = strcasestr(info->str, "event=");
>> +	if (str)
>> +		sscanf(str, "event=%x", &event);
>> +
>> +	str = strcasestr(info->str, "umask=");
>> +	if (str)
>> +		sscanf(str, "umask=%x", &umask);
>> +
>> +	if (event == 0 && *config == (event | umask << 8))
>> +		return 1;
>> +
>> +	return 0;
>> +}
>> +
>> +bool arch_is_topdown_metrics(const struct evsel *evsel)
>> +{
>> +	struct perf_pmu *pmu = evsel__find_pmu(evsel);
>> +	int config = evsel->core.attr.config;
> Humm, can we cache this information? I.e. have some evsel->is_topdown:1
> bit to avoid having to traverse all events if we call this multiple
> times for the same evsel? 

Yeah, good point. Thanks.


>
> - Arnaldo
>
>> +	if (!pmu || !pmu->is_core)
>> +		return false;
>> +
>> +	if (perf_pmu__for_each_event(pmu, false, &config,
>> +				     compare_topdown_event))
>> +		return true;
>> +
>> +	return false;
>> +}
>>  
>>  /*
>>   * Check whether a topdown group supports sample-read.
>> @@ -44,7 +90,7 @@ bool arch_topdown_sample_read(struct evsel *leader)
>>  	if (!evsel__sys_has_perf_metrics(leader))
>>  		return false;
>>  
>> -	if (leader->core.attr.config == TOPDOWN_SLOTS)
>> +	if (arch_is_topdown_slots(leader))
>>  		return true;
>>  
>>  	return false;
>> diff --git a/tools/perf/arch/x86/util/topdown.h b/tools/perf/arch/x86/util/topdown.h
>> index 46bf9273e572..1bae9b1822d7 100644
>> --- a/tools/perf/arch/x86/util/topdown.h
>> +++ b/tools/perf/arch/x86/util/topdown.h
>> @@ -3,5 +3,7 @@
>>  #define _TOPDOWN_H 1
>>  
>>  bool topdown_sys_has_perf_metrics(void);
>> +bool arch_is_topdown_slots(const struct evsel *evsel);
>> +bool arch_is_topdown_metrics(const struct evsel *evsel);
>>  
>>  #endif
>> -- 
>> 2.40.1
>>

Re: [Patch v3 2/5] perf x86/topdown: Correct leader selection with sample_read enabled

[Mi, Dapeng]

On 8/12/2024 11:18 PM, Liang, Kan wrote:
>
> On 2024-07-12 1:03 p.m., Dapeng Mi wrote:
>> Addresses an issue where, in the absence of a topdown metrics event
>> within a sampling group, the slots event was incorrectly bypassed as
>> the sampling leader when sample_read was enabled.
>>
>> perf record -e '{slots,branches}:S' -c 10000 -vv sleep 1
>>
>> In this case, the slots event should be sampled as leader but the
>> branches event is sampled in fact like the verbose output shows.
>>
>> perf_event_attr:
>>   type                             4 (cpu)
>>   size                             168
>>   config                           0x400 (slots)
>>   sample_type                      IP|TID|TIME|READ|CPU|IDENTIFIER
>>   read_format                      ID|GROUP|LOST
>>   disabled                         1
>>   sample_id_all                    1
>>   exclude_guest                    1
>> ------------------------------------------------------------
>> sys_perf_event_open: pid -1  cpu 0  group_fd -1  flags 0x8 = 5
>> ------------------------------------------------------------
>> perf_event_attr:
>>   type                             0 (PERF_TYPE_HARDWARE)
>>   size                             168
>>   config                           0x4 (PERF_COUNT_HW_BRANCH_INSTRUCTIONS)
>>   { sample_period, sample_freq }   10000
>>   sample_type                      IP|TID|TIME|READ|CPU|IDENTIFIER
>>   read_format                      ID|GROUP|LOST
>>   sample_id_all                    1
>>   exclude_guest                    1
>>
>> The sample period of slots event instead of branches event is reset to
>> 0.
>>
>> This fix ensures the slots event remains the leader under these
>> conditions.
>>
>> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
>> ---
>>  tools/perf/arch/x86/util/topdown.c | 16 ++++++++++++++--
>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/perf/arch/x86/util/topdown.c b/tools/perf/arch/x86/util/topdown.c
>> index 49f25d67ed77..857e00cf579f 100644
>> --- a/tools/perf/arch/x86/util/topdown.c
>> +++ b/tools/perf/arch/x86/util/topdown.c
>> @@ -1,6 +1,7 @@
>>  // SPDX-License-Identifier: GPL-2.0
>>  #include "api/fs/fs.h"
>>  #include "util/evsel.h"
>> +#include "util/evlist.h"
>>  #include "util/pmu.h"
>>  #include "util/pmus.h"
>>  #include "util/topdown.h"
>> @@ -87,11 +88,22 @@ bool arch_is_topdown_metrics(const struct evsel *evsel)
>>   */
>>  bool arch_topdown_sample_read(struct evsel *leader)
>>  {
>> +	struct evsel *evsel;
>> +
>>  	if (!evsel__sys_has_perf_metrics(leader))
>>  		return false;
>>  
>> -	if (arch_is_topdown_slots(leader))
>> -		return true;
>> +	if (!arch_is_topdown_slots(leader))
>> +		return false;
>> +
>> +	/*
>> +	 * If slots event as leader event but no topdown metric events
>> +	 * in group, slots event should still sample as leader.
>> +	 */
>> +	evlist__for_each_entry(leader->evlist, evsel) {
> 		evsel = leader->leader;
> 		evlist__for_each_entry_continue(leader->evlist, evsel)
> 			if (evsel->leader != leader->leader)
> 				return false;
>
> Maybe we should limit the check in a group, rather than the entire
> evlist. Something as above (not tested)?

Good point. Would limit the check in the group.


>
> Thanks,
> Kan
>> +		if (evsel != leader && arch_is_topdown_metrics(evsel))
>> +			return true;
>> +	}
>>  
>>  	return false;
>>  }

Re: [Patch v3 3/5] perf x86/topdown: Don't move topdown metric events in group

[Mi, Dapeng]

On 8/12/2024 11:37 PM, Liang, Kan wrote:
>
> On 2024-07-12 1:03 p.m., Dapeng Mi wrote:
>> when running below perf command, we say error is reported.
>>
>> perf record -e "{slots,instructions,topdown-retiring}:S" -vv -C0 sleep 1
>>
>> ------------------------------------------------------------
>> perf_event_attr:
>>   type                             4 (cpu)
>>   size                             168
>>   config                           0x400 (slots)
>>   sample_type                      IP|TID|TIME|READ|CPU|PERIOD|IDENTIFIER
>>   read_format                      ID|GROUP|LOST
>>   disabled                         1
>>   sample_id_all                    1
>>   exclude_guest                    1
>> ------------------------------------------------------------
>> sys_perf_event_open: pid -1  cpu 0  group_fd -1  flags 0x8 = 5
>> ------------------------------------------------------------
>> perf_event_attr:
>>   type                             4 (cpu)
>>   size                             168
>>   config                           0x8000 (topdown-retiring)
>>   { sample_period, sample_freq }   4000
>>   sample_type                      IP|TID|TIME|READ|CPU|PERIOD|IDENTIFIER
>>   read_format                      ID|GROUP|LOST
>>   freq                             1
>>   sample_id_all                    1
>>   exclude_guest                    1
>> ------------------------------------------------------------
>> sys_perf_event_open: pid -1  cpu 0  group_fd 5  flags 0x8
>> sys_perf_event_open failed, error -22
>>
>> Error:
>> The sys_perf_event_open() syscall returned with 22 (Invalid argument) for event (topdown-retiring).
>>
>> The reason of error is that the events are regrouped and
>> topdown-retiring event is moved to closely after the slots event and
>> topdown-retiring event needs to do the sampling, but Intel PMU driver
>> doesn't support to sample topdown metrics events.
>>
>> For topdown metrics events, it just requires to be in a group which has
>> slots event as leader. It doesn't require topdown metrics event must be
>> closely after slots event. Thus it's a overkill to move topdown metrics
>> event closely after slots event in events regrouping and furtherly cause
>> the above issue.
>>
>> Thus don't move topdown metrics events forward if they are already in a
>> group.
>>
>> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
>> ---
>>  tools/perf/arch/x86/util/evlist.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/arch/x86/util/evlist.c b/tools/perf/arch/x86/util/evlist.c
>> index 332e8907f43e..6ae044f21843 100644
>> --- a/tools/perf/arch/x86/util/evlist.c
>> +++ b/tools/perf/arch/x86/util/evlist.c
>> @@ -85,7 +85,12 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
>>  		/* Followed by topdown events. */
>>  		if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
>>  			return -1;
>> -		if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs))
>> +		/*
>> +		 * Move topdown events forward only when topdown events
>> +		 * are not in same group with previous event.
>> +		 */
> Do you mean this case?
>
> perf stat -e '{slots,branches},topdown-retiring' -C0 sleep 1
> WARNING: events were regrouped to match PMUs
>
>  Performance counter stats for 'CPU(s) 0':
>
>         22,568,316      slots
>            569,904      branches
>          3,805,637      topdown-retiring

Yes, this case can be regrouped.


>
> But if I add one more event before topdown-retiring, it seems break again.
>
> perf stat -e '{slots,branches},cycles,topdown-retiring' -C0 sleep 1
>
>  Performance counter stats for 'CPU(s) 0':
>
>         25,218,108      slots
>            647,598      branches
>          4,345,121      cycles
>    <not supported>      topdown-retiring

Yes, this case can't be supported by original code. I ever tried to support
this format, but it's not easy, it needs to fully change current sort logic.


>
> I'm not asking to support all the above cases. I just try to understand
> which cases you plan to support.
>
> Can you please add some comments or update the document to clearly show
> which format is supported, which format will be automatically adjusted
> by the tool, and which format will be error out?

Yeah, I would list all currently supported regrouping format. BTW, is there
a document to describe the topdown metrics feaeture. If not, I would add
comments here.


>
> We should also need test cases for all the supported formats, not just
> the standard one.

Sure. thanks.


>
> Thanks,
> Kan
>
>> +		if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs) &&
>> +		    lhs->core.leader != rhs->core.leader)
>>  			return 1;
>>  	}
>>  

Re: [Patch v3 1/5] perf x86/topdown: Complete topdown slots/metrics events check

[Mi, Dapeng]

On 8/12/2024 9:41 PM, Arnaldo Carvalho de Melo wrote:
> On Fri, Jul 12, 2024 at 05:03:35PM +0000, Dapeng Mi wrote:
>> It's not complete to check whether an event is a topdown slots or
>> topdown metrics event by only comparing the event name since user
>> may assign the event by RAW format, e.g.
>>
>> perf stat -e '{instructions,cpu/r400/,cpu/r8300/}' sleep 1
>>
>>  Performance counter stats for 'sleep 1':
>>
>>      <not counted>      instructions
>>      <not counted>      cpu/r400/
>>    <not supported>      cpu/r8300/
>>
>>        1.002917796 seconds time elapsed
>>
>>        0.002955000 seconds user
>>        0.000000000 seconds sys
>>
>> The RAW format slots and topdown-be-bound events are not recognized and
>> not regroup the events, and eventually cause error.
>>
>> Thus add two helpers arch_is_topdown_slots()/arch_is_topdown_metrics()
>> to detect whether an event is topdown slots/metrics event by comparing
>> the event config directly, and use these two helpers to replace the
>> original event name comparisons.
> Looks ok, I made a comment below, please take a look
>  
>> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
>> ---
>>  tools/perf/arch/x86/util/evlist.c  |  8 ++---
>>  tools/perf/arch/x86/util/evsel.c   |  3 +-
>>  tools/perf/arch/x86/util/topdown.c | 48 +++++++++++++++++++++++++++++-
>>  tools/perf/arch/x86/util/topdown.h |  2 ++
>>  4 files changed, 55 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/perf/arch/x86/util/evlist.c b/tools/perf/arch/x86/util/evlist.c
>> index b1ce0c52d88d..332e8907f43e 100644
>> --- a/tools/perf/arch/x86/util/evlist.c
>> +++ b/tools/perf/arch/x86/util/evlist.c
>> @@ -78,14 +78,14 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
>>  	if (topdown_sys_has_perf_metrics() &&
>>  	    (arch_evsel__must_be_in_group(lhs) || arch_evsel__must_be_in_group(rhs))) {
>>  		/* Ensure the topdown slots comes first. */
>> -		if (strcasestr(lhs->name, "slots") && !strcasestr(lhs->name, "uops_retired.slots"))
>> +		if (arch_is_topdown_slots(lhs))
>>  			return -1;
>> -		if (strcasestr(rhs->name, "slots") && !strcasestr(rhs->name, "uops_retired.slots"))
>> +		if (arch_is_topdown_slots(rhs))
>>  			return 1;
>>  		/* Followed by topdown events. */
>> -		if (strcasestr(lhs->name, "topdown") && !strcasestr(rhs->name, "topdown"))
>> +		if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
>>  			return -1;
>> -		if (!strcasestr(lhs->name, "topdown") && strcasestr(rhs->name, "topdown"))
>> +		if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs))
>>  			return 1;
>>  	}
>>  
>> diff --git a/tools/perf/arch/x86/util/evsel.c b/tools/perf/arch/x86/util/evsel.c
>> index 090d0f371891..181f2ba0bb2a 100644
>> --- a/tools/perf/arch/x86/util/evsel.c
>> +++ b/tools/perf/arch/x86/util/evsel.c
>> @@ -6,6 +6,7 @@
>>  #include "util/pmu.h"
>>  #include "util/pmus.h"
>>  #include "linux/string.h"
>> +#include "topdown.h"
>>  #include "evsel.h"
>>  #include "util/debug.h"
>>  #include "env.h"
>> @@ -44,7 +45,7 @@ bool arch_evsel__must_be_in_group(const struct evsel *evsel)
>>  	    strcasestr(evsel->name, "uops_retired.slots"))
>>  		return false;
>>  
>> -	return strcasestr(evsel->name, "topdown") || strcasestr(evsel->name, "slots");
>> +	return arch_is_topdown_metrics(evsel) || arch_is_topdown_slots(evsel);
>>  }
>>  
>>  int arch_evsel__hw_name(struct evsel *evsel, char *bf, size_t size)
>> diff --git a/tools/perf/arch/x86/util/topdown.c b/tools/perf/arch/x86/util/topdown.c
>> index 3f9a267d4501..49f25d67ed77 100644
>> --- a/tools/perf/arch/x86/util/topdown.c
>> +++ b/tools/perf/arch/x86/util/topdown.c
>> @@ -32,6 +32,52 @@ bool topdown_sys_has_perf_metrics(void)
>>  }
>>  
>>  #define TOPDOWN_SLOTS		0x0400
>> +bool arch_is_topdown_slots(const struct evsel *evsel)
>> +{
>> +	if (evsel->core.attr.config == TOPDOWN_SLOTS)
>> +		return true;
>> +
>> +	return false;
>> +}
>> +
>> +static int compare_topdown_event(void *vstate, struct pmu_event_info *info)
>> +{
>> +	int *config = vstate;
>> +	int event = 0;
>> +	int umask = 0;
>> +	char *str;
>> +
>> +	if (!strcasestr(info->name, "topdown"))
>> +		return 0;
>> +
>> +	str = strcasestr(info->str, "event=");
>> +	if (str)
>> +		sscanf(str, "event=%x", &event);
>> +
>> +	str = strcasestr(info->str, "umask=");
>> +	if (str)
>> +		sscanf(str, "umask=%x", &umask);
>> +
>> +	if (event == 0 && *config == (event | umask << 8))
>> +		return 1;
>> +
>> +	return 0;
>> +}
>> +
>> +bool arch_is_topdown_metrics(const struct evsel *evsel)
>> +{
>> +	struct perf_pmu *pmu = evsel__find_pmu(evsel);
>> +	int config = evsel->core.attr.config;
> Humm, can we cache this information? I.e. have some evsel->is_topdown:1
> bit to avoid having to traverse all events if we call this multiple
> times for the same evsel? 

Arnaldo, I tried to add a variable in evsel structure to represent if the
evsel is a topdown metrics event, but unfortunately the helper argument
"evsel" has "const" qualifier, it forces the "evsel" structure can't be
modified. If we want to add this optimization, the entire call-chain
functions have to be modified and this would reduce the security of code as
well. Since this helper is not in a performance critical patch, I suppose
it won't introduce too much performance hit as the traverse. I would drop
this optimization and keep the original code.


>
> - Arnaldo
>
>> +	if (!pmu || !pmu->is_core)
>> +		return false;
>> +
>> +	if (perf_pmu__for_each_event(pmu, false, &config,
>> +				     compare_topdown_event))
>> +		return true;
>> +
>> +	return false;
>> +}
>>  
>>  /*
>>   * Check whether a topdown group supports sample-read.
>> @@ -44,7 +90,7 @@ bool arch_topdown_sample_read(struct evsel *leader)
>>  	if (!evsel__sys_has_perf_metrics(leader))
>>  		return false;
>>  
>> -	if (leader->core.attr.config == TOPDOWN_SLOTS)
>> +	if (arch_is_topdown_slots(leader))
>>  		return true;
>>  
>>  	return false;
>> diff --git a/tools/perf/arch/x86/util/topdown.h b/tools/perf/arch/x86/util/topdown.h
>> index 46bf9273e572..1bae9b1822d7 100644
>> --- a/tools/perf/arch/x86/util/topdown.h
>> +++ b/tools/perf/arch/x86/util/topdown.h
>> @@ -3,5 +3,7 @@
>>  #define _TOPDOWN_H 1
>>  
>>  bool topdown_sys_has_perf_metrics(void);
>> +bool arch_is_topdown_slots(const struct evsel *evsel);
>> +bool arch_is_topdown_metrics(const struct evsel *evsel);
>>  
>>  #endif
>> -- 
>> 2.40.1
>>