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