Hi Ilpo,
On 9/30/24 6:49 AM, Ilpo Järvinen wrote:
On Thu, 12 Sep 2024, Reinette Chatre wrote:
The CMT, MBM, and MBA tests rely on a benchmark that runs while the test makes changes to needed configuration (for example memory bandwidth allocation) and takes needed measurements. By default the "fill_buf" benchmark is used and by default (via its "once = false" setting) "fill_buf" is configured to run until terminated after the test completes.
An unintended consequence of enabling the user to override the benchmark also enables the user to change parameters to the "fill_buf" benchmark. This enables the user to set "fill_buf" to only cycle through the buffer once (by setting "once = true") and thus breaking the CMT, MBA, and MBM tests that expect workload/interference to be reflected by their measurements.
Prevent user space from changing the "once" parameter and ensure that it is always false for the CMT, MBA, and MBM tests.
Suggested-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com Signed-off-by: Reinette Chatre reinette.chatre@intel.com
Changes since V1:
- New patch
tools/testing/selftests/resctrl/fill_buf.c | 7 ++++--- tools/testing/selftests/resctrl/resctrl.h | 2 +- tools/testing/selftests/resctrl/resctrl_tests.c | 8 +++++++- tools/testing/selftests/resctrl/resctrl_val.c | 11 +---------- 4 files changed, 13 insertions(+), 15 deletions(-)
diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c index 34e5df721430..854f0108d8e6 100644 --- a/tools/testing/selftests/resctrl/fill_buf.c +++ b/tools/testing/selftests/resctrl/fill_buf.c @@ -151,7 +151,7 @@ unsigned char *alloc_buffer(size_t buf_size, int memflush) return buf; } -int run_fill_buf(size_t buf_size, int memflush, int op, bool once) +int run_fill_buf(size_t buf_size, int memflush, int op) { unsigned char *buf; @@ -160,9 +160,10 @@ int run_fill_buf(size_t buf_size, int memflush, int op, bool once) return -1; if (op == 0)
fill_cache_read(buf, buf_size, once);
fill_cache_read(buf, buf_size, false);
fill_cache_write(buf, buf_size, once);
fill_cache_write(buf, buf_size, false);- free(buf);
return 0; diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index 2dda56084588..51f5f4b25e06 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -142,7 +142,7 @@ int perf_event_open(struct perf_event_attr *hw_event, pid_t pid, int cpu, unsigned char *alloc_buffer(size_t buf_size, int memflush); void mem_flush(unsigned char *buf, size_t buf_size); void fill_cache_read(unsigned char *buf, size_t buf_size, bool once); -int run_fill_buf(size_t buf_size, int memflush, int op, bool once); +int run_fill_buf(size_t buf_size, int memflush, int op); int initialize_mem_bw_imc(void); int measure_mem_bw(const struct user_params *uparams, struct resctrl_val_param *param, pid_t bm_pid, diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index ecbb7605a981..bee4123a5a9b 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -266,7 +266,13 @@ int main(int argc, char **argv) uparams.benchmark_cmd[1] = span_str; uparams.benchmark_cmd[2] = "1"; uparams.benchmark_cmd[3] = "0";
uparams.benchmark_cmd[4] = "false";
/** Fourth parameter was previously used to indicate* how long "fill_buf" should run for, with "false"* ("fill_buf" will keep running until terminated)* the only option that works.*/uparams.benchmark_cmd[4] = NULL;Why is the [5] assignment kept around? Is something depending on this double NULL termination? This patch removed the access to [4] so I don't see anything beyong [3] accessed explicitly.
In this case this was kept to reduce changes in preparatory work since this will all be replaced later with the more robust parameter handling. I'll remove the [5] assignment.
Thank you.
Reinette