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

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

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

On Wed, 30 Aug 2023, Reinette Chatre wrote:
> On 8/30/2023 1:59 AM, Ilpo Järvinen wrote:
> > On Tue, 29 Aug 2023, Reinette Chatre wrote:
> >> 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.
> 
> hmmm ... I do not see why an allocation forces the use of a temporary
> variable and a change in control (more below).
> 
> >> 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.
> 
> No new temporary variable is needed. Of course, I do not have insight into
> what is further down in your queue but based on this work I do think it
> can be simplified. Since code is easier to consider, the snippet below
> applies on top of this series and shows what I was proposing:

Okay, I'll use the approach you proposed. It'll prevent moving all
initialization of the default values before the argument parsing code 
which I would have wanted to do (in a patch that is not part of this 
series but it's not an end of the world).

-- 
 i.


> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
> index ae9001ef7b0a..8033eabb9aa8 100644
> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
> @@ -170,11 +170,10 @@ static void run_cat_test(int cpu_no, int no_of_bits)
>  int main(int argc, char **argv)
>  {
>  	bool mbm_test = true, mba_test = true, cmt_test = true;
> +	const char *benchmark_cmd[BENCHMARK_ARGS] = {};
>  	int c, cpu_no = 1, i, no_of_bits = 0;
> -	const char *benchmark_cmd[BENCHMARK_ARGS];
>  	char *span_str = NULL;
>  	bool cat_test = true;
> -	char *skip_reason;
>  	int tests = 0;
>  	int ret;
>  
> @@ -247,17 +246,6 @@ int main(int argc, char **argv)
>  		}
>  	}
>  
> -	/* If no benchmark is given by "-b" argument, use fill_buf. */
> -	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;
> -
>  last_arg:
>  
>  	ksft_print_header();
> @@ -267,23 +255,30 @@ int main(int argc, char **argv)
>  	 * 1. We write to resctrl FS
>  	 * 2. We execute perf commands
>  	 */
> -	if (geteuid() != 0) {
> -		skip_reason = "Not running as root. Skipping...\n";
> -		goto free_span;
> -	}
> +	if (geteuid() != 0)
> +		return ksft_exit_skip("Not running as root. Skipping...\n");
>  
> -	if (!check_resctrlfs_support()) {
> -		skip_reason = "resctrl FS does not exist. Enable X86_CPU_RESCTRL config option.\n";
> -		goto free_span;
> -	}
> +	if (!check_resctrlfs_support())
> +		return ksft_exit_skip("resctrl FS does not exist. Enable X86_CPU_RESCTRL config option.\n");
>  
> -	if (umount_resctrlfs()) {
> -		skip_reason = "resctrl FS unmount failed.\n";
> -		goto free_span;
> -	}
> +	if (umount_resctrlfs())
> +		return ksft_exit_skip("resctrl FS unmount failed.\n");
>  
>  	filter_dmesg();
>  
> +	if (!benchmark_cmd[0]) {
> +		/* If no benchmark is given by "-b" argument, use fill_buf. */
> +		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;
> +	}
> +
>  	ksft_set_plan(tests ? : 4);
>  
>  	if (mbm_test)
> @@ -300,8 +295,4 @@ int main(int argc, char **argv)
>  
>  	free(span_str);
>  	ksft_finished();
> -
> -free_span:
> -	free(span_str);
> -	return ksft_exit_skip(skip_reason);
>  }
>