[Patch v5 0/6] Bug fixes on topdown events reordering

[Patch v5 0/6] Bug fixes on topdown events reordering

[Dapeng Mi]

Changes:
v5 -> v6:
  * no function change.
  * rebase patchset to latest code of perf-tool-next tree.
  * Add Kan's reviewed-by tag.

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

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

 tools/perf/arch/x86/util/evlist.c  | 68 ++++++++++++++++++++++++++++--
 tools/perf/arch/x86/util/evsel.c   |  3 +-
 tools/perf/arch/x86/util/topdown.c | 64 +++++++++++++++++++++++++++-
 tools/perf/arch/x86/util/topdown.h |  2 +
 tools/perf/tests/shell/record.sh   | 45 ++++++++++++++++++++
 tools/perf/tests/shell/stat.sh     | 28 +++++++++++-
 6 files changed, 201 insertions(+), 9 deletions(-)


base-commit: 1de5b5dcb8353f36581c963df2d359a5f151a0be
-- 
2.40.1

[Patch v5 1/6] perf x86/topdown: Complete topdown slots/metrics events check

[Dapeng Mi]

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

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

 Performance counter stats for 'sleep 1':

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

       1.002917796 seconds time elapsed

       0.002955000 seconds user
       0.000000000 seconds sys

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

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

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

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

Re: [Patch v5 1/6] perf x86/topdown: Complete topdown slots/metrics events check

[Ian Rogers]

On Thu, Sep 12, 2024 at 10:21 PM Dapeng Mi <dapeng1.mi@linux.intel.com> wrote:
>
> It's not complete to check whether an event is a topdown slots or
> topdown metrics event by only comparing the event name since user
> may assign the event by RAW format, e.g.
>
> perf stat -e '{instructions,cpu/r400/,cpu/r8300/}' sleep 1
>
>  Performance counter stats for 'sleep 1':
>
>      <not counted>      instructions
>      <not counted>      cpu/r400/
>    <not supported>      cpu/r8300/
>
>        1.002917796 seconds time elapsed
>
>        0.002955000 seconds user
>        0.000000000 seconds sys
>
> The RAW format slots and topdown-be-bound events are not recognized and
> not regroup the events, and eventually cause error.
>
> Thus add two helpers arch_is_topdown_slots()/arch_is_topdown_metrics()
> to detect whether an event is topdown slots/metrics event by comparing
> the event config directly, and use these two helpers to replace the
> original event name comparisons.
>
> Reviewed-by: Kan Liang <kan.liang@linux.intel.com>
> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
> ---
>  tools/perf/arch/x86/util/evlist.c  |  8 ++---
>  tools/perf/arch/x86/util/evsel.c   |  3 +-
>  tools/perf/arch/x86/util/topdown.c | 48 +++++++++++++++++++++++++++++-
>  tools/perf/arch/x86/util/topdown.h |  2 ++
>  4 files changed, 55 insertions(+), 6 deletions(-)
>
> diff --git a/tools/perf/arch/x86/util/evlist.c b/tools/perf/arch/x86/util/evlist.c
> index cebdd483149e..79799865a62a 100644
> --- a/tools/perf/arch/x86/util/evlist.c
> +++ b/tools/perf/arch/x86/util/evlist.c
> @@ -78,14 +78,14 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
>         if (topdown_sys_has_perf_metrics() &&
>             (arch_evsel__must_be_in_group(lhs) || arch_evsel__must_be_in_group(rhs))) {
>                 /* Ensure the topdown slots comes first. */
> -               if (strcasestr(lhs->name, "slots") && !strcasestr(lhs->name, "uops_retired.slots"))
> +               if (arch_is_topdown_slots(lhs))
>                         return -1;
> -               if (strcasestr(rhs->name, "slots") && !strcasestr(rhs->name, "uops_retired.slots"))
> +               if (arch_is_topdown_slots(rhs))
>                         return 1;
>                 /* Followed by topdown events. */
> -               if (strcasestr(lhs->name, "topdown") && !strcasestr(rhs->name, "topdown"))
> +               if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
>                         return -1;
> -               if (!strcasestr(lhs->name, "topdown") && strcasestr(rhs->name, "topdown"))
> +               if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs))
>                         return 1;
>         }
>
> diff --git a/tools/perf/arch/x86/util/evsel.c b/tools/perf/arch/x86/util/evsel.c
> index 090d0f371891..181f2ba0bb2a 100644
> --- a/tools/perf/arch/x86/util/evsel.c
> +++ b/tools/perf/arch/x86/util/evsel.c
> @@ -6,6 +6,7 @@
>  #include "util/pmu.h"
>  #include "util/pmus.h"
>  #include "linux/string.h"
> +#include "topdown.h"
>  #include "evsel.h"
>  #include "util/debug.h"
>  #include "env.h"
> @@ -44,7 +45,7 @@ bool arch_evsel__must_be_in_group(const struct evsel *evsel)
>             strcasestr(evsel->name, "uops_retired.slots"))
>                 return false;
>
> -       return strcasestr(evsel->name, "topdown") || strcasestr(evsel->name, "slots");
> +       return arch_is_topdown_metrics(evsel) || arch_is_topdown_slots(evsel);
>  }
>
>  int arch_evsel__hw_name(struct evsel *evsel, char *bf, size_t size)
> diff --git a/tools/perf/arch/x86/util/topdown.c b/tools/perf/arch/x86/util/topdown.c
> index 3f9a267d4501..49f25d67ed77 100644
> --- a/tools/perf/arch/x86/util/topdown.c
> +++ b/tools/perf/arch/x86/util/topdown.c
> @@ -32,6 +32,52 @@ bool topdown_sys_has_perf_metrics(void)
>  }
>
>  #define TOPDOWN_SLOTS          0x0400
> +bool arch_is_topdown_slots(const struct evsel *evsel)
> +{

Perhaps: assert(evsel__find_pmu(evsel)->is_core);

> +       if (evsel->core.attr.config == TOPDOWN_SLOTS)
> +               return true;
> +
> +       return false;
> +}
> +
> +static int compare_topdown_event(void *vstate, struct pmu_event_info *info)
> +{
> +       int *config = vstate;
> +       int event = 0;
> +       int umask = 0;
> +       char *str;
> +
> +       if (!strcasestr(info->name, "topdown"))
> +               return 0;
> +
> +       str = strcasestr(info->str, "event=");
> +       if (str)
> +               sscanf(str, "event=%x", &event);
> +
> +       str = strcasestr(info->str, "umask=");
> +       if (str)
> +               sscanf(str, "umask=%x", &umask);
> +
> +       if (event == 0 && *config == (event | umask << 8))
> +               return 1;
> +
> +       return 0;
> +}
> +
> +bool arch_is_topdown_metrics(const struct evsel *evsel)
> +{
> +       struct perf_pmu *pmu = evsel__find_pmu(evsel);
> +       int config = evsel->core.attr.config;
> +
> +       if (!pmu || !pmu->is_core)
> +               return false;
> +
> +       if (perf_pmu__for_each_event(pmu, false, &config,
> +                                    compare_topdown_event))
> +               return true;
> +
> +       return false;
> +}


Doing a linear search over every event is painful perf_pmu__have_event
will try to binary search. The search rejects all events without
"topdown" in their name, it then sees if the event code is 0 and the
event|umask match the sysfs/json event. Is there ever a case the
"topdown" string isn't at the beginning of the string? If it is it
should be possible to binary search to the start of the topdown
events.

Strictly you shouldn't hard code the config positions of event and
umask, they are in the format list:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/pmu.h?h=perf-tools-next#n113
There is code doing similar to this here:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/pmu.c?h=perf-tools-next#n2285
but it avoids the scanf, uses formats...
It seems reasonable this code should reject all non-zero configs
before searching all core PMU events. It could also use
perf_pmu__name_from_config. So:
```
bool evsel__is_topdown_metric_event(const struct evsel *evsel)
{
   struct perf_pmu *pmu;
   const char *name_from_config;

   if (evsel->core.attr.config & 0xFF != 0) /* All topdown events have
an event code of 0. */
     return false;

  pmu  = evsel__find_pmu(evsel);
  if (!pmu || !pmu->is_core)
     return false;

  name_from_config = perf_pmu__name_from_config(pmu, config);
  return name_from_config && !strcasestr(name_from_config, "topdown);
}
```
We could tweak perf_pmu__name_from_config to be pased a  "filter". In
this case the filter would skip events without topdown in their name,
without doing a config comparison.

If later we change perf_pmu__name_from_config to say sort events in a
list by config, then this code could take advantage of that. Perhaps
for now there should just be an optional "prefix" that can be used to
binary search to the events.
```
  name_from_config = perf_pmu__name_from_config(pmu, config,
/*prefix=*/"topdown");
```

Thanks,
Ian

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

Re: [Patch v5 1/6] perf x86/topdown: Complete topdown slots/metrics events check

[Mi, Dapeng]

On 10/8/2024 1:55 PM, Ian Rogers wrote:
> On Thu, Sep 12, 2024 at 10:21 PM Dapeng Mi <dapeng1.mi@linux.intel.com> wrote:
>> It's not complete to check whether an event is a topdown slots or
>> topdown metrics event by only comparing the event name since user
>> may assign the event by RAW format, e.g.
>>
>> perf stat -e '{instructions,cpu/r400/,cpu/r8300/}' sleep 1
>>
>>  Performance counter stats for 'sleep 1':
>>
>>      <not counted>      instructions
>>      <not counted>      cpu/r400/
>>    <not supported>      cpu/r8300/
>>
>>        1.002917796 seconds time elapsed
>>
>>        0.002955000 seconds user
>>        0.000000000 seconds sys
>>
>> The RAW format slots and topdown-be-bound events are not recognized and
>> not regroup the events, and eventually cause error.
>>
>> Thus add two helpers arch_is_topdown_slots()/arch_is_topdown_metrics()
>> to detect whether an event is topdown slots/metrics event by comparing
>> the event config directly, and use these two helpers to replace the
>> original event name comparisons.
>>
>> Reviewed-by: Kan Liang <kan.liang@linux.intel.com>
>> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
>> ---
>>  tools/perf/arch/x86/util/evlist.c  |  8 ++---
>>  tools/perf/arch/x86/util/evsel.c   |  3 +-
>>  tools/perf/arch/x86/util/topdown.c | 48 +++++++++++++++++++++++++++++-
>>  tools/perf/arch/x86/util/topdown.h |  2 ++
>>  4 files changed, 55 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/perf/arch/x86/util/evlist.c b/tools/perf/arch/x86/util/evlist.c
>> index cebdd483149e..79799865a62a 100644
>> --- a/tools/perf/arch/x86/util/evlist.c
>> +++ b/tools/perf/arch/x86/util/evlist.c
>> @@ -78,14 +78,14 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
>>         if (topdown_sys_has_perf_metrics() &&
>>             (arch_evsel__must_be_in_group(lhs) || arch_evsel__must_be_in_group(rhs))) {
>>                 /* Ensure the topdown slots comes first. */
>> -               if (strcasestr(lhs->name, "slots") && !strcasestr(lhs->name, "uops_retired.slots"))
>> +               if (arch_is_topdown_slots(lhs))
>>                         return -1;
>> -               if (strcasestr(rhs->name, "slots") && !strcasestr(rhs->name, "uops_retired.slots"))
>> +               if (arch_is_topdown_slots(rhs))
>>                         return 1;
>>                 /* Followed by topdown events. */
>> -               if (strcasestr(lhs->name, "topdown") && !strcasestr(rhs->name, "topdown"))
>> +               if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
>>                         return -1;
>> -               if (!strcasestr(lhs->name, "topdown") && strcasestr(rhs->name, "topdown"))
>> +               if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs))
>>                         return 1;
>>         }
>>
>> diff --git a/tools/perf/arch/x86/util/evsel.c b/tools/perf/arch/x86/util/evsel.c
>> index 090d0f371891..181f2ba0bb2a 100644
>> --- a/tools/perf/arch/x86/util/evsel.c
>> +++ b/tools/perf/arch/x86/util/evsel.c
>> @@ -6,6 +6,7 @@
>>  #include "util/pmu.h"
>>  #include "util/pmus.h"
>>  #include "linux/string.h"
>> +#include "topdown.h"
>>  #include "evsel.h"
>>  #include "util/debug.h"
>>  #include "env.h"
>> @@ -44,7 +45,7 @@ bool arch_evsel__must_be_in_group(const struct evsel *evsel)
>>             strcasestr(evsel->name, "uops_retired.slots"))
>>                 return false;
>>
>> -       return strcasestr(evsel->name, "topdown") || strcasestr(evsel->name, "slots");
>> +       return arch_is_topdown_metrics(evsel) || arch_is_topdown_slots(evsel);
>>  }
>>
>>  int arch_evsel__hw_name(struct evsel *evsel, char *bf, size_t size)
>> diff --git a/tools/perf/arch/x86/util/topdown.c b/tools/perf/arch/x86/util/topdown.c
>> index 3f9a267d4501..49f25d67ed77 100644
>> --- a/tools/perf/arch/x86/util/topdown.c
>> +++ b/tools/perf/arch/x86/util/topdown.c
>> @@ -32,6 +32,52 @@ bool topdown_sys_has_perf_metrics(void)
>>  }
>>
>>  #define TOPDOWN_SLOTS          0x0400
>> +bool arch_is_topdown_slots(const struct evsel *evsel)
>> +{
> Perhaps: assert(evsel__find_pmu(evsel)->is_core);

assert? we don't need an assert as this helper just detects if a evsel is
slots event and it's reasonable if the evsel is not. But it's nice to add
the is_core check. Thanks.


>
>> +       if (evsel->core.attr.config == TOPDOWN_SLOTS)
>> +               return true;
>> +
>> +       return false;
>> +}
>> +
>> +static int compare_topdown_event(void *vstate, struct pmu_event_info *info)
>> +{
>> +       int *config = vstate;
>> +       int event = 0;
>> +       int umask = 0;
>> +       char *str;
>> +
>> +       if (!strcasestr(info->name, "topdown"))
>> +               return 0;
>> +
>> +       str = strcasestr(info->str, "event=");
>> +       if (str)
>> +               sscanf(str, "event=%x", &event);
>> +
>> +       str = strcasestr(info->str, "umask=");
>> +       if (str)
>> +               sscanf(str, "umask=%x", &umask);
>> +
>> +       if (event == 0 && *config == (event | umask << 8))
>> +               return 1;
>> +
>> +       return 0;
>> +}
>> +
>> +bool arch_is_topdown_metrics(const struct evsel *evsel)
>> +{
>> +       struct perf_pmu *pmu = evsel__find_pmu(evsel);
>> +       int config = evsel->core.attr.config;
>> +
>> +       if (!pmu || !pmu->is_core)
>> +               return false;
>> +
>> +       if (perf_pmu__for_each_event(pmu, false, &config,
>> +                                    compare_topdown_event))
>> +               return true;
>> +
>> +       return false;
>> +}
>
> Doing a linear search over every event is painful perf_pmu__have_event
> will try to binary search. The search rejects all events without
> "topdown" in their name, it then sees if the event code is 0 and the
> event|umask match the sysfs/json event. Is there ever a case the
> "topdown" string isn't at the beginning of the string? If it is it

Currently all topdown events would start with "topdown" prefix.


> should be possible to binary search to the start of the topdown
> events.
>
> Strictly you shouldn't hard code the config positions of event and
> umask, they are in the format list:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/pmu.h?h=perf-tools-next#n113
> There is code doing similar to this here:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/pmu.c?h=perf-tools-next#n2285
> but it avoids the scanf, uses formats...
> It seems reasonable this code should reject all non-zero configs
> before searching all core PMU events. It could also use
> perf_pmu__name_from_config. So:
> ```
> bool evsel__is_topdown_metric_event(const struct evsel *evsel)
> {
>    struct perf_pmu *pmu;
>    const char *name_from_config;
>
>    if (evsel->core.attr.config & 0xFF != 0) /* All topdown events have
> an event code of 0. */
>      return false;
>
>   pmu  = evsel__find_pmu(evsel);
>   if (!pmu || !pmu->is_core)
>      return false;
>
>   name_from_config = perf_pmu__name_from_config(pmu, config);
>   return name_from_config && !strcasestr(name_from_config, "topdown);
> }
> ```

Good point! would cook a patch to implement this. Thanks.


> We could tweak perf_pmu__name_from_config to be pased a  "filter". In
> this case the filter would skip events without topdown in their name,
> without doing a config comparison.
>
> If later we change perf_pmu__name_from_config to say sort events in a
> list by config, then this code could take advantage of that. Perhaps
> for now there should just be an optional "prefix" that can be used to
> binary search to the events.
> ```
>   name_from_config = perf_pmu__name_from_config(pmu, config,
> /*prefix=*/"topdown");
> ```
>
> Thanks,
> Ian
>
>>  /*
>>   * Check whether a topdown group supports sample-read.
>> @@ -44,7 +90,7 @@ bool arch_topdown_sample_read(struct evsel *leader)
>>         if (!evsel__sys_has_perf_metrics(leader))
>>                 return false;
>>
>> -       if (leader->core.attr.config == TOPDOWN_SLOTS)
>> +       if (arch_is_topdown_slots(leader))
>>                 return true;
>>
>>         return false;
>> diff --git a/tools/perf/arch/x86/util/topdown.h b/tools/perf/arch/x86/util/topdown.h
>> index 46bf9273e572..1bae9b1822d7 100644
>> --- a/tools/perf/arch/x86/util/topdown.h
>> +++ b/tools/perf/arch/x86/util/topdown.h
>> @@ -3,5 +3,7 @@
>>  #define _TOPDOWN_H 1
>>
>>  bool topdown_sys_has_perf_metrics(void);
>> +bool arch_is_topdown_slots(const struct evsel *evsel);
>> +bool arch_is_topdown_metrics(const struct evsel *evsel);
>>
>>  #endif
>> --
>> 2.40.1
>>

[Patch v5 2/6] perf x86/topdown: Correct leader selection with sample_read enabled

[Dapeng Mi]

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

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

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

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

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

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

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

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

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

[Dapeng Mi]

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

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

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

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

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

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

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

Reviewed-by: Kan Liang <kan.liang@linux.intel.com>
Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
---
 tools/perf/arch/x86/util/evlist.c | 62 ++++++++++++++++++++++++++++++-
 1 file changed, 61 insertions(+), 1 deletion(-)

diff --git a/tools/perf/arch/x86/util/evlist.c b/tools/perf/arch/x86/util/evlist.c
index 79799865a62a..20b4f3c2afa3 100644
--- a/tools/perf/arch/x86/util/evlist.c
+++ b/tools/perf/arch/x86/util/evlist.c
@@ -75,6 +75,61 @@ int arch_evlist__add_default_attrs(struct evlist *evlist,
 
 int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
 {
+	/*
+	 * Currently the following topdown events sequence are supported to
+	 * move and regroup correctly.
+	 *
+	 * a. all events in a group
+	 *    perf stat -e "{instructions,topdown-retiring,slots}" -C0 sleep 1
+	 *    WARNING: events were regrouped to match PMUs
+	 *     Performance counter stats for 'CPU(s) 0':
+	 *          15,066,240     slots
+	 *          1,899,760      instructions
+	 *          2,126,998      topdown-retiring
+	 * b. all events not in a group
+	 *    perf stat -e "instructions,topdown-retiring,slots" -C0 sleep 1
+	 *    WARNING: events were regrouped to match PMUs
+	 *     Performance counter stats for 'CPU(s) 0':
+	 *          2,045,561      instructions
+	 *          17,108,370     slots
+	 *          2,281,116      topdown-retiring
+	 * c. slots event in a group but topdown metrics events outside the group
+	 *    perf stat -e "{instructions,slots},topdown-retiring" -C0 sleep 1
+	 *    WARNING: events were regrouped to match PMUs
+	 *     Performance counter stats for 'CPU(s) 0':
+	 *         20,323,878      slots
+	 *          2,634,884      instructions
+	 *          3,028,656      topdown-retiring
+	 * d. slots event and topdown metrics events in two groups
+	 *    perf stat -e "{instructions,slots},{topdown-retiring}" -C0 sleep 1
+	 *    WARNING: events were regrouped to match PMUs
+	 *     Performance counter stats for 'CPU(s) 0':
+	 *         26,319,024      slots
+	 *          2,427,791      instructions
+	 *          2,683,508      topdown-retiring
+	 *
+	 * If slots event and topdown metrics events are not in same group, the
+	 * topdown metrics events must be first event after the slots event group,
+	 * otherwise topdown metrics events can't be regrouped correctly, e.g.
+	 *
+	 * a. perf stat -e "{instructions,slots},cycles,topdown-retiring" -C0 sleep 1
+	 *    WARNING: events were regrouped to match PMUs
+	 *     Performance counter stats for 'CPU(s) 0':
+	 *         17,923,134      slots
+	 *          2,154,855      instructions
+	 *          3,015,058      cycles
+	 *    <not supported>      topdown-retiring
+	 *
+	 * if slots event and topdown metrics events are in two groups, the group which
+	 * has topdown metrics events must contain only the topdown metrics event,
+	 * otherwise topdown metrics event can't be regrouped correctly as well, e.g.
+	 *
+	 * a. perf stat -e "{instructions,slots},{topdown-retiring,cycles}" -C0 sleep 1
+	 *    WARNING: events were regrouped to match PMUs
+	 *    Error:
+	 *    The sys_perf_event_open() syscall returned with 22 (Invalid argument) for
+	 *    event (topdown-retiring)
+	 */
 	if (topdown_sys_has_perf_metrics() &&
 	    (arch_evsel__must_be_in_group(lhs) || arch_evsel__must_be_in_group(rhs))) {
 		/* Ensure the topdown slots comes first. */
@@ -85,7 +140,12 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
 		/* Followed by topdown events. */
 		if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
 			return -1;
-		if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs))
+		/*
+		 * Move topdown events forward only when topdown events
+		 * are not in same group with previous event.
+		 */
+		if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs) &&
+		    lhs->core.leader != rhs->core.leader)
 			return 1;
 	}
 
-- 
2.40.1

[Patch v5 4/6] perf tests: Add leader sampling test in record tests

[Dapeng Mi]

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

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

diff --git a/tools/perf/tests/shell/record.sh b/tools/perf/tests/shell/record.sh
index 048078ee2eca..45baf7910640 100755
--- a/tools/perf/tests/shell/record.sh
+++ b/tools/perf/tests/shell/record.sh
@@ -17,6 +17,7 @@ skip_test_missing_symbol ${testsym}
 
 err=0
 perfdata=$(mktemp /tmp/__perf_test.perf.data.XXXXX)
+script_output=$(mktemp /tmp/__perf_test.perf.data.XXXXX.script)
 testprog="perf test -w thloop"
 cpu_pmu_dir="/sys/bus/event_source/devices/cpu*"
 br_cntr_file="/caps/branch_counter_nr"
@@ -228,6 +229,32 @@ test_cgroup() {
   echo "Cgroup sampling test [Success]"
 }
 
+test_leader_sampling() {
+  echo "Basic leader sampling test"
+  if ! perf record -o "${perfdata}" -e "{branches,branches}:Su" perf test -w brstack 2> /dev/null
+  then
+    echo "Leader sampling [Failed record]"
+    err=1
+    return
+  fi
+  index=0
+  perf script -i "${perfdata}" > $script_output
+  while IFS= read -r line
+  do
+    # Check if the two branches counts are equal in each record
+    branches=$(echo $line | awk '{for(i=1;i<=NF;i++) if($i=="branches:") print $(i-1)}')
+    if [ $(($index%2)) -ne 0 ] && [ ${branches}x != ${prev_branches}x ]
+    then
+      echo "Leader sampling [Failed inconsistent branches count]"
+      err=1
+      return
+    fi
+    index=$(($index+1))
+    prev_branches=$branches
+  done < $script_output
+  echo "Basic leader sampling test [Success]"
+}
+
 # raise the limit of file descriptors to minimum
 if [[ $default_fd_limit -lt $min_fd_limit ]]; then
        ulimit -Sn $min_fd_limit
@@ -239,6 +266,7 @@ test_system_wide
 test_workload
 test_branch_counter
 test_cgroup
+test_leader_sampling
 
 # restore the default value
 ulimit -Sn $default_fd_limit
-- 
2.40.1

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

[Dapeng Mi]

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

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

diff --git a/tools/perf/tests/shell/record.sh b/tools/perf/tests/shell/record.sh
index 45baf7910640..8d6366d96883 100755
--- a/tools/perf/tests/shell/record.sh
+++ b/tools/perf/tests/shell/record.sh
@@ -255,6 +255,22 @@ test_leader_sampling() {
   echo "Basic leader sampling test [Success]"
 }
 
+test_topdown_leader_sampling() {
+  echo "Topdown leader sampling test"
+  if ! perf stat -e "{slots,topdown-retiring}" true 2> /dev/null
+  then
+    echo "Topdown leader sampling [Skipped event parsing failed]"
+    return
+  fi
+  if ! perf record -o "${perfdata}" -e "{instructions,slots,topdown-retiring}:S" true 2> /dev/null
+  then
+    echo "Topdown leader sampling [Failed topdown events not reordered correctly]"
+    err=1
+    return
+  fi
+  echo "Topdown leader sampling test [Success]"
+}
+
 # raise the limit of file descriptors to minimum
 if [[ $default_fd_limit -lt $min_fd_limit ]]; then
        ulimit -Sn $min_fd_limit
@@ -267,6 +283,7 @@ test_workload
 test_branch_counter
 test_cgroup
 test_leader_sampling
+test_topdown_leader_sampling
 
 # restore the default value
 ulimit -Sn $default_fd_limit
diff --git a/tools/perf/tests/shell/stat.sh b/tools/perf/tests/shell/stat.sh
index 3f1e67795490..092a7a2abcf8 100755
--- a/tools/perf/tests/shell/stat.sh
+++ b/tools/perf/tests/shell/stat.sh
@@ -79,6 +79,12 @@ test_topdown_groups() {
     err=1
     return
   fi
+  if perf stat -e '{instructions,r400,r8000}' true 2>&1 | grep -E -q "<not supported>"
+  then
+    echo "Topdown event group test [Failed raw format slots not reordered first]"
+    err=1
+    return
+  fi
   echo "Topdown event group test [Success]"
 }
 
-- 
2.40.1

[Patch v5 6/6] perf tests: Add more topdown events regroup tests

[Dapeng Mi]

Add more test cases to cover all supported topdown events regroup cases.

Reviewed-by: Kan Liang <kan.liang@linux.intel.com>
Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
---
 tools/perf/tests/shell/stat.sh | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/tools/perf/tests/shell/stat.sh b/tools/perf/tests/shell/stat.sh
index 092a7a2abcf8..85897898a1f4 100755
--- a/tools/perf/tests/shell/stat.sh
+++ b/tools/perf/tests/shell/stat.sh
@@ -73,9 +73,27 @@ test_topdown_groups() {
     err=1
     return
   fi
-  if perf stat -e '{topdown-retiring,slots}' true 2>&1 | grep -E -q "<not supported>"
+  if perf stat -e 'instructions,topdown-retiring,slots' true 2>&1 | grep -E -q "<not supported>"
   then
-    echo "Topdown event group test [Failed slots not reordered first]"
+    echo "Topdown event group test [Failed slots not reordered first in no-group case]"
+    err=1
+    return
+  fi
+  if perf stat -e '{instructions,topdown-retiring,slots}' true 2>&1 | grep -E -q "<not supported>"
+  then
+    echo "Topdown event group test [Failed slots not reordered first in single group case]"
+    err=1
+    return
+  fi
+  if perf stat -e '{instructions,slots},topdown-retiring' true 2>&1 | grep -E -q "<not supported>"
+  then
+    echo "Topdown event group test [Failed topdown metrics event not move into slots group]"
+    err=1
+    return
+  fi
+  if perf stat -e '{instructions,slots},{topdown-retiring}' true 2>&1 | grep -E -q "<not supported>"
+  then
+    echo "Topdown event group test [Failed topdown metrics group not merge into slots group]"
     err=1
     return
   fi
-- 
2.40.1

Re: [Patch v5 0/6] Bug fixes on topdown events reordering

[Namhyung Kim]

On Fri, 13 Sep 2024 08:47:06 +0000, Dapeng Mi wrote:

> Changes:
> v5 -> v6:
>   * no function change.
>   * rebase patchset to latest code of perf-tool-next tree.
>   * Add Kan's reviewed-by tag.
> 
> History:
>   v4: https://lore.kernel.org/all/20240816122938.32228-1-dapeng1.mi@linux.intel.com/
>   v3: https://lore.kernel.org/all/20240712170339.185824-1-dapeng1.mi@linux.intel.com/
>   v2: https://lore.kernel.org/all/20240708144204.839486-1-dapeng1.mi@linux.intel.com/
>   v1: https://lore.kernel.org/all/20240702224037.343958-1-dapeng1.mi@linux.intel.com/
> 
> [...]

Applied to perf-tools-next, thanks!

Best regards,
Namhyung

Re: [Patch v5 0/6] Bug fixes on topdown events reordering

[Ian Rogers]

On Tue, Oct 1, 2024 at 2:02 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Fri, 13 Sep 2024 08:47:06 +0000, Dapeng Mi wrote:
>
> > Changes:
> > v5 -> v6:
> >   * no function change.
> >   * rebase patchset to latest code of perf-tool-next tree.
> >   * Add Kan's reviewed-by tag.
> >
> > History:
> >   v4: https://lore.kernel.org/all/20240816122938.32228-1-dapeng1.mi@linux.intel.com/
> >   v3: https://lore.kernel.org/all/20240712170339.185824-1-dapeng1.mi@linux.intel.com/
> >   v2: https://lore.kernel.org/all/20240708144204.839486-1-dapeng1.mi@linux.intel.com/
> >   v1: https://lore.kernel.org/all/20240702224037.343958-1-dapeng1.mi@linux.intel.com/
> >
> > [...]
>
> Applied to perf-tools-next, thanks!

I disagreed with an early patch set and the issue wasn't resolved. Specifically:

https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/commit/?h=perf-tools-next&id=3b5edc0421e2598a0ae7f0adcd592017f37e3cdf
```
  /* Followed by topdown events. */
  if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
  return -1;
- if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs))
+ /*
+ * Move topdown events forward only when topdown events
+ * are not in same group with previous event.
+ */
+ if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs) &&
+     lhs->core.leader != rhs->core.leader)
  return 1;
```
Is a broken comparator as the lhs then rhs behavior varies from the
rhs then lhs behavior. The qsort implementation can randomly order the
events.
Please drop/revert.

Thanks,
Ian

> Best regards,
> Namhyung

Re: [Patch v5 0/6] Bug fixes on topdown events reordering

[Ian Rogers]

On Tue, Oct 1, 2024 at 3:32 PM Ian Rogers <irogers@google.com> wrote:
>
> On Tue, Oct 1, 2024 at 2:02 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Fri, 13 Sep 2024 08:47:06 +0000, Dapeng Mi wrote:
> >
> > > Changes:
> > > v5 -> v6:
> > >   * no function change.
> > >   * rebase patchset to latest code of perf-tool-next tree.
> > >   * Add Kan's reviewed-by tag.
> > >
> > > History:
> > >   v4: https://lore.kernel.org/all/20240816122938.32228-1-dapeng1.mi@linux.intel.com/
> > >   v3: https://lore.kernel.org/all/20240712170339.185824-1-dapeng1.mi@linux.intel.com/
> > >   v2: https://lore.kernel.org/all/20240708144204.839486-1-dapeng1.mi@linux.intel.com/
> > >   v1: https://lore.kernel.org/all/20240702224037.343958-1-dapeng1.mi@linux.intel.com/
> > >
> > > [...]
> >
> > Applied to perf-tools-next, thanks!
>
> I disagreed with an early patch set and the issue wasn't resolved. Specifically:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/commit/?h=perf-tools-next&id=3b5edc0421e2598a0ae7f0adcd592017f37e3cdf
> ```
>   /* Followed by topdown events. */
>   if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
>   return -1;
> - if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs))
> + /*
> + * Move topdown events forward only when topdown events
> + * are not in same group with previous event.
> + */
> + if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs) &&
> +     lhs->core.leader != rhs->core.leader)
>   return 1;
> ```
> Is a broken comparator as the lhs then rhs behavior varies from the
> rhs then lhs behavior. The qsort implementation can randomly order the
> events.
> Please drop/revert.

Tihs is still in the tree:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/commit/?h=perf-tools-next&id=3b5edc0421e2598a0ae7f0adcd592017f37e3cdf
It should be invariant that for all comparators that:
a < b == b > a
This code breaks this property and so event sorting is broken.

Thanks,
Ian

Re: [Patch v5 0/6] Bug fixes on topdown events reordering

[Namhyung Kim]

On Tue, Oct 01, 2024 at 03:32:04PM -0700, Ian Rogers wrote:
> On Tue, Oct 1, 2024 at 2:02 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Fri, 13 Sep 2024 08:47:06 +0000, Dapeng Mi wrote:
> >
> > > Changes:
> > > v5 -> v6:
> > >   * no function change.
> > >   * rebase patchset to latest code of perf-tool-next tree.
> > >   * Add Kan's reviewed-by tag.
> > >
> > > History:
> > >   v4: https://lore.kernel.org/all/20240816122938.32228-1-dapeng1.mi@linux.intel.com/
> > >   v3: https://lore.kernel.org/all/20240712170339.185824-1-dapeng1.mi@linux.intel.com/
> > >   v2: https://lore.kernel.org/all/20240708144204.839486-1-dapeng1.mi@linux.intel.com/
> > >   v1: https://lore.kernel.org/all/20240702224037.343958-1-dapeng1.mi@linux.intel.com/
> > >
> > > [...]
> >
> > Applied to perf-tools-next, thanks!
> 
> I disagreed with an early patch set and the issue wasn't resolved. Specifically:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/commit/?h=perf-tools-next&id=3b5edc0421e2598a0ae7f0adcd592017f37e3cdf
> ```
>   /* Followed by topdown events. */
>   if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
>   return -1;
> - if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs))
> + /*
> + * Move topdown events forward only when topdown events
> + * are not in same group with previous event.
> + */
> + if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs) &&
> +     lhs->core.leader != rhs->core.leader)
>   return 1;
> ```
> Is a broken comparator as the lhs then rhs behavior varies from the
> rhs then lhs behavior. The qsort implementation can randomly order the
> events.
> Please drop/revert.

Can you please provide an example when it's broken?  I'm not sure how it
can produce new errors, but it seems to fix a specific problem.  Do you
have a new test failure after this change?

Thanks,
Namhyung

Re: [Patch v5 0/6] Bug fixes on topdown events reordering

[Ian Rogers]

On Wed, Oct 2, 2024 at 5:00 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Tue, Oct 01, 2024 at 03:32:04PM -0700, Ian Rogers wrote:
> > On Tue, Oct 1, 2024 at 2:02 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > >
> > > On Fri, 13 Sep 2024 08:47:06 +0000, Dapeng Mi wrote:
> > >
> > > > Changes:
> > > > v5 -> v6:
> > > >   * no function change.
> > > >   * rebase patchset to latest code of perf-tool-next tree.
> > > >   * Add Kan's reviewed-by tag.
> > > >
> > > > History:
> > > >   v4: https://lore.kernel.org/all/20240816122938.32228-1-dapeng1.mi@linux.intel.com/
> > > >   v3: https://lore.kernel.org/all/20240712170339.185824-1-dapeng1.mi@linux.intel.com/
> > > >   v2: https://lore.kernel.org/all/20240708144204.839486-1-dapeng1.mi@linux.intel.com/
> > > >   v1: https://lore.kernel.org/all/20240702224037.343958-1-dapeng1.mi@linux.intel.com/
> > > >
> > > > [...]
> > >
> > > Applied to perf-tools-next, thanks!
> >
> > I disagreed with an early patch set and the issue wasn't resolved. Specifically:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/commit/?h=perf-tools-next&id=3b5edc0421e2598a0ae7f0adcd592017f37e3cdf
> > ```
> >   /* Followed by topdown events. */
> >   if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
> >   return -1;
> > - if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs))
> > + /*
> > + * Move topdown events forward only when topdown events
> > + * are not in same group with previous event.
> > + */
> > + if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs) &&
> > +     lhs->core.leader != rhs->core.leader)
> >   return 1;
> > ```
> > Is a broken comparator as the lhs then rhs behavior varies from the
> > rhs then lhs behavior. The qsort implementation can randomly order the
> > events.
> > Please drop/revert.
>
> Can you please provide an example when it's broken?  I'm not sure how it
> can produce new errors, but it seems to fix a specific problem.  Do you
> have a new test failure after this change?

It may work with a particular sort routine in a particular library
today, the issue is that if the sort routine were say changed from:

if (cmp(a, b) < 0)

to:

if (cmp(b, a) > 0)

the sort would vary with this change even though such a change in the
sorting code is a no-op. It is fundamental algorithms that this code
is broken, I'm not going to spend the time finding a list of
instructions and a sort routine to demonstrate this.

Thanks,
Ian

Re: [Patch v5 0/6] Bug fixes on topdown events reordering

[Liang, Kan]

On 2024-10-02 8:57 p.m., Ian Rogers wrote:
> On Wed, Oct 2, 2024 at 5:00 PM Namhyung Kim <namhyung@kernel.org> wrote:
>>
>> On Tue, Oct 01, 2024 at 03:32:04PM -0700, Ian Rogers wrote:
>>> On Tue, Oct 1, 2024 at 2:02 PM Namhyung Kim <namhyung@kernel.org> wrote:
>>>>
>>>> On Fri, 13 Sep 2024 08:47:06 +0000, Dapeng Mi wrote:
>>>>
>>>>> Changes:
>>>>> v5 -> v6:
>>>>>   * no function change.
>>>>>   * rebase patchset to latest code of perf-tool-next tree.
>>>>>   * Add Kan's reviewed-by tag.
>>>>>
>>>>> History:
>>>>>   v4: https://lore.kernel.org/all/20240816122938.32228-1-dapeng1.mi@linux.intel.com/
>>>>>   v3: https://lore.kernel.org/all/20240712170339.185824-1-dapeng1.mi@linux.intel.com/
>>>>>   v2: https://lore.kernel.org/all/20240708144204.839486-1-dapeng1.mi@linux.intel.com/
>>>>>   v1: https://lore.kernel.org/all/20240702224037.343958-1-dapeng1.mi@linux.intel.com/
>>>>>
>>>>> [...]
>>>>
>>>> Applied to perf-tools-next, thanks!
>>>
>>> I disagreed with an early patch set and the issue wasn't resolved. Specifically:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/commit/?h=perf-tools-next&id=3b5edc0421e2598a0ae7f0adcd592017f37e3cdf
>>> ```
>>>   /* Followed by topdown events. */
>>>   if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
>>>   return -1;
>>> - if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs))
>>> + /*
>>> + * Move topdown events forward only when topdown events
>>> + * are not in same group with previous event.
>>> + */
>>> + if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs) &&
>>> +     lhs->core.leader != rhs->core.leader)
>>>   return 1;
>>> ```
>>> Is a broken comparator as the lhs then rhs behavior varies from the
>>> rhs then lhs behavior. The qsort implementation can randomly order the
>>> events.
>>> Please drop/revert.
>>
>> Can you please provide an example when it's broken?  I'm not sure how it
>> can produce new errors, but it seems to fix a specific problem.  Do you
>> have a new test failure after this change?
> 
> It may work with a particular sort routine in a particular library
> today, the issue is that if the sort routine were say changed from:
> 
> if (cmp(a, b) < 0)
> 
> to:
> 
> if (cmp(b, a) > 0)
> 
> the sort would vary with this change even though such a change in the
> sorting code is a no-op. It is fundamental algorithms that this code
> is broken, I'm not going to spend the time finding a list of
> instructions and a sort routine to demonstrate this.


I'm not sure I fully understand. But just want to mention that the
change only impacts the Topdown perf metric group, which is only
available for the ICL and later p-core. Nothing will change if no perf
metrics topdown events are used. Very limited impact.

I like the patch set is because it provides examples and tests to cover
the possible combination of the slots event and topdown metrics events.
So we will have a clear expectation for different combinations caused by
the perf metrics mess.

If the algorithms cannot be changed, can you please give some
suggestions, especially for the sample read failure?

Thanks,
Kan

Re: [Patch v5 0/6] Bug fixes on topdown events reordering

[Ian Rogers]

On Thu, Oct 3, 2024 at 7:57 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
>
>
> On 2024-10-02 8:57 p.m., Ian Rogers wrote:
> > On Wed, Oct 2, 2024 at 5:00 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >>
> >> On Tue, Oct 01, 2024 at 03:32:04PM -0700, Ian Rogers wrote:
> >>> On Tue, Oct 1, 2024 at 2:02 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >>>>
> >>>> On Fri, 13 Sep 2024 08:47:06 +0000, Dapeng Mi wrote:
> >>>>
> >>>>> Changes:
> >>>>> v5 -> v6:
> >>>>>   * no function change.
> >>>>>   * rebase patchset to latest code of perf-tool-next tree.
> >>>>>   * Add Kan's reviewed-by tag.
> >>>>>
> >>>>> History:
> >>>>>   v4: https://lore.kernel.org/all/20240816122938.32228-1-dapeng1.mi@linux.intel.com/
> >>>>>   v3: https://lore.kernel.org/all/20240712170339.185824-1-dapeng1.mi@linux.intel.com/
> >>>>>   v2: https://lore.kernel.org/all/20240708144204.839486-1-dapeng1.mi@linux.intel.com/
> >>>>>   v1: https://lore.kernel.org/all/20240702224037.343958-1-dapeng1.mi@linux.intel.com/
> >>>>>
> >>>>> [...]
> >>>>
> >>>> Applied to perf-tools-next, thanks!
> >>>
> >>> I disagreed with an early patch set and the issue wasn't resolved. Specifically:
> >>>
> >>> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/commit/?h=perf-tools-next&id=3b5edc0421e2598a0ae7f0adcd592017f37e3cdf
> >>> ```
> >>>   /* Followed by topdown events. */
> >>>   if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
> >>>   return -1;
> >>> - if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs))
> >>> + /*
> >>> + * Move topdown events forward only when topdown events
> >>> + * are not in same group with previous event.
> >>> + */
> >>> + if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs) &&
> >>> +     lhs->core.leader != rhs->core.leader)
> >>>   return 1;
> >>> ```
> >>> Is a broken comparator as the lhs then rhs behavior varies from the
> >>> rhs then lhs behavior. The qsort implementation can randomly order the
> >>> events.
> >>> Please drop/revert.
> >>
> >> Can you please provide an example when it's broken?  I'm not sure how it
> >> can produce new errors, but it seems to fix a specific problem.  Do you
> >> have a new test failure after this change?
> >
> > It may work with a particular sort routine in a particular library
> > today, the issue is that if the sort routine were say changed from:
> >
> > if (cmp(a, b) < 0)
> >
> > to:
> >
> > if (cmp(b, a) > 0)
> >
> > the sort would vary with this change even though such a change in the
> > sorting code is a no-op. It is fundamental algorithms that this code
> > is broken, I'm not going to spend the time finding a list of
> > instructions and a sort routine to demonstrate this.
>
>
> I'm not sure I fully understand. But just want to mention that the
> change only impacts the Topdown perf metric group, which is only
> available for the ICL and later p-core. Nothing will change if no perf
> metrics topdown events are used. Very limited impact.

How is breaking every ICL and later Intel model (excluding atoms)
limited impact?

> I like the patch set is because it provides examples and tests to cover
> the possible combination of the slots event and topdown metrics events.
> So we will have a clear expectation for different combinations caused by
> the perf metrics mess.

Tests are good. I have no problem with the change, possibly it is
inefficient, except that hacking a sort comparator to not be symmetric
is broken.

> If the algorithms cannot be changed, can you please give some
> suggestions, especially for the sample read failure?

So this is symmetric:
```
if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
  return -1;
if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs))
  return 1;
```
That is were lhs and rhs swapped then you'd get the expected comparison order.
```
if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs) &&
lhs->core.leader != rhs->core.leader)
  return -1;
if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs) &&
lhs->core.leader != rhs->core.leader)
  return 1;
```
Is symmetric as well.
```
if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
  return -1;
if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs) &&
lhs->core.leader != rhs->core.leader)
  return 1;
```
(what this patch does) is not symmetric as the group leader impacts
the greater-than case but not the less-than case.

It is not uncommon to see in a sort function:
```
if (cmp(a, b) <= 0) {
  assert(cmp(b,a) >= 0 && "check for unstable/broken compare functions");
```
and we could add this here:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/lib/list_sort.c?h=perf-tools-next#n22
Try searching up "Comparison method violates its general contract"
which is what Java throws when its TimSort implementation detects
cases like this. If you break the comparator you can get the sort into
an infinite loop - maybe that's enough of an indication that the code
is broken and you don't need the exception. As is common in kernel
code we're missing guard rails in list_sort, were the comparator
passed to qsort then expectations on breakage are a roll of the dice.

I believe when I originally gave this feedback I didn't think the fix
was to do it in the comparator and maybe an additional pass over the
list was going to be necessary. Basically a sort needs to be a sort
and it can't kind of be a sort depending on the order of the
comparison, this is just incurring tech debt and when a sort tweak
happens everything will break. As the usual chump cleaning up this
tech debt I'd prefer it wasn't introduced.

Fwiw, I refuse to carry this change in:
https://github.com/googleprodkernel/linux-perf/commits/google_tools_master/
and if the tests break as a consequence then the appropriate thing is
to revert those too. Linus/upstream maintainers follow a different
plan and that's their business, I can't build off of this code.

It is unclear to me why we get patches that have been pointed out to
be broken rebased/resent repeatedly without the broken-ness corrected.
For example, the abuse of aggregation for metrics in perf script. At
least point to the disagreement in the commit message, call me an
idiot, and let the maintainer make an informed or uninformed choice. I
am frequently an idiot and I apologize for all those cases, to the
best of my knowledge this isn't one of them.

Thanks,
Ian

Re: [Patch v5 0/6] Bug fixes on topdown events reordering

[Namhyung Kim]

On Thu, Oct 03, 2024 at 08:55:22AM -0700, Ian Rogers wrote:
> On Thu, Oct 3, 2024 at 7:57 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
> >
> >
> >
> > On 2024-10-02 8:57 p.m., Ian Rogers wrote:
> > > On Wed, Oct 2, 2024 at 5:00 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > >>
> > >> On Tue, Oct 01, 2024 at 03:32:04PM -0700, Ian Rogers wrote:
> > >>> On Tue, Oct 1, 2024 at 2:02 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > >>>>
> > >>>> On Fri, 13 Sep 2024 08:47:06 +0000, Dapeng Mi wrote:
> > >>>>
> > >>>>> Changes:
> > >>>>> v5 -> v6:
> > >>>>>   * no function change.
> > >>>>>   * rebase patchset to latest code of perf-tool-next tree.
> > >>>>>   * Add Kan's reviewed-by tag.
> > >>>>>
> > >>>>> History:
> > >>>>>   v4: https://lore.kernel.org/all/20240816122938.32228-1-dapeng1.mi@linux.intel.com/
> > >>>>>   v3: https://lore.kernel.org/all/20240712170339.185824-1-dapeng1.mi@linux.intel.com/
> > >>>>>   v2: https://lore.kernel.org/all/20240708144204.839486-1-dapeng1.mi@linux.intel.com/
> > >>>>>   v1: https://lore.kernel.org/all/20240702224037.343958-1-dapeng1.mi@linux.intel.com/
> > >>>>>
> > >>>>> [...]
> > >>>>
> > >>>> Applied to perf-tools-next, thanks!
> > >>>
> > >>> I disagreed with an early patch set and the issue wasn't resolved. Specifically:
> > >>>
> > >>> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/commit/?h=perf-tools-next&id=3b5edc0421e2598a0ae7f0adcd592017f37e3cdf
> > >>> ```
> > >>>   /* Followed by topdown events. */
> > >>>   if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
> > >>>   return -1;
> > >>> - if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs))
> > >>> + /*
> > >>> + * Move topdown events forward only when topdown events
> > >>> + * are not in same group with previous event.
> > >>> + */
> > >>> + if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs) &&
> > >>> +     lhs->core.leader != rhs->core.leader)
> > >>>   return 1;
> > >>> ```
> > >>> Is a broken comparator as the lhs then rhs behavior varies from the
> > >>> rhs then lhs behavior. The qsort implementation can randomly order the
> > >>> events.
> > >>> Please drop/revert.
> > >>
> > >> Can you please provide an example when it's broken?  I'm not sure how it
> > >> can produce new errors, but it seems to fix a specific problem.  Do you
> > >> have a new test failure after this change?
> > >
> > > It may work with a particular sort routine in a particular library
> > > today, the issue is that if the sort routine were say changed from:
> > >
> > > if (cmp(a, b) < 0)
> > >
> > > to:
> > >
> > > if (cmp(b, a) > 0)
> > >
> > > the sort would vary with this change even though such a change in the
> > > sorting code is a no-op. It is fundamental algorithms that this code
> > > is broken, I'm not going to spend the time finding a list of
> > > instructions and a sort routine to demonstrate this.
> >
> >
> > I'm not sure I fully understand. But just want to mention that the
> > change only impacts the Topdown perf metric group, which is only
> > available for the ICL and later p-core. Nothing will change if no perf
> > metrics topdown events are used. Very limited impact.
> 
> How is breaking every ICL and later Intel model (excluding atoms)
> limited impact?

I guess he meant it's only for Topdown metric groups with an incorrect
order on those models.

> 
> > I like the patch set is because it provides examples and tests to cover
> > the possible combination of the slots event and topdown metrics events.
> > So we will have a clear expectation for different combinations caused by
> > the perf metrics mess.
> 
> Tests are good. I have no problem with the change, possibly it is
> inefficient, except that hacking a sort comparator to not be symmetric
> is broken.

Ok.

> 
> > If the algorithms cannot be changed, can you please give some
> > suggestions, especially for the sample read failure?
> 
> So this is symmetric:
> ```
> if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
>   return -1;
> if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs))
>   return 1;
> ```
> That is were lhs and rhs swapped then you'd get the expected comparison order.
> ```
> if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs) &&
> lhs->core.leader != rhs->core.leader)
>   return -1;
> if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs) &&
> lhs->core.leader != rhs->core.leader)
>   return 1;
> ```
> Is symmetric as well.
> ```
> if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
>   return -1;
> if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs) &&
> lhs->core.leader != rhs->core.leader)
>   return 1;
> ```
> (what this patch does) is not symmetric as the group leader impacts
> the greater-than case but not the less-than case.
> 
> It is not uncommon to see in a sort function:
> ```
> if (cmp(a, b) <= 0) {
>   assert(cmp(b,a) >= 0 && "check for unstable/broken compare functions");
> ```

I see.  So are you proposing this?

diff --git a/tools/perf/arch/x86/util/evlist.c b/tools/perf/arch/x86/util/evlist.c
index 438e4639fa892304..46884fa17fe658a6 100644
--- a/tools/perf/arch/x86/util/evlist.c
+++ b/tools/perf/arch/x86/util/evlist.c
@@ -70,7 +70,8 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
                if (arch_is_topdown_slots(rhs))
                        return 1;
                /* Followed by topdown events. */
-               if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
+               if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs) &&
+                   lhs->core.leader != rhs->core.leader)
                        return -1;
                /*
                 * Move topdown events forward only when topdown events

Dapeng and Kan, can you verify if it's ok?  My quick tests look ok.


> and we could add this here:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/lib/list_sort.c?h=perf-tools-next#n22
> Try searching up "Comparison method violates its general contract"
> which is what Java throws when its TimSort implementation detects
> cases like this. If you break the comparator you can get the sort into
> an infinite loop - maybe that's enough of an indication that the code
> is broken and you don't need the exception. As is common in kernel
> code we're missing guard rails in list_sort, were the comparator
> passed to qsort then expectations on breakage are a roll of the dice.
> 
> I believe when I originally gave this feedback I didn't think the fix
> was to do it in the comparator and maybe an additional pass over the
> list was going to be necessary. Basically a sort needs to be a sort
> and it can't kind of be a sort depending on the order of the
> comparison, this is just incurring tech debt and when a sort tweak
> happens everything will break. As the usual chump cleaning up this
> tech debt I'd prefer it wasn't introduced.

Sorry, I think I didn't follow the previous thread and missed your
feedback.

> 
> Fwiw, I refuse to carry this change in:
> https://github.com/googleprodkernel/linux-perf/commits/google_tools_master/
> and if the tests break as a consequence then the appropriate thing is
> to revert those too. Linus/upstream maintainers follow a different
> plan and that's their business, I can't build off of this code.
> 
> It is unclear to me why we get patches that have been pointed out to
> be broken rebased/resent repeatedly without the broken-ness corrected.
> For example, the abuse of aggregation for metrics in perf script. At
> least point to the disagreement in the commit message, call me an
> idiot, and let the maintainer make an informed or uninformed choice. I
> am frequently an idiot and I apologize for all those cases, to the
> best of my knowledge this isn't one of them.

Yeah I understand your point and it sounds reasonable.  Thanks for your
feedback.  I don't think I'll revert the change though, we can add the
fix on top instead.

Thanks,
Namhyung

Re: [Patch v5 0/6] Bug fixes on topdown events reordering

[Liang, Kan]

On 2024-10-03 12:45 p.m., Namhyung Kim wrote:
>>> If the algorithms cannot be changed, can you please give some
>>> suggestions, especially for the sample read failure?
>> So this is symmetric:
>> ```
>> if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
>>   return -1;
>> if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs))
>>   return 1;
>> ```
>> That is were lhs and rhs swapped then you'd get the expected comparison order.
>> ```
>> if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs) &&
>> lhs->core.leader != rhs->core.leader)
>>   return -1;
>> if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs) &&
>> lhs->core.leader != rhs->core.leader)
>>   return 1;
>> ```
>> Is symmetric as well.
>> ```
>> if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
>>   return -1;
>> if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs) &&
>> lhs->core.leader != rhs->core.leader)
>>   return 1;
>> ```
>> (what this patch does) is not symmetric as the group leader impacts
>> the greater-than case but not the less-than case.
>>
>> It is not uncommon to see in a sort function:
>> ```
>> if (cmp(a, b) <= 0) {
>>   assert(cmp(b,a) >= 0 && "check for unstable/broken compare functions");
>> ```
> I see.  So are you proposing this?
> 
> diff --git a/tools/perf/arch/x86/util/evlist.c b/tools/perf/arch/x86/util/evlist.c
> index 438e4639fa892304..46884fa17fe658a6 100644
> --- a/tools/perf/arch/x86/util/evlist.c
> +++ b/tools/perf/arch/x86/util/evlist.c
> @@ -70,7 +70,8 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
>                 if (arch_is_topdown_slots(rhs))
>                         return 1;
>                 /* Followed by topdown events. */
> -               if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
> +               if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs) &&
> +                   lhs->core.leader != rhs->core.leader)
>                         return -1;
>                 /*
>                  * Move topdown events forward only when topdown events
> 
> Dapeng and Kan, can you verify if it's ok?  My quick tests look ok.

I verified the above change. It works well.

Tested-by: Kan Liang <kan.liang@linux.intel.com>

Thanks,
Kan

Re: [Patch v5 0/6] Bug fixes on topdown events reordering

[Ian Rogers]

On Thu, Oct 3, 2024 at 12:45 PM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
>
>
> On 2024-10-03 12:45 p.m., Namhyung Kim wrote:
> >>> If the algorithms cannot be changed, can you please give some
> >>> suggestions, especially for the sample read failure?
> >> So this is symmetric:
> >> ```
> >> if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
> >>   return -1;
> >> if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs))
> >>   return 1;
> >> ```
> >> That is were lhs and rhs swapped then you'd get the expected comparison order.
> >> ```
> >> if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs) &&
> >> lhs->core.leader != rhs->core.leader)
> >>   return -1;
> >> if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs) &&
> >> lhs->core.leader != rhs->core.leader)
> >>   return 1;
> >> ```
> >> Is symmetric as well.
> >> ```
> >> if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
> >>   return -1;
> >> if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs) &&
> >> lhs->core.leader != rhs->core.leader)
> >>   return 1;
> >> ```
> >> (what this patch does) is not symmetric as the group leader impacts
> >> the greater-than case but not the less-than case.
> >>
> >> It is not uncommon to see in a sort function:
> >> ```
> >> if (cmp(a, b) <= 0) {
> >>   assert(cmp(b,a) >= 0 && "check for unstable/broken compare functions");
> >> ```
> > I see.  So are you proposing this?
> >
> > diff --git a/tools/perf/arch/x86/util/evlist.c b/tools/perf/arch/x86/util/evlist.c
> > index 438e4639fa892304..46884fa17fe658a6 100644
> > --- a/tools/perf/arch/x86/util/evlist.c
> > +++ b/tools/perf/arch/x86/util/evlist.c
> > @@ -70,7 +70,8 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
> >                 if (arch_is_topdown_slots(rhs))
> >                         return 1;
> >                 /* Followed by topdown events. */
> > -               if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
> > +               if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs) &&
> > +                   lhs->core.leader != rhs->core.leader)
> >                         return -1;
> >                 /*
> >                  * Move topdown events forward only when topdown events
> >
> > Dapeng and Kan, can you verify if it's ok?  My quick tests look ok.
>
> I verified the above change. It works well.
>
> Tested-by: Kan Liang <kan.liang@linux.intel.com>

Dapeng's comment should cover replace the comment /* Followed by
topdown events. */ but there are other things amiss. I'm thinking of
something like: "slots,cycles,{instructions,topdown-be-bound}" the
topdown-be-bound should get sorted and grouped with slots, but cycles
and instructions have no reason to be reordered, so do we end up with
slots, instructions and topdown-be-bound being grouped with cycles
sitting ungrouped in the middle of the evlist? I believe there are
assumptions that grouped evsels are adjacent in the evlist, not least
in:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/parse-events.c?h=perf-tools-next#n2106
Does cycles instructions end up being broken out of a group in this
case? Which feels like the case the code was trying to avoid.

Thanks,
Ian

Re: [Patch v5 0/6] Bug fixes on topdown events reordering

[Namhyung Kim]

On Thu, Oct 03, 2024 at 02:26:29PM -0700, Ian Rogers wrote:
> On Thu, Oct 3, 2024 at 12:45 PM Liang, Kan <kan.liang@linux.intel.com> wrote:
> >
> >
> >
> > On 2024-10-03 12:45 p.m., Namhyung Kim wrote:
> > >>> If the algorithms cannot be changed, can you please give some
> > >>> suggestions, especially for the sample read failure?
> > >> So this is symmetric:
> > >> ```
> > >> if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
> > >>   return -1;
> > >> if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs))
> > >>   return 1;
> > >> ```
> > >> That is were lhs and rhs swapped then you'd get the expected comparison order.
> > >> ```
> > >> if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs) &&
> > >> lhs->core.leader != rhs->core.leader)
> > >>   return -1;
> > >> if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs) &&
> > >> lhs->core.leader != rhs->core.leader)
> > >>   return 1;
> > >> ```
> > >> Is symmetric as well.
> > >> ```
> > >> if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
> > >>   return -1;
> > >> if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs) &&
> > >> lhs->core.leader != rhs->core.leader)
> > >>   return 1;
> > >> ```
> > >> (what this patch does) is not symmetric as the group leader impacts
> > >> the greater-than case but not the less-than case.
> > >>
> > >> It is not uncommon to see in a sort function:
> > >> ```
> > >> if (cmp(a, b) <= 0) {
> > >>   assert(cmp(b,a) >= 0 && "check for unstable/broken compare functions");
> > >> ```
> > > I see.  So are you proposing this?
> > >
> > > diff --git a/tools/perf/arch/x86/util/evlist.c b/tools/perf/arch/x86/util/evlist.c
> > > index 438e4639fa892304..46884fa17fe658a6 100644
> > > --- a/tools/perf/arch/x86/util/evlist.c
> > > +++ b/tools/perf/arch/x86/util/evlist.c
> > > @@ -70,7 +70,8 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
> > >                 if (arch_is_topdown_slots(rhs))
> > >                         return 1;
> > >                 /* Followed by topdown events. */
> > > -               if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
> > > +               if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs) &&
> > > +                   lhs->core.leader != rhs->core.leader)
> > >                         return -1;
> > >                 /*
> > >                  * Move topdown events forward only when topdown events
> > >
> > > Dapeng and Kan, can you verify if it's ok?  My quick tests look ok.
> >
> > I verified the above change. It works well.
> >
> > Tested-by: Kan Liang <kan.liang@linux.intel.com>

Thanks for the check!

> 
> Dapeng's comment should cover replace the comment /* Followed by
> topdown events. */ but there are other things amiss. I'm thinking of
> something like: "slots,cycles,{instructions,topdown-be-bound}" the
> topdown-be-bound should get sorted and grouped with slots, but cycles
> and instructions have no reason to be reordered, so do we end up with
> slots, instructions and topdown-be-bound being grouped with cycles
> sitting ungrouped in the middle of the evlist? I believe there are
> assumptions that grouped evsels are adjacent in the evlist, not least
> in:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/parse-events.c?h=perf-tools-next#n2106
> Does cycles instructions end up being broken out of a group in this
> case? Which feels like the case the code was trying to avoid.

I got this:

  $ sudo ./perf record -a -e "slots,cycles,{instructions,topdown-be-bound}" true
  Error:
  The sys_perf_event_open() syscall returned with 22 (Invalid argument) for event (topdown-be-bound).
  "dmesg | grep -i perf" may provide additional information.

Thanks,
Namhyung

Re: [Patch v5 0/6] Bug fixes on topdown events reordering

[Liang, Kan]

On 2024-10-03 6:13 p.m., Namhyung Kim wrote:
>> Dapeng's comment should cover replace the comment /* Followed by
>> topdown events. */ but there are other things amiss. I'm thinking of
>> something like: "slots,cycles,{instructions,topdown-be-bound}" the
>> topdown-be-bound should get sorted and grouped with slots, but cycles
>> and instructions have no reason to be reordered, so do we end up with
>> slots, instructions and topdown-be-bound being grouped with cycles
>> sitting ungrouped in the middle of the evlist? I believe there are
>> assumptions that grouped evsels are adjacent in the evlist, not least
>> in:
>> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/parse-events.c?h=perf-tools-next#n2106
>> Does cycles instructions end up being broken out of a group in this
>> case? Which feels like the case the code was trying to avoid.
> I got this:
> 
>   $ sudo ./perf record -a -e "slots,cycles,{instructions,topdown-be-bound}" true
>   Error:
>   The sys_perf_event_open() syscall returned with 22 (Invalid argument) for event (topdown-be-bound).
>   "dmesg | grep -i perf" may provide additional information.

To be honest, I think the "slots,cycles,{instructions,topdown-be-bound}"
is a meaningless case. Why a user wants to group instructions and
topdown events, but leave the slots out of the group?
There could be hundreds of different combinations caused by the perf
metrics mess. I don't think the re-ordering code should/can fix all of them.

For the case which the re-ordering cannot cover (like above), an error
out is acceptable. So the end user can update their command to a more
meaningful format, either {slots,cycles,instructions,topdown-be-bound}
or {slots,topdown-be-bound},cycles,instructions still works.

I think what the patch set really fixed is the failure of sample read
with perf metrics. Without the patch set, it never works no matter how
you change the order of the events.
A better ordering is just a nice to have feature. If perf cannot
provides a perfect re-ordering, I think an error out is also OK.

Thanks,
Kan

Re: [Patch v5 0/6] Bug fixes on topdown events reordering

[Ian Rogers]

On Thu, Oct 3, 2024 at 4:29 PM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
>
>
> On 2024-10-03 6:13 p.m., Namhyung Kim wrote:
> >> Dapeng's comment should cover replace the comment /* Followed by
> >> topdown events. */ but there are other things amiss. I'm thinking of
> >> something like: "slots,cycles,{instructions,topdown-be-bound}" the
> >> topdown-be-bound should get sorted and grouped with slots, but cycles
> >> and instructions have no reason to be reordered, so do we end up with
> >> slots, instructions and topdown-be-bound being grouped with cycles
> >> sitting ungrouped in the middle of the evlist? I believe there are
> >> assumptions that grouped evsels are adjacent in the evlist, not least
> >> in:
> >> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/parse-events.c?h=perf-tools-next#n2106
> >> Does cycles instructions end up being broken out of a group in this
> >> case? Which feels like the case the code was trying to avoid.
> > I got this:
> >
> >   $ sudo ./perf record -a -e "slots,cycles,{instructions,topdown-be-bound}" true
> >   Error:
> >   The sys_perf_event_open() syscall returned with 22 (Invalid argument) for event (topdown-be-bound).
> >   "dmesg | grep -i perf" may provide additional information.
>
> To be honest, I think the "slots,cycles,{instructions,topdown-be-bound}"
> is a meaningless case. Why a user wants to group instructions and
> topdown events, but leave the slots out of the group?
> There could be hundreds of different combinations caused by the perf
> metrics mess. I don't think the re-ordering code should/can fix all of them.

I'm happy with better code and things don't need to be perfect. Can we
fix the comments though? It'd be nice to also include that some things
are going to be broken. I can imagine groups with topdown events but
without slots, for example we group events in metrics and in
tma_retiring we add "0 * tma_info_thread_slots" to the metric so that
we get a slots event. If the multiply were optimized away as redundant
then we'd have a topdown group without slots, we could pick up slots
and other events from other metrics.

> For the case which the re-ordering cannot cover (like above), an error
> out is acceptable. So the end user can update their command to a more
> meaningful format, either {slots,cycles,instructions,topdown-be-bound}
> or {slots,topdown-be-bound},cycles,instructions still works.

Perhaps we can add an arch error path that could help more for topdown
events given they are a particular pain to open.

> I think what the patch set really fixed is the failure of sample read
> with perf metrics. Without the patch set, it never works no matter how
> you change the order of the events.
> A better ordering is just a nice to have feature. If perf cannot
> provides a perfect re-ordering, I think an error out is also OK.

Agreed, we don't need to fix everything and focussing on the common
use cases makes sense.

Thanks,
Ian

Re: [Patch v5 0/6] Bug fixes on topdown events reordering

[Namhyung Kim]

On Thu, Oct 03, 2024 at 04:36:25PM -0700, Ian Rogers wrote:
> On Thu, Oct 3, 2024 at 4:29 PM Liang, Kan <kan.liang@linux.intel.com> wrote:
> >
> >
> >
> > On 2024-10-03 6:13 p.m., Namhyung Kim wrote:
> > >> Dapeng's comment should cover replace the comment /* Followed by
> > >> topdown events. */ but there are other things amiss. I'm thinking of
> > >> something like: "slots,cycles,{instructions,topdown-be-bound}" the
> > >> topdown-be-bound should get sorted and grouped with slots, but cycles
> > >> and instructions have no reason to be reordered, so do we end up with
> > >> slots, instructions and topdown-be-bound being grouped with cycles
> > >> sitting ungrouped in the middle of the evlist? I believe there are
> > >> assumptions that grouped evsels are adjacent in the evlist, not least
> > >> in:
> > >> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/parse-events.c?h=perf-tools-next#n2106
> > >> Does cycles instructions end up being broken out of a group in this
> > >> case? Which feels like the case the code was trying to avoid.
> > > I got this:
> > >
> > >   $ sudo ./perf record -a -e "slots,cycles,{instructions,topdown-be-bound}" true
> > >   Error:
> > >   The sys_perf_event_open() syscall returned with 22 (Invalid argument) for event (topdown-be-bound).
> > >   "dmesg | grep -i perf" may provide additional information.
> >
> > To be honest, I think the "slots,cycles,{instructions,topdown-be-bound}"
> > is a meaningless case. Why a user wants to group instructions and
> > topdown events, but leave the slots out of the group?
> > There could be hundreds of different combinations caused by the perf
> > metrics mess. I don't think the re-ordering code should/can fix all of them.
> 
> I'm happy with better code and things don't need to be perfect. Can we
> fix the comments though? It'd be nice to also include that some things
> are going to be broken. I can imagine groups with topdown events but

Can you please propose something?

Thanks,
Namhyung


> without slots, for example we group events in metrics and in
> tma_retiring we add "0 * tma_info_thread_slots" to the metric so that
> we get a slots event. If the multiply were optimized away as redundant
> then we'd have a topdown group without slots, we could pick up slots
> and other events from other metrics.

RE: [Patch v5 0/6] Bug fixes on topdown events reordering

[Mi, Dapeng1]

> -----Original Message-----
> From: Ian Rogers <irogers@google.com>
> Sent: Friday, October 4, 2024 7:36 AM
> To: Liang, Kan <kan.liang@linux.intel.com>
> Cc: Namhyung Kim <namhyung@kernel.org>; Peter Zijlstra
> <peterz@infradead.org>; Ingo Molnar <mingo@redhat.com>; Arnaldo Carvalho
> de Melo <acme@kernel.org>; Hunter, Adrian <adrian.hunter@intel.com>;
> Alexander Shishkin <alexander.shishkin@linux.intel.com>; Dapeng Mi
> <dapeng1.mi@linux.intel.com>; linux-perf-users@vger.kernel.org; linux-
> kernel@vger.kernel.org; Yongwei Ma <yongwei.ma@intel.com>; Mi, Dapeng1
> <dapeng1.mi@intel.com>
> Subject: Re: [Patch v5 0/6] Bug fixes on topdown events reordering
> 
> On Thu, Oct 3, 2024 at 4:29 PM Liang, Kan <kan.liang@linux.intel.com> wrote:
> >
> >
> >
> > On 2024-10-03 6:13 p.m., Namhyung Kim wrote:
> > >> Dapeng's comment should cover replace the comment /* Followed by
> > >> topdown events. */ but there are other things amiss. I'm thinking
> > >> of something like: "slots,cycles,{instructions,topdown-be-bound}"
> > >> the topdown-be-bound should get sorted and grouped with slots, but
> > >> cycles and instructions have no reason to be reordered, so do we
> > >> end up with slots, instructions and topdown-be-bound being grouped
> > >> with cycles sitting ungrouped in the middle of the evlist? I
> > >> believe there are assumptions that grouped evsels are adjacent in
> > >> the evlist, not least
> > >> in:
> > >> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-nex
> > >> t.git/tree/tools/perf/util/parse-events.c?h=perf-tools-next#n2106
> > >> Does cycles instructions end up being broken out of a group in this
> > >> case? Which feels like the case the code was trying to avoid.
> > > I got this:
> > >
> > >   $ sudo ./perf record -a -e "slots,cycles,{instructions,topdown-be-bound}"
> true
> > >   Error:
> > >   The sys_perf_event_open() syscall returned with 22 (Invalid argument) for
> event (topdown-be-bound).
> > >   "dmesg | grep -i perf" may provide additional information.
> >
> > To be honest, I think the "slots,cycles,{instructions,topdown-be-bound}"
> > is a meaningless case. Why a user wants to group instructions and
> > topdown events, but leave the slots out of the group?
> > There could be hundreds of different combinations caused by the perf
> > metrics mess. I don't think the re-ordering code should/can fix all of them.
> 
> I'm happy with better code and things don't need to be perfect. Can we fix the
> comments though? It'd be nice to also include that some things are going to be
> broken. I can imagine groups with topdown events but without slots, for example
> we group events in metrics and in tma_retiring we add "0 *
> tma_info_thread_slots" to the metric so that we get a slots event. If the multiply
> were optimized away as redundant then we'd have a topdown group without
> slots, we could pick up slots and other events from other metrics.

I don't think this patch would break any current regroup case. It just blocks to move topdown metrics event if they are already in same group with slot events. We can see same error for this event combination "slots,cycles,{instructions,topdown-be-bound}" in the original code.

As Kan said, there could be hundreds of topdown metrics combinations, it's unrealistic to cover all these combinations just using sorting, and even it can be done, the comparator would become much complicated and hard to maintain.

I think we'd better just maintain several commonly used regroup cases, it would be fine to raise an error for these unsupported combinations as long as error message is clear enough.

Ian, I don't fully understand your words, could you please give an example? Thanks.


> 
> > For the case which the re-ordering cannot cover (like above), an error
> > out is acceptable. So the end user can update their command to a more
> > meaningful format, either {slots,cycles,instructions,topdown-be-bound}
> > or {slots,topdown-be-bound},cycles,instructions still works.
> 
> Perhaps we can add an arch error path that could help more for topdown events
> given they are a particular pain to open.
> 
> > I think what the patch set really fixed is the failure of sample read
> > with perf metrics. Without the patch set, it never works no matter how
> > you change the order of the events.
> > A better ordering is just a nice to have feature. If perf cannot
> > provides a perfect re-ordering, I think an error out is also OK.
> 
> Agreed, we don't need to fix everything and focussing on the common use cases
> makes sense.
> 
> Thanks,
> Ian

Re: [Patch v5 0/6] Bug fixes on topdown events reordering

[Ian Rogers]

On Mon, Oct 7, 2024 at 7:52 PM Mi, Dapeng1 <dapeng1.mi@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Ian Rogers <irogers@google.com>
> > Sent: Friday, October 4, 2024 7:36 AM
> > To: Liang, Kan <kan.liang@linux.intel.com>
> > Cc: Namhyung Kim <namhyung@kernel.org>; Peter Zijlstra
> > <peterz@infradead.org>; Ingo Molnar <mingo@redhat.com>; Arnaldo Carvalho
> > de Melo <acme@kernel.org>; Hunter, Adrian <adrian.hunter@intel.com>;
> > Alexander Shishkin <alexander.shishkin@linux.intel.com>; Dapeng Mi
> > <dapeng1.mi@linux.intel.com>; linux-perf-users@vger.kernel.org; linux-
> > kernel@vger.kernel.org; Yongwei Ma <yongwei.ma@intel.com>; Mi, Dapeng1
> > <dapeng1.mi@intel.com>
> > Subject: Re: [Patch v5 0/6] Bug fixes on topdown events reordering
> >
> > On Thu, Oct 3, 2024 at 4:29 PM Liang, Kan <kan.liang@linux.intel.com> wrote:
> > >
> > >
> > >
> > > On 2024-10-03 6:13 p.m., Namhyung Kim wrote:
> > > >> Dapeng's comment should cover replace the comment /* Followed by
> > > >> topdown events. */ but there are other things amiss. I'm thinking
> > > >> of something like: "slots,cycles,{instructions,topdown-be-bound}"
> > > >> the topdown-be-bound should get sorted and grouped with slots, but
> > > >> cycles and instructions have no reason to be reordered, so do we
> > > >> end up with slots, instructions and topdown-be-bound being grouped
> > > >> with cycles sitting ungrouped in the middle of the evlist? I
> > > >> believe there are assumptions that grouped evsels are adjacent in
> > > >> the evlist, not least
> > > >> in:
> > > >> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-nex
> > > >> t.git/tree/tools/perf/util/parse-events.c?h=perf-tools-next#n2106
> > > >> Does cycles instructions end up being broken out of a group in this
> > > >> case? Which feels like the case the code was trying to avoid.
> > > > I got this:
> > > >
> > > >   $ sudo ./perf record -a -e "slots,cycles,{instructions,topdown-be-bound}"
> > true
> > > >   Error:
> > > >   The sys_perf_event_open() syscall returned with 22 (Invalid argument) for
> > event (topdown-be-bound).
> > > >   "dmesg | grep -i perf" may provide additional information.
> > >
> > > To be honest, I think the "slots,cycles,{instructions,topdown-be-bound}"
> > > is a meaningless case. Why a user wants to group instructions and
> > > topdown events, but leave the slots out of the group?
> > > There could be hundreds of different combinations caused by the perf
> > > metrics mess. I don't think the re-ordering code should/can fix all of them.
> >
> > I'm happy with better code and things don't need to be perfect. Can we fix the
> > comments though? It'd be nice to also include that some things are going to be
> > broken. I can imagine groups with topdown events but without slots, for example
> > we group events in metrics and in tma_retiring we add "0 *
> > tma_info_thread_slots" to the metric so that we get a slots event. If the multiply
> > were optimized away as redundant then we'd have a topdown group without
> > slots, we could pick up slots and other events from other metrics.
>
> I don't think this patch would break any current regroup case. It just blocks to move topdown metrics event if they are already in same group with slot events. We can see same error for this event combination "slots,cycles,{instructions,topdown-be-bound}" in the original code.
>
> As Kan said, there could be hundreds of topdown metrics combinations, it's unrealistic to cover all these combinations just using sorting, and even it can be done, the comparator would become much complicated and hard to maintain.
>
> I think we'd better just maintain several commonly used regroup cases, it would be fine to raise an error for these unsupported combinations as long as error message is clear enough.
>
> Ian, I don't fully understand your words, could you please give an example? Thanks.

So in the comparator I think most people won't understand the list of
cases that are supported and those that are not supported:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/arch/x86/util/evlist.c?h=perf-tools-next#n35
The groups usually come from metrics and to work around issues there
are constraints on those that may or may not group events. This could
yield situations that don't work given the cases you list, but I don't
think current metrics will violate this. We do test metrics
individually but not together as part of perf test.

Thanks,
Ian

> >
> > > For the case which the re-ordering cannot cover (like above), an error
> > > out is acceptable. So the end user can update their command to a more
> > > meaningful format, either {slots,cycles,instructions,topdown-be-bound}
> > > or {slots,topdown-be-bound},cycles,instructions still works.
> >
> > Perhaps we can add an arch error path that could help more for topdown events
> > given they are a particular pain to open.
> >
> > > I think what the patch set really fixed is the failure of sample read
> > > with perf metrics. Without the patch set, it never works no matter how
> > > you change the order of the events.
> > > A better ordering is just a nice to have feature. If perf cannot
> > > provides a perfect re-ordering, I think an error out is also OK.
> >
> > Agreed, we don't need to fix everything and focussing on the common use cases
> > makes sense.
> >
> > Thanks,
> > Ian

RE: [Patch v5 0/6] Bug fixes on topdown events reordering

[Mi, Dapeng1]

> -----Original Message-----
> From: Ian Rogers <irogers@google.com>
> Sent: Friday, October 4, 2024 5:26 AM
> To: Liang, Kan <kan.liang@linux.intel.com>
> Cc: Namhyung Kim <namhyung@kernel.org>; Peter Zijlstra
> <peterz@infradead.org>; Ingo Molnar <mingo@redhat.com>; Arnaldo Carvalho
> de Melo <acme@kernel.org>; Hunter, Adrian <adrian.hunter@intel.com>;
> Alexander Shishkin <alexander.shishkin@linux.intel.com>; Dapeng Mi
> <dapeng1.mi@linux.intel.com>; linux-perf-users@vger.kernel.org; linux-
> kernel@vger.kernel.org; Yongwei Ma <yongwei.ma@intel.com>; Mi, Dapeng1
> <dapeng1.mi@intel.com>
> Subject: Re: [Patch v5 0/6] Bug fixes on topdown events reordering
> 
> On Thu, Oct 3, 2024 at 12:45 PM Liang, Kan <kan.liang@linux.intel.com> wrote:
> >
> >
> >
> > On 2024-10-03 12:45 p.m., Namhyung Kim wrote:
> > >>> If the algorithms cannot be changed, can you please give some
> > >>> suggestions, especially for the sample read failure?
> > >> So this is symmetric:
> > >> ```
> > >> if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
> > >>   return -1;
> > >> if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs))
> > >>   return 1;
> > >> ```
> > >> That is were lhs and rhs swapped then you'd get the expected comparison
> order.
> > >> ```
> > >> if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs)
> > >> &&
> > >> lhs->core.leader != rhs->core.leader)
> > >>   return -1;
> > >> if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs)
> > >> &&
> > >> lhs->core.leader != rhs->core.leader)
> > >>   return 1;
> > >> ```
> > >> Is symmetric as well.
> > >> ```
> > >> if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
> > >>   return -1;
> > >> if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs)
> > >> &&
> > >> lhs->core.leader != rhs->core.leader)
> > >>   return 1;
> > >> ```
> > >> (what this patch does) is not symmetric as the group leader impacts
> > >> the greater-than case but not the less-than case.
> > >>
> > >> It is not uncommon to see in a sort function:
> > >> ```
> > >> if (cmp(a, b) <= 0) {
> > >>   assert(cmp(b,a) >= 0 && "check for unstable/broken compare
> > >> functions"); ```
> > > I see.  So are you proposing this?
> > >
> > > diff --git a/tools/perf/arch/x86/util/evlist.c
> > > b/tools/perf/arch/x86/util/evlist.c
> > > index 438e4639fa892304..46884fa17fe658a6 100644
> > > --- a/tools/perf/arch/x86/util/evlist.c
> > > +++ b/tools/perf/arch/x86/util/evlist.c
> > > @@ -70,7 +70,8 @@ int arch_evlist__cmp(const struct evsel *lhs, const
> struct evsel *rhs)
> > >                 if (arch_is_topdown_slots(rhs))
> > >                         return 1;
> > >                 /* Followed by topdown events. */
> > > -               if (arch_is_topdown_metrics(lhs)
> && !arch_is_topdown_metrics(rhs))
> > > +               if (arch_is_topdown_metrics(lhs)
> && !arch_is_topdown_metrics(rhs) &&
> > > +                   lhs->core.leader != rhs->core.leader)
> > >                         return -1;
> > >                 /*
> > >                  * Move topdown events forward only when topdown
> > > events
> > >
> > > Dapeng and Kan, can you verify if it's ok?  My quick tests look ok.
> >
> > I verified the above change. It works well.
> >
> > Tested-by: Kan Liang <kan.liang@linux.intel.com>
> 
> Dapeng's comment should cover replace the comment /* Followed by topdown
> events. */ but there are other things amiss. I'm thinking of something like:

Thanks. I would change the comments.

> "slots,cycles,{instructions,topdown-be-bound}" the topdown-be-bound should get
> sorted and grouped with slots, but cycles and instructions have no reason to be
> reordered, so do we end up with slots, instructions and topdown-be-bound being
> grouped with cycles sitting ungrouped in the middle of the evlist? I believe there
> are assumptions that grouped evsels are adjacent in the evlist, not least
> in:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-
> next.git/tree/tools/perf/util/parse-events.c?h=perf-tools-next#n2106
> Does cycles instructions end up being broken out of a group in this case? Which
> feels like the case the code was trying to avoid.
> 
> Thanks,
> Ian

RE: [Patch v5 0/6] Bug fixes on topdown events reordering

[Mi, Dapeng1]

> -----Original Message-----
> From: Liang, Kan <kan.liang@linux.intel.com>
> Sent: Friday, October 4, 2024 3:45 AM
> To: Namhyung Kim <namhyung@kernel.org>; Ian Rogers <irogers@google.com>
> Cc: Peter Zijlstra <peterz@infradead.org>; Ingo Molnar <mingo@redhat.com>;
> Arnaldo Carvalho de Melo <acme@kernel.org>; Hunter, Adrian
> <adrian.hunter@intel.com>; Alexander Shishkin
> <alexander.shishkin@linux.intel.com>; Dapeng Mi
> <dapeng1.mi@linux.intel.com>; linux-perf-users@vger.kernel.org; linux-
> kernel@vger.kernel.org; Yongwei Ma <yongwei.ma@intel.com>; Mi, Dapeng1
> <dapeng1.mi@intel.com>
> Subject: Re: [Patch v5 0/6] Bug fixes on topdown events reordering
> 
> 
> 
> On 2024-10-03 12:45 p.m., Namhyung Kim wrote:
> >>> If the algorithms cannot be changed, can you please give some
> >>> suggestions, especially for the sample read failure?
> >> So this is symmetric:
> >> ```
> >> if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
> >>   return -1;
> >> if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs))
> >>   return 1;
> >> ```
> >> That is were lhs and rhs swapped then you'd get the expected comparison
> order.
> >> ```
> >> if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs) &&
> >> lhs->core.leader != rhs->core.leader)
> >>   return -1;
> >> if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs) &&
> >> lhs->core.leader != rhs->core.leader)
> >>   return 1;
> >> ```
> >> Is symmetric as well.
> >> ```
> >> if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
> >>   return -1;
> >> if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs) &&
> >> lhs->core.leader != rhs->core.leader)
> >>   return 1;
> >> ```
> >> (what this patch does) is not symmetric as the group leader impacts
> >> the greater-than case but not the less-than case.
> >>
> >> It is not uncommon to see in a sort function:
> >> ```
> >> if (cmp(a, b) <= 0) {
> >>   assert(cmp(b,a) >= 0 && "check for unstable/broken compare
> >> functions"); ```
> > I see.  So are you proposing this?
> >
> > diff --git a/tools/perf/arch/x86/util/evlist.c
> > b/tools/perf/arch/x86/util/evlist.c
> > index 438e4639fa892304..46884fa17fe658a6 100644
> > --- a/tools/perf/arch/x86/util/evlist.c
> > +++ b/tools/perf/arch/x86/util/evlist.c
> > @@ -70,7 +70,8 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct
> evsel *rhs)
> >                 if (arch_is_topdown_slots(rhs))
> >                         return 1;
> >                 /* Followed by topdown events. */
> > -               if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
> > +               if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs)
> &&
> > +                   lhs->core.leader != rhs->core.leader)
> >                         return -1;
> >                 /*
> >                  * Move topdown events forward only when topdown
> > events
> >
> > Dapeng and Kan, can you verify if it's ok?  My quick tests look ok.
> 
> I verified the above change. It works well.
> 
> Tested-by: Kan Liang <kan.liang@linux.intel.com>

Since the linux.intel.com mail server is down, I have to use the intel.com mail server (outlook client) to respond this mail. The mail format may vary. Sorry for this.

Thanks Kan for testing this.

Ian, thanks for pointing that the comparators are not symmetrical. I agree it would lead to an inconsistent sorting results if changing comparing condition from "cmp(a, b) < 0" to "cmp(b, a) > 0" or vice versa, but limit to some certain sort library like perf-tool, the sorting result should be still fixed, right?

Anyway, I would provide a new patch to make the comparators are symmetrical. Thanks.

> 
> Thanks,
> Kan