Received-SPF: pass (google.com: domain of linux-kernel+bounces-46177-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Wed, 31 Jan 2024 11:05:50 +0100
From: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
To: Ilpo =?utf-8?B?SsOkcnZpbmVu?= <ilpo.jarvinen@linux.intel.com>
CC: Reinette Chatre <reinette.chatre@intel.com>, <shuah@kernel.org>,
	<fenghua.yu@intel.com>, LKML <linux-kernel@vger.kernel.org>,
	<linux-kselftest@vger.kernel.org>
Subject: Re: [PATCH v3 3/5] selftests/resctrl: Split
 validate_resctrl_feature_request()
Message-ID: <imaqlenxrw3rfjlwlfnyhs4cc4cf2c7ibid7r7jeuqu5brsqdu@xpngruljfhrs>
References: <cover.1706180726.git.maciej.wieczor-retman@intel.com>
 <415cc65c113143c833ca2eaacd3a03c285c2e841.1706180726.git.maciej.wieczor-retman@intel.com>
 <62310cc5-7756-61a2-9529-8ae0fc247c21@linux.intel.com>
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <62310cc5-7756-61a2-9529-8ae0fc247c21@linux.intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?iso-8859-1?Q?AHibciji51ZvMjWyYWw2dq/u92rCy0yfqAcTl+KVTqd/LNx1gBQgDZdvSB?=
 =?iso-8859-1?Q?5D+KNvqSJxpfIJ9NkGX8jHywe05kxS3J0vPqitbW9BzGSdbF7Tpywa+IJN?=
 =?iso-8859-1?Q?cfq2qoaJSC0vS+ryYpI18EZJXZd/HL+JyXvy7XAo+8vDL8pr5ffqoDeuyM?=
 =?iso-8859-1?Q?AlvMCdxpqDbq1fjG3O20g6/jeC9gJLD+cu8eh9I5NTkfWAw0hVW7tvdXI+?=
 =?iso-8859-1?Q?qSktQGV98ZKQSf4/uDHGlwLUfUVWcI8jPvgIyx4v+8aAz/U3YfzpMmaUlG?=
 =?iso-8859-1?Q?jqnT8caHB6SVFhwCD8o1HXlvePbfzcCAG99Of8FXz4HwmL5KYQ+wctGUTl?=
 =?iso-8859-1?Q?WSmmpcCUDhp77KdFFSQjqd/4qVpABwrOKRoeOR3bGlXEkIfwuQkDjv/w1Q?=
 =?iso-8859-1?Q?CMuB1f+H6WPfM2nDja3eL1tloefHY4e47gKaeIvZLJaO0OgzBXhVnf0xgr?=
 =?iso-8859-1?Q?SeJH/S4xmBWAxiv1dgm8us4GashDpHYkxS5wcF9yC4Ru/+6zq+RHJ6h+nQ?=
 =?iso-8859-1?Q?HH6bBpnhfmQnMqgdoRKMfgb69aRvhnYPk9BovSb1uDPPJwAk6Sd03QdG7m?=
 =?iso-8859-1?Q?etrtg1AlA8ZHXUH2P95CF5xmLME+5VB18UxBkJbwer8Pq7ZpyeNBP4puZF?=
 =?iso-8859-1?Q?ZaZmPeoIPBcITWhcZ14Qv5NK0I4VN/V3JrhzvtN0UU++gX9vfCjjruH+gK?=
 =?iso-8859-1?Q?76PA3UFaI/OKfqnQKhkknLdXSuyp7q3X33Japww/JKiXTX7xryO8iCRD3o?=
 =?iso-8859-1?Q?AemfFEr/jsi9njkuXyrMKx7JcqsJhOm+D+4UU+08GpT8t9t01isEbcokFc?=
 =?iso-8859-1?Q?/ZDkVJvpEUdjbQhRcrCLT9t7qUIZIlZFjpIDSfKbRbJBE9gSXrLawig3MS?=
 =?iso-8859-1?Q?kHtSxtrr5fP1ON7yp4zI3JTxTnG/1vKW/kQWXbXWagaHLZavaQgxG5jDWk?=
 =?iso-8859-1?Q?8JXy/FHMV6DWT42ZNyKkQdCkWVNDE9oAGt3xLtb35woaJ/7s1UODeQ7c9D?=
 =?iso-8859-1?Q?EAIGhOWaT2xdvfHKrrzRCPWoygC1LivecbFM4IlreNQnRT7amBpJJijKGp?=
 =?iso-8859-1?Q?JoXIlRL/P8B+Is7p9rpTxgpJmneVt5Q2zq9tPv8y5YtkKPwWFEZbeLE+Gh?=
 =?iso-8859-1?Q?Fr+mlIys0NFpQOgUGCkncBgdoVjCNenFe8eA8xmDzBfyGf2zVmRnl2pfMs?=
 =?iso-8859-1?Q?pPqVtk7HKxdAV/1TAIyDg/UKu+MEJ6Kt8WEx5e7rAt/UDQ0vkB26hRifN8?=
 =?iso-8859-1?Q?nQGPr/AADQzpZChqU9OFnbjrdKMLwJ1KmPgLSyvs5xYP2ZzLn8IN3dxQoF?=
 =?iso-8859-1?Q?n5vSIbT69NYJDuWRMNO7cE9ubwKCss1VXlGr8PyL5m5XobrZ8JPV0u3ZeR?=
 =?iso-8859-1?Q?r+Kf2mj0O7ea1BfNuEj9repP7H+m6P0pOYtfcsuuEOFTReJ62Qz7kuRS17?=
 =?iso-8859-1?Q?jlIeI9UgBytZkxbGDhFveqTRbUcKaw/CdoCZjxqoQBvSNjVjVS/M7WAqX3?=
 =?iso-8859-1?Q?dRFXib+oduF9Mw/i1jUd/Vu1K2ms9NYNytjJw4Vgzv4C1gSGZgT+6yph+c?=
 =?iso-8859-1?Q?/p/GyLdAKxeSIaafGTyljJ4YMv4J6ryBysHB7M6K4LdyvNYWEp7uXg9jS0?=
 =?iso-8859-1?Q?2JYOvPtSnfSdAAR49yhCqVKTa9MD1D3rr8w3qhumgdgryvDaGDQAUuYVr1?=
 =?iso-8859-1?Q?A1iO1h1AgfxaJkTQqQE=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 1bd478a0-2271-4552-36a1-08dc22443903
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6231.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Jan 2024 10:05:59.1525
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 3dz3rjScslaE/cdPuz+U6rGNDVA/N0NsiraUp5yJh4fJ4JExEB0fiWB1m1Y7ot2pb/F7dZ0YAPdz5uEgOgv765ESGXTMzkfb17OuUusRQ34=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR11MB7764
X-OriginatorOrg: intel.com

Hi Ilpo,

On 2024-01-25 at 13:46:51 +0200, Ilpo J?rvinen wrote:
>On Thu, 25 Jan 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@intel.com>
>> ---
>> 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  |  4 +--
>>  tools/testing/selftests/resctrl/mba_test.c  |  4 +--
>>  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, 30 insertions(+), 20 deletions(-)
>> 
>> diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
>> index dd5ca343c469..428de9df81c8 100644
>> --- a/tools/testing/selftests/resctrl/cmt_test.c
>> +++ b/tools/testing/selftests/resctrl/cmt_test.c
>> @@ -169,8 +169,8 @@ 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");
>> +	return resctrl_mon_feature_exists("llc_occupancy") &&
>> +	       resctrl_resource_exists("L3");
>>  }
>>  
>>  struct resctrl_test cmt_test = {
>> diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
>> index da256d2dbe5c..e22285b80e37 100644
>> --- a/tools/testing/selftests/resctrl/mba_test.c
>> +++ b/tools/testing/selftests/resctrl/mba_test.c
>> @@ -170,8 +170,8 @@ static int mba_run_test(const struct resctrl_test *test, const struct user_param
>>  
>>  static bool mba_feature_check(const struct resctrl_test *test)
>>  {
>> -	return test_resource_feature_check(test) &&
>> -	       validate_resctrl_feature_request("L3_MON", "mbm_local_bytes");
>> +	return resctrl_resource_exists(test->resource) &&
>
>I don't understand what's the advantage of converting away from 
>test_resource_feature_check() in CMT and MBA case?
>
>> +	       resctrl_mon_feature_exists("mbm_local_bytes");
>>  }
>
>> @@ -756,7 +765,7 @@ bool validate_resctrl_feature_request(const char *resource, const char *feature)
>>  
>>  bool test_resource_feature_check(const struct resctrl_test *test)
>>  {
>> -	return validate_resctrl_feature_request(test->resource, NULL);
>> +	return resctrl_resource_exists(test->resource);
>
>...The replacement in MBA open coded test_resource_feature_check() 100% 
>and CMT even replaces the test->resource with the string matching to 
>what's in test->resource?
>

You're right, I got carried away with refactoring a bit. I'll keep
test_resource_feature_check() for CMT and MBM. Thanks!

>
>-- 
> i.
>

-- 
Kind regards
Maciej Wiecz?r-Retman