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

  The CAT non-contiguous selftests have to read the file responsible for
reporting support of non-contiguous CBMs in Intel CAT. Then the test
compares if that information matches what is reported by CPUID output.

Add a generic helper function to read a chosen functionality support
information.

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

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
  

Hi!

On 2024-01-08 at 14:36:36 -0800, Reinette Chatre wrote:
>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)"

Sure, will fix.

>> 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.

Right, I'll rewrite this.

>> 
>> 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?

Sure

>> +{
>> +	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().

Thanks for catching this, will fix.

>> +		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. 

Right, that might be confusing. Will fix according to your comment, thanks!

>> +		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