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@linux.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.
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;@@ -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);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