On Wed, 6 Mar 2024, Maciej Wieczor-Retman wrote:
> Memory Bandwidth Monitoring (MBM) measures how much data flows between
> cache levels. Only the flow for a process specified with a Resource
> Monitoring ID (RMID) is measured.
>
> Sub-Numa Clustering (SNC) causes MBM selftest to fail because the
> increased amount of NUMA nodes per socket is not taken into account.
> That in turn makes the test use incorrect values for the start and end
> of the measurement by tracking the wrong node.
>
> For the MBM selftest to be NUMA-aware it needs to track the start and end
> values of a measurement not for a single node but for all nodes on the
> same socket. Then summing all measured values comes out as the real
> bandwidth used by the process.
>
> Reported-by: "Shaopeng Tan (Fujitsu)" <[email protected]>
> Closes:
> https://lore.kernel.org/lkml/tyapr01mb6330a4eb3633b791939ea45e8b...@tyapr01mb6330.jpnprd01.prod.outlook.com/
> Signed-off-by: Maciej Wieczor-Retman <[email protected]>
> ---
> tools/testing/selftests/resctrl/mba_test.c | 1 -
> tools/testing/selftests/resctrl/resctrl_val.c | 37 ++++++++++++-------
> 2 files changed, 23 insertions(+), 15 deletions(-)
>
> diff --git a/tools/testing/selftests/resctrl/mba_test.c
> b/tools/testing/selftests/resctrl/mba_test.c
> index 7946e32e85c8..fc31a61dab0c 100644
> --- a/tools/testing/selftests/resctrl/mba_test.c
> +++ b/tools/testing/selftests/resctrl/mba_test.c
> @@ -147,7 +147,6 @@ static int mba_run_test(const struct resctrl_test *test,
> const struct user_param
> struct resctrl_val_param param = {
> .resctrl_val = MBA_STR,
> .ctrlgrp = "c1",
> - .mongrp = "m1",
> .filename = RESULT_FILE_NAME,
> .bw_report = "reads",
> .setup = mba_setup
> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c
> b/tools/testing/selftests/resctrl/resctrl_val.c
> index e75e3923ebe2..2fe9f8bb4a45 100644
> --- a/tools/testing/selftests/resctrl/resctrl_val.c
> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
> @@ -595,9 +595,10 @@ static void initialize_llc_occu_resctrl(const char
> *ctrlgrp, const char *mongrp,
>
> static int measure_vals(const struct user_params *uparams,
> struct resctrl_val_param *param,
> - unsigned long *bw_resc_start)
> + unsigned long *bw_resc_start,
> + int res_id)
> {
> - unsigned long bw_resc, bw_resc_end;
> + unsigned long bw_resc = 0, bw_resc_sum = 0, bw_resc_end;
> float bw_imc;
> int ret;
>
> @@ -612,17 +613,19 @@ static int measure_vals(const struct user_params
> *uparams,
> if (ret < 0)
> return ret;
>
> - ret = get_mem_bw_resctrl(&bw_resc_end);
> - if (ret < 0)
> - return ret;
> + for (int i = 0 ; i < snc_ways() ; i++) {
> + set_mbm_path(param->ctrlgrp, strlen(param->mongrp) > 0 ?
> param->mongrp : NULL,
> + res_id + i);
> + ret = get_mem_bw_resctrl(&bw_resc_end);
> + bw_resc = (bw_resc_end - bw_resc_start[i]) / MB;
> + bw_resc_sum += bw_resc;
> + bw_resc_start[i] = bw_resc_end;
> + }
>
> - bw_resc = (bw_resc_end - *bw_resc_start) / MB;
> - ret = print_results_bw(param->filename, bm_pid, bw_imc, bw_resc);
> + ret = print_results_bw(param->filename, bm_pid, bw_imc, bw_resc_sum);
> if (ret)
> return ret;
>
> - *bw_resc_start = bw_resc_end;
> -
> return 0;
> }
>
> @@ -697,12 +700,16 @@ int resctrl_val(const struct resctrl_test *test,
> struct resctrl_val_param *param)
> {
> char *resctrl_val = param->resctrl_val;
> - unsigned long bw_resc_start = 0;
> int res_id, ret = 0, pipefd[2];
> + unsigned long *bw_resc_start;
> struct sigaction sigact;
> char pipe_message = 0;
> union sigval value;
>
> + bw_resc_start = calloc(snc_ways(), sizeof(unsigned long));
While correct, this seems a bit overkill given is MAX_SNC = 4, not
something large or unbounded.
This patch would be be much simpler on top of my resctrl_val() cleanup
patches because bw_resc_start is only local to the measurement function.
--
i.
> + if (!bw_resc_start)
> + return -1;
> +
> if (strcmp(param->filename, "") == 0)
> sprintf(param->filename, "stdio");
>
> @@ -710,7 +717,7 @@ int resctrl_val(const struct resctrl_test *test,
> !strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR))) {
> ret = validate_bw_report_request(param->bw_report);
> if (ret)
> - return ret;
> + goto out_free;
> }
>
> /*
> @@ -721,7 +728,7 @@ int resctrl_val(const struct resctrl_test *test,
>
> if (pipe(pipefd)) {
> ksft_perror("Unable to create pipe");
> -
> + free(bw_resc_start);
> return -1;
> }
>
> @@ -733,7 +740,7 @@ int resctrl_val(const struct resctrl_test *test,
> bm_pid = fork();
> if (bm_pid == -1) {
> ksft_perror("Unable to fork");
> -
> + free(bw_resc_start);
> return -1;
> }
>
> @@ -841,7 +848,7 @@ int resctrl_val(const struct resctrl_test *test,
>
> if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) ||
> !strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) {
> - ret = measure_vals(uparams, param, &bw_resc_start);
> + ret = measure_vals(uparams, param, bw_resc_start,
> res_id);
> if (ret)
> break;
> } else if (!strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR))) {
> @@ -855,6 +862,8 @@ int resctrl_val(const struct resctrl_test *test,
>
> out:
> kill(bm_pid, SIGKILL);
> +out_free:
> + free(bw_resc_start);
>
> return ret;
> }
