Re: [PATCH v4 3/5] selftests/resctrl: Split validate_resctrl_feature_request()

[Reinette Chatre]

On 2/5/2024 5:24 AM, Maciej Wieczor-Retman wrote:
> On 2024-02-05 at 14:41:30 +0200, Ilpo Järvinen wrote:
>> On Mon, 5 Feb 2024, Maciej Wieczor-Retman wrote:
>>
>>> validate_resctrl_feature_request() is used to test both if a resource is
>>> present in the info directory, and if a passed monitoring feature is
>>> present in the mon_features file.
>>>
>>> Refactor validate_resctrl_feature_request() into two smaller functions
>>> that each accomplish one check to give feature checking more
>>> granularity:
>>> - Resource directory presence in the /sys/fs/resctrl/info directory.
>>> - Feature name presence in the /sys/fs/resctrl/info/L3_MON/mon_features
>>>   file.
>>>
>>> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@xxxxxxxxx>
>>> ---
>>> Changelog v4:
>>> - Roll back to using test_resource_feature_check() for CMT and MBA.
>>>   (Ilpo).
>>>
>>> Changelog v3:
>>> - Move new function to a separate patch. (Reinette)
>>> - Rewrite resctrl_mon_feature_exists() only for L3_MON.
>>>
>>> Changelog v2:
>>> - Add this patch.
>>>
>>>  tools/testing/selftests/resctrl/cmt_test.c  |  2 +-
>>>  tools/testing/selftests/resctrl/mba_test.c  |  2 +-
>>>  tools/testing/selftests/resctrl/mbm_test.c  |  6 ++--
>>>  tools/testing/selftests/resctrl/resctrl.h   |  3 +-
>>>  tools/testing/selftests/resctrl/resctrlfs.c | 33 +++++++++++++--------
>>>  5 files changed, 28 insertions(+), 18 deletions(-)
>>>
>>
>>> diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
>>> index dd5ca343c469..c1157917a814 100644
>>> --- a/tools/testing/selftests/resctrl/cmt_test.c
>>> +++ b/tools/testing/selftests/resctrl/cmt_test.c
>>> @@ -170,7 +170,7 @@ static int cmt_run_test(const struct resctrl_test *test, const struct user_param
>>>  static bool cmt_feature_check(const struct resctrl_test *test)
>>>  {
>>>  	return test_resource_feature_check(test) &&
>>> -	       validate_resctrl_feature_request("L3_MON", "llc_occupancy");
>>> +	       resctrl_resource_exists("L3");
>>
>> This not correctly transformed.
> 
> Oops, sorry, I'll fix it for the next version.
> 
>>
>>> +/*
>>> + * resctrl_mon_feature_exists - Check if requested monitoring L3_MON feature is valid.
>>> + * @feature:	Required monitor feature (in mon_features file).
>>> + *
>>> + * Return: True if the feature is supported, else false.
>>> + */
>>> +bool resctrl_mon_feature_exists(const char *feature)
>>> +{
>>> +	char *res;
>>> +	FILE *inf;
>>> +
>>>  	if (!feature)
>>> -		return true;
>>> +		return false;
>>>  
>>> -	snprintf(res_path, sizeof(res_path), "%s/%s/mon_features", INFO_PATH, resource);
>>> -	inf = fopen(res_path, "r");
>>> +	inf = fopen("/sys/fs/resctrl/info/L3_MON/mon_features", "r");
>>
>> This became less generic? Could there be other MON resource besides L3 
>> one? Perhaps there aren't today but why remove the ability give it as a 
>> parameter?

This does make the function less generic but the benefit is that four copies of
the same hardcoded parameter is no longer needed. In my opinion this patch thus
makes the code cleaner but it is not a requirement that I will use to hold this
series back.

> During v2 discussion [1] Reinette made me realize this functionality only
> interfaces with L3_MON/mon_features file and the 'resource' parameter isn't
> needed. The 'mon_features' file is only mentioned for L3_MON and I don't know of
> any plans for other MON resources so I assumed it doesn't need to be generic.
> 
> But sure, I can make it use a parameter if Reinette doesn't mind.

I prefer what is in this patch, but I will not object if the function
is changed to take the resource as parameter.

> 
> [1] https://lore.kernel.org/all/2o7adr2cos6qcikcu7oop4ss7vib2n6ue33djgfeds3v6gj53f@uu45lomrp5qv/
> 

Reinette