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.