| Message ID | 491d5a951751dd74ccb84e175f6dd457dbed5c31.1726164080.git.reinette.chatre@intel.com (mailing list archive) |
|---|---|
| State | New |
| 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 to generate > memory traffic. By default this is the "fill_buf" benchmark that > can be replaced via the "-b" command line argument. > > The original intent of the "-b" command line parameter was > to replace the default "fill_buf" benchmark, but the implementation > also exposes an alternative use case where the "fill_buf" parameters > itself can be modified. One of the parameters to "fill_buf" is the > "operation" that can be either "read" or "write" and indicates > whether the "fill_buf" should use "read" or "write" operations on the > allocated buffer. > > While replacing "fill_buf" default parameters is technically possible, > replacing the default "read" parameter with "write" is not supported > because the MBA and MBM tests only measure "read" operations. The > "read" operation is also most appropriate for the CMT test that aims > to use the benchmark to allocate into the cache. > > Avoid any potential inconsistencies between test and measurement by > removing code for unsupported "write" operations to the buffer. > Ignore any attempt from user space to enable this unsupported test > configuration, instead always use read operations. > > Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> > --- > Changes since V1: > - New patch. > --- > tools/testing/selftests/resctrl/fill_buf.c | 28 ++----------------- > tools/testing/selftests/resctrl/resctrl.h | 2 +- > .../testing/selftests/resctrl/resctrl_tests.c | 5 +++- > tools/testing/selftests/resctrl/resctrl_val.c | 5 ++-- > 4 files changed, 9 insertions(+), 31 deletions(-) > > diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c > index 854f0108d8e6..e4f1cea317f1 100644 > --- a/tools/testing/selftests/resctrl/fill_buf.c > +++ b/tools/testing/selftests/resctrl/fill_buf.c > @@ -88,18 +88,6 @@ static int fill_one_span_read(unsigned char *buf, size_t buf_size) > return sum; > } > > -static void fill_one_span_write(unsigned char *buf, size_t buf_size) > -{ > - unsigned char *end_ptr = buf + buf_size; > - unsigned char *p; > - > - p = buf; > - while (p < end_ptr) { > - *p = '1'; > - p += (CL_SIZE / 2); > - } > -} > - > void fill_cache_read(unsigned char *buf, size_t buf_size, bool once) > { > int ret = 0; > @@ -114,15 +102,6 @@ void fill_cache_read(unsigned char *buf, size_t buf_size, bool once) > *value_sink = ret; > } > > -static void fill_cache_write(unsigned char *buf, size_t buf_size, bool once) > -{ > - while (1) { > - fill_one_span_write(buf, buf_size); > - if (once) > - break; > - } > -} > - > unsigned char *alloc_buffer(size_t buf_size, int memflush) > { > void *buf = NULL; > @@ -151,7 +130,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) > +int run_fill_buf(size_t buf_size, int memflush) > { > unsigned char *buf; > > @@ -159,10 +138,7 @@ int run_fill_buf(size_t buf_size, int memflush, int op) > if (!buf) > return -1; > > - if (op == 0) > - fill_cache_read(buf, buf_size, false); > - else > - fill_cache_write(buf, buf_size, false); > + fill_cache_read(buf, buf_size, false); > > free(buf); > > diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h > index 51f5f4b25e06..ba1ce1b35699 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); > +int run_fill_buf(size_t buf_size, int memflush); > 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 bee4123a5a9b..60627dbae20a 100644 > --- a/tools/testing/selftests/resctrl/resctrl_tests.c > +++ b/tools/testing/selftests/resctrl/resctrl_tests.c > @@ -265,13 +265,16 @@ int main(int argc, char **argv) > ksft_exit_fail_msg("Out of memory!\n"); > uparams.benchmark_cmd[1] = span_str; > uparams.benchmark_cmd[2] = "1"; > - uparams.benchmark_cmd[3] = "0"; > /* > + * Third parameter was previously used for "operation" > + * (read/write) of which only (now default) "read"/"0" > + * works. > * 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[3] = NULL; > uparams.benchmark_cmd[4] = NULL; > uparams.benchmark_cmd[5] = NULL; The same question as with the previous patch, why is [4] = NULL kept around?
Hi Ilpo, On 9/30/24 6:52 AM, Ilpo Järvinen wrote: > On Thu, 12 Sep 2024, Reinette Chatre wrote: >> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h >> index 51f5f4b25e06..ba1ce1b35699 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); >> +int run_fill_buf(size_t buf_size, int memflush); >> 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 bee4123a5a9b..60627dbae20a 100644 >> --- a/tools/testing/selftests/resctrl/resctrl_tests.c >> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c >> @@ -265,13 +265,16 @@ int main(int argc, char **argv) >> ksft_exit_fail_msg("Out of memory!\n"); >> uparams.benchmark_cmd[1] = span_str; >> uparams.benchmark_cmd[2] = "1"; >> - uparams.benchmark_cmd[3] = "0"; >> /* >> + * Third parameter was previously used for "operation" >> + * (read/write) of which only (now default) "read"/"0" >> + * works. >> * 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[3] = NULL; >> uparams.benchmark_cmd[4] = NULL; >> uparams.benchmark_cmd[5] = NULL; > > The same question as with the previous patch, why is [4] = NULL kept > around? > You are correct that functionally this is not required. If this parameter disappears at this point then there is no record of parameter 4 ever being used. Even though this is user space I do still have my kernel view that we should aim to maintain ABI. This means that parameter 4 will always be "used" to indicate how long fill_buf should run for and if "fill_buf" ever needs a new parameter, it cannot use parameter 4 since that already has a meaning. While the above may seem unnecessary, I think it makes the more robust parameter processing found in patch #9 that replaces it easier to understand. In that patch the comments above are coded to ensure parameter values are as expected and parameter 4 continue to be dedicated to how long "fill_buf" should run for. As you mention in similar feedback to patch #6, the [5] assignment is also unnecessary. Since it is just used as termination I can remove it. Reinette
diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c index 854f0108d8e6..e4f1cea317f1 100644 --- a/tools/testing/selftests/resctrl/fill_buf.c +++ b/tools/testing/selftests/resctrl/fill_buf.c @@ -88,18 +88,6 @@ static int fill_one_span_read(unsigned char *buf, size_t buf_size) return sum; } -static void fill_one_span_write(unsigned char *buf, size_t buf_size) -{ - unsigned char *end_ptr = buf + buf_size; - unsigned char *p; - - p = buf; - while (p < end_ptr) { - *p = '1'; - p += (CL_SIZE / 2); - } -} - void fill_cache_read(unsigned char *buf, size_t buf_size, bool once) { int ret = 0; @@ -114,15 +102,6 @@ void fill_cache_read(unsigned char *buf, size_t buf_size, bool once) *value_sink = ret; } -static void fill_cache_write(unsigned char *buf, size_t buf_size, bool once) -{ - while (1) { - fill_one_span_write(buf, buf_size); - if (once) - break; - } -} - unsigned char *alloc_buffer(size_t buf_size, int memflush) { void *buf = NULL; @@ -151,7 +130,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) +int run_fill_buf(size_t buf_size, int memflush) { unsigned char *buf; @@ -159,10 +138,7 @@ int run_fill_buf(size_t buf_size, int memflush, int op) if (!buf) return -1; - if (op == 0) - fill_cache_read(buf, buf_size, false); - else - fill_cache_write(buf, buf_size, false); + fill_cache_read(buf, buf_size, false); free(buf); diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index 51f5f4b25e06..ba1ce1b35699 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); +int run_fill_buf(size_t buf_size, int memflush); 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 bee4123a5a9b..60627dbae20a 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -265,13 +265,16 @@ int main(int argc, char **argv) ksft_exit_fail_msg("Out of memory!\n"); uparams.benchmark_cmd[1] = span_str; uparams.benchmark_cmd[2] = "1"; - uparams.benchmark_cmd[3] = "0"; /* + * Third parameter was previously used for "operation" + * (read/write) of which only (now default) "read"/"0" + * works. * 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[3] = NULL; 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 5331354aaf64..8b5973c5e934 100644 --- a/tools/testing/selftests/resctrl/resctrl_val.c +++ b/tools/testing/selftests/resctrl/resctrl_val.c @@ -622,8 +622,8 @@ int measure_mem_bw(const struct user_params *uparams, */ static void run_benchmark(int signum, siginfo_t *info, void *ucontext) { - int operation, ret, memflush; char **benchmark_cmd; + int ret, memflush; size_t span; FILE *fp; @@ -643,9 +643,8 @@ static void run_benchmark(int signum, siginfo_t *info, void *ucontext) /* Execute default fill_buf benchmark */ span = strtoul(benchmark_cmd[1], NULL, 10); memflush = atoi(benchmark_cmd[2]); - operation = atoi(benchmark_cmd[3]); - if (run_fill_buf(span, memflush, operation)) + if (run_fill_buf(span, memflush)) fprintf(stderr, "Error in running fill buffer\n"); } else { /* Execute specified benchmark */
The CMT, MBM, and MBA tests rely on a benchmark to generate memory traffic. By default this is the "fill_buf" benchmark that can be replaced via the "-b" command line argument. The original intent of the "-b" command line parameter was to replace the default "fill_buf" benchmark, but the implementation also exposes an alternative use case where the "fill_buf" parameters itself can be modified. One of the parameters to "fill_buf" is the "operation" that can be either "read" or "write" and indicates whether the "fill_buf" should use "read" or "write" operations on the allocated buffer. While replacing "fill_buf" default parameters is technically possible, replacing the default "read" parameter with "write" is not supported because the MBA and MBM tests only measure "read" operations. The "read" operation is also most appropriate for the CMT test that aims to use the benchmark to allocate into the cache. Avoid any potential inconsistencies between test and measurement by removing code for unsupported "write" operations to the buffer. Ignore any attempt from user space to enable this unsupported test configuration, instead always use read operations. Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> --- Changes since V1: - New patch. --- tools/testing/selftests/resctrl/fill_buf.c | 28 ++----------------- tools/testing/selftests/resctrl/resctrl.h | 2 +- .../testing/selftests/resctrl/resctrl_tests.c | 5 +++- tools/testing/selftests/resctrl/resctrl_val.c | 5 ++-- 4 files changed, 9 insertions(+), 31 deletions(-)