Message-ID: <5db5680-4123-17b8-e657-39ba20356259@linux.intel.com>
Date:   Mon, 24 Apr 2023 19:28:40 +0300 (EEST)
From:   Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To:     Reinette Chatre <reinette.chatre@...el.com>
cc:     linux-kselftest@...r.kernel.org, Fenghua Yu <fenghua.yu@...el.com>,
        Shuah Khan <shuah@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Shaopeng Tan <tan.shaopeng@...fujitsu.com>
Subject: Re: [PATCH v2 16/24] selftests/resctrl: Create cache_alloc_size()
 helper

On Fri, 21 Apr 2023, Reinette Chatre wrote:

> 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.

Does it? Without something to indicate its not the full cache size, 
there's possiblity for confusion. While the tests are mostly interested 
in the allocated size, the full cache size is also collected (solely for 
printing it out, IIRC). Maybe I should rename those variable to 
total_cache_size or something like that to mitigate the confusion?

> > 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.

I was just referring to that the value is filled only on success.

> > + * 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?

It removes n CBM bits -> cache size calculations by collecting the 
calculation into one place.

cache_alloc_size() takes mask instead of n (CBM bits) as input which makes 
things easier down the line when the new CAT test starts to tweak the 
alloc size. The new CAT test would otherwise need to track both the mask 
and n.

cache_alloc_size() is independent of what caller requires so the full mask 
is not passed from the caller.

> I was expecting the calls to get_cbm_mask() and get_cache_size() within 
> cat_perf_miss_val() to be removed.

I would have wanted to remove get_cache_size() but it would mean removing 
cache size print or moving it to elsewhere.

get_cbm_mask() cannot be removed as it's used by the test to calculate the 
mask the test wants (but it no longer has to determine the size itself but 
uses this new helper instead).

I can try to amend the changelog to explain things better.

> > 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
> 

-- 
 i.

Powered by blists - more mailing lists