Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Message-ID: <8eb32195-102b-cbf5-3ea0-88550cc81de6@intel.com>
Date:   Wed, 13 Sep 2023 13:59:26 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.15.0
Subject: Re: [PATCH 3/5] selftests/resctrl: Refactor feature check to use
 resource and feature name
Content-Language: en-US
To:     =?UTF-8?Q?Ilpo_J=c3=a4rvinen?= <ilpo.jarvinen@linux.intel.com>
CC:     Shuah Khan <shuah@kernel.org>,
        Shuah Khan <skhan@linuxfoundation.org>,
        <linux-kselftest@vger.kernel.org>,
        =?UTF-8?Q?Maciej_Wiecz=c3=b3r-Retman?= 
        <maciej.wieczor-retman@intel.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>,
        <stable@vger.kernel.org>
References: <20230911111930.16088-1-ilpo.jarvinen@linux.intel.com>
 <20230911111930.16088-4-ilpo.jarvinen@linux.intel.com>
 <b5a36b8a-c5c7-84a9-270e-bef4478d4bff@intel.com>
 <eac7deb6-3593-7a59-7df8-208392254f7@linux.intel.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <eac7deb6-3593-7a59-7df8-208392254f7@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?SiswZU51S2ZaQ2s1MXNNVmh5aVdQM1U4Q0ZFdHowUy8xMXVLQy9NOEswQndu?=
 =?utf-8?B?UDhYNTcwUE13UGViN3BXRUVmOWJYWTMvc0FCS2tTWm9SN0ViRXV5Uk5WZ200?=
 =?utf-8?B?UEd4RDNaYWsyTzl1ZUdIWFRFVkp2U1YwL0Z1WVh1amErdFp2Wld5UDNjUFdV?=
 =?utf-8?B?RHJrVFg1SHg1SDdXdzY0aFR6ZGNzVkxzYmJzSkJGYzZuWGw5bktEQTkxOHRN?=
 =?utf-8?B?TC9DY3RSRkhqT1ZCMDVPR3czL3M0QVc1R2svVXhPT1NyYjRDenV2M2VneS9l?=
 =?utf-8?B?c1pDVWZ4OUJKd2IvM2Y1eEF0OTRGTWxlMU9iN0JlWXBzZnJBZkxuQzY1eVFY?=
 =?utf-8?B?M21wbGErYS9paUY5SlV5a3l3MldEcnMxQUVMVDBsS1hRdkpsU05VS0gvTVJR?=
 =?utf-8?B?ei9TandSMTBhcDZSS3drS2VlUWVnSy9aSmE5c0xzdGFxeVhFYU5IL3dMaVZK?=
 =?utf-8?B?cmI1SEY3UVRIZ2t0d3VzamVPbHNWanNNclZtb243QnRDVk1Da1JTdlNvaVNo?=
 =?utf-8?B?QUNGb2p2TWhqL3BKWjR0MjduTGk3bTRGRkp5cWlIbDR6QTdtampsVXZvV05V?=
 =?utf-8?B?SC9qc2IweHJYVm9Ob2pVTFJFODZhenFkUWZwVmtKbENXUkJKSjlKbXlnVVFr?=
 =?utf-8?B?Z3pVZklkTkFwSitOUnZpMjZqTFVseEpweWdzZHczc3FFYWRFNWNrL3NESmkw?=
 =?utf-8?B?N1NVa1N0S2NqYTBUdkFuN0NsK3VPaFhPM1BYZVF5eHNheHdiNTdhUW9LcXhH?=
 =?utf-8?B?LzJ4YVk2Ni9ZK0c1OEZ5dDI3OHU2MzhlbzVmdFVBT0prQmRXd1pPVTErTzZY?=
 =?utf-8?B?bHd6UjBwRUlMdzVUdFZKRFRtUXp0aHFBRGVaZ1dpR2Y5SkJUdDlXN2t6Tndr?=
 =?utf-8?B?SXpkOVRXMDl1TlVBNndmTkQzVTZZdEZBL2pLOTliMXJZWUNTWUVrWTRSektH?=
 =?utf-8?B?SDcrWmtscGtjcVV3bjJOOGlqL1hpcExrV21QbFVUSG92R012ZnN3WTBUTm0v?=
 =?utf-8?B?Tm9CT2kzTXlrajNkQk1GaExjWmI3eW1yRW9mS1RCcisxYjVXQzFOa3JzYVZX?=
 =?utf-8?B?b0twQVdua05JUmFEVjVBRkZUUHllY1dTRGhwKy9WekljQlFMYUdOdWdyUVFT?=
 =?utf-8?B?UTVUZzE2ODRzK1EwQWFpWS9RYjE1cWxIeHR3Rm9OaGtaOWY2ZzdtNmtaQXlZ?=
 =?utf-8?B?TTEwMGFXV0pzS3NtNEZWYmlwSXpRTE9oQXcyRVMzNzFJTDZBeDY2UnFQWXpP?=
 =?utf-8?B?TmZ3dXhiSzVBcGMvb0N1dHc1VnNpemd0dzNpc3hvWW04ZjJRclEyRkxFRFdK?=
 =?utf-8?B?dVlEWHR3NWNTNk9SNHFRWm1DeTBCSm1ZQ04weG40QWIyMzdkU1VaZXFjUDlP?=
 =?utf-8?B?djJySjdnRFJHSXJuMjJaZE5mOHpkQU1nSHlhMGNlSDNwbDU2aWhKenl3VjNJ?=
 =?utf-8?B?dS9UYVlDNC9xUjJMUzArOWtYKzl4aWo2TXlkL1lmcm81NmZjQ0IxeitWdm1Q?=
 =?utf-8?B?NzF3SlFUZ0RzUE1XZy9PSElNZ1JSbXVZVS9yMnJBZ2NmNEZkQkhsNDdLZytS?=
 =?utf-8?B?VWJZVWxDSVpDNG42S1NpSmJiYlp4c2F3c2hJQzZ1VGtiYW9TTkNYL2JzR2Y0?=
 =?utf-8?B?V1pRbHAwRmVtWVc2cFdueTdUY1pibFkreDZ5d2NyUzY5QVFLb090RVdtQlJI?=
 =?utf-8?B?MzA3eXJUdXhvT1VEYzVGajViWnppLzR5QVZMNVlHQUxsOFdleklmYVdiRjcr?=
 =?utf-8?B?OWJVeXVITHdqc0FBN1owb3ZJZDRBQjRnZFNHK0t3NDFQRFlBRFZ4MEZOUTcy?=
 =?utf-8?B?UTZBZW5wVmdQNEtzV3JFakpKa2Q3TENtMWJWWHhQdnFiMmhxRGw2TXE2VGZX?=
 =?utf-8?B?b2o0SEgvRkxxc3YvNW1BcHlCWU9Jd3kvcUs1REZjZS96Nzc1WWtyNmxodnQ0?=
 =?utf-8?B?NjVXWndSa1IzRzhVemE2ZWw0YU1KYTJwakxtRmx3NXVuSWJpRFlDSTc0bGkr?=
 =?utf-8?B?VVMwamVrOVNKSFZBYVkrbUQxY1V4dkwvYTN0U1VCa3gyQmYzeDZWcmFnbFo1?=
 =?utf-8?B?OWpRakhzTlFDOWVaM3p4ZEkvN2RIOGZLb251RGRhNkI2R0ZUalhoTmlKczFE?=
 =?utf-8?B?dU9jVjJLcEJDMUl5d3lXdmVsZEY3eXVNRVV4K0pZMVB5eHFabWxmRmMwTkdR?=
 =?utf-8?B?b2c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 1bf085a5-dd76-4b06-8615-08dbb49c5198
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Sep 2023 20:59:28.1227
 (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: mbZ6z6wpGA7F51TYILg1u2nfVxrLWsiepUCEMwXwocTkXudSOogBXnmmCYddSN/CxhfcdqQcgVo4FJCKxhfxESVufAtrNYdhUbQCpNsLjbY=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB7811
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (fry.vger.email [0.0.0.0]); Wed, 13 Sep 2023 13:59:38 -0700 (PDT)
X-Spam-Status: No, score=-2.3 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on fry.vger.email

Hi Ilpo,

On 9/13/2023 4:02 AM, Ilpo Järvinen wrote:
> On Tue, 12 Sep 2023, Reinette Chatre wrote:
>> On 9/11/2023 4:19 AM, Ilpo Järvinen wrote:
>>> Feature check in validate_resctrl_feature_request() takes in the test
>>> name string and maps that to what to check per test.
>>>
>>> Pass resource and feature names to validate_resctrl_feature_request()
>>> directly rather than deriving them from the test name inside the
>>> function which makes the feature check easier to extend for new test
>>> cases.
>>>
>>> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>>> Cc: <stable@vger.kernel.org>
>>
>> This does not seem to be stable material.
> 
> Alone it isn't, but both 2/5 and this 3/5 are prerequisites for 4/5 as 
> shown by the tags there.
> 
>>> ---
>>>  tools/testing/selftests/resctrl/resctrl.h     |  6 +-
>>>  .../testing/selftests/resctrl/resctrl_tests.c | 10 +--
>>>  tools/testing/selftests/resctrl/resctrlfs.c   | 69 ++++++++-----------
>>>  3 files changed, 34 insertions(+), 51 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
>>> index dd07463cdf48..89ced4152933 100644
>>> --- a/tools/testing/selftests/resctrl/resctrl.h
>>> +++ b/tools/testing/selftests/resctrl/resctrl.h
> 
>>> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
>>> index bd36ee206602..bd547a10791c 100644
>>> --- a/tools/testing/selftests/resctrl/resctrlfs.c
>>> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
>>> @@ -10,6 +10,8 @@
>>>   */
>>>  #include "resctrl.h"
>>>  
>>> +#include <limits.h>
>>> +
>>
>> Could you please include <limits.h> before the local resctrl.h?
> 
> Believe me I tried that first but it did not work. So this intentionally 
> in the current order as resctrl.h defines _GNU_SOURCE which is among 
> things that tends to alter many things. If I reorder them, the build gives 
> me these issues:
> 
> resctrlfs.c: In function ‘taskset_benchmark’:
> resctrlfs.c:284:2: warning: implicit declaration of function ‘CPU_ZERO’; 
> did you mean ‘FP_ZERO’? [-Wimplicit-function-declaration]
>   284 |  CPU_ZERO(&my_set);
>       |  ^~~~~~~~
>       |  FP_ZERO
> resctrlfs.c:285:2: warning: implicit declaration of function ‘CPU_SET’ 
> [-Wimplicit-function-declaration]
>   285 |  CPU_SET(cpu_no, &my_set);
>       |  ^~~~~~~
> resctrlfs.c:287:6: warning: implicit declaration of function 
> ‘sched_setaffinity’ [-Wimplicit-function-declaration]
>   287 |  if (sched_setaffinity(bm_pid, sizeof(cpu_set_t), &my_set)) {
>       |      ^~~~~~~~~~~~~~~~~
> 
> It might be useful to move _GNU_SOURCE define into Makefile though to 
> avoid these kind of issues (but that's not material for this patch).

How about a #define _GNU_SOURCE in this file as an intermediate step?
I did see your patch making this change but cannot see how it is
coordinated with fixing the include order in this file.

> 
>>>  static int find_resctrl_mount(char *buffer)
>>>  {
>>>  	FILE *mounts;
>>> @@ -604,63 +606,46 @@ char *fgrep(FILE *inf, const char *str)
>>>  
>>>  /*
>>>   * validate_resctrl_feature_request - Check if requested feature is valid.
>>> - * @resctrl_val:	Requested feature
>>> + * @resource:	Required resource (e.g., MB, L3, L2, L3_MON, etc.)
>>> + * @feature:	Feature to be checked under resource (can be NULL). This path
>>> + *		is relative to the resource path.
>>
>> I do not think "this path" is accurate. @feature is not a path but an entry
>> within the mon_features file.
> 
> Yes, agreed.
> 
>> Also please note that mon_features only exists for L3_MON, none of the other
>> listed resources have an associated mon_features file in resctrl. This
>> function is created to be generic has specific requirements on what
>> valid (never checked) parameters should be. This may be ok with the usage
>> but it should not pretend to be generic.
> 
> So are you recommending I split this function into two where the new one 
> would do the mon_features check?

No need to split the function. That seems overkill considering its
captive usage. I think a snippet making its usage clear will be helpful.
Something like:

	@feature: <description>. Can only be set for L3_MON. Must be
		  NULL for all other resources.

Please feel free to improve.


> 
>>>  	char *res;
>>>  	FILE *inf;
>>>  	int ret;
>>>  
>>> -	if (!resctrl_val)
>>> +	if (!resource)
>>>  		return false;
>>>  
>>>  	ret = find_resctrl_mount(NULL);
>>>  	if (ret)
>>>  		return false;
>>>  
>>> -	if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR))) {
>>> -		if (!stat(L3_PATH, &statbuf))
>>> -			return true;
>>> -	} else if (!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) {
>>> -		if (!stat(MB_PATH, &statbuf))
>>> -			return true;
>>> -	} else if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) ||
>>> -		   !strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR))) {
>>> -		if (!stat(L3_MON_PATH, &statbuf)) {
>>> -			inf = fopen(L3_MON_FEATURES_PATH, "r");
>>> -			if (!inf)
>>> -				return false;
>>> -
>>> -			if (!strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR))) {
>>> -				res = fgrep(inf, "llc_occupancy");
>>> -				if (res) {
>>> -					found = true;
>>> -					free(res);
>>> -				}
>>> -			}
>>> -
>>> -			if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR))) {
>>> -				res = fgrep(inf, "mbm_total_bytes");
>>> -				if (res) {
>>> -					free(res);
>>> -					res = fgrep(inf, "mbm_local_bytes");
>>> -					if (res) {
>>> -						found = true;
>>> -						free(res);
>>> -					}
>>> -				}
>>> -			}
>>> -			fclose(inf);
>>> -		}
>>> -	}
>>> +	snprintf(res_path, sizeof(res_path), "%s/%s", INFO_PATH, resource);
>>> +
>>> +	if (stat(res_path, &statbuf))
>>> +		return false;
>>> +
>>> +	if (!feature)
>>> +		return true;
>>> +
>>> +	snprintf(res_path, sizeof(res_path), "%s/%s/mon_features", INFO_PATH, resource);
>>> +	inf = fopen(res_path, "r");
>>> +	if (!inf)
>>> +		return false;
>>> +
>>> +	res = fgrep(inf, feature);
>>> +	free(res);
>>> +	fclose(inf);
>>>  
>>> -	return found;
>>> +	return res;
>>
>> This is unexpected. Function should return bool but instead returns a char * that
>> has been freed?
> 
> Okay, I understand it looks confusing when relying on implicit conversion 
> to boolean (despite being functionally correct).
> 
> I can do this instead to make it more obvious the intention is to convert 
> the result to boolean and not return a pointer:
> 	return !!res;
> 

Looks good to me. 

Reinette