diff mbox series

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

Message ID 20240503234051.21217-1-jhubbard@nvidia.com
State New
Headers show
Series [v2] selftests/resctrl: fix clang build warnings related to abs(), labs() calls | expand

Commit Message

John Hubbard May 3, 2024, 11:40 p.m. UTC
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

Comments

Reinette Chatre May 7, 2024, 10:30 p.m. UTC | #1
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
John Hubbard May 8, 2024, 1:16 a.m. UTC | #2
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,
Reinette Chatre May 8, 2024, 1:21 a.m. UTC | #3
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
John Hubbard May 8, 2024, 1:25 a.m. UTC | #4
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,
Reinette Chatre May 8, 2024, 1:37 a.m. UTC | #5
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
Ilpo Järvinen May 8, 2024, 8 a.m. UTC | #6
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.
John Hubbard May 8, 2024, 5:29 p.m. UTC | #7
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 mbox series

Patch

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;