Hi John, On 5/3/2024 4:40 PM, John Hubbard wrote: > First of all, in order to build with clang at all, one must first apply > Valentin Obst's build fix for LLVM [1]. Furthermore, for this particular > resctrl directory, my pending fix [2] must also be applied. Once those > fixes are in place, then when building with clang, via: > > make LLVM=1 -C tools/testing/selftests > > ...two types of warnings occur: > > warning: absolute value function 'abs' given an argument of type > 'long' but has parameter of type 'int' which may cause truncation of > value > > warning: taking the absolute value of unsigned type 'unsigned long' > has no effect > > Fix these by: > > a) using labs() in place of abs(), when long integers are involved, and > > b) Change to use signed integer data types, in places where subtraction > is used (and could end up with negative values). > > [1] https://lore.kernel.org/all/20240329-selftests-libmk-llvm-rfc-v1-1-2f9ed7d1c49f@xxxxxxxxxxxxxxx/ > [2] https://lore.kernel.org/all/20240503021712.78601-1-jhubbard@xxxxxxxxxx/ > > Cc: Reinette Chatre <reinette.chatre@xxxxxxxxx> > Signed-off-by: John Hubbard <jhubbard@xxxxxxxxxx> > --- > > Hi Reinette, > > This v2 includes a fix for the bugs that you pointed out (thanks!) in v1. > > I kept the changes to signed integers minimal: only what is required in > order to get a clean clang build. > > thanks, > John Hubbard > > tools/testing/selftests/resctrl/cmt_test.c | 12 ++++++------ > tools/testing/selftests/resctrl/mba_test.c | 4 ++-- > tools/testing/selftests/resctrl/mbm_test.c | 4 ++-- > 3 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c > index a81f91222a89..af33abd1cca7 100644 > --- a/tools/testing/selftests/resctrl/cmt_test.c > +++ b/tools/testing/selftests/resctrl/cmt_test.c > @@ -29,22 +29,22 @@ static int cmt_setup(const struct resctrl_test *test, > return 0; > } > > -static int show_results_info(unsigned long sum_llc_val, int no_of_bits, > - unsigned long cache_span, unsigned long max_diff, > - unsigned long max_diff_percent, unsigned long num_of_runs, > +static int show_results_info(long sum_llc_val, int no_of_bits, > + long cache_span, long max_diff, > + long max_diff_percent, long num_of_runs, > bool platform) > { > - unsigned long avg_llc_val = 0; > + long avg_llc_val = 0; > float diff_percent; > long avg_diff = 0; > int ret; > > avg_llc_val = sum_llc_val / num_of_runs; > - avg_diff = (long)abs(cache_span - avg_llc_val); > + avg_diff = labs(cache_span - avg_llc_val); > diff_percent = ((float)cache_span - avg_llc_val) / cache_span * 100; > > ret = platform && abs((int)diff_percent) > max_diff_percent && > - abs(avg_diff) > max_diff; > + labs(avg_diff) > max_diff; > > ksft_print_msg("%s Check cache miss rate within %lu%%\n", > ret ? "Fail:" : "Pass:", max_diff_percent); The changes in this hunk are unexpected. The changes to this area made by previous version was ok, no? It really seems like this just does a brute force of everything to long (while taking labs() twice) unnecessarily. > diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c > index 7946e32e85c8..707b07687249 100644 > --- a/tools/testing/selftests/resctrl/mba_test.c > +++ b/tools/testing/selftests/resctrl/mba_test.c > @@ -60,8 +60,8 @@ static bool show_mba_info(unsigned long *bw_imc, unsigned long *bw_resc) > /* Memory bandwidth from 100% down to 10% */ > for (allocation = 0; allocation < ALLOCATION_MAX / ALLOCATION_STEP; > allocation++) { > - unsigned long avg_bw_imc, avg_bw_resc; > - unsigned long sum_bw_imc = 0, sum_bw_resc = 0; > + long avg_bw_imc, avg_bw_resc; > + long sum_bw_imc = 0, sum_bw_resc = 0; > int avg_diff_per; > float avg_diff; > On second look it only seems necessary to change avg_bw_imc and avg_bw_resc. What do you think? > diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c > index d67ffa3ec63a..30af15020731 100644 > --- a/tools/testing/selftests/resctrl/mbm_test.c > +++ b/tools/testing/selftests/resctrl/mbm_test.c > @@ -17,8 +17,8 @@ > static int > show_bw_info(unsigned long *bw_imc, unsigned long *bw_resc, size_t span) > { > - unsigned long avg_bw_imc = 0, avg_bw_resc = 0; > - unsigned long sum_bw_imc = 0, sum_bw_resc = 0; > + long avg_bw_imc = 0, avg_bw_resc = 0; > + long sum_bw_imc = 0, sum_bw_resc = 0; > int runs, ret, avg_diff_per; > float avg_diff = 0; > Same here wrt the avg_ variables. > > base-commit: ddb4c3f25b7b95df3d6932db0b379d768a6ebdf7 > prerequisite-patch-id: b901ece2a5b78503e2fb5480f20e304d36a0ea27 > prerequisite-patch-id: 8d96c4b8c3ed6d9ea2588ef7f594ae0f9f83c279 Reinette
