* [PATCH 1/3] perf expr: Add debug logging for literals @ 2021-11-23 22:48 Ian Rogers 2021-11-23 22:48 ` [PATCH 2/3] perf tools: Fix SMT not detected with large core count Ian Rogers 2021-11-23 22:48 ` [PATCH 3/3] perf tools: Probe non-deprecated sysfs path 1st Ian Rogers 0 siblings, 2 replies; 5+ messages in thread From: Ian Rogers @ 2021-11-23 22:48 UTC (permalink / raw) To: Andi Kleen, Jiri Olsa, Namhyung Kim, John Garry, Kajol Jain, Paul A . Clarke, Arnaldo Carvalho de Melo, Kan Liang, Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin, Konstantin Khlebnikov, linux-perf-users, linux-kernel Cc: eranian, Ian Rogers Useful for diagnosing problems with metrics. Signed-off-by: Ian Rogers <irogers@google.com> --- tools/perf/util/expr.c | 35 ++++++++++++++++++++++++----------- 1 file changed, 24 insertions(+), 11 deletions(-) diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c index 1d532b9fed29..cdbab4f959fe 100644 --- a/tools/perf/util/expr.c +++ b/tools/perf/util/expr.c @@ -395,12 +395,17 @@ double expr_id_data__source_count(const struct expr_id_data *data) double expr__get_literal(const char *literal) { static struct cpu_topology *topology; + double result = NAN; - if (!strcmp("#smt_on", literal)) - return smt_on() > 0 ? 1.0 : 0.0; + if (!strcmp("#smt_on", literal)) { + result = smt_on() > 0 ? 1.0 : 0.0; + goto out; + } - if (!strcmp("#num_cpus", literal)) - return cpu__max_present_cpu(); + if (!strcmp("#num_cpus", literal)) { + result = cpu__max_present_cpu(); + goto out; + } /* * Assume that topology strings are consistent, such as CPUs "0-1" @@ -415,13 +420,21 @@ double expr__get_literal(const char *literal) return NAN; } } - if (!strcmp("#num_packages", literal)) - return topology->package_cpus_lists; - if (!strcmp("#num_dies", literal)) - return topology->die_cpus_lists; - if (!strcmp("#num_cores", literal)) - return topology->core_cpus_lists; + if (!strcmp("#num_packages", literal)) { + result = topology->package_cpus_lists; + goto out; + } + if (!strcmp("#num_dies", literal)) { + result = topology->die_cpus_lists; + goto out; + } + if (!strcmp("#num_cores", literal)) { + result = topology->core_cpus_lists; + goto out; + } pr_err("Unrecognized literal '%s'", literal); - return NAN; +out: + pr_debug2("literal: %s = %f\n", literal, result); + return result; } -- 2.34.0.rc2.393.gf8c9666880-goog ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/3] perf tools: Fix SMT not detected with large core count 2021-11-23 22:48 [PATCH 1/3] perf expr: Add debug logging for literals Ian Rogers @ 2021-11-23 22:48 ` Ian Rogers 2021-11-23 23:34 ` Arnaldo Carvalho de Melo 2021-11-23 22:48 ` [PATCH 3/3] perf tools: Probe non-deprecated sysfs path 1st Ian Rogers 1 sibling, 1 reply; 5+ messages in thread From: Ian Rogers @ 2021-11-23 22:48 UTC (permalink / raw) To: Andi Kleen, Jiri Olsa, Namhyung Kim, John Garry, Kajol Jain, Paul A . Clarke, Arnaldo Carvalho de Melo, Kan Liang, Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin, Konstantin Khlebnikov, linux-perf-users, linux-kernel Cc: eranian, Ian Rogers sysfs__read_int returns 0 on success, and so the fast read path was always failing. strtoull can only read a 64-bit bitmap. On an AMD EPYC core_cpus may look like: 00000000,00000000,00000000,00000001,00000000,00000000,00000000,00000001 and so the sibling wasn't spotted. Fix by writing a simple hweight string parser. Fixes: bb629484d924 (perf tools: Simplify checking if SMT is active.) Signed-off-by: Ian Rogers <irogers@google.com> --- tools/perf/util/smt.c | 68 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 58 insertions(+), 10 deletions(-) diff --git a/tools/perf/util/smt.c b/tools/perf/util/smt.c index 20bacd5972ad..2636be65305a 100644 --- a/tools/perf/util/smt.c +++ b/tools/perf/util/smt.c @@ -5,6 +5,56 @@ #include "api/fs/fs.h" #include "smt.h" +/** + * hweight_str - Returns the number of bits set in str. Stops at first non-hex + * or ',' character. + */ +static int hweight_str(char *str) +{ + int result = 0; + + while (*str) { + switch (*str++) { + case '0': + case ',': + break; + case '1': + case '2': + case '4': + case '8': + result++; + break; + case '3': + case '5': + case '6': + case '9': + case 'a': + case 'A': + case 'c': + case 'C': + result += 2; + break; + case '7': + case 'b': + case 'B': + case 'd': + case 'D': + case 'e': + case 'E': + result += 3; + break; + case 'f': + case 'F': + result += 4; + break; + default: + goto done; + } + } +done: + return result; +} + int smt_on(void) { static bool cached; @@ -15,9 +65,12 @@ int smt_on(void) if (cached) return cached_result; - if (sysfs__read_int("devices/system/cpu/smt/active", &cached_result) > 0) - goto done; + if (sysfs__read_int("devices/system/cpu/smt/active", &cached_result) >= 0) { + cached = true; + return cached_result; + } + cached_result = 0; ncpu = sysconf(_SC_NPROCESSORS_CONF); for (cpu = 0; cpu < ncpu; cpu++) { unsigned long long siblings; @@ -35,18 +88,13 @@ int smt_on(void) continue; } /* Entry is hex, but does not have 0x, so need custom parser */ - siblings = strtoull(str, NULL, 16); + siblings = hweight_str(str); free(str); - if (hweight64(siblings) > 1) { + if (siblings > 1) { cached_result = 1; - cached = true; break; } } - if (!cached) { - cached_result = 0; -done: - cached = true; - } + cached = true; return cached_result; } -- 2.34.0.rc2.393.gf8c9666880-goog ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/3] perf tools: Fix SMT not detected with large core count 2021-11-23 22:48 ` [PATCH 2/3] perf tools: Fix SMT not detected with large core count Ian Rogers @ 2021-11-23 23:34 ` Arnaldo Carvalho de Melo 2021-11-24 0:02 ` Ian Rogers 0 siblings, 1 reply; 5+ messages in thread From: Arnaldo Carvalho de Melo @ 2021-11-23 23:34 UTC (permalink / raw) To: Ian Rogers Cc: Andi Kleen, Jiri Olsa, Namhyung Kim, John Garry, Kajol Jain, Paul A . Clarke, Kan Liang, Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin, Konstantin Khlebnikov, linux-perf-users, linux-kernel, eranian Em Tue, Nov 23, 2021 at 02:48:20PM -0800, Ian Rogers escreveu: > sysfs__read_int returns 0 on success, and so the fast read path was > always failing. Please split this into two patches, the above part should be in one, and the strtoull in another. Also can't we just do as ./tools/perf/util/cputopo.c and use instead core_cpus_list? On a 5950x: ⬢[acme@toolbox perf]$ cat /sys/devices/system/cpu/cpu31/topology/core_cpus 80008000 ⬢[acme@toolbox perf]$ cat /sys/devices/system/cpu/cpu31/topology/core_cpus_list 15,31 ⬢[acme@toolbox perf]$ cat /sys/devices/system/cpu/cpu0/topology/core_cpus 00010001 ⬢[acme@toolbox perf]$ cat /sys/devices/system/cpu/cpu0/topology/core_cpus_list 0,16 ⬢[acme@toolbox perf]$ - Arnaldo > strtoull can only read a 64-bit bitmap. On an AMD EPYC core_cpus may look > like: > 00000000,00000000,00000000,00000001,00000000,00000000,00000000,00000001 > and so the sibling wasn't spotted. Fix by writing a simple hweight string > parser. > > Fixes: bb629484d924 (perf tools: Simplify checking if SMT is active.) > Signed-off-by: Ian Rogers <irogers@google.com> > --- > tools/perf/util/smt.c | 68 ++++++++++++++++++++++++++++++++++++------- > 1 file changed, 58 insertions(+), 10 deletions(-) > > diff --git a/tools/perf/util/smt.c b/tools/perf/util/smt.c > index 20bacd5972ad..2636be65305a 100644 > --- a/tools/perf/util/smt.c > +++ b/tools/perf/util/smt.c > @@ -5,6 +5,56 @@ > #include "api/fs/fs.h" > #include "smt.h" > > +/** > + * hweight_str - Returns the number of bits set in str. Stops at first non-hex > + * or ',' character. > + */ > +static int hweight_str(char *str) > +{ > + int result = 0; > + > + while (*str) { > + switch (*str++) { > + case '0': > + case ',': > + break; > + case '1': > + case '2': > + case '4': > + case '8': > + result++; > + break; > + case '3': > + case '5': > + case '6': > + case '9': > + case 'a': > + case 'A': > + case 'c': > + case 'C': > + result += 2; > + break; > + case '7': > + case 'b': > + case 'B': > + case 'd': > + case 'D': > + case 'e': > + case 'E': > + result += 3; > + break; > + case 'f': > + case 'F': > + result += 4; > + break; > + default: > + goto done; > + } > + } > +done: > + return result; > +} > + > int smt_on(void) > { > static bool cached; > @@ -15,9 +65,12 @@ int smt_on(void) > if (cached) > return cached_result; > > - if (sysfs__read_int("devices/system/cpu/smt/active", &cached_result) > 0) > - goto done; > + if (sysfs__read_int("devices/system/cpu/smt/active", &cached_result) >= 0) { > + cached = true; > + return cached_result; > + } > > + cached_result = 0; > ncpu = sysconf(_SC_NPROCESSORS_CONF); > for (cpu = 0; cpu < ncpu; cpu++) { > unsigned long long siblings; > @@ -35,18 +88,13 @@ int smt_on(void) > continue; > } > /* Entry is hex, but does not have 0x, so need custom parser */ > - siblings = strtoull(str, NULL, 16); > + siblings = hweight_str(str); > free(str); > - if (hweight64(siblings) > 1) { > + if (siblings > 1) { > cached_result = 1; > - cached = true; > break; > } > } > - if (!cached) { > - cached_result = 0; > -done: > - cached = true; > - } > + cached = true; > return cached_result; > } > -- > 2.34.0.rc2.393.gf8c9666880-goog -- - Arnaldo ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/3] perf tools: Fix SMT not detected with large core count 2021-11-23 23:34 ` Arnaldo Carvalho de Melo @ 2021-11-24 0:02 ` Ian Rogers 0 siblings, 0 replies; 5+ messages in thread From: Ian Rogers @ 2021-11-24 0:02 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Andi Kleen, Jiri Olsa, Namhyung Kim, John Garry, Kajol Jain, Paul A . Clarke, Kan Liang, Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin, Konstantin Khlebnikov, linux-perf-users, linux-kernel, eranian On Tue, Nov 23, 2021 at 3:34 PM Arnaldo Carvalho de Melo <acme@kernel.org> wrote: > > Em Tue, Nov 23, 2021 at 02:48:20PM -0800, Ian Rogers escreveu: > > sysfs__read_int returns 0 on success, and so the fast read path was > > always failing. > > Please split this into two patches, the above part should be in one, and > the strtoull in another. Will do. > Also can't we just do as ./tools/perf/util/cputopo.c and use instead > core_cpus_list? It is more complex for the list case as the list may have a range. It shouldn't really matter with the active fix in any case. I think ideally we'd have an abstraction like cpuset in util-linux: https://github.com/util-linux/util-linux/blob/master/lib/cpuset.c This is a bit beyond what I'm trying to fix here. Thanks, Ian > On a 5950x: > > ⬢[acme@toolbox perf]$ cat /sys/devices/system/cpu/cpu31/topology/core_cpus > 80008000 > ⬢[acme@toolbox perf]$ cat /sys/devices/system/cpu/cpu31/topology/core_cpus_list > 15,31 > ⬢[acme@toolbox perf]$ cat /sys/devices/system/cpu/cpu0/topology/core_cpus > 00010001 > ⬢[acme@toolbox perf]$ cat /sys/devices/system/cpu/cpu0/topology/core_cpus_list > 0,16 > ⬢[acme@toolbox perf]$ > > - Arnaldo > > > strtoull can only read a 64-bit bitmap. On an AMD EPYC core_cpus may look > > like: > > 00000000,00000000,00000000,00000001,00000000,00000000,00000000,00000001 > > and so the sibling wasn't spotted. Fix by writing a simple hweight string > > parser. > > > > Fixes: bb629484d924 (perf tools: Simplify checking if SMT is active.) > > Signed-off-by: Ian Rogers <irogers@google.com> > > --- > > tools/perf/util/smt.c | 68 ++++++++++++++++++++++++++++++++++++------- > > 1 file changed, 58 insertions(+), 10 deletions(-) > > > > diff --git a/tools/perf/util/smt.c b/tools/perf/util/smt.c > > index 20bacd5972ad..2636be65305a 100644 > > --- a/tools/perf/util/smt.c > > +++ b/tools/perf/util/smt.c > > @@ -5,6 +5,56 @@ > > #include "api/fs/fs.h" > > #include "smt.h" > > > > +/** > > + * hweight_str - Returns the number of bits set in str. Stops at first non-hex > > + * or ',' character. > > + */ > > +static int hweight_str(char *str) > > +{ > > + int result = 0; > > + > > + while (*str) { > > + switch (*str++) { > > + case '0': > > + case ',': > > + break; > > + case '1': > > + case '2': > > + case '4': > > + case '8': > > + result++; > > + break; > > + case '3': > > + case '5': > > + case '6': > > + case '9': > > + case 'a': > > + case 'A': > > + case 'c': > > + case 'C': > > + result += 2; > > + break; > > + case '7': > > + case 'b': > > + case 'B': > > + case 'd': > > + case 'D': > > + case 'e': > > + case 'E': > > + result += 3; > > + break; > > + case 'f': > > + case 'F': > > + result += 4; > > + break; > > + default: > > + goto done; > > + } > > + } > > +done: > > + return result; > > +} > > + > > int smt_on(void) > > { > > static bool cached; > > @@ -15,9 +65,12 @@ int smt_on(void) > > if (cached) > > return cached_result; > > > > - if (sysfs__read_int("devices/system/cpu/smt/active", &cached_result) > 0) > > - goto done; > > + if (sysfs__read_int("devices/system/cpu/smt/active", &cached_result) >= 0) { > > + cached = true; > > + return cached_result; > > + } > > > > + cached_result = 0; > > ncpu = sysconf(_SC_NPROCESSORS_CONF); > > for (cpu = 0; cpu < ncpu; cpu++) { > > unsigned long long siblings; > > @@ -35,18 +88,13 @@ int smt_on(void) > > continue; > > } > > /* Entry is hex, but does not have 0x, so need custom parser */ > > - siblings = strtoull(str, NULL, 16); > > + siblings = hweight_str(str); > > free(str); > > - if (hweight64(siblings) > 1) { > > + if (siblings > 1) { > > cached_result = 1; > > - cached = true; > > break; > > } > > } > > - if (!cached) { > > - cached_result = 0; > > -done: > > - cached = true; > > - } > > + cached = true; > > return cached_result; > > } > > -- > > 2.34.0.rc2.393.gf8c9666880-goog > > -- > > - Arnaldo ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 3/3] perf tools: Probe non-deprecated sysfs path 1st 2021-11-23 22:48 [PATCH 1/3] perf expr: Add debug logging for literals Ian Rogers 2021-11-23 22:48 ` [PATCH 2/3] perf tools: Fix SMT not detected with large core count Ian Rogers @ 2021-11-23 22:48 ` Ian Rogers 1 sibling, 0 replies; 5+ messages in thread From: Ian Rogers @ 2021-11-23 22:48 UTC (permalink / raw) To: Andi Kleen, Jiri Olsa, Namhyung Kim, John Garry, Kajol Jain, Paul A . Clarke, Arnaldo Carvalho de Melo, Kan Liang, Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin, Konstantin Khlebnikov, linux-perf-users, linux-kernel Cc: eranian, Ian Rogers Following Documentation/ABI/stable/sysfs-devices-system-cpu the /sys/devices/system/cpu/cpuX/topology/core_cpus is deprecated in favor of thread_siblings, so probe thread_siblings before falling back on core_cpus. Signed-off-by: Ian Rogers <irogers@google.com> --- tools/perf/util/smt.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tools/perf/util/smt.c b/tools/perf/util/smt.c index 2636be65305a..2b0a36ebf27a 100644 --- a/tools/perf/util/smt.c +++ b/tools/perf/util/smt.c @@ -79,11 +79,10 @@ int smt_on(void) char fn[256]; snprintf(fn, sizeof fn, - "devices/system/cpu/cpu%d/topology/core_cpus", cpu); + "devices/system/cpu/cpu%d/topology/thread_siblings", cpu); if (sysfs__read_str(fn, &str, &strlen) < 0) { snprintf(fn, sizeof fn, - "devices/system/cpu/cpu%d/topology/thread_siblings", - cpu); + "devices/system/cpu/cpu%d/topology/core_cpus", cpu); if (sysfs__read_str(fn, &str, &strlen) < 0) continue; } -- 2.34.0.rc2.393.gf8c9666880-goog ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-11-24 0:03 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-11-23 22:48 [PATCH 1/3] perf expr: Add debug logging for literals Ian Rogers 2021-11-23 22:48 ` [PATCH 2/3] perf tools: Fix SMT not detected with large core count Ian Rogers 2021-11-23 23:34 ` Arnaldo Carvalho de Melo 2021-11-24 0:02 ` Ian Rogers 2021-11-23 22:48 ` [PATCH 3/3] perf tools: Probe non-deprecated sysfs path 1st Ian Rogers
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).