Re: [PATCH v3 4/5] selftests/resctrl: Add resource_info_file_exists()

[Maciej Wieczor-Retman]

Hello Reinette,

On 2024-01-26 at 13:08:52 -0800, Reinette Chatre wrote:
>Hi Maciej,
>
>On 1/25/2024 3:12 AM, Maciej Wieczor-Retman wrote:
>> Feature checking done by resctrl_mon_feature_exists() covers features
>> represented by the feature name presence inside the 'mon_features' file
>> in /sys/fs/resctrl/info/L3_MON directory. There exists a different way
>> to represent feature support and that is by the presence of 0 or 1 in a
>> single file in the info/resource directory. In this case the filename
>> represents what feature support is being indicated.
>> 
>> Add a generic function to check file presence in the
>> /sys/fs/resctrl/info/<RESOURCE> directory.
>> 
>> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@xxxxxxxxx>
>> ---
>> Changelog v3:
>> - Split off the new function into this patch. (Reinette)
>> 
>> Changelog v2:
>> - Add this patch.
>> 
>>  tools/testing/selftests/resctrl/resctrl.h   |  2 ++
>>  tools/testing/selftests/resctrl/resctrlfs.c | 26 +++++++++++++++++++++
>>  2 files changed, 28 insertions(+)
>> 
>> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
>> index 4603b215b97e..c39105f46da9 100644
>> --- a/tools/testing/selftests/resctrl/resctrl.h
>> +++ b/tools/testing/selftests/resctrl/resctrl.h
>> @@ -138,6 +138,8 @@ int umount_resctrlfs(void);
>>  int validate_bw_report_request(char *bw_report);
>>  bool resctrl_resource_exists(const char *resource);
>>  bool resctrl_mon_feature_exists(const char *feature);
>> +bool resource_info_file_exists(const char *resource,
>> +			       const char *feature);
>
>In addition what Ilpo commented about other line, this too can be
>on one line.

Thanks!

>
>>  bool test_resource_feature_check(const struct resctrl_test *test);
>>  char *fgrep(FILE *inf, const char *str);
>>  int taskset_benchmark(pid_t bm_pid, int cpu_no, cpu_set_t *old_affinity);
>> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
>> index e4ba8614fb7b..a6427732e0ad 100644
>> --- a/tools/testing/selftests/resctrl/resctrlfs.c
>> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
>> @@ -763,6 +763,32 @@ bool resctrl_mon_feature_exists(const char *feature)
>>  	return !!res;
>>  }
>>  
>> +/*
>> + * resource_info_file_exists - Check if a file is present inside
>> + * /sys/fs/resctrl/info/RESOURCE.
>
>As confirmed in the changelog this is intended to be a generic function that
>tests if a file exists ...
>
>> + * @resource:	Required resource (Eg: MB, L3, L2, etc.)
>> + * @feature:	Required feature.
>
>... so assuming a use of this by referring to the file as "feature" is unexpected.
>
>This function jumps between "file" and "feature" in comments and code, please
>just stick to goal of making it a generic utility that checks for a file and
>refer to it consistently so.

Okay, I'll stick to 'file'.

>
>Reinette

-- 
Kind regards
Maciej Wieczór-Retman