| Message ID | 20240503234051.21217-1-jhubbard@nvidia.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] selftests/resctrl: fix clang build warnings related to abs(), labs() calls | expand |
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@valentinobst.de/ > [2] https://lore.kernel.org/all/20240503021712.78601-1-jhubbard@nvidia.com/ > > Cc: Reinette Chatre <reinette.chatre@intel.com> > Signed-off-by: John Hubbard <jhubbard@nvidia.com> > --- > > 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
On 5/7/24 3:30 PM, Reinette Chatre wrote: ... >> 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 Well, not entirely. That first version was when I still believed clang's claim that abs()/labs() was a no-op. I've since been corrected! :) > to long (while taking labs() twice) unnecessarily. Which part exactly is unnecessary? Are you looking at the function args? Or something else? I've stared at it too much and am not spotting the issue yet. > >> 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? Yes, that works! I'll change it. > > >> 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. Also yes here, I'll change that too. thanks,
Hi John, On 5/7/2024 6:16 PM, John Hubbard wrote: > On 5/7/24 3:30 PM, Reinette Chatre wrote: > ... >>> 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 > > Well, not entirely. That first version was when I still believed clang's > claim that abs()/labs() was a no-op. I've since been corrected! :) > >> to long (while taking labs() twice) unnecessarily. > > Which part exactly is unnecessary? Are you looking at the function args? > Or something else? I've stared at it too much and am not spotting the > issue yet. > The following (what was in v1) looks good to me. What am I missing? diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c index a81f91222a89..05a241519ae8 100644 --- a/tools/testing/selftests/resctrl/cmt_test.c +++ b/tools/testing/selftests/resctrl/cmt_test.c @@ -40,11 +40,11 @@ static int show_results_info(unsigned long sum_llc_val, int no_of_bits, int ret; avg_llc_val = sum_llc_val / num_of_runs; - avg_diff = (long)abs(cache_span - avg_llc_val); + avg_diff = (long)(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); Reinette
On 5/7/24 6:21 PM, Reinette Chatre wrote: > Hi John, ... > > The following (what was in v1) looks good to me. What am I missing? > > diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c > index a81f91222a89..05a241519ae8 100644 > --- a/tools/testing/selftests/resctrl/cmt_test.c > +++ b/tools/testing/selftests/resctrl/cmt_test.c > @@ -40,11 +40,11 @@ static int show_results_info(unsigned long sum_llc_val, int no_of_bits, > int ret; > > avg_llc_val = sum_llc_val / num_of_runs; > - avg_diff = (long)abs(cache_span - avg_llc_val); > + avg_diff = (long)(cache_span - avg_llc_val); This deletes the abs() call, because I incorrectly let clang's warning lead me to believe that the abs() call was a no-op. But both you and Ilpo pointed out that the math breaks if you do that. > 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; This hunk is OK. > > ksft_print_msg("%s Check cache miss rate within %lu%%\n", > ret ? "Fail:" : "Pass:", max_diff_percent); > > Reinette thanks,
Hi John, On 5/7/2024 6:25 PM, John Hubbard wrote: > On 5/7/24 6:21 PM, Reinette Chatre wrote: >> Hi John, > ... >> >> The following (what was in v1) looks good to me. What am I missing? >> >> diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c >> index a81f91222a89..05a241519ae8 100644 >> --- a/tools/testing/selftests/resctrl/cmt_test.c >> +++ b/tools/testing/selftests/resctrl/cmt_test.c >> @@ -40,11 +40,11 @@ static int show_results_info(unsigned long sum_llc_val, int no_of_bits, >> int ret; >> avg_llc_val = sum_llc_val / num_of_runs; >> - avg_diff = (long)abs(cache_span - avg_llc_val); >> + avg_diff = (long)(cache_span - avg_llc_val); > > This deletes the abs() call, because I incorrectly let clang's warning > lead me to believe that the abs() call was a no-op. But both you and Ilpo > pointed out that the math breaks if you do that. To me the extra abs() was unnecessary anyway ... > >> 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; > ... because it is repeated here. > This hunk is OK. > >> ksft_print_msg("%s Check cache miss rate within %lu%%\n", >> ret ? "Fail:" : "Pass:", max_diff_percent); >> >> Reinette > > thanks, Reinette
On Tue, 7 May 2024, Reinette Chatre wrote: > Hi John, > > On 5/7/2024 6:25 PM, John Hubbard wrote: > > On 5/7/24 6:21 PM, Reinette Chatre wrote: > >> Hi John, > > ... > >> > >> The following (what was in v1) looks good to me. What am I missing? > >> > >> diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c > >> index a81f91222a89..05a241519ae8 100644 > >> --- a/tools/testing/selftests/resctrl/cmt_test.c > >> +++ b/tools/testing/selftests/resctrl/cmt_test.c > >> @@ -40,11 +40,11 @@ static int show_results_info(unsigned long sum_llc_val, int no_of_bits, > >> int ret; > >> avg_llc_val = sum_llc_val / num_of_runs; > >> - avg_diff = (long)abs(cache_span - avg_llc_val); > >> + avg_diff = (long)(cache_span - avg_llc_val); > > > > This deletes the abs() call, because I incorrectly let clang's warning > > lead me to believe that the abs() call was a no-op. But both you and Ilpo > > pointed out that the math breaks if you do that. > > To me the extra abs() was unnecessary anyway ... > > > > >> 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; > > > > .. because it is repeated here. Yes, there are two *abs() calls in this function. In this case is okay to remove the first one since it didn't remove absolute value completely, whereas in the MBA/MBM cases v1 removed *abs() call entirely which was wrong thing to do. I explicitly noted in my v1 comment that this CMT change is okay but the other two were not.
On 5/8/24 1:00 AM, Ilpo Järvinen wrote: > On Tue, 7 May 2024, Reinette Chatre wrote: > >> Hi John, >> >> On 5/7/2024 6:25 PM, John Hubbard wrote: >>> On 5/7/24 6:21 PM, Reinette Chatre wrote: >>>> Hi John, >>> ... > Yes, there are two *abs() calls in this function. > > In this case is okay to remove the first one since it didn't remove > absolute value completely, whereas in the MBA/MBM cases v1 removed *abs() > call entirely which was wrong thing to do. > > I explicitly noted in my v1 comment that this CMT change is okay but the > other two were not. > Got it, thanks Ilpo (and Reinette), I'll send a v3 with it fixed up accordingly, appreciate your patience here. :) thanks,
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); 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; 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;
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@valentinobst.de/ [2] https://lore.kernel.org/all/20240503021712.78601-1-jhubbard@nvidia.com/ Cc: Reinette Chatre <reinette.chatre@intel.com> Signed-off-by: John Hubbard <jhubbard@nvidia.com> --- 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(-) base-commit: ddb4c3f25b7b95df3d6932db0b379d768a6ebdf7 prerequisite-patch-id: b901ece2a5b78503e2fb5480f20e304d36a0ea27 prerequisite-patch-id: 8d96c4b8c3ed6d9ea2588ef7f594ae0f9f83c279