[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 at linux.vnet.ibm.com> wrote:
>
>
>
> > On 30 Oct 2024, at 5:29 AM, Namhyung Kim <namhyung at 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 at 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
>
>