Hi Jarkko
Thanks for your review.
On Tue, 04 Jun 2024 17:00:34 -0500, Jarkko Sakkinen <jarkko@xxxxxxxxxx>
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@xxxxxxxxxxxxxxx>
Reviewed-by: Jarkko Sakkinen <jarkko@xxxxxxxxxx>
Tested-by: Jarkko Sakkinen <jarkko@xxxxxxxxxx>
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@xxxxxxxxx/
}
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@xxxxxxxxx/
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@xxxxxxxxx/
Thanks
Haitao
