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

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

[John Hubbard]

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
prerequisite-patch-id: b901ece2a5b78503e2fb5480f20e304d36a0ea27
prerequisite-patch-id: 8d96c4b8c3ed6d9ea2588ef7f594ae0f9f83c279
-- 
2.45.0

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

[John Hubbard]

This was supposed to be a v3 patch. I'll resend as v3, sorry
about creating a confusing email here.

thanks,
-- 
John Hubbard
NVIDIA

On 5/8/24 12:02 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;
>   		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
> prerequisite-patch-id: b901ece2a5b78503e2fb5480f20e304d36a0ea27
> prerequisite-patch-id: 8d96c4b8c3ed6d9ea2588ef7f594ae0f9f83c279

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

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 3753 bytes --]

On Wed, 8 May 2024, 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.

In general, instead of filename, it's better to refer to the actual 
function in this kind of description. And in this particular case, 
cmt_test.c could be replaced with "the CMT selftest" which is more 
descriptive and still unambiguous.

> Cc: Reinette Chatre <reinette.chatre@intel.com>
> Cc: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>

Thank you.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

> ---
>  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
> prerequisite-patch-id: b901ece2a5b78503e2fb5480f20e304d36a0ea27
> prerequisite-patch-id: 8d96c4b8c3ed6d9ea2588ef7f594ae0f9f83c279
> 

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

[John Hubbard]

On 5/10/24 2:06 AM, Ilpo Järvinen wrote:
> On Wed, 8 May 2024, 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.
> 
> In general, instead of filename, it's better to refer to the actual
> function in this kind of description. And in this particular case,
> cmt_test.c could be replaced with "the CMT selftest" which is more
> descriptive and still unambiguous.

Sure, I'll change that.

> 
>> Cc: Reinette Chatre <reinette.chatre@intel.com>
>> Cc: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> 
> Thank you.
> 
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> 

Thanks for the review!


thanks,
-- 
John Hubbard
NVIDIA

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

[John Hubbard]

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) don't call labs() unnecessarily.

[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/

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..673b2bb800f7 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -77,7 +77,7 @@ static bool show_mba_info(unsigned long *bw_imc, unsigned long *bw_resc)
 
 		avg_bw_imc = sum_bw_imc / (NUM_OF_RUNS - 1);
 		avg_bw_resc = sum_bw_resc / (NUM_OF_RUNS - 1);
-		avg_diff = (float)labs(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
+		avg_diff = (float)(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
 		avg_diff_per = (int)(avg_diff * 100);
 
 		ksft_print_msg("%s Check MBA diff within %d%% for schemata %u\n",
diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
index d67ffa3ec63a..c873793d016d 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -33,7 +33,7 @@ show_bw_info(unsigned long *bw_imc, unsigned long *bw_resc, size_t span)
 
 	avg_bw_imc = sum_bw_imc / 4;
 	avg_bw_resc = sum_bw_resc / 4;
-	avg_diff = (float)labs(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
+	avg_diff = (float)(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
 	avg_diff_per = (int)(avg_diff * 100);
 
 	ret = avg_diff_per > MAX_DIFF_PERCENT;

base-commit: f03359bca01bf4372cf2c118cd9a987a5951b1c8
prerequisite-patch-id: b901ece2a5b78503e2fb5480f20e304d36a0ea27
prerequisite-patch-id: 8d96c4b8c3ed6d9ea2588ef7f594ae0f9f83c279
-- 
2.45.0

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

[Ilpo Järvinen]

On Thu, 2 May 2024, 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) don't call labs() unnecessarily.
> 
> [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/
> 
> 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);

This seems fine but...

> diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
> index 7946e32e85c8..673b2bb800f7 100644
> --- a/tools/testing/selftests/resctrl/mba_test.c
> +++ b/tools/testing/selftests/resctrl/mba_test.c
> @@ -77,7 +77,7 @@ static bool show_mba_info(unsigned long *bw_imc, unsigned long *bw_resc)
>  
>  		avg_bw_imc = sum_bw_imc / (NUM_OF_RUNS - 1);
>  		avg_bw_resc = sum_bw_resc / (NUM_OF_RUNS - 1);
> -		avg_diff = (float)labs(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
> +		avg_diff = (float)(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
>  		avg_diff_per = (int)(avg_diff * 100);
>  
>  		ksft_print_msg("%s Check MBA diff within %d%% for schemata %u\n",
> diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
> index d67ffa3ec63a..c873793d016d 100644
> --- a/tools/testing/selftests/resctrl/mbm_test.c
> +++ b/tools/testing/selftests/resctrl/mbm_test.c
> @@ -33,7 +33,7 @@ show_bw_info(unsigned long *bw_imc, unsigned long *bw_resc, size_t span)
>  
>  	avg_bw_imc = sum_bw_imc / 4;
>  	avg_bw_resc = sum_bw_resc / 4;
> -	avg_diff = (float)labs(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
> +	avg_diff = (float)(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
>  	avg_diff_per = (int)(avg_diff * 100);
>  
>  	ret = avg_diff_per > MAX_DIFF_PERCENT;

But how are these two cases same after your change when you ended up 
removing taking the absolute value entirely?


-- 
 i.

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

[John Hubbard]

On 5/3/24 1:00 AM, Ilpo Järvinen wrote:
> On Thu, 2 May 2024, John Hubbard wrote:
...
>> diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
>> index d67ffa3ec63a..c873793d016d 100644
>> --- a/tools/testing/selftests/resctrl/mbm_test.c
>> +++ b/tools/testing/selftests/resctrl/mbm_test.c
>> @@ -33,7 +33,7 @@ show_bw_info(unsigned long *bw_imc, unsigned long *bw_resc, size_t span)
>>   
>>   	avg_bw_imc = sum_bw_imc / 4;
>>   	avg_bw_resc = sum_bw_resc / 4;
>> -	avg_diff = (float)labs(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
>> +	avg_diff = (float)(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
>>   	avg_diff_per = (int)(avg_diff * 100);
>>   
>>   	ret = avg_diff_per > MAX_DIFF_PERCENT;
> 
> But how are these two cases same after your change when you ended up
> removing taking the absolute value entirely?

All of the arguments are unsigned integers, so all arithmetic results
are interpreted as unsigned, so taking the absolute value of that is
always a no-op.

thanks,
-- 
John Hubbard
NVIDIA

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

[Reinette Chatre]

On 5/3/2024 9:52 AM, John Hubbard wrote:
> On 5/3/24 1:00 AM, Ilpo Järvinen wrote:
>> On Thu, 2 May 2024, John Hubbard wrote:
> ...
>>> diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
>>> index d67ffa3ec63a..c873793d016d 100644
>>> --- a/tools/testing/selftests/resctrl/mbm_test.c
>>> +++ b/tools/testing/selftests/resctrl/mbm_test.c
>>> @@ -33,7 +33,7 @@ show_bw_info(unsigned long *bw_imc, unsigned long *bw_resc, size_t span)
>>>         avg_bw_imc = sum_bw_imc / 4;
>>>       avg_bw_resc = sum_bw_resc / 4;
>>> -    avg_diff = (float)labs(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
>>> +    avg_diff = (float)(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
>>>       avg_diff_per = (int)(avg_diff * 100);
>>>         ret = avg_diff_per > MAX_DIFF_PERCENT;
>>
>> But how are these two cases same after your change when you ended up
>> removing taking the absolute value entirely?
> 
> All of the arguments are unsigned integers, so all arithmetic results
> are interpreted as unsigned, so taking the absolute value of that is
> always a no-op.

It does not seem as though clang can see when values have been casted.
I tried to do so explicitly with a:
 	avg_diff = labs((long)avg_bw_resc - avg_bw_imc) / (float)avg_bw_imc;

But that still triggers:
warning: taking the absolute value of unsigned type 'unsigned long' has no effect [-Wabsolute-value]

Looks like we may need to be more explicit types and not rely on casting so much
to make the compiler happy.

Reinette

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

[John Hubbard]

On 5/3/24 11:37 AM, Reinette Chatre wrote:
> On 5/3/2024 9:52 AM, John Hubbard wrote:
>> On 5/3/24 1:00 AM, Ilpo Järvinen wrote:
>>> On Thu, 2 May 2024, John Hubbard wrote:
>> ...
>>>> diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
>>>> index d67ffa3ec63a..c873793d016d 100644
>>>> --- a/tools/testing/selftests/resctrl/mbm_test.c
>>>> +++ b/tools/testing/selftests/resctrl/mbm_test.c
>>>> @@ -33,7 +33,7 @@ show_bw_info(unsigned long *bw_imc, unsigned long *bw_resc, size_t span)
>>>>          avg_bw_imc = sum_bw_imc / 4;
>>>>        avg_bw_resc = sum_bw_resc / 4;
>>>> -    avg_diff = (float)labs(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
>>>> +    avg_diff = (float)(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
>>>>        avg_diff_per = (int)(avg_diff * 100);
>>>>          ret = avg_diff_per > MAX_DIFF_PERCENT;
>>>
>>> But how are these two cases same after your change when you ended up
>>> removing taking the absolute value entirely?
>>
>> All of the arguments are unsigned integers, so all arithmetic results
>> are interpreted as unsigned, so taking the absolute value of that is
>> always a no-op.
> 
> It does not seem as though clang can see when values have been casted.
> I tried to do so explicitly with a:
>   	avg_diff = labs((long)avg_bw_resc - avg_bw_imc) / (float)avg_bw_imc;

The subtraction result will get promoted to an unsigned long, before being
passed into labs(3).

> 
> But that still triggers:
> warning: taking the absolute value of unsigned type 'unsigned long' has no effect [-Wabsolute-value]

As expected, yes.

> 
> Looks like we may need to be more explicit types and not rely on casting so much
> to make the compiler happy.
> 

I assumed that this code did not expect to handle negative numbers,
because it is using unsigned math throughout.

If you do expect it to handle cases where, for example, this happens:

    avg_bw_imc > avg_bw_resc

...then a proper solution is easy, and looks like this:

diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
index c873793d016d..b87f91a41494 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;

Should I resend the patch with that approach?

thanks,
-- 
John Hubbard
NVIDIA

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

[Reinette Chatre]

Hi John,

On 5/3/2024 12:12 PM, John Hubbard wrote:
> On 5/3/24 11:37 AM, Reinette Chatre wrote:
>> On 5/3/2024 9:52 AM, John Hubbard wrote:
>>> On 5/3/24 1:00 AM, Ilpo Järvinen wrote:
>>>> On Thu, 2 May 2024, John Hubbard wrote:
>>> ...
>>>>> diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
>>>>> index d67ffa3ec63a..c873793d016d 100644
>>>>> --- a/tools/testing/selftests/resctrl/mbm_test.c
>>>>> +++ b/tools/testing/selftests/resctrl/mbm_test.c
>>>>> @@ -33,7 +33,7 @@ show_bw_info(unsigned long *bw_imc, unsigned long *bw_resc, size_t span)
>>>>>          avg_bw_imc = sum_bw_imc / 4;
>>>>>        avg_bw_resc = sum_bw_resc / 4;
>>>>> -    avg_diff = (float)labs(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
>>>>> +    avg_diff = (float)(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
>>>>>        avg_diff_per = (int)(avg_diff * 100);
>>>>>          ret = avg_diff_per > MAX_DIFF_PERCENT;
>>>>
>>>> But how are these two cases same after your change when you ended up
>>>> removing taking the absolute value entirely?
>>>
>>> All of the arguments are unsigned integers, so all arithmetic results
>>> are interpreted as unsigned, so taking the absolute value of that is
>>> always a no-op.
>>
>> It does not seem as though clang can see when values have been casted.
>> I tried to do so explicitly with a:
>>       avg_diff = labs((long)avg_bw_resc - avg_bw_imc) / (float)avg_bw_imc;
> 
> The subtraction result will get promoted to an unsigned long, before being
> passed into labs(3).
> 
>>
>> But that still triggers:
>> warning: taking the absolute value of unsigned type 'unsigned long' has no effect [-Wabsolute-value]
> 
> As expected, yes.
> 
>>
>> Looks like we may need to be more explicit types and not rely on casting so much
>> to make the compiler happy.
>>
> 
> I assumed that this code did not expect to handle negative numbers,
> because it is using unsigned math throughout.
> 
> If you do expect it to handle cases where, for example, this happens:
> 
>    avg_bw_imc > avg_bw_resc

The existing code seems to handle this ok. A sample program with this
scenario comparing existing computation with your first proposal is
below:

#include <stdio.h>
#include <stdlib.h>

void main(void) {
	unsigned long avg_bw_resc = 20000;
	unsigned long avg_bw_imc = 40000;
	float avg_diff;

	/* Existing code */
	avg_diff = (float)labs(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
	printf("Existing code: avg_diff = %f\n", avg_diff);

	/* Original proposed fix */
	avg_diff = (float)(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
	printf("Original proposed fix: avg_diff = %f\n", avg_diff);
}

output:
Existing code: avg_diff = 0.500000
Original proposed fix: avg_diff = 461168590192640.000000

> 
> ...then a proper solution is easy, and looks like this:
> 
> diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
> index c873793d016d..b87f91a41494 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;
> 
> Should I resend the patch with that approach?

ok. That indeed makes the computations easier to understand. I assume
you intend to fix the snippet in mba_test.c also?

Reinette

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

[John Hubbard]

On 5/3/24 1:46 PM, Reinette Chatre wrote:
> Hi John,
> On 5/3/2024 12:12 PM, John Hubbard wrote:
>> On 5/3/24 11:37 AM, Reinette Chatre wrote:
>>> On 5/3/2024 9:52 AM, John Hubbard wrote:
>>>> On 5/3/24 1:00 AM, Ilpo Järvinen wrote:
>>>>> On Thu, 2 May 2024, John Hubbard wrote:
>>>> ...
...
>> I assumed that this code did not expect to handle negative numbers,
>> because it is using unsigned math throughout.
>>
>> If you do expect it to handle cases where, for example, this happens:
>>
>>     avg_bw_imc > avg_bw_resc
> 
> The existing code seems to handle this ok. A sample program with this
> scenario comparing existing computation with your first proposal is
> below:
> 
> #include <stdio.h>
> #include <stdlib.h>
> 
> void main(void) {
> 	unsigned long avg_bw_resc = 20000;
> 	unsigned long avg_bw_imc = 40000;
> 	float avg_diff;
> 
> 	/* Existing code */
> 	avg_diff = (float)labs(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
> 	printf("Existing code: avg_diff = %f\n", avg_diff);
> 
> 	/* Original proposed fix */
> 	avg_diff = (float)(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
> 	printf("Original proposed fix: avg_diff = %f\n", avg_diff);
> }
> 
> output:
> Existing code: avg_diff = 0.500000
> Original proposed fix: avg_diff = 461168590192640.000000

That seems "a little bit" wrong. haha :)

> 
>>
>> ...then a proper solution is easy, and looks like this:
>>
>> diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
>> index c873793d016d..b87f91a41494 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;
>>
>> Should I resend the patch with that approach?
> 
> ok. That indeed makes the computations easier to understand. I assume
> you intend to fix the snippet in mba_test.c also?
> 

Yes, will do that. Thanks for spotting the bug in the original "fix"!

thanks,
-- 
John Hubbard
NVIDIA

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

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2772 bytes --]

On Fri, 3 May 2024, John Hubbard wrote:

> On 5/3/24 11:37 AM, Reinette Chatre wrote:
> > On 5/3/2024 9:52 AM, John Hubbard wrote:
> > > On 5/3/24 1:00 AM, Ilpo Järvinen wrote:
> > > > On Thu, 2 May 2024, John Hubbard wrote:
> > > ...
> > > > > diff --git a/tools/testing/selftests/resctrl/mbm_test.c
> > > > > b/tools/testing/selftests/resctrl/mbm_test.c
> > > > > index d67ffa3ec63a..c873793d016d 100644
> > > > > --- a/tools/testing/selftests/resctrl/mbm_test.c
> > > > > +++ b/tools/testing/selftests/resctrl/mbm_test.c
> > > > > @@ -33,7 +33,7 @@ show_bw_info(unsigned long *bw_imc, unsigned long
> > > > > *bw_resc, size_t span)
> > > > >          avg_bw_imc = sum_bw_imc / 4;
> > > > >        avg_bw_resc = sum_bw_resc / 4;
> > > > > -    avg_diff = (float)labs(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
> > > > > +    avg_diff = (float)(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
> > > > >        avg_diff_per = (int)(avg_diff * 100);
> > > > >          ret = avg_diff_per > MAX_DIFF_PERCENT;
> > > > 
> > > > But how are these two cases same after your change when you ended up
> > > > removing taking the absolute value entirely?
> > > 
> > > All of the arguments are unsigned integers, so all arithmetic results
> > > are interpreted as unsigned, so taking the absolute value of that is
> > > always a no-op.

(I see there's a better patch posted already but since there are a few 
incorrect claims in this discussion, I'll do for the record type of 
reply.)

This discussion now went to a tangent about the warning. My main point is 
that logic is not correct after removing labs().

I also disagree with the claim that using labs() on unsigned value is 
no-op because labs() takes long so unsigned is just forced into signed 
when calling which is why the warning triggers but it's very misleading 
warning (see below).

> > It does not seem as though clang can see when values have been casted.
> > I tried to do so explicitly with a:
> >   	avg_diff = labs((long)avg_bw_resc - avg_bw_imc) / (float)avg_bw_imc;
>
> The subtraction result will get promoted to an unsigned long, before being
> passed into labs(3).
>
> > But that still triggers:
> > warning: taking the absolute value of unsigned type 'unsigned long' has no
> > effect [-Wabsolute-value]
> 
> As expected, yes.

That error message isn't factually correct:

        unsigned long a = LONG_MAX;
        long b;

        a += 2;
        b = (long)a;
        printf("%llu %lli %lli\n", a, b, labs(a));

Prints (at least when built with gcc):

9223372036854775809 -9223372036854775807 9223372036854775807

labs(LONG_MAX + 1) won't work though since it's not positively presentable 
with long and the value is left untouched.

-- 
 i.

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

[John Hubbard]

On 5/6/24 2:07 AM, Ilpo Järvinen wrote:
> On Fri, 3 May 2024, John Hubbard wrote:
> 
>> On 5/3/24 11:37 AM, Reinette Chatre wrote:
>>> On 5/3/2024 9:52 AM, John Hubbard wrote:
>>>> On 5/3/24 1:00 AM, Ilpo Järvinen wrote:
>>>>> On Thu, 2 May 2024, John Hubbard wrote:
>>>> ...
>>>>>> diff --git a/tools/testing/selftests/resctrl/mbm_test.c
>>>>>> b/tools/testing/selftests/resctrl/mbm_test.c
>>>>>> index d67ffa3ec63a..c873793d016d 100644
>>>>>> --- a/tools/testing/selftests/resctrl/mbm_test.c
>>>>>> +++ b/tools/testing/selftests/resctrl/mbm_test.c
>>>>>> @@ -33,7 +33,7 @@ show_bw_info(unsigned long *bw_imc, unsigned long
>>>>>> *bw_resc, size_t span)
>>>>>>           avg_bw_imc = sum_bw_imc / 4;
>>>>>>         avg_bw_resc = sum_bw_resc / 4;
>>>>>> -    avg_diff = (float)labs(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
>>>>>> +    avg_diff = (float)(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
>>>>>>         avg_diff_per = (int)(avg_diff * 100);
>>>>>>           ret = avg_diff_per > MAX_DIFF_PERCENT;
>>>>>
>>>>> But how are these two cases same after your change when you ended up
>>>>> removing taking the absolute value entirely?
>>>>
>>>> All of the arguments are unsigned integers, so all arithmetic results
>>>> are interpreted as unsigned, so taking the absolute value of that is
>>>> always a no-op.
> 
> (I see there's a better patch posted already but since there are a few
> incorrect claims in this discussion, I'll do for the record type of
> reply.)
> 
> This discussion now went to a tangent about the warning. My main point is
> that logic is not correct after removing labs().
> 
> I also disagree with the claim that using labs() on unsigned value is
> no-op because labs() takes long so unsigned is just forced into signed
> when calling which is why the warning triggers but it's very misleading
> warning (see below).
>

Yes you are correct.

>>> It does not seem as though clang can see when values have been casted.
>>> I tried to do so explicitly with a:
>>>    	avg_diff = labs((long)avg_bw_resc - avg_bw_imc) / (float)avg_bw_imc;
>>
>> The subtraction result will get promoted to an unsigned long, before being
>> passed into labs(3).
>>
>>> But that still triggers:
>>> warning: taking the absolute value of unsigned type 'unsigned long' has no
>>> effect [-Wabsolute-value]
>>
>> As expected, yes.
> 
> That error message isn't factually correct:
> 
>          unsigned long a = LONG_MAX;
>          long b;
> 
>          a += 2;
>          b = (long)a;
>          printf("%llu %lli %lli\n", a, b, labs(a));
> 
> Prints (at least when built with gcc):
> 
> 9223372036854775809 -9223372036854775807 9223372036854775807
> 
> labs(LONG_MAX + 1) won't work though since it's not positively presentable
> with long and the value is left untouched.
> 

Thanks for setting the detailed record straight! :)


thanks,
-- 
John Hubbard
NVIDIA