On Thu, 06 Jun 2024 00:32:55 -0500, Jarkko Sakkinen <jarkko@xxxxxxxxxx>
wrote:
On Wed Jun 5, 2024 at 6:33 PM EEST, Haitao Huang wrote:
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
~~~~~~~~~~~~~~
Sure:
"Keep it simple and corpse"
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.
Even disabling all of SGX would be a less catastrophical measure.
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:
Not great advice given. Please just document it. In patch, which
BUG_ON() I don't want to see my R-by in it, until I've reviewed an
updated version.
BR, Jarkko
Sure. that makes sens. So far I have following changes. I did not do the
renaming for the functions as I am not sure it is still needed. Let me
know otherwise and if I missed any more.
--- a/arch/x86/kernel/cpu/sgx/epc_cgroup.c
+++ b/arch/x86/kernel/cpu/sgx/epc_cgroup.c
@@ -329,7 +329,7 @@ const struct misc_res_ops sgx_cgroup_ops = {
.free = sgx_cgroup_free,
};
-void sgx_cgroup_init(void)
+int __init sgx_cgroup_init(void)
{
/*
* misc root always exists even if misc is disabled from command line.
@@ -345,7 +345,8 @@ void sgx_cgroup_init(void)
if (cgroup_subsys_enabled(misc_cgrp_subsys)) {
sgx_cg_wq = alloc_workqueue("sgx_cg_wq", WQ_UNBOUND |
WQ_FREEZABLE,
WQ_UNBOUND_MAX_ACTIVE);
- BUG_ON(!sgx_cg_wq);
+ return -ENOMEM;
}
+ return 0;
}
--- a/arch/x86/kernel/cpu/sgx/epc_cgroup.h
+++ b/arch/x86/kernel/cpu/sgx/epc_cgroup.h
@@ -99,7 +99,7 @@ bool sgx_cgroup_should_reclaim(struct sgx_cgroup
*sgx_cg);
struct misc_cg *sgx_cgroup_reclaim_pages(struct misc_cg *root,
struct misc_cg *next_cg,
struct mm_struct *charge_mm);
-void sgx_cgroup_init(void);
+int __init sgx_cgroup_init(void);
#endif /* CONFIG_CGROUP_MISC */
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -1045,7 +1045,7 @@ static int __init sgx_init(void)
if (!sgx_page_cache_init())
return -ENOMEM;
- if (!sgx_page_reclaimer_init()) {
+ if (!sgx_page_reclaimer_init() || !sgx_cgroup_init()) {
ret = -ENOMEM;
goto err_page_cache;
}
@@ -1067,9 +1067,6 @@ static int __init sgx_init(void)
if (sgx_vepc_init() && ret)
goto err_provision;
- /* Setup cgroup if either the native or vepc driver is active */
- sgx_cgroup_init();
-
return 0;
err_provision:
--- a/arch/x86/kernel/cpu/sgx/sgx.h
+++ b/arch/x86/kernel/cpu/sgx/sgx.h
@@ -119,6 +119,7 @@ static inline void *sgx_get_epc_virt_addr(struct
sgx_epc_page *page)
* cgroup.
*/
struct sgx_epc_lru_list {
+ /* Use this lock to protect access from multiple reclamation worker
threads */
spinlock_t lock;
struct list_head reclaimable;
};
--- a/kernel/cgroup/misc.c
+++ b/kernel/cgroup/misc.c
@@ -446,7 +446,8 @@ misc_cg_alloc(struct cgroup_subsys_state *parent_css)
int ret;
if (!parent_css) {
- parent_cg = cg = &root_cg;
+ parent_cg = &root_cg;
+ cg = &root_cg;
} else {
cg = kzalloc(sizeof(*cg), GFP_KERNEL);
if (!cg)
Thanks
Haitao
