Received-SPF: pass (google.com: domain of linux-kernel+bounces-40652-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <e486cafe-86f7-47d5-9d11-eaca2007e5db@intel.com>
Date: Fri, 26 Jan 2024 13:08:19 -0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 2/5] selftests/resctrl: Add helpers for the
 non-contiguous test
Content-Language: en-US
To: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>,
	<fenghua.yu@intel.com>, <shuah@kernel.org>
CC: <linux-kernel@vger.kernel.org>, <linux-kselftest@vger.kernel.org>,
	<ilpo.jarvinen@linux.intel.com>
References: <cover.1706180726.git.maciej.wieczor-retman@intel.com>
 <85b1efc3ddd698b3ac81aa72a6dc987ee17da3e2.1706180726.git.maciej.wieczor-retman@intel.com>
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <85b1efc3ddd698b3ac81aa72a6dc987ee17da3e2.1706180726.git.maciej.wieczor-retman@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Z1ZkT0RZS1JSbTVqREllWi93ejJ5WCtVcWFlV25mMklQbzBNK05MbmQvaVJL?=
 =?utf-8?B?TlRBYlF0dE1tTjVLcG5GemRwVzVRYlJCZFQ4ekgwTDM1MDJaa3RLZEV6VlV6?=
 =?utf-8?B?ZVRBN0NtK21LaE5qSXhkbkRZZklVWExjQWVmUVhQUnk5bkpNbmVFeUJzbHN1?=
 =?utf-8?B?Vk5sYS9rZngyZjlCeXdibzdaeU14enFRVk9OaXBCU21EVWZIVitGUEJLcUlL?=
 =?utf-8?B?TEtGQjM1OUt4K3hsaEdSMGYzcFMzQzdTdHQzcmVTdE5KLyszM2pYSVBlKzgr?=
 =?utf-8?B?VWtmRXVnNVhOVHVkMGVyYmdHbGJVUUhUTFVtSnNxWWsyT25odjg3Q1czS25n?=
 =?utf-8?B?ZTFyZjNGc0JUbFQ2N2ZuTFVnVlROaWQyYUl4YnpraXhQNm5tTXl4TldOa1hI?=
 =?utf-8?B?R0Z4cDBSTGtPQ2htS01xd1NIbDVFZUdPVVBBTSs2SXZmTlU0azd5dFIvNDY2?=
 =?utf-8?B?dFR3blBPM28wQllxbnFlaThzSEZPUkd4ZTZybUhLZmdPaGdPeE00YVhzQm52?=
 =?utf-8?B?cFYxMGRRbEVGbW5kN0daYlArWUF0VSszdUNjdmo5VkxGeVgraURUUzJyMVlD?=
 =?utf-8?B?N3ZWNDBjQjBjQmcxSHgvMVNiNFBtZmgwOWk1VzBPbXRkSnVXWEFzUDBOaXNB?=
 =?utf-8?B?am1CUXJaZXJDTVI2d1AwQzN0bExoZVBUa2Q1WndqL2ZLWW1yZ1RxQVdiMWhw?=
 =?utf-8?B?aytVOFZBcldLNEhKUGRkTGEzWW1lM2dyRHBEWmdJb3BFakh4UGRtcSt3SkRE?=
 =?utf-8?B?U01XZ2tDb09lWkdBMlBPeVpZdWtZdFM4QmhOQ0JSSHF4dEZTMVB1aUo0ZXRI?=
 =?utf-8?B?Y2VlcXJPS0JXaGFyanhjbnNJU1Jtbi9CcFc0TGhuM2hxSDRWMSswd1VWWlQ0?=
 =?utf-8?B?WE93MmY3a29FSlU3cDNTck1mMVBVbzJjWnRYWHdFNmhTMHFmb25taitUMkQ4?=
 =?utf-8?B?bG9RcFBEdG81cmNtV1J0R2lLOWxlbXVUbHFXUC9BUmFtUnhlSEFKOEtrVEE1?=
 =?utf-8?B?aHZLbk9uS21BMGwybXZDTW9OUmV1U0ovNk9ST3pLZnl2eDczRDVMdU11ZzBM?=
 =?utf-8?B?UjFldDh1VFFRcWF4N1dPTVVUdWRyQkdZRTdQUHZTZEZTeE1HZXlUR2NkN0tI?=
 =?utf-8?B?UzlRYno4MVNmVDk3cWFsa2pwQ1Q1cy85bDNhRE1rbFhxYUhzVklhajBYSjJp?=
 =?utf-8?B?SjlvTjVaZ2JFblAzVWtHeW4xOHlpUUxILzFITDdIaUZwN3ZPQTZBNHZURytG?=
 =?utf-8?B?RUppN2VFTnVQanNUeE5rcmtVNzFNZGpFTTQrcWFSTHhqSFlaMEEyWklLWXJV?=
 =?utf-8?B?OVk1enhuSldNSFRqMCtFdXcya21wZldUSjFVUVZ0QTJSU1g2cTNLd1luRlM0?=
 =?utf-8?B?OEl6TGVsZEM3bkt5VmtLQkFEMGQwQU1uL3pJODJLY29HYTI5UGcxR3NaTVpS?=
 =?utf-8?B?WEdGVjhqQTdFbGYrWUUySFNpNG5WRXhzTzd2NTVyL1hkMm9tRmFLOGtzRnMw?=
 =?utf-8?B?VER2T0w2MlEvdTNLNGVwaGs1UkxGQ09FVllRNG5UTjkwWWJRZng3Wng3WW1Q?=
 =?utf-8?B?a3FXdU9IUHE4N0NXSUpzNURtblRRaGF5M1NUTEFQNm5GMWROOUkrV1FNQWhT?=
 =?utf-8?B?QzVMN0UwNWh5VkQrdVZGeFdvcFA5dlpPNjhrVWFHTzNRanhGTUdjU1kvN0c1?=
 =?utf-8?B?dDdNUWtxS0NQWmVFZmhVdDE2UHVpSnZtRXNGNFp6bFRpd1ptL0Q0eW1xaHBW?=
 =?utf-8?B?aDBHNUE4clc1RnFWbHI3b3pzZlFJZlk3cXhsMXVxdFk2b3RldnBkeWM3cDJY?=
 =?utf-8?B?VlhPd1pKWEJXc1BRcmhtZWVYRTVXdGVyNXYvRGk1NENxRDl3MllkRlMyL3RZ?=
 =?utf-8?B?QjIvbnBwVnUrUklLWHNQb0VIcjRkaUFkMFhJUWNBb2tJdzdvUzVERFR0UzNC?=
 =?utf-8?B?dW9Sd1ZhR29MbXVDUTJ0OXFYaXdlSCtKWGVOK05HOWxFYXd1clVYc3dCeE91?=
 =?utf-8?B?OVZqTFIva1N4S1NpVHRzc3BOalcyaXBuUU9Xc0N6SFJsMFZGYW5BQ09aUnox?=
 =?utf-8?B?T2ppSWdJVUtqT09pczR0UGFDcHRXbVBJV2kxZS8weDR1a2pYMXdhbmlhSlFP?=
 =?utf-8?B?bnBTdDRMS20rWnU5M2lha1lmclFUYTR1Z2k1YWZheEdTd0RncXdJQ0xMSk5p?=
 =?utf-8?B?OHc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: a0e8b13c-1c87-49e1-a6e1-08dc1eb2ed16
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Jan 2024 21:08:21.2041
 (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: KH+HYRnqTDTfsoJtOye9RcY20kRjImCe1VPegUzEIghSdel5c1//Vq6tJJaKJML5+UJjJjMq7QyffofBF/u3K3veR6dCNzgTxOh2V1Qbve8=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB7570
X-OriginatorOrg: intel.com

Hi Maciej,

On 1/25/2024 3:10 AM, Maciej Wieczor-Retman wrote:
> The CAT non-contiguous selftests have to read the file responsible for
> reporting support of non-contiguous CBMs in kernel (resctrl). Then the
> test compares if that information matches what is reported by CPUID
> output.
> 
> Add a generic helper function to read an unsigned number from a file in
> /sys/fs/resctrl/info/<RESOURCE>/<FILE>.
> 
> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
> ---
> Changelog v3:
> - Rewrite patch message.
> - Add documentation and rewrote the function. (Reinette)
> 
> Changelog v2:
> - Add this patch.
> 
>  tools/testing/selftests/resctrl/resctrl.h   |  1 +
>  tools/testing/selftests/resctrl/resctrlfs.c | 39 +++++++++++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index a1462029998e..5116ea082d03 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -162,6 +162,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 resource_info_unsigned_get(const char *resource, const char *filename, unsigned int *val);
>  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 5750662cce57..cb5147c5f9a9 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -249,6 +249,45 @@ static int get_bit_mask(const char *filename, unsigned long *mask)
>  	return 0;
>  }
>  
> +/*

By not starting with /** the following will not be interpreted as kernel-doc
but the formatting does appear to follow the syntax, but not consistently so.
I think it would be more readable if the kernel-doc syntax is followed consistently.

> + * resource_info_unsigned_get - Read an unsigned value from a file in
> + * /sys/fs/resctrl/info/RESOURCE/FILENAME

"Read an unsigned value from /sys/fs/resctrl/info/RESOURCE/FILENAME"?

> + * @resource:	Resource name that matches directory names in

names? (plural)

> + *		/sys/fs/resctrl/info
> + * @filename:	Filename of a file located in a directory specified with the
> + *		'resource' variable.

I think this can be shortened to "File in /sys/fs/resctrl/info/@resource"

> + * @val:	Variable where the read value is saved on success.

"Contains read value on success."

(no need to refer to it as a variable/parameter, it is implied by syntax).

> + *
> + * Return: = 0 on success, < 0 on failure. On success the read value is saved into the 'val'
> + * variable.

"saved into the 'val' variable" -> "saved into @val" (since syntax indicates it is the parameter
there is no need to elaborate). 
Also please let lines in comments be of consistent length.

> + */


> +int resource_info_unsigned_get(const char *resource, const char *filename,
> +			       unsigned int *val)
> +{
> +	char reason[128], file_path[PATH_MAX];
> +	FILE *fp;
> +
> +	snprintf(file_path, sizeof(file_path), "%s/%s/%s", INFO_PATH, resource,
> +		 filename);
> +
> +	fp = fopen(file_path, "r");
> +	if (!fp) {
> +		snprintf(reason, sizeof(reason), "Error in opening %s file\n", filename);

(apart from other discussions). "file" in message seems redundant. It can just be "Error
opening %s". It may also be useful to print file_path instead of filename to be specific
of what the code tried to open.

> +		ksft_perror(reason);
> +		return -1;
> +	}
> +
> +	if (fscanf(fp, "%u", val) <= 0) {
> +		snprintf(reason, sizeof(reason), "Could not get %s's contents\n", filename);
> +		ksft_perror(reason);

filename -> file_path ?

> +		fclose(fp);
> +		return -1;
> +	}
> +
> +	fclose(fp);
> +	return 0;
> +}
> +


Reinette