Re: [PATCH V3 02/15] selftests/resctrl: Print accurate buffer size as part of MBM results

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

On Thu, 17 Oct 2024, Reinette Chatre wrote:

> By default the MBM test uses the "fill_buf" benchmark to keep reading
> from a buffer with size DEFAULT_SPAN while measuring memory bandwidth.
> User space can provide an alternate benchmark or amend the size of
> the buffer "fill_buf" should use.
> 
> Analysis of the MBM measurements do not require that a buffer be used
> and thus do not require knowing the size of the buffer if it was used
> during testing. Even so, the buffer size is printed as informational
> as part of the MBM test results. What is printed as buffer size is
> hardcoded as DEFAULT_SPAN, even if the test relied on another benchmark
> (that may or may not use a buffer) or if user space amended the buffer
> size.
> 
> Ensure that accurate buffer size is printed when using "fill_buf"
> benchmark and omit the buffer size information if another benchmark
> is used.
> 
> Fixes: ecdbb911f22d ("selftests/resctrl: Add MBM test")
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> ---
> Backporting is not recommended. Backporting this fix will be
> a challenge with all the refactoring done since then. This issue
> does not impact default tests and there is no sign that
> folks run these tests with anything but the defaults. This issue is
> also minor since it does not impact actual test runs or results,
> just the information printed during a test run.
> 
> Changes since V2:
> - Make user input checks more robust. (Ilpo)
> 
> Changes since V1:
> - New patch.
> ---
>  tools/testing/selftests/resctrl/mbm_test.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
> index 6b5a3b52d861..36ae29a03784 100644
> --- a/tools/testing/selftests/resctrl/mbm_test.c
> +++ b/tools/testing/selftests/resctrl/mbm_test.c
> @@ -40,7 +40,8 @@ show_bw_info(unsigned long *bw_imc, unsigned long *bw_resc, size_t span)
>  	ksft_print_msg("%s Check MBM diff within %d%%\n",
>  		       ret ? "Fail:" : "Pass:", MAX_DIFF_PERCENT);
>  	ksft_print_msg("avg_diff_per: %d%%\n", avg_diff_per);
> -	ksft_print_msg("Span (MB): %zu\n", span / MB);
> +	if (span)
> +		ksft_print_msg("Span (MB): %zu\n", span / MB);
>  	ksft_print_msg("avg_bw_imc: %lu\n", avg_bw_imc);
>  	ksft_print_msg("avg_bw_resc: %lu\n", avg_bw_resc);
>  
> @@ -138,15 +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;
>  	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 || *endptr != '\0')

This no longer catches "" string as error. I tested strtoul() with an 
empty string and errno remains at 0.

> +				return -errno;

Another issue is that in cases where errno=0 (both *endptr != '\0' and 
endptr == uparams->benchmark_cmd[1]), this function doesn't return 
a proper error code but -0.

-- 
 i.

> +		}
> +	}
> +
>  	ret = resctrl_val(test, uparams, uparams->benchmark_cmd, &param);
>  	if (ret)
>  		return ret;
>  
> -	ret = check_results(DEFAULT_SPAN);
> +	ret = check_results(span);
>  	if (ret && (get_vendor() == ARCH_INTEL))
>  		ksft_print_msg("Intel MBM may be inaccurate when Sub-NUMA Clustering is enabled. Check BIOS configuration.\n");
>  
>