| Message ID | 1f5ad02dc424bfc3cca705ed9a322df8f35f2ff4.1726164080.git.reinette.chatre@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | selftests/resctrl: Support diverse platforms with MBM and MBA tests | expand |
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); > else > - 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; > uparams.benchmark_cmd[5] = 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.
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); >> else >> - 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; >> uparams.benchmark_cmd[5] = 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
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); else - 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; uparams.benchmark_cmd[5] = NULL; } diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c index e88d5ca30517..5331354aaf64 100644 --- a/tools/testing/selftests/resctrl/resctrl_val.c +++ b/tools/testing/selftests/resctrl/resctrl_val.c @@ -625,7 +625,6 @@ static void run_benchmark(int signum, siginfo_t *info, void *ucontext) int operation, ret, memflush; char **benchmark_cmd; size_t span; - bool once; FILE *fp; benchmark_cmd = info->si_ptr; @@ -645,16 +644,8 @@ static void run_benchmark(int signum, siginfo_t *info, void *ucontext) span = strtoul(benchmark_cmd[1], NULL, 10); memflush = atoi(benchmark_cmd[2]); operation = atoi(benchmark_cmd[3]); - if (!strcmp(benchmark_cmd[4], "true")) { - once = true; - } else if (!strcmp(benchmark_cmd[4], "false")) { - once = false; - } else { - ksft_print_msg("Invalid once parameter\n"); - parent_exit(ppid); - } - if (run_fill_buf(span, memflush, operation, once)) + if (run_fill_buf(span, memflush, operation)) fprintf(stderr, "Error in running fill buffer\n"); } else { /* Execute specified benchmark */
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(-)