[v2,17/24] selftests/resctrl: Replace count_bits with count_consecutive_bits()

  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(-)
  

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