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-ret...@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
--
Kind regards
Maciej Wieczór-Retman
