Re: [PATCH V3 08/15] selftests/resctrl: Only support measured read operation

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 6606 bytes --]

On Thu, 17 Oct 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.
> 
> Keep the initialization of the, now unused, "fill_buf" parameters
> to reserve these parameter positions since it has been exposed as an API.
> Future parameter additions cannot use these parameter positions.
> 
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

--
 i.

> ---
> Changes since V2:
> - Update changelog to justify keeping the assignment to benchmark_cmd[4].
>   (Ilpo)
> 
> 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 e7878077883f..0f91c475b255 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;
>  	}
>  
> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
> index b0f3c594c4da..113ca18d67c1 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 */
>