Messages in this thread | | | Date | Tue, 30 Nov 2021 11:41:35 -0300 | From | Arnaldo Carvalho de Melo <> | Subject | Re: [PATCH v2] tools/perf: Fix perf test 7 Simple expression parser |
| |
Em Mon, Nov 29, 2021 at 12:23:39PM +0100, Thomas Richter escreveu: > Some platforms do not have CPU die support, for example s390. > Commit fdf1e29b6118 ("perf expr: Add metric literals for topology.") > fails on s390: > # ./perf test -Fv 7 > ... > # FAILED tests/expr.c:173 #num_dies >= #num_packages > ---- end ---- > Simple expression parser: FAILED! > # > > Investigating this issue leads to these functions: > build_cpu_topology() > +--> has_die_topology(void) > { > struct utsname uts; > > if (uname(&uts) < 0) > return false; > if (strncmp(uts.machine, "x86_64", 6)) > return false; > .... > } > > which always returns false on s390. The caller build_cpu_topology() > checks has_die_topology() return value. On false the > the struct cpu_topology::die_cpu_list is not contructed and has zero > entries. This leads to the failing comparison: #num_dies >= #num_packages. > s390 of course has a positive number of packages. > > Fix this and check if the function build_cpu_topology() did build up > a die_cpus_list. The number of entries in this list should be larger > than 0. If the number of list element is zero, the die_cpus_list has > not been created and the check in function test__expr() > TEST_ASSERT_VAL("#num_dies >= #num_packages", \ > num_dies >= num_packages) > always fails. > > Output after: > # ./perf test -Fv 7 > 7: Simple expression parser : > --- start --- > division by zero > syntax error > ---- end ---- > Simple expression parser: Ok > # > Cc: Ian Rogers <irogers@google.com> > Fixes: fdf1e29b6118 ("perf expr: Add metric literals for topology.") > > Signed-off-by: Thomas Richter <tmricht@linux.ibm.com> > Acked-by: Ian Rogers <irogers@google.com> > --- > tools/perf/tests/expr.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c > index c895de481fe1..9356d90bf8db 100644 > --- a/tools/perf/tests/expr.c > +++ b/tools/perf/tests/expr.c > @@ -169,7 +169,8 @@ static int test__expr(struct test_suite *t __maybe_unused, int subtest __maybe_u > TEST_ASSERT_VAL("#num_dies", expr__parse(&num_dies, ctx, "#num_dies") == 0); > TEST_ASSERT_VAL("#num_cores >= #num_dies", num_cores >= num_dies); > TEST_ASSERT_VAL("#num_packages", expr__parse(&num_packages, ctx, "#num_packages") == 0); > - TEST_ASSERT_VAL("#num_dies >= #num_packages", num_dies >= num_packages); > + if (num_dies) > + TEST_ASSERT_VAL("#num_dies >= #num_packages", num_dies >= num_packages); > > /* > * Source count returns the number of events aggregating in a leader > --
I'll add a comment for the casual reader:
+ 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);
| |