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

[John Hubbard <jhubbard@nvidia.com>]

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,
-- 
John Hubbard
NVIDIA