Re: [PATCH] perf tool: fix detection of active SMT

[Arnaldo Carvalho de Melo]

Em Wed, Apr 29, 2020 at 01:00:39PM +0300, Konstantin Khlebnikov escreveu:
> SMT now could be disabled via "/sys/devices/system/cpu/smt/control".
> Status shown in "/sys/devices/system/cpu/smt/active" simply as "0" / "1".
> 
> If this knob isn't here fallback to checking topology but fix couple bugs:

In these cases, please break the patch into two, the first looking at
this new /sys/devices/system/cpu/smt/active file, then you go and do the
fix, and if its more than one, please break those fixes also in separate
patches.

Also you forgot to add all the people that worked on the fixed csets in
the CC list, so that they could review your work and give their Acked-by
or Reviewed-by, I'm adding the missing one, Andi Kleen, to this
response,

Thanks,

- Arnaldo
 
> Check access("devices/system/cpu/cpu%d/topology/core_cpus", F_OK) fails,
> unless current directory is "/sys". Simply try read this file first.
> 
> 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")
> Fixes: 0ccdb8407a46 ("perf tools: Apply new CPU topology sysfs attributes")
> ---
>  tools/perf/util/smt.c |   41 +++++++++++++++++++++--------------------
>  1 file changed, 21 insertions(+), 20 deletions(-)
> 
> diff --git a/tools/perf/util/smt.c b/tools/perf/util/smt.c
> index 3b791ef2cd50..c398528d1006 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,39 @@ int smt_on(void)
>  {
>  	static bool cached;
>  	static int cached_result;
> +	int active;
>  	int cpu;
>  	int ncpu;
> +	char *str = NULL;
> +	size_t strlen;
>  
>  	if (cached)
>  		return cached_result;
>  
> +	if (sysfs__read_int("devices/system/cpu/smt/active", &active) > 0)
> +		goto done;
> +
>  	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 (access(fn, F_OK) == -1) {
> -			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);
> +
> +done:
>  	if (!cached) {
> -		cached_result = 0;
> +		cached_result = active;
>  		cached = true;
>  	}
>  	return cached_result;
> 

-- 

- Arnaldo