[v3,2/5] 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 kernel (resctrl). Then the
test compares if that information matches what is reported by CPUID
output.

Add a generic helper function to read an unsigned number from a file in
/sys/fs/resctrl/info/<RESOURCE>/<FILE>.

Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
---
Changelog v3:
- Rewrite patch message.
- Add documentation and rewrote the function. (Reinette)

Changelog v2:
- Add this patch.

 tools/testing/selftests/resctrl/resctrl.h   |  1 +
 tools/testing/selftests/resctrl/resctrlfs.c | 39 +++++++++++++++++++++
 2 files changed, 40 insertions(+)
  

On Thu, 25 Jan 2024, Maciej Wieczor-Retman wrote:

> The CAT non-contiguous selftests have to read the file responsible for
> reporting support of non-contiguous CBMs in kernel (resctrl). Then the
> test compares if that information matches what is reported by CPUID
> output.
> 
> Add a generic helper function to read an unsigned number from a file in
> /sys/fs/resctrl/info/<RESOURCE>/<FILE>.
> 
> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
> ---
> Changelog v3:
> - Rewrite patch message.
> - Add documentation and rewrote the function. (Reinette)
> 
> Changelog v2:
> - Add this patch.
> 
>  tools/testing/selftests/resctrl/resctrl.h   |  1 +
>  tools/testing/selftests/resctrl/resctrlfs.c | 39 +++++++++++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index a1462029998e..5116ea082d03 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -162,6 +162,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 resource_info_unsigned_get(const char *resource, const char *filename, unsigned int *val);
>  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 5750662cce57..cb5147c5f9a9 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -249,6 +249,45 @@ static int get_bit_mask(const char *filename, unsigned long *mask)
>  	return 0;
>  }
>  
> +/*
> + * resource_info_unsigned_get - Read an unsigned value from a file in
> + * /sys/fs/resctrl/info/RESOURCE/FILENAME
> + * @resource:	Resource name that matches directory names in
> + *		/sys/fs/resctrl/info
> + * @filename:	Filename of a file located in a directory specified with the
> + *		'resource' variable.
> + * @val:	Variable where the read value is saved on success.
> + *
> + * Return: = 0 on success, < 0 on failure. On success the read value is saved into the 'val'
> + * variable.
> + */
> +int resource_info_unsigned_get(const char *resource, const char *filename,
> +			       unsigned int *val)
> +{
> +	char reason[128], file_path[PATH_MAX];
> +	FILE *fp;
> +
> +	snprintf(file_path, sizeof(file_path), "%s/%s/%s", INFO_PATH, resource,
> +		 filename);
> +
> +	fp = fopen(file_path, "r");
> +	if (!fp) {
> +		snprintf(reason, sizeof(reason), "Error in opening %s file\n", filename);
> +		ksft_perror(reason);

Was this the conclusion of the kstf_perror() discussion with Reinette? I 
expected a bit different outcome when I stopped following it...

In any case, it would be nice though if ksft_perror() (or some kselftest.h 
function yet to be added with a different name) would accept full printf 
interface and just add the errno string into the end of the string so one 
would not need to build constructs like this at all.

It will require a bit of macro trickery into kselftest.h. I don't know how 
it should handle the case where somebody just passes a char pointer to it, 
not a string literal, but I guess it would just throw an error while 
compiling if somebody tries to do that as the macro string literal 
concatenation could not build useful/compilable token.

It would make these prints informative enough to become actually useful 
without needed to resort to preparing the string in advance which seems
to be required almost every single case with the current interface.

> +		return -1;
> +	}
> +
> +	if (fscanf(fp, "%u", val) <= 0) {
> +		snprintf(reason, sizeof(reason), "Could not get %s's contents\n", filename);
> +		ksft_perror(reason);
> +		fclose(fp);
> +		return -1;
> +	}
> +
> +	fclose(fp);
> +	return 0;
> +}
> +
>  /*
>   * create_bit_mask- Create bit mask from start, len pair
>   * @start:	LSB of the mask
>
  

On 1/25/2024 4:14 AM, Ilpo Järvinen wrote:
> On Thu, 25 Jan 2024, Maciej Wieczor-Retman wrote:


>> +	fp = fopen(file_path, "r");
>> +	if (!fp) {
>> +		snprintf(reason, sizeof(reason), "Error in opening %s file\n", filename);
>> +		ksft_perror(reason);
> 
> Was this the conclusion of the kstf_perror() discussion with Reinette? I 
> expected a bit different outcome when I stopped following it...
> 
> In any case, it would be nice though if ksft_perror() (or some kselftest.h 
> function yet to be added with a different name) would accept full printf 
> interface and just add the errno string into the end of the string so one 
> would not need to build constructs like this at all.
> 
> It will require a bit of macro trickery into kselftest.h. I don't know how 
> it should handle the case where somebody just passes a char pointer to it, 
> not a string literal, but I guess it would just throw an error while 
> compiling if somebody tries to do that as the macro string literal 
> concatenation could not build useful/compilable token.
> 
> It would make these prints informative enough to become actually useful 
> without needed to resort to preparing the string in advance which seems
> to be required almost every single case with the current interface.

I think this can be accomplished with a new:
	void  ksft_vprint_msg(const char *msg, va_list args)

.. but ksft_perror() does conform to perror() and I expect that having one
support variable number of arguments while the other does to cause confusion.

To support variable number of arguments with errno I'd propose just to use
ksft_print_msg() with strerror(errno), errno as the arguments (or even %m
that that errno handling within ksft_print_msg() aims to support). This does
indeed seem to be the custom in other tests.

Reinette
  

Hi Maciej,

On 1/25/2024 3:10 AM, Maciej Wieczor-Retman wrote:
> The CAT non-contiguous selftests have to read the file responsible for
> reporting support of non-contiguous CBMs in kernel (resctrl). Then the
> test compares if that information matches what is reported by CPUID
> output.
> 
> Add a generic helper function to read an unsigned number from a file in
> /sys/fs/resctrl/info/<RESOURCE>/<FILE>.
> 
> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
> ---
> Changelog v3:
> - Rewrite patch message.
> - Add documentation and rewrote the function. (Reinette)
> 
> Changelog v2:
> - Add this patch.
> 
>  tools/testing/selftests/resctrl/resctrl.h   |  1 +
>  tools/testing/selftests/resctrl/resctrlfs.c | 39 +++++++++++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index a1462029998e..5116ea082d03 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -162,6 +162,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 resource_info_unsigned_get(const char *resource, const char *filename, unsigned int *val);
>  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 5750662cce57..cb5147c5f9a9 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -249,6 +249,45 @@ static int get_bit_mask(const char *filename, unsigned long *mask)
>  	return 0;
>  }
>  
> +/*

By not starting with /** the following will not be interpreted as kernel-doc
but the formatting does appear to follow the syntax, but not consistently so.
I think it would be more readable if the kernel-doc syntax is followed consistently.

> + * resource_info_unsigned_get - Read an unsigned value from a file in
> + * /sys/fs/resctrl/info/RESOURCE/FILENAME

"Read an unsigned value from /sys/fs/resctrl/info/RESOURCE/FILENAME"?

> + * @resource:	Resource name that matches directory names in

names? (plural)

> + *		/sys/fs/resctrl/info
> + * @filename:	Filename of a file located in a directory specified with the
> + *		'resource' variable.

I think this can be shortened to "File in /sys/fs/resctrl/info/@resource"

> + * @val:	Variable where the read value is saved on success.

"Contains read value on success."

(no need to refer to it as a variable/parameter, it is implied by syntax).

> + *
> + * Return: = 0 on success, < 0 on failure. On success the read value is saved into the 'val'
> + * variable.

"saved into the 'val' variable" -> "saved into @val" (since syntax indicates it is the parameter
there is no need to elaborate). 
Also please let lines in comments be of consistent length.

> + */


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

(apart from other discussions). "file" in message seems redundant. It can just be "Error
opening %s". It may also be useful to print file_path instead of filename to be specific
of what the code tried to open.

> +		ksft_perror(reason);
> +		return -1;
> +	}
> +
> +	if (fscanf(fp, "%u", val) <= 0) {
> +		snprintf(reason, sizeof(reason), "Could not get %s's contents\n", filename);
> +		ksft_perror(reason);

filename -> file_path ?

> +		fclose(fp);
> +		return -1;
> +	}
> +
> +	fclose(fp);
> +	return 0;
> +}
> +


Reinette
  

Hello!

On 2024-01-26 at 13:08:19 -0800, Reinette Chatre wrote:
>Hi Maciej,
>
>On 1/25/2024 3:10 AM, Maciej Wieczor-Retman wrote:
>> The CAT non-contiguous selftests have to read the file responsible for
>> reporting support of non-contiguous CBMs in kernel (resctrl). Then the
>> test compares if that information matches what is reported by CPUID
>> output.
>> 
>> Add a generic helper function to read an unsigned number from a file in
>> /sys/fs/resctrl/info/<RESOURCE>/<FILE>.
>> 
>> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
>> ---
>> Changelog v3:
>> - Rewrite patch message.
>> - Add documentation and rewrote the function. (Reinette)
>> 
>> Changelog v2:
>> - Add this patch.
>> 
>>  tools/testing/selftests/resctrl/resctrl.h   |  1 +
>>  tools/testing/selftests/resctrl/resctrlfs.c | 39 +++++++++++++++++++++
>>  2 files changed, 40 insertions(+)
>> 
>> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
>> index a1462029998e..5116ea082d03 100644
>> --- a/tools/testing/selftests/resctrl/resctrl.h
>> +++ b/tools/testing/selftests/resctrl/resctrl.h
>> @@ -162,6 +162,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 resource_info_unsigned_get(const char *resource, const char *filename, unsigned int *val);
>>  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 5750662cce57..cb5147c5f9a9 100644
>> --- a/tools/testing/selftests/resctrl/resctrlfs.c
>> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
>> @@ -249,6 +249,45 @@ static int get_bit_mask(const char *filename, unsigned long *mask)
>>  	return 0;
>>  }
>>  
>> +/*
>
>By not starting with /** the following will not be interpreted as kernel-doc
>but the formatting does appear to follow the syntax, but not consistently so.
>I think it would be more readable if the kernel-doc syntax is followed consistently.

Sure, I'll change it.

>
>> + * resource_info_unsigned_get - Read an unsigned value from a file in
>> + * /sys/fs/resctrl/info/RESOURCE/FILENAME
>
>"Read an unsigned value from /sys/fs/resctrl/info/RESOURCE/FILENAME"?

Okay

>
>> + * @resource:	Resource name that matches directory names in
>
>names? (plural)

Right, it doesn't make much sense, I'll move it to singular.

>
>> + *		/sys/fs/resctrl/info
>> + * @filename:	Filename of a file located in a directory specified with the
>> + *		'resource' variable.
>
>I think this can be shortened to "File in /sys/fs/resctrl/info/@resource"

Sure, thanks

>
>> + * @val:	Variable where the read value is saved on success.
>
>"Contains read value on success."
>
>(no need to refer to it as a variable/parameter, it is implied by syntax).

Right, I'll change it.

>
>> + *
>> + * Return: = 0 on success, < 0 on failure. On success the read value is saved into the 'val'
>> + * variable.
>
>"saved into the 'val' variable" -> "saved into @val" (since syntax indicates it is the parameter
>there is no need to elaborate). 

Sure, thanks

>Also please let lines in comments be of consistent length.

Okay, I'll keep it to 80 characters.

>
>> + */
>
>
>> +int resource_info_unsigned_get(const char *resource, const char *filename,
>> +			       unsigned int *val)
>> +{
>> +	char reason[128], file_path[PATH_MAX];
>> +	FILE *fp;
>> +
>> +	snprintf(file_path, sizeof(file_path), "%s/%s/%s", INFO_PATH, resource,
>> +		 filename);
>> +
>> +	fp = fopen(file_path, "r");
>> +	if (!fp) {
>> +		snprintf(reason, sizeof(reason), "Error in opening %s file\n", filename);
>
>(apart from other discussions). "file" in message seems redundant. It can just be "Error
>opening %s". It may also be useful to print file_path instead of filename to be specific
>of what the code tried to open.

Okay, I'll change it to file_path.

>
>> +		ksft_perror(reason);
>> +		return -1;
>> +	}
>> +
>> +	if (fscanf(fp, "%u", val) <= 0) {
>> +		snprintf(reason, sizeof(reason), "Could not get %s's contents\n", filename);
>> +		ksft_perror(reason);
>
>filename -> file_path ?

Same as above.

>
>> +		fclose(fp);
>> +		return -1;
>> +	}
>> +
>> +	fclose(fp);
>> +	return 0;
>> +}
>> +
>
>
>Reinette
  

Hi,

On 2024-01-26 at 10:58:04 -0800, Reinette Chatre wrote:
>
>
>On 1/25/2024 4:14 AM, Ilpo Järvinen wrote:
>> On Thu, 25 Jan 2024, Maciej Wieczor-Retman wrote:
>
>
>>> +	fp = fopen(file_path, "r");
>>> +	if (!fp) {
>>> +		snprintf(reason, sizeof(reason), "Error in opening %s file\n", filename);
>>> +		ksft_perror(reason);
>> 
>> Was this the conclusion of the kstf_perror() discussion with Reinette? I 
>> expected a bit different outcome when I stopped following it...
>> 
>> In any case, it would be nice though if ksft_perror() (or some kselftest.h 
>> function yet to be added with a different name) would accept full printf 
>> interface and just add the errno string into the end of the string so one 
>> would not need to build constructs like this at all.
>> 
>> It will require a bit of macro trickery into kselftest.h. I don't know how 
>> it should handle the case where somebody just passes a char pointer to it, 
>> not a string literal, but I guess it would just throw an error while 
>> compiling if somebody tries to do that as the macro string literal 
>> concatenation could not build useful/compilable token.
>> 
>> It would make these prints informative enough to become actually useful 
>> without needed to resort to preparing the string in advance which seems
>> to be required almost every single case with the current interface.
>
>I think this can be accomplished with a new:
>	void  ksft_vprint_msg(const char *msg, va_list args)
>
>... but ksft_perror() does conform to perror() and I expect that having one
>support variable number of arguments while the other does to cause confusion.
>
>To support variable number of arguments with errno I'd propose just to use
>ksft_print_msg() with strerror(errno), errno as the arguments (or even %m
>that that errno handling within ksft_print_msg() aims to support). This does
>indeed seem to be the custom in other tests.

Does something like this look okay?

	fp = fopen(file_path, "r");
	if (!fp) {
		ksft_print_msg("Error in opening %s\n: %m\n", file_path);
		return -1;
	}

The '%m' seems to work fine but doesn't print errno's number code. Do you want
me to add errno after '%m' so it is the same as ksft_perror()? I looked through
some other tests where '%m' is used, and only few ones add errno with '%d'.

>Reinette
  

On Wed, 31 Jan 2024, Maciej Wieczor-Retman wrote:
> On 2024-01-26 at 10:58:04 -0800, Reinette Chatre wrote:
> >On 1/25/2024 4:14 AM, Ilpo Järvinen wrote:
> >> On Thu, 25 Jan 2024, Maciej Wieczor-Retman wrote:
> >
> >>> +	fp = fopen(file_path, "r");
> >>> +	if (!fp) {
> >>> +		snprintf(reason, sizeof(reason), "Error in opening %s file\n", filename);
> >>> +		ksft_perror(reason);
> >> 
> >> Was this the conclusion of the kstf_perror() discussion with Reinette? I 
> >> expected a bit different outcome when I stopped following it...
> >> 
> >> In any case, it would be nice though if ksft_perror() (or some kselftest.h 
> >> function yet to be added with a different name) would accept full printf 
> >> interface and just add the errno string into the end of the string so one 
> >> would not need to build constructs like this at all.
> >> 
> >> It will require a bit of macro trickery into kselftest.h. I don't know how 
> >> it should handle the case where somebody just passes a char pointer to it, 
> >> not a string literal, but I guess it would just throw an error while 
> >> compiling if somebody tries to do that as the macro string literal 
> >> concatenation could not build useful/compilable token.
> >> 
> >> It would make these prints informative enough to become actually useful 
> >> without needed to resort to preparing the string in advance which seems
> >> to be required almost every single case with the current interface.
> >
> >I think this can be accomplished with a new:
> >	void  ksft_vprint_msg(const char *msg, va_list args)
> >
> >... but ksft_perror() does conform to perror() and I expect that having one
> >support variable number of arguments while the other does to cause confusion.
> >
> >To support variable number of arguments with errno I'd propose just to use
> >ksft_print_msg() with strerror(errno), errno as the arguments (or even %m
> >that that errno handling within ksft_print_msg() aims to support). This does
> >indeed seem to be the custom in other tests.
> 
> Does something like this look okay?
> 
> 	fp = fopen(file_path, "r");
> 	if (!fp) {
> 		ksft_print_msg("Error in opening %s\n: %m\n", file_path);
> 		return -1;
> 	}
> 
> The '%m' seems to work fine but doesn't print errno's number code. Do you want
> me to add errno after '%m' so it is the same as ksft_perror()? I looked through
> some other tests where '%m' is used, and only few ones add errno with '%d'.

I think %m is enough.