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

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

[Athira Rajeev]

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 <atrajeev@linux.vnet.ibm.com>
---
 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

[Namhyung Kim]

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

> 
> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> ---
>  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

[Athira Rajeev]

> On 30 Oct 2024, at 5:29 AM, Namhyung Kim <namhyung@kernel.org> wrote:
> 
> 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
> 

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

>> 
>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
>> ---
>> 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

[Ian Rogers]

On Sun, Nov 3, 2024 at 8:17 PM Athira Rajeev
<atrajeev@linux.vnet.ibm.com> wrote:
>
>
>
> > On 30 Oct 2024, at 5:29 AM, Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > 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
> >
>
> 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
>
> >>
> >> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> >> ---
> >> 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

[Athira Rajeev]

> On 5 Nov 2024, at 2:14 AM, Ian Rogers <irogers@google.com> wrote:
> 
> On Sun, Nov 3, 2024 at 8:17 PM Athira Rajeev
> <atrajeev@linux.vnet.ibm.com> wrote:
>> 
>> 
>> 
>>> On 30 Oct 2024, at 5:29 AM, Namhyung Kim <namhyung@kernel.org> wrote:
>>> 
>>> 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
>>> 
>> 
>> 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
> 
>> Thanks
>> Athira
>> 
>>>> 
>>>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
>>>> ---
>>>> 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

[Leo Yan]

Hi Athira,

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

[...]

> > 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

[Athira Rajeev]

> On 7 Nov 2024, at 7:26 PM, Leo Yan <leo.yan@arm.com> wrote:
> 
> Hi Athira,
> 
> On Wed, Nov 06, 2024 at 03:04:57PM +0530, Athira Rajeev wrote:
> 
> [...]
> 
>>> 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

[Namhyung Kim]

Hello,

On Fri, Nov 08, 2024 at 10:50:10AM +0530, Athira Rajeev wrote:
> 
> 
> > On 7 Nov 2024, at 7:26 PM, Leo Yan <leo.yan@arm.com> wrote:
> > 
> > Hi Athira,
> > 
> > On Wed, Nov 06, 2024 at 03:04:57PM +0530, Athira Rajeev wrote:
> > 
> > [...]
> > 
> >>> 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

[Namhyung Kim]

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:
> > 
> > 
> > > On 7 Nov 2024, at 7:26 PM, Leo Yan <leo.yan@arm.com> wrote:
> > > 
> > > Hi Athira,
> > > 
> > > On Wed, Nov 06, 2024 at 03:04:57PM +0530, Athira Rajeev wrote:
> > > 
> > > [...]
> > > 
> > >>> 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

[Namhyung Kim]

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:
> > Hello,
> > 
> > On Fri, Nov 08, 2024 at 10:50:10AM +0530, Athira Rajeev wrote:
> > > 
> > > 
> > > > On 7 Nov 2024, at 7:26 PM, Leo Yan <leo.yan@arm.com> wrote:
> > > > 
> > > > Hi Athira,
> > > > 
> > > > On Wed, Nov 06, 2024 at 03:04:57PM +0530, Athira Rajeev wrote:
> > > > 
> > > > [...]
> > > > 
> > > >>> 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

[Athira Rajeev]

> On 3 Dec 2024, at 11:46 PM, Namhyung Kim <namhyung@kernel.org> wrote:
> 
> Hello,
> 
> On Fri, Nov 08, 2024 at 10:50:10AM +0530, Athira Rajeev wrote:
>> 
>> 
>>> On 7 Nov 2024, at 7:26 PM, Leo Yan <leo.yan@arm.com> wrote:
>>> 
>>> Hi Athira,
>>> 
>>> On Wed, Nov 06, 2024 at 03:04:57PM +0530, Athira Rajeev wrote:
>>> 
>>> [...]
>>> 
>>>>> 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