On Wed, Apr 29, 2020 at 9:16 PM Arnaldo Carvalho de Melo
<arnaldo.m...@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 <khlebni...@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.
>
> 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
