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@xxxxxxxxx>
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> > ---
> >
> > 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.
