Received-SPF: pass (google.com: domain of linux-kernel+bounces-197292-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <1fa7f60d-6feb-4b99-bc3b-3dcdd22515f5@intel.com>
Date: Fri, 31 May 2024 11:18:14 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v5 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: <20240531131142.1716-1-ilpo.jarvinen@linux.intel.com>
 <20240531131142.1716-14-ilpo.jarvinen@linux.intel.com>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20240531131142.1716-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?blNMN0lzVS8rbXFFRUcwaXdhdlc5bmJXOGd4a1plRnNZSFdmZldvNUtvYUEx?=
 =?utf-8?B?MGJBeUV5aldhU1JmRGhpRloxUTlCbVFZbmJTeFJmN2pINkd3YTJvVmNRWFg0?=
 =?utf-8?B?RWpGUE5XUkZVZDlYeGZFeTc1TEY4Ui9zTmVLZERYYXVGckpXcGlocUtKdENw?=
 =?utf-8?B?RDJyZTdrTFduYUdUT1I4dGdxN0hVRm84LzRib0p4ZTBJb0ZUUmV6cEZ2ZFN2?=
 =?utf-8?B?bWpON0VrT0tJb3JYV2FGOHIybVlycis1Qk9lZys4NDArYzVJTExLTG9teks1?=
 =?utf-8?B?MDJ0RjJubWVvQjBPSFFQeUx2VnppZzN3cU4vcUVwaEVVMkpsNXo2ZWtIMlJZ?=
 =?utf-8?B?ZXVUeDBVVmgvRFlsU3QwNUl1V1REOS9oam1pYjM0TXpaSzVoMFFqd1gzNnRz?=
 =?utf-8?B?cUttNHZ6eTlRVUQvTjJnYVU5eklvSTdidzZIYjBMcitTR1ErTmpoYU93VUlW?=
 =?utf-8?B?dW1yeXdPazMreEJUbXc1YWY5dUNta0ZBb2hXZ1RSaGVzZXhIL3dDL2NuQzFh?=
 =?utf-8?B?Nk9HcmZRb3ZuZFQzcDIzZnRtV3RSTlhSWmVLc2dTODVhOHUyNjcwU05oUTVw?=
 =?utf-8?B?ZlltWTJvbUdXNTg2TFlvekEwRXVpdkhTQmFFVkpPOUpDcHEvRnhuTXhlTjht?=
 =?utf-8?B?SWlNbkRyYmVlWnJrOFJzdElpazBsU1FPTXdhNXo3Y1FQUUdka0kyM2MwQXFX?=
 =?utf-8?B?ZHEzS1VPSzNpR0JPcjBZd1I2R0lSZHFZemdTanpPY3ljeENTUlpCWGpqWHR4?=
 =?utf-8?B?dHlpUEJLV1VPbVZiK1NuVWhyM0R3cEZWa3g0Ny80VlJDdnlCVmNFTGovejlm?=
 =?utf-8?B?b241MnlTemJrN1N0aDJXakIwWnI1Y3N6K1h5Q2VaeG02clVxaEpUYU1acnhJ?=
 =?utf-8?B?YkxUWEYvcXM0QWlBZ2FoZHQrckxCYkQvcTNlWkQ1QTNjUTZicnMrRTN5dmNF?=
 =?utf-8?B?SzBhVzk2Vmd5VVcrV2JSZFBFTGtEajg2OTcyUnZjazVnSlFhSlJvWlJTNXpl?=
 =?utf-8?B?Nm54V3dsaFhNdnpncUlMNTdrOFdVVHRwN1huN2ZWWXdVLzBCdXdqYldXU2c5?=
 =?utf-8?B?b1hxMkVrQ0N5aldBd3FVVkUreXl6YnFVaTFsc1I0c2podXd6cTRLTWtvY1Nw?=
 =?utf-8?B?YWorSXNHWmhsdXFhdzFVT3ZQMmdBS2ZhR2dGWFJacVFwQ3pvT2VSTkRYV0xV?=
 =?utf-8?B?cXJyYzF6RUVSbGQwbjVNSStWdERzbVJUcGZHOXBFY3l1bnBmc0tNVldRR3pu?=
 =?utf-8?B?NVhGM2lxcFNybXlCVVk3M2xTYmd4ZGNiL2tETUtFYUdPNGhOSjZrcGNNZFRa?=
 =?utf-8?B?ZjFqS1dqdFFkZVBxQlhidngrMGFUVG1TOWE5TWd6ejVwem95Y1hMbGUxOUlR?=
 =?utf-8?B?eWIxT3h0UFgyNUVqa1JwRE42Z3Fha1JEakNYTFpTS0RwZ1ZOSVhFZDM2RDFV?=
 =?utf-8?B?elYzRnJvRlJHa3BRb2JTak9vdWthd0VWeUxYam1qeDNLK3V5WWV6ZllnWGtj?=
 =?utf-8?B?OC95ZzNVQnA3Y3RrNzMwOFY1RW9QdWtlMkxyTHJkcVFYS0g5M3hGbjVhWXRq?=
 =?utf-8?B?SHRxR3ViUWZZOGFOUmdLMDRVV0pobGxBa3FZZml6cTVvNG5KdVJkVUEyM3Ez?=
 =?utf-8?B?VThyM3VnYmJ1NXU4VFVVNUw4dWRkdnRuek4wR0Q3MTljRURxa3JjUlVMOTkz?=
 =?utf-8?B?Qlgrd2FBVzJFck9VVE45emxodnhkWHkvNkRkL3Y4TWtTVHg2Vng1L3ZibFg3?=
 =?utf-8?B?T1VGZWRoQ2xLcjNjSDRMMU9lS0d6RGJiOFdzYnFzdVprSC9taXRPVzNmalZw?=
 =?utf-8?B?RlRSRE93WWRGTlhsMS94d2pqbjgrWW8ycDJDUVUwMG0zcm9hRVBST1VnVTFQ?=
 =?utf-8?B?eXE4eXJLeTB3cGtsOWdleHNYZ0ZuT2plV2pOWFRPc3VCcjJtT1VubmMxRTVM?=
 =?utf-8?B?M0JGa1VsY2hCV2l4YmdvNXhaOGNZM1p2VnFycjNnMzloWllBSWx4YTN5OU1z?=
 =?utf-8?B?eGZoREh3YlVKSTFmY0dGZzMvQkVYci93Mmt3Y08yNzlqb1pWMlpVS1UzME81?=
 =?utf-8?B?enMxL1E2Tks2YnR3c3VIOU9yZWVIUCs4OE9Iekd1cVNpWDVZcUYrMEVhZU0w?=
 =?utf-8?B?dWpiWVAzZzg4TEtVVU5JbTBRWTQxTURVUFc3TTlxWmowUVBVbmxwN3JhdVV1?=
 =?utf-8?B?UkE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 13b57f15-dc83-440f-b6b7-08dc819e0a90
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 May 2024 18:18:16.2935
 (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: SVOeKaDYk0jxUUpbex8aHBerKTpD/ZvKgWPqGLduvnb4dNMTUgNJcYnns+EdO8HurC2APH7huBRblfZcHr9jZOMvVTsCJA+23fu8FW0NcaU=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR11MB8316
X-OriginatorOrg: intel.com

Hi Ilpo,

On 5/31/24 6:11 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).

This paragraph is no longer relevant and can be dropped.

> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
> v5:
> - Return 0 if grp_name is null (restore earlier behavior)
> - Move the root group note from comment into kerneldoc
> 
> 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 | 16 +++++++---------
>   2 files changed, 9 insertions(+), 11 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 f4dc8ef23a8c..893041faaf18 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -456,6 +456,9 @@ int taskset_restore(pid_t bm_pid, cpu_set_t *old_affinity)
>    * @grp:	Full path and name of the group
>    * @parent_grp:	Full path and name of the parent group
>    *
> + * Creates a group @grp_name if it does not exist yet. If @grp_name is NULL,
> + * it is interpreted as the root group which always results success.

"always results success" -> "always results in success"?

With changelog and comment fixups addressed:
| Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette