Re: [PATCH v5 02/16] selftests/resctrl: Calculate resctrl FS derived mem bw over sleep(1) only

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3822 bytes --]

On Fri, 31 May 2024, Reinette Chatre wrote:
> On 5/31/24 6:11 AM, Ilpo Järvinen wrote:
> > For MBM/MBA tests, measure_vals() calls get_mem_bw_imc() that performs
> > the measurement over a duration of sleep(1) call. The memory bandwidth
> > numbers from IMC are derived over this duration. The resctrl FS derived
> > memory bandwidth, however, is calculated inside measure_vals() and only
> > takes delta between the previous value and the current one which
> > besides the actual test, also samples inter-test noise.
> > 
> > Rework the logic in measure_vals() and get_mem_bw_imc() such that the
> > resctrl FS memory bandwidth section covers much shorter duration
> > closely matching that of the IMC perf counters to improve measurement
> > accuracy.
> > 
> > For the second read after rewind() to return a fresh value, also
> > newline has to be consumed by the fscanf().
> > 
> > Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > ---
> > 
> > v5:
> > - Open mem bw file once and use rewind()
> > - Read \n from the mem bw file to allow rewind to return a new value.
> > v4:
> > - Open resctrl mem bw file (twice) beforehand to avoid opening it during
> >    the test
> > v3:
> > - Don't drop Return: entry from perf_open_imc_mem_bw() func comment
> > ---
> >   tools/testing/selftests/resctrl/resctrl_val.c | 115 ++++++++++++------
> >   1 file changed, 80 insertions(+), 35 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/resctrl/resctrl_val.c
> > b/tools/testing/selftests/resctrl/resctrl_val.c
> > index f55f5989de72..6231275a6e6c 100644
> > --- a/tools/testing/selftests/resctrl/resctrl_val.c
> > +++ b/tools/testing/selftests/resctrl/resctrl_val.c

> >   @@ -616,13 +645,17 @@ static void initialize_llc_occu_resctrl(const char
> > *ctrlgrp, const char *mongrp,
> >   }
> >     static int measure_vals(const struct user_params *uparams,
> > -			struct resctrl_val_param *param,
> > -			unsigned long *bw_resc_start)
> > +			struct resctrl_val_param *param)
> >   {
> > -	unsigned long bw_resc, bw_resc_end;
> > +	unsigned long bw_resc, bw_resc_start, bw_resc_end;
> > +	FILE *mem_bw_fp;
> >   	float bw_imc;
> >   	int ret;
> >   +	mem_bw_fp = open_mem_bw_resctrl(mbm_total_path);
> > +	if (!mem_bw_fp)
> > +		return -1;
> > +
> 
> The comment below seems to refer to the resctrl measurement
> that starts with the above snippet. Any reason why this snippet
> is above the comment that follows since the comment seems to
> apply to it?

No particular reason. I've made the comment a function one now which 
seemed better placement for it.

> >   	/*
> >   	 * Measure memory bandwidth from resctrl and from
> >   	 * another source which is perf imc value or could

> > @@ -630,22 +663,35 @@ static int measure_vals(const struct user_params
> > *uparams,
> >   	 * Compare the two values to validate resctrl value.
> >   	 * It takes 1sec to measure the data.
> >   	 */
> > -	ret = get_mem_bw_imc(uparams->cpu, param->bw_report, &bw_imc);
> > +	ret = perf_open_imc_mem_bw(uparams->cpu);
> >   	if (ret < 0)
> > -		return ret;
> > +		goto close_fp;
> >   -	ret = get_mem_bw_resctrl(&bw_resc_end);
> > +	ret = get_mem_bw_resctrl(mem_bw_fp, &bw_resc_start);
> >   	if (ret < 0)
> > -		return ret;
> > +		goto close_fp;
> 
> perf_close_imc_mem_bw() seems to be missing from error path?
> 
> Symmetrical code is easier to understand. Looks like
> perf_close_imc_mem_bw() stayed behind in get_mem_bw_imc() but I think
> it would make things easier if get_mem_bw_imc() no longer calls
> perf_close_imc_mem_bw() but instead leave that to the one that
> calls perf_open_imc_mem_bw().

Okay yeah, it makes things more tractable.

-- 
 i.