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

[Arnaldo Carvalho de Melo]

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

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.

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