Date:   Fri, 21 Apr 2023 17:19:27 -0700
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
        <linux-kselftest@...r.kernel.org>,
        Fenghua Yu <fenghua.yu@...el.com>,
        "Shuah Khan" <shuah@...nel.org>, <linux-kernel@...r.kernel.org>
CC:     Shaopeng Tan <tan.shaopeng@...fujitsu.com>
Subject: Re: [PATCH v2 16/24] selftests/resctrl: Create cache_alloc_size()
 helper

Hi Ilpo,

On 4/18/2023 4:44 AM, Ilpo Järvinen wrote:
> CAT and CMT tests calculate the span size from the n-bits cache
> allocation on their own.
> 
> Add cache_alloc_size() helper which calculates size of the cache
> allocation for the given number of bits to avoid duplicating code.

This patch is very heavy on the usage of allocation when I think it
only refers to the cache size ... how that size is used by the caller
is independent from this. 

Compare to how it sounds with some small changes to changelog:

	CAT and CMT tests calculate the span size from the capacity
	bitmask independently.
	
	Add cache_size() helper which calculates the size of the
	cache for the given number of bits to avoid duplicating code.

I think removing "alloc" helps to convey what this code actually does.

> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
> ---
>  tools/testing/selftests/resctrl/cache.c    | 27 ++++++++++++++++++++++
>  tools/testing/selftests/resctrl/cat_test.c |  8 +++++--
>  tools/testing/selftests/resctrl/cmt_test.c |  4 +++-
>  tools/testing/selftests/resctrl/resctrl.h  |  2 ++
>  4 files changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c
> index 6bc912de38be..b983af394e33 100644
> --- a/tools/testing/selftests/resctrl/cache.c
> +++ b/tools/testing/selftests/resctrl/cache.c
> @@ -15,6 +15,33 @@ static struct read_format rf_cqm;
>  static int fd_lm;
>  char llc_occup_path[1024];
>  
> +/*
> + * cache_alloc_size - Calculate alloc size for given cache alloc mask

"cache_size - Calculate number of bytes represented by bitmask" ?
Please feel free to improve.


> + * @cpu_no:		CPU number
> + * @cache_type:		Cache level L2/L3
> + * @alloc_mask:		Cache alloc mask

The description is mostly a rewrite of the variable name. Can it be
more descriptive?

> + * @alloc_size:		Alloc size returned on success

I do not think the utility should assume anything about how
the value it provides should be used. Instead it should just reflect
what the value is.

> + *
> + * Returns: 0 on success with @alloc_size filled, non-zero on error.
> + */
> +int cache_alloc_size(int cpu_no, char *cache_type, unsigned long alloc_mask,
> +		     unsigned long *alloc_size)
> +{
> +	unsigned long cache_size, full_mask;
> +	int ret;
> +
> +	ret = get_cbm_mask(cache_type, &full_mask);
> +	if (ret)
> +		return ret;
> +
> +	ret = get_cache_size(cpu_no, cache_type, &cache_size);
> +	if (ret)
> +		return ret;
> +
> +	*alloc_size = cache_size * count_bits(alloc_mask) / count_bits(full_mask);
> +	return 0;
> +}
> +
>  static void initialize_perf_event_attr(void)
>  {
>  	pea_llc_miss.type = PERF_TYPE_HARDWARE;
> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
> index 9bf5d05d9e74..d3fbd4de9f8a 100644
> --- a/tools/testing/selftests/resctrl/cat_test.c
> +++ b/tools/testing/selftests/resctrl/cat_test.c
> @@ -140,7 +140,9 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
>  	/* Set param values for parent thread which will be allocated bitmask
>  	 * with (max_bits - n) bits
>  	 */
> -	param.span = cache_size * (count_of_bits - n) / count_of_bits;
> +	ret = cache_alloc_size(cpu_no, cache_type, l_mask, &param.span);
> +	if (ret)
> +		return ret;
>  	strcpy(param.ctrlgrp, "c2");
>  	strcpy(param.mongrp, "m2");
>  	strcpy(param.filename, RESULT_FILE_NAME2);
> @@ -162,7 +164,9 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
>  		param.mask = l_mask_1;
>  		strcpy(param.ctrlgrp, "c1");
>  		strcpy(param.mongrp, "m1");
> -		param.span = cache_size * n / count_of_bits;
> +		ret = cache_alloc_size(cpu_no, cache_type, l_mask_1, &param.span);
> +		if (ret)
> +			exit(-1);
>  		strcpy(param.filename, RESULT_FILE_NAME1);
>  		param.num_of_runs = 0;
>  		param.cpu_no = sibling_cpu_no;

Did this change intend to remove the duplicate code mentioned
in the changelog? I was expecting the calls to get_cbm_mask() and
get_cache_size() within cat_perf_miss_val() to be removed.

> diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
> index ae54bbabbd91..efe77e0f1d4c 100644
> --- a/tools/testing/selftests/resctrl/cmt_test.c
> +++ b/tools/testing/selftests/resctrl/cmt_test.c
> @@ -105,10 +105,12 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd)
>  		.cpu_no		= cpu_no,
>  		.filename	= RESULT_FILE_NAME,
>  		.mask		= ~(long_mask << n) & long_mask,
> -		.span		= cache_size * n / count_of_bits,
>  		.num_of_runs	= 0,
>  		.setup		= cmt_setup,
>  	};
> +	ret = cache_alloc_size(cpu_no, "L3", param.mask, &param.span);
> +	if (ret)
> +		return ret;
>  
>  	if (strcmp(benchmark_cmd[0], "fill_buf") == 0)
>  		sprintf(benchmark_cmd[1], "%lu", param.span);

Same here regarding removal of code.

> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index bcc95faa5b4e..65425d92684e 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -108,6 +108,8 @@ int mba_schemata_change(int cpu_no, char *bw_report, char **benchmark_cmd);
>  void mba_test_cleanup(void);
>  int get_cbm_mask(char *cache_type, unsigned long *mask);
>  int get_cache_size(int cpu_no, char *cache_type, unsigned long *cache_size);
> +int cache_alloc_size(int cpu_no, char *cache_type, unsigned long alloc_mask,
> +		     unsigned long *alloc_size);
>  void ctrlc_handler(int signum, siginfo_t *info, void *ptr);
>  int signal_handler_register(void);
>  void signal_handler_unregister(void);


Reinette

Powered by blists - more mailing lists