linux-kernel - Re: [PATCH v4 3/5] selftests/resctrl: Split validate_resctrl_feature

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a863439f-1897-4d72-bcb7-f4f8575c2a39@intel.com>
Date: Mon, 5 Feb 2024 20:17:18 -0800
From: Reinette Chatre <reinette.chatre@...el.com>
To: Maciej Wieczor-Retman <maciej.wieczor-retman@...el.com>,
	Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
CC: <fenghua.yu@...el.com>, <shuah@...nel.org>,
	<linux-kselftest@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 3/5] selftests/resctrl: Split
 validate_resctrl_feature_request()



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@...el.com>
>>> ---
>>> 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