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

[Ilpo Järvinen]

On Tue, 19 Mar 2024, Reinette Chatre wrote:
> On 3/11/2024 6:52 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.
> > 
> 
> Thank you very much for doing this.
> 
> > Suggested-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> > ---
> >  tools/testing/selftests/resctrl/resctrl_val.c | 72 +++++++++++++------
> >  1 file changed, 50 insertions(+), 22 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
> > index 36139cba7be8..4df2cd738f88 100644
> > --- a/tools/testing/selftests/resctrl/resctrl_val.c
> > +++ b/tools/testing/selftests/resctrl/resctrl_val.c

> > -static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc)
> > +static int perf_open_imc_mem_bw(int cpu_no)
> >  {
> > -	float reads, writes, of_mul_read, of_mul_write;
> >  	int imc, j, ret;
> >  
> > -	/* Start all iMC counters to log values (both read and write) */
> > -	reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1;
> >  	for (imc = 0; imc < imcs; imc++) {
> >  		for (j = 0; j < 2; j++) {
> >  			ret = open_perf_event(imc, cpu_no, j);
> >  			if (ret)
> >  				return -1;
> >  		}
> 
> I'm feeling more strongly that this inner loop makes the code harder to
> understand and unwinding it would make it easier to understand.

Okay, I'll unwind them in the first patch.

> >  	}
> > +}
> > +
> > +/*
> > + * get_mem_bw_imc - Memory band width as reported by iMC counters
> > + * @bw_report:		Bandwidth report type (reads, writes)
> > + *
> > + * Memory B/W utilized by a process on a socket can be calculated using
> > + * iMC counters. Perf events are used to read these counters.
> 
> In the above there are three variations of the same: "band width", "Bandwidth",
> and "B/W". Please just use one and use it consistently.

Okay but I'll do that in a separate patch because these are just the 
"removed" lines above, the diff is more messy than the actual change 
here as is often the case with this kind of split function refactoring 
because the diff algorithm fails to pair the lines optimally from 
human-readed PoV.

> > + * Return: = 0 on success. < 0 on failure.
> > + */
> > +static int get_mem_bw_imc(char *bw_report, float *bw_imc)
> > +{
> > +	float reads, writes, of_mul_read, of_mul_write;
> > +	int imc, j;
> > +
> > +	/* Start all iMC counters to log values (both read and write) */
> > +	reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1;
> >  
> >  	/*
> >  	 * Get results which are stored in struct type imc_counter_config

> > @@ -696,7 +725,6 @@ int resctrl_val(const struct resctrl_test *test,
> >  		struct resctrl_val_param *param)
> >  {
> >  	char *resctrl_val = param->resctrl_val;
> > -	unsigned long bw_resc_start = 0;
> 
> In the current implementation the first iteration's starting measurement
> is, as seen above, 0 ... which makes the first measurement unreliable
> and dropped for both the MBA and MBM tests. In this enhancement, the
> first measurement is no longer skewed so much so I wonder if this enhancement
> can be expanded to the analysis phase where first measurement no longer
> needs to be dropped?

In ideal world, yes, but I'll have to check the raw numbers. My general 
feel is that the numbers tend to converge slowly with more iterations 
being run so the first iteration might still be "off" by quite much (this 
is definitely the case with CAT tests iterations but I'm not entirely sure 
any more how it is with other selftests).

Thanks for the review.

-- 
 i.