Received-SPF: pass (google.com: domain of linux-kernel+bounces-28799-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Wed, 17 Jan 2024 10:59:46 +0100
From: Maciej =?utf-8?Q?Wiecz=C3=B3r-Retman?= <maciej.wieczor-retman@intel.com>
To: Reinette Chatre <reinette.chatre@intel.com>
CC: Fenghua Yu <fenghua.yu@intel.com>, Shuah Khan <shuah@kernel.org>,
	<ilpo.jarvinen@linux.intel.com>, <linux-kernel@vger.kernel.org>,
	<linux-kselftest@vger.kernel.org>
Subject: Re: [PATCH v2 2/4] selftests/resctrl: Add helpers for the
 non-contiguous test
Message-ID: <v4phdq2julcydn4wxj2rzsacjp6sqyf6h5w7nzyc436fwf2spy@4d2fal3jk2g4>
References: <cover.1702392177.git.maciej.wieczor-retman@intel.com>
 <c7b66a4682829894ec72d8a1f78e324233ef0535.1702392177.git.maciej.wieczor-retman@intel.com>
 <ddc900a6-7337-4721-ab0f-917c32e917ff@intel.com>
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <ddc900a6-7337-4721-ab0f-917c32e917ff@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?jwpS9lrcSv8mUB4m38lc7EELIu0v+jpom8Oj0YcUeG90yg43yy6bdumG6q?=
 =?iso-8859-1?Q?Te8l59n/EhGeJj7ZuR2VTW5Smm6R91sKMm166tCSh6008cN88iAiWz7Zdb?=
 =?iso-8859-1?Q?13MgvdhGpHtaeGuQtcDb07TCPsDHnJhUNoF/1VXSyLLN/taMyoezJzHnAU?=
 =?iso-8859-1?Q?AVhujpAIL/GDemRGg24YoP5SrPbGfsaFme/VgzfN01vBCVkaoqFqfCIE+6?=
 =?iso-8859-1?Q?m2Ci+hpg6om82YqgxeyxZUMZL5k8+AJBX7lzj7pnLLvwtSByDQvF46MmE2?=
 =?iso-8859-1?Q?zTaQB79tw1WCN/fkNGf9MNt/s3xU4Z/iQT0CVh1tLtkn3FAl771iO/9N7Y?=
 =?iso-8859-1?Q?URFRRr1ZImHpxY+orcwQq0nvBsWExYAr8eLEVeuS8BkUqxgHVwHKFcL6JM?=
 =?iso-8859-1?Q?D7CjTDMu5HWfUyBnBPCfWpQoPJVvEna0a42+cwzWCTw9HZwI5QhnGniUY6?=
 =?iso-8859-1?Q?Z9mXwxRS5FV/O2eiOYX8injpSgttWttF52O0ChIeuWH64Khf/Hyn3AJU/y?=
 =?iso-8859-1?Q?g8RHlK99Qas6rmBgixikiqGuXAc+mQcsPT2m4HVv04zGqCn/mWw4ukCKsk?=
 =?iso-8859-1?Q?/emV52oeBedmgIqrbC/0kOHEw7TPKrmZZYWckXjgpoOpkNBL6DsYeA80TC?=
 =?iso-8859-1?Q?lROxosnLi5lxtaCbQ7IZXIHAm+2cImVidyqBgPz/XX6WEqJUVXzNSSVt2/?=
 =?iso-8859-1?Q?zepwa7dyo3NHghvIp2tFI7K8felwwxOC45082U5VFC3jQQl+3f0fQYI3K1?=
 =?iso-8859-1?Q?KQqvNrJRpeH0E86PKCmFpr55aB9OWUg729wlSBWo3eU6VLAWqu8qKgOgTk?=
 =?iso-8859-1?Q?PSqlGYLLf6H7dXJo6sj2DNKtcmcHE/P77dcE6BqtSKDXvYmItVDjFy0kFi?=
 =?iso-8859-1?Q?KJqcra/t/wOBBtkBng4hgtXtIyDWOpoFdUfFnbTEVMgSr9sn/tOiw/L4Gv?=
 =?iso-8859-1?Q?HL5/R/K769tEvR+0btP6Xbctox0+v8j/e+nQuuF1pzyY6/HhCFgA4WOz1i?=
 =?iso-8859-1?Q?po1cMPZKYNXaXxPwJxz+uYh66IildidBKnKmcoDsmPpIN1AONyLItAtQTI?=
 =?iso-8859-1?Q?cF1V86QAPAqXe8B/a3B/S0VlKBt8otdZYGUGk6BsrAJqhQkLA0mBtwa636?=
 =?iso-8859-1?Q?0KOmbG6gY4QY93q014Rg0jGQG1fXD4CwxhozV2kF7uFe3ZPdo2+SFYVJ0S?=
 =?iso-8859-1?Q?+wOjHGjeuSbDwAz6wUmUMGh29dxxGZUYrbNEbi8f7KnvWpkXXzJvhKqXEh?=
 =?iso-8859-1?Q?CYfIWLd4RCLaF/+16JF0RCtqWeXqEqkFCaP6PE9Mx5yuunZbtMzvrHzuO9?=
 =?iso-8859-1?Q?YW10P36ow/w1LYCQ3jDYTTxD/6nZQzlioAUhOU4Znhwq4eyhfL+5AH5Rdj?=
 =?iso-8859-1?Q?hYpRAZECV2WgI5ACYw8doRf7DIgYdFttlSmzc/aZA3I5p1vlRflbb8oIsZ?=
 =?iso-8859-1?Q?0es+C5WFj3IQKPJ8YsHEgaggN2g30ccE2ExycwE+lucNM6LkV3uMxWIdYG?=
 =?iso-8859-1?Q?qF+/VfcJMyE94G8Ht1e1cG+hDAIkgTm6ZqI9Z+kCoh9+GkgwS7FR/3U6gI?=
 =?iso-8859-1?Q?J/P+0F8RicxoTpSYzfCd3NiZjc6eIXjdeOgOjkYINxsnViZimQMOkbblW/?=
 =?iso-8859-1?Q?HHZVqCxKLh9EvuaB+HY7y0biUj38M+X0WKwHMNXBZS7xFyizJnuxP1t6Vy?=
 =?iso-8859-1?Q?SOs6/kN8ZatpX/L9mFk=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 3c379025-2e1e-4729-9d60-08dc17430d18
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6231.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Jan 2024 09:59:53.1585
 (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: vCL7D/TfohzcOZfi+/7U2La+0hnoxsoMEm4FziaYOY/QwGa+G66v39HE6fWj+Tt90uCjfVZbxo66L45AHy7EknfEjMctokzunmvyF0BO6Fg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB6555
X-OriginatorOrg: intel.com

Hi!

On 2024-01-08 at 14:36:36 -0800, Reinette Chatre wrote:
>Hi Maciej,
>
>On 12/12/2023 6:52 AM, Maciej Wieczor-Retman wrote:
>> The CAT non-contiguous selftests have to read the file responsible for
>> reporting support of non-contiguous CBMs in Intel CAT. Then the test
>
>"in Intel CAT" -> "in kernel (resctrl)"

Sure, will fix.

>> compares if that information matches what is reported by CPUID output.
>> 
>> Add a generic helper function to read a chosen functionality support
>> information.
>
>Since this is a generic function that just reads a value from a file it
>cannot be assumed that the value represents functionality support.

Right, I'll rewrite this.

>> 
>> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
>> ---
>> Changelog v2:
>> - Added this patch.
>> 
>>  tools/testing/selftests/resctrl/resctrl.h   |  1 +
>>  tools/testing/selftests/resctrl/resctrlfs.c | 25 +++++++++++++++++++++
>>  2 files changed, 26 insertions(+)
>> 
>> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
>> index 739e16d08a7b..8f72d94b9cbe 100644
>> --- a/tools/testing/selftests/resctrl/resctrl.h
>> +++ b/tools/testing/selftests/resctrl/resctrl.h
>> @@ -161,6 +161,7 @@ unsigned int count_contiguous_bits(unsigned long val, unsigned int *start);
>>  int get_full_cbm(const char *cache_type, unsigned long *mask);
>>  int get_mask_no_shareable(const char *cache_type, unsigned long *mask);
>>  int get_cache_size(int cpu_no, const char *cache_type, unsigned long *cache_size);
>> +int read_info_res_file(const char *resource, const char *filename);
>>  void ctrlc_handler(int signum, siginfo_t *info, void *ptr);
>>  int signal_handler_register(void);
>>  void signal_handler_unregister(void);
>> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
>> index 0e97036a64b8..70333440ff2f 100644
>> --- a/tools/testing/selftests/resctrl/resctrlfs.c
>> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
>> @@ -249,6 +249,31 @@ static int get_bit_mask(const char *filename, unsigned long *mask)
>>  	return 0;
>>  }
>>  
>> +int read_info_res_file(const char *resource, const char *filename)
>
>Considering that this is intended to be a new generic utility, could you
>please add some function documentation?

Sure

>> +{
>> +	char file_path[PATH_MAX];
>> +	FILE *fp;
>> +	int ret;
>> +
>> +	snprintf(file_path, sizeof(file_path), "%s/%s/%s", INFO_PATH, resource,
>> +		 filename);
>> +
>> +	fp = fopen(file_path, "r");
>> +	if (!fp) {
>> +		perror("Error in opening sparse_masks file\n");
>
>The error messages do not match the goal of this function to be generic.
>Also, please note the recent cleanup done by Ilpo to replace the perror()
>by ksft_perror().

Thanks for catching this, will fix.

>> +		return -1;
>> +	}
>> +
>> +	if (fscanf(fp, "%u", &ret) <= 0) {
>
>I find this to be potentially confusing. The function claims to be a generic
>utility to read a value from a resctrl file ... but hidden within is that the
>value is required to be unsigned, which is then cast into an int. This could be
>made more specific and robust with something like below:
>	int resource_info_unsigned_get(const char *resource, const char *filename,
>					unsigned int *val)
>
>The return value will be the result of the request. If resource_info_unsigned_get()
>returns 0 then @val will contain the value read. 

Right, that might be confusing. Will fix according to your comment, thanks!

>> +		perror("Could not get sparse_masks contents\n");
>> +		fclose(fp);
>> +		return -1;
>> +	}
>> +
>> +	fclose(fp);
>> +	return ret;
>> +}
>> +
>>  /*
>>   * create_bit_mask- Create bit mask from start, len pair
>>   * @start:	LSB of the mask
>
>Reinette

-- 
Kind regards
Maciej Wiecz?r-Retman