| Message ID | cover.1729804024.git.reinette.chatre@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | selftests/resctrl: Support diverse platforms with MBM and MBA tests | expand |
On 10/24/24 15:18, Reinette Chatre wrote: > Changes since V3: > - V3: https://lore.kernel.org/all/cover.1729218182.git.reinette.chatre@intel.com/ > - Rebased on HEAD 2a027d6bb660 of kselftest/next. > - Fix empty string parsing issues pointed out by Ilpo. > - Add Reviewed-by tags. > - Please see individual patches for detailed changes. > > Changes since V2: > - V2: https://lore.kernel.org/all/cover.1726164080.git.reinette.chatre@intel.com/ > - Add fix to protect against buffer overflow when parsing text from sysfs files. > - Add cleanup patch to address use of magic constants as pointed out by > Ilpo. > - Add Reviewed-by tags where received, except for "selftests/resctrl: Use cache > size to determine "fill_buf" buffer size" that changed too much since > receiving the Reviewed-by tag. > - Please see individual patches for detailed changes. > > Changes since V1: > - V1: https://lore.kernel.org/cover.1724970211.git.reinette.chatre@intel.com/ > - V2 contains the same general solutions to stated problem as V1 but these > are now preceded by more fixes (patches 1 to 5) and improved robustness > (patches 6 to 9) to existing tests before the series gets back > to solving the original problem with more confidence in patches 10 to 13. > - The posibility of making "memflush = false" for CMT test was discussed > during V1. Modifying this setting does not have a significant impact on the > observed results that are already well within acceptable range and this > version thus keeps original default. If performance was a goal it may > be possible to do further experimentation where "memflush = false" could > eliminate the need for the sleep(1) within the test wrapper, but > improving the performance is not a goal of this work. > - (New) Support what seems to be unintended ability for user space to provide > parameters to "fill_buf" by making the parsing robust and only support > changing parameters that are supported to be changed. Drop support for > "write" operation since it has never been measured. > - (New) Improve wraparound handling. (Ilpo) > - (New) A couple of new fixes addressing issues discovered during development. > - (Change from V1) To support fill_buf parameters provided by user space as > well as test specific fill_buf parameters struct fill_buf_param is no longer > just a member of struct resctrl_val_param, instead there could be at most > two instances of struct fill_buf_param, the immutable parameters provided > by user space and the parameters used by individual tests. (Ilpo) > - Please see individual patches for detailed changes. > > V1 cover: > > The resctrl selftests for Memory Bandwidth Allocation (MBA) and Memory > Bandwidth Monitoring (MBM) are failing on some (for example [1]) Emerald > Rapids systems. The test failures result from the following two > properties of these systems: > 1) Emerald Rapids systems can have up to 320MB L3 cache. The resctrl > MBA and MBM selftests measure memory traffic for which a hardcoded > 250MB buffer has been sufficient so far. On platforms with L3 cache > larger than the buffer, the buffer fits in the L3 cache and thus > no/very little memory traffic is generated during the "memory > bandwidth" tests. > 2) Some platform features, for example RAS features or memory > performance features that generate memory traffic may drive accesses > that are counted differently by performance counters and MBM > respectively, for instance generating "overhead" traffic which is not > counted against any specific RMID. Until now these counting > differences have always been "in the noise". On Emerald Rapids > systems the maximum MBA throttling (10% memory bandwidth) > throttles memory bandwidth to where memory accesses by these other > platform features push the memory bandwidth difference between > memory controller performance counters and resctrl (MBM) beyond the > tests' hardcoded tolerance. > > Make the tests more robust against platform variations: > 1) Let the buffer used by memory bandwidth tests be guided by the size > of the L3 cache. > 2) Larger buffers require longer initialization time before the buffer can > be used to measurement. Rework the tests to ensure that buffer > initialization is complete before measurements start. > 3) Do not compare performance counters and MBM measurements at low > bandwidth. The value of "low" is hardcoded to 750MiB based on > measurements on Emerald Rapids, Sapphire Rapids, and Ice Lake > systems. This limit is not applicable to AMD systems since it > only applies to the MBA and MBM tests that are isolated to Intel. > > [1] > https://ark.intel.com/content/www/us/en/ark/products/237261/intel-xeon-platinum-8592-processor-320m-cache-1-9-ghz.html > > Reinette Chatre (15): > selftests/resctrl: Make functions only used in same file static > selftests/resctrl: Print accurate buffer size as part of MBM results > selftests/resctrl: Fix memory overflow due to unhandled wraparound > selftests/resctrl: Protect against array overrun during iMC config > parsing > selftests/resctrl: Protect against array overflow when reading strings > selftests/resctrl: Make wraparound handling obvious > selftests/resctrl: Remove "once" parameter required to be false > selftests/resctrl: Only support measured read operation > selftests/resctrl: Remove unused measurement code > selftests/resctrl: Make benchmark parameter passing robust > selftests/resctrl: Ensure measurements skip initialization of default > benchmark > selftests/resctrl: Use cache size to determine "fill_buf" buffer size > selftests/resctrl: Do not compare performance counters and resctrl at > low bandwidth > selftests/resctrl: Keep results from first test run > selftests/resctrl: Replace magic constants used as array size > > tools/testing/selftests/resctrl/cmt_test.c | 37 +- > tools/testing/selftests/resctrl/fill_buf.c | 45 +- > tools/testing/selftests/resctrl/mba_test.c | 54 ++- > tools/testing/selftests/resctrl/mbm_test.c | 37 +- > tools/testing/selftests/resctrl/resctrl.h | 79 +++- > .../testing/selftests/resctrl/resctrl_tests.c | 95 +++- > tools/testing/selftests/resctrl/resctrl_val.c | 447 +++++------------- > tools/testing/selftests/resctrl/resctrlfs.c | 19 +- > 8 files changed, 354 insertions(+), 459 deletions(-) > > > base-commit: 2a027d6bb66002c8e50e974676f932b33c5fce10 Is this patch series ready to be applied? thanks, -- Shuah
Hi Shuah, On 10/24/24 3:36 PM, Shuah Khan wrote: > > Is this patch series ready to be applied? > I believe it is close ... I would like to give Ilpo some time to peek at patches 2 and 10 to confirm if I got their fixes right this time. The rest of the series is ready. Thank you Reinette
On Thu, 24 Oct 2024, Reinette Chatre wrote: > Hi Shuah, > > On 10/24/24 3:36 PM, Shuah Khan wrote: > > > > Is this patch series ready to be applied? > > > > I believe it is close ... I would like to give Ilpo some time to peek > at patches 2 and 10 to confirm if I got their fixes right this time. The > rest of the series is ready. Hi, I took a look at those two patches now and they seemed fine to me so this series should be ready to go now.
On 10/25/24 6:54 AM, Ilpo Järvinen wrote: > On Thu, 24 Oct 2024, Reinette Chatre wrote: > >> Hi Shuah, >> >> On 10/24/24 3:36 PM, Shuah Khan wrote: >>> >>> Is this patch series ready to be applied? >>> >> >> I believe it is close ... I would like to give Ilpo some time to peek >> at patches 2 and 10 to confirm if I got their fixes right this time. The >> rest of the series is ready. > > Hi, > > I took a look at those two patches now and they seemed fine to me so this > series should be ready to go now. > Thank you very much Ilpo. Reinette
Changes since V3: - V3: https://lore.kernel.org/all/cover.1729218182.git.reinette.chatre@intel.com/ - Rebased on HEAD 2a027d6bb660 of kselftest/next. - Fix empty string parsing issues pointed out by Ilpo. - Add Reviewed-by tags. - Please see individual patches for detailed changes. Changes since V2: - V2: https://lore.kernel.org/all/cover.1726164080.git.reinette.chatre@intel.com/ - Add fix to protect against buffer overflow when parsing text from sysfs files. - Add cleanup patch to address use of magic constants as pointed out by Ilpo. - Add Reviewed-by tags where received, except for "selftests/resctrl: Use cache size to determine "fill_buf" buffer size" that changed too much since receiving the Reviewed-by tag. - Please see individual patches for detailed changes. Changes since V1: - V1: https://lore.kernel.org/cover.1724970211.git.reinette.chatre@intel.com/ - V2 contains the same general solutions to stated problem as V1 but these are now preceded by more fixes (patches 1 to 5) and improved robustness (patches 6 to 9) to existing tests before the series gets back to solving the original problem with more confidence in patches 10 to 13. - The posibility of making "memflush = false" for CMT test was discussed during V1. Modifying this setting does not have a significant impact on the observed results that are already well within acceptable range and this version thus keeps original default. If performance was a goal it may be possible to do further experimentation where "memflush = false" could eliminate the need for the sleep(1) within the test wrapper, but improving the performance is not a goal of this work. - (New) Support what seems to be unintended ability for user space to provide parameters to "fill_buf" by making the parsing robust and only support changing parameters that are supported to be changed. Drop support for "write" operation since it has never been measured. - (New) Improve wraparound handling. (Ilpo) - (New) A couple of new fixes addressing issues discovered during development. - (Change from V1) To support fill_buf parameters provided by user space as well as test specific fill_buf parameters struct fill_buf_param is no longer just a member of struct resctrl_val_param, instead there could be at most two instances of struct fill_buf_param, the immutable parameters provided by user space and the parameters used by individual tests. (Ilpo) - Please see individual patches for detailed changes. V1 cover: The resctrl selftests for Memory Bandwidth Allocation (MBA) and Memory Bandwidth Monitoring (MBM) are failing on some (for example [1]) Emerald Rapids systems. The test failures result from the following two properties of these systems: 1) Emerald Rapids systems can have up to 320MB L3 cache. The resctrl MBA and MBM selftests measure memory traffic for which a hardcoded 250MB buffer has been sufficient so far. On platforms with L3 cache larger than the buffer, the buffer fits in the L3 cache and thus no/very little memory traffic is generated during the "memory bandwidth" tests. 2) Some platform features, for example RAS features or memory performance features that generate memory traffic may drive accesses that are counted differently by performance counters and MBM respectively, for instance generating "overhead" traffic which is not counted against any specific RMID. Until now these counting differences have always been "in the noise". On Emerald Rapids systems the maximum MBA throttling (10% memory bandwidth) throttles memory bandwidth to where memory accesses by these other platform features push the memory bandwidth difference between memory controller performance counters and resctrl (MBM) beyond the tests' hardcoded tolerance. Make the tests more robust against platform variations: 1) Let the buffer used by memory bandwidth tests be guided by the size of the L3 cache. 2) Larger buffers require longer initialization time before the buffer can be used to measurement. Rework the tests to ensure that buffer initialization is complete before measurements start. 3) Do not compare performance counters and MBM measurements at low bandwidth. The value of "low" is hardcoded to 750MiB based on measurements on Emerald Rapids, Sapphire Rapids, and Ice Lake systems. This limit is not applicable to AMD systems since it only applies to the MBA and MBM tests that are isolated to Intel. [1] https://ark.intel.com/content/www/us/en/ark/products/237261/intel-xeon-platinum-8592-processor-320m-cache-1-9-ghz.html Reinette Chatre (15): selftests/resctrl: Make functions only used in same file static selftests/resctrl: Print accurate buffer size as part of MBM results selftests/resctrl: Fix memory overflow due to unhandled wraparound selftests/resctrl: Protect against array overrun during iMC config parsing selftests/resctrl: Protect against array overflow when reading strings selftests/resctrl: Make wraparound handling obvious selftests/resctrl: Remove "once" parameter required to be false selftests/resctrl: Only support measured read operation selftests/resctrl: Remove unused measurement code selftests/resctrl: Make benchmark parameter passing robust selftests/resctrl: Ensure measurements skip initialization of default benchmark selftests/resctrl: Use cache size to determine "fill_buf" buffer size selftests/resctrl: Do not compare performance counters and resctrl at low bandwidth selftests/resctrl: Keep results from first test run selftests/resctrl: Replace magic constants used as array size tools/testing/selftests/resctrl/cmt_test.c | 37 +- tools/testing/selftests/resctrl/fill_buf.c | 45 +- tools/testing/selftests/resctrl/mba_test.c | 54 ++- tools/testing/selftests/resctrl/mbm_test.c | 37 +- tools/testing/selftests/resctrl/resctrl.h | 79 +++- .../testing/selftests/resctrl/resctrl_tests.c | 95 +++- tools/testing/selftests/resctrl/resctrl_val.c | 447 +++++------------- tools/testing/selftests/resctrl/resctrlfs.c | 19 +- 8 files changed, 354 insertions(+), 459 deletions(-) base-commit: 2a027d6bb66002c8e50e974676f932b33c5fce10