[bug report] selftests/resctrl: Add callback to start a benchmark

[bug report] selftests/resctrl: Add callback to start a benchmark

[Dan Carpenter]

Hello Sai Praneeth Prakhya,

The patch 7f4d257e3a2a: "selftests/resctrl: Add callback to start a
benchmark" from Jan 16, 2020, leads to the following static checker
warning:

	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

tools/testing/selftests/resctrl/resctrl_val.c
   531  static int
   532  measure_vals(struct resctrl_val_param *param, unsigned long *bw_resc_start)
   533  {
   534          unsigned long bw_imc, bw_resc, bw_resc_end;
   535          int ret;
   536  
   537          /*
   538           * Measure memory bandwidth from resctrl and from
   539           * another source which is perf imc value or could
   540           * be something else if perf imc event is not available.
   541           * Compare the two values to validate resctrl value.
   542           * It takes 1sec to measure the data.
   543           */
   544          bw_imc = get_mem_bw_imc(param->cpu_no, param->bw_report);
   545          if (bw_imc <= 0)
                    ^^^^^^^^^^^
Unsigned.  Also the comments for get_mem_bw_imc() says that zero is
success.

   546                  return bw_imc;
   547  
   548          bw_resc_end = get_mem_bw_resctrl();
   549          if (bw_resc_end <= 0)
                    ^^^^^^^^^^^^^^^^
Unsigned

   550                  return bw_resc_end;
   551  
   552          bw_resc = (bw_resc_end - *bw_resc_start) / MB;
   553          ret = print_results_bw(param->filename, bm_pid, bw_imc, bw_resc);
   554          if (ret)
   555                  return ret;
   556  
   557          *bw_resc_start = bw_resc_end;
   558  
   559          return 0;
   560  }

regards,
dan carpenter

RE: [bug report] selftests/resctrl: Add callback to start a benchmark

[Prakhya, Sai Praneeth]

Hi Dan,

Thanks for reporting the issues. They look legitimate to me.
I will send a fix.

> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@oracle.com>
> Sent: Thursday, May 7, 2020 4:51 AM
> To: Prakhya, Sai Praneeth <sai.praneeth.prakhya@intel.com>
> Cc: linux-kselftest@vger.kernel.org
> Subject: [bug report] selftests/resctrl: Add callback to start a benchmark
> 
> Hello Sai Praneeth Prakhya,
> 
> The patch 7f4d257e3a2a: "selftests/resctrl: Add callback to start a benchmark"
> from Jan 16, 2020, leads to the following static checker
> warning:
> 
> 	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
> 

I agree. There is no point checking for < 0 for unsigned values.

> tools/testing/selftests/resctrl/resctrl_val.c
>    531  static int
>    532  measure_vals(struct resctrl_val_param *param, unsigned long
> *bw_resc_start)
>    533  {
>    534          unsigned long bw_imc, bw_resc, bw_resc_end;
>    535          int ret;
>    536
>    537          /*
>    538           * Measure memory bandwidth from resctrl and from
>    539           * another source which is perf imc value or could
>    540           * be something else if perf imc event is not available.
>    541           * Compare the two values to validate resctrl value.
>    542           * It takes 1sec to measure the data.
>    543           */
>    544          bw_imc = get_mem_bw_imc(param->cpu_no, param->bw_report);
>    545          if (bw_imc <= 0)
>                     ^^^^^^^^^^^
> Unsigned.  Also the comments for get_mem_bw_imc() says that zero is success.

The comment is wrong. It should have been < 0 failure and > 0 as success.

>    546                  return bw_imc;
>    547
>    548          bw_resc_end = get_mem_bw_resctrl();
>    549          if (bw_resc_end <= 0)
>                     ^^^^^^^^^^^^^^^^
> Unsigned
> 
>    550                  return bw_resc_end;
>    551
>    552          bw_resc = (bw_resc_end - *bw_resc_start) / MB;
>    553          ret = print_results_bw(param->filename, bm_pid, bw_imc, bw_resc);
>    554          if (ret)
>    555                  return ret;
>    556
>    557          *bw_resc_start = bw_resc_end;
>    558
>    559          return 0;
>    560  }
> 
> regards,
> dan carpenter