Re: [PATCH v2 16/24] selftests/resctrl: Create cache_alloc_size() helper

[Reinette Chatre]

Hi Ilpo,

On 4/24/2023 9:28 AM, Ilpo Järvinen wrote:
> 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?

This patch adds and use a utility that converts a bitmask into bytes.
I do not think it should dictate what the meaning or usage of the bitmask
is.

>>> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
>>> ---
>>>  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.

I understand. My comment was about the naming of the parameter, which can,
for example, just be "size".

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

The current motivation for this patch is to avoid duplicating code but as I
see it it introduces more duplicated code.

Reinette