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

From: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
To: Konstantin Khlebnikov <koct9i@gmail.com>
Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>,
	Konstantin Khlebnikov <khlebnikov@yandex-team.ru>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>,
	Kan Liang <kan.liang@linux.intel.com>,
	Jiri Olsa <jolsa@kernel.org>, Andi Kleen <ak@linux.intel.com>,
	Dmitry Monakhov <dmtrmonakhov@yandex-team.ru>
Subject: Re: [PATCH v2 2/3] perf tool: fix detecting smt at machines with more than 32 cpus
Date: Thu, 30 Apr 2020 10:37:42 -0300	[thread overview]
Message-ID: <20200430133742.GK30487@kernel.org> (raw)
In-Reply-To: <CALYGNiPcicchqKr4+a8QT=mm20ReGMv5-V605RzCAHP8-2vN3g@mail.gmail.com>

Em Wed, Apr 29, 2020 at 09:38:52PM +0300, Konstantin Khlebnikov escreveu:
> On Wed, Apr 29, 2020 at 9:16 PM Arnaldo Carvalho de Melo
> <arnaldo.melo@gmail.com> 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@yandex-team.ru>
> > > 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.

Sorry, I saw it in a + prefixed line so from a quick look I thought you
were introducing it, my bad.

> >
> > 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.

Well, in this case on up-to-date machines that code is not even used,
its a fallback.

If you don't have the time I'll eventually adjust the patch to what I
have now in my perf/core branch, since I've reordered them,

Thanks,

- Arnaldo

> > 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

-- 

- Arnaldo