linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/3] perf tool: fix reading new topology attribute "core_cpus"
@ 2020-04-29 16:19 Konstantin Khlebnikov
  2020-04-29 16:22 ` [PATCH v2 2/3] perf tool: fix detecting smt at machines with more than 32 cpus Konstantin Khlebnikov
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Konstantin Khlebnikov @ 2020-04-29 16:19 UTC (permalink / raw)
  To: linux-kernel, Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar
  Cc: Kan Liang, Jiri Olsa, Andi Kleen, Dmitry Monakhov

Check access("devices/system/cpu/cpu%d/topology/core_cpus", F_OK) fails,
unless current directory is "/sys". Simply try read this file first.

Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
Fixes: 0ccdb8407a46 ("perf tools: Apply new CPU topology sysfs attributes")
---
 tools/perf/util/smt.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/smt.c b/tools/perf/util/smt.c
index 3b791ef2cd50..8481842e9edb 100644
--- a/tools/perf/util/smt.c
+++ b/tools/perf/util/smt.c
@@ -24,13 +24,13 @@ int smt_on(void)
 
 		snprintf(fn, sizeof fn,
 			"devices/system/cpu/cpu%d/topology/core_cpus", cpu);
-		if (access(fn, F_OK) == -1) {
+		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;
 		}
-		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);


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 2/3] perf tool: fix detecting smt at machines with more than 32 cpus
  2020-04-29 16:19 [PATCH v2 1/3] perf tool: fix reading new topology attribute "core_cpus" Konstantin Khlebnikov
@ 2020-04-29 16:22 ` Konstantin Khlebnikov
  2020-04-29 18:13   ` Arnaldo Carvalho de Melo
  2020-04-29 16:23 ` [PATCH v2 3/3] perf tool: simplify checking active smt Konstantin Khlebnikov
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Konstantin Khlebnikov @ 2020-04-29 16:22 UTC (permalink / raw)
  To: linux-kernel, Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar
  Cc: Kan Liang, Jiri Olsa, Andi Kleen, Dmitry Monakhov

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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 3/3] perf tool: simplify checking active smt
  2020-04-29 16:19 [PATCH v2 1/3] perf tool: fix reading new topology attribute "core_cpus" Konstantin Khlebnikov
  2020-04-29 16:22 ` [PATCH v2 2/3] perf tool: fix detecting smt at machines with more than 32 cpus Konstantin Khlebnikov
@ 2020-04-29 16:23 ` Konstantin Khlebnikov
  2020-04-29 18:16   ` Arnaldo Carvalho de Melo
  2020-05-08 13:05   ` [tip: perf/core] perf tools: Simplify checking if SMT is active tip-bot2 for Konstantin Khlebnikov
  2020-04-29 18:11 ` [PATCH v2 1/3] perf tool: fix reading new topology attribute "core_cpus" Arnaldo Carvalho de Melo
  2020-05-08 13:05 ` [tip: perf/core] perf tools: Fix " tip-bot2 for Konstantin Khlebnikov
  3 siblings, 2 replies; 10+ messages in thread
From: Konstantin Khlebnikov @ 2020-04-29 16:23 UTC (permalink / raw)
  To: linux-kernel, Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar
  Cc: Kan Liang, Jiri Olsa, Andi Kleen, Dmitry Monakhov

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 then fallback to checking topology as before.

Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
---
 tools/perf/util/smt.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/perf/util/smt.c b/tools/perf/util/smt.c
index dc37b5abd1c3..c398528d1006 100644
--- a/tools/perf/util/smt.c
+++ b/tools/perf/util/smt.c
@@ -19,6 +19,9 @@ int smt_on(void)
 	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++) {
 		char fn[256];
@@ -37,6 +40,7 @@ int smt_on(void)
 	active = str && (strchr(str, ',') != NULL || strchr(str, '-') != NULL);
 	free(str);
 
+done:
 	if (!cached) {
 		cached_result = active;
 		cached = true;


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/3] perf tool: fix reading new topology attribute "core_cpus"
  2020-04-29 16:19 [PATCH v2 1/3] perf tool: fix reading new topology attribute "core_cpus" Konstantin Khlebnikov
  2020-04-29 16:22 ` [PATCH v2 2/3] perf tool: fix detecting smt at machines with more than 32 cpus Konstantin Khlebnikov
  2020-04-29 16:23 ` [PATCH v2 3/3] perf tool: simplify checking active smt Konstantin Khlebnikov
@ 2020-04-29 18:11 ` Arnaldo Carvalho de Melo
  2020-05-08 13:05 ` [tip: perf/core] perf tools: Fix " tip-bot2 for Konstantin Khlebnikov
  3 siblings, 0 replies; 10+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-04-29 18:11 UTC (permalink / raw)
  To: Konstantin Khlebnikov
  Cc: linux-kernel, Peter Zijlstra, Ingo Molnar, Kan Liang, Jiri Olsa,
	Andi Kleen, Dmitry Monakhov

Em Wed, Apr 29, 2020 at 07:19:47PM +0300, Konstantin Khlebnikov escreveu:
> Check access("devices/system/cpu/cpu%d/topology/core_cpus", F_OK) fails,
> unless current directory is "/sys". Simply try read this file first.
> 
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
> Fixes: 0ccdb8407a46 ("perf tools: Apply new CPU topology sysfs attributes")
> ---
>  tools/perf/util/smt.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Thanks, applied,

- Arnaldo
 
> diff --git a/tools/perf/util/smt.c b/tools/perf/util/smt.c
> index 3b791ef2cd50..8481842e9edb 100644
> --- a/tools/perf/util/smt.c
> +++ b/tools/perf/util/smt.c
> @@ -24,13 +24,13 @@ int smt_on(void)
>  
>  		snprintf(fn, sizeof fn,
>  			"devices/system/cpu/cpu%d/topology/core_cpus", cpu);
> -		if (access(fn, F_OK) == -1) {
> +		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;
>  		}
> -		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);
> 

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/3] perf tool: fix detecting smt at machines with more than 32 cpus
  2020-04-29 16:22 ` [PATCH v2 2/3] perf tool: fix detecting smt at machines with more than 32 cpus Konstantin Khlebnikov
@ 2020-04-29 18:13   ` Arnaldo Carvalho de Melo
  2020-04-29 18:38     ` Konstantin Khlebnikov
  0 siblings, 1 reply; 10+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-04-29 18:13 UTC (permalink / raw)
  To: Konstantin Khlebnikov
  Cc: linux-kernel, Peter Zijlstra, Ingo Molnar, Kan Liang, Jiri Olsa,
	Andi Kleen, Dmitry Monakhov

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

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 3/3] perf tool: simplify checking active smt
  2020-04-29 16:23 ` [PATCH v2 3/3] perf tool: simplify checking active smt Konstantin Khlebnikov
@ 2020-04-29 18:16   ` Arnaldo Carvalho de Melo
  2020-05-08 13:05   ` [tip: perf/core] perf tools: Simplify checking if SMT is active tip-bot2 for Konstantin Khlebnikov
  1 sibling, 0 replies; 10+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-04-29 18:16 UTC (permalink / raw)
  To: Konstantin Khlebnikov
  Cc: linux-kernel, Peter Zijlstra, Ingo Molnar, Kan Liang, Jiri Olsa,
	Andi Kleen, Dmitry Monakhov

Em Wed, Apr 29, 2020 at 07:23:41PM +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 then fallback to checking topology as before.

I've manually applied this one, thanks, please check my perf/core branch
later before resending 2/3, thanks.

- Arnaldo
 
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
> ---
>  tools/perf/util/smt.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/tools/perf/util/smt.c b/tools/perf/util/smt.c
> index dc37b5abd1c3..c398528d1006 100644
> --- a/tools/perf/util/smt.c
> +++ b/tools/perf/util/smt.c
> @@ -19,6 +19,9 @@ int smt_on(void)
>  	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++) {
>  		char fn[256];
> @@ -37,6 +40,7 @@ int smt_on(void)
>  	active = str && (strchr(str, ',') != NULL || strchr(str, '-') != NULL);
>  	free(str);
>  
> +done:
>  	if (!cached) {
>  		cached_result = active;
>  		cached = true;
> 

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/3] perf tool: fix detecting smt at machines with more than 32 cpus
  2020-04-29 18:13   ` Arnaldo Carvalho de Melo
@ 2020-04-29 18:38     ` Konstantin Khlebnikov
  2020-04-30 13:37       ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 10+ messages in thread
From: Konstantin Khlebnikov @ 2020-04-29 18:38 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Konstantin Khlebnikov, Linux Kernel Mailing List, Peter Zijlstra,
	Ingo Molnar, Kan Liang, Jiri Olsa, Andi Kleen, Dmitry Monakhov

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.

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/3] perf tool: fix detecting smt at machines with more than 32 cpus
  2020-04-29 18:38     ` Konstantin Khlebnikov
@ 2020-04-30 13:37       ` Arnaldo Carvalho de Melo
  0 siblings, 0 replies; 10+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-04-30 13:37 UTC (permalink / raw)
  To: Konstantin Khlebnikov
  Cc: Arnaldo Carvalho de Melo, Konstantin Khlebnikov,
	Linux Kernel Mailing List, Peter Zijlstra, Ingo Molnar,
	Kan Liang, Jiri Olsa, Andi Kleen, Dmitry Monakhov

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [tip: perf/core] perf tools: Fix reading new topology attribute "core_cpus"
  2020-04-29 16:19 [PATCH v2 1/3] perf tool: fix reading new topology attribute "core_cpus" Konstantin Khlebnikov
                   ` (2 preceding siblings ...)
  2020-04-29 18:11 ` [PATCH v2 1/3] perf tool: fix reading new topology attribute "core_cpus" Arnaldo Carvalho de Melo
@ 2020-05-08 13:05 ` tip-bot2 for Konstantin Khlebnikov
  3 siblings, 0 replies; 10+ messages in thread
From: tip-bot2 for Konstantin Khlebnikov @ 2020-05-08 13:05 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Konstantin Khlebnikov, Andi Kleen, Dmitry Monakhov, Jiri Olsa,
	Kan Liang, Peter Zijlstra, Arnaldo Carvalho de Melo, x86, LKML

The following commit has been merged into the perf/core branch of tip:

Commit-ID:     846de4371fdfddfa49481e3d04884539870dc127
Gitweb:        https://git.kernel.org/tip/846de4371fdfddfa49481e3d04884539870dc127
Author:        Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
AuthorDate:    Wed, 29 Apr 2020 19:19:47 +03:00
Committer:     Arnaldo Carvalho de Melo <acme@redhat.com>
CommitterDate: Tue, 05 May 2020 16:35:29 -03:00

perf tools: Fix reading new topology attribute "core_cpus"

Check if access("devices/system/cpu/cpu%d/topology/core_cpus", F_OK)
fails, which will happen unless the current directory is "/sys".

Simply try to read this file first.

Fixes: 0ccdb8407a46 ("perf tools: Apply new CPU topology sysfs attributes")
Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Dmitry Monakhov <dmtrmonakhov@yandex-team.ru>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lore.kernel.org/lkml/158817718710.747528.11009278875028211991.stgit@buzz
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/smt.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/smt.c b/tools/perf/util/smt.c
index 3b791ef..8481842 100644
--- a/tools/perf/util/smt.c
+++ b/tools/perf/util/smt.c
@@ -24,13 +24,13 @@ int smt_on(void)
 
 		snprintf(fn, sizeof fn,
 			"devices/system/cpu/cpu%d/topology/core_cpus", cpu);
-		if (access(fn, F_OK) == -1) {
+		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;
 		}
-		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);

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [tip: perf/core] perf tools: Simplify checking if SMT is active.
  2020-04-29 16:23 ` [PATCH v2 3/3] perf tool: simplify checking active smt Konstantin Khlebnikov
  2020-04-29 18:16   ` Arnaldo Carvalho de Melo
@ 2020-05-08 13:05   ` tip-bot2 for Konstantin Khlebnikov
  1 sibling, 0 replies; 10+ messages in thread
From: tip-bot2 for Konstantin Khlebnikov @ 2020-05-08 13:05 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Konstantin Khlebnikov, Andi Kleen, Dmitry Monakhov, Jiri Olsa,
	Kan Liang, Peter Zijlstra, Arnaldo Carvalho de Melo, x86, LKML

The following commit has been merged into the perf/core branch of tip:

Commit-ID:     bb629484d924118e3b1d8652177040115adcba01
Gitweb:        https://git.kernel.org/tip/bb629484d924118e3b1d8652177040115adcba01
Author:        Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
AuthorDate:    Wed, 29 Apr 2020 19:23:41 +03:00
Committer:     Arnaldo Carvalho de Melo <acme@redhat.com>
CommitterDate: Tue, 05 May 2020 16:35:29 -03:00

perf tools: Simplify checking if SMT is active.

SMT now could be disabled via "/sys/devices/system/cpu/smt/control".

Status is shown in "/sys/devices/system/cpu/smt/active" simply as "0" / "1".

If this knob isn't here then fallback to checking topology as before.

Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Dmitry Monakhov <dmtrmonakhov@yandex-team.ru>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lore.kernel.org/lkml/158817741394.748034.9273604089138009552.stgit@buzz
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/smt.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/perf/util/smt.c b/tools/perf/util/smt.c
index 8481842..20bacd5 100644
--- a/tools/perf/util/smt.c
+++ b/tools/perf/util/smt.c
@@ -15,6 +15,9 @@ int smt_on(void)
 	if (cached)
 		return cached_result;
 
+	if (sysfs__read_int("devices/system/cpu/smt/active", &cached_result) > 0)
+		goto done;
+
 	ncpu = sysconf(_SC_NPROCESSORS_CONF);
 	for (cpu = 0; cpu < ncpu; cpu++) {
 		unsigned long long siblings;
@@ -42,6 +45,7 @@ int smt_on(void)
 	}
 	if (!cached) {
 		cached_result = 0;
+done:
 		cached = true;
 	}
 	return cached_result;

^ permalink raw reply related	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-05-08 13:09 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-29 16:19 [PATCH v2 1/3] perf tool: fix reading new topology attribute "core_cpus" Konstantin Khlebnikov
2020-04-29 16:22 ` [PATCH v2 2/3] perf tool: fix detecting smt at machines with more than 32 cpus Konstantin Khlebnikov
2020-04-29 18:13   ` Arnaldo Carvalho de Melo
2020-04-29 18:38     ` Konstantin Khlebnikov
2020-04-30 13:37       ` Arnaldo Carvalho de Melo
2020-04-29 16:23 ` [PATCH v2 3/3] perf tool: simplify checking active smt Konstantin Khlebnikov
2020-04-29 18:16   ` Arnaldo Carvalho de Melo
2020-05-08 13:05   ` [tip: perf/core] perf tools: Simplify checking if SMT is active tip-bot2 for Konstantin Khlebnikov
2020-04-29 18:11 ` [PATCH v2 1/3] perf tool: fix reading new topology attribute "core_cpus" Arnaldo Carvalho de Melo
2020-05-08 13:05 ` [tip: perf/core] perf tools: Fix " tip-bot2 for Konstantin Khlebnikov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).