Re: [PATCH v2 2/3] perf tool: fix detecting smt at machines with more than 32 cpus

[Konstantin Khlebnikov]

On Wed, Apr 29, 2020 at 9:16 PM Arnaldo Carvalho de Melo
<arnaldo.melo@xxxxxxxxx> wrote:
>
> Em Wed, Apr 29, 2020 at 07:22:43PM +0300, Konstantin Khlebnikov escreveu:
> > Cpu bitmap is split into 32 bit words. For system with more than 32 cores
> > threads are always in different words thus first word never has two bits:
> > cpu0: "0000,00000100,00000001", cpu 79: "8000,00000080,00000000".
> >
> > Instead of parsing bitmap read "core_cpus_list" or "thread_siblings_list"
> > and simply check presence of ',' or '-' in it.
> >
> > Signed-off-by: Konstantin Khlebnikov <khlebnikov@xxxxxxxxxxxxxx>
> > Fixes: de5077c4e38f ("perf tools: Add utility function to detect SMT status")
> > ---
> >  tools/perf/util/smt.c |   37 +++++++++++++++++--------------------
> >  1 file changed, 17 insertions(+), 20 deletions(-)
> >
> > diff --git a/tools/perf/util/smt.c b/tools/perf/util/smt.c
> > index 8481842e9edb..dc37b5abd1c3 100644
> > --- a/tools/perf/util/smt.c
> > +++ b/tools/perf/util/smt.c
> > @@ -1,6 +1,7 @@
> >  #include <stdio.h>
> >  #include <stdlib.h>
> >  #include <unistd.h>
> > +#include <string.h>
> >  #include <linux/bitops.h>
> >  #include "api/fs/fs.h"
> >  #include "smt.h"
> > @@ -9,39 +10,35 @@ int smt_on(void)
> >  {
> >       static bool cached;
> >       static int cached_result;
> > +     int active;
> >       int cpu;
> >       int ncpu;
> > +     char *str = NULL;
> > +     size_t strlen;
>
> Try not to use as the name of a variable the well known name of a
> standard C library function, there are cases where some of those names
> are used as #define directives and then all hell break loose...

You mean "strlen"? Yeah, that's weird name for variable
but it was here before me thus I haven't noticed.

>
> Also doing first the change that makes the use of that new file would
> allow me to have processed that patch first, which is way simpler than
> this one, i.e. try to leave the more involved changes to the end of the
> patchkit, that helps cherry-picking the less complex/smaller parts of
> your patchkit.

Hmm. Common sense tells to put cleanups and bugfixes before new features.

>
> I've applied the first one, thanks!
>
> - Arnaldo
>
> >       if (cached)
> >               return cached_result;
> >
> >       ncpu = sysconf(_SC_NPROCESSORS_CONF);
> >       for (cpu = 0; cpu < ncpu; cpu++) {
> > -             unsigned long long siblings;
> > -             char *str;
> > -             size_t strlen;
> >               char fn[256];
> >
> > -             snprintf(fn, sizeof fn,
> > -                     "devices/system/cpu/cpu%d/topology/core_cpus", cpu);
> > -             if (sysfs__read_str(fn, &str, &strlen) < 0) {
> > -                     snprintf(fn, sizeof fn,
> > -                             "devices/system/cpu/cpu%d/topology/thread_siblings",
> > -                             cpu);
> > -                     if (sysfs__read_str(fn, &str, &strlen) < 0)
> > -                             continue;
> > -             }
> > -             /* Entry is hex, but does not have 0x, so need custom parser */
> > -             siblings = strtoull(str, NULL, 16);
> > -             free(str);
> > -             if (hweight64(siblings) > 1) {
> > -                     cached_result = 1;
> > -                     cached = true;
> > +             snprintf(fn, sizeof(fn), "devices/system/cpu/cpu%d/topology/%s",
> > +                      cpu, "core_cpus_list");
> > +             if (sysfs__read_str(fn, &str, &strlen) > 0)
> > +                     break;
> > +
> > +             snprintf(fn, sizeof(fn), "devices/system/cpu/cpu%d/topology/%s",
> > +                      cpu, "thread_siblings_list");
> > +             if (sysfs__read_str(fn, &str, &strlen) > 0)
> >                       break;
> > -             }
> >       }
> > +
> > +     active = str && (strchr(str, ',') != NULL || strchr(str, '-') != NULL);
> > +     free(str);
> > +
> >       if (!cached) {
> > -             cached_result = 0;
> > +             cached_result = active;
> >               cached = true;
> >       }
> >       return cached_result;
> >
>
> --
>
> - Arnaldo