| Message ID | 20230418114506.46788-18-ilpo.jarvinen@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/resctrl: Fixes, cleanups, and rewritten CAT test | expand |
Hi Ilpo, On 4/18/2023 4:44 AM, Ilpo Järvinen wrote: > CAT and CMT tests depends on masks being continuous. The term used in the spec is "contiguous", using it here may help to convey the goal. > > Replace count_bits with more appropriate variant that counts > consecutive bits. Could you please elaborate why this is more appropriate and why this is necessary? What is wrong with current solution? Please note that cbm_mask in resctrl will always be contiguous. Reinette
Hi Ilpo, On 4/25/2023 4:41 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 depends on masks being continuous. >> >> The term used in the spec is "contiguous", using it here >> may help to convey the goal. >> >>> >>> Replace count_bits with more appropriate variant that counts >>> consecutive bits. >> >> Could you please elaborate why this is more appropriate and >> why this is necessary? What is wrong with current solution? >> >> Please note that cbm_mask in resctrl will always be contiguous. > > Hi, > > It's good that you asked this question. > > This comes from a change (not by me originally) that also excluded the > shareable bits from the mask the CAT test uses. I assumed (w/o better > knowledge) that those shareable bits could create a hole into the mask. You are correct. Shareable bits can create a hole in the mask. > > It could be wrong assumption and the shareable bits are always at one end > of the CBM mask. > > Do you happen to know how the shareable bits are positioned within the > mask? This depends on the hardware. Software learns about it via a bitmask (as opposed to "number of bits") so the hardware has flexibility to communicate any combination of shared ways. These comments are not related to this patch though. I understand that this patch has been created in support of the changes that follow. My questions are related to this patch that communicates that it is "more appropriate" for what the code currently does without consideration of what is to come. I would like to understand how this is more appropriate. Also note that cbm_mask will always be contiguous - in this case the hardware communicates a number of bits, not a bitmask, so this will always be contiguous. This patch claims that the current solution is not appropriate to parse cbm_mask, could you please elaborate why? Reinette
diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c index d3fbd4de9f8a..a1834dd5ad9a 100644 --- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -78,7 +78,7 @@ static int check_results(struct resctrl_val_param *param) } fclose(fp); - no_of_bits = count_bits(param->mask); + no_of_bits = count_consecutive_bits(param->mask, NULL); return show_cache_info(sum_llc_perf_miss, no_of_bits, param->span / 64, MAX_DIFF, MAX_DIFF_PERCENT, NUM_OF_RUNS, @@ -103,6 +103,7 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type) ret = get_cbm_mask(cache_type, &long_mask); if (ret) return ret; + count_of_bits = count_consecutive_bits(long_mask, NULL); /* Get L3/L2 cache size */ ret = get_cache_size(cpu_no, cache_type, &cache_size); @@ -110,9 +111,6 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type) return ret; ksft_print_msg("Cache size :%lu\n", cache_size); - /* Get max number of bits from default-cabm mask */ - count_of_bits = count_bits(long_mask); - if (!n) n = count_of_bits / 2; diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c index efe77e0f1d4c..98e7d3accd73 100644 --- a/tools/testing/selftests/resctrl/cmt_test.c +++ b/tools/testing/selftests/resctrl/cmt_test.c @@ -84,14 +84,13 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd) ret = get_cbm_mask("L3", &long_mask); if (ret) return ret; + count_of_bits = count_consecutive_bits(long_mask, NULL); ret = get_cache_size(cpu_no, "L3", &cache_size); if (ret) return ret; ksft_print_msg("Cache size :%lu\n", cache_size); - count_of_bits = count_bits(long_mask); - if (n < 1 || n > count_of_bits) { ksft_print_msg("Invalid input value for numbr_of_bits n!\n"); ksft_print_msg("Please enter value in range 1 to %d\n", count_of_bits); diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index 65425d92684e..aa5dc8b95a06 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -106,6 +106,7 @@ void tests_cleanup(void); void mbm_test_cleanup(void); int mba_schemata_change(int cpu_no, char *bw_report, char **benchmark_cmd); void mba_test_cleanup(void); +unsigned int count_consecutive_bits(unsigned long val, unsigned int *start); 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, diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c index f01ecfa64063..4efaf69c8152 100644 --- a/tools/testing/selftests/resctrl/resctrlfs.c +++ b/tools/testing/selftests/resctrl/resctrlfs.c @@ -10,6 +10,8 @@ */ #include "resctrl.h" +#include <strings.h> + static int find_resctrl_mount(char *buffer) { FILE *mounts; @@ -218,6 +220,34 @@ static int get_bit_mask(char *filename, unsigned long *mask) return 0; } +/* + * count_consecutive_bits - Returns the longest train of bits in a bit mask + * @val A bit mask + * @start The location of the least-significant bit of the longest train + * + * Return: The length of the consecutive bits in the longest train of bits + */ +unsigned int count_consecutive_bits(unsigned long val, unsigned int *start) +{ + unsigned long last_val; + int count = 0; + + while (val) { + last_val = val; + val &= (val >> 1); + count++; + } + + if (start) { + if (count) + *start = ffsl(last_val) - 1; + else + *start = 0; + } + + return count; +} + /* * get_cbm_bits - Get number of bits in cbm mask * @cache_type: Cache level L2/L3
CAT and CMT tests depends on masks being continuous. Replace count_bits with more appropriate variant that counts consecutive bits. Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> --- tools/testing/selftests/resctrl/cat_test.c | 6 ++--- tools/testing/selftests/resctrl/cmt_test.c | 3 +-- tools/testing/selftests/resctrl/resctrl.h | 1 + tools/testing/selftests/resctrl/resctrlfs.c | 30 +++++++++++++++++++++ 4 files changed, 34 insertions(+), 6 deletions(-)