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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <68a7ff59-289c-1d50-d454-ab2fcf6baf22@linux.intel.com>
Date: Fri, 18 Oct 2024 11:46:37 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Reinette Chatre <reinette.chatre@...el.com>
cc: fenghua.yu@...el.com, shuah@...nel.org, tony.luck@...el.com, 
    peternewman@...gle.com, babu.moger@....com, 
    Maciej Wieczór-Retman <maciej.wieczor-retman@...el.com>, 
    linux-kselftest@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V3 02/15] selftests/resctrl: Print accurate buffer size
 as part of MBM results

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@...el.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");
>  
>