Re: [PATCH 5/7] selftests/resctrl: Use pointers to build benchmark cmd and make it const

[Reinette Chatre]

Hi Ilpo,

On 8/8/2023 2:16 AM, Ilpo Järvinen wrote:
> Benchmark parameter uses fixed-size buffers in stack which is slightly
> dangerous. As benchmark command is used in multiple tests, it should

Could you please be specific with issues with current implementation?
The term "slightly dangerous" is vague.

> not be mutated by the tests. Due to the order of tests, mutating the
> span argument in CMT test does not trigger any real problems currently.
> 
> Mark benchmark_cmd strings as const and setup the benchmark command
> using pointers. As span is constant in main(), just provide the default
> span also as string to be used in setting up the default fill_buf
> argument so no malloc() is required for it.

What is wrong with using malloc()?

> 
> CMT test has to create a copy of the benchmark command before altering
> the benchmark command.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> ---
>  tools/testing/selftests/resctrl/cmt_test.c    | 23 ++++++++++---
>  tools/testing/selftests/resctrl/mba_test.c    |  2 +-
>  tools/testing/selftests/resctrl/mbm_test.c    |  2 +-
>  tools/testing/selftests/resctrl/resctrl.h     | 16 ++++++---
>  .../testing/selftests/resctrl/resctrl_tests.c | 33 ++++++++-----------
>  tools/testing/selftests/resctrl/resctrl_val.c | 10 ++++--
>  6 files changed, 54 insertions(+), 32 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
> index 9d8e38e995ef..a40e12c3b1a7 100644
> --- a/tools/testing/selftests/resctrl/cmt_test.c
> +++ b/tools/testing/selftests/resctrl/cmt_test.c
> @@ -68,14 +68,16 @@ void cmt_test_cleanup(void)
>  	remove(RESULT_FILE_NAME);
>  }
>  
> -int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd)
> +int cmt_resctrl_val(int cpu_no, int n, const char * const *benchmark_cmd)
>  {
> +	const char *cmd[BENCHMARK_ARGS];
>  	unsigned long cache_size = 0;
>  	unsigned long long_mask;
> +	char *span_str = NULL;
>  	char cbm_mask[256];
>  	int count_of_bits;
>  	size_t span;
> -	int ret;
> +	int ret, i;
>  
>  	if (!validate_resctrl_feature_request(CMT_STR))
>  		return -1;
> @@ -111,12 +113,22 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd)
>  	};
>  
>  	span = cache_size * n / count_of_bits;
> -	if (strcmp(benchmark_cmd[0], "fill_buf") == 0)
> -		sprintf(benchmark_cmd[1], "%zu", span);
> +	/* Duplicate the command to be able to replace span in it */
> +	for (i = 0; benchmark_cmd[i]; i++)
> +		cmd[i] = benchmark_cmd[i];
> +	cmd[i] = NULL;
> +
> +	if (strcmp(cmd[0], "fill_buf") == 0) {
> +		span_str = malloc(SIZE_MAX_DECIMAL_SIZE);
> +		if (!span_str)
> +			return -1;
> +		snprintf(span_str, SIZE_MAX_DECIMAL_SIZE, "%zu", span);

Have you considered asprintf()?

> +		cmd[1] = span_str;
> +	}

It looks to me that array only needs to be duplicated if the
default benchmark is used?

>  
>  	remove(RESULT_FILE_NAME);
>  
> -	ret = resctrl_val(benchmark_cmd, &param);
> +	ret = resctrl_val(cmd, &param);
>  	if (ret)
>  		goto out;
>  

...

> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index bcd0d2060f81..ddb1e83a3a64 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -6,6 +6,7 @@
>  #include <math.h>
>  #include <errno.h>
>  #include <sched.h>
> +#include <stdint.h>
>  #include <stdlib.h>
>  #include <unistd.h>
>  #include <string.h>
> @@ -38,7 +39,14 @@
>  
>  #define END_OF_TESTS	1
>  
> +#define BENCHMARK_ARGS		64
> +
> +/* Approximate %zu max length */
> +#define SIZE_MAX_DECIMAL_SIZE	(sizeof(SIZE_MAX) * 8 / 3 + 2)
> +
> +/* Define default span both as integer and string, these should match */
>  #define DEFAULT_SPAN		(250 * MB)
> +#define DEFAULT_SPAN_STR	"262144000"

I think above hardcoding can be eliminated by using asprintf()? This
does allocate memory though so I would like to understand why one
goal is to not dynamically allocate memory.

>  
>  #define PARENT_EXIT(err_msg)			\
>  	do {					\

Reinette