[PATCH v5 0/2] perf list: Remove duplicate PMUs

[PATCH v5 0/2] 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.

v5: Improved the 2nd patch's commit message and removed an unused
    variable as suggested by Kan Liang <kan.liang@linux.intel.com>.
v4: Rebase on top of lazy PMU changes. Ignore numeric ordering due to
    gaps, suggested by Kan Liang <kan.liang@linux.intel.com>. Fold
    patches 2 & 3 as suggested by John Garry <john.g.garry@oracle.com>
    (done by accident as part of rebasing).
v3: Add detail to patch 1 sorting commit message about the suffix and
    why sorting is necessary.
v2: List all PMUs when "-v" is passed as suggested by John Garry
    <john.g.garry@oracle.com>.

Ian Rogers (2):
  perf pmus: Sort pmus by name then suffix
  perf pmus: Skip duplicate PMUs and don't print list suffix by default

 tools/perf/builtin-list.c         |  8 +++
 tools/perf/util/pmu.c             | 17 ++++--
 tools/perf/util/pmu.h             |  3 +-
 tools/perf/util/pmus.c            | 99 ++++++++++++++++++++++++++++++-
 tools/perf/util/pmus.h            |  2 +
 tools/perf/util/print-events.h    |  1 +
 tools/perf/util/s390-sample-raw.c |  3 +-
 7 files changed, 123 insertions(+), 10 deletions(-)

-- 
2.42.0.rc2.253.gd59a3bf2b4-goog

[PATCH v5 1/2] 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". Suffixes need to be treated specially as
otherwise they will be ordered like 0, 1, 10, 11, .., 2, 20, 21, ..,
etc. Only PMUs starting 'uncore_' are considered to have a potential
suffix.

Sorting of PMUs is done so that later patches can skip duplicate
uncore PMUs that differ only by there suffix.

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 4dd5912617ff..b1f6a64693fe 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.42.0.rc2.253.gd59a3bf2b4-goog

[PATCH v5 2/2] perf pmus: Skip duplicate PMUs and don't print list suffix by default

[Ian Rogers]

Add a PMUs scan that ignores duplicates. When there are multiple PMUs
that differ only by suffix, by default just list the first one and
skip all others. The scan routine checks that the PMU names match but
doesn't enforce that the numbers are consecutive as for some PMUs
there are gaps. If "-v" is passed to "perf list" then list all PMUs.

With the previous change duplicate PMUs are no longer printed 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]
  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]
...
```
Signed-off-by: Ian Rogers <irogers@google.com>

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/builtin-list.c         |  8 +++++
 tools/perf/util/pmu.c             | 17 +++++++---
 tools/perf/util/pmu.h             |  3 +-
 tools/perf/util/pmus.c            | 53 ++++++++++++++++++++++++++++---
 tools/perf/util/pmus.h            |  2 ++
 tools/perf/util/print-events.h    |  1 +
 tools/perf/util/s390-sample-raw.c |  3 +-
 7 files changed, 76 insertions(+), 11 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/pmu.c b/tools/perf/util/pmu.c
index bb2ca29cd7bd..b9ed829318c3 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -1576,7 +1576,9 @@ int perf_pmu__find_event(struct perf_pmu *pmu, const char *event, void *state, p
 		.cb = cb,
 	};
 
-	return perf_pmu__for_each_event(pmu, &args, find_event_callback);
+	/* Sub-optimal, but function is only used by tests. */
+	return perf_pmu__for_each_event(pmu, /*skip_duplicate_pmus=*/ false,
+					&args, find_event_callback);
 }
 
 static void perf_pmu__del_formats(struct list_head *formats)
@@ -1650,10 +1652,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)
@@ -1675,7 +1680,8 @@ static char *format_alias(char *buf, int len, const struct perf_pmu *pmu,
 	return buf;
 }
 
-int perf_pmu__for_each_event(struct perf_pmu *pmu, void *state, pmu_event_callback cb)
+int perf_pmu__for_each_event(struct perf_pmu *pmu, bool skip_duplicate_pmus,
+			     void *state, pmu_event_callback cb)
 {
 	char buf[1024];
 	struct perf_pmu_alias *event;
@@ -1694,7 +1700,8 @@ int perf_pmu__for_each_event(struct perf_pmu *pmu, void *state, pmu_event_callba
 			info.name = event->name;
 			buf_used = 0;
 		} else {
-			info.name = format_alias(buf, sizeof(buf), pmu, event);
+			info.name = format_alias(buf, sizeof(buf), pmu, event,
+						 skip_duplicate_pmus);
 			if (pmu->is_core) {
 				info.alias = info.name;
 				info.name = event->name;
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index bae0de3ed7a5..b5c506f35b42 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -212,7 +212,8 @@ bool perf_pmu__supports_legacy_cache(const struct perf_pmu *pmu);
 bool perf_pmu__auto_merge_stats(const struct perf_pmu *pmu);
 bool perf_pmu__have_event(struct perf_pmu *pmu, const char *name);
 size_t perf_pmu__num_events(struct perf_pmu *pmu);
-int perf_pmu__for_each_event(struct perf_pmu *pmu, void *state, pmu_event_callback cb);
+int perf_pmu__for_each_event(struct perf_pmu *pmu, bool skip_duplicate_pmus,
+			     void *state, pmu_event_callback cb);
 bool pmu__name_match(const struct perf_pmu *pmu, const char *pmu_name);
 
 /**
diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
index b1f6a64693fe..28909c9a32a1 100644
--- a/tools/perf/util/pmus.c
+++ b/tools/perf/util/pmus.c
@@ -35,7 +35,7 @@ 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 pmu_name_len_no_suffix(const char *str, unsigned long *num)
 {
 	int orig_len, len;
 
@@ -275,6 +275,43 @@ 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;
+	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 ?: "", NULL);
+
+	if (use_core_pmus) {
+		list_for_each_entry_continue(pmu, &core_pmus, list) {
+			int pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", /*num=*/NULL);
+
+			if (last_pmu_name_len == pmu_name_len &&
+			    !strncmp(last_pmu_name, pmu->name ?: "", pmu_name_len))
+				continue;
+
+			return pmu;
+		}
+		pmu = NULL;
+		pmu = list_prepare_entry(pmu, &other_pmus, list);
+	}
+	list_for_each_entry_continue(pmu, &other_pmus, list) {
+		int pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "", /*num=*/NULL);
+
+		if (last_pmu_name_len == pmu_name_len &&
+		    !strncmp(last_pmu_name, pmu->name ?: "", pmu_name_len))
+			continue;
+
+		return pmu;
+	}
+	return NULL;
+}
+
 const struct perf_pmu *perf_pmus__pmu_for_pmu_filter(const char *str)
 {
 	struct perf_pmu *pmu = NULL;
@@ -400,10 +437,17 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
 	int len;
 	struct sevent *aliases;
 	struct events_callback_state state;
+	bool skip_duplicate_pmus = print_cb->skip_duplicate_pmus(print_state);
+	struct perf_pmu *(*scan_fn)(struct perf_pmu *);
+
+	if (skip_duplicate_pmus)
+		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)
 		len += perf_pmu__num_events(pmu);
 
 	aliases = zalloc(sizeof(struct sevent) * len);
@@ -417,8 +461,9 @@ void perf_pmus__print_pmu_events(const struct print_callbacks *print_cb, void *p
 		.aliases_len = len,
 		.index = 0,
 	};
-	while ((pmu = perf_pmus__scan(pmu)) != NULL) {
-		perf_pmu__for_each_event(pmu, &state, perf_pmus__print_pmu_events__callback);
+	while ((pmu = scan_fn(pmu)) != NULL) {
+		perf_pmu__for_each_event(pmu, skip_duplicate_pmus, &state,
+					 perf_pmus__print_pmu_events__callback);
 	}
 	qsort(aliases, len, sizeof(struct sevent), cmp_sevent);
 	for (int j = 0; j < len; j++) {
diff --git a/tools/perf/util/pmus.h b/tools/perf/util/pmus.h
index a21464432d0f..4c67153ac257 100644
--- a/tools/perf/util/pmus.h
+++ b/tools/perf/util/pmus.h
@@ -5,6 +5,8 @@
 struct perf_pmu;
 struct print_callbacks;
 
+int pmu_name_len_no_suffix(const char *str, unsigned long *num);
+
 void perf_pmus__destroy(void);
 
 struct perf_pmu *perf_pmus__find(const char *name);
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. */
diff --git a/tools/perf/util/s390-sample-raw.c b/tools/perf/util/s390-sample-raw.c
index dc1ed3e95d4d..115b16edb451 100644
--- a/tools/perf/util/s390-sample-raw.c
+++ b/tools/perf/util/s390-sample-raw.c
@@ -171,7 +171,8 @@ static char *get_counter_name(int set, int nr, struct perf_pmu *pmu)
 	if (!pmu)
 		return NULL;
 
-	perf_pmu__for_each_event(pmu, &data, get_counter_name_callback);
+	perf_pmu__for_each_event(pmu, /*skip_duplicate_pmus=*/ true,
+				 &data, get_counter_name_callback);
 	return data.result;
 }
 
-- 
2.42.0.rc2.253.gd59a3bf2b4-goog

Re: [PATCH v5 0/2] perf list: Remove duplicate PMUs

[Liang, Kan]

On 2023-08-25 9:52 a.m., Ian Rogers wrote:
> 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.
> 
> v5: Improved the 2nd patch's commit message and removed an unused
>     variable as suggested by Kan Liang <kan.liang@linux.intel.com>.
> v4: Rebase on top of lazy PMU changes. Ignore numeric ordering due to
>     gaps, suggested by Kan Liang <kan.liang@linux.intel.com>. Fold
>     patches 2 & 3 as suggested by John Garry <john.g.garry@oracle.com>
>     (done by accident as part of rebasing).
> v3: Add detail to patch 1 sorting commit message about the suffix and
>     why sorting is necessary.
> v2: List all PMUs when "-v" is passed as suggested by John Garry
>     <john.g.garry@oracle.com>.
> 
> Ian Rogers (2):
>   perf pmus: Sort pmus by name then suffix
>   perf pmus: Skip duplicate PMUs and don't print list suffix by default


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

Thanks,
Kan

> 
>  tools/perf/builtin-list.c         |  8 +++
>  tools/perf/util/pmu.c             | 17 ++++--
>  tools/perf/util/pmu.h             |  3 +-
>  tools/perf/util/pmus.c            | 99 ++++++++++++++++++++++++++++++-
>  tools/perf/util/pmus.h            |  2 +
>  tools/perf/util/print-events.h    |  1 +
>  tools/perf/util/s390-sample-raw.c |  3 +-
>  7 files changed, 123 insertions(+), 10 deletions(-)
> 

Re: [PATCH v5 0/2] perf list: Remove duplicate PMUs

[Arnaldo Carvalho de Melo]

Em Fri, Aug 25, 2023 at 10:05:46AM -0400, Liang, Kan escreveu:
> On 2023-08-25 9:52 a.m., Ian Rogers wrote:
> > 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.
> > 
> > v5: Improved the 2nd patch's commit message and removed an unused
> >     variable as suggested by Kan Liang <kan.liang@linux.intel.com>.
> > v4: Rebase on top of lazy PMU changes. Ignore numeric ordering due to
> >     gaps, suggested by Kan Liang <kan.liang@linux.intel.com>. Fold
> >     patches 2 & 3 as suggested by John Garry <john.g.garry@oracle.com>
> >     (done by accident as part of rebasing).
> > v3: Add detail to patch 1 sorting commit message about the suffix and
> >     why sorting is necessary.
> > v2: List all PMUs when "-v" is passed as suggested by John Garry
> >     <john.g.garry@oracle.com>.
> > 
> > Ian Rogers (2):
> >   perf pmus: Sort pmus by name then suffix
> >   perf pmus: Skip duplicate PMUs and don't print list suffix by default
> 
> 
> Reviewed-by: Kan Liang <kan.liang@linux.intel.com>

Thanks, applied.

- Arnaldo

Re: [PATCH v5 1/2] perf pmus: Sort pmus by name then suffix

[Arnaldo Carvalho de Melo]

Em Fri, Aug 25, 2023 at 06:52:36AM -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". Suffixes need to be treated specially as
> otherwise they will be ordered like 0, 1, 10, 11, .., 2, 20, 21, ..,
> etc. Only PMUs starting 'uncore_' are considered to have a potential
> suffix.
> 
> Sorting of PMUs is done so that later patches can skip duplicate
> uncore PMUs that differ only by there suffix.
> 
> 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 4dd5912617ff..b1f6a64693fe 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;

I applied the patch, but we have strstarts() for this case, to avoid
having to count the size of the prefix in tools/include/linux/string.h,
that we copied from the kernel sources:

/**
 * strstarts - does @str start with @prefix?
 * @str: string to examine
 * @prefix: prefix to look for.
 */
static inline bool strstarts(const char *str, const char *prefix)
{
        return strncmp(str, prefix, strlen(prefix)) == 0;
}

I'll change it, ok?

- Arnaldo



> +
> +	/*
> +	 * 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.42.0.rc2.253.gd59a3bf2b4-goog
> 

-- 

- Arnaldo

Re: [PATCH v5 1/2] perf pmus: Sort pmus by name then suffix

[Arnaldo Carvalho de Melo]

Em Fri, Aug 25, 2023 at 11:48:08AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Fri, Aug 25, 2023 at 06:52:36AM -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". Suffixes need to be treated specially as
> > otherwise they will be ordered like 0, 1, 10, 11, .., 2, 20, 21, ..,
> > etc. Only PMUs starting 'uncore_' are considered to have a potential
> > suffix.
> > 
> > Sorting of PMUs is done so that later patches can skip duplicate
> > uncore PMUs that differ only by there suffix.
> > 
> > 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 4dd5912617ff..b1f6a64693fe 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;
> 
> I applied the patch, but we have strstarts() for this case, to avoid
> having to count the size of the prefix in tools/include/linux/string.h,
> that we copied from the kernel sources:
> 
> /**
>  * strstarts - does @str start with @prefix?
>  * @str: string to examine
>  * @prefix: prefix to look for.
>  */
> static inline bool strstarts(const char *str, const char *prefix)
> {
>         return strncmp(str, prefix, strlen(prefix)) == 0;
> }
> 
> I'll change it, ok?

This:

diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
index b1f6a64693fe0d49..bbf84ccc3aba7d5c 100644
--- a/tools/perf/util/pmus.c
+++ b/tools/perf/util/pmus.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <linux/list.h>
 #include <linux/list_sort.h>
+#include <linux/string.h>
 #include <linux/zalloc.h>
 #include <subcmd/pager.h>
 #include <sys/types.h>
@@ -42,7 +43,7 @@ static int pmu_name_len_no_suffix(const char *str, unsigned long *num)
 	orig_len = len = strlen(str);
 
 	/* Non-uncore PMUs have their full length, for example, i915. */
-	if (strncmp(str, "uncore_", 7))
+	if (!strstarts(str, "uncore_"))
 		return len;
 
 	/*

Re: [PATCH v5 1/2] perf pmus: Sort pmus by name then suffix

[Ian Rogers]

On Fri, Aug 25, 2023 at 7:51 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Fri, Aug 25, 2023 at 11:48:08AM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Fri, Aug 25, 2023 at 06:52:36AM -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". Suffixes need to be treated specially as
> > > otherwise they will be ordered like 0, 1, 10, 11, .., 2, 20, 21, ..,
> > > etc. Only PMUs starting 'uncore_' are considered to have a potential
> > > suffix.
> > >
> > > Sorting of PMUs is done so that later patches can skip duplicate
> > > uncore PMUs that differ only by there suffix.
> > >
> > > 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 4dd5912617ff..b1f6a64693fe 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;
> >
> > I applied the patch, but we have strstarts() for this case, to avoid
> > having to count the size of the prefix in tools/include/linux/string.h,
> > that we copied from the kernel sources:
> >
> > /**
> >  * strstarts - does @str start with @prefix?
> >  * @str: string to examine
> >  * @prefix: prefix to look for.
> >  */
> > static inline bool strstarts(const char *str, const char *prefix)
> > {
> >         return strncmp(str, prefix, strlen(prefix)) == 0;
> > }
> >
> > I'll change it, ok?

Makes sense to me. We also do this same strncmp here:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/parse-events.y?h=perf-tools-next#n315
So perhaps do a follow up patch cleaning up all instances of the not
use of strstarts.

Thanks,
Ian

> This:
>
> diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> index b1f6a64693fe0d49..bbf84ccc3aba7d5c 100644
> --- a/tools/perf/util/pmus.c
> +++ b/tools/perf/util/pmus.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include <linux/list.h>
>  #include <linux/list_sort.h>
> +#include <linux/string.h>
>  #include <linux/zalloc.h>
>  #include <subcmd/pager.h>
>  #include <sys/types.h>
> @@ -42,7 +43,7 @@ static int pmu_name_len_no_suffix(const char *str, unsigned long *num)
>         orig_len = len = strlen(str);
>
>         /* Non-uncore PMUs have their full length, for example, i915. */
> -       if (strncmp(str, "uncore_", 7))
> +       if (!strstarts(str, "uncore_"))
>                 return len;
>
>         /*

Re: [PATCH v5 1/2] perf pmus: Sort pmus by name then suffix

[Arnaldo Carvalho de Melo]

Em Fri, Aug 25, 2023 at 08:56:44AM -0700, Ian Rogers escreveu:
> On Fri, Aug 25, 2023 at 7:51 AM Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> > Em Fri, Aug 25, 2023 at 11:48:08AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Fri, Aug 25, 2023 at 06:52:36AM -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". Suffixes need to be treated specially as
> > > > otherwise they will be ordered like 0, 1, 10, 11, .., 2, 20, 21, ..,
> > > > etc. Only PMUs starting 'uncore_' are considered to have a potential
> > > > suffix.
> > > >
> > > > Sorting of PMUs is done so that later patches can skip duplicate
> > > > uncore PMUs that differ only by there suffix.
> > > >
> > > > 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 4dd5912617ff..b1f6a64693fe 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;
> > >
> > > I applied the patch, but we have strstarts() for this case, to avoid
> > > having to count the size of the prefix in tools/include/linux/string.h,
> > > that we copied from the kernel sources:
> > >
> > > /**
> > >  * strstarts - does @str start with @prefix?
> > >  * @str: string to examine
> > >  * @prefix: prefix to look for.
> > >  */
> > > static inline bool strstarts(const char *str, const char *prefix)
> > > {
> > >         return strncmp(str, prefix, strlen(prefix)) == 0;
> > > }
> > >
> > > I'll change it, ok?
> 
> Makes sense to me. We also do this same strncmp here:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/parse-events.y?h=perf-tools-next#n315

I did it in this case, to keep 'git blame' info in place, will look for
the other cases and do it.

This is something that should be automated somehow, maybe here is a good
opportunity for checkpatch to do something useful :-)

- Arnaldo

> So perhaps do a follow up patch cleaning up all instances of the not
> use of strstarts.
> 
> Thanks,
> Ian
> 
> > This:
> >
> > diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> > index b1f6a64693fe0d49..bbf84ccc3aba7d5c 100644
> > --- a/tools/perf/util/pmus.c
> > +++ b/tools/perf/util/pmus.c
> > @@ -1,6 +1,7 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >  #include <linux/list.h>
> >  #include <linux/list_sort.h>
> > +#include <linux/string.h>
> >  #include <linux/zalloc.h>
> >  #include <subcmd/pager.h>
> >  #include <sys/types.h>
> > @@ -42,7 +43,7 @@ static int pmu_name_len_no_suffix(const char *str, unsigned long *num)
> >         orig_len = len = strlen(str);
> >
> >         /* Non-uncore PMUs have their full length, for example, i915. */
> > -       if (strncmp(str, "uncore_", 7))
> > +       if (!strstarts(str, "uncore_"))
> >                 return len;
> >
> >         /*

-- 

- Arnaldo

Re: [PATCH v5 1/2] perf pmus: Sort pmus by name then suffix

[Arnaldo Carvalho de Melo]

Em Fri, Aug 25, 2023 at 06:52:36AM -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". Suffixes need to be treated specially as
> otherwise they will be ordered like 0, 1, 10, 11, .., 2, 20, 21, ..,
> etc. Only PMUs starting 'uncore_' are considered to have a potential
> suffix.
> 
> Sorting of PMUs is done so that later patches can skip duplicate
> uncore PMUs that differ only by there suffix.
> 
> 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 4dd5912617ff..b1f6a64693fe 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);
> +}

Also, I got this in some distros/gcc versions:

  31    12.95 opensuse:15.5                 : FAIL gcc version 7.5.0 (SUSE Linux)
    util/pmus.c: In function 'pmus_cmp':
    util/pmus.c:169:57: error: 'lhs_num' may be used uninitialized in this function [-Werror=maybe-uninitialized]
      return lhs_num < rhs_num ? -1 : (lhs_num > rhs_num ? 1 : 0);
                                      ~~~~~~~~~~~~~~~~~~~~~~~^~~~
    cc1: all warnings being treated as errors
    make[3]: *** [/git/perf-6.5.0-rc5/tools/build/Makefile.build:150: util] Error 2

I applied this, please check.

diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
index bbf84ccc3aba7d5c..7316da1c0ddb8eaf 100644
--- a/tools/perf/util/pmus.c
+++ b/tools/perf/util/pmus.c
@@ -153,7 +153,7 @@ static struct perf_pmu *perf_pmu__find2(int dirfd, const char *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;
+	unsigned long lhs_num = 0, rhs_num = 0;
 	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 ?: "";

Re: [PATCH v5 1/2] perf pmus: Sort pmus by name then suffix

[Ian Rogers]

On Fri, Aug 25, 2023 at 1:23 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Fri, Aug 25, 2023 at 06:52:36AM -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". Suffixes need to be treated specially as
> > otherwise they will be ordered like 0, 1, 10, 11, .., 2, 20, 21, ..,
> > etc. Only PMUs starting 'uncore_' are considered to have a potential
> > suffix.
> >
> > Sorting of PMUs is done so that later patches can skip duplicate
> > uncore PMUs that differ only by there suffix.
> >
> > 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 4dd5912617ff..b1f6a64693fe 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);
> > +}
>
> Also, I got this in some distros/gcc versions:
>
>   31    12.95 opensuse:15.5                 : FAIL gcc version 7.5.0 (SUSE Linux)
>     util/pmus.c: In function 'pmus_cmp':
>     util/pmus.c:169:57: error: 'lhs_num' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>       return lhs_num < rhs_num ? -1 : (lhs_num > rhs_num ? 1 : 0);
>                                       ~~~~~~~~~~~~~~~~~~~~~~~^~~~
>     cc1: all warnings being treated as errors
>     make[3]: *** [/git/perf-6.5.0-rc5/tools/build/Makefile.build:150: util] Error 2
>
> I applied this, please check.

Makes sense, thanks!

Ian

> diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> index bbf84ccc3aba7d5c..7316da1c0ddb8eaf 100644
> --- a/tools/perf/util/pmus.c
> +++ b/tools/perf/util/pmus.c
> @@ -153,7 +153,7 @@ static struct perf_pmu *perf_pmu__find2(int dirfd, const char *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;
> +       unsigned long lhs_num = 0, rhs_num = 0;
>         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 ?: "";

Re: [PATCH v5 0/2] perf list: Remove duplicate PMUs

[kajoljain]

Hi,
 Patchset looks good to me.

Reviewed-by: Kajol Jain <kjain@linux.ibm.com>

Thanks,
Kajol Jain

On 8/25/23 19:22, Ian Rogers wrote:
> 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.
> 
> v5: Improved the 2nd patch's commit message and removed an unused
>     variable as suggested by Kan Liang <kan.liang@linux.intel.com>.
> v4: Rebase on top of lazy PMU changes. Ignore numeric ordering due to
>     gaps, suggested by Kan Liang <kan.liang@linux.intel.com>. Fold
>     patches 2 & 3 as suggested by John Garry <john.g.garry@oracle.com>
>     (done by accident as part of rebasing).
> v3: Add detail to patch 1 sorting commit message about the suffix and
>     why sorting is necessary.
> v2: List all PMUs when "-v" is passed as suggested by John Garry
>     <john.g.garry@oracle.com>.
> 
> Ian Rogers (2):
>   perf pmus: Sort pmus by name then suffix
>   perf pmus: Skip duplicate PMUs and don't print list suffix by default
> 
>  tools/perf/builtin-list.c         |  8 +++
>  tools/perf/util/pmu.c             | 17 ++++--
>  tools/perf/util/pmu.h             |  3 +-
>  tools/perf/util/pmus.c            | 99 ++++++++++++++++++++++++++++++-
>  tools/perf/util/pmus.h            |  2 +
>  tools/perf/util/print-events.h    |  1 +
>  tools/perf/util/s390-sample-raw.c |  3 +-
>  7 files changed, 123 insertions(+), 10 deletions(-)
>