Date: Wed, 05 Jun 2024 10:33:23 -0500
From: "Haitao Huang" <haitao.huang@...ux.intel.com>
To: dave.hansen@...ux.intel.com, kai.huang@...el.com, tj@...nel.org,
 mkoutny@...e.com, linux-kernel@...r.kernel.org, linux-sgx@...r.kernel.org,
 x86@...nel.org, cgroups@...r.kernel.org, tglx@...utronix.de, mingo@...hat.com,
 bp@...en8.de, hpa@...or.com, sohil.mehta@...el.com,
 tim.c.chen@...ux.intel.com, "Jarkko Sakkinen" <jarkko@...nel.org>
Cc: zhiquan1.li@...el.com, kristen@...ux.intel.com, seanjc@...gle.com,
 zhanb@...rosoft.com, anakrish@...rosoft.com, mikko.ylinen@...ux.intel.com,
 yangjie@...rosoft.com, chrisyan@...rosoft.com
Subject: Re: [PATCH v14 14/14] selftests/sgx: Add scripts for EPC cgroup
 testing

Hi Jarkko

Thanks for your review.

On Tue, 04 Jun 2024 17:00:34 -0500, Jarkko Sakkinen <jarkko@...nel.org>  
wrote:

> On Sat Jun 1, 2024 at 1:26 AM EEST, Haitao Huang wrote:
>> With different cgroups, the script starts one or multiple concurrent SGX
>> selftests (test_sgx), each to run the unclobbered_vdso_oversubscribed
>> test case, which loads an enclave of EPC size equal to the EPC capacity
>> available on the platform. The script checks results against the
>> expectation set for each cgroup and reports success or failure.
>>
>> The script creates 3 different cgroups at the beginning with following
>> expectations:
>>
>> 1) small - intentionally small enough to fail the test loading an
>> enclave of size equal to the capacity.
>> 2) large - large enough to run up to 4 concurrent tests but fail some if
>> more than 4 concurrent tests are run. The script starts 4 expecting at
>> least one test to pass, and then starts 5 expecting at least one test
>> to fail.
>> 3) larger - limit is the same as the capacity, large enough to run lots  
>> of
>> concurrent tests. The script starts 8 of them and expects all pass.
>> Then it reruns the same test with one process randomly killed and
>> usage checked to be zero after all processes exit.
>>
>> The script also includes a test with low mem_cg limit and large sgx_epc
>> limit to verify that the RAM used for per-cgroup reclamation is charged
>> to a proper mem_cg. For this test, it turns off swapping before start,
>> and turns swapping back on afterwards.
>>
>> Add README to document how to run the tests.
>>
>> Signed-off-by: Haitao Huang <haitao.huang@...ux.intel.com>
>> Reviewed-by: Jarkko Sakkinen <jarkko@...nel.org>
>> Tested-by: Jarkko Sakkinen <jarkko@...nel.org>
>
> 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.

Another thought we could just guard queue_work() with a null check for  
sgx_cg_wq, but I thought that's also not good for user as it basically  
disables all async global and per-cgroup reclamation. If user needs run  
SGX in such a case, better have async anyways.

See previous discusion:  
https://lore.kernel.org/linux-sgx/e56216ee4d5f7ef6d97c70f56243a5ddc8dea19d.camel@intel.com/

> 	}
>
> 	misc_cg_set_ops(MISC_CG_RES_SGX_EPC, &sgx_cgroup_ops);
> 	sgx_cgroup_misc_init(misc_cg_root(), &sgx_cg_root);
>
> 	/* Depending on misc state, keep or destory the workqueue: */
> 	if (cgroup_subsys_enabled(misc_cgrp_subsys))
> 		sgx_cg_wq = wq;
> 	else
> 		destroy_workqueue(wq);
> }
>
> BTW, why two previous operations are performed if subsystem is not
> enabled?
>

Note this file is conditionally compiled on CGROUP_MISC KConfig. Even  
though subsystem can be 'disabled' from the command line when CGROUP_MISC  
is on, we still need initialize the misc root node which is static for  
below reason.

All process will be assigned to misc root if misc is disabled and  
get_current_misc_cg() will return the root. So we need initialize ->lru  
and ->reclaim_work for the root sgx cgroup.

> I.e. why not instead:
>
> 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;
> 	}
>

Same comment as above

> 	if (!cgroup_subsys_enabled(misc_cgrp_subsys)) {
> 		destroy_workqueue(wq);
> 		return;
> 	}
>
> 	misc_cg_set_ops(MISC_CG_RES_SGX_EPC, &sgx_cgroup_ops);
> 	sgx_cgroup_misc_init(misc_cg_root(), &sgx_cg_root);
> 	sgx_cg_wq = wq;
> }
>
> Finally, why this does not have __init?

This was my misunderstanding that __init only applies to the main init  
function. After looking at some actual usages, I tend to agree __init is  
appropriate. Thanks for catching this.

> And neither sgx_cgroup_misc_init().

This one is called when a specific misc cgroup is created (a new  
sub-folder in sysfs paraent in which misc is enabled), by the callback  
sgx_cgroup_alloc(). Not necessarily when subsys init.

>
> The names for these are also somewhat confusing, maybe something like:
>
> * __sgx_cgroups_misc_init()
> * sgx_cgroups_misc_init()
>
> And both with __init.
>
sgx_cgroup_init() is for the whole sgx cgroup support so original name is  
OK?
similar to sgx_drv_init(), sgx_vepc_init()?

I'm fine to rename the other one and add __init for sgx_cgroup().

> I just made a trivial checkpatch run as a final check, and spotted the
> warning on BUG_ON(), and noticed that this can't be right as it is but
> please comment and correct where I might have gotten something wrong.
>

See above

> 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/


> 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/

Thanks
Haitao

Powered by blists - more mailing lists