[v16,08/16] x86/sgx: Encapsulate uses of the global LRU

Commit Message

Haitao Huang Aug. 21, 2024, 1:53 a.m. UTC

To support the per-cgroup reclamation, each cgroup will have its own
"per-cgroup LRU" and EPC pages will be in its owner cgroup's LRU instead
of the global LRU. Abstract the code that is directly working with the
global LRU into functions reusable with per-cgroup LRUs.

Currently the basic reclamation procedure, sgx_reclaim_pages() directly
reclaims pages from the global LRU. Change it to take in an LRU.

Note the global EPC reclamation will still be needed when the total EPC
usage reaches the system capacity while usages of some cgroups are below
their respective limits. Create a separate wrapper for the global
reclamation, sgx_reclaim_pages_global(), passing in the global LRU to
the new sgx_reclaim_pages() now. Later it will be revised to reclaim
from multiple LRUs from all EPC cgroups instead of a single global LRU.

Wrap the existing emptiness check of the global LRU with a helper so
that it can be changed later to work with multiple LRUs when per-cgroup
LRU comes to play.

Also the per-cgroup EPC reclaim and global EPC reclaim will have
different check on whether they should be done.  Rename the existing
sgx_should_reclaim() to sgx_should_reclaim_global() to separate the two
cases.

Co-developed-by: Sean Christopherson <[e-mail]>
Signed-off-by: Sean Christopherson <[e-mail]>
Co-developed-by: Kristen Carlson Accardi <[e-mail]>
Signed-off-by: Kristen Carlson Accardi <[e-mail]>
Signed-off-by: Haitao Huang <[e-mail]>
---
v16:
- Regroup all abstraction related to global LRU usage to this patch from
  different patches in previous version. Position this before adding
per-cgroup reclaim. (Kai)

V13:
- Rename sgx_can_reclaim() to sgx_can_reclaim_global() and
sgx_should_reclaim() to sgx_should_reclaim_global(). (Kai)

V10:
- Add comments for the new function. (Jarkko)

V7:
- Split this out from the big patch, #10 in V6. (Dave, Kai)
---
 arch/x86/kernel/cpu/sgx/main.c | 63 ++++++++++++++++++++++------------
 1 file changed, 41 insertions(+), 22 deletions(-)

Comments

Huang, Kai Aug. 27, 2024, 11:13 a.m. UTC | #1

> +static inline bool sgx_can_reclaim_global(void)
> +{
> +	/*
> +	 * Now all EPC pages are still tracked in the @sgx_global_lru, so only
> +	 * check @sgx_global_lru.
> +	 *
> +	 * When EPC pages are tracked in the actual per-cgroup LRUs,
> +	 * replace with sgx_cgroup_lru_empty(misc_cg_root()).
> +	 */
> +	return !list_empty(&sgx_global_lru.reclaimable);
> +}

Firstly, sgx_cgroup_lru_empty() is only introduced in the next patch, so it's
wrong to mention it in the comment in _this_ patch.

It's weird to add the above comment here in this patch anyway, since ...

[...]


> +static void sgx_reclaim_pages_global(void)
> +{
> +	sgx_reclaim_pages(&sgx_global_lru);
>  }

... this function (which is no difference IMHO) doesn't have a similar comment
here.

The similar comment to this function is only added in the later "[PATCH v16
12/16] x86/sgx: Revise global reclamation for EPC cgroups":

 static void sgx_reclaim_pages_global(struct mm_struct *charge_mm)
 {
+	/*
+	 * Now all EPC pages are still tracked in the @sgx_global_lru.
+	 * Still reclaim from it.
+	 *
+	 * When EPC pages are tracked in the actual per-cgroup LRUs,
+	 * sgx_cgroup_reclaim_pages_global() will be called.
+	 */
 	sgx_reclaim_pages(&sgx_global_lru, charge_mm);
 }

So if the comment of sgx_can_reclaim_global() were needed, it's more
reasonable to add it in the later patch where the comment for
sgx_reclaim_pages_global() is also added IMHO?

Jarkko Sakkinen Aug. 27, 2024, 6:15 p.m. UTC | #2

On Wed Aug 21, 2024 at 4:53 AM EEST, Haitao Huang wrote:
> To support the per-cgroup reclamation, each cgroup will have its own
> "per-cgroup LRU" and EPC pages will be in its owner cgroup's LRU instead
> of the global LRU. Abstract the code that is directly working with the
> global LRU into functions reusable with per-cgroup LRUs.
>
> Currently the basic reclamation procedure, sgx_reclaim_pages() directly
> reclaims pages from the global LRU. Change it to take in an LRU.
>
> Note the global EPC reclamation will still be needed when the total EPC
> usage reaches the system capacity while usages of some cgroups are below
> their respective limits. Create a separate wrapper for the global
> reclamation, sgx_reclaim_pages_global(), passing in the global LRU to
> the new sgx_reclaim_pages() now. Later it will be revised to reclaim
> from multiple LRUs from all EPC cgroups instead of a single global LRU.
>
> Wrap the existing emptiness check of the global LRU with a helper so
> that it can be changed later to work with multiple LRUs when per-cgroup
> LRU comes to play.
>
> Also the per-cgroup EPC reclaim and global EPC reclaim will have
> different check on whether they should be done.  Rename the existing
> sgx_should_reclaim() to sgx_should_reclaim_global() to separate the two
> cases.
>
> Co-developed-by: Sean Christopherson <[e-mail]>
> Signed-off-by: Sean Christopherson <[e-mail]>
> Co-developed-by: Kristen Carlson Accardi <[e-mail]>
> Signed-off-by: Kristen Carlson Accardi <[e-mail]>
> Signed-off-by: Haitao Huang <[e-mail]>

Reviewed-by: Jarkko Sakkinen <[e-mail]>

BR, Jarkko

Huang, Kai Aug. 27, 2024, 11:58 p.m. UTC | #3

On 27/08/2024 11:13 pm, Huang, Kai wrote:
> 
>> +static inline bool sgx_can_reclaim_global(void)
>> +{
>> +	/*
>> +	 * Now all EPC pages are still tracked in the @sgx_global_lru, so only
>> +	 * check @sgx_global_lru.
>> +	 *
>> +	 * When EPC pages are tracked in the actual per-cgroup LRUs,
>> +	 * replace with sgx_cgroup_lru_empty(misc_cg_root()).
>> +	 */
>> +	return !list_empty(&sgx_global_lru.reclaimable);
>> +}
> 
> Firstly, sgx_cgroup_lru_empty() is only introduced in the next patch, so it's
> wrong to mention it in the comment in _this_ patch.
> 
> It's weird to add the above comment here in this patch anyway, since ...

With this comment moved to patch "x86/sgx: Revise global reclamation for 
EPC cgroups",

Reviewed-by: Kai Huang <[e-mail]>

Patch

diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 5d5f6baac9c8..47dfba6f45af 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -37,6 +37,21 @@  static inline struct sgx_epc_lru_list *sgx_epc_page_lru(struct sgx_epc_page *epc
 	return &sgx_global_lru;
 }
 
+/*
+ * Check if there is any reclaimable page at global level.
+ */
+static inline bool sgx_can_reclaim_global(void)
+{
+	/*
+	 * Now all EPC pages are still tracked in the @sgx_global_lru, so only
+	 * check @sgx_global_lru.
+	 *
+	 * When EPC pages are tracked in the actual per-cgroup LRUs,
+	 * replace with sgx_cgroup_lru_empty(misc_cg_root()).
+	 */
+	return !list_empty(&sgx_global_lru.reclaimable);
+}
+
 static atomic_long_t sgx_nr_free_pages = ATOMIC_LONG_INIT(0);
 
 /* Nodes with one or more EPC sections. */
@@ -287,10 +302,10 @@  static void sgx_reclaimer_write(struct sgx_epc_page *epc_page,
 }
 
 /*
- * Take a fixed number of pages from the head of the active page pool and
- * reclaim them to the enclave's private shmem files. Skip the pages, which have
- * been accessed since the last scan. Move those pages to the tail of active
- * page pool so that the pages get scanned in LRU like fashion.
+ * Take a fixed number of pages from the head of a given LRU and reclaim them to
+ * the enclave's private shmem files. Skip the pages, which have been accessed
+ * since the last scan. Move those pages to the tail of the list so that the
+ * pages get scanned in LRU like fashion.
  *
  * Batch process a chunk of pages (at the moment 16) in order to degrade amount
  * of IPI's and ETRACK's potentially required. sgx_encl_ewb() does degrade a bit
@@ -299,7 +314,7 @@  static void sgx_reclaimer_write(struct sgx_epc_page *epc_page,
  * problematic as it would increase the lock contention too much, which would
  * halt forward progress.
  */
-static void sgx_reclaim_pages(void)
+static void sgx_reclaim_pages(struct sgx_epc_lru_list *lru)
 {
 	struct sgx_epc_page *chunk[SGX_NR_TO_SCAN];
 	struct sgx_backing backing[SGX_NR_TO_SCAN];
@@ -310,10 +325,9 @@  static void sgx_reclaim_pages(void)
 	int ret;
 	int i;
 
-	spin_lock(&sgx_global_lru.lock);
+	spin_lock(&lru->lock);
 	for (i = 0; i < SGX_NR_TO_SCAN; i++) {
-		epc_page = list_first_entry_or_null(&sgx_global_lru.reclaimable,
-						    struct sgx_epc_page, list);
+		epc_page = list_first_entry_or_null(&lru->reclaimable, struct sgx_epc_page, list);
 		if (!epc_page)
 			break;
 
@@ -328,7 +342,7 @@  static void sgx_reclaim_pages(void)
 			 */
 			epc_page->flags &= ~SGX_EPC_PAGE_RECLAIMER_TRACKED;
 	}
-	spin_unlock(&sgx_global_lru.lock);
+	spin_unlock(&lru->lock);
 
 	for (i = 0; i < cnt; i++) {
 		epc_page = chunk[i];
@@ -351,9 +365,9 @@  static void sgx_reclaim_pages(void)
 		continue;
 
 skip:
-		spin_lock(&sgx_global_lru.lock);
-		list_add_tail(&epc_page->list, &sgx_global_lru.reclaimable);
-		spin_unlock(&sgx_global_lru.lock);
+		spin_lock(&lru->lock);
+		list_add_tail(&epc_page->list, &lru->reclaimable);
+		spin_unlock(&lru->lock);
 
 		kref_put(&encl_page->encl->refcount, sgx_encl_release);
 
@@ -381,10 +395,15 @@  static void sgx_reclaim_pages(void)
 	}
 }
 
-static bool sgx_should_reclaim(unsigned long watermark)
+static bool sgx_should_reclaim_global(unsigned long watermark)
 {
 	return atomic_long_read(&sgx_nr_free_pages) < watermark &&
-	       !list_empty(&sgx_global_lru.reclaimable);
+		sgx_can_reclaim_global();
+}
+
+static void sgx_reclaim_pages_global(void)
+{
+	sgx_reclaim_pages(&sgx_global_lru);
 }
 
 /*
@@ -394,8 +413,8 @@  static bool sgx_should_reclaim(unsigned long watermark)
  */
 void sgx_reclaim_direct(void)
 {
-	if (sgx_should_reclaim(SGX_NR_LOW_PAGES))
-		sgx_reclaim_pages();
+	if (sgx_should_reclaim_global(SGX_NR_LOW_PAGES))
+		sgx_reclaim_pages_global();
 }
 
 static int ksgxd(void *p)
@@ -415,10 +434,10 @@  static int ksgxd(void *p)
 
 		wait_event_freezable(ksgxd_waitq,
 				     kthread_should_stop() ||
-				     sgx_should_reclaim(SGX_NR_HIGH_PAGES));
+				     sgx_should_reclaim_global(SGX_NR_HIGH_PAGES));
 
-		if (sgx_should_reclaim(SGX_NR_HIGH_PAGES))
-			sgx_reclaim_pages();
+		if (sgx_should_reclaim_global(SGX_NR_HIGH_PAGES))
+			sgx_reclaim_pages_global();
 
 		cond_resched();
 	}
@@ -585,7 +604,7 @@  struct sgx_epc_page *sgx_alloc_epc_page(void *owner, enum sgx_reclaim reclaim)
 			break;
 		}
 
-		if (list_empty(&sgx_global_lru.reclaimable)) {
+		if (!sgx_can_reclaim_global()) {
 			page = ERR_PTR(-ENOMEM);
 			break;
 		}
@@ -600,7 +619,7 @@  struct sgx_epc_page *sgx_alloc_epc_page(void *owner, enum sgx_reclaim reclaim)
 			break;
 		}
 
-		sgx_reclaim_pages();
+		sgx_reclaim_pages_global();
 		cond_resched();
 	}
 
@@ -613,7 +632,7 @@  struct sgx_epc_page *sgx_alloc_epc_page(void *owner, enum sgx_reclaim reclaim)
 		sgx_put_cg(sgx_cg);
 	}
 
-	if (sgx_should_reclaim(SGX_NR_LOW_PAGES))
+	if (sgx_should_reclaim_global(SGX_NR_LOW_PAGES))
 		wake_up(&ksgxd_waitq);
 
 	return page;