On Fri, 7 Jun 2024, Reinette Chatre wrote: > On 6/7/24 5:53 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> > > Tested-by: Babu Moger <babu.moger@xxxxxxx> > > --- > > > > v6: > > - Adjust closing/rollback of the IMC perf > > - Move the comment in measure_vals() to function level > > - Capitalize MBM > > - binded to -> bound to > > This change does not match the patch. > > > 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 | 141 +++++++++++------- > > 1 file changed, 91 insertions(+), 50 deletions(-) > > > > diff --git a/tools/testing/selftests/resctrl/resctrl_val.c > > b/tools/testing/selftests/resctrl/resctrl_val.c > > index f55f5989de72..1575c5c09ac7 100644 > > --- a/tools/testing/selftests/resctrl/resctrl_val.c > > +++ b/tools/testing/selftests/resctrl/resctrl_val.c > > @@ -306,18 +306,13 @@ static void perf_close_imc_mem_bw(void) > > } > > /* > > - * get_mem_bw_imc: Memory band width as reported by iMC counters > > - * @cpu_no: CPU number that the benchmark PID is binded to > > - * @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. > > + * perf_open_imc_mem_bw - Open perf fds for IMCs > > + * @cpu_no: CPU number that the benchmark PID is bounded to > > "is bounded to" -> "is bound to"? > > Just the one nitpick from me. The patch looks good to me. Thank you > very much. > > | Reviewed-by: Reinette Chatre <reinette.chatre@xxxxxxxxx> I'll correct this in v7. I guess it's because I now make these corrections "twice", once for the per patch changelog bullet and then the actual change which unfortunately means my brains switch to repeated actions mode => autotype without thinking. Earlier when I made the version changelogs only into 00/xx, I actually read through the diff of diffs against the previous version in order to find out what to write into the changelog. I ended up catching many error like this in that stage. Now it's just mostly copying the pre-existing entries from per patch changelogs (and less attention is obviously paid on looking the diff of diffs because I no longer need to derive information out of them). -- i.
