Re: [PATCH v14 14/14] selftests/sgx: Add scripts for EPC cgroup testing

["Haitao Huang" <haitao.huang@xxxxxxxxxxxxxxx>]

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