[v3] selftests/resctrl: fix clang build warnings related to abs(), labs() calls

[PATCH v3] selftests/resctrl: fix clang build warnings related to abs(), labs() calls

Posted by John Hubbard 1 week, 4 days ago

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).

c) Remove a duplicate abs() call in cmt_test.c.

Cc: Reinette Chatre <reinette.chatre@intel.com>
Cc: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 tools/testing/selftests/resctrl/cmt_test.c | 4 ++--
 tools/testing/selftests/resctrl/mba_test.c | 2 +-
 tools/testing/selftests/resctrl/mbm_test.c | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

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);
diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
index 7946e32e85c8..5fffbc9ff6a4 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -60,7 +60,7 @@ 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;
+		long avg_bw_imc, avg_bw_resc;
 		unsigned 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..a4c3ea49b0e8 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -17,7 +17,7 @@
 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;
+	long avg_bw_imc = 0, avg_bw_resc = 0;
 	unsigned long sum_bw_imc = 0, sum_bw_resc = 0;
 	int runs, ret, avg_diff_per;
 	float avg_diff = 0;

base-commit: 45db3ab70092637967967bfd8e6144017638563c
-- 
2.45.0

Re: [PATCH v3] selftests/resctrl: fix clang build warnings related to abs(), labs() calls

Posted by Reinette Chatre 1 week, 4 days ago

Hi John,

On 5/8/2024 12:41 PM, John Hubbard wrote:
> 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).
> 
> c) Remove a duplicate abs() call in cmt_test.c.
> 
> Cc: Reinette Chatre <reinette.chatre@intel.com>
> Cc: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
>  tools/testing/selftests/resctrl/cmt_test.c | 4 ++--
>  tools/testing/selftests/resctrl/mba_test.c | 2 +-
>  tools/testing/selftests/resctrl/mbm_test.c | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> 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);
> diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
> index 7946e32e85c8..5fffbc9ff6a4 100644
> --- a/tools/testing/selftests/resctrl/mba_test.c
> +++ b/tools/testing/selftests/resctrl/mba_test.c
> @@ -60,7 +60,7 @@ 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;
> +		long avg_bw_imc, avg_bw_resc;
>  		unsigned long sum_bw_imc = 0, sum_bw_resc = 0;

nit: since you are touching this line, could you please move it down one line to
establish reverse fir order?

>  		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..a4c3ea49b0e8 100644
> --- a/tools/testing/selftests/resctrl/mbm_test.c
> +++ b/tools/testing/selftests/resctrl/mbm_test.c
> @@ -17,7 +17,7 @@
>  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;
> +	long avg_bw_imc = 0, avg_bw_resc = 0;
>  	unsigned long sum_bw_imc = 0, sum_bw_resc = 0;
>  	int runs, ret, avg_diff_per;
>  	float avg_diff = 0;

Same nit here about reverse fir order.

Thank you very much for sticking with this to fix the warnings.
With the nitpicks addressed you can add:
| Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette

Re: [PATCH v3] selftests/resctrl: fix clang build warnings related to abs(), labs() calls

Posted by John Hubbard 1 week, 4 days ago

On 5/8/24 1:24 PM, Reinette Chatre wrote:
> Hi John,
> 
> On 5/8/2024 12:41 PM, John Hubbard wrote:
...
>> diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
>> index 7946e32e85c8..5fffbc9ff6a4 100644
>> --- a/tools/testing/selftests/resctrl/mba_test.c
>> +++ b/tools/testing/selftests/resctrl/mba_test.c
>> @@ -60,7 +60,7 @@ 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;
>> +		long avg_bw_imc, avg_bw_resc;
>>   		unsigned long sum_bw_imc = 0, sum_bw_resc = 0;
> 
> nit: since you are touching this line, could you please move it down one line to
> establish reverse fir order?
> 
>>   		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..a4c3ea49b0e8 100644
>> --- a/tools/testing/selftests/resctrl/mbm_test.c
>> +++ b/tools/testing/selftests/resctrl/mbm_test.c
>> @@ -17,7 +17,7 @@
>>   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;
>> +	long avg_bw_imc = 0, avg_bw_resc = 0;
>>   	unsigned long sum_bw_imc = 0, sum_bw_resc = 0;
>>   	int runs, ret, avg_diff_per;
>>   	float avg_diff = 0;
> 
> Same nit here about reverse fir order.
> 
> Thank you very much for sticking with this to fix the warnings.
> With the nitpicks addressed you can add:
> | Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
> 

Sure, I'll post another one with those two changes and your rb,
thanks for the review(s)!

thanks,
-- 
John Hubbard
NVIDIA