[PATCH] tools/perf/tests/expr: Make the system_tsc_freq test only for intel

Subsystems: performance events subsystem, the rest

STALE653d

11 messages, 4 authors, 2024-12-05 · open the first message on its own page

[PATCH] tools/perf/tests/expr: Make the system_tsc_freq test only for intel

From: Athira Rajeev <hidden>
Date: 2024-10-22 14:02:39

The "Simple expression parser" test fails on powerpc
as below:

 parsing metric: #system_tsc_freq
 Unrecognized literal '#system_tsc_freq'literal: #system_tsc_freq = nan
 syntax error
 FAILED tests/expr.c:247 #system_tsc_freq
 ---- end(-1) ----
 7: Simple expression parser  : FAILED!

In the test, system_tsc_freq is checked as below:

 if (is_intel)
    TEST_ASSERT_VAL("#system_tsc_freq > 0", val > 0);
 else

But commit 609aa2667f67 ("perf tool_pmu: Switch to standard
pmu functions and json descriptions")' changed condition in
tool_pmu__skip_event so that system_tsc_freq event should
only appear on x86

 +#if !defined(__i386__) && !defined(__x86_64__)
 +       /* The system_tsc_freq event should only appear on x86. */
 +       if (strcasecmp(name, "system_tsc_freq") == 0)
 +               return true;
 +#endif

After this commit, the testcase breaks for expr__parse of
system_tsc_freq in powerpc case. Fix the testcase to have
complete system_tsc_freq test within "is_intel" check.

Signed-off-by: Athira Rajeev <redacted>
---
 tools/perf/tests/expr.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index e3aa9d4fcf3a..eb3bd68fc4ce 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c

@@ -244,11 +244,10 @@ 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)
+	if (is_intel) {
+		TEST_ASSERT_VAL("#system_tsc_freq", expr__parse(&val, ctx, "#system_tsc_freq") == 0);
 		TEST_ASSERT_VAL("#system_tsc_freq > 0", val > 0);
-	else
-		TEST_ASSERT_VAL("#system_tsc_freq == 0", fpclassify(val) == FP_ZERO);
+	}
 
 	/*
 	 * Source count returns the number of events aggregating in a leader

-- 
2.43.5

Re: [PATCH] tools/perf/tests/expr: Make the system_tsc_freq test only for intel

From: Namhyung Kim <namhyung@kernel.org>
Date: 2024-10-30 00:00:02

Hello,

On Tue, Oct 22, 2024 at 07:31:56PM +0530, Athira Rajeev wrote:

The "Simple expression parser" test fails on powerpc
as below:

 parsing metric: #system_tsc_freq
 Unrecognized literal '#system_tsc_freq'literal: #system_tsc_freq = nan
 syntax error
 FAILED tests/expr.c:247 #system_tsc_freq
 ---- end(-1) ----
 7: Simple expression parser  : FAILED!

In the test, system_tsc_freq is checked as below:

 if (is_intel)
    TEST_ASSERT_VAL("#system_tsc_freq > 0", val > 0);
 else

But commit 609aa2667f67 ("perf tool_pmu: Switch to standard
pmu functions and json descriptions")' changed condition in

Probably need to put it as Fixes: tag.

tool_pmu__skip_event so that system_tsc_freq event should
only appear on x86

 +#if !defined(__i386__) && !defined(__x86_64__)
 +       /* The system_tsc_freq event should only appear on x86. */
 +       if (strcasecmp(name, "system_tsc_freq") == 0)
 +               return true;
 +#endif

After this commit, the testcase breaks for expr__parse of
system_tsc_freq in powerpc case. Fix the testcase to have
complete system_tsc_freq test within "is_intel" check.

Ian, are you ok with this?

Thanks,
Namhyung

quoted hunk

Signed-off-by: Athira Rajeev <redacted>
---
 tools/perf/tests/expr.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index e3aa9d4fcf3a..eb3bd68fc4ce 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c

@@ -244,11 +244,10 @@ 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)
+	if (is_intel) {
+		TEST_ASSERT_VAL("#system_tsc_freq", expr__parse(&val, ctx, "#system_tsc_freq") == 0);
 		TEST_ASSERT_VAL("#system_tsc_freq > 0", val > 0);
-	else
-		TEST_ASSERT_VAL("#system_tsc_freq == 0", fpclassify(val) == FP_ZERO);
+	}
 
 	/*
 	 * Source count returns the number of events aggregating in a leader

-- 
2.43.5

Re: [PATCH] tools/perf/tests/expr: Make the system_tsc_freq test only for intel

From: Athira Rajeev <hidden>
Date: 2024-11-04 04:17:57

On 30 Oct 2024, at 5:29 AM, Namhyung Kim [off-list ref] wrote:

Hello,

On Tue, Oct 22, 2024 at 07:31:56PM +0530, Athira Rajeev wrote:

quoted

The "Simple expression parser" test fails on powerpc
as below:

parsing metric: #system_tsc_freq
Unrecognized literal '#system_tsc_freq'literal: #system_tsc_freq = nan
syntax error
FAILED tests/expr.c:247 #system_tsc_freq
---- end(-1) ----
7: Simple expression parser  : FAILED!

In the test, system_tsc_freq is checked as below:

if (is_intel)
   TEST_ASSERT_VAL("#system_tsc_freq > 0", val > 0);
else

But commit 609aa2667f67 ("perf tool_pmu: Switch to standard
pmu functions and json descriptions")' changed condition in

Probably need to put it as Fixes: tag.

quoted

tool_pmu__skip_event so that system_tsc_freq event should
only appear on x86

+#if !defined(__i386__) && !defined(__x86_64__)
+       /* The system_tsc_freq event should only appear on x86. */
+       if (strcasecmp(name, "system_tsc_freq") == 0)
+               return true;
+#endif

After this commit, the testcase breaks for expr__parse of
system_tsc_freq in powerpc case. Fix the testcase to have
complete system_tsc_freq test within "is_intel" check.

Ian, are you ok with this?

Thanks,
Namhyung

Hi Ian

If the change looks good to you, I will send a V2 with Fixes tag added. Please share your review comments

Hi James, Thomas

Looking for help to test since in non-intel platform, this test will fail without the patch

Thanks
Athira

quoted

Signed-off-by: Athira Rajeev <redacted>
---
tools/perf/tests/expr.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index e3aa9d4fcf3a..eb3bd68fc4ce 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c

@@ -244,11 +244,10 @@ 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)
+ if (is_intel) {
+ TEST_ASSERT_VAL("#system_tsc_freq", expr__parse(&val, ctx, "#system_tsc_freq") == 0);
TEST_ASSERT_VAL("#system_tsc_freq > 0", val > 0);
- else
- TEST_ASSERT_VAL("#system_tsc_freq == 0", fpclassify(val) == FP_ZERO);
+ }

/*
 * Source count returns the number of events aggregating in a leader
-- 
2.43.5

Re: [PATCH] tools/perf/tests/expr: Make the system_tsc_freq test only for intel

From: Ian Rogers <irogers@google.com>
Date: 2024-11-04 20:45:12

On Sun, Nov 3, 2024 at 8:17 PM Athira Rajeev
[off-list ref] wrote:

quoted

On 30 Oct 2024, at 5:29 AM, Namhyung Kim [off-list ref] wrote:

Hello,

On Tue, Oct 22, 2024 at 07:31:56PM +0530, Athira Rajeev wrote:

quoted

The "Simple expression parser" test fails on powerpc
as below:

parsing metric: #system_tsc_freq
Unrecognized literal '#system_tsc_freq'literal: #system_tsc_freq = nan
syntax error
FAILED tests/expr.c:247 #system_tsc_freq
---- end(-1) ----
7: Simple expression parser  : FAILED!

In the test, system_tsc_freq is checked as below:

if (is_intel)
   TEST_ASSERT_VAL("#system_tsc_freq > 0", val > 0);
else

But commit 609aa2667f67 ("perf tool_pmu: Switch to standard
pmu functions and json descriptions")' changed condition in

Probably need to put it as Fixes: tag.

quoted

tool_pmu__skip_event so that system_tsc_freq event should
only appear on x86

+#if !defined(__i386__) && !defined(__x86_64__)
+       /* The system_tsc_freq event should only appear on x86. */
+       if (strcasecmp(name, "system_tsc_freq") == 0)
+               return true;
+#endif

After this commit, the testcase breaks for expr__parse of
system_tsc_freq in powerpc case. Fix the testcase to have
complete system_tsc_freq test within "is_intel" check.

Ian, are you ok with this?

Thanks,
Namhyung

Hi Ian

If the change looks good to you, I will send a V2 with Fixes tag added. Please share your review comments

Hi James, Thomas

Looking for help to test since in non-intel platform, this test will fail without the patch


Hi Athira,

sorry for the breakage and thank you for the detailed explanation. As
the code will run on AMD I think your change will break that - . It is
probably safest to keep the ".. else { .." for this case but guard it
in the ifdef.

Thanks,
Ian

Thanks
Athira

quoted

Signed-off-by: Athira Rajeev <redacted>
---
tools/perf/tests/expr.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index e3aa9d4fcf3a..eb3bd68fc4ce 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c

@@ -244,11 +244,10 @@ 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)
+ if (is_intel) {
+ TEST_ASSERT_VAL("#system_tsc_freq", expr__parse(&val, ctx, "#system_tsc_freq") == 0);
TEST_ASSERT_VAL("#system_tsc_freq > 0", val > 0);
- else
- TEST_ASSERT_VAL("#system_tsc_freq == 0", fpclassify(val) == FP_ZERO);
+ }

/*
 * Source count returns the number of events aggregating in a leader
--
2.43.5

Re: [PATCH] tools/perf/tests/expr: Make the system_tsc_freq test only for intel

From: Athira Rajeev <hidden>
Date: 2024-11-06 10:09:14

On 5 Nov 2024, at 2:14 AM, Ian Rogers [off-list ref] wrote:

On Sun, Nov 3, 2024 at 8:17 PM Athira Rajeev
[off-list ref] wrote:

quoted

On 30 Oct 2024, at 5:29 AM, Namhyung Kim [off-list ref] wrote:

Hello,

On Tue, Oct 22, 2024 at 07:31:56PM +0530, Athira Rajeev wrote:

quoted

The "Simple expression parser" test fails on powerpc
as below:

parsing metric: #system_tsc_freq
Unrecognized literal '#system_tsc_freq'literal: #system_tsc_freq = nan
syntax error
FAILED tests/expr.c:247 #system_tsc_freq
---- end(-1) ----
7: Simple expression parser  : FAILED!

In the test, system_tsc_freq is checked as below:

if (is_intel)
  TEST_ASSERT_VAL("#system_tsc_freq > 0", val > 0);
else

But commit 609aa2667f67 ("perf tool_pmu: Switch to standard
pmu functions and json descriptions")' changed condition in

Probably need to put it as Fixes: tag.

quoted

tool_pmu__skip_event so that system_tsc_freq event should
only appear on x86

+#if !defined(__i386__) && !defined(__x86_64__)
+       /* The system_tsc_freq event should only appear on x86. */
+       if (strcasecmp(name, "system_tsc_freq") == 0)
+               return true;
+#endif

After this commit, the testcase breaks for expr__parse of
system_tsc_freq in powerpc case. Fix the testcase to have
complete system_tsc_freq test within "is_intel" check.

Ian, are you ok with this?

Thanks,
Namhyung

Hi Ian

If the change looks good to you, I will send a V2 with Fixes tag added. Please share your review comments

Hi James, Thomas

Looking for help to test since in non-intel platform, this test will fail without the patch


Hi Athira,

sorry for the breakage and thank you for the detailed explanation. As
the code will run on AMD I think your change will break that - . It is
probably safest to keep the ".. else { .." for this case but guard it
in the ifdef.

Hi Ian

Thanks for your comments. Does the below change looks good ?

diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index e3aa9d4fcf3a..f5b2d96bb59b 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c

@@ -74,14 +74,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_pmu *pmu = perf_pmus__find_core_pmu();
    char *cpuid = perf_pmu__getcpuid(pmu);
    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);

@@ -244,11 +242,13 @@ 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 defined(__i386__) && defined(__x86_64__)
    TEST_ASSERT_VAL("#system_tsc_freq", expr__parse(&val, ctx, "#system_tsc_freq") == 0);
-    if (is_intel)
+    if (strstr(cpuid, "Intel") != NULL)
        TEST_ASSERT_VAL("#system_tsc_freq > 0", val > 0);
    else
        TEST_ASSERT_VAL("#system_tsc_freq == 0", fpclassify(val) == FP_ZERO);
+#endif
 
    /*
     * Source count returns the number of events aggregating in a leader

Thanks
Athira

Thanks,
Ian

quoted

Thanks
Athira

quoted

Signed-off-by: Athira Rajeev <redacted>
---
tools/perf/tests/expr.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index e3aa9d4fcf3a..eb3bd68fc4ce 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c

@@ -244,11 +244,10 @@ 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)
+ if (is_intel) {
+ TEST_ASSERT_VAL("#system_tsc_freq", expr__parse(&val, ctx, "#system_tsc_freq") == 0);
TEST_ASSERT_VAL("#system_tsc_freq > 0", val > 0);
- else
- TEST_ASSERT_VAL("#system_tsc_freq == 0", fpclassify(val) == FP_ZERO);
+ }

/*
* Source count returns the number of events aggregating in a leader
--
2.43.5

Re: [PATCH] tools/perf/tests/expr: Make the system_tsc_freq test only for intel

From: Leo Yan <leo.yan@arm.com>
Date: 2024-11-07 20:40:06

Hi Athira,

On Wed, Nov 06, 2024 at 03:04:57PM +0530, Athira Rajeev wrote:

[...]

quoted hunk

quoted

Hi Athira,

sorry for the breakage and thank you for the detailed explanation. As
the code will run on AMD I think your change will break that - . It is
probably safest to keep the ".. else { .." for this case but guard it
in the ifdef.

Hi Ian

Thanks for your comments. Does the below change looks good ?

diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index e3aa9d4fcf3a..f5b2d96bb59b 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c

@@ -74,14 +74,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_pmu *pmu = perf_pmus__find_core_pmu();
    char *cpuid = perf_pmu__getcpuid(pmu);
    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);

@@ -244,11 +242,13 @@ 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 defined(__i386__) && defined(__x86_64__)
    TEST_ASSERT_VAL("#system_tsc_freq", expr__parse(&val, ctx, "#system_tsc_freq") == 0);
-    if (is_intel)
+    if (strstr(cpuid, "Intel") != NULL)
        TEST_ASSERT_VAL("#system_tsc_freq > 0", val > 0);
    else
        TEST_ASSERT_VAL("#system_tsc_freq == 0", fpclassify(val) == FP_ZERO);
+#endif

    /*
     * Source count returns the number of events aggregating in a leader

I confirmed the change above fixes the failure on Arm64.

Tested-by: Leo Yan <leo.yan@arm.com>

Re: [PATCH] tools/perf/tests/expr: Make the system_tsc_freq test only for intel

From: Athira Rajeev <hidden>
Date: 2024-11-08 05:21:09

On 7 Nov 2024, at 7:26 PM, Leo Yan [off-list ref] wrote:

Hi Athira,

On Wed, Nov 06, 2024 at 03:04:57PM +0530, Athira Rajeev wrote:

[...]

quoted

Hi Athira,

sorry for the breakage and thank you for the detailed explanation. As
the code will run on AMD I think your change will break that - . It is
probably safest to keep the ".. else { .." for this case but guard it
in the ifdef.

Hi Ian

Thanks for your comments. Does the below change looks good ?

diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index e3aa9d4fcf3a..f5b2d96bb59b 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c

@@ -74,14 +74,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_pmu *pmu = perf_pmus__find_core_pmu();
   char *cpuid = perf_pmu__getcpuid(pmu);
   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);

@@ -244,11 +242,13 @@ 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 defined(__i386__) && defined(__x86_64__)
   TEST_ASSERT_VAL("#system_tsc_freq", expr__parse(&val, ctx, "#system_tsc_freq") == 0);
-    if (is_intel)
+    if (strstr(cpuid, "Intel") != NULL)
       TEST_ASSERT_VAL("#system_tsc_freq > 0", val > 0);
   else
       TEST_ASSERT_VAL("#system_tsc_freq == 0", fpclassify(val) == FP_ZERO);
+#endif

   /*
    * Source count returns the number of events aggregating in a leader

I confirmed the change above fixes the failure on Arm64.

Tested-by: Leo Yan <leo.yan@arm.com>

Thanks Leo Yan for testing.

Hi Ian,

If the change above looks good, I will post a V2 . Please share your review comments

Thanks
Athira

Re: [PATCH] tools/perf/tests/expr: Make the system_tsc_freq test only for intel

From: Namhyung Kim <namhyung@kernel.org>
Date: 2024-12-03 18:16:16

Hello,

On Fri, Nov 08, 2024 at 10:50:10AM +0530, Athira Rajeev wrote:

quoted

On 7 Nov 2024, at 7:26 PM, Leo Yan [off-list ref] wrote:

Hi Athira,

On Wed, Nov 06, 2024 at 03:04:57PM +0530, Athira Rajeev wrote:

[...]

quoted

Hi Athira,

sorry for the breakage and thank you for the detailed explanation. As
the code will run on AMD I think your change will break that - . It is
probably safest to keep the ".. else { .." for this case but guard it
in the ifdef.

Hi Ian

Thanks for your comments. Does the below change looks good ?

diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index e3aa9d4fcf3a..f5b2d96bb59b 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c

@@ -74,14 +74,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_pmu *pmu = perf_pmus__find_core_pmu();
   char *cpuid = perf_pmu__getcpuid(pmu);
   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);

@@ -244,11 +242,13 @@ 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 defined(__i386__) && defined(__x86_64__)
   TEST_ASSERT_VAL("#system_tsc_freq", expr__parse(&val, ctx, "#system_tsc_freq") == 0);
-    if (is_intel)
+    if (strstr(cpuid, "Intel") != NULL)
       TEST_ASSERT_VAL("#system_tsc_freq > 0", val > 0);
   else
       TEST_ASSERT_VAL("#system_tsc_freq == 0", fpclassify(val) == FP_ZERO);
+#endif

   /*
    * Source count returns the number of events aggregating in a leader

I confirmed the change above fixes the failure on Arm64.

Tested-by: Leo Yan <leo.yan@arm.com>

Thanks Leo Yan for testing.

Hi Ian,

If the change above looks good, I will post a V2 . Please share your review comments

Sorry for the delay, it looks good to me.  Can you please send the v2?

Thanks,
Namhyung

Re: [PATCH] tools/perf/tests/expr: Make the system_tsc_freq test only for intel

From: Namhyung Kim <namhyung@kernel.org>
Date: 2024-12-03 18:42:52

On Tue, Dec 03, 2024 at 10:16:06AM -0800, Namhyung Kim wrote:

Hello,

On Fri, Nov 08, 2024 at 10:50:10AM +0530, Athira Rajeev wrote:

quoted

On 7 Nov 2024, at 7:26 PM, Leo Yan [off-list ref] wrote:

Hi Athira,

On Wed, Nov 06, 2024 at 03:04:57PM +0530, Athira Rajeev wrote:

[...]

quoted

Hi Athira,

sorry for the breakage and thank you for the detailed explanation. As
the code will run on AMD I think your change will break that - . It is
probably safest to keep the ".. else { .." for this case but guard it
in the ifdef.

Hi Ian

Thanks for your comments. Does the below change looks good ?

diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index e3aa9d4fcf3a..f5b2d96bb59b 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c

@@ -74,14 +74,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_pmu *pmu = perf_pmus__find_core_pmu();
   char *cpuid = perf_pmu__getcpuid(pmu);
   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);

@@ -244,11 +242,13 @@ 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 defined(__i386__) && defined(__x86_64__)
   TEST_ASSERT_VAL("#system_tsc_freq", expr__parse(&val, ctx, "#system_tsc_freq") == 0);
-    if (is_intel)
+    if (strstr(cpuid, "Intel") != NULL)
       TEST_ASSERT_VAL("#system_tsc_freq > 0", val > 0);
   else
       TEST_ASSERT_VAL("#system_tsc_freq == 0", fpclassify(val) == FP_ZERO);
+#endif

   /*
    * Source count returns the number of events aggregating in a leader

I confirmed the change above fixes the failure on Arm64.

Tested-by: Leo Yan <leo.yan@arm.com>

Thanks Leo Yan for testing.

Hi Ian,

If the change above looks good, I will post a V2 . Please share your review comments

Sorry for the delay, it looks good to me.  Can you please send the v2?

After looking at another report, I think we need to check the value of
TSC freq, not just the vendor.  Can you please test this?

Thanks,
Namhyung


---8<---

diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index 41ff1affdfcdf31c..45151696e7b76308 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c

@@ -5,6 +5,7 @@
 #include "util/hashmap.h"
 #include "util/header.h"
 #include "util/smt.h"
+#include "util/tsc.h"
 #include "tests.h"
 #include <perf/cpumap.h>
 #include <math.h>

@@ -75,14 +76,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);

@@ -246,10 +245,10 @@ static int test__expr(struct test_suite *t __maybe_unused, int subtest __maybe_u
 		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 (val > 0) {
+		TEST_ASSERT_VAL("#system_tsc_freq == arch_get_tsc_freq()",
+				val == arch_get_tsc_freq());
+	}
 
 	/*
 	 * Source count returns the number of events aggregating in a leader

Re: [PATCH] tools/perf/tests/expr: Make the system_tsc_freq test only for intel

From: Namhyung Kim <namhyung@kernel.org>
Date: 2024-12-03 18:59:35

On Tue, Dec 03, 2024 at 10:42:45AM -0800, Namhyung Kim wrote:

On Tue, Dec 03, 2024 at 10:16:06AM -0800, Namhyung Kim wrote:

quoted

Hello,

On Fri, Nov 08, 2024 at 10:50:10AM +0530, Athira Rajeev wrote:

quoted

On 7 Nov 2024, at 7:26 PM, Leo Yan [off-list ref] wrote:

Hi Athira,

On Wed, Nov 06, 2024 at 03:04:57PM +0530, Athira Rajeev wrote:

[...]

quoted

Hi Athira,

sorry for the breakage and thank you for the detailed explanation. As
the code will run on AMD I think your change will break that - . It is
probably safest to keep the ".. else { .." for this case but guard it
in the ifdef.

Hi Ian

Thanks for your comments. Does the below change looks good ?

diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index e3aa9d4fcf3a..f5b2d96bb59b 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c

@@ -74,14 +74,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_pmu *pmu = perf_pmus__find_core_pmu();
   char *cpuid = perf_pmu__getcpuid(pmu);
   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);

@@ -244,11 +242,13 @@ 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 defined(__i386__) && defined(__x86_64__)
   TEST_ASSERT_VAL("#system_tsc_freq", expr__parse(&val, ctx, "#system_tsc_freq") == 0);
-    if (is_intel)
+    if (strstr(cpuid, "Intel") != NULL)
       TEST_ASSERT_VAL("#system_tsc_freq > 0", val > 0);
   else
       TEST_ASSERT_VAL("#system_tsc_freq == 0", fpclassify(val) == FP_ZERO);
+#endif

   /*
    * Source count returns the number of events aggregating in a leader

I confirmed the change above fixes the failure on Arm64.

Tested-by: Leo Yan <leo.yan@arm.com>

Thanks Leo Yan for testing.

Hi Ian,

If the change above looks good, I will post a V2 . Please share your review comments

Sorry for the delay, it looks good to me.  Can you please send the v2?

After looking at another report, I think we need to check the value of
TSC freq, not just the vendor.  Can you please test this?

Oops, nevermind.  I've realized we have two different issues at the same
time.  So !x86 archs should not use #system_tsc_freq at all, and only
*some* of (real) Intel machines have the value actually.  Hmm...

I think we need the original v2 here, and check the value even on Intel
separately.

Thanks,
Namhyung

Re: [PATCH] tools/perf/tests/expr: Make the system_tsc_freq test only for intel

From: Athira Rajeev <hidden>
Date: 2024-12-05 17:02:25

On 3 Dec 2024, at 11:46 PM, Namhyung Kim [off-list ref] wrote:

Hello,

On Fri, Nov 08, 2024 at 10:50:10AM +0530, Athira Rajeev wrote:

quoted

On 7 Nov 2024, at 7:26 PM, Leo Yan [off-list ref] wrote:

Hi Athira,

On Wed, Nov 06, 2024 at 03:04:57PM +0530, Athira Rajeev wrote:

[...]

quoted

Hi Athira,

sorry for the breakage and thank you for the detailed explanation. As
the code will run on AMD I think your change will break that - . It is
probably safest to keep the ".. else { .." for this case but guard it
in the ifdef.

Hi Ian

Thanks for your comments. Does the below change looks good ?

diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
index e3aa9d4fcf3a..f5b2d96bb59b 100644
--- a/tools/perf/tests/expr.c
+++ b/tools/perf/tests/expr.c

@@ -74,14 +74,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_pmu *pmu = perf_pmus__find_core_pmu();
  char *cpuid = perf_pmu__getcpuid(pmu);
  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);

@@ -244,11 +242,13 @@ 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 defined(__i386__) && defined(__x86_64__)
  TEST_ASSERT_VAL("#system_tsc_freq", expr__parse(&val, ctx, "#system_tsc_freq") == 0);
-    if (is_intel)
+    if (strstr(cpuid, "Intel") != NULL)
      TEST_ASSERT_VAL("#system_tsc_freq > 0", val > 0);
  else
      TEST_ASSERT_VAL("#system_tsc_freq == 0", fpclassify(val) == FP_ZERO);
+#endif

  /*
   * Source count returns the number of events aggregating in a leader

I confirmed the change above fixes the failure on Arm64.

Tested-by: Leo Yan <leo.yan@arm.com>

Thanks Leo Yan for testing.

Hi Ian,

If the change above looks good, I will post a V2 . Please share your review comments

Sorry for the delay, it looks good to me.  Can you please send the v2?

Hi Namhyung

Thanks for checking on this.
I will test with the latest version sent by Ian and respond with the results soon

Thanks
Athira Rajeev

Thanks,
Namhyung

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help