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/ (local)
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