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, ¶m);
> 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");
>
>