Re: [PATCH v3 5/7] selftests/resctrl: Make benchmark command const and build it with pointers

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Ilpo,

On 8/23/2023 6:15 AM, Ilpo Järvinen wrote:
...
> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index bcd0d2060f81..32d23e665697 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>

What does this header provide?

>  #include <stdlib.h>
>  #include <unistd.h>
>  #include <string.h>
> @@ -38,6 +39,8 @@
>  
>  #define END_OF_TESTS	1
>  
> +#define BENCHMARK_ARGS		64
> +
>  #define DEFAULT_SPAN		(250 * MB)
>  
>  #define PARENT_EXIT(err_msg)			\
> @@ -97,11 +100,11 @@ int write_bm_pid_to_resctrl(pid_t bm_pid, char *ctrlgrp, char *mongrp,
>  int perf_event_open(struct perf_event_attr *hw_event, pid_t pid, int cpu,
>  		    int group_fd, unsigned long flags);
>  int run_fill_buf(size_t span, int memflush, int op, bool once);
> -int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param);
> -int mbm_bw_change(int cpu_no, char **benchmark_cmd);
> +int resctrl_val(const char * const *benchmark_cmd, struct resctrl_val_param *param);
> +int mbm_bw_change(int cpu_no, const char * const *benchmark_cmd);
>  void tests_cleanup(void);
>  void mbm_test_cleanup(void);
> -int mba_schemata_change(int cpu_no, char **benchmark_cmd);
> +int mba_schemata_change(int cpu_no, const char *const *benchmark_cmd);

Could you please use consistent spacing ("char * const" vs "char *const")?

>  void mba_test_cleanup(void);
>  int get_cbm_mask(char *cache_type, char *cbm_mask);
>  int get_cache_size(int cpu_no, char *cache_type, unsigned long *cache_size);
> @@ -111,7 +114,7 @@ void signal_handler_unregister(void);
>  int cat_val(struct resctrl_val_param *param, size_t span);
>  void cat_test_cleanup(void);
>  int cat_perf_miss_val(int cpu_no, int no_of_bits, char *cache_type);
> -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);
>  unsigned int count_bits(unsigned long n);
>  void cmt_test_cleanup(void);
>  int get_core_sibling(int cpu_no);
> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
> index d4ce8592466c..84a37bf67306 100644
> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
> @@ -10,9 +10,6 @@
>   */
>  #include "resctrl.h"
>  
> -#define BENCHMARK_ARGS		64
> -#define BENCHMARK_ARG_SIZE	64
> -
>  static int detect_vendor(void)
>  {
>  	FILE *inf = fopen("/proc/cpuinfo", "r");
> @@ -70,7 +67,7 @@ void tests_cleanup(void)
>  	cat_test_cleanup();
>  }
>  
> -static void run_mbm_test(char **benchmark_cmd, int cpu_no)
> +static void run_mbm_test(const char **benchmark_cmd, int cpu_no)
>  {
>  	int res;
>  
> @@ -96,7 +93,7 @@ static void run_mbm_test(char **benchmark_cmd, int cpu_no)
>  	umount_resctrlfs();
>  }
>  
> -static void run_mba_test(char **benchmark_cmd, int cpu_no)
> +static void run_mba_test(const char **benchmark_cmd, int cpu_no)
>  {
>  	int res;
>  
> @@ -120,7 +117,7 @@ static void run_mba_test(char **benchmark_cmd, int cpu_no)
>  	umount_resctrlfs();
>  }
>  
> -static void run_cmt_test(char **benchmark_cmd, int cpu_no)
> +static void run_cmt_test(const char **benchmark_cmd, int cpu_no)
>  {
>  	int res;
>  

Could you please elaborate why the above functions have
"const char **" instead of "const char * const *"?

> @@ -173,11 +170,13 @@ static void run_cat_test(int cpu_no, int no_of_bits)
>  int main(int argc, char **argv)
>  {
>  	bool has_ben = false, mbm_test = true, mba_test = true, cmt_test = true;
> -	char benchmark_cmd_area[BENCHMARK_ARGS][BENCHMARK_ARG_SIZE];
>  	int c, cpu_no = 1, argc_new = argc, i, no_of_bits = 0;
> -	char *benchmark_cmd[BENCHMARK_ARGS];
> +	const char *benchmark_cmd[BENCHMARK_ARGS];
>  	int ben_ind, ben_count, tests = 0;
> +	char *span_str = NULL;
>  	bool cat_test = true;
> +	char *skip_reason;
> +	int ret;
>  
>  	for (i = 0; i < argc; i++) {
>  		if (strcmp(argv[i], "-b") == 0) {
> @@ -257,31 +256,31 @@ int main(int argc, char **argv)
>  			ksft_exit_fail_msg("Too long benchmark command.\n");
>  
>  		/* Extract benchmark command from command line. */
> -		for (i = ben_ind; i < argc; i++) {
> -			benchmark_cmd[i - ben_ind] = benchmark_cmd_area[i];
> -			if (strlen(argv[i]) >= BENCHMARK_ARG_SIZE - 1)
> -				ksft_exit_fail_msg("Too long benchmark command argument.\n");
> -			sprintf(benchmark_cmd[i - ben_ind], "%s", argv[i]);
> -		}
> +		for (i = 0; i < argc - ben_ind; i++)
> +			benchmark_cmd[i] = argv[i + ben_ind];
>  		benchmark_cmd[ben_count] = NULL;
>  	} else {
>  		/* If no benchmark is given by "-b" argument, use fill_buf. */
> -		for (i = 0; i < 5; i++)
> -			benchmark_cmd[i] = benchmark_cmd_area[i];
> -
> -		strcpy(benchmark_cmd[0], "fill_buf");
> -		sprintf(benchmark_cmd[1], "%u", DEFAULT_SPAN);
> -		strcpy(benchmark_cmd[2], "1");
> -		strcpy(benchmark_cmd[3], "0");
> -		strcpy(benchmark_cmd[4], "false");
> +		benchmark_cmd[0] = "fill_buf";
> +		ret = asprintf(&span_str, "%u", DEFAULT_SPAN);
> +		if (ret < 0)
> +			ksft_exit_fail_msg("Out of memory!\n");
> +		benchmark_cmd[1] = span_str;
> +		benchmark_cmd[2] = "1";
> +		benchmark_cmd[3] = "0";
> +		benchmark_cmd[4] = "false";
>  		benchmark_cmd[5] = NULL;
>  	}
>  
> -	if (!check_resctrlfs_support())
> -		return ksft_exit_skip("resctrl FS does not exist. Enable X86_CPU_RESCTRL config option.\n");
> +	if (!check_resctrlfs_support()) {
> +		skip_reason = "resctrl FS does not exist. Enable X86_CPU_RESCTRL config option.\n";
> +		goto free_span;
> +	}
>  
> -	if (umount_resctrlfs())
> -		return ksft_exit_skip("resctrl FS unmount failed.\n");
> +	if (umount_resctrlfs()) {
> +		skip_reason = "resctrl FS unmount failed.\n";
> +		goto free_span;
> +	}
>  
>  	filter_dmesg();
>  
> @@ -299,5 +298,10 @@ int main(int argc, char **argv)
>  	if (cat_test)
>  		run_cat_test(cpu_no, no_of_bits);
>  
> +	free(span_str);
>  	ksft_finished();
> +
> +free_span:
> +	free(span_str);
> +	return ksft_exit_skip(skip_reason);
>  }

This is a tricky one. If I understand correctly this goto target makes
some assumptions about the state (no test plan created yet) and exit
reason (it has to be skipped). A temporary variable is also thrown into
the mix. Can this not be simplified by moving the snippet where
benchmark_cmd[] is initialized to fill_buf to be just before the tests are run?
Perhaps right before ksft_set_plan()? This may be an easier move to consider
when the changes in patch 7 are taken into account.

Reinette