Re: [PATCH v2 2/4] selftests/resctrl: Add helpers for the non-contiguous test

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Maciej,

On 12/12/2023 6:52 AM, Maciej Wieczor-Retman wrote:
> The CAT non-contiguous selftests have to read the file responsible for
> reporting support of non-contiguous CBMs in Intel CAT. Then the test

"in Intel CAT" -> "in kernel (resctrl)"

> compares if that information matches what is reported by CPUID output.
> 
> Add a generic helper function to read a chosen functionality support
> information.

Since this is a generic function that just reads a value from a file it
cannot be assumed that the value represents functionality support.

> 
> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
> ---
> Changelog v2:
> - Added this patch.
> 
>  tools/testing/selftests/resctrl/resctrl.h   |  1 +
>  tools/testing/selftests/resctrl/resctrlfs.c | 25 +++++++++++++++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index 739e16d08a7b..8f72d94b9cbe 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -161,6 +161,7 @@ unsigned int count_contiguous_bits(unsigned long val, unsigned int *start);
>  int get_full_cbm(const char *cache_type, unsigned long *mask);
>  int get_mask_no_shareable(const char *cache_type, unsigned long *mask);
>  int get_cache_size(int cpu_no, const char *cache_type, unsigned long *cache_size);
> +int read_info_res_file(const char *resource, const char *filename);
>  void ctrlc_handler(int signum, siginfo_t *info, void *ptr);
>  int signal_handler_register(void);
>  void signal_handler_unregister(void);
> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
> index 0e97036a64b8..70333440ff2f 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -249,6 +249,31 @@ static int get_bit_mask(const char *filename, unsigned long *mask)
>  	return 0;
>  }
>  
> +int read_info_res_file(const char *resource, const char *filename)

Considering that this is intended to be a new generic utility, could you
please add some function documentation?

> +{
> +	char file_path[PATH_MAX];
> +	FILE *fp;
> +	int ret;
> +
> +	snprintf(file_path, sizeof(file_path), "%s/%s/%s", INFO_PATH, resource,
> +		 filename);
> +
> +	fp = fopen(file_path, "r");
> +	if (!fp) {
> +		perror("Error in opening sparse_masks file\n");

The error messages do not match the goal of this function to be generic.
Also, please note the recent cleanup done by Ilpo to replace the perror()
by ksft_perror().

> +		return -1;
> +	}
> +
> +	if (fscanf(fp, "%u", &ret) <= 0) {

I find this to be potentially confusing. The function claims to be a generic
utility to read a value from a resctrl file ... but hidden within is that the
value is required to be unsigned, which is then cast into an int. This could be
made more specific and robust with something like below:
	int resource_info_unsigned_get(const char *resource, const char *filename,
					unsigned int *val)

The return value will be the result of the request. If resource_info_unsigned_get()
returns 0 then @val will contain the value read. 

> +		perror("Could not get sparse_masks contents\n");
> +		fclose(fp);
> +		return -1;
> +	}
> +
> +	fclose(fp);
> +	return ret;
> +}
> +
>  /*
>   * create_bit_mask- Create bit mask from start, len pair
>   * @start:	LSB of the mask

Reinette