Re: [PATCH V2 09/13] selftests/resctrl: Make benchmark parameter passing robust

[Ilpo Järvinen]

On Thu, 12 Sep 2024, Reinette Chatre wrote:

> The benchmark used during the CMT, MBM, and MBA tests can be provided by
> the user via (-b) parameter, if not provided the default "fill_buf"
> benchmark is used. The user is additionally able to override
> any of the "fill_buf" default parameters when running the tests with
> "-b fill_buf <fill_buf parameters>".
> 
> The "fill_buf" parameters are managed as an array of strings. Using an
> array of strings is complex because it requires transformations to/from
> strings at every producer and consumer. This is made worse for the
> individual tests where the default benchmark parameters values may not
> be appropriate and additional data wrangling is required. For example,
> the CMT test duplicates the entire array of strings in order to replace
> one of the parameters.
> 
> More issues appear when combining the usage of an array of strings with
> the use case of user overriding default parameters by specifying
> "-b fill_buf <parameters>". This use case is fragile with opportunities
> to trigger a SIGSEGV because of opportunities for NULL pointers to exist
> in the array of strings. For example, by running below (thus by specifying
> "fill_buf" should be used but all parameters are NULL):
> 	$ sudo resctrl_tests -t mbm -b fill_buf
> 
> Replace the "array of strings" parameters used for "fill_buf" with
> new struct fill_buf_param that contains the "fill_buf" parameters that
> can be used directly without transformations to/from strings. Two
> instances of struct fill_buf_param may exist at any point in time:
> 	* If the user provides new parameters to "fill_buf", the
> 	  user parameter structure (struct user_params) will point to a
> 	  fully initialized and immutable struct fill_buf_param
> 	  containing the user provided parameters.
> 	* If "fill_buf" is the benchmark that should be used by a test,
> 	  then the test parameter structure (struct resctrl_val_param)
> 	  will point to a fully initialized struct fill_buf_param. The
> 	  latter may contain (a) the user provided parameters verbatim,
> 	  (b) user provided parameters adjusted to be appropriate for
> 	  the test, or (c) the default parameters for "fill_buf" that
> 	  is appropriate for the test if the user did not provide
> 	  "fill_buf" parameters nor an alternate benchmark.
> 
> The existing behavior of CMT test is to use test defined value for the
> buffer size even if the user provides another value via command line.
> This behavior is maintained since the test requires that the buffer size
> matches the size of the cache allocated, and the amount of cache
> allocated can instead be changed by the user with the "-n" command line
> parameter.
> 
> Signed-off-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>
> ---
> Changes since V1:
> - Maintain original behavior where user can override "fill_buf"
>   parameters via command line ... but only those that can actually
>   be changed. (Ilpo)
> - Fix parsing issues associated with original behavior to ensure
>   any parameter is valid before any attempt to use it.
> - Move patch earlier in series to highlight that this fixes existing
>   issues.
> - Make struct fill_buf_param dynamic to support user provided
>   parameters as well as test specific parameters.
> - Rewrite changelog.
> ---
>  tools/testing/selftests/resctrl/cmt_test.c    |  32 ++----
>  tools/testing/selftests/resctrl/mba_test.c    |  13 ++-
>  tools/testing/selftests/resctrl/mbm_test.c    |  22 ++--
>  tools/testing/selftests/resctrl/resctrl.h     |  54 ++++++---
>  .../testing/selftests/resctrl/resctrl_tests.c | 104 +++++++++++++-----
>  tools/testing/selftests/resctrl/resctrl_val.c |  43 ++++----
>  6 files changed, 176 insertions(+), 92 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
> index 0c045080d808..a7effe76b419 100644
> --- a/tools/testing/selftests/resctrl/cmt_test.c
> +++ b/tools/testing/selftests/resctrl/cmt_test.c
> @@ -116,15 +116,13 @@ static void cmt_test_cleanup(void)
>  
>  static int cmt_run_test(const struct resctrl_test *test, const struct user_params *uparams)
>  {
> -	const char * const *cmd = uparams->benchmark_cmd;
> -	const char *new_cmd[BENCHMARK_ARGS];
> +	struct fill_buf_param fill_buf = {0};

Empty initializer is enough to have the fields initialized to zero.

>  	unsigned long cache_total_size = 0;
>  	int n = uparams->bits ? : 5;
>  	unsigned long long_mask;
> -	char *span_str = NULL;
>  	int count_of_bits;
>  	size_t span;
> -	int ret, i;
> +	int ret;
>  
>  	ret = get_full_cbm("L3", &long_mask);
>  	if (ret)
> @@ -155,32 +153,26 @@ static int cmt_run_test(const struct resctrl_test *test, const struct user_param
>  
>  	span = cache_portion_size(cache_total_size, param.mask, long_mask);
>  
> -	if (strcmp(cmd[0], "fill_buf") == 0) {
> -		/* Duplicate the command to be able to replace span in it */
> -		for (i = 0; uparams->benchmark_cmd[i]; i++)
> -			new_cmd[i] = uparams->benchmark_cmd[i];
> -		new_cmd[i] = NULL;
> -
> -		ret = asprintf(&span_str, "%zu", span);
> -		if (ret < 0)
> -			return -1;
> -		new_cmd[1] = span_str;
> -		cmd = new_cmd;
> +	if (uparams->fill_buf) {
> +		fill_buf.buf_size = span;
> +		fill_buf.memflush = uparams->fill_buf->memflush;
> +		param.fill_buf = &fill_buf;
> +	} else if (!uparams->benchmark_cmd[0]) {
> +		fill_buf.buf_size = span;
> +		fill_buf.memflush = 1;
> +		param.fill_buf = &fill_buf;
>  	}
>  
>  	remove(RESULT_FILE_NAME);
>  
> -	ret = resctrl_val(test, uparams, cmd, &param);
> +	ret = resctrl_val(test, uparams, &param);
>  	if (ret)
> -		goto out;
> +		return ret;
>  
>  	ret = check_results(&param, span, n);
>  	if (ret && (get_vendor() == ARCH_INTEL))
>  		ksft_print_msg("Intel CMT may be inaccurate when Sub-NUMA Clustering is enabled. Check BIOS configuration.\n");
>  
> -out:
> -	free(span_str);
> -
>  	return ret;
>  }
>  
> diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
> index be0ead73e55d..7e43056c8737 100644
> --- a/tools/testing/selftests/resctrl/mba_test.c
> +++ b/tools/testing/selftests/resctrl/mba_test.c
> @@ -172,11 +172,22 @@ static int mba_run_test(const struct resctrl_test *test, const struct user_param
>  		.setup		= mba_setup,
>  		.measure	= mba_measure,
>  	};
> +	struct fill_buf_param fill_buf = {0};

{}

>  	int ret;
>  
>  	remove(RESULT_FILE_NAME);
>  
> -	ret = resctrl_val(test, uparams, uparams->benchmark_cmd, &param);
> +	if (uparams->fill_buf) {
> +		fill_buf.buf_size = uparams->fill_buf->buf_size;
> +		fill_buf.memflush = uparams->fill_buf->memflush;
> +		param.fill_buf = &fill_buf;
> +	} else if (!uparams->benchmark_cmd[0]) {
> +		fill_buf.buf_size = DEFAULT_SPAN;
> +		fill_buf.memflush = 1;
> +		param.fill_buf = &fill_buf;
> +	}
> +
> +	ret = resctrl_val(test, uparams, &param);
>  	if (ret)
>  		return ret;
>  
> diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
> index 9744bf1ac16f..b1f03a73333f 100644
> --- a/tools/testing/selftests/resctrl/mbm_test.c
> +++ b/tools/testing/selftests/resctrl/mbm_test.c
> @@ -139,26 +139,26 @@ static int mbm_run_test(const struct resctrl_test *test, const struct user_param
>  		.setup		= mbm_setup,
>  		.measure	= mbm_measure,
>  	};
> -	char *endptr = NULL;
> -	size_t span = 0;
> +	struct fill_buf_param fill_buf = {0};
>  	int ret;
>  
>  	remove(RESULT_FILE_NAME);
>  
> -	if (uparams->benchmark_cmd[0] && strcmp(uparams->benchmark_cmd[0], "fill_buf") == 0) {
> -		if (uparams->benchmark_cmd[1]) {
> -			errno = 0;
> -			span = strtoul(uparams->benchmark_cmd[1], &endptr, 10);
> -			if (errno || uparams->benchmark_cmd[1] == endptr)
> -				return -errno;
> -		}
> +	if (uparams->fill_buf) {
> +		fill_buf.buf_size = uparams->fill_buf->buf_size;
> +		fill_buf.memflush = uparams->fill_buf->memflush;
> +		param.fill_buf = &fill_buf;
> +	} else if (!uparams->benchmark_cmd[0]) {
> +		fill_buf.buf_size = DEFAULT_SPAN;
> +		fill_buf.memflush = 1;
> +		param.fill_buf = &fill_buf;
>  	}
>  
> -	ret = resctrl_val(test, uparams, uparams->benchmark_cmd, &param);
> +	ret = resctrl_val(test, uparams, &param);
>  	if (ret)
>  		return ret;
>  
> -	ret = check_results(span);
> +	ret = check_results(param.fill_buf ? param.fill_buf->buf_size : 0);
>  	if (ret && (get_vendor() == ARCH_INTEL))
>  		ksft_print_msg("Intel MBM may be inaccurate when Sub-NUMA Clustering is enabled. Check BIOS configuration.\n");
>  
> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index 82801245e4c1..98462752cb46 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -43,16 +43,35 @@
>  
>  #define DEFAULT_SPAN		(250 * MB)
>  
> +/*
> + * fill_buf_param:	"fill_buf" benchmark parameters
> + * @buf_size:		Size (in bytes) of buffer used in benchmark.
> + *			"fill_buf" allocates and initializes buffer of
> + *			@buf_size. User can change value via command line.
> + * @memflush:		If 0 then the buffer will not be flushed after
> + *			allocation and initialization, otherwise the
> + *			buffer will be flushed. User can change value via
> + *			command line.
> + */
> +struct fill_buf_param {
> +	size_t		buf_size;
> +	int		memflush;

bool?

Or is there a plan to use other values than 0 and 1?

> +};
> +
>  /*
>   * user_params:		User supplied parameters
>   * @cpu:		CPU number to which the benchmark will be bound to
>   * @bits:		Number of bits used for cache allocation size
>   * @benchmark_cmd:	Benchmark command to run during (some of the) tests
> + * @fill_buf:		Pointer to user provided parameters for "fill_buf",
> + *			NULL if user did not provide parameters and test
> + *			specific defaults should be used.
>   */
>  struct user_params {
>  	int cpu;
>  	int bits;
>  	const char *benchmark_cmd[BENCHMARK_ARGS];
> +	const struct fill_buf_param *fill_buf;
>  };
>  
>  /*
> @@ -87,21 +106,29 @@ struct resctrl_test {
>   * @init:		Callback function to initialize test environment
>   * @setup:		Callback function to setup per test run environment
>   * @measure:		Callback that performs the measurement (a single test)
> + * @fill_buf:		Parameters for default "fill_buf" benchmark.
> + *			Initialized with user provided parameters, possibly
> + *			adapted to be relevant to the test. If user does
> + *			not provide parameters for "fill_buf" nor a
> + *			replacement benchmark then initialized with defaults
> + *			appropriate for test. NULL if user provided
> + *			benchmark.
>   */
>  struct resctrl_val_param {
> -	const char	*ctrlgrp;
> -	const char	*mongrp;
> -	char		filename[64];
> -	unsigned long	mask;
> -	int		num_of_runs;
> -	int		(*init)(const struct resctrl_val_param *param,
> -				int domain_id);
> -	int		(*setup)(const struct resctrl_test *test,
> -				 const struct user_params *uparams,
> -				 struct resctrl_val_param *param);
> -	int		(*measure)(const struct user_params *uparams,
> -				   struct resctrl_val_param *param,
> -				   pid_t bm_pid);
> +	const char		*ctrlgrp;
> +	const char		*mongrp;
> +	char			filename[64];
> +	unsigned long		mask;
> +	int			num_of_runs;
> +	int			(*init)(const struct resctrl_val_param *param,
> +					int domain_id);
> +	int			(*setup)(const struct resctrl_test *test,
> +					 const struct user_params *uparams,
> +					 struct resctrl_val_param *param);
> +	int			(*measure)(const struct user_params *uparams,
> +					   struct resctrl_val_param *param,
> +					   pid_t bm_pid);
> +	struct fill_buf_param	*fill_buf;
>  };
>  
>  struct perf_event_read {
> @@ -149,7 +176,6 @@ void initialize_mem_bw_resctrl(const struct resctrl_val_param *param,
>  			       int domain_id);
>  int resctrl_val(const struct resctrl_test *test,
>  		const struct user_params *uparams,
> -		const char * const *benchmark_cmd,
>  		struct resctrl_val_param *param);
>  unsigned long create_bit_mask(unsigned int start, unsigned int len);
>  unsigned int count_contiguous_bits(unsigned long val, unsigned int *start);
> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
> index 60627dbae20a..121079363208 100644
> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
> @@ -148,6 +148,78 @@ static void run_single_test(const struct resctrl_test *test, const struct user_p
>  	test_cleanup(test);
>  }
>  
> +/*
> + * Allocate and initialize a struct fill_buf_param with user provided
> + * (via "-b fill_buf <fill_buf parameters>") parameters.
> + *
> + * Use defaults (that may not be appropriate for all tests) for any
> + * fill_buf parameters omitted by the user.
> + *
> + * Historically it may have been possible for user space to provide
> + * additional parameters, "operation" ("read" vs "write") in
> + * benchmark_cmd[3] and "once" (run "once" or until terminated) in
> + * benchmark_cmd[4]. Changing these parameters have never been
> + * supported with the default of "read" operation and running until
> + * terminated built into the tests. Any unsupported values for
> + * (original) "fill_buf" parameters are treated as failure.
> + *
> + * Return: On failure, forcibly exits the test on any parsing failure,
> + *         returns NULL if no parsing needed (user did not actually provide
> + *         "-b fill_buf").
> + *         On success, returns pointer to newly allocated and fully
> + *         initialized struct fill_buf_param that caller must free.
> + */
> +static struct fill_buf_param *alloc_fill_buf_param(struct user_params *uparams)
> +{
> +	struct fill_buf_param *fill_param = NULL;
> +	char *endptr = NULL;
> +
> +	if (!uparams->benchmark_cmd[0] || strcmp(uparams->benchmark_cmd[0], "fill_buf"))
> +		return NULL;
> +
> +	fill_param = malloc(sizeof(*fill_param));
> +	if (!fill_param)
> +		ksft_exit_skip("Unable to allocate memory for fill_buf parameters.\n");
> +
> +	if (uparams->benchmark_cmd[1]) {
> +		errno = 0;
> +		fill_param->buf_size = strtoul(uparams->benchmark_cmd[1], &endptr, 10);
> +		if (errno || uparams->benchmark_cmd[1] == endptr) {

Should this also check that there is no extra garbage?

> +			free(fill_param);
> +			ksft_exit_skip("Unable to parse benchmark buffer size.\n");
> +		}
> +	} else {
> +		fill_param->buf_size = DEFAULT_SPAN;
> +	}
> +
> +	if (uparams->benchmark_cmd[2]) {
> +		errno = 0;
> +		fill_param->memflush = strtol(uparams->benchmark_cmd[2], &endptr, 10);
> +		if (errno || uparams->benchmark_cmd[2] == endptr) {

Ditto ?

> +			free(fill_param);
> +			ksft_exit_skip("Unable to parse benchmark memflush parameter.\n");
> +		}
> +	} else {
> +		fill_param->memflush = 1;

memflush could be bool ?

> +	}
> +
> +	if (uparams->benchmark_cmd[3]) {
> +		if (strcmp(uparams->benchmark_cmd[3], "0")) {
> +			free(fill_param);
> +			ksft_exit_skip("Only read operations supported.\n");
> +		}
> +	}
> +
> +	if (uparams->benchmark_cmd[4]) {
> +		if (strcmp(uparams->benchmark_cmd[4], "false")) {
> +			free(fill_param);
> +			ksft_exit_skip("fill_buf is required to run until termination.\n");
> +		}
> +	}
> +
> +	return fill_param;
> +}
> +
>  static void init_user_params(struct user_params *uparams)
>  {
>  	memset(uparams, 0, sizeof(*uparams));
> @@ -158,11 +230,11 @@ static void init_user_params(struct user_params *uparams)
>  
>  int main(int argc, char **argv)
>  {
> +	struct fill_buf_param *fill_param = NULL;
>  	int tests = ARRAY_SIZE(resctrl_tests);
>  	bool test_param_seen = false;
>  	struct user_params uparams;
> -	char *span_str = NULL;
> -	int ret, c, i;
> +	int c, i;
>  
>  	init_user_params(&uparams);
>  
> @@ -239,6 +311,10 @@ int main(int argc, char **argv)
>  	}
>  last_arg:
>  
> +	fill_param = alloc_fill_buf_param(&uparams);
> +	if (fill_param)
> +		uparams.fill_buf = fill_param;
> +
>  	ksft_print_header();
>  
>  	/*
> @@ -257,33 +333,11 @@ int main(int argc, char **argv)
>  
>  	filter_dmesg();
>  
> -	if (!uparams.benchmark_cmd[0]) {
> -		/* If no benchmark is given by "-b" argument, use fill_buf. */
> -		uparams.benchmark_cmd[0] = "fill_buf";
> -		ret = asprintf(&span_str, "%u", DEFAULT_SPAN);
> -		if (ret < 0)
> -			ksft_exit_fail_msg("Out of memory!\n");
> -		uparams.benchmark_cmd[1] = span_str;
> -		uparams.benchmark_cmd[2] = "1";
> -		/*
> -		 * 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;
> -	}
> -
>  	ksft_set_plan(tests);
>  
>  	for (i = 0; i < ARRAY_SIZE(resctrl_tests); i++)
>  		run_single_test(resctrl_tests[i], &uparams);
>  
> -	free(span_str);
> +	free(fill_param);
>  	ksft_finished();
>  }
> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
> index a1e097dc8e05..380c3f218d01 100644
> --- a/tools/testing/selftests/resctrl/resctrl_val.c
> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
> @@ -535,6 +535,11 @@ int measure_read_mem_bw(const struct user_params *uparams,
>  	return ret;
>  }
>  
> +struct benchmark_info {
> +	const struct user_params *uparams;
> +	struct resctrl_val_param *param;
> +};
> +
>  /*
>   * run_benchmark - Run a specified benchmark or fill_buf (default benchmark)
>   *		   in specified signal. Direct benchmark stdio to /dev/null.
> @@ -544,12 +549,15 @@ int measure_read_mem_bw(const struct user_params *uparams,
>   */
>  static void run_benchmark(int signum, siginfo_t *info, void *ucontext)
>  {
> -	char **benchmark_cmd;
> -	int ret, memflush;
> -	size_t span;
> +	struct benchmark_info *benchmark_info;
> +	const struct user_params *uparams;
> +	struct resctrl_val_param *param;
>  	FILE *fp;
> +	int ret;
>  
> -	benchmark_cmd = info->si_ptr;
> +	benchmark_info = info->si_ptr;

I'd just assign this directly while defining the local variable.

-- 
 i.

> +	uparams = benchmark_info->uparams;
> +	param = benchmark_info->param;
>  
>  	/*
>  	 * Direct stdio of child to /dev/null, so that only parent writes to
> @@ -561,16 +569,13 @@ static void run_benchmark(int signum, siginfo_t *info, void *ucontext)
>  		parent_exit(ppid);
>  	}
>  
> -	if (strcmp(benchmark_cmd[0], "fill_buf") == 0) {
> -		/* Execute default fill_buf benchmark */
> -		span = strtoul(benchmark_cmd[1], NULL, 10);
> -		memflush =  atoi(benchmark_cmd[2]);
> -
> -		if (run_fill_buf(span, memflush))
> +	if (param->fill_buf) {
> +		if (run_fill_buf(param->fill_buf->buf_size,
> +				 param->fill_buf->memflush))
>  			fprintf(stderr, "Error in running fill buffer\n");
> -	} else {
> +	} else if (uparams->benchmark_cmd[0]) {
>  		/* Execute specified benchmark */
> -		ret = execvp(benchmark_cmd[0], benchmark_cmd);
> +		ret = execvp(uparams->benchmark_cmd[0], (char **)uparams->benchmark_cmd);
>  		if (ret)
>  			ksft_perror("execvp");
>  	}
> @@ -585,16 +590,15 @@ static void run_benchmark(int signum, siginfo_t *info, void *ucontext)
>   *			the benchmark
>   * @test:		test information structure
>   * @uparams:		user supplied parameters
> - * @benchmark_cmd:	benchmark command and its arguments
>   * @param:		parameters passed to resctrl_val()
>   *
>   * Return:		0 when the test was run, < 0 on error.
>   */
>  int resctrl_val(const struct resctrl_test *test,
>  		const struct user_params *uparams,
> -		const char * const *benchmark_cmd,
>  		struct resctrl_val_param *param)
>  {
> +	struct benchmark_info benchmark_info;
>  	struct sigaction sigact;
>  	int ret = 0, pipefd[2];
>  	char pipe_message = 0;
> @@ -610,6 +614,9 @@ int resctrl_val(const struct resctrl_test *test,
>  		return ret;
>  	}
>  
> +	benchmark_info.uparams = uparams;
> +	benchmark_info.param = param;
> +
>  	/*
>  	 * If benchmark wasn't successfully started by child, then child should
>  	 * kill parent, so save parent's pid
> @@ -671,13 +678,7 @@ int resctrl_val(const struct resctrl_test *test,
>  
>  	ksft_print_msg("Benchmark PID: %d\n", (int)bm_pid);
>  
> -	/*
> -	 * The cast removes constness but nothing mutates benchmark_cmd within
> -	 * the context of this process. At the receiving process, it becomes
> -	 * argv, which is mutable, on exec() but that's after fork() so it
> -	 * doesn't matter for the process running the tests.
> -	 */
> -	value.sival_ptr = (void *)benchmark_cmd;
> +	value.sival_ptr = (void *)&benchmark_info;
>  
>  	/* Taskset benchmark to specified cpu */
>  	ret = taskset_benchmark(bm_pid, uparams->cpu, NULL);
>