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

Arnaldo Carvalho de Melo
2023-08-25T14:52:30Z

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;

/*