Received-SPF: pass (google.com: domain of linux-kernel+bounces-97142-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Date: Fri, 8 Mar 2024 16:07:05 +0200 (EET)
To: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
cc: Fenghua Yu <fenghua.yu@intel.com>, 
    Reinette Chatre <reinette.chatre@intel.com>, Shuah Khan <shuah@kernel.org>, 
    tony.luck@intel.com, "Shaopeng Tan (Fujitsu)" <tan.shaopeng@fujitsu.com>, 
    LKML <linux-kernel@vger.kernel.org>, linux-kselftest@vger.kernel.org
Subject: Re: [PATCH 3/4] selftests/resctrl: SNC support for MBM
In-Reply-To: <def02de22a4747ed56ddc28f334e55872ba95f13.1709721159.git.maciej.wieczor-retman@intel.com>
Message-ID: <37bd082c-a187-43de-892b-c5fa32a8b3f2@linux.intel.com>
References: <cover.1709721159.git.maciej.wieczor-retman@intel.com> <def02de22a4747ed56ddc28f334e55872ba95f13.1709721159.git.maciej.wieczor-retman@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII

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)" <tan.shaopeng@fujitsu.com>
> Closes: https://lore.kernel.org/lkml/TYAPR01MB6330A4EB3633B791939EA45E8B39A@TYAPR01MB6330.jpnprd01.prod.outlook.com/
> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
> ---
>  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;
>  }