[PATCH v2 0/3] perf list: Remove duplicate PMUs

[PATCH v2 0/3] perf list: Remove duplicate PMUs

[Ian Rogers]

When there are multiple PMUs, differing by ordered suffixes, by
default only display one. This avoids repeated listing of events, in
particular when there are 10s of uncore PMUs. If "-v" is passed to
"perf list" then still list all PMUs.

Listing fewer PMU/event combinations helps speed the all PMU event
tests.

Before:
```
$ perf list
...
  uncore_imc_free_running_0/data_read/               [Kernel PMU event]
  uncore_imc_free_running_0/data_total/              [Kernel PMU event]
  uncore_imc_free_running_0/data_write/              [Kernel PMU event]
  uncore_imc_free_running_1/data_read/               [Kernel PMU event]
  uncore_imc_free_running_1/data_total/              [Kernel PMU event]
  uncore_imc_free_running_1/data_write/              [Kernel PMU event]
```

After:
```
$ perf list
...
  uncore_imc_free_running/data_read/                 [Kernel PMU event]
  uncore_imc_free_running/data_total/                [Kernel PMU event]
  uncore_imc_free_running/data_write/                [Kernel PMU event]
...
$ perf list -v
...
  uncore_imc_free_running_0/data_read/               [Kernel PMU event]
  uncore_imc_free_running_0/data_total/              [Kernel PMU event]
  uncore_imc_free_running_0/data_write/              [Kernel PMU event]
  uncore_imc_free_running_1/data_read/               [Kernel PMU event]
  uncore_imc_free_running_1/data_total/              [Kernel PMU event]
  uncore_imc_free_running_1/data_write/              [Kernel PMU event]
...
```

The PMUs are sorted by name then suffix as a part of this change.

v2: List all PMUs when "-v" is passed as suggested by John Garry
    <john.g.garry@oracle.com>.

Ian Rogers (3):
  perf pmus: Sort pmus by name then suffix
  perf pmus: Add scan that ignores duplicates, use for perf list
  perf pmus: Don't print duplicate PMU suffix in list by default

 tools/perf/builtin-list.c      |   8 +++
 tools/perf/util/pmus.c         | 120 +++++++++++++++++++++++++++++++--
 tools/perf/util/print-events.h |   1 +
 3 files changed, 122 insertions(+), 7 deletions(-)

-- 
2.41.0.640.ga95def55d0-goog

[PATCH v2 1/3] perf pmus: Sort pmus by name then suffix

[Ian Rogers]

Sort PMUs by name. If two PMUs have the same name but differ by
suffix, sort the suffixes numerically. For example, "breakpoint" comes
before "cpu", "uncore_imc_free_running_0" comes before
"uncore_imc_free_running_1".

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/pmus.c | 48 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
index c58ba9fb6a36..3581710667b0 100644
--- a/tools/perf/util/pmus.c
+++ b/tools/perf/util/pmus.c
@@ -1,8 +1,10 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <linux/list.h>
+#include <linux/list_sort.h>
 #include <linux/zalloc.h>
 #include <subcmd/pager.h>
 #include <sys/types.h>
+#include <ctype.h>
 #include <dirent.h>
 #include <pthread.h>
 #include <string.h>
@@ -33,6 +35,31 @@ static LIST_HEAD(other_pmus);
 static bool read_sysfs_core_pmus;
 static bool read_sysfs_all_pmus;
 
+static int pmu_name_len_no_suffix(const char *str, unsigned long *num)
+{
+	int orig_len, len;
+
+	orig_len = len = strlen(str);
+
+	/* Non-uncore PMUs have their full length, for example, i915. */
+	if (strncmp(str, "uncore_", 7))
+		return len;
+
+	/*
+	 * Count trailing digits and '_', if '_{num}' suffix isn't present use
+	 * the full length.
+	 */
+	while (len > 0 && isdigit(str[len - 1]))
+		len--;
+
+	if (len > 0 && len != orig_len && str[len - 1] == '_') {
+		if (num)
+			*num = strtoul(&str[len], NULL, 10);
+		return len - 1;
+	}
+	return orig_len;
+}
+
 void perf_pmus__destroy(void)
 {
 	struct perf_pmu *pmu, *tmp;
@@ -122,6 +149,25 @@ static struct perf_pmu *perf_pmu__find2(int dirfd, const char *name)
 	return perf_pmu__lookup(core_pmu ? &core_pmus : &other_pmus, dirfd, name);
 }
 
+static int pmus_cmp(void *priv __maybe_unused,
+		    const struct list_head *lhs, const struct list_head *rhs)
+{
+	unsigned long lhs_num, rhs_num;
+	struct perf_pmu *lhs_pmu = container_of(lhs, struct perf_pmu, list);
+	struct perf_pmu *rhs_pmu = container_of(rhs, struct perf_pmu, list);
+	const char *lhs_pmu_name = lhs_pmu->name ?: "";
+	const char *rhs_pmu_name = rhs_pmu->name ?: "";
+	int lhs_pmu_name_len = pmu_name_len_no_suffix(lhs_pmu_name, &lhs_num);
+	int rhs_pmu_name_len = pmu_name_len_no_suffix(rhs_pmu_name, &rhs_num);
+	int ret = strncmp(lhs_pmu_name, rhs_pmu_name,
+			lhs_pmu_name_len < rhs_pmu_name_len ? lhs_pmu_name_len : rhs_pmu_name_len);
+
+	if (lhs_pmu_name_len != rhs_pmu_name_len || ret != 0 || lhs_pmu_name_len == 0)
+		return ret;
+
+	return lhs_num < rhs_num ? -1 : (lhs_num > rhs_num ? 1 : 0);
+}
+
 /* Add all pmus in sysfs to pmu list: */
 static void pmu_read_sysfs(bool core_only)
 {
@@ -156,6 +202,8 @@ static void pmu_read_sysfs(bool core_only)
 		if (!perf_pmu__create_placeholder_core_pmu(&core_pmus))
 			pr_err("Failure to set up any core PMUs\n");
 	}
+	list_sort(NULL, &core_pmus, pmus_cmp);
+	list_sort(NULL, &other_pmus, pmus_cmp);
 	if (!list_empty(&core_pmus)) {
 		read_sysfs_core_pmus = true;
 		if (!core_only)
-- 
2.41.0.640.ga95def55d0-goog

[PATCH v2 2/3] perf pmus: Add scan that ignores duplicates, use for perf list

[Ian Rogers]

When there are multiple PMUs that differ only by suffix, by default
just list the first one and skip all others. As the PMUs are sorted,
the scan routine checks that the PMU names match and the numbers are
consecutive. If "-v" is passed to "perf list" then list all PMUs.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/builtin-list.c      |  8 +++++
 tools/perf/util/pmus.c         | 54 ++++++++++++++++++++++++++++++++--
 tools/perf/util/print-events.h |  1 +
 3 files changed, 61 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
index 7fec2cca759f..8fe4ddf02c14 100644
--- a/tools/perf/builtin-list.c
+++ b/tools/perf/builtin-list.c
@@ -423,6 +423,13 @@ static void json_print_metric(void *ps __maybe_unused, const char *group,
 	strbuf_release(&buf);
 }
 
+static bool default_skip_duplicate_pmus(void *ps)
+{
+	struct print_state *print_state = ps;
+
+	return !print_state->long_desc;
+}
+
 int cmd_list(int argc, const char **argv)
 {
 	int i, ret = 0;
@@ -434,6 +441,7 @@ int cmd_list(int argc, const char **argv)
 		.print_end = default_print_end,
 		.print_event = default_print_event,
 		.print_metric = default_print_metric,
+		.skip_duplicate_pmus = default_skip_duplicate_pmus,
 	};
 	const char *cputype = NULL;
 	const char *unit_name = NULL;
diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
index 3581710667b0..5073843aca19 100644
--- a/tools/perf/util/pmus.c
+++ b/tools/perf/util/pmus.c
@@ -275,6 +275,50 @@ struct perf_pmu *perf_pmus__scan_core(struct perf_pmu *pmu)
 	return NULL;
 }
 
+static struct perf_pmu *perf_pmus__scan_skip_duplicates(struct perf_pmu *pmu)
+{
+	bool use_core_pmus = !pmu || pmu->is_core;
+	int last_pmu_name_len = 0;
+	unsigned long last_pmu_num = 0;
+	const char *last_pmu_name = (pmu && pmu->name) ? pmu->name : "";
+
+	if (!pmu) {
+		pmu_read_sysfs(/*core_only=*/false);
+		pmu = list_prepare_entry(pmu, &core_pmus, list);
+	} else
+		last_pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &last_pmu_num);
+
+	if (use_core_pmus) {
+		list_for_each_entry_continue(pmu, &core_pmus, list) {
+			unsigned long pmu_num = 0;
+			int pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &pmu_num);
+
+			if (last_pmu_name_len == pmu_name_len &&
+			    (last_pmu_num + 1 == pmu_num) &&
+			    !strncmp(last_pmu_name, pmu->name ?: "", pmu_name_len)) {
+				last_pmu_num++;
+				continue;
+			}
+			return pmu;
+		}
+		pmu = NULL;
+		pmu = list_prepare_entry(pmu, &other_pmus, list);
+	}
+	list_for_each_entry_continue(pmu, &other_pmus, list) {
+		unsigned long pmu_num = 0;
+		int pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &pmu_num);
+
+		if (last_pmu_name_len == pmu_name_len &&
+		    (last_pmu_num + 1 == pmu_num) &&
+		    !strncmp(last_pmu_name, pmu->name ?: "", pmu_name_len)) {
+			last_pmu_num++;
+			continue;
+		}
+		return pmu;
+	}
+	return NULL;
+}
+
 const struct perf_pmu *perf_pmus__pmu_for_pmu_filter(const char *str)
 {
 	struct perf_pmu *pmu = NULL;
@@ -429,10 +473,16 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
 	int printed = 0;
 	int len, j;
 	struct sevent *aliases;
+	struct perf_pmu *(*scan_fn)(struct perf_pmu *);
+
+	if (print_cb->skip_duplicate_pmus(print_state))
+		scan_fn = perf_pmus__scan_skip_duplicates;
+	else
+		scan_fn = perf_pmus__scan;
 
 	pmu = NULL;
 	len = 0;
-	while ((pmu = perf_pmus__scan(pmu)) != NULL) {
+	while ((pmu = scan_fn(pmu)) != NULL) {
 		list_for_each_entry(event, &pmu->aliases, list)
 			len++;
 		if (pmu->selectable)
@@ -445,7 +495,7 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
 	}
 	pmu = NULL;
 	j = 0;
-	while ((pmu = perf_pmus__scan(pmu)) != NULL) {
+	while ((pmu = scan_fn(pmu)) != NULL) {
 		bool is_cpu = pmu->is_core;
 
 		list_for_each_entry(event, &pmu->aliases, list) {
diff --git a/tools/perf/util/print-events.h b/tools/perf/util/print-events.h
index d7fab411e75c..bf4290bef0cd 100644
--- a/tools/perf/util/print-events.h
+++ b/tools/perf/util/print-events.h
@@ -26,6 +26,7 @@ struct print_callbacks {
 			const char *expr,
 			const char *threshold,
 			const char *unit);
+	bool (*skip_duplicate_pmus)(void *print_state);
 };
 
 /** Print all events, the default when no options are specified. */
-- 
2.41.0.640.ga95def55d0-goog

[PATCH v2 3/3] perf pmus: Don't print duplicate PMU suffix in list by default

[Ian Rogers]

Duplicate PMUs are no longer printed by default but the suffix of the
first is printed. When duplicate PMUs are being skipped avoid printing
the suffix.

Before:
```
$ perf list
...
  uncore_imc_free_running_0/data_read/               [Kernel PMU event]
  uncore_imc_free_running_0/data_total/              [Kernel PMU event]
  uncore_imc_free_running_0/data_write/              [Kernel PMU event]
```

After:
```
$ perf list
...
  uncore_imc_free_running/data_read/                 [Kernel PMU event]
  uncore_imc_free_running/data_total/                [Kernel PMU event]
  uncore_imc_free_running/data_write/                [Kernel PMU event]
...
$ perf list -v
  uncore_imc_free_running_0/data_read/               [Kernel PMU event]
  uncore_imc_free_running_0/data_total/              [Kernel PMU event]
  uncore_imc_free_running_0/data_write/              [Kernel PMU event]
  uncore_imc_free_running_1/data_read/               [Kernel PMU event]
  uncore_imc_free_running_1/data_total/              [Kernel PMU event]
  uncore_imc_free_running_1/data_write/              [Kernel PMU event]
...
```

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/pmus.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
index 5073843aca19..b0ecb2e5bdcc 100644
--- a/tools/perf/util/pmus.c
+++ b/tools/perf/util/pmus.c
@@ -440,10 +440,13 @@ static int sub_non_neg(int a, int b)
 }
 
 static char *format_alias(char *buf, int len, const struct perf_pmu *pmu,
-			  const struct perf_pmu_alias *alias)
+			  const struct perf_pmu_alias *alias, bool skip_duplicate_pmus)
 {
 	struct parse_events_term *term;
-	int used = snprintf(buf, len, "%s/%s", pmu->name, alias->name);
+	int pmu_name_len = skip_duplicate_pmus
+		? pmu_name_len_no_suffix(pmu->name, /*num=*/NULL)
+		: (int)strlen(pmu->name);
+	int used = snprintf(buf, len, "%.*s/%s", pmu_name_len, pmu->name, alias->name);
 
 	list_for_each_entry(term, &alias->terms, list) {
 		if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR)
@@ -473,9 +476,10 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
 	int printed = 0;
 	int len, j;
 	struct sevent *aliases;
+	bool skip_duplicate_pmus = print_cb->skip_duplicate_pmus(print_state);
 	struct perf_pmu *(*scan_fn)(struct perf_pmu *);
 
-	if (print_cb->skip_duplicate_pmus(print_state))
+	if (skip_duplicate_pmus)
 		scan_fn = perf_pmus__scan_skip_duplicates;
 	else
 		scan_fn = perf_pmus__scan;
@@ -518,6 +522,7 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
 			*desc = NULL, *long_desc = NULL,
 			*encoding_desc = NULL, *topic = NULL,
 			*pmu_name = NULL;
+		int pmu_name_len;
 		bool deprecated = false;
 		size_t buf_used;
 
@@ -528,7 +533,8 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
 		if (!aliases[j].event) {
 			/* A selectable event. */
 			pmu_name = aliases[j].pmu->name;
-			buf_used = snprintf(buf, sizeof(buf), "%s//", pmu_name) + 1;
+			pmu_name_len = pmu_name_len_no_suffix(pmu_name, /*num=*/NULL);
+			buf_used = snprintf(buf, sizeof(buf), "%.*s//", pmu_name_len, pmu_name) + 1;
 			name = buf;
 		} else {
 			if (aliases[j].event->desc) {
@@ -536,7 +542,7 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
 				buf_used = 0;
 			} else {
 				name = format_alias(buf, sizeof(buf), aliases[j].pmu,
-						    aliases[j].event);
+						    aliases[j].event, skip_duplicate_pmus);
 				if (aliases[j].is_cpu) {
 					alias = name;
 					name = aliases[j].event->name;
@@ -554,8 +560,10 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
 			long_desc = aliases[j].event->long_desc;
 			topic = aliases[j].event->topic;
 			encoding_desc = buf + buf_used;
+			pmu_name_len = pmu_name_len_no_suffix(pmu_name, /*num=*/NULL);
 			buf_used += snprintf(buf + buf_used, sizeof(buf) - buf_used,
-					"%s/%s/", pmu_name, aliases[j].event->str) + 1;
+					"%.*s/%s/", pmu_name_len, pmu_name,
+					aliases[j].event->str) + 1;
 			deprecated = aliases[j].event->deprecated;
 		}
 		print_cb->print_event(print_state,
-- 
2.41.0.640.ga95def55d0-goog

Re: [PATCH v2 1/3] perf pmus: Sort pmus by name then suffix

[Arnaldo Carvalho de Melo]

Em Thu, Aug 10, 2023 at 02:49:50PM -0700, Ian Rogers escreveu:
> Sort PMUs by name. If two PMUs have the same name but differ by
> suffix, sort the suffixes numerically. For example, "breakpoint" comes
> before "cpu", "uncore_imc_free_running_0" comes before
> "uncore_imc_free_running_1".

Why is this needed?

- Arnaldo
 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/pmus.c | 48 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> index c58ba9fb6a36..3581710667b0 100644
> --- a/tools/perf/util/pmus.c
> +++ b/tools/perf/util/pmus.c
> @@ -1,8 +1,10 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include <linux/list.h>
> +#include <linux/list_sort.h>
>  #include <linux/zalloc.h>
>  #include <subcmd/pager.h>
>  #include <sys/types.h>
> +#include <ctype.h>
>  #include <dirent.h>
>  #include <pthread.h>
>  #include <string.h>
> @@ -33,6 +35,31 @@ static LIST_HEAD(other_pmus);
>  static bool read_sysfs_core_pmus;
>  static bool read_sysfs_all_pmus;
>  
> +static int pmu_name_len_no_suffix(const char *str, unsigned long *num)
> +{
> +	int orig_len, len;
> +
> +	orig_len = len = strlen(str);
> +
> +	/* Non-uncore PMUs have their full length, for example, i915. */
> +	if (strncmp(str, "uncore_", 7))
> +		return len;
> +
> +	/*
> +	 * Count trailing digits and '_', if '_{num}' suffix isn't present use
> +	 * the full length.
> +	 */
> +	while (len > 0 && isdigit(str[len - 1]))
> +		len--;
> +
> +	if (len > 0 && len != orig_len && str[len - 1] == '_') {
> +		if (num)
> +			*num = strtoul(&str[len], NULL, 10);
> +		return len - 1;
> +	}
> +	return orig_len;
> +}
> +
>  void perf_pmus__destroy(void)
>  {
>  	struct perf_pmu *pmu, *tmp;
> @@ -122,6 +149,25 @@ static struct perf_pmu *perf_pmu__find2(int dirfd, const char *name)
>  	return perf_pmu__lookup(core_pmu ? &core_pmus : &other_pmus, dirfd, name);
>  }
>  
> +static int pmus_cmp(void *priv __maybe_unused,
> +		    const struct list_head *lhs, const struct list_head *rhs)
> +{
> +	unsigned long lhs_num, rhs_num;
> +	struct perf_pmu *lhs_pmu = container_of(lhs, struct perf_pmu, list);
> +	struct perf_pmu *rhs_pmu = container_of(rhs, struct perf_pmu, list);
> +	const char *lhs_pmu_name = lhs_pmu->name ?: "";
> +	const char *rhs_pmu_name = rhs_pmu->name ?: "";
> +	int lhs_pmu_name_len = pmu_name_len_no_suffix(lhs_pmu_name, &lhs_num);
> +	int rhs_pmu_name_len = pmu_name_len_no_suffix(rhs_pmu_name, &rhs_num);
> +	int ret = strncmp(lhs_pmu_name, rhs_pmu_name,
> +			lhs_pmu_name_len < rhs_pmu_name_len ? lhs_pmu_name_len : rhs_pmu_name_len);
> +
> +	if (lhs_pmu_name_len != rhs_pmu_name_len || ret != 0 || lhs_pmu_name_len == 0)
> +		return ret;
> +
> +	return lhs_num < rhs_num ? -1 : (lhs_num > rhs_num ? 1 : 0);
> +}
> +
>  /* Add all pmus in sysfs to pmu list: */
>  static void pmu_read_sysfs(bool core_only)
>  {
> @@ -156,6 +202,8 @@ static void pmu_read_sysfs(bool core_only)
>  		if (!perf_pmu__create_placeholder_core_pmu(&core_pmus))
>  			pr_err("Failure to set up any core PMUs\n");
>  	}
> +	list_sort(NULL, &core_pmus, pmus_cmp);
> +	list_sort(NULL, &other_pmus, pmus_cmp);
>  	if (!list_empty(&core_pmus)) {
>  		read_sysfs_core_pmus = true;
>  		if (!core_only)
> -- 
> 2.41.0.640.ga95def55d0-goog
> 

-- 

- Arnaldo

Re: [PATCH v2 1/3] perf pmus: Sort pmus by name then suffix

[John Garry]

On 10/08/2023 22:49, Ian Rogers wrote:
> Sort PMUs by name. If two PMUs have the same name but differ by
> suffix

I think that the wording here can be improved. If they have the same 
name, then they cannot differ. I think that you mean that two PMUs have 
the same name apart from a difference in suffix.

> , sort the suffixes numerically.

I don't know how this will affect some hisi pmus which have special 
naming formats, like hisi_l3c_sscl1_4

> For example, "breakpoint" comes
> before "cpu", "uncore_imc_free_running_0" comes before
> "uncore_imc_free_running_1".
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>   tools/perf/util/pmus.c | 48 ++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 48 insertions(+)
> 
> diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> index c58ba9fb6a36..3581710667b0 100644
> --- a/tools/perf/util/pmus.c
> +++ b/tools/perf/util/pmus.c
> @@ -1,8 +1,10 @@
>   // SPDX-License-Identifier: GPL-2.0
>   #include <linux/list.h>
> +#include <linux/list_sort.h>
>   #include <linux/zalloc.h>
>   #include <subcmd/pager.h>
>   #include <sys/types.h>
> +#include <ctype.h>
>   #include <dirent.h>
>   #include <pthread.h>
>   #include <string.h>
> @@ -33,6 +35,31 @@ static LIST_HEAD(other_pmus);
>   static bool read_sysfs_core_pmus;
>   static bool read_sysfs_all_pmus;
>   
> +static int pmu_name_len_no_suffix(const char *str, unsigned long *num)
> +{
> +	int orig_len, len;
> +
> +	orig_len = len = strlen(str);
> +
> +	/* Non-uncore PMUs have their full length, for example, i915. */
> +	if (strncmp(str, "uncore_", 7))
> +		return len;
> +
> +	/*
> +	 * Count trailing digits and '_', if '_{num}' suffix isn't present use
> +	 * the full length.
> +	 */
> +	while (len > 0 && isdigit(str[len - 1]))
> +		len--;
> +
> +	if (len > 0 && len != orig_len && str[len - 1] == '_') {
> +		if (num)
> +			*num = strtoul(&str[len], NULL, 10);
> +		return len - 1;
> +	}
> +	return orig_len;
> +}
> +
>   void perf_pmus__destroy(void)
>   {
>   	struct perf_pmu *pmu, *tmp;
> @@ -122,6 +149,25 @@ static struct perf_pmu *perf_pmu__find2(int dirfd, const char *name)
>   	return perf_pmu__lookup(core_pmu ? &core_pmus : &other_pmus, dirfd, name);
>   }
>   
> +static int pmus_cmp(void *priv __maybe_unused,
> +		    const struct list_head *lhs, const struct list_head *rhs)
> +{
> +	unsigned long lhs_num, rhs_num;
> +	struct perf_pmu *lhs_pmu = container_of(lhs, struct perf_pmu, list);
> +	struct perf_pmu *rhs_pmu = container_of(rhs, struct perf_pmu, list);
> +	const char *lhs_pmu_name = lhs_pmu->name ?: "";
> +	const char *rhs_pmu_name = rhs_pmu->name ?: "";
> +	int lhs_pmu_name_len = pmu_name_len_no_suffix(lhs_pmu_name, &lhs_num);
> +	int rhs_pmu_name_len = pmu_name_len_no_suffix(rhs_pmu_name, &rhs_num);


This is a bit of a monster... at least it should have a comment on what 
it is doing. Do you consider your own version of strncmp which can 
handle numbers in the suffix as another solution?

> +	int ret = strncmp(lhs_pmu_name, rhs_pmu_name,
> +			lhs_pmu_name_len < rhs_pmu_name_len ? lhs_pmu_name_len : rhs_pmu_name_len);

Could min(lhs_pmu_name_len, rhs_pmu_name_len) be used here?

> +
> +	if (lhs_pmu_name_len != rhs_pmu_name_len || ret != 0 || lhs_pmu_name_len == 0)
> +		return ret;
> +
> +	return lhs_num < rhs_num ? -1 : (lhs_num > rhs_num ? 1 : 0);

double ternary operator on same line ain't great - can this be changed 
into multiple return statements and also commented.

> +}
> +
>   /* Add all pmus in sysfs to pmu list: */
>   static void pmu_read_sysfs(bool core_only)
>   {
> @@ -156,6 +202,8 @@ static void pmu_read_sysfs(bool core_only)
>   		if (!perf_pmu__create_placeholder_core_pmu(&core_pmus))
>   			pr_err("Failure to set up any core PMUs\n");
>   	}
> +	list_sort(NULL, &core_pmus, pmus_cmp);
> +	list_sort(NULL, &other_pmus, pmus_cmp);
>   	if (!list_empty(&core_pmus)) {
>   		read_sysfs_core_pmus = true;
>   		if (!core_only)

Re: [PATCH v2 1/3] perf pmus: Sort pmus by name then suffix

[Ian Rogers]

On Fri, Aug 11, 2023 at 7:00 AM John Garry <john.g.garry@oracle.com> wrote:
>
> On 10/08/2023 22:49, Ian Rogers wrote:
> > Sort PMUs by name. If two PMUs have the same name but differ by
> > suffix
>
> I think that the wording here can be improved. If they have the same
> name, then they cannot differ. I think that you mean that two PMUs have
> the same name apart from a difference in suffix.

Sure.

> > , sort the suffixes numerically.
>
> I don't know how this will affect some hisi pmus which have special
> naming formats, like hisi_l3c_sscl1_4

Anything not starting with uncore_ is assumed not to have a suffix.

> > For example, "breakpoint" comes
> > before "cpu", "uncore_imc_free_running_0" comes before
> > "uncore_imc_free_running_1".
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >   tools/perf/util/pmus.c | 48 ++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 48 insertions(+)
> >
> > diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> > index c58ba9fb6a36..3581710667b0 100644
> > --- a/tools/perf/util/pmus.c
> > +++ b/tools/perf/util/pmus.c
> > @@ -1,8 +1,10 @@
> >   // SPDX-License-Identifier: GPL-2.0
> >   #include <linux/list.h>
> > +#include <linux/list_sort.h>
> >   #include <linux/zalloc.h>
> >   #include <subcmd/pager.h>
> >   #include <sys/types.h>
> > +#include <ctype.h>
> >   #include <dirent.h>
> >   #include <pthread.h>
> >   #include <string.h>
> > @@ -33,6 +35,31 @@ static LIST_HEAD(other_pmus);
> >   static bool read_sysfs_core_pmus;
> >   static bool read_sysfs_all_pmus;
> >
> > +static int pmu_name_len_no_suffix(const char *str, unsigned long *num)
> > +{
> > +     int orig_len, len;
> > +
> > +     orig_len = len = strlen(str);
> > +
> > +     /* Non-uncore PMUs have their full length, for example, i915. */
> > +     if (strncmp(str, "uncore_", 7))
> > +             return len;
> > +
> > +     /*
> > +      * Count trailing digits and '_', if '_{num}' suffix isn't present use
> > +      * the full length.
> > +      */
> > +     while (len > 0 && isdigit(str[len - 1]))
> > +             len--;
> > +
> > +     if (len > 0 && len != orig_len && str[len - 1] == '_') {
> > +             if (num)
> > +                     *num = strtoul(&str[len], NULL, 10);
> > +             return len - 1;
> > +     }
> > +     return orig_len;
> > +}
> > +
> >   void perf_pmus__destroy(void)
> >   {
> >       struct perf_pmu *pmu, *tmp;
> > @@ -122,6 +149,25 @@ static struct perf_pmu *perf_pmu__find2(int dirfd, const char *name)
> >       return perf_pmu__lookup(core_pmu ? &core_pmus : &other_pmus, dirfd, name);
> >   }
> >
> > +static int pmus_cmp(void *priv __maybe_unused,
> > +                 const struct list_head *lhs, const struct list_head *rhs)
> > +{
> > +     unsigned long lhs_num, rhs_num;
> > +     struct perf_pmu *lhs_pmu = container_of(lhs, struct perf_pmu, list);
> > +     struct perf_pmu *rhs_pmu = container_of(rhs, struct perf_pmu, list);
> > +     const char *lhs_pmu_name = lhs_pmu->name ?: "";
> > +     const char *rhs_pmu_name = rhs_pmu->name ?: "";
> > +     int lhs_pmu_name_len = pmu_name_len_no_suffix(lhs_pmu_name, &lhs_num);
> > +     int rhs_pmu_name_len = pmu_name_len_no_suffix(rhs_pmu_name, &rhs_num);
>
>
> This is a bit of a monster... at least it should have a comment on what
> it is doing. Do you consider your own version of strncmp which can
> handle numbers in the suffix as another solution?

Sure, the intention is to be intention revealing getting a left hand
and right hand pmu name, the length of the name part and the suffix
number. I'm not sure a comment can do more than restate what the code
is doing.

> > +     int ret = strncmp(lhs_pmu_name, rhs_pmu_name,
> > +                     lhs_pmu_name_len < rhs_pmu_name_len ? lhs_pmu_name_len : rhs_pmu_name_len);
>
> Could min(lhs_pmu_name_len, rhs_pmu_name_len) be used here?

Right, there is a suitable definition in linux/kernel.h

> > +
> > +     if (lhs_pmu_name_len != rhs_pmu_name_len || ret != 0 || lhs_pmu_name_len == 0)
> > +             return ret;
> > +
> > +     return lhs_num < rhs_num ? -1 : (lhs_num > rhs_num ? 1 : 0);
>
> double ternary operator on same line ain't great - can this be changed
> into multiple return statements and also commented.

The alternative is:
return lhs_num  - rhs_num;
which removes the compares and has a bug around minimum integer
everybody ignores.

Thanks,
Ian

> > +}
> > +
> >   /* Add all pmus in sysfs to pmu list: */
> >   static void pmu_read_sysfs(bool core_only)
> >   {
> > @@ -156,6 +202,8 @@ static void pmu_read_sysfs(bool core_only)
> >               if (!perf_pmu__create_placeholder_core_pmu(&core_pmus))
> >                       pr_err("Failure to set up any core PMUs\n");
> >       }
> > +     list_sort(NULL, &core_pmus, pmus_cmp);
> > +     list_sort(NULL, &other_pmus, pmus_cmp);
> >       if (!list_empty(&core_pmus)) {
> >               read_sysfs_core_pmus = true;
> >               if (!core_only)
>

Re: [PATCH v2 1/3] perf pmus: Sort pmus by name then suffix

[Ian Rogers]

On Fri, Aug 11, 2023 at 6:56 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Thu, Aug 10, 2023 at 02:49:50PM -0700, Ian Rogers escreveu:
> > Sort PMUs by name. If two PMUs have the same name but differ by
> > suffix, sort the suffixes numerically. For example, "breakpoint" comes
> > before "cpu", "uncore_imc_free_running_0" comes before
> > "uncore_imc_free_running_1".
>
> Why is this needed?

It is needed so that in the later patches we just "perf list" the
uncore_imc_free_running_0 and skip all the other suffix numbers.
Sorting using strcmp isn't sufficient as consider uncore_imc_10 and
uncore_imc_9, where 9 would appear before 10 if only the characters
were being compared.

Thanks,
Ian

> - Arnaldo
>
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/pmus.c | 48 ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 48 insertions(+)
> >
> > diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> > index c58ba9fb6a36..3581710667b0 100644
> > --- a/tools/perf/util/pmus.c
> > +++ b/tools/perf/util/pmus.c
> > @@ -1,8 +1,10 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >  #include <linux/list.h>
> > +#include <linux/list_sort.h>
> >  #include <linux/zalloc.h>
> >  #include <subcmd/pager.h>
> >  #include <sys/types.h>
> > +#include <ctype.h>
> >  #include <dirent.h>
> >  #include <pthread.h>
> >  #include <string.h>
> > @@ -33,6 +35,31 @@ static LIST_HEAD(other_pmus);
> >  static bool read_sysfs_core_pmus;
> >  static bool read_sysfs_all_pmus;
> >
> > +static int pmu_name_len_no_suffix(const char *str, unsigned long *num)
> > +{
> > +     int orig_len, len;
> > +
> > +     orig_len = len = strlen(str);
> > +
> > +     /* Non-uncore PMUs have their full length, for example, i915. */
> > +     if (strncmp(str, "uncore_", 7))
> > +             return len;
> > +
> > +     /*
> > +      * Count trailing digits and '_', if '_{num}' suffix isn't present use
> > +      * the full length.
> > +      */
> > +     while (len > 0 && isdigit(str[len - 1]))
> > +             len--;
> > +
> > +     if (len > 0 && len != orig_len && str[len - 1] == '_') {
> > +             if (num)
> > +                     *num = strtoul(&str[len], NULL, 10);
> > +             return len - 1;
> > +     }
> > +     return orig_len;
> > +}
> > +
> >  void perf_pmus__destroy(void)
> >  {
> >       struct perf_pmu *pmu, *tmp;
> > @@ -122,6 +149,25 @@ static struct perf_pmu *perf_pmu__find2(int dirfd, const char *name)
> >       return perf_pmu__lookup(core_pmu ? &core_pmus : &other_pmus, dirfd, name);
> >  }
> >
> > +static int pmus_cmp(void *priv __maybe_unused,
> > +                 const struct list_head *lhs, const struct list_head *rhs)
> > +{
> > +     unsigned long lhs_num, rhs_num;
> > +     struct perf_pmu *lhs_pmu = container_of(lhs, struct perf_pmu, list);
> > +     struct perf_pmu *rhs_pmu = container_of(rhs, struct perf_pmu, list);
> > +     const char *lhs_pmu_name = lhs_pmu->name ?: "";
> > +     const char *rhs_pmu_name = rhs_pmu->name ?: "";
> > +     int lhs_pmu_name_len = pmu_name_len_no_suffix(lhs_pmu_name, &lhs_num);
> > +     int rhs_pmu_name_len = pmu_name_len_no_suffix(rhs_pmu_name, &rhs_num);
> > +     int ret = strncmp(lhs_pmu_name, rhs_pmu_name,
> > +                     lhs_pmu_name_len < rhs_pmu_name_len ? lhs_pmu_name_len : rhs_pmu_name_len);
> > +
> > +     if (lhs_pmu_name_len != rhs_pmu_name_len || ret != 0 || lhs_pmu_name_len == 0)
> > +             return ret;
> > +
> > +     return lhs_num < rhs_num ? -1 : (lhs_num > rhs_num ? 1 : 0);
> > +}
> > +
> >  /* Add all pmus in sysfs to pmu list: */
> >  static void pmu_read_sysfs(bool core_only)
> >  {
> > @@ -156,6 +202,8 @@ static void pmu_read_sysfs(bool core_only)
> >               if (!perf_pmu__create_placeholder_core_pmu(&core_pmus))
> >                       pr_err("Failure to set up any core PMUs\n");
> >       }
> > +     list_sort(NULL, &core_pmus, pmus_cmp);
> > +     list_sort(NULL, &other_pmus, pmus_cmp);
> >       if (!list_empty(&core_pmus)) {
> >               read_sysfs_core_pmus = true;
> >               if (!core_only)
> > --
> > 2.41.0.640.ga95def55d0-goog
> >
>
> --
>
> - Arnaldo

Re: [PATCH v2 1/3] perf pmus: Sort pmus by name then suffix

[Arnaldo Carvalho de Melo]

Em Fri, Aug 11, 2023 at 08:19:00AM -0700, Ian Rogers escreveu:
> On Fri, Aug 11, 2023 at 6:56 AM Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> > Em Thu, Aug 10, 2023 at 02:49:50PM -0700, Ian Rogers escreveu:
> > > Sort PMUs by name. If two PMUs have the same name but differ by
> > > suffix, sort the suffixes numerically. For example, "breakpoint" comes
> > > before "cpu", "uncore_imc_free_running_0" comes before
> > > "uncore_imc_free_running_1".
> >
> > Why is this needed?
> 
> It is needed so that in the later patches we just "perf list" the
> uncore_imc_free_running_0 and skip all the other suffix numbers.
> Sorting using strcmp isn't sufficient as consider uncore_imc_10 and
> uncore_imc_9, where 9 would appear before 10 if only the characters
> were being compared.

I think there will be a v2 for this series, from other reviews, so
please add this to this patch so that we know what is its intent in
addition to the description of what it is doing.
 
> Thanks,
> Ian
> 
> > - Arnaldo
> >
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > >  tools/perf/util/pmus.c | 48 ++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 48 insertions(+)
> > >
> > > diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> > > index c58ba9fb6a36..3581710667b0 100644
> > > --- a/tools/perf/util/pmus.c
> > > +++ b/tools/perf/util/pmus.c
> > > @@ -1,8 +1,10 @@
> > >  // SPDX-License-Identifier: GPL-2.0
> > >  #include <linux/list.h>
> > > +#include <linux/list_sort.h>
> > >  #include <linux/zalloc.h>
> > >  #include <subcmd/pager.h>
> > >  #include <sys/types.h>
> > > +#include <ctype.h>
> > >  #include <dirent.h>
> > >  #include <pthread.h>
> > >  #include <string.h>
> > > @@ -33,6 +35,31 @@ static LIST_HEAD(other_pmus);
> > >  static bool read_sysfs_core_pmus;
> > >  static bool read_sysfs_all_pmus;
> > >
> > > +static int pmu_name_len_no_suffix(const char *str, unsigned long *num)
> > > +{
> > > +     int orig_len, len;
> > > +
> > > +     orig_len = len = strlen(str);
> > > +
> > > +     /* Non-uncore PMUs have their full length, for example, i915. */
> > > +     if (strncmp(str, "uncore_", 7))
> > > +             return len;
> > > +
> > > +     /*
> > > +      * Count trailing digits and '_', if '_{num}' suffix isn't present use
> > > +      * the full length.
> > > +      */
> > > +     while (len > 0 && isdigit(str[len - 1]))
> > > +             len--;
> > > +
> > > +     if (len > 0 && len != orig_len && str[len - 1] == '_') {
> > > +             if (num)
> > > +                     *num = strtoul(&str[len], NULL, 10);
> > > +             return len - 1;
> > > +     }
> > > +     return orig_len;
> > > +}
> > > +
> > >  void perf_pmus__destroy(void)
> > >  {
> > >       struct perf_pmu *pmu, *tmp;
> > > @@ -122,6 +149,25 @@ static struct perf_pmu *perf_pmu__find2(int dirfd, const char *name)
> > >       return perf_pmu__lookup(core_pmu ? &core_pmus : &other_pmus, dirfd, name);
> > >  }
> > >
> > > +static int pmus_cmp(void *priv __maybe_unused,
> > > +                 const struct list_head *lhs, const struct list_head *rhs)
> > > +{
> > > +     unsigned long lhs_num, rhs_num;
> > > +     struct perf_pmu *lhs_pmu = container_of(lhs, struct perf_pmu, list);
> > > +     struct perf_pmu *rhs_pmu = container_of(rhs, struct perf_pmu, list);
> > > +     const char *lhs_pmu_name = lhs_pmu->name ?: "";
> > > +     const char *rhs_pmu_name = rhs_pmu->name ?: "";
> > > +     int lhs_pmu_name_len = pmu_name_len_no_suffix(lhs_pmu_name, &lhs_num);
> > > +     int rhs_pmu_name_len = pmu_name_len_no_suffix(rhs_pmu_name, &rhs_num);
> > > +     int ret = strncmp(lhs_pmu_name, rhs_pmu_name,
> > > +                     lhs_pmu_name_len < rhs_pmu_name_len ? lhs_pmu_name_len : rhs_pmu_name_len);
> > > +
> > > +     if (lhs_pmu_name_len != rhs_pmu_name_len || ret != 0 || lhs_pmu_name_len == 0)
> > > +             return ret;
> > > +
> > > +     return lhs_num < rhs_num ? -1 : (lhs_num > rhs_num ? 1 : 0);
> > > +}
> > > +
> > >  /* Add all pmus in sysfs to pmu list: */
> > >  static void pmu_read_sysfs(bool core_only)
> > >  {
> > > @@ -156,6 +202,8 @@ static void pmu_read_sysfs(bool core_only)
> > >               if (!perf_pmu__create_placeholder_core_pmu(&core_pmus))
> > >                       pr_err("Failure to set up any core PMUs\n");
> > >       }
> > > +     list_sort(NULL, &core_pmus, pmus_cmp);
> > > +     list_sort(NULL, &other_pmus, pmus_cmp);
> > >       if (!list_empty(&core_pmus)) {
> > >               read_sysfs_core_pmus = true;
> > >               if (!core_only)
> > > --
> > > 2.41.0.640.ga95def55d0-goog
> > >
> >
> > --
> >
> > - Arnaldo

-- 

- Arnaldo

Re: [PATCH v2 2/3] perf pmus: Add scan that ignores duplicates, use for perf list

[John Garry]

On 10/08/2023 22:49, Ian Rogers wrote:
> When there are multiple PMUs that differ only by suffix, by default
> just list the first one and skip all others. As the PMUs are sorted,
> the scan routine checks that the PMU names match and the numbers are
> consecutive. If "-v" is passed to "perf list" then list all PMUs.

I really think that this should be merged with the next change. I don't 
like the intermediate step of by default only printing the first PMU.

> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>   tools/perf/builtin-list.c      |  8 +++++
>   tools/perf/util/pmus.c         | 54 ++++++++++++++++++++++++++++++++--
>   tools/perf/util/print-events.h |  1 +
>   3 files changed, 61 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
> index 7fec2cca759f..8fe4ddf02c14 100644
> --- a/tools/perf/builtin-list.c
> +++ b/tools/perf/builtin-list.c
> @@ -423,6 +423,13 @@ static void json_print_metric(void *ps __maybe_unused, const char *group,
>   	strbuf_release(&buf);
>   }
>   
> +static bool default_skip_duplicate_pmus(void *ps)
> +{
> +	struct print_state *print_state = ps;
> +
> +	return !print_state->long_desc;
> +}
> +
>   int cmd_list(int argc, const char **argv)
>   {
>   	int i, ret = 0;
> @@ -434,6 +441,7 @@ int cmd_list(int argc, const char **argv)
>   		.print_end = default_print_end,
>   		.print_event = default_print_event,
>   		.print_metric = default_print_metric,
> +		.skip_duplicate_pmus = default_skip_duplicate_pmus,
>   	};
>   	const char *cputype = NULL;
>   	const char *unit_name = NULL;
> diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> index 3581710667b0..5073843aca19 100644
> --- a/tools/perf/util/pmus.c
> +++ b/tools/perf/util/pmus.c
> @@ -275,6 +275,50 @@ struct perf_pmu *perf_pmus__scan_core(struct perf_pmu *pmu)
>   	return NULL;
>   }
>   
> +static struct perf_pmu *perf_pmus__scan_skip_duplicates(struct perf_pmu *pmu)
> +{
> +	bool use_core_pmus = !pmu || pmu->is_core;
> +	int last_pmu_name_len = 0;
> +	unsigned long last_pmu_num = 0;
> +	const char *last_pmu_name = (pmu && pmu->name) ? pmu->name : "";
> +
> +	if (!pmu) {
> +		pmu_read_sysfs(/*core_only=*/false);
> +		pmu = list_prepare_entry(pmu, &core_pmus, list);
> +	} else
> +		last_pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &last_pmu_num);
> +
> +	if (use_core_pmus) {
> +		list_for_each_entry_continue(pmu, &core_pmus, list) {
> +			unsigned long pmu_num = 0;
> +			int pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &pmu_num);
> +
> +			if (last_pmu_name_len == pmu_name_len &&
> +			    (last_pmu_num + 1 == pmu_num) &&
> +			    !strncmp(last_pmu_name, pmu->name ?: "", pmu_name_len)) {
> +				last_pmu_num++;
> +				continue;
> +			}
> +			return pmu;
> +		}
> +		pmu = NULL;

you assign pmu NULL

> +		pmu = list_prepare_entry(pmu, &other_pmus, list);

and then re-assign it. If list_prepare_entry() needs first arg = NULL, 
then can just use NULL explicitly?

> +	}
> +	list_for_each_entry_continue(pmu, &other_pmus, list) {
> +		unsigned long pmu_num = 0;
> +		int pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &pmu_num);
> +
> +		if (last_pmu_name_len == pmu_name_len &&
> +		    (last_pmu_num + 1 == pmu_num) &&
> +		    !strncmp(last_pmu_name, pmu->name ?: "", pmu_name_len)) {
> +			last_pmu_num++;
> +			continue;

Can some of this code be factored out from the previous patch? It's 
doing something similar, right?

> +		}
> +		return pmu;
> +	}
> +	return NULL;
> +}
> +
>   const struct perf_pmu *perf_pmus__pmu_for_pmu_filter(const char *str)
>   {
>   	struct perf_pmu *pmu = NULL;
> @@ -429,10 +473,16 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
>   	int printed = 0;
>   	int len, j;
>   	struct sevent *aliases;
> +	struct perf_pmu *(*scan_fn)(struct perf_pmu *);
> +
> +	if (print_cb->skip_duplicate_pmus(print_state))
> +		scan_fn = perf_pmus__scan_skip_duplicates;
> +	else
> +		scan_fn = perf_pmus__scan;
>   
>   	pmu = NULL;
>   	len = 0;
> -	while ((pmu = perf_pmus__scan(pmu)) != NULL) {
> +	while ((pmu = scan_fn(pmu)) != NULL) {
>   		list_for_each_entry(event, &pmu->aliases, list)
>   			len++;
>   		if (pmu->selectable)
> @@ -445,7 +495,7 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
>   	}
>   	pmu = NULL;
>   	j = 0;
> -	while ((pmu = perf_pmus__scan(pmu)) != NULL) {
> +	while ((pmu = scan_fn(pmu)) != NULL) {
>   		bool is_cpu = pmu->is_core;
>   
>   		list_for_each_entry(event, &pmu->aliases, list) {
> diff --git a/tools/perf/util/print-events.h b/tools/perf/util/print-events.h
> index d7fab411e75c..bf4290bef0cd 100644
> --- a/tools/perf/util/print-events.h
> +++ b/tools/perf/util/print-events.h
> @@ -26,6 +26,7 @@ struct print_callbacks {
>   			const char *expr,
>   			const char *threshold,
>   			const char *unit);
> +	bool (*skip_duplicate_pmus)(void *print_state);
>   };
>   
>   /** Print all events, the default when no options are specified. */

Re: [PATCH v2 3/3] perf pmus: Don't print duplicate PMU suffix in list by default

[John Garry]

On 10/08/2023 22:49, Ian Rogers wrote:
> Duplicate PMUs are no longer printed by default but the suffix of the
> first is printed. When duplicate PMUs are being skipped avoid printing
> the suffix.
> 
> Before:
> ```
> $ perf list
> ...
>    uncore_imc_free_running_0/data_read/               [Kernel PMU event]
>    uncore_imc_free_running_0/data_total/              [Kernel PMU event]
>    uncore_imc_free_running_0/data_write/              [Kernel PMU event]
> ```
> 
> After:
> ```
> $ perf list
> ...
>    uncore_imc_free_running/data_read/                 [Kernel PMU event]
>    uncore_imc_free_running/data_total/                [Kernel PMU event]
>    uncore_imc_free_running/data_write/                [Kernel PMU event]
> ...
> $ perf list -v
>    uncore_imc_free_running_0/data_read/               [Kernel PMU event]
>    uncore_imc_free_running_0/data_total/              [Kernel PMU event]
>    uncore_imc_free_running_0/data_write/              [Kernel PMU event]
>    uncore_imc_free_running_1/data_read/               [Kernel PMU event]
>    uncore_imc_free_running_1/data_total/              [Kernel PMU event]
>    uncore_imc_free_running_1/data_write/              [Kernel PMU event]
> ...
> ```
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>   tools/perf/util/pmus.c | 20 ++++++++++++++------
>   1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> index 5073843aca19..b0ecb2e5bdcc 100644
> --- a/tools/perf/util/pmus.c
> +++ b/tools/perf/util/pmus.c
> @@ -440,10 +440,13 @@ static int sub_non_neg(int a, int b)
>   }
>   
>   static char *format_alias(char *buf, int len, const struct perf_pmu *pmu,
> -			  const struct perf_pmu_alias *alias)
> +			  const struct perf_pmu_alias *alias, bool skip_duplicate_pmus)
>   {
>   	struct parse_events_term *term;
> -	int used = snprintf(buf, len, "%s/%s", pmu->name, alias->name);
> +	int pmu_name_len = skip_duplicate_pmus
> +		? pmu_name_len_no_suffix(pmu->name, /*num=*/NULL)
> +		: (int)strlen(pmu->name);
> +	int used = snprintf(buf, len, "%.*s/%s", pmu_name_len, pmu->name, alias->name);
>   
>   	list_for_each_entry(term, &alias->terms, list) {
>   		if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR)
> @@ -473,9 +476,10 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
>   	int printed = 0;
>   	int len, j;
>   	struct sevent *aliases;
> +	bool skip_duplicate_pmus = print_cb->skip_duplicate_pmus(print_state);

nit: that code could have been in the previous patch

>   	struct perf_pmu *(*scan_fn)(struct perf_pmu *);
>   
> -	if (print_cb->skip_duplicate_pmus(print_state))
> +	if (skip_duplicate_pmus)
>   		scan_fn = perf_pmus__scan_skip_duplicates;
>   	else
>   		scan_fn = perf_pmus__scan;
> @@ -518,6 +522,7 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
>   			*desc = NULL, *long_desc = NULL,
>   			*encoding_desc = NULL, *topic = NULL,
>   			*pmu_name = NULL;
> +		int pmu_name_len;
>   		bool deprecated = false;
>   		size_t buf_used;
>   
> @@ -528,7 +533,8 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
>   		if (!aliases[j].event) {
>   			/* A selectable event. */
>   			pmu_name = aliases[j].pmu->name;
> -			buf_used = snprintf(buf, sizeof(buf), "%s//", pmu_name) + 1;
> +			pmu_name_len = pmu_name_len_no_suffix(pmu_name, /*num=*/NULL);
> +			buf_used = snprintf(buf, sizeof(buf), "%.*s//", pmu_name_len, pmu_name) + 1;
>   			name = buf;
>   		} else {
>   			if (aliases[j].event->desc) {
> @@ -536,7 +542,7 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
>   				buf_used = 0;
>   			} else {
>   				name = format_alias(buf, sizeof(buf), aliases[j].pmu,
> -						    aliases[j].event);
> +						    aliases[j].event, skip_duplicate_pmus);
>   				if (aliases[j].is_cpu) {
>   					alias = name;
>   					name = aliases[j].event->name;
> @@ -554,8 +560,10 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
>   			long_desc = aliases[j].event->long_desc;
>   			topic = aliases[j].event->topic;
>   			encoding_desc = buf + buf_used;
> +			pmu_name_len = pmu_name_len_no_suffix(pmu_name, /*num=*/NULL);
>   			buf_used += snprintf(buf + buf_used, sizeof(buf) - buf_used,
> -					"%s/%s/", pmu_name, aliases[j].event->str) + 1;
> +					"%.*s/%s/", pmu_name_len, pmu_name,
> +					aliases[j].event->str) + 1;
>   			deprecated = aliases[j].event->deprecated;
>   		}
>   		print_cb->print_event(print_state,

Re: [PATCH v2 2/3] perf pmus: Add scan that ignores duplicates, use for perf list

[Ian Rogers]

On Fri, Aug 11, 2023 at 8:51 AM John Garry <john.g.garry@oracle.com> wrote:
>
> On 10/08/2023 22:49, Ian Rogers wrote:
> > When there are multiple PMUs that differ only by suffix, by default
> > just list the first one and skip all others. As the PMUs are sorted,
> > the scan routine checks that the PMU names match and the numbers are
> > consecutive. If "-v" is passed to "perf list" then list all PMUs.
>
> I really think that this should be merged with the next change. I don't
> like the intermediate step of by default only printing the first PMU.

Ack. I'll leave it as 3 patches and then leave it to Arnaldo squash as
quite often he wants more patches.

> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >   tools/perf/builtin-list.c      |  8 +++++
> >   tools/perf/util/pmus.c         | 54 ++++++++++++++++++++++++++++++++--
> >   tools/perf/util/print-events.h |  1 +
> >   3 files changed, 61 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
> > index 7fec2cca759f..8fe4ddf02c14 100644
> > --- a/tools/perf/builtin-list.c
> > +++ b/tools/perf/builtin-list.c
> > @@ -423,6 +423,13 @@ static void json_print_metric(void *ps __maybe_unused, const char *group,
> >       strbuf_release(&buf);
> >   }
> >
> > +static bool default_skip_duplicate_pmus(void *ps)
> > +{
> > +     struct print_state *print_state = ps;
> > +
> > +     return !print_state->long_desc;
> > +}
> > +
> >   int cmd_list(int argc, const char **argv)
> >   {
> >       int i, ret = 0;
> > @@ -434,6 +441,7 @@ int cmd_list(int argc, const char **argv)
> >               .print_end = default_print_end,
> >               .print_event = default_print_event,
> >               .print_metric = default_print_metric,
> > +             .skip_duplicate_pmus = default_skip_duplicate_pmus,
> >       };
> >       const char *cputype = NULL;
> >       const char *unit_name = NULL;
> > diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> > index 3581710667b0..5073843aca19 100644
> > --- a/tools/perf/util/pmus.c
> > +++ b/tools/perf/util/pmus.c
> > @@ -275,6 +275,50 @@ struct perf_pmu *perf_pmus__scan_core(struct perf_pmu *pmu)
> >       return NULL;
> >   }
> >
> > +static struct perf_pmu *perf_pmus__scan_skip_duplicates(struct perf_pmu *pmu)
> > +{
> > +     bool use_core_pmus = !pmu || pmu->is_core;
> > +     int last_pmu_name_len = 0;
> > +     unsigned long last_pmu_num = 0;
> > +     const char *last_pmu_name = (pmu && pmu->name) ? pmu->name : "";
> > +
> > +     if (!pmu) {
> > +             pmu_read_sysfs(/*core_only=*/false);
> > +             pmu = list_prepare_entry(pmu, &core_pmus, list);
> > +     } else
> > +             last_pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &last_pmu_num);
> > +
> > +     if (use_core_pmus) {
> > +             list_for_each_entry_continue(pmu, &core_pmus, list) {
> > +                     unsigned long pmu_num = 0;
> > +                     int pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &pmu_num);
> > +
> > +                     if (last_pmu_name_len == pmu_name_len &&
> > +                         (last_pmu_num + 1 == pmu_num) &&
> > +                         !strncmp(last_pmu_name, pmu->name ?: "", pmu_name_len)) {
> > +                             last_pmu_num++;
> > +                             continue;
> > +                     }
> > +                     return pmu;
> > +             }
> > +             pmu = NULL;
>
> you assign pmu NULL
>
> > +             pmu = list_prepare_entry(pmu, &other_pmus, list);
>
> and then re-assign it. If list_prepare_entry() needs first arg = NULL,
> then can just use NULL explicitly?

Done.

> > +     }
> > +     list_for_each_entry_continue(pmu, &other_pmus, list) {
> > +             unsigned long pmu_num = 0;
> > +             int pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &pmu_num);
> > +
> > +             if (last_pmu_name_len == pmu_name_len &&
> > +                 (last_pmu_num + 1 == pmu_num) &&
> > +                 !strncmp(last_pmu_name, pmu->name ?: "", pmu_name_len)) {
> > +                     last_pmu_num++;
> > +                     continue;
>
> Can some of this code be factored out from the previous patch? It's
> doing something similar, right?

The previous patch implemented list sorting and a list comparator
whilst this patch is skipping PMUs if they follow the pattern:
uncore_xyz_0
uncore_xyz_1 <- skip
uncore_xyz_2 <- skip
The pmu_name_len_no_suffix is factored out and shared between both
routines. The comparator doesn't maintain state whilst this code does.
So I don't see a way to refactor things further.

Thanks,
Ian

> > +             }
> > +             return pmu;
> > +     }
> > +     return NULL;
> > +}
> > +
> >   const struct perf_pmu *perf_pmus__pmu_for_pmu_filter(const char *str)
> >   {
> >       struct perf_pmu *pmu = NULL;
> > @@ -429,10 +473,16 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
> >       int printed = 0;
> >       int len, j;
> >       struct sevent *aliases;
> > +     struct perf_pmu *(*scan_fn)(struct perf_pmu *);
> > +
> > +     if (print_cb->skip_duplicate_pmus(print_state))
> > +             scan_fn = perf_pmus__scan_skip_duplicates;
> > +     else
> > +             scan_fn = perf_pmus__scan;
> >
> >       pmu = NULL;
> >       len = 0;
> > -     while ((pmu = perf_pmus__scan(pmu)) != NULL) {
> > +     while ((pmu = scan_fn(pmu)) != NULL) {
> >               list_for_each_entry(event, &pmu->aliases, list)
> >                       len++;
> >               if (pmu->selectable)
> > @@ -445,7 +495,7 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
> >       }
> >       pmu = NULL;
> >       j = 0;
> > -     while ((pmu = perf_pmus__scan(pmu)) != NULL) {
> > +     while ((pmu = scan_fn(pmu)) != NULL) {
> >               bool is_cpu = pmu->is_core;
> >
> >               list_for_each_entry(event, &pmu->aliases, list) {
> > diff --git a/tools/perf/util/print-events.h b/tools/perf/util/print-events.h
> > index d7fab411e75c..bf4290bef0cd 100644
> > --- a/tools/perf/util/print-events.h
> > +++ b/tools/perf/util/print-events.h
> > @@ -26,6 +26,7 @@ struct print_callbacks {
> >                       const char *expr,
> >                       const char *threshold,
> >                       const char *unit);
> > +     bool (*skip_duplicate_pmus)(void *print_state);
> >   };
> >
> >   /** Print all events, the default when no options are specified. */
>

Re: [PATCH v2 2/3] perf pmus: Add scan that ignores duplicates, use for perf list

[Ian Rogers]

On Mon, Aug 14, 2023 at 8:57 AM Ian Rogers <irogers@google.com> wrote:
>
> On Fri, Aug 11, 2023 at 8:51 AM John Garry <john.g.garry@oracle.com> wrote:
> >
> > On 10/08/2023 22:49, Ian Rogers wrote:
> > > When there are multiple PMUs that differ only by suffix, by default
> > > just list the first one and skip all others. As the PMUs are sorted,
> > > the scan routine checks that the PMU names match and the numbers are
> > > consecutive. If "-v" is passed to "perf list" then list all PMUs.
> >
> > I really think that this should be merged with the next change. I don't
> > like the intermediate step of by default only printing the first PMU.
>
> Ack. I'll leave it as 3 patches and then leave it to Arnaldo squash as
> quite often he wants more patches.
>
> > >
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > >   tools/perf/builtin-list.c      |  8 +++++
> > >   tools/perf/util/pmus.c         | 54 ++++++++++++++++++++++++++++++++--
> > >   tools/perf/util/print-events.h |  1 +
> > >   3 files changed, 61 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
> > > index 7fec2cca759f..8fe4ddf02c14 100644
> > > --- a/tools/perf/builtin-list.c
> > > +++ b/tools/perf/builtin-list.c
> > > @@ -423,6 +423,13 @@ static void json_print_metric(void *ps __maybe_unused, const char *group,
> > >       strbuf_release(&buf);
> > >   }
> > >
> > > +static bool default_skip_duplicate_pmus(void *ps)
> > > +{
> > > +     struct print_state *print_state = ps;
> > > +
> > > +     return !print_state->long_desc;
> > > +}
> > > +
> > >   int cmd_list(int argc, const char **argv)
> > >   {
> > >       int i, ret = 0;
> > > @@ -434,6 +441,7 @@ int cmd_list(int argc, const char **argv)
> > >               .print_end = default_print_end,
> > >               .print_event = default_print_event,
> > >               .print_metric = default_print_metric,
> > > +             .skip_duplicate_pmus = default_skip_duplicate_pmus,
> > >       };
> > >       const char *cputype = NULL;
> > >       const char *unit_name = NULL;
> > > diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> > > index 3581710667b0..5073843aca19 100644
> > > --- a/tools/perf/util/pmus.c
> > > +++ b/tools/perf/util/pmus.c
> > > @@ -275,6 +275,50 @@ struct perf_pmu *perf_pmus__scan_core(struct perf_pmu *pmu)
> > >       return NULL;
> > >   }
> > >
> > > +static struct perf_pmu *perf_pmus__scan_skip_duplicates(struct perf_pmu *pmu)
> > > +{
> > > +     bool use_core_pmus = !pmu || pmu->is_core;
> > > +     int last_pmu_name_len = 0;
> > > +     unsigned long last_pmu_num = 0;
> > > +     const char *last_pmu_name = (pmu && pmu->name) ? pmu->name : "";
> > > +
> > > +     if (!pmu) {
> > > +             pmu_read_sysfs(/*core_only=*/false);
> > > +             pmu = list_prepare_entry(pmu, &core_pmus, list);
> > > +     } else
> > > +             last_pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &last_pmu_num);
> > > +
> > > +     if (use_core_pmus) {
> > > +             list_for_each_entry_continue(pmu, &core_pmus, list) {
> > > +                     unsigned long pmu_num = 0;
> > > +                     int pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &pmu_num);
> > > +
> > > +                     if (last_pmu_name_len == pmu_name_len &&
> > > +                         (last_pmu_num + 1 == pmu_num) &&
> > > +                         !strncmp(last_pmu_name, pmu->name ?: "", pmu_name_len)) {
> > > +                             last_pmu_num++;
> > > +                             continue;
> > > +                     }
> > > +                     return pmu;
> > > +             }
> > > +             pmu = NULL;
> >
> > you assign pmu NULL
> >
> > > +             pmu = list_prepare_entry(pmu, &other_pmus, list);
> >
> > and then re-assign it. If list_prepare_entry() needs first arg = NULL,
> > then can just use NULL explicitly?
>
> Done.

So because of the macro magic in list_prepare_entry you can't
explicitly pass NULL here as doing so yields:
tools/include/linux/kernel.h:36:33: error: request for member ‘list’
in something not a structure or union
  36 |         const typeof(((type *)0)->member) * __mptr = (ptr);     \
      |                                 ^~

Thanks,
Ian

> > > +     }
> > > +     list_for_each_entry_continue(pmu, &other_pmus, list) {
> > > +             unsigned long pmu_num = 0;
> > > +             int pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &pmu_num);
> > > +
> > > +             if (last_pmu_name_len == pmu_name_len &&
> > > +                 (last_pmu_num + 1 == pmu_num) &&
> > > +                 !strncmp(last_pmu_name, pmu->name ?: "", pmu_name_len)) {
> > > +                     last_pmu_num++;
> > > +                     continue;
> >
> > Can some of this code be factored out from the previous patch? It's
> > doing something similar, right?
>
> The previous patch implemented list sorting and a list comparator
> whilst this patch is skipping PMUs if they follow the pattern:
> uncore_xyz_0
> uncore_xyz_1 <- skip
> uncore_xyz_2 <- skip
> The pmu_name_len_no_suffix is factored out and shared between both
> routines. The comparator doesn't maintain state whilst this code does.
> So I don't see a way to refactor things further.
>
> Thanks,
> Ian
>
> > > +             }
> > > +             return pmu;
> > > +     }
> > > +     return NULL;
> > > +}
> > > +
> > >   const struct perf_pmu *perf_pmus__pmu_for_pmu_filter(const char *str)
> > >   {
> > >       struct perf_pmu *pmu = NULL;
> > > @@ -429,10 +473,16 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
> > >       int printed = 0;
> > >       int len, j;
> > >       struct sevent *aliases;
> > > +     struct perf_pmu *(*scan_fn)(struct perf_pmu *);
> > > +
> > > +     if (print_cb->skip_duplicate_pmus(print_state))
> > > +             scan_fn = perf_pmus__scan_skip_duplicates;
> > > +     else
> > > +             scan_fn = perf_pmus__scan;
> > >
> > >       pmu = NULL;
> > >       len = 0;
> > > -     while ((pmu = perf_pmus__scan(pmu)) != NULL) {
> > > +     while ((pmu = scan_fn(pmu)) != NULL) {
> > >               list_for_each_entry(event, &pmu->aliases, list)
> > >                       len++;
> > >               if (pmu->selectable)
> > > @@ -445,7 +495,7 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
> > >       }
> > >       pmu = NULL;
> > >       j = 0;
> > > -     while ((pmu = perf_pmus__scan(pmu)) != NULL) {
> > > +     while ((pmu = scan_fn(pmu)) != NULL) {
> > >               bool is_cpu = pmu->is_core;
> > >
> > >               list_for_each_entry(event, &pmu->aliases, list) {
> > > diff --git a/tools/perf/util/print-events.h b/tools/perf/util/print-events.h
> > > index d7fab411e75c..bf4290bef0cd 100644
> > > --- a/tools/perf/util/print-events.h
> > > +++ b/tools/perf/util/print-events.h
> > > @@ -26,6 +26,7 @@ struct print_callbacks {
> > >                       const char *expr,
> > >                       const char *threshold,
> > >                       const char *unit);
> > > +     bool (*skip_duplicate_pmus)(void *print_state);
> > >   };
> > >
> > >   /** Print all events, the default when no options are specified. */
> >

Re: [PATCH v2 2/3] perf pmus: Add scan that ignores duplicates, use for perf list

[John Garry]

On 14/08/2023 17:09, Ian Rogers wrote:
> On Mon, Aug 14, 2023 at 8:57 AM Ian Rogers<irogers@google.com>  wrote:
>> On Fri, Aug 11, 2023 at 8:51 AM John Garry<john.g.garry@oracle.com>  wrote:
>>> On 10/08/2023 22:49, Ian Rogers wrote:
>>>> When there are multiple PMUs that differ only by suffix, by default
>>>> just list the first one and skip all others. As the PMUs are sorted,
>>>> the scan routine checks that the PMU names match and the numbers are
>>>> consecutive. If "-v" is passed to "perf list" then list all PMUs.
>>> I really think that this should be merged with the next change. I don't
>>> like the intermediate step of by default only printing the first PMU.
>> Ack. I'll leave it as 3 patches and then leave it to Arnaldo squash as
>> quite often he wants more patches.

Why are more patches desirable? I do like the approach of taking a bite 
at a time, but we should also maintain ability to easily bisect and keep 
logical steps as one.

>>
>>>> Signed-off-by: Ian Rogers<irogers@google.com>
>>>> ---
>>>>    tools/perf/builtin-list.c      |  8 +++++
>>>>    tools/perf/util/pmus.c         | 54 ++++++++++++++++++++++++++++++++--
>>>>    tools/perf/util/print-events.h |  1 +
>>>>    3 files changed, 61 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
>>>> index 7fec2cca759f..8fe4ddf02c14 100644
>>>> --- a/tools/perf/builtin-list.c
>>>> +++ b/tools/perf/builtin-list.c
>>>> @@ -423,6 +423,13 @@ static void json_print_metric(void *ps __maybe_unused, const char *group,
>>>>        strbuf_release(&buf);
>>>>    }
>>>>
>>>> +static bool default_skip_duplicate_pmus(void *ps)
>>>> +{
>>>> +     struct print_state *print_state = ps;
>>>> +
>>>> +     return !print_state->long_desc;
>>>> +}
>>>> +
>>>>    int cmd_list(int argc, const char **argv)
>>>>    {
>>>>        int i, ret = 0;
>>>> @@ -434,6 +441,7 @@ int cmd_list(int argc, const char **argv)
>>>>                .print_end = default_print_end,
>>>>                .print_event = default_print_event,
>>>>                .print_metric = default_print_metric,
>>>> +             .skip_duplicate_pmus = default_skip_duplicate_pmus,
>>>>        };
>>>>        const char *cputype = NULL;
>>>>        const char *unit_name = NULL;
>>>> diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
>>>> index 3581710667b0..5073843aca19 100644
>>>> --- a/tools/perf/util/pmus.c
>>>> +++ b/tools/perf/util/pmus.c
>>>> @@ -275,6 +275,50 @@ struct perf_pmu *perf_pmus__scan_core(struct perf_pmu *pmu)
>>>>        return NULL;
>>>>    }
>>>>
>>>> +static struct perf_pmu *perf_pmus__scan_skip_duplicates(struct perf_pmu *pmu)
>>>> +{
>>>> +     bool use_core_pmus = !pmu || pmu->is_core;
>>>> +     int last_pmu_name_len = 0;
>>>> +     unsigned long last_pmu_num = 0;
>>>> +     const char *last_pmu_name = (pmu && pmu->name) ? pmu->name : "";
>>>> +
>>>> +     if (!pmu) {
>>>> +             pmu_read_sysfs(/*core_only=*/false);
>>>> +             pmu = list_prepare_entry(pmu, &core_pmus, list);
>>>> +     } else
>>>> +             last_pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &last_pmu_num);
>>>> +
>>>> +     if (use_core_pmus) {
>>>> +             list_for_each_entry_continue(pmu, &core_pmus, list) {
>>>> +                     unsigned long pmu_num = 0;
>>>> +                     int pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", &pmu_num);
>>>> +
>>>> +                     if (last_pmu_name_len == pmu_name_len &&
>>>> +                         (last_pmu_num + 1 == pmu_num) &&
>>>> +                         !strncmp(last_pmu_name, pmu->name ?: "", pmu_name_len)) {
>>>> +                             last_pmu_num++;
>>>> +                             continue;
>>>> +                     }
>>>> +                     return pmu;
>>>> +             }
>>>> +             pmu = NULL;
>>> you assign pmu NULL
>>>
>>>> +             pmu = list_prepare_entry(pmu, &other_pmus, list);
>>> and then re-assign it. If list_prepare_entry() needs first arg = NULL,
>>> then can just use NULL explicitly?
>> Done.
> So because of the macro magic in list_prepare_entry you can't
> explicitly pass NULL here as doing so yields:
> tools/include/linux/kernel.h:36:33: error: request for member ‘list’
> in something not a structure or union
>    36 |         const typeof(((type *)0)->member) * __mptr = (ptr);     \
>        |                                 ^~

ok, fine, so maybe add a comment as the code looks odd..

Thanks,
John