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