| Message ID | 20241022140156.98854-1-atrajeev@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tools/perf/tests/expr: Make the system_tsc_freq test only for intel | expand |
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 >
> 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
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 > >
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
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(-)