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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 25 Aug 2023 08:56:44 -0700
From:   Ian Rogers <irogers@...gle.com>
To:     Arnaldo Carvalho de Melo <acme@...nel.org>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Kan Liang <kan.liang@...ux.intel.com>,
        Ravi Bangoria <ravi.bangoria@....com>,
        Kajol Jain <kjain@...ux.ibm.com>,
        John Garry <john.g.garry@...cle.com>,
        linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 1/2] perf pmus: Sort pmus by name then suffix

On Fri, Aug 25, 2023 at 7:51 AM Arnaldo Carvalho de Melo
<acme@...nel.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@...gle.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;
>
>         /*