Re: [PATCH] selftests/resctrl: fix clang build warnings related to abs(), labs() calls
From: John Hubbard
Date: Fri May 03 2024 - 18:01:15 EST
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