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, ¶m.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, ¶m.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
