| Message ID | 20230418114506.46788-18-ilpo.jarvinen@linux.intel.com (mailing list archive) |
|---|---|
| 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
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. 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?
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
Hi Ilpo, > 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(-) > > 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++; > + } There may not be a case that the most-significant bit is 1 at present, but if this case exists, it cannot count correctly. Best regards, Shaopeng TAN
On Wed, 31 May 2023, Shaopeng Tan (Fujitsu) wrote: > Hi Ilpo, > > > 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> > > --- > > @@ -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++; > > + } > > There may not be a case that the most-significant bit is 1 at present, > but if this case exists, it cannot count correctly. Can you please rephrase what is your concern here please? I test 0U, 1U, ~0U, and a few more complex combinations of bits, and all returned correct count so I might not have understood what case you meant with your description. This function does not count nor calculate the most-significant bit in any phase but the longest train of bits using the count variable (and the least-significant bit of the longest train in the code that is not included into this partial snippet).
Hi Ilpo, > On Wed, 31 May 2023, Shaopeng Tan (Fujitsu) wrote: > > > Hi Ilpo, > > > > > 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> > > > --- > > > > @@ -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++; > > > + } > > > > There may not be a case that the most-significant bit is 1 at present, > > but if this case exists, it cannot count correctly. > > Can you please rephrase what is your concern here please? > > I test 0U, 1U, ~0U, and a few more complex combinations of bits, and all > returned correct count so I might not have understood what case you meant > with your description. > > This function does not count nor calculate the most-significant bit in any > phase but the longest train of bits using the count variable (and the > least-significant bit of the longest train in the code that is not included into this > partial snippet). Thanks for your explanation. I'm sorry, it was my mistake. No problem here. Best regards Shaopeng TAN
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(-)