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

[Ilpo Järvinen]

On Tue, 29 Aug 2023, Reinette Chatre wrote:
> 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
> > @@ -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")?
>
> > @@ -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 *"?

Thanks for the review!

Sure. I'll make them consistent.

> > @@ -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.

So in the end the symmetry proved to be not as simple as was depicted 
earlier but "tricky"... I tried to warn about this and it's why I wished 
to avoid the allocation entirely. Without allocation, there would have not 
been need for the temporary variable nor adjusting the control flow with 
that label.

> 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()?

So I throw a temporary variable into the mix (has_ben) to keep track when 
benchmark_cmd needs to be initialized to the default command? It doesn't 
play well with what I've in queue after this when user parameters are 
collected into a struct which is initialized to default value by a helper 
function before any argument processing. That is, initializing the 
parameters to defaults needs to be split before and after the parameter 
parsing code.

> This may be an easier move to consider
> when the changes in patch 7 are taken into account.

Perhaps you could consider accepting my earlier approach which avoided 
the allocation entirely now that you've seen where this leads to? ...At 
least you should understand my reasoning for that on much deeper level 
now.


-- 
 i.