[PATCH v1] perf test expr: Fix system_tsc_freq for only x86

The refactoring of tool PMU events to have a PMU then adding the expr
literals to the tool PMU made it so that the literal system_tsc_freq
was only supported on x86. Update the test expectations to match -
namely the parsing is x86 specific and only yields a non-zero value on
Intel.

Fixes: 609aa2667f67 ("perf tool_pmu: Switch to standard pmu functions and json descriptions")
Reported-by: Athira Rajeev <redacted>
Closes: https://lore.kernel.org/linux-perf-users/20241022140156.98854-1-atrajeev@linux.vnet.ibm.com/
Co-developed-by: Athira Rajeev <redacted>
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/expr.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index 41ff1affdfcd..726cf8d4da28 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c

@@ -75,14 +75,12 @@ static int test__expr(struct test_suite *t __maybe_unused, int subtest __maybe_u
 	double val, num_cpus_online, num_cpus, num_cores, num_dies, num_packages;
 	int ret;
 	struct expr_parse_ctx *ctx;
-	bool is_intel = false;
 	char strcmp_cpuid_buf[256];
 	struct perf_cpu cpu = {-1};
 	char *cpuid = get_cpuid_allow_env_override(cpu);
 	char *escaped_cpuid1, *escaped_cpuid2;
 
 	TEST_ASSERT_VAL("get_cpuid", cpuid);
-	is_intel = strstr(cpuid, "Intel") != NULL;
 
 	TEST_ASSERT_EQUAL("ids_union", test_ids_union(), 0);
 

@@ -245,12 +243,19 @@ 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);
 
-	TEST_ASSERT_VAL("#system_tsc_freq", expr__parse(&val, ctx, "#system_tsc_freq") == 0);
-	if (is_intel)
-		TEST_ASSERT_VAL("#system_tsc_freq > 0", val > 0);
-	else
-		TEST_ASSERT_VAL("#system_tsc_freq == 0", fpclassify(val) == FP_ZERO);
 
+	if (expr__parse(&val, ctx, "#system_tsc_freq") == 0) {
+		bool is_intel = strstr(cpuid, "Intel") != NULL;
+
+		if (is_intel)
+			TEST_ASSERT_VAL("#system_tsc_freq > 0", val > 0);
+		else
+			TEST_ASSERT_VAL("#system_tsc_freq == 0", fpclassify(val) == FP_ZERO);
+	} else {
+#if defined(__i386__) || defined(__x86_64__)
+		TEST_ASSERT_VAL("#system_tsc_freq unsupported", 0);
+#endif
+	}
 	/*
 	 * Source count returns the number of events aggregating in a leader
 	 * event including the leader. Check parsing yields an id.

-- 
2.47.0.338.g60cca15819-goog

Hi Ian,

On Wed, Dec 04, 2024 at 06:23:05PM -0800, Ian Rogers wrote:

The refactoring of tool PMU events to have a PMU then adding the expr
literals to the tool PMU made it so that the literal system_tsc_freq
was only supported on x86. Update the test expectations to match -
namely the parsing is x86 specific and only yields a non-zero value on
Intel.

Fixes: 609aa2667f67 ("perf tool_pmu: Switch to standard pmu functions and json descriptions")
Reported-by: Athira Rajeev <redacted>
Closes: https://lore.kernel.org/linux-perf-users/20241022140156.98854-1-atrajeev@linux.vnet.ibm.com/
Co-developed-by: Athira Rajeev <redacted>
Signed-off-by: Ian Rogers <irogers@google.com>

It failed on my VM.

  root@arm64-vm:~/build# ./perf test -v 7
  --- start ---
  test child forked, pid 2096
  Using CPUID 0x00000000000f0510
  division by zero
  syntax error
  Unrecognized literal '#system_tsc_freq'FAILED tests/expr.c:253 #system_tsc_freq == 0
  ---- end(-1) ----
    7: Simple expression parser                                        : FAILED!

Also Sasha reported that some (Intel?) guest machine doesn't have TSC
frequency.

Thanks,
Namhyung

+		else
+			TEST_ASSERT_VAL("#system_tsc_freq == 0", fpclassify(val) == FP_ZERO);
+	} else {
+#if defined(__i386__) || defined(__x86_64__)
+		TEST_ASSERT_VAL("#system_tsc_freq unsupported", 0);
+#endif
+	}
 	/*
 	 * Source count returns the number of events aggregating in a leader
 	 * event including the leader. Check parsing yields an id.
-- 
2.47.0.338.g60cca15819-goog

On Wed, Dec 4, 2024 at 9:47 PM Namhyung Kim [off-list ref] wrote:

Hi Ian,

On Wed, Dec 04, 2024 at 06:23:05PM -0800, Ian Rogers wrote:

quoted

The refactoring of tool PMU events to have a PMU then adding the expr
literals to the tool PMU made it so that the literal system_tsc_freq
was only supported on x86. Update the test expectations to match -
namely the parsing is x86 specific and only yields a non-zero value on
Intel.

Fixes: 609aa2667f67 ("perf tool_pmu: Switch to standard pmu functions and json descriptions")
Reported-by: Athira Rajeev <redacted>
Closes: https://lore.kernel.org/linux-perf-users/20241022140156.98854-1-atrajeev@linux.vnet.ibm.com/
Co-developed-by: Athira Rajeev <redacted>
Signed-off-by: Ian Rogers <irogers@google.com>

It failed on my VM.

  root@arm64-vm:~/build# ./perf test -v 7
  --- start ---
  test child forked, pid 2096
  Using CPUID 0x00000000000f0510
  division by zero
  syntax error
  Unrecognized literal '#system_tsc_freq'FAILED tests/expr.c:253 #system_tsc_freq == 0
  ---- end(-1) ----
    7: Simple expression parser                                        : FAILED!

I'll need to check this. The test is looking for parsing failures, so
it's confusing to me expr__parse is returning 0. I was testing on x86
but disabling the literal in the tool PMU.

quoted

---
 tools/perf/tests/expr.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index 41ff1affdfcd..726cf8d4da28 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c

@@ -75,14 +75,12 @@ static int test__expr(struct test_suite *t __maybe_unused, int subtest __maybe_u
      double val, num_cpus_online, num_cpus, num_cores, num_dies, num_packages;
      int ret;
      struct expr_parse_ctx *ctx;
-     bool is_intel = false;
      char strcmp_cpuid_buf[256];
      struct perf_cpu cpu = {-1};
      char *cpuid = get_cpuid_allow_env_override(cpu);
      char *escaped_cpuid1, *escaped_cpuid2;

      TEST_ASSERT_VAL("get_cpuid", cpuid);
-     is_intel = strstr(cpuid, "Intel") != NULL;

      TEST_ASSERT_EQUAL("ids_union", test_ids_union(), 0);

@@ -245,12 +243,19 @@ 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);

-     TEST_ASSERT_VAL("#system_tsc_freq", expr__parse(&val, ctx, "#system_tsc_freq") == 0);
-     if (is_intel)
-             TEST_ASSERT_VAL("#system_tsc_freq > 0", val > 0);
-     else
-             TEST_ASSERT_VAL("#system_tsc_freq == 0", fpclassify(val) == FP_ZERO);

+     if (expr__parse(&val, ctx, "#system_tsc_freq") == 0) {
+             bool is_intel = strstr(cpuid, "Intel") != NULL;
+
+             if (is_intel)
+                     TEST_ASSERT_VAL("#system_tsc_freq > 0", val > 0);

Also Sasha reported that some (Intel?) guest machine doesn't have TSC
frequency.

I think, unfortunately, this is working as intended. Intel metrics use
#system_tsc_freq in metrics for most models:

$ grep -ril system_tsc_freq tools/perf/pmu-events/arch/x86/
tools/perf/pmu-events/arch/x86/emeraldrapids/emr-metrics.json
tools/perf/pmu-events/arch/x86/broadwellx/bdx-metrics.json
tools/perf/pmu-events/arch/x86/haswellx/hsx-metrics.json
tools/perf/pmu-events/arch/x86/grandridge/grr-metrics.json
tools/perf/pmu-events/arch/x86/icelakex/icx-metrics.json
tools/perf/pmu-events/arch/x86/sierraforest/srf-metrics.json
tools/perf/pmu-events/arch/x86/skylakex/skx-metrics.json
tools/perf/pmu-events/arch/x86/cascadelakex/clx-metrics.json
tools/perf/pmu-events/arch/x86/sapphirerapids/spr-metrics.json

The code to generate the TSC frequency uses the CPUID leaf
information, but this can be disabled by the host operating system for
guest operating systems. The fallback logic using `/proc/cpuinfo` is
intended for older models and it appears the more recent formatting
won't be parse-able by perf. The host has also likely disabled the
information if the CPUID leaf is hidden. So the test is correctly
failing because metrics using #system_tsc_freq would be broken inside
the guest OS. Kan was involved in the conversation when the literal
was added and this was the best we could do.

Thanks,
Ian

quoted

+             else
+                     TEST_ASSERT_VAL("#system_tsc_freq == 0", fpclassify(val) == FP_ZERO);
+     } else {
+#if defined(__i386__) || defined(__x86_64__)
+             TEST_ASSERT_VAL("#system_tsc_freq unsupported", 0);
+#endif
+     }
      /*
       * Source count returns the number of events aggregating in a leader
       * event including the leader. Check parsing yields an id.
--
2.47.0.338.g60cca15819-goog

On Wed, Dec 4, 2024 at 10:33 PM Ian Rogers [off-list ref] wrote:

On Wed, Dec 4, 2024 at 9:47 PM Namhyung Kim [off-list ref] wrote:

quoted

Hi Ian,

On Wed, Dec 04, 2024 at 06:23:05PM -0800, Ian Rogers wrote:

quoted

The refactoring of tool PMU events to have a PMU then adding the expr
literals to the tool PMU made it so that the literal system_tsc_freq
was only supported on x86. Update the test expectations to match -
namely the parsing is x86 specific and only yields a non-zero value on
Intel.

Fixes: 609aa2667f67 ("perf tool_pmu: Switch to standard pmu functions and json descriptions")
Reported-by: Athira Rajeev <redacted>
Closes: https://lore.kernel.org/linux-perf-users/20241022140156.98854-1-atrajeev@linux.vnet.ibm.com/
Co-developed-by: Athira Rajeev <redacted>
Signed-off-by: Ian Rogers <irogers@google.com>

It failed on my VM.

  root@arm64-vm:~/build# ./perf test -v 7
  --- start ---
  test child forked, pid 2096
  Using CPUID 0x00000000000f0510
  division by zero
  syntax error
  Unrecognized literal '#system_tsc_freq'FAILED tests/expr.c:253 #system_tsc_freq == 0
  ---- end(-1) ----
    7: Simple expression parser                                        : FAILED!

I'll need to check this. The test is looking for parsing failures, so
it's confusing to me expr__parse is returning 0. I was testing on x86
but disabling the literal in the tool PMU.

Hmm.. perhaps you had a similar issue to me and that b4 silently
failed as git user.email/user.name weren't configured? When I test on
a raspberry pi 5:

$ uname -a
Linux raspberrypi 6.6.51+rpt-rpi-2712 #1 SMP PREEMPT Debian
1:6.6.51-1+rpt3 (2024-10-08) aarch64 GNU/Linux
$ git log -1 --oneline
94733a0e50fd (HEAD -> ptn-expr-test) perf test expr: Fix
system_tsc_freq for only x86
$ /tmp/perf/perf test expr -v
Couldn't bump rlimit(MEMLOCK), failures may take place when creating
BPF maps, etc
  7: Simple expression parser                                        : Ok

Thanks,
Ian

On 5 Dec 2024, at 7:53 AM, Ian Rogers [off-list ref] wrote:

The refactoring of tool PMU events to have a PMU then adding the expr
literals to the tool PMU made it so that the literal system_tsc_freq
was only supported on x86. Update the test expectations to match -
namely the parsing is x86 specific and only yields a non-zero value on
Intel.

Fixes: 609aa2667f67 ("perf tool_pmu: Switch to standard pmu functions and json descriptions")
Reported-by: Athira Rajeev <redacted>
Closes: https://lore.kernel.org/linux-perf-users/20241022140156.98854-1-atrajeev@linux.vnet.ibm.com/
Co-developed-by: Athira Rajeev <redacted>
Signed-off-by: Ian Rogers <irogers@google.com>

Hi Ian, Namhyung

Tested with the changes on powerpc and good with the changes

# ./perf test "Simple expression parser"
  7: Simple expression parser                                        : Ok

Thanks
Athira

On Wed, Dec 04, 2024 at 11:09:50PM -0800, Ian Rogers wrote:

On Wed, Dec 4, 2024 at 10:33 PM Ian Rogers [off-list ref] wrote:

quoted

On Wed, Dec 4, 2024 at 9:47 PM Namhyung Kim [off-list ref] wrote:

quoted

Hi Ian,

On Wed, Dec 04, 2024 at 06:23:05PM -0800, Ian Rogers wrote:

quoted

The refactoring of tool PMU events to have a PMU then adding the expr
literals to the tool PMU made it so that the literal system_tsc_freq
was only supported on x86. Update the test expectations to match -
namely the parsing is x86 specific and only yields a non-zero value on
Intel.

Fixes: 609aa2667f67 ("perf tool_pmu: Switch to standard pmu functions and json descriptions")
Reported-by: Athira Rajeev <redacted>
Closes: https://lore.kernel.org/linux-perf-users/20241022140156.98854-1-atrajeev@linux.vnet.ibm.com/
Co-developed-by: Athira Rajeev <redacted>
Signed-off-by: Ian Rogers <irogers@google.com>

It failed on my VM.

  root@arm64-vm:~/build# ./perf test -v 7
  --- start ---
  test child forked, pid 2096
  Using CPUID 0x00000000000f0510
  division by zero
  syntax error
  Unrecognized literal '#system_tsc_freq'FAILED tests/expr.c:253 #system_tsc_freq == 0
  ---- end(-1) ----
    7: Simple expression parser                                        : FAILED!

I'll need to check this. The test is looking for parsing failures, so
it's confusing to me expr__parse is returning 0. I was testing on x86
but disabling the literal in the tool PMU.

Hmm.. perhaps you had a similar issue to me and that b4 silently
failed as git user.email/user.name weren't configured? When I test on

No, I confirmed it's appplied.  Maybe my VM setting has some problem.
After reboot + rebuild it works now, sorry for the noise.

Thanks,
Namhyung

a raspberry pi 5:

$ uname -a
Linux raspberrypi 6.6.51+rpt-rpi-2712 #1 SMP PREEMPT Debian
1:6.6.51-1+rpt3 (2024-10-08) aarch64 GNU/Linux
$ git log -1 --oneline
94733a0e50fd (HEAD -> ptn-expr-test) perf test expr: Fix
system_tsc_freq for only x86
$ /tmp/perf/perf test expr -v
Couldn't bump rlimit(MEMLOCK), failures may take place when creating
BPF maps, etc
  7: Simple expression parser                                        : Ok

Thanks,
Ian

On Wed, Dec 04, 2024 at 06:23:05PM -0800, Ian Rogers wrote:

The refactoring of tool PMU events to have a PMU then adding the expr
literals to the tool PMU made it so that the literal system_tsc_freq
was only supported on x86. Update the test expectations to match -
namely the parsing is x86 specific and only yields a non-zero value on
Intel.

Namhyung,

	Since you see no more problems and Athira tested it, I think
this should go via perf-tools, right?

- Arnaldo
 

On Tue, Dec 10, 2024 at 04:20:57PM -0300, Arnaldo Carvalho de Melo wrote:

On Wed, Dec 04, 2024 at 06:23:05PM -0800, Ian Rogers wrote:

quoted

The refactoring of tool PMU events to have a PMU then adding the expr
literals to the tool PMU made it so that the literal system_tsc_freq
was only supported on x86. Update the test expectations to match -
namely the parsing is x86 specific and only yields a non-zero value on
Intel.

Namhyung,

	Since you see no more problems and Athira tested it, I think
this should go via perf-tools, right?

Yep, will do.

Thanks,
Namhyung

On Wed, 04 Dec 2024 18:23:05 -0800, Ian Rogers wrote:

The refactoring of tool PMU events to have a PMU then adding the expr
literals to the tool PMU made it so that the literal system_tsc_freq
was only supported on x86. Update the test expectations to match -
namely the parsing is x86 specific and only yields a non-zero value on
Intel.


[...]

Applied to perf-tools, thanks!

Best regards,
Namhyung

On 12 Dec 2024, at 12:37 PM, Namhyung Kim [off-list ref] wrote:

On Wed, 04 Dec 2024 18:23:05 -0800, Ian Rogers wrote:

quoted

The refactoring of tool PMU events to have a PMU then adding the expr
literals to the tool PMU made it so that the literal system_tsc_freq
was only supported on x86. Update the test expectations to match -
namely the parsing is x86 specific and only yields a non-zero value on
Intel.


[...]

Applied to perf-tools, thanks!

Best regards,
Namhyung

Thanks Namhyung for pulling this in 

Athira