Re: [PATCH V2 07/13] selftests/resctrl: Only support measured read operation

From: Ilpo Järvinen
Date: Mon Sep 30 2024 - 09:52:32 EST

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@xxxxxxxxx>
> ---
> 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?

--
i.

> }
> 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 */
>