[PATCH v3 0/2] Add arch TSC frequency information

[PATCH v3 0/2] Add arch TSC frequency information

[Ian Rogers]

The first patch adds support for deriving from /proc/cpuinfo and adds
tests. The second patch from Kan Liang derives it from CPUID available
on newer Intel processors as discussed here:
https://lore.kernel.org/lkml/20220527040407.4193232-1-irogers@google.com/#t

v2. Adds warnings to make clear if things have changed/broken on future
    Intel platforms. It also adds caching and an Intel specific that a
    value is computed.

Ian Rogers (1):
  perf metrics: Add literal for system TSC frequency

Kan Liang (1):
  perf tsc: Add arch TSC frequency information

 tools/perf/arch/x86/util/cpuid.h  | 34 +++++++++++++++++
 tools/perf/arch/x86/util/header.c | 27 ++++++--------
 tools/perf/arch/x86/util/tsc.c    | 33 ++++++++++++++++
 tools/perf/tests/expr.c           | 15 ++++++++
 tools/perf/util/expr.c            | 62 +++++++++++++++++++++++++++++++
 tools/perf/util/tsc.h             |  1 +
 6 files changed, 156 insertions(+), 16 deletions(-)
 create mode 100644 tools/perf/arch/x86/util/cpuid.h

-- 
2.37.0.170.g444d1eabd0-goog

[PATCH v3 1/2] perf metrics: Add literal for system TSC frequency

[Ian Rogers]

Such a literal is useful to calculate things like the average frequency
[1]. The TSC frequency isn't exposed by sysfs although some experimental
drivers look to add it [2]. This change computes the value using the
frequency in /proc/cpuinfo which is accruate at least on Intel
processors.

[1] https://github.com/intel/perfmon-metrics/blob/5ad9ef7056f31075e8178b9f1fb732af183b2c8d/SKX/metrics/perf/skx_metric_perf.json#L11
[2] https://github.com/trailofbits/tsc_freq_khz

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/expr.c | 15 +++++++++++++
 tools/perf/util/expr.c  | 49 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+)

diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index 5c0032fe93ae..45afe4f24859 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c
@@ -1,8 +1,10 @@
 // SPDX-License-Identifier: GPL-2.0
 #include "util/debug.h"
 #include "util/expr.h"
+#include "util/header.h"
 #include "util/smt.h"
 #include "tests.h"
+#include <math.h>
 #include <stdlib.h>
 #include <string.h>
 #include <linux/zalloc.h>
@@ -69,6 +71,11 @@ static int test__expr(struct test_suite *t __maybe_unused, int subtest __maybe_u
 	double val, num_cpus, num_cores, num_dies, num_packages;
 	int ret;
 	struct expr_parse_ctx *ctx;
+	bool is_intel = false;
+	char buf[128];
+
+	if (!get_cpuid(buf, sizeof(buf)))
+		is_intel = strstr(buf, "Intel") != NULL;
 
 	TEST_ASSERT_EQUAL("ids_union", test_ids_union(), 0);
 
@@ -175,6 +182,14 @@ static int test__expr(struct test_suite *t __maybe_unused, int subtest __maybe_u
 	if (num_dies) // Some platforms do not have CPU die support, for example s390
 		TEST_ASSERT_VAL("#num_dies >= #num_packages", num_dies >= num_packages);
 
+	if (is_intel) {
+		double system_tsc_freq;
+
+		TEST_ASSERT_VAL("#system_tsc_freq", expr__parse(&system_tsc_freq, ctx,
+								"#system_tsc_freq") == 0);
+		TEST_ASSERT_VAL("!isnan(#system_tsc_freq)", !isnan(system_tsc_freq));
+	}
+
 	/*
 	 * Source count returns the number of events aggregating in a leader
 	 * event including the leader. Check parsing yields an id.
diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
index 675f318ce7c1..4c81533e4b43 100644
--- a/tools/perf/util/expr.c
+++ b/tools/perf/util/expr.c
@@ -402,6 +402,50 @@ double expr_id_data__source_count(const struct expr_id_data *data)
 	return data->val.source_count;
 }
 
+/*
+ * Derive the TSC frequency in Hz from the /proc/cpuinfo, for example:
+ * ...
+ * model name      : Intel(R) Xeon(R) Gold 6154 CPU @ 3.00GHz
+ * ...
+ * will return 3000000000.
+ */
+static double system_tsc_freq(void)
+{
+	static double result;
+	static bool computed;
+	FILE *cpuinfo;
+	char *line = NULL;
+	size_t len = 0;
+
+	if (computed)
+		return result;
+
+	computed = true;
+	result = NAN;
+	cpuinfo = fopen("/proc/cpuinfo", "r");
+	if (!cpuinfo) {
+		pr_err("Failed to read /proc/cpuinfo for TSC frequency");
+		return NAN;
+	}
+	while (getline(&line, &len, cpuinfo) > 0) {
+		if (!strncmp(line, "model name", 10)) {
+			char *pos = strstr(line + 11, " @ ");
+
+			if (pos && sscanf(pos, " @ %lfGHz", &result) == 1) {
+				result *= 1000000000;
+				goto out;
+			}
+		}
+	}
+out:
+	if (isnan(result))
+		pr_err("Failed to find TSC frequency in /proc/cpuinfo");
+
+	free(line);
+	fclose(cpuinfo);
+	return result;
+}
+
 double expr__get_literal(const char *literal)
 {
 	static struct cpu_topology *topology;
@@ -417,6 +461,11 @@ double expr__get_literal(const char *literal)
 		goto out;
 	}
 
+	if (!strcasecmp("#system_tsc_freq", literal)) {
+		result = system_tsc_freq();
+		goto out;
+	}
+
 	/*
 	 * Assume that topology strings are consistent, such as CPUs "0-1"
 	 * wouldn't be listed as "0,1", and so after deduplication the number of
-- 
2.37.0.170.g444d1eabd0-goog

Re: [PATCH v3 1/2] perf metrics: Add literal for system TSC frequency

[Liang, Kan]

Thanks Ian for working on the issue and coordinate the patches.

On 2022-07-15 6:35 p.m., Ian Rogers wrote:
> Such a literal is useful to calculate things like the average frequency
> [1]. The TSC frequency isn't exposed by sysfs although some experimental
> drivers look to add it [2]. This change computes the value using the
> frequency in /proc/cpuinfo which is accruate 

%s/accruate/accurate/

> at least on Intel processors.

I googled the cpuinfo of other Archs, e.g., arm and s390. It looks like
only Intel display the TSC frequency in the "model name".

So it may be better to move it to X86 specific code, arch/x86/util/tsc.c

> 
> [1] https://github.com/intel/perfmon-metrics/blob/5ad9ef7056f31075e8178b9f1fb732af183b2c8d/SKX/metrics/perf/skx_metric_perf.json#L11
> [2] https://github.com/trailofbits/tsc_freq_khz
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/tests/expr.c | 15 +++++++++++++
>  tools/perf/util/expr.c  | 49 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 64 insertions(+)
> 
> diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
> index 5c0032fe93ae..45afe4f24859 100644
> --- a/tools/perf/tests/expr.c
> +++ b/tools/perf/tests/expr.c
> @@ -1,8 +1,10 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include "util/debug.h"
>  #include "util/expr.h"
> +#include "util/header.h"
>  #include "util/smt.h"
>  #include "tests.h"
> +#include <math.h>
>  #include <stdlib.h>
>  #include <string.h>
>  #include <linux/zalloc.h>
> @@ -69,6 +71,11 @@ static int test__expr(struct test_suite *t __maybe_unused, int subtest __maybe_u
>  	double val, num_cpus, num_cores, num_dies, num_packages;
>  	int ret;
>  	struct expr_parse_ctx *ctx;
> +	bool is_intel = false;
> +	char buf[128];
> +
> +	if (!get_cpuid(buf, sizeof(buf)))
> +		is_intel = strstr(buf, "Intel") != NULL;
>  
>  	TEST_ASSERT_EQUAL("ids_union", test_ids_union(), 0);
>  
> @@ -175,6 +182,14 @@ static int test__expr(struct test_suite *t __maybe_unused, int subtest __maybe_u
>  	if (num_dies) // Some platforms do not have CPU die support, for example s390
>  		TEST_ASSERT_VAL("#num_dies >= #num_packages", num_dies >= num_packages);
>  
> +	if (is_intel) {
> +		double system_tsc_freq;
> +
> +		TEST_ASSERT_VAL("#system_tsc_freq", expr__parse(&system_tsc_freq, ctx,
> +								"#system_tsc_freq") == 0);


I think we should use arch_get_tsc_freq() to replace here.
This belong to a separate patch.


Thanks,
Kan
> +		TEST_ASSERT_VAL("!isnan(#system_tsc_freq)", !isnan(system_tsc_freq));
> +	}
> +
>  	/*
>  	 * Source count returns the number of events aggregating in a leader
>  	 * event including the leader. Check parsing yields an id.
> diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
> index 675f318ce7c1..4c81533e4b43 100644
> --- a/tools/perf/util/expr.c
> +++ b/tools/perf/util/expr.c
> @@ -402,6 +402,50 @@ double expr_id_data__source_count(const struct expr_id_data *data)
>  	return data->val.source_count;
>  }
>  
> +/*
> + * Derive the TSC frequency in Hz from the /proc/cpuinfo, for example:
> + * ...
> + * model name      : Intel(R) Xeon(R) Gold 6154 CPU @ 3.00GHz
> + * ...
> + * will return 3000000000.
> + */
> +static double system_tsc_freq(void)
> +{
> +	static double result;
> +	static bool computed;
> +	FILE *cpuinfo;
> +	char *line = NULL;
> +	size_t len = 0;
> +
> +	if (computed)
> +		return result;
> +
> +	computed = true;
> +	result = NAN;
> +	cpuinfo = fopen("/proc/cpuinfo", "r");
> +	if (!cpuinfo) {
> +		pr_err("Failed to read /proc/cpuinfo for TSC frequency");
> +		return NAN;
> +	}
> +	while (getline(&line, &len, cpuinfo) > 0) {
> +		if (!strncmp(line, "model name", 10)) {
> +			char *pos = strstr(line + 11, " @ ");
> +
> +			if (pos && sscanf(pos, " @ %lfGHz", &result) == 1) {
> +				result *= 1000000000;
> +				goto out;
> +			}
> +		}
> +	}
> +out:
> +	if (isnan(result))
> +		pr_err("Failed to find TSC frequency in /proc/cpuinfo");
> +
> +	free(line);
> +	fclose(cpuinfo);
> +	return result;
> +}
> +
>  double expr__get_literal(const char *literal)
>  {
>  	static struct cpu_topology *topology;
> @@ -417,6 +461,11 @@ double expr__get_literal(const char *literal)
>  		goto out;
>  	}
>  
> +	if (!strcasecmp("#system_tsc_freq", literal)) {
> +		result = system_tsc_freq();
> +		goto out;
> +	}
> +
>  	/*
>  	 * Assume that topology strings are consistent, such as CPUs "0-1"
>  	 * wouldn't be listed as "0,1", and so after deduplication the number of

[PATCH v3 2/2] perf tsc: Add arch TSC frequency information

[Ian Rogers]

From: Kan Liang <kan.liang@linux.intel.com>

The TSC frequency information is required for the event metrics with
the literal, system_tsc_freq. For the newer Intel platform, the TSC
frequency information can be retrieved from the CPUID leaf 0x15.
If the TSC frequency information isn't present the /proc/cpuinfo
approach is used.

Refactor cpuid for this use. Note, the previous stack pushing/popping
approach was broken on x86-64 that has stack red zones that would be
clobbered.

Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/arch/x86/util/cpuid.h  | 34 +++++++++++++++++++++++++++++++
 tools/perf/arch/x86/util/header.c | 27 ++++++++++--------------
 tools/perf/arch/x86/util/tsc.c    | 33 ++++++++++++++++++++++++++++++
 tools/perf/util/expr.c            | 15 +++++++++++++-
 tools/perf/util/tsc.h             |  1 +
 5 files changed, 93 insertions(+), 17 deletions(-)
 create mode 100644 tools/perf/arch/x86/util/cpuid.h

diff --git a/tools/perf/arch/x86/util/cpuid.h b/tools/perf/arch/x86/util/cpuid.h
new file mode 100644
index 000000000000..0a3ae0ace7e9
--- /dev/null
+++ b/tools/perf/arch/x86/util/cpuid.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef PERF_CPUID_H
+#define PERF_CPUID_H 1
+
+
+static inline void
+cpuid(unsigned int op, unsigned int op2, unsigned int *a, unsigned int *b,
+	unsigned int *c, unsigned int *d)
+{
+	/*
+	 * Preserve %ebx/%rbx register by either placing it in %rdi or saving it
+	 * on the stack - x86-64 needs to avoid the stack red zone. In PIC
+	 * compilations %ebx contains the address of the global offset
+	 * table. %rbx is occasionally used to address stack variables in
+	 * presence of dynamic allocas.
+	 */
+	asm(
+#if defined(__x86_64__)
+		"mov %%rbx, %%rdi\n"
+		"cpuid\n"
+		"xchg %%rdi, %%rbx\n"
+#else
+		"pushl %%ebx\n"
+		"cpuid\n"
+		"movl %%ebx, %%edi\n"
+		"popl %%ebx\n"
+#endif
+		: "=a"(*a), "=D"(*b), "=c"(*c), "=d"(*d)
+		: "a"(op), "2"(op2));
+}
+
+void get_cpuid_0(char *vendor, unsigned int *lvl);
+
+#endif
diff --git a/tools/perf/arch/x86/util/header.c b/tools/perf/arch/x86/util/header.c
index 578c8c568ffd..a51444a77a5f 100644
--- a/tools/perf/arch/x86/util/header.c
+++ b/tools/perf/arch/x86/util/header.c
@@ -9,18 +9,17 @@
 
 #include "../../../util/debug.h"
 #include "../../../util/header.h"
+#include "cpuid.h"
 
-static inline void
-cpuid(unsigned int op, unsigned int *a, unsigned int *b, unsigned int *c,
-      unsigned int *d)
+void get_cpuid_0(char *vendor, unsigned int *lvl)
 {
-	__asm__ __volatile__ (".byte 0x53\n\tcpuid\n\t"
-			      "movl %%ebx, %%esi\n\t.byte 0x5b"
-			: "=a" (*a),
-			"=S" (*b),
-			"=c" (*c),
-			"=d" (*d)
-			: "a" (op));
+	unsigned int b, c, d;
+
+	cpuid(0, 0, lvl, &b, &c, &d);
+	strncpy(&vendor[0], (char *)(&b), 4);
+	strncpy(&vendor[4], (char *)(&d), 4);
+	strncpy(&vendor[8], (char *)(&c), 4);
+	vendor[12] = '\0';
 }
 
 static int
@@ -31,14 +30,10 @@ __get_cpuid(char *buffer, size_t sz, const char *fmt)
 	int nb;
 	char vendor[16];
 
-	cpuid(0, &lvl, &b, &c, &d);
-	strncpy(&vendor[0], (char *)(&b), 4);
-	strncpy(&vendor[4], (char *)(&d), 4);
-	strncpy(&vendor[8], (char *)(&c), 4);
-	vendor[12] = '\0';
+	get_cpuid_0(vendor, &lvl);
 
 	if (lvl >= 1) {
-		cpuid(1, &a, &b, &c, &d);
+		cpuid(1, 0, &a, &b, &c, &d);
 
 		family = (a >> 8) & 0xf;  /* bits 11 - 8 */
 		model  = (a >> 4) & 0xf;  /* Bits  7 - 4 */
diff --git a/tools/perf/arch/x86/util/tsc.c b/tools/perf/arch/x86/util/tsc.c
index 559365f8fe52..b69144f22489 100644
--- a/tools/perf/arch/x86/util/tsc.c
+++ b/tools/perf/arch/x86/util/tsc.c
@@ -1,7 +1,9 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <linux/types.h>
+#include <string.h>
 
 #include "../../../util/tsc.h"
+#include "cpuid.h"
 
 u64 rdtsc(void)
 {
@@ -11,3 +13,34 @@ u64 rdtsc(void)
 
 	return low | ((u64)high) << 32;
 }
+
+double arch_get_tsc_freq(void)
+{
+	unsigned int a, b, c, d, lvl;
+	static bool cached;
+	static double tsc;
+	char vendor[16];
+
+	if (cached)
+		return tsc;
+
+	cached = true;
+	get_cpuid_0(vendor, &lvl);
+	if (!strstr(vendor, "Intel"))
+		return 0;
+
+	/*
+	 * Don't support Time Stamp Counter and
+	 * Nominal Core Crystal Clock Information Leaf.
+	 */
+	if (lvl < 0x15)
+		return 0;
+
+	cpuid(0x15, 0, &a, &b, &c, &d);
+	/* TSC frequency is not enumerated */
+	if (!a || !b || !c)
+		return 0;
+
+	tsc = (double)c * (double)b / (double)a;
+	return tsc;
+}
diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
index 4c81533e4b43..16f10e6d5ca5 100644
--- a/tools/perf/util/expr.c
+++ b/tools/perf/util/expr.c
@@ -12,6 +12,7 @@
 #include "expr-bison.h"
 #include "expr-flex.h"
 #include "smt.h"
+#include "tsc.h"
 #include <linux/err.h>
 #include <linux/kernel.h>
 #include <linux/zalloc.h>
@@ -443,9 +444,19 @@ static double system_tsc_freq(void)
 
 	free(line);
 	fclose(cpuinfo);
+	if (isnan(result))
+		pr_err("Error reading system_tsc_freq");
+
 	return result;
 }
 
+#if !defined(__i386__) && !defined(__x86_64__)
+double arch_get_tsc_freq(void)
+{
+	return 0.0;
+}
+#endif
+
 double expr__get_literal(const char *literal)
 {
 	static struct cpu_topology *topology;
@@ -462,7 +473,9 @@ double expr__get_literal(const char *literal)
 	}
 
 	if (!strcasecmp("#system_tsc_freq", literal)) {
-		result = system_tsc_freq();
+		result = arch_get_tsc_freq();
+		if (fpclassify(result) == FP_ZERO)
+			result = system_tsc_freq();
 		goto out;
 	}
 
diff --git a/tools/perf/util/tsc.h b/tools/perf/util/tsc.h
index 7d83a31732a7..88fd1c4c1cb8 100644
--- a/tools/perf/util/tsc.h
+++ b/tools/perf/util/tsc.h
@@ -25,6 +25,7 @@ int perf_read_tsc_conversion(const struct perf_event_mmap_page *pc,
 u64 perf_time_to_tsc(u64 ns, struct perf_tsc_conversion *tc);
 u64 tsc_to_perf_time(u64 cyc, struct perf_tsc_conversion *tc);
 u64 rdtsc(void);
+double arch_get_tsc_freq(void);
 
 size_t perf_event__fprintf_time_conv(union perf_event *event, FILE *fp);
 
-- 
2.37.0.170.g444d1eabd0-goog

Re: [PATCH v3 2/2] perf tsc: Add arch TSC frequency information

[Liang, Kan]

On 2022-07-15 6:35 p.m., Ian Rogers wrote:
> From: Kan Liang <kan.liang@linux.intel.com>
> 
> The TSC frequency information is required for the event metrics with
> the literal, system_tsc_freq. For the newer Intel platform, the TSC
> frequency information can be retrieved from the CPUID leaf 0x15.
> If the TSC frequency information isn't present the /proc/cpuinfo
> approach is used.
> 
> Refactor cpuid for this use. Note, the previous stack pushing/popping
> approach was broken on x86-64 that has stack red zones that would be
> clobbered.
> 
> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/arch/x86/util/cpuid.h  | 34 +++++++++++++++++++++++++++++++
>  tools/perf/arch/x86/util/header.c | 27 ++++++++++--------------
>  tools/perf/arch/x86/util/tsc.c    | 33 ++++++++++++++++++++++++++++++
>  tools/perf/util/expr.c            | 15 +++++++++++++-
>  tools/perf/util/tsc.h             |  1 +
>  5 files changed, 93 insertions(+), 17 deletions(-)
>  create mode 100644 tools/perf/arch/x86/util/cpuid.h
> 
> diff --git a/tools/perf/arch/x86/util/cpuid.h b/tools/perf/arch/x86/util/cpuid.h
> new file mode 100644
> index 000000000000..0a3ae0ace7e9
> --- /dev/null
> +++ b/tools/perf/arch/x86/util/cpuid.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef PERF_CPUID_H
> +#define PERF_CPUID_H 1
> +
> +
> +static inline void
> +cpuid(unsigned int op, unsigned int op2, unsigned int *a, unsigned int *b,
> +	unsigned int *c, unsigned int *d)
> +{
> +	/*
> +	 * Preserve %ebx/%rbx register by either placing it in %rdi or saving it
> +	 * on the stack - x86-64 needs to avoid the stack red zone. In PIC
> +	 * compilations %ebx contains the address of the global offset
> +	 * table. %rbx is occasionally used to address stack variables in
> +	 * presence of dynamic allocas.
> +	 */
> +	asm(
> +#if defined(__x86_64__)
> +		"mov %%rbx, %%rdi\n"
> +		"cpuid\n"
> +		"xchg %%rdi, %%rbx\n"
> +#else
> +		"pushl %%ebx\n"
> +		"cpuid\n"
> +		"movl %%ebx, %%edi\n"
> +		"popl %%ebx\n"
> +#endif
> +		: "=a"(*a), "=D"(*b), "=c"(*c), "=d"(*d)
> +		: "a"(op), "2"(op2));
> +}
> +
> +void get_cpuid_0(char *vendor, unsigned int *lvl);
> +
> +#endif
> diff --git a/tools/perf/arch/x86/util/header.c b/tools/perf/arch/x86/util/header.c
> index 578c8c568ffd..a51444a77a5f 100644
> --- a/tools/perf/arch/x86/util/header.c
> +++ b/tools/perf/arch/x86/util/header.c
> @@ -9,18 +9,17 @@
>  
>  #include "../../../util/debug.h"
>  #include "../../../util/header.h"
> +#include "cpuid.h"
>  
> -static inline void
> -cpuid(unsigned int op, unsigned int *a, unsigned int *b, unsigned int *c,
> -      unsigned int *d)
> +void get_cpuid_0(char *vendor, unsigned int *lvl)
>  {
> -	__asm__ __volatile__ (".byte 0x53\n\tcpuid\n\t"
> -			      "movl %%ebx, %%esi\n\t.byte 0x5b"
> -			: "=a" (*a),
> -			"=S" (*b),
> -			"=c" (*c),
> -			"=d" (*d)
> -			: "a" (op));
> +	unsigned int b, c, d;
> +
> +	cpuid(0, 0, lvl, &b, &c, &d);
> +	strncpy(&vendor[0], (char *)(&b), 4);
> +	strncpy(&vendor[4], (char *)(&d), 4);
> +	strncpy(&vendor[8], (char *)(&c), 4);
> +	vendor[12] = '\0';
>  }
>  
>  static int
> @@ -31,14 +30,10 @@ __get_cpuid(char *buffer, size_t sz, const char *fmt)
>  	int nb;
>  	char vendor[16];
>  
> -	cpuid(0, &lvl, &b, &c, &d);
> -	strncpy(&vendor[0], (char *)(&b), 4);
> -	strncpy(&vendor[4], (char *)(&d), 4);
> -	strncpy(&vendor[8], (char *)(&c), 4);
> -	vendor[12] = '\0';
> +	get_cpuid_0(vendor, &lvl);
>  
>  	if (lvl >= 1) {
> -		cpuid(1, &a, &b, &c, &d);
> +		cpuid(1, 0, &a, &b, &c, &d);
>  
>  		family = (a >> 8) & 0xf;  /* bits 11 - 8 */
>  		model  = (a >> 4) & 0xf;  /* Bits  7 - 4 */
> diff --git a/tools/perf/arch/x86/util/tsc.c b/tools/perf/arch/x86/util/tsc.c
> index 559365f8fe52..b69144f22489 100644
> --- a/tools/perf/arch/x86/util/tsc.c
> +++ b/tools/perf/arch/x86/util/tsc.c
> @@ -1,7 +1,9 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include <linux/types.h>
> +#include <string.h>
>  
>  #include "../../../util/tsc.h"
> +#include "cpuid.h"
>  
>  u64 rdtsc(void)
>  {
> @@ -11,3 +13,34 @@ u64 rdtsc(void)
>  
>  	return low | ((u64)high) << 32;
>  }
> +
> +double arch_get_tsc_freq(void)
> +{
> +	unsigned int a, b, c, d, lvl;
> +	static bool cached;
> +	static double tsc;
> +	char vendor[16];
> +
> +	if (cached)
> +		return tsc;
> +
> +	cached = true;
> +	get_cpuid_0(vendor, &lvl);
> +	if (!strstr(vendor, "Intel"))
> +		return 0;
> +
> +	/*
> +	 * Don't support Time Stamp Counter and
> +	 * Nominal Core Crystal Clock Information Leaf.
> +	 */
> +	if (lvl < 0x15)
> +		return 0;
> +
> +	cpuid(0x15, 0, &a, &b, &c, &d);
> +	/* TSC frequency is not enumerated */
> +	if (!a || !b || !c)
> +		return 0;
> +
> +	tsc = (double)c * (double)b / (double)a;
> +	return tsc;
> +}
> diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
> index 4c81533e4b43..16f10e6d5ca5 100644
> --- a/tools/perf/util/expr.c
> +++ b/tools/perf/util/expr.c
> @@ -12,6 +12,7 @@
>  #include "expr-bison.h"
>  #include "expr-flex.h"
>  #include "smt.h"
> +#include "tsc.h"
>  #include <linux/err.h>
>  #include <linux/kernel.h>
>  #include <linux/zalloc.h>
> @@ -443,9 +444,19 @@ static double system_tsc_freq(void)
>  
>  	free(line);
>  	fclose(cpuinfo);
> +	if (isnan(result))
> +		pr_err("Error reading system_tsc_freq");
> +
>  	return result;
>  }
>  
> +#if !defined(__i386__) && !defined(__x86_64__)
> +double arch_get_tsc_freq(void)

Other arch_* functions are __weak functions. I think it's better to keep
it consistent. It also avoid to add a new #if defined when adding a new
arch. Is there a problem to use __weak here?

Thanks,
Kan

> +{
> +	return 0.0;
> +}
> +#endif
> +
>  double expr__get_literal(const char *literal)
>  {
>  	static struct cpu_topology *topology;
> @@ -462,7 +473,9 @@ double expr__get_literal(const char *literal)
>  	}
>  
>  	if (!strcasecmp("#system_tsc_freq", literal)) {
> -		result = system_tsc_freq();
> +		result = arch_get_tsc_freq();
> +		if (fpclassify(result) == FP_ZERO)
> +			result = system_tsc_freq();
>  		goto out;
>  	}
>  
> diff --git a/tools/perf/util/tsc.h b/tools/perf/util/tsc.h
> index 7d83a31732a7..88fd1c4c1cb8 100644
> --- a/tools/perf/util/tsc.h
> +++ b/tools/perf/util/tsc.h
> @@ -25,6 +25,7 @@ int perf_read_tsc_conversion(const struct perf_event_mmap_page *pc,
>  u64 perf_time_to_tsc(u64 ns, struct perf_tsc_conversion *tc);
>  u64 tsc_to_perf_time(u64 cyc, struct perf_tsc_conversion *tc);
>  u64 rdtsc(void);
> +double arch_get_tsc_freq(void);
>  
>  size_t perf_event__fprintf_time_conv(union perf_event *event, FILE *fp);
>  

Re: [PATCH v3 2/2] perf tsc: Add arch TSC frequency information

[Ian Rogers]

On Mon, Jul 18, 2022 at 5:49 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
> On 2022-07-15 6:35 p.m., Ian Rogers wrote:
> > From: Kan Liang <kan.liang@linux.intel.com>
> >
> > The TSC frequency information is required for the event metrics with
> > the literal, system_tsc_freq. For the newer Intel platform, the TSC
> > frequency information can be retrieved from the CPUID leaf 0x15.
> > If the TSC frequency information isn't present the /proc/cpuinfo
> > approach is used.
> >
> > Refactor cpuid for this use. Note, the previous stack pushing/popping
> > approach was broken on x86-64 that has stack red zones that would be
> > clobbered.
> >
> > Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/arch/x86/util/cpuid.h  | 34 +++++++++++++++++++++++++++++++
> >  tools/perf/arch/x86/util/header.c | 27 ++++++++++--------------
> >  tools/perf/arch/x86/util/tsc.c    | 33 ++++++++++++++++++++++++++++++
> >  tools/perf/util/expr.c            | 15 +++++++++++++-
> >  tools/perf/util/tsc.h             |  1 +
> >  5 files changed, 93 insertions(+), 17 deletions(-)
> >  create mode 100644 tools/perf/arch/x86/util/cpuid.h
> >
> > diff --git a/tools/perf/arch/x86/util/cpuid.h b/tools/perf/arch/x86/util/cpuid.h
> > new file mode 100644
> > index 000000000000..0a3ae0ace7e9
> > --- /dev/null
> > +++ b/tools/perf/arch/x86/util/cpuid.h
> > @@ -0,0 +1,34 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef PERF_CPUID_H
> > +#define PERF_CPUID_H 1
> > +
> > +
> > +static inline void
> > +cpuid(unsigned int op, unsigned int op2, unsigned int *a, unsigned int *b,
> > +     unsigned int *c, unsigned int *d)
> > +{
> > +     /*
> > +      * Preserve %ebx/%rbx register by either placing it in %rdi or saving it
> > +      * on the stack - x86-64 needs to avoid the stack red zone. In PIC
> > +      * compilations %ebx contains the address of the global offset
> > +      * table. %rbx is occasionally used to address stack variables in
> > +      * presence of dynamic allocas.
> > +      */
> > +     asm(
> > +#if defined(__x86_64__)
> > +             "mov %%rbx, %%rdi\n"
> > +             "cpuid\n"
> > +             "xchg %%rdi, %%rbx\n"
> > +#else
> > +             "pushl %%ebx\n"
> > +             "cpuid\n"
> > +             "movl %%ebx, %%edi\n"
> > +             "popl %%ebx\n"
> > +#endif
> > +             : "=a"(*a), "=D"(*b), "=c"(*c), "=d"(*d)
> > +             : "a"(op), "2"(op2));
> > +}
> > +
> > +void get_cpuid_0(char *vendor, unsigned int *lvl);
> > +
> > +#endif
> > diff --git a/tools/perf/arch/x86/util/header.c b/tools/perf/arch/x86/util/header.c
> > index 578c8c568ffd..a51444a77a5f 100644
> > --- a/tools/perf/arch/x86/util/header.c
> > +++ b/tools/perf/arch/x86/util/header.c
> > @@ -9,18 +9,17 @@
> >
> >  #include "../../../util/debug.h"
> >  #include "../../../util/header.h"
> > +#include "cpuid.h"
> >
> > -static inline void
> > -cpuid(unsigned int op, unsigned int *a, unsigned int *b, unsigned int *c,
> > -      unsigned int *d)
> > +void get_cpuid_0(char *vendor, unsigned int *lvl)
> >  {
> > -     __asm__ __volatile__ (".byte 0x53\n\tcpuid\n\t"
> > -                           "movl %%ebx, %%esi\n\t.byte 0x5b"
> > -                     : "=a" (*a),
> > -                     "=S" (*b),
> > -                     "=c" (*c),
> > -                     "=d" (*d)
> > -                     : "a" (op));
> > +     unsigned int b, c, d;
> > +
> > +     cpuid(0, 0, lvl, &b, &c, &d);
> > +     strncpy(&vendor[0], (char *)(&b), 4);
> > +     strncpy(&vendor[4], (char *)(&d), 4);
> > +     strncpy(&vendor[8], (char *)(&c), 4);
> > +     vendor[12] = '\0';
> >  }
> >
> >  static int
> > @@ -31,14 +30,10 @@ __get_cpuid(char *buffer, size_t sz, const char *fmt)
> >       int nb;
> >       char vendor[16];
> >
> > -     cpuid(0, &lvl, &b, &c, &d);
> > -     strncpy(&vendor[0], (char *)(&b), 4);
> > -     strncpy(&vendor[4], (char *)(&d), 4);
> > -     strncpy(&vendor[8], (char *)(&c), 4);
> > -     vendor[12] = '\0';
> > +     get_cpuid_0(vendor, &lvl);
> >
> >       if (lvl >= 1) {
> > -             cpuid(1, &a, &b, &c, &d);
> > +             cpuid(1, 0, &a, &b, &c, &d);
> >
> >               family = (a >> 8) & 0xf;  /* bits 11 - 8 */
> >               model  = (a >> 4) & 0xf;  /* Bits  7 - 4 */
> > diff --git a/tools/perf/arch/x86/util/tsc.c b/tools/perf/arch/x86/util/tsc.c
> > index 559365f8fe52..b69144f22489 100644
> > --- a/tools/perf/arch/x86/util/tsc.c
> > +++ b/tools/perf/arch/x86/util/tsc.c
> > @@ -1,7 +1,9 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >  #include <linux/types.h>
> > +#include <string.h>
> >
> >  #include "../../../util/tsc.h"
> > +#include "cpuid.h"
> >
> >  u64 rdtsc(void)
> >  {
> > @@ -11,3 +13,34 @@ u64 rdtsc(void)
> >
> >       return low | ((u64)high) << 32;
> >  }
> > +
> > +double arch_get_tsc_freq(void)
> > +{
> > +     unsigned int a, b, c, d, lvl;
> > +     static bool cached;
> > +     static double tsc;
> > +     char vendor[16];
> > +
> > +     if (cached)
> > +             return tsc;
> > +
> > +     cached = true;
> > +     get_cpuid_0(vendor, &lvl);
> > +     if (!strstr(vendor, "Intel"))
> > +             return 0;
> > +
> > +     /*
> > +      * Don't support Time Stamp Counter and
> > +      * Nominal Core Crystal Clock Information Leaf.
> > +      */
> > +     if (lvl < 0x15)
> > +             return 0;
> > +
> > +     cpuid(0x15, 0, &a, &b, &c, &d);
> > +     /* TSC frequency is not enumerated */
> > +     if (!a || !b || !c)
> > +             return 0;
> > +
> > +     tsc = (double)c * (double)b / (double)a;
> > +     return tsc;
> > +}
> > diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
> > index 4c81533e4b43..16f10e6d5ca5 100644
> > --- a/tools/perf/util/expr.c
> > +++ b/tools/perf/util/expr.c
> > @@ -12,6 +12,7 @@
> >  #include "expr-bison.h"
> >  #include "expr-flex.h"
> >  #include "smt.h"
> > +#include "tsc.h"
> >  #include <linux/err.h>
> >  #include <linux/kernel.h>
> >  #include <linux/zalloc.h>
> > @@ -443,9 +444,19 @@ static double system_tsc_freq(void)
> >
> >       free(line);
> >       fclose(cpuinfo);
> > +     if (isnan(result))
> > +             pr_err("Error reading system_tsc_freq");
> > +
> >       return result;
> >  }
> >
> > +#if !defined(__i386__) && !defined(__x86_64__)
> > +double arch_get_tsc_freq(void)
>
> Other arch_* functions are __weak functions. I think it's better to keep
> it consistent. It also avoid to add a new #if defined when adding a new
> arch. Is there a problem to use __weak here?

There are problems with weak in general and link time optimizations -
weak is implemented as a C compiler extension. There have been
patches/threads in the past about avoiding it. In this case it'd be
easy to get two arch_get_tsc_freq defined or none. Without weak these
are both linker errors. With weak your mileage varies.

Thanks,
Ian

> Thanks,
> Kan
>
> > +{
> > +     return 0.0;
> > +}
> > +#endif
> > +
> >  double expr__get_literal(const char *literal)
> >  {
> >       static struct cpu_topology *topology;
> > @@ -462,7 +473,9 @@ double expr__get_literal(const char *literal)
> >       }
> >
> >       if (!strcasecmp("#system_tsc_freq", literal)) {
> > -             result = system_tsc_freq();
> > +             result = arch_get_tsc_freq();
> > +             if (fpclassify(result) == FP_ZERO)
> > +                     result = system_tsc_freq();
> >               goto out;
> >       }
> >
> > diff --git a/tools/perf/util/tsc.h b/tools/perf/util/tsc.h
> > index 7d83a31732a7..88fd1c4c1cb8 100644
> > --- a/tools/perf/util/tsc.h
> > +++ b/tools/perf/util/tsc.h
> > @@ -25,6 +25,7 @@ int perf_read_tsc_conversion(const struct perf_event_mmap_page *pc,
> >  u64 perf_time_to_tsc(u64 ns, struct perf_tsc_conversion *tc);
> >  u64 tsc_to_perf_time(u64 cyc, struct perf_tsc_conversion *tc);
> >  u64 rdtsc(void);
> > +double arch_get_tsc_freq(void);
> >
> >  size_t perf_event__fprintf_time_conv(union perf_event *event, FILE *fp);
> >