RE: [PATCH v2 15/24] selftests/resctrl: Refactor get_cbm_mask()

[Ilpo Järvinen]

On Wed, 31 May 2023, Shaopeng Tan (Fujitsu) wrote:

> Hi Ilpo,
> 
> > Callers of get_cbm_mask() are required to pass a string into which the CBM bit
> > mask is read into. Neither CAT nor CMT tests need the mask as string but just
> > convert it into an unsigned long value.
> > 
> > The bit mask reader can only read .../cbm_mask files.
> > 
> > Generalize the bit mask reading function into get_bit_mask() such that it can
> > be used to handle other files besides the .../cbm_mask and handle the
> > unsigned long conversion within within get_bit_mask() using fscanf(). Alter
> > get_cbm_mask() to construct the filename for get_bit_mask().
> > 
> > Co-developed-by: Fenghua Yu <fenghua.yu@xxxxxxxxx>
> > Signed-off-by: Fenghua Yu <fenghua.yu@xxxxxxxxx>
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> > ---
> >  tools/testing/selftests/resctrl/cat_test.c  |  5 +--
> > tools/testing/selftests/resctrl/cmt_test.c  |  5 +--
> >  tools/testing/selftests/resctrl/resctrl.h   |  2 +-
> >  tools/testing/selftests/resctrl/resctrlfs.c | 50 +++++++++++++++------
> >  4 files changed, 40 insertions(+), 22 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/resctrl/cat_test.c
> > b/tools/testing/selftests/resctrl/cat_test.c
> > index a998e6397518..9bf5d05d9e74 100644
> > --- a/tools/testing/selftests/resctrl/cat_test.c
> > +++ b/tools/testing/selftests/resctrl/cat_test.c
> > @@ -18,7 +18,6 @@
> >  #define MAX_DIFF		1000000
> > 
> >  static int count_of_bits;
> > -static char cbm_mask[256];
> >  static unsigned long long_mask;
> >  static unsigned long cache_size;
> > 
> > @@ -101,12 +100,10 @@ int cat_perf_miss_val(int cpu_no, int n, char
> > *cache_type)
> >  	cache_size = 0;
> > 
> >  	/* Get default cbm mask for L3/L2 cache */
> > -	ret = get_cbm_mask(cache_type, cbm_mask);
> > +	ret = get_cbm_mask(cache_type, &long_mask);
> >  	if (ret)
> >  		return ret;
> > 
> > -	long_mask = strtoul(cbm_mask, NULL, 16);
> > -
> >  	/* Get L3/L2 cache size */
> >  	ret = get_cache_size(cpu_no, cache_type, &cache_size);
> >  	if (ret)
> > diff --git a/tools/testing/selftests/resctrl/cmt_test.c
> > b/tools/testing/selftests/resctrl/cmt_test.c
> > index 2d434c03cbba..ae54bbabbd91 100644
> > --- a/tools/testing/selftests/resctrl/cmt_test.c
> > +++ b/tools/testing/selftests/resctrl/cmt_test.c
> > @@ -17,7 +17,6 @@
> >  #define MAX_DIFF_PERCENT	15
> > 
> >  static int count_of_bits;
> > -static char cbm_mask[256];
> >  static unsigned long long_mask;
> >  static unsigned long cache_size;
> > 
> > @@ -82,12 +81,10 @@ int cmt_resctrl_val(int cpu_no, int n, char
> > **benchmark_cmd)
> >  	if (!validate_resctrl_feature_request(CMT_STR))
> >  		return -1;
> > 
> > -	ret = get_cbm_mask("L3", cbm_mask);
> > +	ret = get_cbm_mask("L3", &long_mask);
> >  	if (ret)
> >  		return ret;
> > 
> > -	long_mask = strtoul(cbm_mask, NULL, 16);
> > -
> >  	ret = get_cache_size(cpu_no, "L3", &cache_size);
> >  	if (ret)
> >  		return ret;
> > diff --git a/tools/testing/selftests/resctrl/resctrl.h
> > b/tools/testing/selftests/resctrl/resctrl.h
> > index ba36eb5fdf0d..bcc95faa5b4e 100644
> > --- a/tools/testing/selftests/resctrl/resctrl.h
> > +++ b/tools/testing/selftests/resctrl/resctrl.h
> > @@ -106,7 +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); -int
> > get_cbm_mask(char *cache_type, char *cbm_mask);
> > +int get_cbm_mask(char *cache_type, unsigned long *mask);
> >  int get_cache_size(int cpu_no, char *cache_type, unsigned long
> > *cache_size);  void ctrlc_handler(int signum, siginfo_t *info, void *ptr);  int
> > signal_handler_register(void); diff --git
> > a/tools/testing/selftests/resctrl/resctrlfs.c
> > b/tools/testing/selftests/resctrl/resctrlfs.c
> > index 7fef9068d7fd..f01ecfa64063 100644
> > --- a/tools/testing/selftests/resctrl/resctrlfs.c
> > +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> > @@ -186,30 +186,29 @@ int get_cache_size(int cpu_no, char *cache_type,
> > unsigned long *cache_size)
> >  #define CORE_SIBLINGS_PATH	"/sys/bus/cpu/devices/cpu"
> > 
> >  /*
> > - * get_cbm_mask - Get cbm mask for given cache
> > - * @cache_type:	Cache level L2/L3
> > - * @cbm_mask:	cbm_mask returned as a string
> > + * get_bit_mask - Get bit mask from given file
> > + * @filename:	File containing the mask
> > + * @mask:	The bit mask returned as unsigned long
> >   *
> >   * Return: = 0 on success, < 0 on failure.
> >   */
> > -int get_cbm_mask(char *cache_type, char *cbm_mask)
> > +static int get_bit_mask(char *filename, unsigned long *mask)
> >  {
> > -	char cbm_mask_path[1024];
> >  	FILE *fp;
> > 
> > -	if (!cbm_mask)
> > +	if (!filename || !mask)
> >  		return -1;
> > 
> > -	sprintf(cbm_mask_path, "%s/%s/cbm_mask", INFO_PATH,
> > cache_type);
> > -
> > -	fp = fopen(cbm_mask_path, "r");
> > +	fp = fopen(filename, "r");
> >  	if (!fp) {
> > -		perror("Failed to open cache level");
> > -
> > +		fprintf(stderr, "Failed to open bit mask file '%s': %s\n",
> > +			filename, strerror(errno));
> >  		return -1;
> >  	}
> > -	if (fscanf(fp, "%s", cbm_mask) <= 0) {
> > -		perror("Could not get max cbm_mask");
> > +
> > +	if (fscanf(fp, "%lx", mask) <= 0) {
> > +		fprintf(stderr, "Could not read bit mask file '%s': %s\n",
> > +			filename, strerror(errno));
> >  		fclose(fp);
> > 
> >  		return -1;
> > @@ -219,6 +218,31 @@ int get_cbm_mask(char *cache_type, char
> > *cbm_mask)
> >  	return 0;
> >  }
> > 
> > +/*
> > + * get_cbm_bits - Get number of bits in cbm mask
>
> Is it get_cbm_mask?

Sure, thanks for noticing this. While correcting it, I also improved the 
description to match what the function now does.

-- 
 i.