[PATCH 1/2] perf evlist: Keep topdown counters in weak group

[PATCH 1/2] perf evlist: Keep topdown counters in weak group

[Ian Rogers]

On Intel Icelake, topdown events must always be grouped with a slots
event as leader. When a metric is parsed a weak group is formed and
retried if perf_event_open fails. The retried events aren't grouped
breaking the slots leader requirement. This change modifies the weak
group "reset" behavior so that topdown events aren't broken from the
group for the retry.

$ perf stat -e '{slots,topdown-bad-spec,topdown-be-bound,topdown-fe-bound,topdown-retiring,branch-instructions,branch-misses,bus-cycles,cache-misses,cache-references,cpu-cycles,instructions,mem-loads,mem-stores,ref-cycles,baclears.any,ARITH.DIVIDER_ACTIVE}:W' -a sleep 1

 Performance counter stats for 'system wide':

    47,867,188,483      slots                                                         (92.27%)
   <not supported>      topdown-bad-spec
   <not supported>      topdown-be-bound
   <not supported>      topdown-fe-bound
   <not supported>      topdown-retiring
     2,173,346,937      branch-instructions                                           (92.27%)
        10,540,253      branch-misses             #    0.48% of all branches          (92.29%)
        96,291,140      bus-cycles                                                    (92.29%)
         6,214,202      cache-misses              #   20.120 % of all cache refs      (92.29%)
        30,886,082      cache-references                                              (76.91%)
    11,773,726,641      cpu-cycles                                                    (84.62%)
    11,807,585,307      instructions              #    1.00  insn per cycle           (92.31%)
                 0      mem-loads                                                     (92.32%)
     2,212,928,573      mem-stores                                                    (84.69%)
    10,024,403,118      ref-cycles                                                    (92.35%)
        16,232,978      baclears.any                                                  (92.35%)
        23,832,633      ARITH.DIVIDER_ACTIVE                                          (84.59%)

       0.981070734 seconds time elapsed

After:

$ perf stat -e '{slots,topdown-bad-spec,topdown-be-bound,topdown-fe-bound,topdown-retiring,branch-instructions,branch-misses,bus-cycles,cache-misses,cache-references,cpu-cycles,instructions,mem-loads,mem-stores,ref-cycles,baclears.any,ARITH.DIVIDER_ACTIVE}:W' -a sleep 1

 Performance counter stats for 'system wide':

       31040189283      slots                                                         (92.27%)
        8997514811      topdown-bad-spec          #     28.2% bad speculation         (92.27%)
       10997536028      topdown-be-bound          #     34.5% backend bound           (92.27%)
        4778060526      topdown-fe-bound          #     15.0% frontend bound          (92.27%)
        7086628768      topdown-retiring          #     22.2% retiring                (92.27%)
        1417611942      branch-instructions                                           (92.26%)
           5285529      branch-misses             #    0.37% of all branches          (92.28%)
          62922469      bus-cycles                                                    (92.29%)
           1440708      cache-misses              #    8.292 % of all cache refs      (92.30%)
          17374098      cache-references                                              (76.94%)
        8040889520      cpu-cycles                                                    (84.63%)
        7709992319      instructions              #    0.96  insn per cycle           (92.32%)
                 0      mem-loads                                                     (92.32%)
        1515669558      mem-stores                                                    (84.68%)
        6542411177      ref-cycles                                                    (92.35%)
           4154149      baclears.any                                                  (92.35%)
          20556152      ARITH.DIVIDER_ACTIVE                                          (84.59%)

       1.010799593 seconds time elapsed

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/arch/x86/util/evsel.c | 12 ++++++++++++
 tools/perf/util/evlist.c         | 16 ++++++++++++++--
 tools/perf/util/evsel.c          | 10 ++++++++++
 tools/perf/util/evsel.h          |  3 +++
 4 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/tools/perf/arch/x86/util/evsel.c b/tools/perf/arch/x86/util/evsel.c
index ac2899a25b7a..40b171de2086 100644
--- a/tools/perf/arch/x86/util/evsel.c
+++ b/tools/perf/arch/x86/util/evsel.c
@@ -3,6 +3,7 @@
 #include <stdlib.h>
 #include "util/evsel.h"
 #include "util/env.h"
+#include "util/pmu.h"
 #include "linux/string.h"
 
 void arch_evsel__set_sample_weight(struct evsel *evsel)
@@ -29,3 +30,14 @@ void arch_evsel__fixup_new_cycles(struct perf_event_attr *attr)
 
 	free(env.cpuid);
 }
+
+bool arch_evsel__must_be_in_group(const struct evsel *evsel)
+{
+	if ((evsel->pmu_name && strcmp(evsel->pmu_name, "cpu")) ||
+	    !pmu_have_event("cpu", "slots"))
+		return false;
+
+	return evsel->name &&
+		(!strcasecmp(evsel->name, "slots") ||
+		 !strncasecmp(evsel->name, "topdown", 7));
+}
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 52ea004ba01e..dfa65a383502 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -1790,8 +1790,17 @@ struct evsel *evlist__reset_weak_group(struct evlist *evsel_list, struct evsel *
 		if (evsel__has_leader(c2, leader)) {
 			if (is_open && close)
 				perf_evsel__close(&c2->core);
-			evsel__set_leader(c2, c2);
-			c2->core.nr_members = 0;
+			/*
+			 * We want to close all members of the group and reopen
+			 * them. Some events, like Intel topdown, require being
+			 * in a group and so keep these in the group.
+			 */
+			if (!evsel__must_be_in_group(c2) && c2 != leader) {
+				evsel__set_leader(c2, c2);
+				c2->core.nr_members = 0;
+				leader->core.nr_members--;
+			}
+
 			/*
 			 * Set this for all former members of the group
 			 * to indicate they get reopened.
@@ -1799,6 +1808,9 @@ struct evsel *evlist__reset_weak_group(struct evlist *evsel_list, struct evsel *
 			c2->reset_group = true;
 		}
 	}
+	/* Reset the leader count if all entries were removed. */
+	if (leader->core.nr_members)
+		leader->core.nr_members = 0;
 	return leader;
 }
 
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index d38722560e80..b7c0c9775673 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -3082,3 +3082,13 @@ int evsel__source_count(const struct evsel *evsel)
 	}
 	return count;
 }
+
+bool __weak arch_evsel__must_be_in_group(const struct evsel *evsel __maybe_unused)
+{
+	return false;
+}
+
+bool evsel__must_be_in_group(const struct evsel *evsel)
+{
+	return arch_evsel__must_be_in_group(evsel);
+}
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 45d674812239..0ed2850b7ebb 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -488,6 +488,9 @@ bool evsel__has_leader(struct evsel *evsel, struct evsel *leader);
 bool evsel__is_leader(struct evsel *evsel);
 void evsel__set_leader(struct evsel *evsel, struct evsel *leader);
 int evsel__source_count(const struct evsel *evsel);
+bool evsel__must_be_in_group(const struct evsel *evsel);
+
+bool arch_evsel__must_be_in_group(const struct evsel *evsel);
 
 /*
  * Macro to swap the bit-field postition and size.
-- 
2.36.0.464.gb9c8b46e94-goog

[PATCH 2/2] perf test: Add basic stat and topdown group test

[Ian Rogers]

Add a basic stat test.
Add two tests of grouping behavior for topdown events. Topdown events
are special as they must be grouped with the slots event first.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/shell/stat.sh | 65 ++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)
 create mode 100755 tools/perf/tests/shell/stat.sh

diff --git a/tools/perf/tests/shell/stat.sh b/tools/perf/tests/shell/stat.sh
new file mode 100755
index 000000000000..80869ea6debc
--- /dev/null
+++ b/tools/perf/tests/shell/stat.sh
@@ -0,0 +1,65 @@
+#!/bin/sh
+# perf stat tests
+# SPDX-License-Identifier: GPL-2.0
+
+set -e
+
+err=0
+test_default_stat() {
+  echo "Basic stat command test"
+  if ! perf stat true 2>&1 | egrep -q "Performance counter stats for 'true':"
+  then
+    echo "Basic stat command test [Failed]"
+    err=1
+    return
+  fi
+  echo "Basic stat command test [Success]"
+}
+
+test_topdown_groups() {
+  # Topdown events must be grouped with the slots event first. Test that
+  # parse-events reorders this.
+  echo "Topdown event group test"
+  if ! perf stat -e '{slots,topdown-retiring}' true > /dev/null 2>&1
+  then
+    echo "Topdown event group test [Skipped event parsing failed]"
+    return
+  fi
+  if perf stat -e '{slots,topdown-retiring}' true 2>&1 | egrep -q "<not supported>"
+  then
+    echo "Topdown event group test [Failed events not supported]"
+    err=1
+    return
+  fi
+  if perf stat -e '{topdown-retiring,slots}' true 2>&1 | egrep -q "<not supported>"
+  then
+    echo "Topdown event group test [Failed slots not reordered first]"
+    err=1
+    return
+  fi
+  echo "Topdown event group test [Success]"
+}
+
+test_topdown_weak_groups() {
+  # Weak groups break if the perf_event_open of multiple grouped events
+  # fails. Breaking a topdown group causes the events to fail. Test a very large
+  # grouping to see that the topdown events aren't broken out.
+  echo "Topdown weak groups test"
+  if ! perf stat -e '{slots,topdown-bad-spec,topdown-be-bound,topdown-fe-bound,topdown-retiring},branch-instructions,branch-misses,bus-cycles,cache-misses,cache-references,cpu-cycles,instructions,mem-loads,mem-stores,ref-cycles,baclears.any,ARITH.DIVIDER_ACTIVE' true > /dev/null 2>&1
+  then
+    echo "Topdown weak groups test [Skipped event parsing failed]"
+    return
+  fi
+  if perf stat -e '{slots,topdown-bad-spec,topdown-be-bound,topdown-fe-bound,topdown-retiring,branch-instructions,branch-misses,bus-cycles,cache-misses,cache-references,cpu-cycles,instructions,mem-loads,mem-stores,ref-cycles,baclears.any,ARITH.DIVIDER_ACTIVE}:W' true 2>&1 | egrep -q "<not supported>"
+  then
+    echo "Topdown weak groups test [Failed events not supported]"
+    err=1
+    return
+  fi
+  echo "Topdown weak groups test [Success]"
+}
+
+test_default_stat
+test_topdown_groups
+test_topdown_weak_groups
+exit $err
-- 
2.36.0.464.gb9c8b46e94-goog

Re: [PATCH 2/2] perf test: Add basic stat and topdown group test

[Liang, Kan]

On 5/5/2022 12:38 AM, Ian Rogers wrote:
> Add a basic stat test.
> Add two tests of grouping behavior for topdown events. Topdown events
> are special as they must be grouped with the slots event first.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>   tools/perf/tests/shell/stat.sh | 65 ++++++++++++++++++++++++++++++++++
>   1 file changed, 65 insertions(+)
>   create mode 100755 tools/perf/tests/shell/stat.sh
> 
> diff --git a/tools/perf/tests/shell/stat.sh b/tools/perf/tests/shell/stat.sh
> new file mode 100755
> index 000000000000..80869ea6debc
> --- /dev/null
> +++ b/tools/perf/tests/shell/stat.sh
> @@ -0,0 +1,65 @@
> +#!/bin/sh
> +# perf stat tests
> +# SPDX-License-Identifier: GPL-2.0
> +
> +set -e
> +
> +err=0
> +test_default_stat() {
> +  echo "Basic stat command test"
> +  if ! perf stat true 2>&1 | egrep -q "Performance counter stats for 'true':"
> +  then
> +    echo "Basic stat command test [Failed]"
> +    err=1
> +    return
> +  fi
> +  echo "Basic stat command test [Success]"
> +}
> +
> +test_topdown_groups() {
> +  # Topdown events must be grouped with the slots event first. Test that
> +  # parse-events reorders this.
> +  echo "Topdown event group test"
> +  if ! perf stat -e '{slots,topdown-retiring}' true > /dev/null 2>&1
> +  then
> +    echo "Topdown event group test [Skipped event parsing failed]"
> +    return
> +  fi
> +  if perf stat -e '{slots,topdown-retiring}' true 2>&1 | egrep -q "<not supported>"
> +  then
> +    echo "Topdown event group test [Failed events not supported]"
> +    err=1
> +    return
> +  fi
> +  if perf stat -e '{topdown-retiring,slots}' true 2>&1 | egrep -q "<not supported>"
> +  then
> +    echo "Topdown event group test [Failed slots not reordered first]"
> +    err=1
> +    return
> +  fi
> +  echo "Topdown event group test [Success]"
> +}
> +
> +test_topdown_weak_groups() {
> +  # Weak groups break if the perf_event_open of multiple grouped events
> +  # fails. Breaking a topdown group causes the events to fail. Test a very large
> +  # grouping to see that the topdown events aren't broken out.
> +  echo "Topdown weak groups test"
> +  if ! perf stat -e '{slots,topdown-bad-spec,topdown-be-bound,topdown-fe-bound,topdown-retiring},branch-instructions,branch-misses,bus-cycles,cache-misses,cache-references,cpu-cycles,instructions,mem-loads,mem-stores,ref-cycles,baclears.any,ARITH.DIVIDER_ACTIVE' true > /dev/null 2>&1
> +  then
> +    echo "Topdown weak groups test [Skipped event parsing failed]"
> +    return
> +  fi
> +  if perf stat -e '{slots,topdown-bad-spec,topdown-be-bound,topdown-fe-bound,topdown-retiring,branch-instructions,branch-misses,bus-cycles,cache-misses,cache-references,cpu-cycles,instructions,mem-loads,mem-stores,ref-cycles,baclears.any,ARITH.DIVIDER_ACTIVE}:W' true 2>&1 | egrep -q "<not supported>"
> +  then
> +    echo "Topdown weak groups test [Failed events not supported]"
> +    err=1
> +    return
> +  fi
> +  echo "Topdown weak groups test [Success]"
> +}
> +

Should we check the existence of the slots event before the test?
The perf metrics feature only be available on the new platform after 
ICL. It doesn't work on Atom.

Also, I think the test may fails on the hybrid platform, since big core 
and small core have different formula for the topdown. I think we should 
avoid the test for the hybrid platform for now.
+Zhengjun, who is fixing the topdown gap for the hybrid platform. I 
think he may take care of the hybrid support later.

Thanks,
Kan
> +test_default_stat
> +test_topdown_groups
> +test_topdown_weak_groups
> +exit $err

Re: [PATCH 2/2] perf test: Add basic stat and topdown group test

[Ian Rogers]

On Thu, May 5, 2022 at 5:12 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
> On 5/5/2022 12:38 AM, Ian Rogers wrote:
> > Add a basic stat test.
> > Add two tests of grouping behavior for topdown events. Topdown events
> > are special as they must be grouped with the slots event first.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >   tools/perf/tests/shell/stat.sh | 65 ++++++++++++++++++++++++++++++++++
> >   1 file changed, 65 insertions(+)
> >   create mode 100755 tools/perf/tests/shell/stat.sh
> >
> > diff --git a/tools/perf/tests/shell/stat.sh b/tools/perf/tests/shell/stat.sh
> > new file mode 100755
> > index 000000000000..80869ea6debc
> > --- /dev/null
> > +++ b/tools/perf/tests/shell/stat.sh
> > @@ -0,0 +1,65 @@
> > +#!/bin/sh
> > +# perf stat tests
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +set -e
> > +
> > +err=0
> > +test_default_stat() {
> > +  echo "Basic stat command test"
> > +  if ! perf stat true 2>&1 | egrep -q "Performance counter stats for 'true':"
> > +  then
> > +    echo "Basic stat command test [Failed]"
> > +    err=1
> > +    return
> > +  fi
> > +  echo "Basic stat command test [Success]"
> > +}
> > +
> > +test_topdown_groups() {
> > +  # Topdown events must be grouped with the slots event first. Test that
> > +  # parse-events reorders this.
> > +  echo "Topdown event group test"
> > +  if ! perf stat -e '{slots,topdown-retiring}' true > /dev/null 2>&1
> > +  then
> > +    echo "Topdown event group test [Skipped event parsing failed]"
> > +    return
> > +  fi
> > +  if perf stat -e '{slots,topdown-retiring}' true 2>&1 | egrep -q "<not supported>"
> > +  then
> > +    echo "Topdown event group test [Failed events not supported]"
> > +    err=1
> > +    return
> > +  fi
> > +  if perf stat -e '{topdown-retiring,slots}' true 2>&1 | egrep -q "<not supported>"
> > +  then
> > +    echo "Topdown event group test [Failed slots not reordered first]"
> > +    err=1
> > +    return
> > +  fi
> > +  echo "Topdown event group test [Success]"
> > +}
> > +
> > +test_topdown_weak_groups() {
> > +  # Weak groups break if the perf_event_open of multiple grouped events
> > +  # fails. Breaking a topdown group causes the events to fail. Test a very large
> > +  # grouping to see that the topdown events aren't broken out.
> > +  echo "Topdown weak groups test"
> > +  if ! perf stat -e '{slots,topdown-bad-spec,topdown-be-bound,topdown-fe-bound,topdown-retiring},branch-instructions,branch-misses,bus-cycles,cache-misses,cache-references,cpu-cycles,instructions,mem-loads,mem-stores,ref-cycles,baclears.any,ARITH.DIVIDER_ACTIVE' true > /dev/null 2>&1
> > +  then
> > +    echo "Topdown weak groups test [Skipped event parsing failed]"
> > +    return
> > +  fi
> > +  if perf stat -e '{slots,topdown-bad-spec,topdown-be-bound,topdown-fe-bound,topdown-retiring,branch-instructions,branch-misses,bus-cycles,cache-misses,cache-references,cpu-cycles,instructions,mem-loads,mem-stores,ref-cycles,baclears.any,ARITH.DIVIDER_ACTIVE}:W' true 2>&1 | egrep -q "<not supported>"
> > +  then
> > +    echo "Topdown weak groups test [Failed events not supported]"
> > +    err=1
> > +    return
> > +  fi
> > +  echo "Topdown weak groups test [Success]"
> > +}
> > +
>
> Should we check the existence of the slots event before the test?
> The perf metrics feature only be available on the new platform after
> ICL. It doesn't work on Atom.
>
> Also, I think the test may fails on the hybrid platform, since big core
> and small core have different formula for the topdown. I think we should
> avoid the test for the hybrid platform for now.
> +Zhengjun, who is fixing the topdown gap for the hybrid platform. I
> think he may take care of the hybrid support later.

Thanks Kan, the test filters out systems that don't support the events
and silently skips the test. The main purpose of the test is to make
sure the somewhat complicated grouping operations for Icelake have
some coverage. Adding more coverage for hybrid would be great, but not
something I think gates this change.

Thanks,
Ian

> Thanks,
> Kan
> > +test_default_stat
> > +test_topdown_groups
> > +test_topdown_weak_groups
> > +exit $err

Re: [PATCH 2/2] perf test: Add basic stat and topdown group test

[Liang, Kan]

On 5/5/2022 11:22 AM, Ian Rogers wrote:
> On Thu, May 5, 2022 at 5:12 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>>
>> On 5/5/2022 12:38 AM, Ian Rogers wrote:
>>> Add a basic stat test.
>>> Add two tests of grouping behavior for topdown events. Topdown events
>>> are special as they must be grouped with the slots event first.
>>>
>>> Signed-off-by: Ian Rogers <irogers@google.com>
>>> ---
>>>    tools/perf/tests/shell/stat.sh | 65 ++++++++++++++++++++++++++++++++++
>>>    1 file changed, 65 insertions(+)
>>>    create mode 100755 tools/perf/tests/shell/stat.sh
>>>
>>> diff --git a/tools/perf/tests/shell/stat.sh b/tools/perf/tests/shell/stat.sh
>>> new file mode 100755
>>> index 000000000000..80869ea6debc
>>> --- /dev/null
>>> +++ b/tools/perf/tests/shell/stat.sh
>>> @@ -0,0 +1,65 @@
>>> +#!/bin/sh
>>> +# perf stat tests
>>> +# SPDX-License-Identifier: GPL-2.0
>>> +
>>> +set -e
>>> +
>>> +err=0
>>> +test_default_stat() {
>>> +  echo "Basic stat command test"
>>> +  if ! perf stat true 2>&1 | egrep -q "Performance counter stats for 'true':"
>>> +  then
>>> +    echo "Basic stat command test [Failed]"
>>> +    err=1
>>> +    return
>>> +  fi
>>> +  echo "Basic stat command test [Success]"
>>> +}
>>> +
>>> +test_topdown_groups() {
>>> +  # Topdown events must be grouped with the slots event first. Test that
>>> +  # parse-events reorders this.
>>> +  echo "Topdown event group test"
>>> +  if ! perf stat -e '{slots,topdown-retiring}' true > /dev/null 2>&1
>>> +  then
>>> +    echo "Topdown event group test [Skipped event parsing failed]"
>>> +    return
>>> +  fi
>>> +  if perf stat -e '{slots,topdown-retiring}' true 2>&1 | egrep -q "<not supported>"
>>> +  then
>>> +    echo "Topdown event group test [Failed events not supported]"
>>> +    err=1
>>> +    return
>>> +  fi
>>> +  if perf stat -e '{topdown-retiring,slots}' true 2>&1 | egrep -q "<not supported>"
>>> +  then
>>> +    echo "Topdown event group test [Failed slots not reordered first]"
>>> +    err=1
>>> +    return
>>> +  fi
>>> +  echo "Topdown event group test [Success]"
>>> +}
>>> +
>>> +test_topdown_weak_groups() {
>>> +  # Weak groups break if the perf_event_open of multiple grouped events
>>> +  # fails. Breaking a topdown group causes the events to fail. Test a very large
>>> +  # grouping to see that the topdown events aren't broken out.
>>> +  echo "Topdown weak groups test"
>>> +  if ! perf stat -e '{slots,topdown-bad-spec,topdown-be-bound,topdown-fe-bound,topdown-retiring},branch-instructions,branch-misses,bus-cycles,cache-misses,cache-references,cpu-cycles,instructions,mem-loads,mem-stores,ref-cycles,baclears.any,ARITH.DIVIDER_ACTIVE' true > /dev/null 2>&1
>>> +  then
>>> +    echo "Topdown weak groups test [Skipped event parsing failed]"
>>> +    return
>>> +  fi
>>> +  if perf stat -e '{slots,topdown-bad-spec,topdown-be-bound,topdown-fe-bound,topdown-retiring,branch-instructions,branch-misses,bus-cycles,cache-misses,cache-references,cpu-cycles,instructions,mem-loads,mem-stores,ref-cycles,baclears.any,ARITH.DIVIDER_ACTIVE}:W' true 2>&1 | egrep -q "<not supported>"
>>> +  then
>>> +    echo "Topdown weak groups test [Failed events not supported]"
>>> +    err=1
>>> +    return
>>> +  fi
>>> +  echo "Topdown weak groups test [Success]"
>>> +}
>>> +
>>
>> Should we check the existence of the slots event before the test?
>> The perf metrics feature only be available on the new platform after
>> ICL. It doesn't work on Atom.
>>
>> Also, I think the test may fails on the hybrid platform, since big core
>> and small core have different formula for the topdown. I think we should
>> avoid the test for the hybrid platform for now.
>> +Zhengjun, who is fixing the topdown gap for the hybrid platform. I
>> think he may take care of the hybrid support later.
> 
> Thanks Kan, the test filters out systems that don't support the events
> and silently skips the test. The main purpose of the test is to make
> sure the somewhat complicated grouping operations for Icelake have
> some coverage. Adding more coverage for hybrid would be great, but not
> something I think gates this change.
>

Sure, we can add the coverage for hybrid later. But please make sure the 
test can filter out both the systems which doesn't support perf metircs 
and the hybrid system.

Thanks,
Kan

> Thanks,
> Ian
> 
>> Thanks,
>> Kan
>>> +test_default_stat
>>> +test_topdown_groups
>>> +test_topdown_weak_groups
>>> +exit $err

Re: [PATCH 2/2] perf test: Add basic stat and topdown group test

[Ian Rogers]

On Thu, May 5, 2022 at 11:19 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
> On 5/5/2022 11:22 AM, Ian Rogers wrote:
> > On Thu, May 5, 2022 at 5:12 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
> >>
> >> On 5/5/2022 12:38 AM, Ian Rogers wrote:
> >>> Add a basic stat test.
> >>> Add two tests of grouping behavior for topdown events. Topdown events
> >>> are special as they must be grouped with the slots event first.
> >>>
> >>> Signed-off-by: Ian Rogers <irogers@google.com>
> >>> ---
> >>>    tools/perf/tests/shell/stat.sh | 65 ++++++++++++++++++++++++++++++++++
> >>>    1 file changed, 65 insertions(+)
> >>>    create mode 100755 tools/perf/tests/shell/stat.sh
> >>>
> >>> diff --git a/tools/perf/tests/shell/stat.sh b/tools/perf/tests/shell/stat.sh
> >>> new file mode 100755
> >>> index 000000000000..80869ea6debc
> >>> --- /dev/null
> >>> +++ b/tools/perf/tests/shell/stat.sh
> >>> @@ -0,0 +1,65 @@
> >>> +#!/bin/sh
> >>> +# perf stat tests
> >>> +# SPDX-License-Identifier: GPL-2.0
> >>> +
> >>> +set -e
> >>> +
> >>> +err=0
> >>> +test_default_stat() {
> >>> +  echo "Basic stat command test"
> >>> +  if ! perf stat true 2>&1 | egrep -q "Performance counter stats for 'true':"
> >>> +  then
> >>> +    echo "Basic stat command test [Failed]"
> >>> +    err=1
> >>> +    return
> >>> +  fi
> >>> +  echo "Basic stat command test [Success]"
> >>> +}
> >>> +
> >>> +test_topdown_groups() {
> >>> +  # Topdown events must be grouped with the slots event first. Test that
> >>> +  # parse-events reorders this.
> >>> +  echo "Topdown event group test"
> >>> +  if ! perf stat -e '{slots,topdown-retiring}' true > /dev/null 2>&1
> >>> +  then
> >>> +    echo "Topdown event group test [Skipped event parsing failed]"
> >>> +    return
> >>> +  fi
> >>> +  if perf stat -e '{slots,topdown-retiring}' true 2>&1 | egrep -q "<not supported>"
> >>> +  then
> >>> +    echo "Topdown event group test [Failed events not supported]"
> >>> +    err=1
> >>> +    return
> >>> +  fi
> >>> +  if perf stat -e '{topdown-retiring,slots}' true 2>&1 | egrep -q "<not supported>"
> >>> +  then
> >>> +    echo "Topdown event group test [Failed slots not reordered first]"
> >>> +    err=1
> >>> +    return
> >>> +  fi
> >>> +  echo "Topdown event group test [Success]"
> >>> +}
> >>> +
> >>> +test_topdown_weak_groups() {
> >>> +  # Weak groups break if the perf_event_open of multiple grouped events
> >>> +  # fails. Breaking a topdown group causes the events to fail. Test a very large
> >>> +  # grouping to see that the topdown events aren't broken out.
> >>> +  echo "Topdown weak groups test"
> >>> +  if ! perf stat -e '{slots,topdown-bad-spec,topdown-be-bound,topdown-fe-bound,topdown-retiring},branch-instructions,branch-misses,bus-cycles,cache-misses,cache-references,cpu-cycles,instructions,mem-loads,mem-stores,ref-cycles,baclears.any,ARITH.DIVIDER_ACTIVE' true > /dev/null 2>&1
> >>> +  then
> >>> +    echo "Topdown weak groups test [Skipped event parsing failed]"
> >>> +    return
> >>> +  fi
> >>> +  if perf stat -e '{slots,topdown-bad-spec,topdown-be-bound,topdown-fe-bound,topdown-retiring,branch-instructions,branch-misses,bus-cycles,cache-misses,cache-references,cpu-cycles,instructions,mem-loads,mem-stores,ref-cycles,baclears.any,ARITH.DIVIDER_ACTIVE}:W' true 2>&1 | egrep -q "<not supported>"
> >>> +  then
> >>> +    echo "Topdown weak groups test [Failed events not supported]"
> >>> +    err=1
> >>> +    return
> >>> +  fi
> >>> +  echo "Topdown weak groups test [Success]"
> >>> +}
> >>> +
> >>
> >> Should we check the existence of the slots event before the test?
> >> The perf metrics feature only be available on the new platform after
> >> ICL. It doesn't work on Atom.
> >>
> >> Also, I think the test may fails on the hybrid platform, since big core
> >> and small core have different formula for the topdown. I think we should
> >> avoid the test for the hybrid platform for now.
> >> +Zhengjun, who is fixing the topdown gap for the hybrid platform. I
> >> think he may take care of the hybrid support later.
> >
> > Thanks Kan, the test filters out systems that don't support the events
> > and silently skips the test. The main purpose of the test is to make
> > sure the somewhat complicated grouping operations for Icelake have
> > some coverage. Adding more coverage for hybrid would be great, but not
> > something I think gates this change.
> >
>
> Sure, we can add the coverage for hybrid later. But please make sure the
> test can filter out both the systems which doesn't support perf metircs
> and the hybrid system.
>
> Thanks,
> Kan

If the test fails on hybrid then that feels like value add :-) We
genuinely have broken grouping functions. We could just add to the
test a skip if /sys/devices/cpu_core and /sys/devices/cpu_atom
directories exist (making assumptions on where sysfs is mounted). I'm
not yet able to test on Alderlake hence not wanting to have a lot of
untested code.

Thanks,
Ian

> > Thanks,
> > Ian
> >
> >> Thanks,
> >> Kan
> >>> +test_default_stat
> >>> +test_topdown_groups
> >>> +test_topdown_weak_groups
> >>> +exit $err

Re: [PATCH 2/2] perf test: Add basic stat and topdown group test

[Liang, Kan]

On 5/5/2022 2:35 PM, Ian Rogers wrote:
> On Thu, May 5, 2022 at 11:19 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>>
>> On 5/5/2022 11:22 AM, Ian Rogers wrote:
>>> On Thu, May 5, 2022 at 5:12 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>>>>
>>>> On 5/5/2022 12:38 AM, Ian Rogers wrote:
>>>>> Add a basic stat test.
>>>>> Add two tests of grouping behavior for topdown events. Topdown events
>>>>> are special as they must be grouped with the slots event first.
>>>>>
>>>>> Signed-off-by: Ian Rogers <irogers@google.com>
>>>>> ---
>>>>>     tools/perf/tests/shell/stat.sh | 65 ++++++++++++++++++++++++++++++++++
>>>>>     1 file changed, 65 insertions(+)
>>>>>     create mode 100755 tools/perf/tests/shell/stat.sh
>>>>>
>>>>> diff --git a/tools/perf/tests/shell/stat.sh b/tools/perf/tests/shell/stat.sh
>>>>> new file mode 100755
>>>>> index 000000000000..80869ea6debc
>>>>> --- /dev/null
>>>>> +++ b/tools/perf/tests/shell/stat.sh
>>>>> @@ -0,0 +1,65 @@
>>>>> +#!/bin/sh
>>>>> +# perf stat tests
>>>>> +# SPDX-License-Identifier: GPL-2.0
>>>>> +
>>>>> +set -e
>>>>> +
>>>>> +err=0
>>>>> +test_default_stat() {
>>>>> +  echo "Basic stat command test"
>>>>> +  if ! perf stat true 2>&1 | egrep -q "Performance counter stats for 'true':"
>>>>> +  then
>>>>> +    echo "Basic stat command test [Failed]"
>>>>> +    err=1
>>>>> +    return
>>>>> +  fi
>>>>> +  echo "Basic stat command test [Success]"
>>>>> +}
>>>>> +
>>>>> +test_topdown_groups() {
>>>>> +  # Topdown events must be grouped with the slots event first. Test that
>>>>> +  # parse-events reorders this.
>>>>> +  echo "Topdown event group test"
>>>>> +  if ! perf stat -e '{slots,topdown-retiring}' true > /dev/null 2>&1
>>>>> +  then
>>>>> +    echo "Topdown event group test [Skipped event parsing failed]"
>>>>> +    return
>>>>> +  fi
>>>>> +  if perf stat -e '{slots,topdown-retiring}' true 2>&1 | egrep -q "<not supported>"
>>>>> +  then
>>>>> +    echo "Topdown event group test [Failed events not supported]"
>>>>> +    err=1
>>>>> +    return
>>>>> +  fi
>>>>> +  if perf stat -e '{topdown-retiring,slots}' true 2>&1 | egrep -q "<not supported>"
>>>>> +  then
>>>>> +    echo "Topdown event group test [Failed slots not reordered first]"
>>>>> +    err=1
>>>>> +    return
>>>>> +  fi
>>>>> +  echo "Topdown event group test [Success]"
>>>>> +}
>>>>> +
>>>>> +test_topdown_weak_groups() {
>>>>> +  # Weak groups break if the perf_event_open of multiple grouped events
>>>>> +  # fails. Breaking a topdown group causes the events to fail. Test a very large
>>>>> +  # grouping to see that the topdown events aren't broken out.
>>>>> +  echo "Topdown weak groups test"
>>>>> +  if ! perf stat -e '{slots,topdown-bad-spec,topdown-be-bound,topdown-fe-bound,topdown-retiring},branch-instructions,branch-misses,bus-cycles,cache-misses,cache-references,cpu-cycles,instructions,mem-loads,mem-stores,ref-cycles,baclears.any,ARITH.DIVIDER_ACTIVE' true > /dev/null 2>&1
>>>>> +  then
>>>>> +    echo "Topdown weak groups test [Skipped event parsing failed]"
>>>>> +    return
>>>>> +  fi
>>>>> +  if perf stat -e '{slots,topdown-bad-spec,topdown-be-bound,topdown-fe-bound,topdown-retiring,branch-instructions,branch-misses,bus-cycles,cache-misses,cache-references,cpu-cycles,instructions,mem-loads,mem-stores,ref-cycles,baclears.any,ARITH.DIVIDER_ACTIVE}:W' true 2>&1 | egrep -q "<not supported>"
>>>>> +  then
>>>>> +    echo "Topdown weak groups test [Failed events not supported]"
>>>>> +    err=1
>>>>> +    return
>>>>> +  fi
>>>>> +  echo "Topdown weak groups test [Success]"
>>>>> +}
>>>>> +
>>>>
>>>> Should we check the existence of the slots event before the test?
>>>> The perf metrics feature only be available on the new platform after
>>>> ICL. It doesn't work on Atom.
>>>>
>>>> Also, I think the test may fails on the hybrid platform, since big core
>>>> and small core have different formula for the topdown. I think we should
>>>> avoid the test for the hybrid platform for now.
>>>> +Zhengjun, who is fixing the topdown gap for the hybrid platform. I
>>>> think he may take care of the hybrid support later.
>>>
>>> Thanks Kan, the test filters out systems that don't support the events
>>> and silently skips the test. The main purpose of the test is to make
>>> sure the somewhat complicated grouping operations for Icelake have
>>> some coverage. Adding more coverage for hybrid would be great, but not
>>> something I think gates this change.
>>>
>>
>> Sure, we can add the coverage for hybrid later. But please make sure the
>> test can filter out both the systems which doesn't support perf metircs
>> and the hybrid system.
>>
>> Thanks,
>> Kan
> 
> If the test fails on hybrid then that feels like value add :-) 

Indeed. The test case is valid.

Could you please use architecture events to replace the 
baclears.any,ARITH.DIVIDER_ACTIVE?
It's not guaranteed that the two events are supported for the future 
platforms.

I use the duplicated cache-misses,cache-references events to replace 
baclears.any,ARITH.DIVIDER_ACTIVE and tested on the ADL. The results 
look good.

./perf stat -e 
'{slots,topdown-bad-spec,topdown-be-bound,topdown-fe-bound,topdown-retiring,branch-instructions,branch-misses,bus-cycles,cache-misses,cache-references,cpu-cycles,instructions,mem-loads,mem-stores,ref-cycles,cache-misses,cache-references}:W' 
true
WARNING: events in group from different hybrid PMUs!

  Performance counter stats for 'true':

          3,603,798      cpu_core/slots/
            480,506      cpu_core/topdown-bad-spec/ #     13.3% bad 
speculation
      <not counted>      cpu_atom/topdown-bad-spec/ 
                 (0.00%)
            833,819      cpu_core/topdown-be-bound/ #     23.0% backend 
bound
      <not counted>      cpu_atom/topdown-be-bound/ 
                 (0.00%)
          1,483,916      cpu_core/topdown-fe-bound/ #     41.0% frontend 
bound
      <not counted>      cpu_atom/topdown-fe-bound/ 
                 (0.00%)
            819,687      cpu_core/topdown-retiring/ #     22.7% retiring
      <not counted>      cpu_atom/topdown-retiring/ 
                 (0.00%)
            151,010      cpu_core/branch-instructions/
      <not counted>      cpu_atom/branch-instructions/ 
                    (0.00%)
              4,402      cpu_core/branch-misses/
      <not counted>      cpu_atom/branch-misses/ 
                (0.00%)
          1,456,308      cpu_core/bus-cycles/
      <not counted>      cpu_atom/bus-cycles/ 
                (0.00%)
              1,132      cpu_core/cache-misses/
      <not counted>      cpu_atom/cache-misses/ 
                (0.00%)
             14,226      cpu_core/cache-references/
      <not counted>      cpu_atom/cache-references/ 
                 (0.00%)
            600,633      cpu_core/cpu-cycles/
      <not counted>      cpu_atom/cpu-cycles/ 
                (0.00%)
            737,806      cpu_core/instructions/
      <not counted>      cpu_atom/instructions/ 
                (0.00%)
                  0      cpu_core/mem-loads/
      <not counted>      cpu_atom/mem-loads/ 
                (0.00%)
            120,151      cpu_core/mem-stores/
      <not counted>      cpu_atom/mem-stores/ 
                (0.00%)
          1,456,308      cpu_core/ref-cycles/
      <not counted>      cpu_atom/ref-cycles/ 
                (0.00%)
      <not counted>      cpu_core/cache-misses/ 
                (0.00%)
      <not counted>      cpu_atom/cache-misses/ 
                (0.00%)
      <not counted>      cpu_core/cache-references/ 
                 (0.00%)
      <not counted>      cpu_atom/cache-references/ 
                 (0.00%)

        0.001518955 seconds time elapsed

        0.001531000 seconds user
        0.000000000 seconds sys

Thanks,
Kan
> We
> genuinely have broken grouping functions. We could just add to the
> test a skip if /sys/devices/cpu_core and /sys/devices/cpu_atom
> directories exist (making assumptions on where sysfs is mounted). I'm
> not yet able to test on Alderlake hence not wanting to have a lot of
> untested code.
> 
> Thanks,
> Ian
> 
>>> Thanks,
>>> Ian
>>>
>>>> Thanks,
>>>> Kan
>>>>> +test_default_stat
>>>>> +test_topdown_groups
>>>>> +test_topdown_weak_groups
>>>>> +exit $err

Re: [PATCH 1/2] perf evlist: Keep topdown counters in weak group

[Liang, Kan]

Hi Ian,

Thanks for the patches.

On 5/5/2022 12:38 AM, Ian Rogers wrote:
> On Intel Icelake, topdown events must always be grouped with a slots
> event as leader. When a metric is parsed a weak group is formed and
> retried if perf_event_open fails. The retried events aren't grouped
> breaking the slots leader requirement. This change modifies the weak
> group "reset" behavior so that topdown events aren't broken from the
> group for the retry.
> 
> $ perf stat -e '{slots,topdown-bad-spec,topdown-be-bound,topdown-fe-bound,topdown-retiring,branch-instructions,branch-misses,bus-cycles,cache-misses,cache-references,cpu-cycles,instructions,mem-loads,mem-stores,ref-cycles,baclears.any,ARITH.DIVIDER_ACTIVE}:W' -a sleep 1
> 
>   Performance counter stats for 'system wide':
> 
>      47,867,188,483      slots                                                         (92.27%)
>     <not supported>      topdown-bad-spec
>     <not supported>      topdown-be-bound
>     <not supported>      topdown-fe-bound
>     <not supported>      topdown-retiring
>       2,173,346,937      branch-instructions                                           (92.27%)
>          10,540,253      branch-misses             #    0.48% of all branches          (92.29%)
>          96,291,140      bus-cycles                                                    (92.29%)
>           6,214,202      cache-misses              #   20.120 % of all cache refs      (92.29%)
>          30,886,082      cache-references                                              (76.91%)
>      11,773,726,641      cpu-cycles                                                    (84.62%)
>      11,807,585,307      instructions              #    1.00  insn per cycle           (92.31%)
>                   0      mem-loads                                                     (92.32%)
>       2,212,928,573      mem-stores                                                    (84.69%)
>      10,024,403,118      ref-cycles                                                    (92.35%)
>          16,232,978      baclears.any                                                  (92.35%)
>          23,832,633      ARITH.DIVIDER_ACTIVE                                          (84.59%)
> 
>         0.981070734 seconds time elapsed
> 
> After:
> 
> $ perf stat -e '{slots,topdown-bad-spec,topdown-be-bound,topdown-fe-bound,topdown-retiring,branch-instructions,branch-misses,bus-cycles,cache-misses,cache-references,cpu-cycles,instructions,mem-loads,mem-stores,ref-cycles,baclears.any,ARITH.DIVIDER_ACTIVE}:W' -a sleep 1
> 
>   Performance counter stats for 'system wide':
> 
>         31040189283      slots                                                         (92.27%)
>          8997514811      topdown-bad-spec          #     28.2% bad speculation         (92.27%)
>         10997536028      topdown-be-bound          #     34.5% backend bound           (92.27%)
>          4778060526      topdown-fe-bound          #     15.0% frontend bound          (92.27%)
>          7086628768      topdown-retiring          #     22.2% retiring                (92.27%)
>          1417611942      branch-instructions                                           (92.26%)
>             5285529      branch-misses             #    0.37% of all branches          (92.28%)
>            62922469      bus-cycles                                                    (92.29%)
>             1440708      cache-misses              #    8.292 % of all cache refs      (92.30%)
>            17374098      cache-references                                              (76.94%)
>          8040889520      cpu-cycles                                                    (84.63%)
>          7709992319      instructions              #    0.96  insn per cycle           (92.32%)
>                   0      mem-loads                                                     (92.32%)
>          1515669558      mem-stores                                                    (84.68%)
>          6542411177      ref-cycles                                                    (92.35%)
>             4154149      baclears.any                                                  (92.35%)
>            20556152      ARITH.DIVIDER_ACTIVE                                          (84.59%)
> 
>         1.010799593 seconds time elapsed
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>   tools/perf/arch/x86/util/evsel.c | 12 ++++++++++++
>   tools/perf/util/evlist.c         | 16 ++++++++++++++--
>   tools/perf/util/evsel.c          | 10 ++++++++++
>   tools/perf/util/evsel.h          |  3 +++
>   4 files changed, 39 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/arch/x86/util/evsel.c b/tools/perf/arch/x86/util/evsel.c
> index ac2899a25b7a..40b171de2086 100644
> --- a/tools/perf/arch/x86/util/evsel.c
> +++ b/tools/perf/arch/x86/util/evsel.c
> @@ -3,6 +3,7 @@
>   #include <stdlib.h>
>   #include "util/evsel.h"
>   #include "util/env.h"
> +#include "util/pmu.h"
>   #include "linux/string.h"
>   
>   void arch_evsel__set_sample_weight(struct evsel *evsel)
> @@ -29,3 +30,14 @@ void arch_evsel__fixup_new_cycles(struct perf_event_attr *attr)
>   
>   	free(env.cpuid);
>   }
> +
> +bool arch_evsel__must_be_in_group(const struct evsel *evsel)
> +{
> +	if ((evsel->pmu_name && strcmp(evsel->pmu_name, "cpu")) ||
> +	    !pmu_have_event("cpu", "slots"))
> +		return false;
> +

The big core of ADL also supports Perf_metrics. At least, we should 
check both "cpu" and "cpu_core" here.

Thanks,
Kan
> +	return evsel->name &&
> +		(!strcasecmp(evsel->name, "slots") ||
> +		 !strncasecmp(evsel->name, "topdown", 7));
> +}
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 52ea004ba01e..dfa65a383502 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -1790,8 +1790,17 @@ struct evsel *evlist__reset_weak_group(struct evlist *evsel_list, struct evsel *
>   		if (evsel__has_leader(c2, leader)) {
>   			if (is_open && close)
>   				perf_evsel__close(&c2->core);
> -			evsel__set_leader(c2, c2);
> -			c2->core.nr_members = 0;
> +			/*
> +			 * We want to close all members of the group and reopen
> +			 * them. Some events, like Intel topdown, require being
> +			 * in a group and so keep these in the group.
> +			 */
> +			if (!evsel__must_be_in_group(c2) && c2 != leader) {
> +				evsel__set_leader(c2, c2);
> +				c2->core.nr_members = 0;
> +				leader->core.nr_members--;
> +			}
> +
>   			/*
>   			 * Set this for all former members of the group
>   			 * to indicate they get reopened.
> @@ -1799,6 +1808,9 @@ struct evsel *evlist__reset_weak_group(struct evlist *evsel_list, struct evsel *
>   			c2->reset_group = true;
>   		}
>   	}
> +	/* Reset the leader count if all entries were removed. */
> +	if (leader->core.nr_members)
> +		leader->core.nr_members = 0;
>   	return leader;
>   }
>   
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index d38722560e80..b7c0c9775673 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -3082,3 +3082,13 @@ int evsel__source_count(const struct evsel *evsel)
>   	}
>   	return count;
>   }
> +
> +bool __weak arch_evsel__must_be_in_group(const struct evsel *evsel __maybe_unused)
> +{
> +	return false;
> +}
> +
> +bool evsel__must_be_in_group(const struct evsel *evsel)
> +{
> +	return arch_evsel__must_be_in_group(evsel);
> +}
> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> index 45d674812239..0ed2850b7ebb 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -488,6 +488,9 @@ bool evsel__has_leader(struct evsel *evsel, struct evsel *leader);
>   bool evsel__is_leader(struct evsel *evsel);
>   void evsel__set_leader(struct evsel *evsel, struct evsel *leader);
>   int evsel__source_count(const struct evsel *evsel);
> +bool evsel__must_be_in_group(const struct evsel *evsel);
> +
> +bool arch_evsel__must_be_in_group(const struct evsel *evsel);
>   
>   /*
>    * Macro to swap the bit-field postition and size.

Re: [PATCH 1/2] perf evlist: Keep topdown counters in weak group

[Ian Rogers]

On Thu, May 5, 2022 at 4:56 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
> Hi Ian,
>
> Thanks for the patches.
>
> On 5/5/2022 12:38 AM, Ian Rogers wrote:
> > On Intel Icelake, topdown events must always be grouped with a slots
> > event as leader. When a metric is parsed a weak group is formed and
> > retried if perf_event_open fails. The retried events aren't grouped
> > breaking the slots leader requirement. This change modifies the weak
> > group "reset" behavior so that topdown events aren't broken from the
> > group for the retry.
> >
> > $ perf stat -e '{slots,topdown-bad-spec,topdown-be-bound,topdown-fe-bound,topdown-retiring,branch-instructions,branch-misses,bus-cycles,cache-misses,cache-references,cpu-cycles,instructions,mem-loads,mem-stores,ref-cycles,baclears.any,ARITH.DIVIDER_ACTIVE}:W' -a sleep 1
> >
> >   Performance counter stats for 'system wide':
> >
> >      47,867,188,483      slots                                                         (92.27%)
> >     <not supported>      topdown-bad-spec
> >     <not supported>      topdown-be-bound
> >     <not supported>      topdown-fe-bound
> >     <not supported>      topdown-retiring
> >       2,173,346,937      branch-instructions                                           (92.27%)
> >          10,540,253      branch-misses             #    0.48% of all branches          (92.29%)
> >          96,291,140      bus-cycles                                                    (92.29%)
> >           6,214,202      cache-misses              #   20.120 % of all cache refs      (92.29%)
> >          30,886,082      cache-references                                              (76.91%)
> >      11,773,726,641      cpu-cycles                                                    (84.62%)
> >      11,807,585,307      instructions              #    1.00  insn per cycle           (92.31%)
> >                   0      mem-loads                                                     (92.32%)
> >       2,212,928,573      mem-stores                                                    (84.69%)
> >      10,024,403,118      ref-cycles                                                    (92.35%)
> >          16,232,978      baclears.any                                                  (92.35%)
> >          23,832,633      ARITH.DIVIDER_ACTIVE                                          (84.59%)
> >
> >         0.981070734 seconds time elapsed
> >
> > After:
> >
> > $ perf stat -e '{slots,topdown-bad-spec,topdown-be-bound,topdown-fe-bound,topdown-retiring,branch-instructions,branch-misses,bus-cycles,cache-misses,cache-references,cpu-cycles,instructions,mem-loads,mem-stores,ref-cycles,baclears.any,ARITH.DIVIDER_ACTIVE}:W' -a sleep 1
> >
> >   Performance counter stats for 'system wide':
> >
> >         31040189283      slots                                                         (92.27%)
> >          8997514811      topdown-bad-spec          #     28.2% bad speculation         (92.27%)
> >         10997536028      topdown-be-bound          #     34.5% backend bound           (92.27%)
> >          4778060526      topdown-fe-bound          #     15.0% frontend bound          (92.27%)
> >          7086628768      topdown-retiring          #     22.2% retiring                (92.27%)
> >          1417611942      branch-instructions                                           (92.26%)
> >             5285529      branch-misses             #    0.37% of all branches          (92.28%)
> >            62922469      bus-cycles                                                    (92.29%)
> >             1440708      cache-misses              #    8.292 % of all cache refs      (92.30%)
> >            17374098      cache-references                                              (76.94%)
> >          8040889520      cpu-cycles                                                    (84.63%)
> >          7709992319      instructions              #    0.96  insn per cycle           (92.32%)
> >                   0      mem-loads                                                     (92.32%)
> >          1515669558      mem-stores                                                    (84.68%)
> >          6542411177      ref-cycles                                                    (92.35%)
> >             4154149      baclears.any                                                  (92.35%)
> >            20556152      ARITH.DIVIDER_ACTIVE                                          (84.59%)
> >
> >         1.010799593 seconds time elapsed
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >   tools/perf/arch/x86/util/evsel.c | 12 ++++++++++++
> >   tools/perf/util/evlist.c         | 16 ++++++++++++++--
> >   tools/perf/util/evsel.c          | 10 ++++++++++
> >   tools/perf/util/evsel.h          |  3 +++
> >   4 files changed, 39 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/perf/arch/x86/util/evsel.c b/tools/perf/arch/x86/util/evsel.c
> > index ac2899a25b7a..40b171de2086 100644
> > --- a/tools/perf/arch/x86/util/evsel.c
> > +++ b/tools/perf/arch/x86/util/evsel.c
> > @@ -3,6 +3,7 @@
> >   #include <stdlib.h>
> >   #include "util/evsel.h"
> >   #include "util/env.h"
> > +#include "util/pmu.h"
> >   #include "linux/string.h"
> >
> >   void arch_evsel__set_sample_weight(struct evsel *evsel)
> > @@ -29,3 +30,14 @@ void arch_evsel__fixup_new_cycles(struct perf_event_attr *attr)
> >
> >       free(env.cpuid);
> >   }
> > +
> > +bool arch_evsel__must_be_in_group(const struct evsel *evsel)
> > +{
> > +     if ((evsel->pmu_name && strcmp(evsel->pmu_name, "cpu")) ||
> > +         !pmu_have_event("cpu", "slots"))
> > +             return false;
> > +
>
> The big core of ADL also supports Perf_metrics. At least, we should
> check both "cpu" and "cpu_core" here.
>
> Thanks,
> Kan

Thanks Kan, we have the same problem for
arch_evlist__add_default_attrs and arch_evlist__leader:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/arch/x86/util/evlist.c?h=perf/core
So I think fixing all of these should be a follow up. I am working to
get access to an Alderlake system, could we land this first?

Thanks,
Ian

> > +     return evsel->name &&
> > +             (!strcasecmp(evsel->name, "slots") ||
> > +              !strncasecmp(evsel->name, "topdown", 7));
> > +}
> > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> > index 52ea004ba01e..dfa65a383502 100644
> > --- a/tools/perf/util/evlist.c
> > +++ b/tools/perf/util/evlist.c
> > @@ -1790,8 +1790,17 @@ struct evsel *evlist__reset_weak_group(struct evlist *evsel_list, struct evsel *
> >               if (evsel__has_leader(c2, leader)) {
> >                       if (is_open && close)
> >                               perf_evsel__close(&c2->core);
> > -                     evsel__set_leader(c2, c2);
> > -                     c2->core.nr_members = 0;
> > +                     /*
> > +                      * We want to close all members of the group and reopen
> > +                      * them. Some events, like Intel topdown, require being
> > +                      * in a group and so keep these in the group.
> > +                      */
> > +                     if (!evsel__must_be_in_group(c2) && c2 != leader) {
> > +                             evsel__set_leader(c2, c2);
> > +                             c2->core.nr_members = 0;
> > +                             leader->core.nr_members--;
> > +                     }
> > +
> >                       /*
> >                        * Set this for all former members of the group
> >                        * to indicate they get reopened.
> > @@ -1799,6 +1808,9 @@ struct evsel *evlist__reset_weak_group(struct evlist *evsel_list, struct evsel *
> >                       c2->reset_group = true;
> >               }
> >       }
> > +     /* Reset the leader count if all entries were removed. */
> > +     if (leader->core.nr_members)
> > +             leader->core.nr_members = 0;
> >       return leader;
> >   }
> >
> > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > index d38722560e80..b7c0c9775673 100644
> > --- a/tools/perf/util/evsel.c
> > +++ b/tools/perf/util/evsel.c
> > @@ -3082,3 +3082,13 @@ int evsel__source_count(const struct evsel *evsel)
> >       }
> >       return count;
> >   }
> > +
> > +bool __weak arch_evsel__must_be_in_group(const struct evsel *evsel __maybe_unused)
> > +{
> > +     return false;
> > +}
> > +
> > +bool evsel__must_be_in_group(const struct evsel *evsel)
> > +{
> > +     return arch_evsel__must_be_in_group(evsel);
> > +}
> > diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> > index 45d674812239..0ed2850b7ebb 100644
> > --- a/tools/perf/util/evsel.h
> > +++ b/tools/perf/util/evsel.h
> > @@ -488,6 +488,9 @@ bool evsel__has_leader(struct evsel *evsel, struct evsel *leader);
> >   bool evsel__is_leader(struct evsel *evsel);
> >   void evsel__set_leader(struct evsel *evsel, struct evsel *leader);
> >   int evsel__source_count(const struct evsel *evsel);
> > +bool evsel__must_be_in_group(const struct evsel *evsel);
> > +
> > +bool arch_evsel__must_be_in_group(const struct evsel *evsel);
> >
> >   /*
> >    * Macro to swap the bit-field postition and size.

Re: [PATCH 1/2] perf evlist: Keep topdown counters in weak group

[Liang, Kan]

On 5/5/2022 11:18 AM, Ian Rogers wrote:
> On Thu, May 5, 2022 at 4:56 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>>
>> Hi Ian,
>>
>> Thanks for the patches.
>>
>> On 5/5/2022 12:38 AM, Ian Rogers wrote:
>>> On Intel Icelake, topdown events must always be grouped with a slots
>>> event as leader. When a metric is parsed a weak group is formed and
>>> retried if perf_event_open fails. The retried events aren't grouped
>>> breaking the slots leader requirement. This change modifies the weak
>>> group "reset" behavior so that topdown events aren't broken from the
>>> group for the retry.
>>>
>>> $ perf stat -e '{slots,topdown-bad-spec,topdown-be-bound,topdown-fe-bound,topdown-retiring,branch-instructions,branch-misses,bus-cycles,cache-misses,cache-references,cpu-cycles,instructions,mem-loads,mem-stores,ref-cycles,baclears.any,ARITH.DIVIDER_ACTIVE}:W' -a sleep 1
>>>
>>>    Performance counter stats for 'system wide':
>>>
>>>       47,867,188,483      slots                                                         (92.27%)
>>>      <not supported>      topdown-bad-spec
>>>      <not supported>      topdown-be-bound
>>>      <not supported>      topdown-fe-bound
>>>      <not supported>      topdown-retiring
>>>        2,173,346,937      branch-instructions                                           (92.27%)
>>>           10,540,253      branch-misses             #    0.48% of all branches          (92.29%)
>>>           96,291,140      bus-cycles                                                    (92.29%)
>>>            6,214,202      cache-misses              #   20.120 % of all cache refs      (92.29%)
>>>           30,886,082      cache-references                                              (76.91%)
>>>       11,773,726,641      cpu-cycles                                                    (84.62%)
>>>       11,807,585,307      instructions              #    1.00  insn per cycle           (92.31%)
>>>                    0      mem-loads                                                     (92.32%)
>>>        2,212,928,573      mem-stores                                                    (84.69%)
>>>       10,024,403,118      ref-cycles                                                    (92.35%)
>>>           16,232,978      baclears.any                                                  (92.35%)
>>>           23,832,633      ARITH.DIVIDER_ACTIVE                                          (84.59%)
>>>
>>>          0.981070734 seconds time elapsed
>>>
>>> After:
>>>
>>> $ perf stat -e '{slots,topdown-bad-spec,topdown-be-bound,topdown-fe-bound,topdown-retiring,branch-instructions,branch-misses,bus-cycles,cache-misses,cache-references,cpu-cycles,instructions,mem-loads,mem-stores,ref-cycles,baclears.any,ARITH.DIVIDER_ACTIVE}:W' -a sleep 1
>>>
>>>    Performance counter stats for 'system wide':
>>>
>>>          31040189283      slots                                                         (92.27%)
>>>           8997514811      topdown-bad-spec          #     28.2% bad speculation         (92.27%)
>>>          10997536028      topdown-be-bound          #     34.5% backend bound           (92.27%)
>>>           4778060526      topdown-fe-bound          #     15.0% frontend bound          (92.27%)
>>>           7086628768      topdown-retiring          #     22.2% retiring                (92.27%)
>>>           1417611942      branch-instructions                                           (92.26%)
>>>              5285529      branch-misses             #    0.37% of all branches          (92.28%)
>>>             62922469      bus-cycles                                                    (92.29%)
>>>              1440708      cache-misses              #    8.292 % of all cache refs      (92.30%)
>>>             17374098      cache-references                                              (76.94%)
>>>           8040889520      cpu-cycles                                                    (84.63%)
>>>           7709992319      instructions              #    0.96  insn per cycle           (92.32%)
>>>                    0      mem-loads                                                     (92.32%)
>>>           1515669558      mem-stores                                                    (84.68%)
>>>           6542411177      ref-cycles                                                    (92.35%)
>>>              4154149      baclears.any                                                  (92.35%)
>>>             20556152      ARITH.DIVIDER_ACTIVE                                          (84.59%)
>>>
>>>          1.010799593 seconds time elapsed
>>>
>>> Signed-off-by: Ian Rogers <irogers@google.com>
>>> ---
>>>    tools/perf/arch/x86/util/evsel.c | 12 ++++++++++++
>>>    tools/perf/util/evlist.c         | 16 ++++++++++++++--
>>>    tools/perf/util/evsel.c          | 10 ++++++++++
>>>    tools/perf/util/evsel.h          |  3 +++
>>>    4 files changed, 39 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tools/perf/arch/x86/util/evsel.c b/tools/perf/arch/x86/util/evsel.c
>>> index ac2899a25b7a..40b171de2086 100644
>>> --- a/tools/perf/arch/x86/util/evsel.c
>>> +++ b/tools/perf/arch/x86/util/evsel.c
>>> @@ -3,6 +3,7 @@
>>>    #include <stdlib.h>
>>>    #include "util/evsel.h"
>>>    #include "util/env.h"
>>> +#include "util/pmu.h"
>>>    #include "linux/string.h"
>>>
>>>    void arch_evsel__set_sample_weight(struct evsel *evsel)
>>> @@ -29,3 +30,14 @@ void arch_evsel__fixup_new_cycles(struct perf_event_attr *attr)
>>>
>>>        free(env.cpuid);
>>>    }
>>> +
>>> +bool arch_evsel__must_be_in_group(const struct evsel *evsel)
>>> +{
>>> +     if ((evsel->pmu_name && strcmp(evsel->pmu_name, "cpu")) ||
>>> +         !pmu_have_event("cpu", "slots"))
>>> +             return false;
>>> +
>>
>> The big core of ADL also supports Perf_metrics. At least, we should
>> check both "cpu" and "cpu_core" here.
>>
>> Thanks,
>> Kan
> 
> Thanks Kan, we have the same problem for
> arch_evlist__add_default_attrs and arch_evlist__leader:
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/arch/x86/util/evlist.c?h=perf/core

These are known issues. Zhengjun already have some patches to fix them, 
but hasn't posted yet.

The arch_evlist__add_default_attrs impacts the perf stat default. On the 
ADL, perf stat default doesn't display the topdown events.

The arch_evlist__leader impacts the sampling read of the topdown events. 
Sampling read the topdown events should always fail with the current 
perf tool on ADL.

We already have many backlogs on ADL. I think it's better not to 
introduce more issues.

> So I think fixing all of these should be a follow up. I am working to
> get access to an Alderlake system, could we land this first?
> 

I think we can use pmu_name to replace the "cpu" to fix the issue for 
the hybrid platform. For a hybrid platform, the pmu_name is either 
cpu_atom or cpu_core.

Besides, the topdown events may have a PMU prefix, e.g., 
cpu_core/topdown-be-bound/. The strcasecmp may not work well for this case.

How about the below patch?
If it's OK for you, could you please merge it into your V2 patch set?
I can do the test on a ADL system.

diff --git a/tools/perf/arch/x86/util/evsel.c 
b/tools/perf/arch/x86/util/evsel.c
index 40b171de2086..551ae2bab70e 100644
--- a/tools/perf/arch/x86/util/evsel.c
+++ b/tools/perf/arch/x86/util/evsel.c
@@ -33,11 +33,12 @@ void arch_evsel__fixup_new_cycles(struct 
perf_event_attr *attr)

  bool arch_evsel__must_be_in_group(const struct evsel *evsel)
  {
-	if ((evsel->pmu_name && strcmp(evsel->pmu_name, "cpu")) ||
-	    !pmu_have_event("cpu", "slots"))
+	const char *pmu_name = evsel->pmu_name ? evsel->pmu_name : "cpu";
+
+	if (!pmu_have_event(pmu_name, "slots"))
  		return false;

  	return evsel->name &&
-		(!strcasecmp(evsel->name, "slots") ||
-		 !strncasecmp(evsel->name, "topdown", 7));
+		(strcasestr(evsel->name, "slots") ||
+		 strcasestr(evsel->name, "topdown"));
  }



Thanks,
Kan

> Thanks,
> Ian
> 
>>> +     return evsel->name &&
>>> +             (!strcasecmp(evsel->name, "slots") ||
>>> +              !strncasecmp(evsel->name, "topdown", 7));
>>> +}
>>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>>> index 52ea004ba01e..dfa65a383502 100644
>>> --- a/tools/perf/util/evlist.c
>>> +++ b/tools/perf/util/evlist.c
>>> @@ -1790,8 +1790,17 @@ struct evsel *evlist__reset_weak_group(struct evlist *evsel_list, struct evsel *
>>>                if (evsel__has_leader(c2, leader)) {
>>>                        if (is_open && close)
>>>                                perf_evsel__close(&c2->core);
>>> -                     evsel__set_leader(c2, c2);
>>> -                     c2->core.nr_members = 0;
>>> +                     /*
>>> +                      * We want to close all members of the group and reopen
>>> +                      * them. Some events, like Intel topdown, require being
>>> +                      * in a group and so keep these in the group.
>>> +                      */
>>> +                     if (!evsel__must_be_in_group(c2) && c2 != leader) {
>>> +                             evsel__set_leader(c2, c2);
>>> +                             c2->core.nr_members = 0;
>>> +                             leader->core.nr_members--;
>>> +                     }
>>> +
>>>                        /*
>>>                         * Set this for all former members of the group
>>>                         * to indicate they get reopened.
>>> @@ -1799,6 +1808,9 @@ struct evsel *evlist__reset_weak_group(struct evlist *evsel_list, struct evsel *
>>>                        c2->reset_group = true;
>>>                }
>>>        }
>>> +     /* Reset the leader count if all entries were removed. */
>>> +     if (leader->core.nr_members)
>>> +             leader->core.nr_members = 0;
>>>        return leader;
>>>    }
>>>
>>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
>>> index d38722560e80..b7c0c9775673 100644
>>> --- a/tools/perf/util/evsel.c
>>> +++ b/tools/perf/util/evsel.c
>>> @@ -3082,3 +3082,13 @@ int evsel__source_count(const struct evsel *evsel)
>>>        }
>>>        return count;
>>>    }
>>> +
>>> +bool __weak arch_evsel__must_be_in_group(const struct evsel *evsel __maybe_unused)
>>> +{
>>> +     return false;
>>> +}
>>> +
>>> +bool evsel__must_be_in_group(const struct evsel *evsel)
>>> +{
>>> +     return arch_evsel__must_be_in_group(evsel);
>>> +}
>>> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
>>> index 45d674812239..0ed2850b7ebb 100644
>>> --- a/tools/perf/util/evsel.h
>>> +++ b/tools/perf/util/evsel.h
>>> @@ -488,6 +488,9 @@ bool evsel__has_leader(struct evsel *evsel, struct evsel *leader);
>>>    bool evsel__is_leader(struct evsel *evsel);
>>>    void evsel__set_leader(struct evsel *evsel, struct evsel *leader);
>>>    int evsel__source_count(const struct evsel *evsel);
>>> +bool evsel__must_be_in_group(const struct evsel *evsel);
>>> +
>>> +bool arch_evsel__must_be_in_group(const struct evsel *evsel);
>>>
>>>    /*
>>>     * Macro to swap the bit-field postition and size.

Re: [PATCH 1/2] perf evlist: Keep topdown counters in weak group

[Ian Rogers]

On Thu, May 5, 2022 at 11:15 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
> On 5/5/2022 11:18 AM, Ian Rogers wrote:
> > On Thu, May 5, 2022 at 4:56 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
> >>
> >> Hi Ian,
> >>
> >> Thanks for the patches.
> >>
> >> On 5/5/2022 12:38 AM, Ian Rogers wrote:
> >>> On Intel Icelake, topdown events must always be grouped with a slots
> >>> event as leader. When a metric is parsed a weak group is formed and
> >>> retried if perf_event_open fails. The retried events aren't grouped
> >>> breaking the slots leader requirement. This change modifies the weak
> >>> group "reset" behavior so that topdown events aren't broken from the
> >>> group for the retry.
> >>>
> >>> $ perf stat -e '{slots,topdown-bad-spec,topdown-be-bound,topdown-fe-bound,topdown-retiring,branch-instructions,branch-misses,bus-cycles,cache-misses,cache-references,cpu-cycles,instructions,mem-loads,mem-stores,ref-cycles,baclears.any,ARITH.DIVIDER_ACTIVE}:W' -a sleep 1
> >>>
> >>>    Performance counter stats for 'system wide':
> >>>
> >>>       47,867,188,483      slots                                                         (92.27%)
> >>>      <not supported>      topdown-bad-spec
> >>>      <not supported>      topdown-be-bound
> >>>      <not supported>      topdown-fe-bound
> >>>      <not supported>      topdown-retiring
> >>>        2,173,346,937      branch-instructions                                           (92.27%)
> >>>           10,540,253      branch-misses             #    0.48% of all branches          (92.29%)
> >>>           96,291,140      bus-cycles                                                    (92.29%)
> >>>            6,214,202      cache-misses              #   20.120 % of all cache refs      (92.29%)
> >>>           30,886,082      cache-references                                              (76.91%)
> >>>       11,773,726,641      cpu-cycles                                                    (84.62%)
> >>>       11,807,585,307      instructions              #    1.00  insn per cycle           (92.31%)
> >>>                    0      mem-loads                                                     (92.32%)
> >>>        2,212,928,573      mem-stores                                                    (84.69%)
> >>>       10,024,403,118      ref-cycles                                                    (92.35%)
> >>>           16,232,978      baclears.any                                                  (92.35%)
> >>>           23,832,633      ARITH.DIVIDER_ACTIVE                                          (84.59%)
> >>>
> >>>          0.981070734 seconds time elapsed
> >>>
> >>> After:
> >>>
> >>> $ perf stat -e '{slots,topdown-bad-spec,topdown-be-bound,topdown-fe-bound,topdown-retiring,branch-instructions,branch-misses,bus-cycles,cache-misses,cache-references,cpu-cycles,instructions,mem-loads,mem-stores,ref-cycles,baclears.any,ARITH.DIVIDER_ACTIVE}:W' -a sleep 1
> >>>
> >>>    Performance counter stats for 'system wide':
> >>>
> >>>          31040189283      slots                                                         (92.27%)
> >>>           8997514811      topdown-bad-spec          #     28.2% bad speculation         (92.27%)
> >>>          10997536028      topdown-be-bound          #     34.5% backend bound           (92.27%)
> >>>           4778060526      topdown-fe-bound          #     15.0% frontend bound          (92.27%)
> >>>           7086628768      topdown-retiring          #     22.2% retiring                (92.27%)
> >>>           1417611942      branch-instructions                                           (92.26%)
> >>>              5285529      branch-misses             #    0.37% of all branches          (92.28%)
> >>>             62922469      bus-cycles                                                    (92.29%)
> >>>              1440708      cache-misses              #    8.292 % of all cache refs      (92.30%)
> >>>             17374098      cache-references                                              (76.94%)
> >>>           8040889520      cpu-cycles                                                    (84.63%)
> >>>           7709992319      instructions              #    0.96  insn per cycle           (92.32%)
> >>>                    0      mem-loads                                                     (92.32%)
> >>>           1515669558      mem-stores                                                    (84.68%)
> >>>           6542411177      ref-cycles                                                    (92.35%)
> >>>              4154149      baclears.any                                                  (92.35%)
> >>>             20556152      ARITH.DIVIDER_ACTIVE                                          (84.59%)
> >>>
> >>>          1.010799593 seconds time elapsed
> >>>
> >>> Signed-off-by: Ian Rogers <irogers@google.com>
> >>> ---
> >>>    tools/perf/arch/x86/util/evsel.c | 12 ++++++++++++
> >>>    tools/perf/util/evlist.c         | 16 ++++++++++++++--
> >>>    tools/perf/util/evsel.c          | 10 ++++++++++
> >>>    tools/perf/util/evsel.h          |  3 +++
> >>>    4 files changed, 39 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/tools/perf/arch/x86/util/evsel.c b/tools/perf/arch/x86/util/evsel.c
> >>> index ac2899a25b7a..40b171de2086 100644
> >>> --- a/tools/perf/arch/x86/util/evsel.c
> >>> +++ b/tools/perf/arch/x86/util/evsel.c
> >>> @@ -3,6 +3,7 @@
> >>>    #include <stdlib.h>
> >>>    #include "util/evsel.h"
> >>>    #include "util/env.h"
> >>> +#include "util/pmu.h"
> >>>    #include "linux/string.h"
> >>>
> >>>    void arch_evsel__set_sample_weight(struct evsel *evsel)
> >>> @@ -29,3 +30,14 @@ void arch_evsel__fixup_new_cycles(struct perf_event_attr *attr)
> >>>
> >>>        free(env.cpuid);
> >>>    }
> >>> +
> >>> +bool arch_evsel__must_be_in_group(const struct evsel *evsel)
> >>> +{
> >>> +     if ((evsel->pmu_name && strcmp(evsel->pmu_name, "cpu")) ||
> >>> +         !pmu_have_event("cpu", "slots"))
> >>> +             return false;
> >>> +
> >>
> >> The big core of ADL also supports Perf_metrics. At least, we should
> >> check both "cpu" and "cpu_core" here.
> >>
> >> Thanks,
> >> Kan
> >
> > Thanks Kan, we have the same problem for
> > arch_evlist__add_default_attrs and arch_evlist__leader:
> > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/arch/x86/util/evlist.c?h=perf/core
>
> These are known issues. Zhengjun already have some patches to fix them,
> but hasn't posted yet.
>
> The arch_evlist__add_default_attrs impacts the perf stat default. On the
> ADL, perf stat default doesn't display the topdown events.
>
> The arch_evlist__leader impacts the sampling read of the topdown events.
> Sampling read the topdown events should always fail with the current
> perf tool on ADL.
>
> We already have many backlogs on ADL. I think it's better not to
> introduce more issues.
>
> > So I think fixing all of these should be a follow up. I am working to
> > get access to an Alderlake system, could we land this first?
> >
>
> I think we can use pmu_name to replace the "cpu" to fix the issue for
> the hybrid platform. For a hybrid platform, the pmu_name is either
> cpu_atom or cpu_core.
>
> Besides, the topdown events may have a PMU prefix, e.g.,
> cpu_core/topdown-be-bound/. The strcasecmp may not work well for this case.
>
> How about the below patch?
> If it's OK for you, could you please merge it into your V2 patch set?
> I can do the test on a ADL system.
>
> diff --git a/tools/perf/arch/x86/util/evsel.c
> b/tools/perf/arch/x86/util/evsel.c
> index 40b171de2086..551ae2bab70e 100644
> --- a/tools/perf/arch/x86/util/evsel.c
> +++ b/tools/perf/arch/x86/util/evsel.c
> @@ -33,11 +33,12 @@ void arch_evsel__fixup_new_cycles(struct
> perf_event_attr *attr)
>
>   bool arch_evsel__must_be_in_group(const struct evsel *evsel)
>   {
> -       if ((evsel->pmu_name && strcmp(evsel->pmu_name, "cpu")) ||
> -           !pmu_have_event("cpu", "slots"))
> +       const char *pmu_name = evsel->pmu_name ? evsel->pmu_name : "cpu";
> +
> +       if (!pmu_have_event(pmu_name, "slots"))
>                 return false;

Hmm. The idea with this test is to see if the architecture supports
topdown events before going further. There's a similar test in all the
arch_evlist functions. I think with cpu_core this needs to become:

if (!pmu_have_event("cpu", "slots") && !pmu_have_event("cpu_core", "slots") )

But we should add a helper function for this. It is odd to have this
change supporting Alderlake but the existing evlist work not. Perhaps
we should just wait until Zhengjun's patches land.

Thanks,
Ian

>         return evsel->name &&
> -               (!strcasecmp(evsel->name, "slots") ||
> -                !strncasecmp(evsel->name, "topdown", 7));
> +               (strcasestr(evsel->name, "slots") ||
> +                strcasestr(evsel->name, "topdown"));
>   }
>
>
>
> Thanks,
> Kan
>
> > Thanks,
> > Ian
> >
> >>> +     return evsel->name &&
> >>> +             (!strcasecmp(evsel->name, "slots") ||
> >>> +              !strncasecmp(evsel->name, "topdown", 7));
> >>> +}
> >>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> >>> index 52ea004ba01e..dfa65a383502 100644
> >>> --- a/tools/perf/util/evlist.c
> >>> +++ b/tools/perf/util/evlist.c
> >>> @@ -1790,8 +1790,17 @@ struct evsel *evlist__reset_weak_group(struct evlist *evsel_list, struct evsel *
> >>>                if (evsel__has_leader(c2, leader)) {
> >>>                        if (is_open && close)
> >>>                                perf_evsel__close(&c2->core);
> >>> -                     evsel__set_leader(c2, c2);
> >>> -                     c2->core.nr_members = 0;
> >>> +                     /*
> >>> +                      * We want to close all members of the group and reopen
> >>> +                      * them. Some events, like Intel topdown, require being
> >>> +                      * in a group and so keep these in the group.
> >>> +                      */
> >>> +                     if (!evsel__must_be_in_group(c2) && c2 != leader) {
> >>> +                             evsel__set_leader(c2, c2);
> >>> +                             c2->core.nr_members = 0;
> >>> +                             leader->core.nr_members--;
> >>> +                     }
> >>> +
> >>>                        /*
> >>>                         * Set this for all former members of the group
> >>>                         * to indicate they get reopened.
> >>> @@ -1799,6 +1808,9 @@ struct evsel *evlist__reset_weak_group(struct evlist *evsel_list, struct evsel *
> >>>                        c2->reset_group = true;
> >>>                }
> >>>        }
> >>> +     /* Reset the leader count if all entries were removed. */
> >>> +     if (leader->core.nr_members)
> >>> +             leader->core.nr_members = 0;
> >>>        return leader;
> >>>    }
> >>>
> >>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> >>> index d38722560e80..b7c0c9775673 100644
> >>> --- a/tools/perf/util/evsel.c
> >>> +++ b/tools/perf/util/evsel.c
> >>> @@ -3082,3 +3082,13 @@ int evsel__source_count(const struct evsel *evsel)
> >>>        }
> >>>        return count;
> >>>    }
> >>> +
> >>> +bool __weak arch_evsel__must_be_in_group(const struct evsel *evsel __maybe_unused)
> >>> +{
> >>> +     return false;
> >>> +}
> >>> +
> >>> +bool evsel__must_be_in_group(const struct evsel *evsel)
> >>> +{
> >>> +     return arch_evsel__must_be_in_group(evsel);
> >>> +}
> >>> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> >>> index 45d674812239..0ed2850b7ebb 100644
> >>> --- a/tools/perf/util/evsel.h
> >>> +++ b/tools/perf/util/evsel.h
> >>> @@ -488,6 +488,9 @@ bool evsel__has_leader(struct evsel *evsel, struct evsel *leader);
> >>>    bool evsel__is_leader(struct evsel *evsel);
> >>>    void evsel__set_leader(struct evsel *evsel, struct evsel *leader);
> >>>    int evsel__source_count(const struct evsel *evsel);
> >>> +bool evsel__must_be_in_group(const struct evsel *evsel);
> >>> +
> >>> +bool arch_evsel__must_be_in_group(const struct evsel *evsel);
> >>>
> >>>    /*
> >>>     * Macro to swap the bit-field postition and size.

Re: [PATCH 1/2] perf evlist: Keep topdown counters in weak group

[Liang, Kan]

On 5/5/2022 2:31 PM, Ian Rogers wrote:
>>> So I think fixing all of these should be a follow up. I am working to
>>> get access to an Alderlake system, could we land this first?
>>>
>> I think we can use pmu_name to replace the "cpu" to fix the issue for
>> the hybrid platform. For a hybrid platform, the pmu_name is either
>> cpu_atom or cpu_core.
>>
>> Besides, the topdown events may have a PMU prefix, e.g.,
>> cpu_core/topdown-be-bound/. The strcasecmp may not work well for this case.
>>
>> How about the below patch?
>> If it's OK for you, could you please merge it into your V2 patch set?
>> I can do the test on a ADL system.
>>
>> diff --git a/tools/perf/arch/x86/util/evsel.c
>> b/tools/perf/arch/x86/util/evsel.c
>> index 40b171de2086..551ae2bab70e 100644
>> --- a/tools/perf/arch/x86/util/evsel.c
>> +++ b/tools/perf/arch/x86/util/evsel.c
>> @@ -33,11 +33,12 @@ void arch_evsel__fixup_new_cycles(struct
>> perf_event_attr *attr)
>>
>>    bool arch_evsel__must_be_in_group(const struct evsel *evsel)
>>    {
>> -       if ((evsel->pmu_name && strcmp(evsel->pmu_name, "cpu")) ||
>> -           !pmu_have_event("cpu", "slots"))
>> +       const char *pmu_name = evsel->pmu_name ? evsel->pmu_name : "cpu";
>> +
>> +       if (!pmu_have_event(pmu_name, "slots"))
>>                  return false;
> Hmm. The idea with this test is to see if the architecture supports
> topdown events before going further. There's a similar test in all the
> arch_evlist functions. I think with cpu_core this needs to become:
>

The case is a little bit different here. For the arch_evlist functions, 
the input is the evlist, not the specific evsel. So we have to check all 
the possible PMU names which are "cpu" and "cpu_core". Then we decide 
whether going further.

The input of the evsel__must_be_in_group() is the evsel. The PMU name is 
stored in the evsel->pmu_name. I don't think we need to check all the 
possible PMU names. Using evsel->pmu_name should be good enough.

> if (!pmu_have_event("cpu", "slots") && !pmu_have_event("cpu_core", "slots") )
> 
> But we should add a helper function for this. It is odd to have this
> change supporting Alderlake but the existing evlist work not. Perhaps
> we should just wait until Zhengjun's patches land.

Yes, a helper function is good for the arch_evlist functions. But I 
don't think this patch needs the helper function. Zhengjun's patches are 
to fix the other topdown issues on ADL. There is no dependency between 
this patch and zhengjun's patches.

Thanks,
Kan

Re: [PATCH 1/2] perf evlist: Keep topdown counters in weak group

[Ian Rogers]

On Thu, May 5, 2022 at 12:44 PM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
>
>
> On 5/5/2022 2:31 PM, Ian Rogers wrote:
> >>> So I think fixing all of these should be a follow up. I am working to
> >>> get access to an Alderlake system, could we land this first?
> >>>
> >> I think we can use pmu_name to replace the "cpu" to fix the issue for
> >> the hybrid platform. For a hybrid platform, the pmu_name is either
> >> cpu_atom or cpu_core.
> >>
> >> Besides, the topdown events may have a PMU prefix, e.g.,
> >> cpu_core/topdown-be-bound/. The strcasecmp may not work well for this case.
> >>
> >> How about the below patch?
> >> If it's OK for you, could you please merge it into your V2 patch set?
> >> I can do the test on a ADL system.
> >>
> >> diff --git a/tools/perf/arch/x86/util/evsel.c
> >> b/tools/perf/arch/x86/util/evsel.c
> >> index 40b171de2086..551ae2bab70e 100644
> >> --- a/tools/perf/arch/x86/util/evsel.c
> >> +++ b/tools/perf/arch/x86/util/evsel.c
> >> @@ -33,11 +33,12 @@ void arch_evsel__fixup_new_cycles(struct
> >> perf_event_attr *attr)
> >>
> >>    bool arch_evsel__must_be_in_group(const struct evsel *evsel)
> >>    {
> >> -       if ((evsel->pmu_name && strcmp(evsel->pmu_name, "cpu")) ||
> >> -           !pmu_have_event("cpu", "slots"))
> >> +       const char *pmu_name = evsel->pmu_name ? evsel->pmu_name : "cpu";
> >> +
> >> +       if (!pmu_have_event(pmu_name, "slots"))
> >>                  return false;
> > Hmm. The idea with this test is to see if the architecture supports
> > topdown events before going further. There's a similar test in all the
> > arch_evlist functions. I think with cpu_core this needs to become:
> >
>
> The case is a little bit different here. For the arch_evlist functions,
> the input is the evlist, not the specific evsel. So we have to check all
> the possible PMU names which are "cpu" and "cpu_core". Then we decide
> whether going further.
>
> The input of the evsel__must_be_in_group() is the evsel. The PMU name is
> stored in the evsel->pmu_name. I don't think we need to check all the
> possible PMU names. Using evsel->pmu_name should be good enough.
>
> > if (!pmu_have_event("cpu", "slots") && !pmu_have_event("cpu_core", "slots") )
> >
> > But we should add a helper function for this. It is odd to have this
> > change supporting Alderlake but the existing evlist work not. Perhaps
> > we should just wait until Zhengjun's patches land.
>
> Yes, a helper function is good for the arch_evlist functions. But I
> don't think this patch needs the helper function. Zhengjun's patches are
> to fix the other topdown issues on ADL. There is no dependency between
> this patch and zhengjun's patches.
>
> Thanks,
> Kan

TL;DR I think we can move forward with landing these patches to fix Icelake.

For Alderlake/hybrid we have a problem. To determine what happens with
grouping we need to know does the CPU have topdown events? This is a
runtime question for doing perf_event_open and so an arch test and
weak symbol are appropriate. For Icelake we are determining the
presence of topdown events by looking at the special PMU cpu. For
Alderlake the same information can be found by looking at the PMUs
cpu_core and cpu_atom, but how to discover those PMU names? It is
already somewhat concerning that we've hard coded "cpu" and we don't
want to have an ever growing list of PMU names.

We have similarly hard coded "cpu" in the topology code here:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/cputopo.c?h=tmp.perf/core#n18
Is this unreasonable given cpu is already supposed to be ABI stable:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/Documentation/ABI/stable/sysfs-devices-system-cpu?h=tmp.perf/core

It is hard to say what the right hybrid fix is here. I should get a
system I can poke shortly. I'd also like to compare what's in sysfs
for Alderlake with ARM's big.little approach. I can imagine we need a
function that returns a list of CPU like PMUs for probing. Ideally we
could work this out from sysfs and use some stable ABI.

Thanks,
Ian

Re: [PATCH 1/2] perf evlist: Keep topdown counters in weak group

[Liang, Kan]

On 5/9/2022 1:28 PM, Ian Rogers wrote:
> On Thu, May 5, 2022 at 12:44 PM Liang, Kan <kan.liang@linux.intel.com> wrote:
>>
>>
>>
>> On 5/5/2022 2:31 PM, Ian Rogers wrote:
>>>>> So I think fixing all of these should be a follow up. I am working to
>>>>> get access to an Alderlake system, could we land this first?
>>>>>
>>>> I think we can use pmu_name to replace the "cpu" to fix the issue for
>>>> the hybrid platform. For a hybrid platform, the pmu_name is either
>>>> cpu_atom or cpu_core.
>>>>
>>>> Besides, the topdown events may have a PMU prefix, e.g.,
>>>> cpu_core/topdown-be-bound/. The strcasecmp may not work well for this case.
>>>>
>>>> How about the below patch?
>>>> If it's OK for you, could you please merge it into your V2 patch set?
>>>> I can do the test on a ADL system.
>>>>
>>>> diff --git a/tools/perf/arch/x86/util/evsel.c
>>>> b/tools/perf/arch/x86/util/evsel.c
>>>> index 40b171de2086..551ae2bab70e 100644
>>>> --- a/tools/perf/arch/x86/util/evsel.c
>>>> +++ b/tools/perf/arch/x86/util/evsel.c
>>>> @@ -33,11 +33,12 @@ void arch_evsel__fixup_new_cycles(struct
>>>> perf_event_attr *attr)
>>>>
>>>>     bool arch_evsel__must_be_in_group(const struct evsel *evsel)
>>>>     {
>>>> -       if ((evsel->pmu_name && strcmp(evsel->pmu_name, "cpu")) ||
>>>> -           !pmu_have_event("cpu", "slots"))
>>>> +       const char *pmu_name = evsel->pmu_name ? evsel->pmu_name : "cpu";
>>>> +
>>>> +       if (!pmu_have_event(pmu_name, "slots"))
>>>>                   return false;
>>> Hmm. The idea with this test is to see if the architecture supports
>>> topdown events before going further. There's a similar test in all the
>>> arch_evlist functions. I think with cpu_core this needs to become:
>>>
>>
>> The case is a little bit different here. For the arch_evlist functions,
>> the input is the evlist, not the specific evsel. So we have to check all
>> the possible PMU names which are "cpu" and "cpu_core". Then we decide
>> whether going further.
>>
>> The input of the evsel__must_be_in_group() is the evsel. The PMU name is
>> stored in the evsel->pmu_name. I don't think we need to check all the
>> possible PMU names. Using evsel->pmu_name should be good enough.
>>
>>> if (!pmu_have_event("cpu", "slots") && !pmu_have_event("cpu_core", "slots") )
>>>
>>> But we should add a helper function for this. It is odd to have this
>>> change supporting Alderlake but the existing evlist work not. Perhaps
>>> we should just wait until Zhengjun's patches land.
>>
>> Yes, a helper function is good for the arch_evlist functions. But I
>> don't think this patch needs the helper function. Zhengjun's patches are
>> to fix the other topdown issues on ADL. There is no dependency between
>> this patch and zhengjun's patches.
>>
>> Thanks,
>> Kan
> 
> TL;DR I think we can move forward with landing these patches to fix Icelake.

This patch doesn't work with the hybrid platform for sure. I can send 
you a fix for the hybrid part if you prefer this way? Then I guess you 
may append it as the patch 3 for V2.

Besides the hybrid thing, the patch set also has other two issues I 
mentioned in the previous reply.
- I don't think the strcasecmp() can handle the case like 
cpu/topdown-bad-spec/ or cpu/slots/. It should be an issue for both 
hybrid and non-hybrid platforms.
- It's better not to use non-architecture events, e.g., baclears.any, 
ARITH.DIVIDER_ACTIVE, even in the test case. The non-architecture events 
may be disappear in the future platforms. If so, you have to update the 
test case again for the future platforms.
IMHO, I don't think the patch set is ready.

> 
> For Alderlake/hybrid we have a problem. To determine what happens with
> grouping we need to know does the CPU have topdown events? This is a
> runtime question for doing perf_event_open and so an arch test and
> weak symbol are appropriate. For Icelake we are determining the
> presence of topdown events by looking at the special PMU cpu. For
> Alderlake the same information can be found by looking at the PMUs
> cpu_core and cpu_atom, but how to discover those PMU names?

The PMU name can be retrieved either from the event list or perf command.
For the non-hybrid, the PMU name is hard code to "cpu" for the core 
events. So users/event files don't need to specify the PMU name.
For the hybrid platform, a PMU name is required and stored in the 
evsel->pmu_name. If the evsel->pmu_name is NULL, we can assume that it's 
a non-hybrid PMU, CPU.

> It is
> already somewhat concerning that we've hard coded "cpu" and we don't
> want to have an ever growing list of PMU names.
> 
> We have similarly hard coded "cpu" in the topology code here:
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/cputopo.c?h=tmp.perf/core#n18
> Is this unreasonable given cpu is already supposed to be ABI stable:
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/Documentation/ABI/stable/sysfs-devices-system-cpu?h=tmp.perf/core

I don't think there is a stable ABI for a PMU name.
The PMU name may be changed generation by generation because of the 
different micro arch. We will try our best to keep it unchanged in X86 
but it's not guaranteed especially when the hybrid is introduced.


> 
> It is hard to say what the right hybrid fix is here. I should get a
> system I can poke shortly. I'd also like to compare what's in sysfs
> for Alderlake with ARM's big.little approach. I can imagine we need a
> function that returns a list of CPU like PMUs for probing. Ideally we
> could work this out from sysfs and use some stable ABI.
>

I don't think there is a standard PMU naming rule for all the ARCHs. For 
X86, it may be possible. You can assume that the name like "cpu" or 
"cpu_*" are for core PMUs. But for other ARCH e.g., ARM, AFAIK, they use 
a quite different naming rule.


Thanks,
Kan

Re: [PATCH 1/2] perf evlist: Keep topdown counters in weak group

[Ian Rogers]

On Mon, May 9, 2022 at 2:01 PM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
> On 5/9/2022 1:28 PM, Ian Rogers wrote:
> > On Thu, May 5, 2022 at 12:44 PM Liang, Kan <kan.liang@linux.intel.com> wrote:
> >>
> >>
> >>
> >> On 5/5/2022 2:31 PM, Ian Rogers wrote:
> >>>>> So I think fixing all of these should be a follow up. I am working to
> >>>>> get access to an Alderlake system, could we land this first?
> >>>>>
> >>>> I think we can use pmu_name to replace the "cpu" to fix the issue for
> >>>> the hybrid platform. For a hybrid platform, the pmu_name is either
> >>>> cpu_atom or cpu_core.
> >>>>
> >>>> Besides, the topdown events may have a PMU prefix, e.g.,
> >>>> cpu_core/topdown-be-bound/. The strcasecmp may not work well for this case.
> >>>>
> >>>> How about the below patch?
> >>>> If it's OK for you, could you please merge it into your V2 patch set?
> >>>> I can do the test on a ADL system.
> >>>>
> >>>> diff --git a/tools/perf/arch/x86/util/evsel.c
> >>>> b/tools/perf/arch/x86/util/evsel.c
> >>>> index 40b171de2086..551ae2bab70e 100644
> >>>> --- a/tools/perf/arch/x86/util/evsel.c
> >>>> +++ b/tools/perf/arch/x86/util/evsel.c
> >>>> @@ -33,11 +33,12 @@ void arch_evsel__fixup_new_cycles(struct
> >>>> perf_event_attr *attr)
> >>>>
> >>>>     bool arch_evsel__must_be_in_group(const struct evsel *evsel)
> >>>>     {
> >>>> -       if ((evsel->pmu_name && strcmp(evsel->pmu_name, "cpu")) ||
> >>>> -           !pmu_have_event("cpu", "slots"))
> >>>> +       const char *pmu_name = evsel->pmu_name ? evsel->pmu_name : "cpu";
> >>>> +
> >>>> +       if (!pmu_have_event(pmu_name, "slots"))
> >>>>                   return false;
> >>> Hmm. The idea with this test is to see if the architecture supports
> >>> topdown events before going further. There's a similar test in all the
> >>> arch_evlist functions. I think with cpu_core this needs to become:
> >>>
> >>
> >> The case is a little bit different here. For the arch_evlist functions,
> >> the input is the evlist, not the specific evsel. So we have to check all
> >> the possible PMU names which are "cpu" and "cpu_core". Then we decide
> >> whether going further.
> >>
> >> The input of the evsel__must_be_in_group() is the evsel. The PMU name is
> >> stored in the evsel->pmu_name. I don't think we need to check all the
> >> possible PMU names. Using evsel->pmu_name should be good enough.
> >>
> >>> if (!pmu_have_event("cpu", "slots") && !pmu_have_event("cpu_core", "slots") )
> >>>
> >>> But we should add a helper function for this. It is odd to have this
> >>> change supporting Alderlake but the existing evlist work not. Perhaps
> >>> we should just wait until Zhengjun's patches land.
> >>
> >> Yes, a helper function is good for the arch_evlist functions. But I
> >> don't think this patch needs the helper function. Zhengjun's patches are
> >> to fix the other topdown issues on ADL. There is no dependency between
> >> this patch and zhengjun's patches.
> >>
> >> Thanks,
> >> Kan
> >
> > TL;DR I think we can move forward with landing these patches to fix Icelake.
>
> This patch doesn't work with the hybrid platform for sure. I can send
> you a fix for the hybrid part if you prefer this way? Then I guess you
> may append it as the patch 3 for V2.
>
> Besides the hybrid thing, the patch set also has other two issues I
> mentioned in the previous reply.
> - I don't think the strcasecmp() can handle the case like
> cpu/topdown-bad-spec/ or cpu/slots/. It should be an issue for both
> hybrid and non-hybrid platforms.
> - It's better not to use non-architecture events, e.g., baclears.any,
> ARITH.DIVIDER_ACTIVE, even in the test case. The non-architecture events
> may be disappear in the future platforms. If so, you have to update the
> test case again for the future platforms.
> IMHO, I don't think the patch set is ready.

So all the stated objections are that I'm checking cpu/slots/ for an
indication of topdown support and this doesn't work for hybrid? This
is identical to the arch evlist code:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/arch/x86/util/evlist.c?h=perf/core#n10
both in the functions arch_evlist__add_default_attrs and
arch_evlist__leader (one I wrote and one I didn't). So the patch set
isn't ready because I haven't fixed alderlake, but alderlake already
isn't working? And there is no example of how to make this work for
alderlake. So basically your ask is that I bring up alderlake. I think
this is stretching things for a patch fixing icelake. The value here
is in fixing icelake and alderlake will have to be the next problem.

> >
> > For Alderlake/hybrid we have a problem. To determine what happens with
> > grouping we need to know does the CPU have topdown events? This is a
> > runtime question for doing perf_event_open and so an arch test and
> > weak symbol are appropriate. For Icelake we are determining the
> > presence of topdown events by looking at the special PMU cpu. For
> > Alderlake the same information can be found by looking at the PMUs
> > cpu_core and cpu_atom, but how to discover those PMU names?
>
> The PMU name can be retrieved either from the event list or perf command.
> For the non-hybrid, the PMU name is hard code to "cpu" for the core
> events. So users/event files don't need to specify the PMU name.
> For the hybrid platform, a PMU name is required and stored in the
> evsel->pmu_name. If the evsel->pmu_name is NULL, we can assume that it's
> a non-hybrid PMU, CPU.

This doesn't make sense. Hybrid implies more than 1 CPU type, how can
more than one be the same as 1 type to be used for the PMU? Again, you
are asking I make all the alderlake logic work and I think that should
be follow up. As shown above the arch evlist code also needs fixing as
follow up.

Thanks,
Ian

> > It is
> > already somewhat concerning that we've hard coded "cpu" and we don't
> > want to have an ever growing list of PMU names.
> >
> > We have similarly hard coded "cpu" in the topology code here:
> > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/cputopo.c?h=tmp.perf/core#n18
> > Is this unreasonable given cpu is already supposed to be ABI stable:
> > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/Documentation/ABI/stable/sysfs-devices-system-cpu?h=tmp.perf/core
>
> I don't think there is a stable ABI for a PMU name.
> The PMU name may be changed generation by generation because of the
> different micro arch. We will try our best to keep it unchanged in X86
> but it's not guaranteed especially when the hybrid is introduced.
>
>
> >
> > It is hard to say what the right hybrid fix is here. I should get a
> > system I can poke shortly. I'd also like to compare what's in sysfs
> > for Alderlake with ARM's big.little approach. I can imagine we need a
> > function that returns a list of CPU like PMUs for probing. Ideally we
> > could work this out from sysfs and use some stable ABI.
> >
>
> I don't think there is a standard PMU naming rule for all the ARCHs. For
> X86, it may be possible. You can assume that the name like "cpu" or
> "cpu_*" are for core PMUs. But for other ARCH e.g., ARM, AFAIK, they use
> a quite different naming rule.
>
>
> Thanks,
> Kan

Re: [PATCH 1/2] perf evlist: Keep topdown counters in weak group

[Liang, Kan]

On 5/10/2022 12:58 PM, Ian Rogers wrote:
> On Mon, May 9, 2022 at 2:01 PM Liang, Kan <kan.liang@linux.intel.com> wrote:
>>
>> On 5/9/2022 1:28 PM, Ian Rogers wrote:
>>> On Thu, May 5, 2022 at 12:44 PM Liang, Kan <kan.liang@linux.intel.com> wrote:
>>>>
>>>>
>>>>
>>>> On 5/5/2022 2:31 PM, Ian Rogers wrote:
>>>>>>> So I think fixing all of these should be a follow up. I am working to
>>>>>>> get access to an Alderlake system, could we land this first?
>>>>>>>
>>>>>> I think we can use pmu_name to replace the "cpu" to fix the issue for
>>>>>> the hybrid platform. For a hybrid platform, the pmu_name is either
>>>>>> cpu_atom or cpu_core.
>>>>>>
>>>>>> Besides, the topdown events may have a PMU prefix, e.g.,
>>>>>> cpu_core/topdown-be-bound/. The strcasecmp may not work well for this case.
>>>>>>
>>>>>> How about the below patch?
>>>>>> If it's OK for you, could you please merge it into your V2 patch set?
>>>>>> I can do the test on a ADL system.
>>>>>>
>>>>>> diff --git a/tools/perf/arch/x86/util/evsel.c
>>>>>> b/tools/perf/arch/x86/util/evsel.c
>>>>>> index 40b171de2086..551ae2bab70e 100644
>>>>>> --- a/tools/perf/arch/x86/util/evsel.c
>>>>>> +++ b/tools/perf/arch/x86/util/evsel.c
>>>>>> @@ -33,11 +33,12 @@ void arch_evsel__fixup_new_cycles(struct
>>>>>> perf_event_attr *attr)
>>>>>>
>>>>>>      bool arch_evsel__must_be_in_group(const struct evsel *evsel)
>>>>>>      {
>>>>>> -       if ((evsel->pmu_name && strcmp(evsel->pmu_name, "cpu")) ||
>>>>>> -           !pmu_have_event("cpu", "slots"))
>>>>>> +       const char *pmu_name = evsel->pmu_name ? evsel->pmu_name : "cpu";
>>>>>> +
>>>>>> +       if (!pmu_have_event(pmu_name, "slots"))
>>>>>>                    return false;
>>>>> Hmm. The idea with this test is to see if the architecture supports
>>>>> topdown events before going further. There's a similar test in all the
>>>>> arch_evlist functions. I think with cpu_core this needs to become:
>>>>>
>>>>
>>>> The case is a little bit different here. For the arch_evlist functions,
>>>> the input is the evlist, not the specific evsel. So we have to check all
>>>> the possible PMU names which are "cpu" and "cpu_core". Then we decide
>>>> whether going further.
>>>>
>>>> The input of the evsel__must_be_in_group() is the evsel. The PMU name is
>>>> stored in the evsel->pmu_name. I don't think we need to check all the
>>>> possible PMU names. Using evsel->pmu_name should be good enough.
>>>>
>>>>> if (!pmu_have_event("cpu", "slots") && !pmu_have_event("cpu_core", "slots") )
>>>>>
>>>>> But we should add a helper function for this. It is odd to have this
>>>>> change supporting Alderlake but the existing evlist work not. Perhaps
>>>>> we should just wait until Zhengjun's patches land.
>>>>
>>>> Yes, a helper function is good for the arch_evlist functions. But I
>>>> don't think this patch needs the helper function. Zhengjun's patches are
>>>> to fix the other topdown issues on ADL. There is no dependency between
>>>> this patch and zhengjun's patches.
>>>>
>>>> Thanks,
>>>> Kan
>>>
>>> TL;DR I think we can move forward with landing these patches to fix Icelake.
>>
>> This patch doesn't work with the hybrid platform for sure. I can send
>> you a fix for the hybrid part if you prefer this way? Then I guess you
>> may append it as the patch 3 for V2.
>>
>> Besides the hybrid thing, the patch set also has other two issues I
>> mentioned in the previous reply.
>> - I don't think the strcasecmp() can handle the case like
>> cpu/topdown-bad-spec/ or cpu/slots/. It should be an issue for both
>> hybrid and non-hybrid platforms.
>> - It's better not to use non-architecture events, e.g., baclears.any,
>> ARITH.DIVIDER_ACTIVE, even in the test case. The non-architecture events
>> may be disappear in the future platforms. If so, you have to update the
>> test case again for the future platforms.
>> IMHO, I don't think the patch set is ready.
> 
> So all the stated objections are that I'm checking cpu/slots/ for an
> indication of topdown support and this doesn't work for hybrid?

No, besides the hybrid issue, I also pointed out two non-hybrid issues 
for the patch set.

Have you tried this with the patch set on ICX?

./perf stat -e 
'{cpu/slots/,cpu/topdown-bad-spec/,cpu/topdown-be-bound/,cpu/topdown-fe-bound/,cpu/topdown-retiring/,branch-instructions,branch-misses,bus-cycles,cache-misses,cache-references,cpu-cycles,instructions,mem-loads,mem-stores,ref-cycles,cache-misses,cache-references}:W' 
-a sleep 1

I don't think it works.


The ARITH.DIVIDER_ACTIVE is already a deprecated event in SPR.
I don't think we want to update the test case for the platform after SPR.


> This
> is identical to the arch evlist code:
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/arch/x86/util/evlist.c?h=perf/core#n10
> both in the functions arch_evlist__add_default_attrs and
> arch_evlist__leader (one I wrote and one I didn't). So the patch set
> isn't ready because I haven't fixed alderlake, but alderlake already
> isn't working? And there is no example of how to make this work for
> alderlake. So basically your ask is that I bring up alderlake. I think
> this is stretching things for a patch fixing icelake. The value here
> is in fixing icelake and alderlake will have to be the next problem.
> 
>>>
>>> For Alderlake/hybrid we have a problem. To determine what happens with
>>> grouping we need to know does the CPU have topdown events? This is a
>>> runtime question for doing perf_event_open and so an arch test and
>>> weak symbol are appropriate. For Icelake we are determining the
>>> presence of topdown events by looking at the special PMU cpu. For
>>> Alderlake the same information can be found by looking at the PMUs
>>> cpu_core and cpu_atom, but how to discover those PMU names?
>>
>> The PMU name can be retrieved either from the event list or perf command.
>> For the non-hybrid, the PMU name is hard code to "cpu" for the core
>> events. So users/event files don't need to specify the PMU name.
>> For the hybrid platform, a PMU name is required and stored in the
>> evsel->pmu_name. If the evsel->pmu_name is NULL, we can assume that it's
>> a non-hybrid PMU, CPU.
> 
> This doesn't make sense. Hybrid implies more than 1 CPU type, how can
> more than one be the same as 1 type to be used for the PMU? 

One event can only belongs to one PMU. It's saved in the evsel->pmu_name.
But it may be NULL on a non-hybrid machine, because there is only one 
CPU type for a non-hybrid machine. The old code may not set the variable.

> Again, you
> are asking I make all the alderlake logic work and I think that should
> be follow up.

I didn't ask you to make all the alderlake logic work.

What I asked is not to introduce more such kind problem, because we 
already have enough to fix.

I even offered you a fixed patch for the problem.

https://lore.kernel.org/all/f1c212e3-c1fd-4a2d-0dfe-bc913d4f4f36@linux.intel.com/T/#m22465c6d35be59fc853a7d86f5bd6025ce7e539b


> As shown above the arch evlist code also needs fixing as
> follow up.

Zhengjun is working on them. There is no dependency between this patch 
and that fix. Don't worry about it.


Thanks,
Kan