Received-SPF: pass (google.com: domain of linux-kernel+bounces-46378-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Wed, 31 Jan 2024 12:48:34 +0100
From: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
To: Reinette Chatre <reinette.chatre@intel.com>
CC: <fenghua.yu@intel.com>, <shuah@kernel.org>,
	<linux-kernel@vger.kernel.org>, <linux-kselftest@vger.kernel.org>,
	<ilpo.jarvinen@linux.intel.com>
Subject: Re: [PATCH v3 2/5] selftests/resctrl: Add helpers for the
 non-contiguous test
Message-ID: <a2kk6npzrdy4qeimmb3ifdbuse64t3c7wbnn2gocdqxuzkcqm5@lwoy3ou2eihc>
References: <cover.1706180726.git.maciej.wieczor-retman@intel.com>
 <85b1efc3ddd698b3ac81aa72a6dc987ee17da3e2.1706180726.git.maciej.wieczor-retman@intel.com>
 <e486cafe-86f7-47d5-9d11-eaca2007e5db@intel.com>
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <e486cafe-86f7-47d5-9d11-eaca2007e5db@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?RH1ejaWFA05cnGos8QJZcVKaejp3vhZv/B6P7RZeiyuya02SzZGTqLUSsB?=
 =?iso-8859-1?Q?sAfySuIaGeUiiDu4m8FZPWMfmzIIi55aclKwDFUl/v8lHHn1u36y1iDxo8?=
 =?iso-8859-1?Q?DeS1ikR9XKWenuS1nlUtTcOkoLzOGRHGCUIOzNp8lEgqmppX7yLVW2oWY1?=
 =?iso-8859-1?Q?bmP8pdUGTuvN89dW4LGgbsvqnYBoXaDhl1hy0O/TeH2kP1KLCoDGF83wEm?=
 =?iso-8859-1?Q?rwr+yPZJuw8OLFjL50YCTBnNl3qGO/51Eb59CPQvOzFx1aGauj4Jx015TX?=
 =?iso-8859-1?Q?WQsz1WBGm11SqqfxJJNSt52ta7+H717jLsGTxuae3fqnqGM+dYeEN0I27B?=
 =?iso-8859-1?Q?4LFRFCbKeZ8tYN7TBS6plFmwZV/rH7UCBnoqdRUKIwYKlUAlCZy3G0Lvrb?=
 =?iso-8859-1?Q?4zwcwdzBdcc+gov/8OpqQGAvmDFui6SgcbTmvd0CcdWKw3WUQCub/+xB4x?=
 =?iso-8859-1?Q?69nZw8ruFTUSTv0ZHEbCWXNepWnpmKUhkjaAP2/a2nS6RK84uwJaHb5GSE?=
 =?iso-8859-1?Q?ZdYAy3DW3IUssRVaJiV1Hef+cxNI4YhRyFwW1AyHUxj9W9f3BjNidIBDGA?=
 =?iso-8859-1?Q?F2ApPnIQSwYa3CwlcjxKX9OZ3tiWjC78/gjRlfyV0bAocYVDNKB3mRybu0?=
 =?iso-8859-1?Q?PNpK7JJSUAhLC+gEMhzz7dAzsHxbAE1Bo9sbxlt1Xs2XXQQ72Konbw6egE?=
 =?iso-8859-1?Q?JE3ZFUfg5nEJ2v/i4LFxv22qMnwPaq7XhIztjC0exFox61ZBP9u3uSF0om?=
 =?iso-8859-1?Q?6aBZLEQrmD4finmoZsLF+DKolHZfL0O3YkGzPxFnFNf6AxOUkhTbP1oRwo?=
 =?iso-8859-1?Q?1pygje8ivcbdetqls8rCkghvy0iww/jHIg8ldF5MTzC4BGIwivkQ87tYOk?=
 =?iso-8859-1?Q?um62o/Xc8Jd9I9NpMrweQ/vB2HDHrXwPxLslVFg2URqEIQfBEgceWlfQ2D?=
 =?iso-8859-1?Q?b6zWVUoS693140rRFCeeGmJ0RYtJbzQdt6gVphO/Egmxg7smKcbW/LE107?=
 =?iso-8859-1?Q?9myHT/Kx5NYGabWdBIAtBqJiH2SdwytoW+DHlZilcPOZH3s5nZGFllL1Vw?=
 =?iso-8859-1?Q?l+pSMiYRkPXuAW4yxVoIaiPcxf/scy+hHt/bvDXIMw7osuE3BLTakEiHSB?=
 =?iso-8859-1?Q?URW+8tVrjJTQsHYa9/w4fqDvSeoSUhAK+uXA4YufZA2w+OCDEWyhm9n0Me?=
 =?iso-8859-1?Q?vLrut3pyptYTjPBBCPPS8kz5vn6UeipOekX5t7N64xv/4+5C78raF86p2w?=
 =?iso-8859-1?Q?DoOVbg5wcWxbaWtpigb0Slc0FAiwFdEsPQIJ10jP0IowCnKc/Lo+6lrK+B?=
 =?iso-8859-1?Q?Qxz8jupoEJoPGfWOnwNRRaI6oqoOpIUYMzcYGBnsDXf5X04KWxjtyID+uK?=
 =?iso-8859-1?Q?ZeGVRJ+k5IK2lBVShJLC/9n6BMjsx4CD/Wx1SECr/ZjhEHGr1vB9jnukO0?=
 =?iso-8859-1?Q?8pLgALjJSwBBW6Tky0Q8qUPPhCXancdiDFgtVrKFbMrWBkM35GqBx7qB1V?=
 =?iso-8859-1?Q?mH0kAbo2qc0Pu5uNEzlsmYzNBbqsesR9FD57O3dguawSxeS7Q1OHGXTIQc?=
 =?iso-8859-1?Q?T0lKXEOjqrwt+6rBnkXnjRFqOB5XqBPM/Nf8ThaTM5KyjTGGDgZ84uZpD3?=
 =?iso-8859-1?Q?bQss1siGGoJV2CcNF26OvKKpepdBjedfNP9KFW3G+BzWjrvg+Y5u+4AMFJ?=
 =?iso-8859-1?Q?WGdyscygALFpPWq7CsY=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 8b0e8e64-fe44-451d-8e8e-08dc22529284
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6231.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Jan 2024 11:48:42.2290
 (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: fdWIwK10nhhli8L1b8z442hlUzFb3iL/XySY1JouwNbXlsM3H/v4QajSGQ1yew5HOu2COn7ooue2Mo0vhGMzg2HaLwQW4AWWCYA+v0QOB+Q=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN0PR11MB5696
X-OriginatorOrg: intel.com

Hello!

On 2024-01-26 at 13:08:19 -0800, Reinette Chatre wrote:
>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.

Sure, I'll change it.

>
>> + * 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"?

Okay

>
>> + * @resource:	Resource name that matches directory names in
>
>names? (plural)

Right, it doesn't make much sense, I'll move it to singular.

>
>> + *		/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"

Sure, thanks

>
>> + * @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).

Right, I'll change it.

>
>> + *
>> + * 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). 

Sure, thanks

>Also please let lines in comments be of consistent length.

Okay, I'll keep it to 80 characters.

>
>> + */
>
>
>> +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.

Okay, I'll change it to file_path.

>
>> +		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 ?

Same as above.

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

-- 
Kind regards
Maciej Wiecz?r-Retman