Dan reported that he noticed following static checker warnings
tools/testing/selftests/resctrl/resctrl_val.c:545 measure_vals() warn: 'bw_imc' unsigned <= 0
tools/testing/selftests/resctrl/resctrl_val.c:549 measure_vals() warn: 'bw_resc_end' unsigned <= 0
These warnings are reported because 1. measure_vals() declares 'bw_imc' and 'bw_resc_end' as unsigned long variables 2. Return value of get_mem_bw_imc() and get_mem_bw_resctrl() are assigned to 'bw_imc' and 'bw_resc_end' respectively 3. The return values are checked for <=0 to see if the calls were successful or not
Checking for < 0 for an unsigned value doesn't make any sense. So, to fix this issue, change the implementation of get_mem_bw_imc() and get_mem_bw_resctrl() such that they now accept reference to a variable and set the variable appropriately upon success and return 0, else return <0 on error.
Fixes: 7f4d257e3a2a: "selftests/resctrl: Add callback to start a benchmark" Reported-by: Dan Carpenter dan.carpenter@oracle.com Signed-off-by: Sai Praneeth Prakhya sai.praneeth.prakhya@intel.com --- tools/testing/selftests/resctrl/resctrl_val.c | 33 ++++++++++--------- 1 file changed, 17 insertions(+), 16 deletions(-)
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c index 94f3a34e21bb..003457b16ce7 100644 --- a/tools/testing/selftests/resctrl/resctrl_val.c +++ b/tools/testing/selftests/resctrl/resctrl_val.c @@ -300,9 +300,9 @@ static int initialize_mem_bw_imc(void) * Memory B/W utilized by a process on a socket can be calculated using * iMC counters. Perf events are used to read these counters. * - * Return: >= 0 on success. < 0 on failure. + * Return: = 0 on success. < 0 on failure. */ -static float get_mem_bw_imc(int cpu_no, char *bw_report) +static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc) { float reads, writes, of_mul_read, of_mul_write; int imc, j, ret; @@ -374,12 +374,13 @@ static float get_mem_bw_imc(int cpu_no, char *bw_report) }
if (strcmp(bw_report, "reads") == 0) - return reads; + *bw_imc = reads;
if (strcmp(bw_report, "writes") == 0) - return writes; + *bw_imc = writes;
- return (reads + writes); + *bw_imc = reads + writes; + return 0; }
void set_mbm_path(const char *ctrlgrp, const char *mongrp, int resource_id) @@ -438,9 +439,8 @@ static void initialize_mem_bw_resctrl(const char *ctrlgrp, const char *mongrp, * 1. If con_mon grp is given, then read from it * 2. If con_mon grp is not given, then read from root con_mon grp */ -static unsigned long get_mem_bw_resctrl(void) +static int get_mem_bw_resctrl(unsigned long *mbm_total) { - unsigned long mbm_total = 0; FILE *fp;
fp = fopen(mbm_total_path, "r"); @@ -449,7 +449,7 @@ static unsigned long get_mem_bw_resctrl(void)
return -1; } - if (fscanf(fp, "%lu", &mbm_total) <= 0) { + if (fscanf(fp, "%lu", mbm_total) <= 0) { perror("Could not get mbm local bytes"); fclose(fp);
@@ -457,7 +457,7 @@ static unsigned long get_mem_bw_resctrl(void) } fclose(fp);
- return mbm_total; + return 0; }
pid_t bm_pid, ppid; @@ -549,7 +549,8 @@ static void initialize_llc_occu_resctrl(const char *ctrlgrp, const char *mongrp, static int measure_vals(struct resctrl_val_param *param, unsigned long *bw_resc_start) { - unsigned long bw_imc, bw_resc, bw_resc_end; + unsigned long bw_resc, bw_resc_end; + float bw_imc; int ret;
/* @@ -559,13 +560,13 @@ measure_vals(struct resctrl_val_param *param, unsigned long *bw_resc_start) * Compare the two values to validate resctrl value. * It takes 1sec to measure the data. */ - bw_imc = get_mem_bw_imc(param->cpu_no, param->bw_report); - if (bw_imc <= 0) - return bw_imc; + ret = get_mem_bw_imc(param->cpu_no, param->bw_report, &bw_imc); + if (ret < 0) + return ret;
- bw_resc_end = get_mem_bw_resctrl(); - if (bw_resc_end <= 0) - return bw_resc_end; + ret = get_mem_bw_resctrl(&bw_resc_end); + if (ret < 0) + return ret;
bw_resc = (bw_resc_end - *bw_resc_start) / MB; ret = print_results_bw(param->filename, bm_pid, bw_imc, bw_resc);