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

[Shaopeng Tan (Fujitsu)]

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?

Best regards,
Shaopeng TAN

> + * @cache_type:		Cache level L2/L3
> + * @mask:		cbm_mask returned as unsigned long
> + *
> + * Return: = 0 on success, < 0 on failure.
> + */
> +int get_cbm_mask(char *cache_type, unsigned long *mask) {
> +	char cbm_mask_path[1024];
> +	int ret;
> +
> +	if (!cache_type)
> +		return -1;
> +
> +	snprintf(cbm_mask_path, sizeof(cbm_mask_path),
> "%s/%s/cbm_mask",
> +		 INFO_PATH, cache_type);
> +
> +	ret = get_bit_mask(cbm_mask_path, mask);
> +	if (ret)
> +		return -1;
> +
> +	return 0;
> +}
> +
>  /*
>   * get_core_sibling - Get sibling core id from the same socket for given CPU
>   * @cpu_no:	CPU number
> --
> 2.30.2