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

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 ?: "";

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 ?: "";

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

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;
 
 	/*

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;
>
>         /*

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