Re: [PATCH 2/4] selftests/resctrl: SNC support for CMT

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

On Wed, 6 Mar 2024, Maciej Wieczor-Retman wrote:

> Cache Monitoring Technology (CMT) works by measuring how much data in L3
> cache is occupied by a given process identified by its Resource
> Monitoring ID (RMID).
> 
> On systems with Sub-Numa Clusters (SNC) enabled, a process can occupy
> not only the cache that belongs to its own NUMA node but also pieces of
> other NUMA nodes' caches that lie on the same socket.
> 
> A simple correction to make the CMT selftest NUMA-aware is to sum values
> reported by all nodes on the same socket for a given RMID.
> 
> Reported-by: "Shaopeng Tan (Fujitsu)" <tan.shaopeng@fujitsu.com>
> Closes: https://lore.kernel.org/all/TYAPR01MB6330B9B17686EF426D2C3F308B25A@TYAPR01MB6330.jpnprd01.prod.outlook.com/
> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
> ---
>  tools/testing/selftests/resctrl/cache.c       | 17 +++++++++++------
>  tools/testing/selftests/resctrl/resctrl.h     |  4 +++-
>  tools/testing/selftests/resctrl/resctrl_val.c |  9 ++++++---
>  3 files changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c
> index 1b339d6bbff1..dab81920033b 100644
> --- a/tools/testing/selftests/resctrl/cache.c
> +++ b/tools/testing/selftests/resctrl/cache.c
> @@ -161,16 +161,21 @@ int perf_event_measure(int pe_fd, struct perf_event_read *pe_read,
>   *
>   * Return: =0 on success. <0 on failure.
>   */
> -int measure_llc_resctrl(const char *filename, int bm_pid)
> +int measure_llc_resctrl(const char *filename, int bm_pid, const char *ctrlgrp,
> +			const char *mongrp, int res_id)
>  {
> -	unsigned long llc_occu_resc = 0;
> +	unsigned long sum = 0, llc_occu_resc = 0;
>  	int ret;
>  
> -	ret = get_llc_occu_resctrl(&llc_occu_resc);
> -	if (ret < 0)
> -		return ret;
> +	for (int i = 0 ; i < snc_ways() ; i++) {

Spaces as per usual coding style:

	for (int i = 0; i < snc_ways(); i++) {

> +		set_cmt_path(ctrlgrp, mongrp, res_id + i);
> +		ret = get_llc_occu_resctrl(&llc_occu_resc);
> +		if (ret < 0)
> +			return ret;
> +		sum += llc_occu_resc;
> +	}
>  
> -	return print_results_cache(filename, bm_pid, llc_occu_resc);
> +	return print_results_cache(filename, bm_pid, sum);
>  }
>  
>  /*

> @@ -828,6 +828,8 @@ int resctrl_val(const struct resctrl_test *test,
>  	sleep(1);
>  
>  	/* Test runs until the callback setup() tells the test to stop. */
> +	get_domain_id("L3", uparams->cpu, &res_id);

Hardcoding L3 here limits the genericness of this function. You don't even 
need to do it, get_domain_id() does "MB" -> "L3" transformation implicitly 
for you so you can just pass test->resource instead.

Also, I don't understand why you now again make the naming inconsistent 
with "res_id".

If you based this on top of the patches I just posted, resctl_val() 
already the domain_id variable.

-- 
 i.

> +	res_id *= snc_ways();
>  	while (1) {
>  		ret = param->setup(test, uparams, param);
>  		if (ret == END_OF_TESTS) {
> @@ -844,7 +846,8 @@ int resctrl_val(const struct resctrl_test *test,
>  				break;
>  		} else if (!strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR))) {
>  			sleep(1);
> -			ret = measure_llc_resctrl(param->filename, bm_pid);
> +			ret = measure_llc_resctrl(param->filename, bm_pid, param->ctrlgrp,
> +						  param->mongrp, res_id);
>  			if (ret)
>  				break;
>  		}
>