Received-SPF: pass (google.com: domain of linux-kernel+bounces-204332-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Content-Type: text/plain; charset=iso-8859-15; format=flowed; delsp=yes
To: dave.hansen@linux.intel.com, kai.huang@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
Subject: Re: [PATCH v14 14/14] selftests/sgx: Add scripts for EPC cgroup
 testing
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>
 <D1SOT40TEXMI.A5J72PR5IWSP@kernel.org>
Date: Thu, 06 Jun 2024 08:13:14 -0500
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
From: "Haitao Huang" <haitao.huang@linux.intel.com>
Organization: Intel
Message-ID: <op.2ox4ccz7wjvjmi@hhuan26-mobl.amr.corp.intel.com>
In-Reply-To: <D1SOT40TEXMI.A5J72PR5IWSP@kernel.org>
User-Agent: Opera Mail/1.0 (Win32)

On Thu, 06 Jun 2024 00:32:55 -0500, Jarkko Sakkinen <jarkko@kernel.org>  
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