Received-SPF: pass (google.com: domain of linux-kernel+bounces-194532-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <bafcfe0c-e052-426f-be0d-9bcbb4c16a62@intel.com>
Date: Wed, 29 May 2024 10:48:46 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 13/16] selftests/resctrl: Convert ctrlgrp & mongrp to
 pointers
To: =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>,
	<linux-kselftest@vger.kernel.org>, Shuah Khan <shuah@kernel.org>, Babu Moger
	<babu.moger@amd.com>, =?UTF-8?Q?Maciej_Wiecz=C3=B3r-Retman?=
	<maciej.wieczor-retman@intel.com>
CC: <linux-kernel@vger.kernel.org>, Fenghua Yu <fenghua.yu@intel.com>, "Shuah
 Khan" <skhan@linuxfoundation.org>
References: <20240520123020.18938-1-ilpo.jarvinen@linux.intel.com>
 <20240520123020.18938-14-ilpo.jarvinen@linux.intel.com>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20240520123020.18938-14-ilpo.jarvinen@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?QnltcE1GNVVaN2xobEsvcWNLcmtiRWdjSlhKS2F6b3RCTlEvYlBTbVJqV2pW?=
 =?utf-8?B?c3ZoL1doaFNONUlIZG0xcERKd29GMDNBWFdRenlEWTNNdzc4Uld6NVFJSGhk?=
 =?utf-8?B?a2VKRGEwQklJSnZjWWFsNm5nUG01QWdHcW8xbDhKRVZJL1lFRVIwQ1lDM2U0?=
 =?utf-8?B?aUh6Vm1vQ21URXhrRFFZM2JNS2VyV2lPd3dmazZodWxqSENMUitvdmQzMldF?=
 =?utf-8?B?dFJ2NHo0TmpJeXZkK0pRWXRXdEdBY1ZYbDNyZGQxMHhBS1JKZHZORUttK0po?=
 =?utf-8?B?M0JPa3htQmxhdnEyUG13UFFEbWlCWGVWR25BcjBuZmgyQmVURlNyN09zMTlr?=
 =?utf-8?B?TGFaa0FRNmF0VDc5cmlRSFExQ09HcFpYd2w0Vi9wYnpMRUExVzV2S3huUnhJ?=
 =?utf-8?B?REJoUENoTUV6OUtXV09uMXRCaUZDS3Fxdzc2b2o5ZlZ1QlpnOHZmS2o2U1o2?=
 =?utf-8?B?ejl1OG9xeGk3YmJIT1B6djRRQzdMM3lXeE5sb1kzcStUVXJXUERFZ3NRbHJJ?=
 =?utf-8?B?eGxoUmtOUTVzdHlLRG9GOUN0TkIwaVJHclBsZkN3bE9XdENYV1ozWkVIR0gv?=
 =?utf-8?B?NS9XSTdIc3FHSlc2T2dpWGd6NGg2ODZKZ0paNWlFYlNPRXR6VVhnSmdKYVFG?=
 =?utf-8?B?Q2duRWVMOVdTNkc0eGNFN0duNTRQMW96RFM2Z0pDSDMxcXJ0ZTF5UGlPTUsr?=
 =?utf-8?B?cnltREZNY1hPS1Y4SGNCeWlZMDBra3NOcjlKdU9GT21BMGJmbXlhRHlsbnMz?=
 =?utf-8?B?azF0NDFkL3lLR0x3dms3Yzd3Wlc1c0NNYkNmZC9sY1BhZTM2ejZ1d20yN0Zv?=
 =?utf-8?B?dlhCYW5yWlk3N1A3dkRQVzZFb2ZycnZiUEVpTW5ERi8rV3BaTlQ1a3pjL2dM?=
 =?utf-8?B?b3VWV0pldkVvblFpK2pHbTdjeUNJQ2dFM2hKTHRFMmUyalc3cUx5NTVFV1ZH?=
 =?utf-8?B?bDZBdnRIQlRRczd1WTFub2IzbE5ZM09SRDZGV1FaNzlES3Q1dXNoL1JKbFR1?=
 =?utf-8?B?SmRrVHFEcFNOVXBKd3BOVzYyUWw5V0NqWjhpQ1h5aXVJTFgwOFk4RGxGK2Yx?=
 =?utf-8?B?emZDK0JCZGhEaTB1enpDYTdRUFRNSUJYQkRIRXF2MzR1NXRpSnJPL0xJQXM0?=
 =?utf-8?B?cXVuRmYxNXBIb3JZQ3haY1ZNL21OV3M5MDZISU9WbmlGVE10ck1YcDJReXB4?=
 =?utf-8?B?T1hHczlNcWFhVUIxRDZXKzFiSlhtMHVYMEo4YmpTZXZYL0EzVVdleFd3UkY3?=
 =?utf-8?B?Yzg5YTUxRFdHVnU1QjJFVGNoS2Nhd1luMnBBYWtIYkRqcSt2M1A0d2RFcTl3?=
 =?utf-8?B?NWlISUpWR1FMTHUyOGlVUUNDNkhIdnEzaEJHTTI5T3pGMEJRUGZWdmN0c0lC?=
 =?utf-8?B?eldTY25xZkhiWDNUWW9KL2U4VjdDUUI4WFFoQ0U5V29BbVJwZzBJOGRpcU9i?=
 =?utf-8?B?OFBKa2U1UTU4bWV0UHVFblVkRVNRYituRVJ0ZTVJdzRqUWRSV2p6NHRoWm5P?=
 =?utf-8?B?UzNiVHR1aW5NbmlJa2dZemdpNVREVDl2Snd4THNILzJ6WTA0N3hzTXJRdCth?=
 =?utf-8?B?SzIwUVYzQ2I3WklrR1k1MG43YXFUMDJOYzdoYUdPUEppbDZnN2U2WktEUWVU?=
 =?utf-8?B?VEt4bHQ3L2lLeTl0cHVvTnRUSDM2ZE4yeGVBaXZHTjVQaUdTalFDQVlPQVVP?=
 =?utf-8?B?WmVjU1JaZW4zS1g3OVlNZmVQSElValdEUW5wcGdWbEp6ZUd6UG5yV3IvZTVI?=
 =?utf-8?B?Yk44a1c1WmJ4cTdwdjJhV2lvVWtCV294ZmdYMWhaYzVYQnFTSmkxb3NEUjlR?=
 =?utf-8?B?Ykxzc1RSR1BZL2c3QzU0RnF2LzZhcmJsS2FyOExUclJpeHkwQ0x4dzdYRUlZ?=
 =?utf-8?B?UmVnYks4K3E3Y3hhK1FQQXFFVkVrVXhPb2lzaVdZZ3c4MjY5V0FvN21rSjJ1?=
 =?utf-8?B?dXFlb3R6TjRoVkRpbXlBeHpoN0hJeVhrQ1c3OHlPWEtzTk0rNE9EOEM5QWlR?=
 =?utf-8?B?SUp2NE5abEF5MXNESm5EZW8xaWthSmNKTGZMNkcxOXBTUHRxSHlYQ2JkUWVs?=
 =?utf-8?B?a0RsMzdyY0hIbGNlUkVUZU44eTYwcklPU3B6RytIY1JERWpXd1E3QnRDZzlO?=
 =?utf-8?B?cjlWVVlyK1NMZzFtYXcwaEtiUjI3bVA2SUtVd0JhbXEzOHBXWHN2VU9DanFq?=
 =?utf-8?B?K0E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: c3d0e134-6068-4205-e6a1-08dc8007983a
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 May 2024 17:48:48.7781
 (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: B3vDvgm49ykouhjsZ/uXfIuJ1ExdjcQh3pJvraBa4y5imdXaJv1kuXKGkn4H2wYyPKZG+5DmEwTLVDPtTgPE4BnCXeJCoSxKTWENUeOed18=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA0PR11MB7185
X-OriginatorOrg: intel.com

Hi Ilpo,

On 5/20/24 5:30 AM, Ilpo Järvinen wrote:
> The struct resctrl_val_param has control and monitor groups as char
> arrays but they are not supposed to be mutated within resctrl_val().
> 
> Convert the ctrlgrp and mongrp char array within resctrl_val_param to
> plain const char pointers and adjust the strlen() based checks to
> check NULL instead.
> 
> Convert !grp_name check in create_grp() into internal sanity check by
> returning error if the caller asked to create a group but doesn't
> provide a name for the group. The existing code already abides this by
> only calling create_grp() if mongrp is non-NULL so the error should
> never be returned with the current selftests (ctrlgrp is never NULL).
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
> 
> v3:
> - Removed wrong comment
> - Changed grp_name check to return -1 on fail (internal sanity check)
> ---
>   tools/testing/selftests/resctrl/resctrl.h   |  4 ++--
>   tools/testing/selftests/resctrl/resctrlfs.c | 15 +++++----------
>   2 files changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index 5967389038d4..a999fbc13fd3 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -91,8 +91,8 @@ struct resctrl_test {
>    */
>   struct resctrl_val_param {
>   	char		*resctrl_val;
> -	char		ctrlgrp[64];
> -	char		mongrp[64];
> +	const char	*ctrlgrp;
> +	const char	*mongrp;
>   	char		filename[64];
>   	unsigned long	mask;
>   	int		num_of_runs;
> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
> index a0e84157eb63..6b4448588666 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -464,13 +464,8 @@ static int create_grp(const char *grp_name, char *grp, const char *parent_grp)
>   	struct dirent *ep;
>   	DIR *dp;
>   
> -	/*
> -	 * At this point, we are guaranteed to have resctrl FS mounted and if
> -	 * length of grp_name == 0, it means, user wants to use root con_mon
> -	 * grp, so do nothing
> -	 */
> -	if (strlen(grp_name) == 0)
> -		return 0;
> +	if (!grp_name)
> +		return -1;

As I said during review of v2, this should not be an error. I went back to read
your comments and the argument that this is done for benefit of API is unclear since
the default control group does not have a name, exactly what create_grp() supports
when not returning an error in this case.

Reinette