Received-SPF: pass (google.com: domain of linux-kernel+bounces-203353-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <7cbf3583-a23e-4437-afc2-1faeb4a1f436@intel.com>
Date: Thu, 6 Jun 2024 10:30:10 +1200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v14 14/14] selftests/sgx: Add scripts for EPC cgroup
 testing
To: Haitao Huang <haitao.huang@linux.intel.com>,
	<dave.hansen@linux.intel.com>, <tj@kernel.org>, <mkoutny@suse.com>,
	<linux-kernel@vger.kernel.org>, <linux-sgx@vger.kernel.org>,
	<x86@kernel.org>, <cgroups@vger.kernel.org>, <tglx@linutronix.de>,
	<mingo@redhat.com>, <bp@alien8.de>, <hpa@zytor.com>, <sohil.mehta@intel.com>,
	<tim.c.chen@linux.intel.com>, Jarkko Sakkinen <jarkko@kernel.org>
CC: <zhiquan1.li@intel.com>, <kristen@linux.intel.com>, <seanjc@google.com>,
	<zhanb@microsoft.com>, <anakrish@microsoft.com>,
	<mikko.ylinen@linux.intel.com>, <yangjie@microsoft.com>,
	<chrisyan@microsoft.com>
References: <20240531222630.4634-1-haitao.huang@linux.intel.com>
 <20240531222630.4634-15-haitao.huang@linux.intel.com>
 <D1RKK8CENNXI.1KMNDADV9C1YM@kernel.org>
 <op.2owf5xiwwjvjmi@hhuan26-mobl.amr.corp.intel.com>
Content-Language: en-US
From: "Huang, Kai" <kai.huang@intel.com>
In-Reply-To: <op.2owf5xiwwjvjmi@hhuan26-mobl.amr.corp.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?ajBqUzRvOVpBcXc1b0MrdDZyQmdVMG9aQ0F2enBSa2NSZzR1UXBxa3dCalps?=
 =?utf-8?B?a0JRVkZ1MzZwdDJuczNJeitJUC9PQTBZNkc3dmtaVi9rMGxHN0wzUmhZdndN?=
 =?utf-8?B?dHAwUWNrMGErTlU1ellHZk9mclNvMjZ0Zzh4YUFSOGlvc0h4OUVmM2ZXNFdH?=
 =?utf-8?B?QS83WTBVTHhqTDhXc0hkaUJmL2ZWY0RwWEUxWHZBS0N5R1FXOW1BUjVBSjd0?=
 =?utf-8?B?aDc0TEY1N1J6d2FKTFB5ME1SUnA4dWkrV3A4RXlmNFNIa2lLZjdYQXVYdU9R?=
 =?utf-8?B?bk9DT2xDRzJON2JwQ2syaHgveW1HZ2FDOG1kVXRwL3FJZ2t2UndIb3NzdElR?=
 =?utf-8?B?dS9oR3dua3V5bjIxOHRHYTlQSk1tUXhmUUYxNm81OE9NVzhSVHpudFZaVUpV?=
 =?utf-8?B?VEt3SGkxKy9Sa2pJTCtpSjlBWUxWRG9SR0JMdDZHNkVaTDkyMFhZZkRSY3g3?=
 =?utf-8?B?dmlJY2NPdktZM0hGbTFVWms4OTQvalNWZTJxUE53VGN4bEFSdDJ1ckFiKzUv?=
 =?utf-8?B?UzNPZ3VtSVdLQmxjSHBTRDhzM1JvcmNIQW5IVkNOcGN3UHNnMFpvZlhyRWRa?=
 =?utf-8?B?bkFmUTNSbW9mSEZCSzB6ekgvZk4yL3N5OGFXWFpteHU4V3l1U2MxZy9RUDVT?=
 =?utf-8?B?L0NwOXIrdmlHaDV0dnRieUI0NDd1OGo0SXBITnQwS1k2azVrWWV1MDRIZUJp?=
 =?utf-8?B?SVVzS29IUG9qS2tEKzc1S1d0NnhCeThLaVRmZXFPbEhxWXRyUGU0cW0wUEZv?=
 =?utf-8?B?a1kvM1NYY2RneE5Dbkc0MEpZTFhxZkxKMEhFUHUrTXNFYU5yN3pKTEtGekd5?=
 =?utf-8?B?VGJCelFZcW01Y2tPbFRyRTBLMzAzL29HTVM1TGdrdEtMdUpMR3J5ekhxSVNz?=
 =?utf-8?B?bGlYYUxadWFSNjI2TVBmNk9KMkdGck9rV1RZd2RhUmNJQkVCSFNMK2lvcTlt?=
 =?utf-8?B?UjB1MnUzNXpXNGN3T3dmVW5UY0JRY0tYRUNGbTRZbVVBK296SUs3QWJvRzB3?=
 =?utf-8?B?K2xOZzRXanA4K1JPK0QrNjVPMWdGOUxDRktjeFFQbnpRNURRNGsxaTRzS1Bk?=
 =?utf-8?B?emwvaEovQjh5dW9UdElYSlRGWDkzK2MwUmwzUUtLb1RKTFFjNGRnd3hCOXNY?=
 =?utf-8?B?R1A3MGhLS1Y2b3hNZlRZUitoUU8vWWFFSFdPbnppRlk0ZGJaVUIxSzdsc2gr?=
 =?utf-8?B?aEVCallRbWcxZkFMbVMvbkFWZlJwQWVCZ05zd2tpQ2lTdGR6b3lRNm55WnJl?=
 =?utf-8?B?bXZrbUJFQVRJZmMvVHFFaE82WTU4MDlvdDlsdTVxK0dDR1YrREtaY0RXQ2Zk?=
 =?utf-8?B?TlVUVGh0VFpTZGpCdzVWWEFQWFdNaVVZclg4bWJsQWIxcHM3N1J0eXNoRDd3?=
 =?utf-8?B?c0lNUXZrYVZsVDVZU1RLSWV1dUt2MFRuUFpQVGl0OXFDSFlEbndqNWExQjB6?=
 =?utf-8?B?Q2F2MndhclZYMENLSXMyaHVZaUtxalJvelFqbUJ4eStrT3JuU01zUnpWeFVu?=
 =?utf-8?B?dmtqVGlOdVVEVGE5V3A5Vllydm5TcWpiSGI2MWk3Nng2SVBaUVY4S1RHZFp3?=
 =?utf-8?B?MFF3ZFpYYzM5cmRxREd0eTJ3OWFTdlVBMmY0SjJsbkRsU0J0WTlCdkdQZEJv?=
 =?utf-8?B?dXowRXBWdUZXNTlHOUgzQ1ZaN205cTlmOVdtd0Noei9aNzlyTHFoTEE4MU9n?=
 =?utf-8?B?WTh4QzczOEp2eFdsRDhIZmxrdThSOWs0cm1jaDF4UVUwbnVmejVYUmk2bS9t?=
 =?utf-8?B?ZktHM2FJOURlQWRLU2JYWWR4Ty9SMEJUcnNtaUpGMEpwM1h3VnNMZnRnTGE5?=
 =?utf-8?B?YkRqejY2TVltYjh0M2ZxakUvZHRNcTdPNVU4aHdEZm8xWWJVclg0MDZTdkw5?=
 =?utf-8?B?Nlo1L1ZWUGx3Q1NKeStsNWMwdVJ4Z09VbFNDN2J0RTJ2WDBYK1J1S2ZDN1dY?=
 =?utf-8?B?THVVcW5zK1JOMEU5QmNhOElreHR2ZzNyNzBnL3B4bkIyTGlLdi9KeWZjTmtY?=
 =?utf-8?B?a3dxVjlvODFGdTFDRUVpa2tFODVVS2NpNThRZnMrbGNnaStvU21IOG9WdEtQ?=
 =?utf-8?B?TGZsOUNIWlMvL3VQcmtTSTU1VWVTeGovdEYwVW1wbTNheFZCOTg4S1Viamxy?=
 =?utf-8?Q?dObxX6xKTxYOr/KkMPmI6+HB+?=
X-MS-Exchange-CrossTenant-Network-Message-Id: e5ced2f7-9a6c-4c91-7ad0-08dc85af16e9
X-MS-Exchange-CrossTenant-AuthSource: BL1PR11MB5978.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Jun 2024 22:30:23.1678
 (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: LpBkyIoovjIUnmO4trvNkiWQPqPppWp2VizaAordjL6UmNB3SoFVesdV5MeGsQNNbv+a2xA/1ZEUuecUeN+yyA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR11MB6817
X-OriginatorOrg: intel.com


>> Reorg:
>>
>> void sgx_cgroup_init(void)
>> {
>>     struct workqueue_struct *wq;
>>
>>     /* eagerly allocate the workqueue: */
>>     wq = alloc_workqueue("sgx_cg_wq", wq_unbound | wq_freezable, 
>> wq_unbound_max_active);
>>     if (!wq) {
>>         pr_warn("sgx_cg_wq creation failed\n");
>>         return;
> 
> sgx_cgroup_try_charge() expects sgx_cg_wq, so it would break unless we 
> check and return 0 which was the initially implemented in v12. But then 
> Kai had some concern on that we expose all the interface files to allow 
> user to set limits but we don't enforce. To keep it simple we settled 
> down back to BUG_ON(). 

[...]

> This would only happen rarely and user can add 
> command-line to disable SGX if s/he really wants to start kernel in this 
> case, just can't do SGX.

Just to be clear that I don't like BUG_ON() either.  It's inevitable you 
will get attention because of using it.

This is a compromise that you don't want to reset "capacity" to 0 when 
alloc_workqueue() fails.

There are existing code where BUG_ON() is used during the kernel early 
boot code when memory allocation fails (e.g., see cgroup_init_subsys()), 
so it might be acceptable to use BUG_ON() here, but it's up to 
maintainers to decide whether it is OK.

[...]

>> With "--strict" flag I also catched these:
>>
>> CHECK: spinlock_t definition without comment
>> #1308: FILE: arch/x86/kernel/cpu/sgx/sgx.h:122:
>> +    spinlock_t lock;
>>
> Yes I had a comment but Kai thought it was too obvious and I can't think 
> of a better one that's not obvious so I removed:
> 
> https://lore.kernel.org/linux-sgx/9ffb02a3344807f2c173fe8c7cb000cd6c7843b6.camel@intel.com/
>
To be clear, my reply was really about the comment itself isn't useful, 
but didn't say we shouldn't use a comment here.

> 
>> CHECK: multiple assignments should be avoided
>> #444: FILE: kernel/cgroup/misc.c:450:
>> +        parent_cg = cg = &root_cg;
>>
> 
> This was also suggested by Kai a few versions back:
> https://lore.kernel.org/linux-sgx/8f08f0b0f2b04b90d7cdb7b628f16f9080687c43.camel@intel.com/
> 

I didn't know checkpatch complains this.  Feel free to revert back as it 
is trivial to me.