* [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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.