Received-SPF: pass (google.com: domain of linux-kernel+bounces-54290-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <d6c1933c-b6af-4677-8f66-22d8a38d534f@intel.com>
Date: Mon, 5 Feb 2024 20:16:22 -0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 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-kselftest@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<ilpo.jarvinen@linux.intel.com>
References: <cover.1707130307.git.maciej.wieczor-retman@intel.com>
 <89b8965d563e4e61b95b20be55c26475d830b245.1707130307.git.maciej.wieczor-retman@intel.com>
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <89b8965d563e4e61b95b20be55c26475d830b245.1707130307.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?YmhoVzRrd0FYT2E5NkRlRlc4bG9hblpNSy80N0NGdnpaVUxxaXptZmkxTWM3?=
 =?utf-8?B?bnhtdEFLczNrbHhUVEhtSlhlNitrSzZuNm1yZjdqVmpIa1c2SWpqNXNLSlVq?=
 =?utf-8?B?VE5MMmVZZk1wVXRxQU5NODFJeldzUGJ4ZFBqYkcrRnQ2ZWE0ckkwZGZYeHdS?=
 =?utf-8?B?bTEzaDgrNGJrZ0oyMlphaE9jRVU3VStKbm5UcDE4eUhoSktrQWRLUFJFbWU5?=
 =?utf-8?B?aXZqai9VUjEwV2F6TWVuaGloaGJWRlRtdElXZVJ4QUFwQWR4UGUxWDk0MzBU?=
 =?utf-8?B?NjdKakZ2NzJqellsWUNKMCtVbXY1OFVkUDVTMUJjNmswUkFlQ2tlUUZmbCs0?=
 =?utf-8?B?NXNtUS83ZEZ0d0xkSndEeDZ3ZjNacEExN0Z4d04xc29OU3Qxem43Z2NMbmJH?=
 =?utf-8?B?d0szUEdnZTc4eDBYUnpVQTVLVHBxZEJkcnBkVFhIKzBIdTZPZlZmSUwrdGVT?=
 =?utf-8?B?YVdGTVQ4dXhrc3dlNkRoZFVyZE1FZ1U1RkYvSmdpU0pxdURLdkVVcDhnYTZI?=
 =?utf-8?B?UklnS051cncxeGs3cnZ4ZkkzaUwyUk9jUXdIZkt5WGNJZkdjQnJUbWJVM2dq?=
 =?utf-8?B?MHY4Umw3Q0lrR1pBVHp4dkFYN3k5TVRWdFBYeWs3UWpxUVU4WHoycURrdG1S?=
 =?utf-8?B?RGlud2lEdkY3M2daYXpRMEc0WUxJc2Q3dCs1WkdxckcxZ1pYM1NqYzZTSldB?=
 =?utf-8?B?R2l1RWswTVR4NllZVHZFQlM3T0ptWk8veW44QkxkS1llQnhQVWx6eFZMYVQ3?=
 =?utf-8?B?cEdMZ1hpbDdiSHBQcTdqdk9oRXo4U2IwdzlQaVVUaUFxY2JBbGFCVE5DbSth?=
 =?utf-8?B?TlpWYityVEh3bTdwYk9SYXQwTnhQN1ZBd0RLTWpoaFEzVmM2ZWJYZzRkbDk4?=
 =?utf-8?B?TmtvK0JpSlZWQ3U3eDlROVNZcU1SbG9Kck52Wk5yVlF6dGs4Y3JDY2UxNHUr?=
 =?utf-8?B?NjJyVHpPUmNFMDZCSU9hWi93V2JiV0VlbzBaWitrbC9SRldsY2ROVXNMeGJt?=
 =?utf-8?B?b3lpTnorUXRzMlRlT0VPZ2hPTkUxOXo4UkRXOFcwL0VDWnVJRDVKcjFKRzRi?=
 =?utf-8?B?T0ZPT3VLdExrOHpCbkZVUFZ3R1pKaml4ZDFFVWxaUTVmK1NJME9xazVibm42?=
 =?utf-8?B?TXAwSTlQOFBMdlFGNUNPb094TFRsTG5zQkJvdC85VHNHTHNLT3NwM09JR0pT?=
 =?utf-8?B?NzA1VWRSWVBDWnUxTDBtT3RicGdlelZHZkxDQU9ubno1V1F5YUgrazRXK2pD?=
 =?utf-8?B?T3p4ZGdOQ0NXRjB2NTI2OE9tcHRMcUhkRm50N1hoSHdHZElVNStJaGM4aXBn?=
 =?utf-8?B?YWRNWmJORmR5bkpPOThiRWh5NmU3M3I3bzJ1S0lxSlora1d5dHFDUFZ5c3pD?=
 =?utf-8?B?NndkR0pXQ2ZHbkgvSVNRaDlyTU5UTytNNElpSlRrOTY5ZVVYZ29uQjRkRFgw?=
 =?utf-8?B?SGFENm5JSWFkNjAxZnBJWDNqeVRxOXV2dG1ZcmZqYlgzczU5QnB5QnJHTmc4?=
 =?utf-8?B?dHNIL00xNzF5am1XSWFYUjAvRmdXVzdNVmZId2dOTW1PL1RlaGJ2TW9tVVhk?=
 =?utf-8?B?aDU5bTEwdmxibDRwZG5UK1F5S0hhK0FpcGJtYnZhK1pmZ3RzeFFPdENCUFJH?=
 =?utf-8?B?eThuc3NkYmNYS2R3TU1abFI4a2lSVktLbVk4cWV2QU44aTAwa0R4MmxmNUhX?=
 =?utf-8?B?dmFrTU9XdGNnMlF0d2xaSzRFY1JFTUZERklCRG5BZGl3aGJXeVFUZWZkRisy?=
 =?utf-8?B?dUZtRDhjZDl5K01yUGFLYUphNnFhL3ByTngrL0FSTk11RzYvNmpEY2VHM0ZR?=
 =?utf-8?B?NmlNZnpjNGRRM1l1RWpTUHdhY1g1Wi82YWZpbmQ2N2ViY21hSEw2UjF2Skxj?=
 =?utf-8?B?R3JsbUNGNllKb2lZbGxqL2FPNXBGNDFYRHlIeXRpZ1RKbk9xNDZVODJXUFZO?=
 =?utf-8?B?MkJ5OSt0VUtzckFBaEZ3eVE3UlROZ3VsU0RTSWY3cERBL2lTenBiTUxURjdU?=
 =?utf-8?B?NG9SVjlCTitMZW9ORmtDZlNSTno4L21lM2UyakNTbmEyL0xWbVViWWkvYlda?=
 =?utf-8?B?bHNnTExFWVFobzQySEFNcFZ5OWgwR3dEVDJEck5ZdkdRMGNiWjBKQjE2b1d3?=
 =?utf-8?B?TDRtQWVnTytIbHhLOHJ0cDVhVzRKc2FnU1hqd250ZHJ2Yjg0NFlNUnl3ZFI0?=
 =?utf-8?B?YUE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: efa2a7a1-0611-4dcb-ccbe-08dc26ca60f1
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Feb 2024 04:16:23.2634
 (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: ViH4NzE7GHc9H8U94lc+jJpoBKX15eLMDgxFN2+UuhvxRcGRHkPfZqGB8dEK95L7P05pXrJIpF0Qd2sLID1CNQ8vmd68wDSFWBBxKoLehoo=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA0PR11MB7956
X-OriginatorOrg: intel.com

Hi Maciej,

The subject mentions "helpers" (plural) that may not be accurate
anymore.

On 2/5/2024 4:08 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>.

The "a file in" above can be dropped or it should read "from a file in
/sys/fs/resctrl/info/<RESOURCE>".

> 
> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
> ---
> Changelog v4:
> - Rewrite function comment.
> - Redo ksft_perror() as ksft_print_msg(). (Reinette)
> 
> 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 | 36 +++++++++++++++++++++
>  2 files changed, 37 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..e0fbc46a917a 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -249,6 +249,42 @@ static int get_bit_mask(const char *filename, unsigned long *mask)
>  	return 0;
>  }
>  
> +/**

Apologies for not being clear in my previous comment. I do not
think we want to start exposing resctrl selftest comments to the
kernel-doc tool using this change. The tests are using kernel-doc
style but this is done outside from the kernel-doc tool's view.

> + * resource_info_unsigned_get - Read an unsigned value from
> + * /sys/fs/resctrl/info/RESOURCE/FILENAME
> + * @resource:	Resource name that matches directory name in
> + *		/sys/fs/resctrl/info
> + * @filename:	File in /sys/fs/resctrl/info/@resource
> + * @val:	Contains read value on success.
> + *
> + * Return: = 0 on success, < 0 on failure. On success the read
> + * value is saved into the @val.

It can just be "saved into @val"

> + */
> +int resource_info_unsigned_get(const char *resource, const char *filename,
> +			       unsigned int *val)
> +{
> +	char 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) {
> +		ksft_print_msg("Error in opening %s\n: %m\n", file_path);
> +		return -1;
> +	}

Apart from Ilpo's comment this can also just be "Error opening %s: ..."

> +
> +	if (fscanf(fp, "%u", val) <= 0) {
> +		ksft_print_msg("Could not get contents of %s\n: %m\n", file_path);
> +		fclose(fp);
> +		return -1;
> +	}
> +
> +	fclose(fp);
> +	return 0;
> +}
> +
>  /*
>   * create_bit_mask- Create bit mask from start, len pair
>   * @start:	LSB of the mask


Reinette