diff mbox series

[v13,11/14] x86/sgx: Abstract check for global reclaimable pages

Message ID 20240430195108.5676-12-haitao.huang@linux.intel.com (mailing list archive)
State New
Headers show
Series Add Cgroup support for SGX EPC memory | expand

Commit Message

Haitao Huang April 30, 2024, 7:51 p.m. UTC 
From: Kristen Carlson Accardi <kristen@linux.intel.com>

For the global reclaimer to determine if any page available for
reclamation at the global level, it currently only checks for emptiness
of the global LRU. That will be inadequate when pages are tracked in
multiple LRUs, one per cgroup. For this purpose, create a new helper,
sgx_can_reclaim_global(), to abstract this check. Currently it only
checks the global LRU, later will check emptiness of LRUs of all cgroups
when per-cgroup tracking is turned on.

Replace all the checks for emptiness of the global LRU,
list_empty(&sgx_global_lru.reclaimable), with calls to
sgx_can_reclaim_global().

Rename sgx_should_reclaim() to sgx_should_reclaim_global() as it is used
to check if a global reclamation should be performed.

Co-developed-by: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
Co-developed-by: Haitao Huang <haitao.huang@linux.intel.com>
Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com>
Tested-by: Jarkko Sakkinen <jarkko@kernel.org>
---
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 | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

Comments

Huang, Kai May 2, 2024, 11:23 p.m. UTC | #1 
On 1/05/2024 7:51 am, Haitao Huang wrote:
> From: Kristen Carlson Accardi <kristen@linux.intel.com>
> 
> For the global reclaimer to determine if any page available for
> reclamation at the global level, it currently only checks for emptiness
> of the global LRU. That will be inadequate when pages are tracked in
> multiple LRUs, one per cgroup. For this purpose, create a new helper,
> sgx_can_reclaim_global(), to abstract this check. Currently it only
> checks the global LRU, later will check emptiness of LRUs of all cgroups
> when per-cgroup tracking is turned on.
> 
> Replace all the checks for emptiness of the global LRU,
> list_empty(&sgx_global_lru.reclaimable), with calls to
> sgx_can_reclaim_global().
> 
> Rename sgx_should_reclaim() to sgx_should_reclaim_global() as it is used
> to check if a global reclamation should be performed.
> 
> Co-developed-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
> Co-developed-by: Haitao Huang <haitao.huang@linux.intel.com>
> Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com>
> Tested-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---

Free free to add:

Reviewed-by: Kai Huang <kai.huang@intel.com>

One thing below:

[...]

> -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(struct mm_struct *charge_mm)
> @@ -405,7 +413,7 @@ static void sgx_reclaim_pages_global(struct mm_struct *charge_mm)
>    */
>   void sgx_reclaim_direct(void)
>   {
> -	if (sgx_should_reclaim(SGX_NR_LOW_PAGES))
> +	if (sgx_should_reclaim_global(SGX_NR_LOW_PAGES))
>   		sgx_reclaim_pages_global(current->mm);
>   }
>   

Hmm.. Sorry for not pointing out in the previous version.

Perhaps it makes more sense to do the rename in the patch ...

   x86/sgx: Add basic EPC reclamation flow for cgroup

... where we have actually introduced the concept of per-cgroup reclaim, 
and we literally have below change in that patch:

  void sgx_reclaim_direct(void)
  {
  	if (sgx_should_reclaim(SGX_NR_LOW_PAGES))
-		sgx_reclaim_pages();
+		sgx_reclaim_pages_global();
  }

So in that patch, the sgx_should_reclaim() literally just means we 
should do gloabl reclaim, but not the per-cgruop reclaim.  Thus, perhaps 
we just do the renaming here together with the new 
sgx_reclaim_pages_global().

If there's a new version needed, please move the renaming to that patch?
Haitao Huang May 6, 2024, 6 p.m. UTC | #2 
Hi Kai
Sorry there seems to be some delay in receiving my emails.

On Thu, 02 May 2024 18:23:06 -0500, Huang, Kai <kai.huang@intel.com> wrote:

>
>
> On 1/05/2024 7:51 am, Haitao Huang wrote:
>> From: Kristen Carlson Accardi <kristen@linux.intel.com>
>>  For the global reclaimer to determine if any page available for
>> reclamation at the global level, it currently only checks for emptiness
>> of the global LRU. That will be inadequate when pages are tracked in
>> multiple LRUs, one per cgroup. For this purpose, create a new helper,
>> sgx_can_reclaim_global(), to abstract this check. Currently it only
>> checks the global LRU, later will check emptiness of LRUs of all cgroups
>> when per-cgroup tracking is turned on.
>>  Replace all the checks for emptiness of the global LRU,
>> list_empty(&sgx_global_lru.reclaimable), with calls to
>> sgx_can_reclaim_global().
>>  Rename sgx_should_reclaim() to sgx_should_reclaim_global() as it is  
>> used
>> to check if a global reclamation should be performed.
>>  Co-developed-by: Sean Christopherson <sean.j.christopherson@intel.com>
>> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
>> Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com>
>> Co-developed-by: Haitao Huang <haitao.huang@linux.intel.com>
>> Signed-off-by: Haitao Huang <haitao.huang@linux.intel.com>
>> Tested-by: Jarkko Sakkinen <jarkko@kernel.org>
>> ---
>
> Free free to add:
>
> Reviewed-by: Kai Huang <kai.huang@intel.com>
>

Thanks

> One thing below:
>
> [...]
>
>> -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(struct mm_struct *charge_mm)
>> @@ -405,7 +413,7 @@ static void sgx_reclaim_pages_global(struct  
>> mm_struct *charge_mm)
>>    */
>>   void sgx_reclaim_direct(void)
>>   {
>> -	if (sgx_should_reclaim(SGX_NR_LOW_PAGES))
>> +	if (sgx_should_reclaim_global(SGX_NR_LOW_PAGES))
>>   		sgx_reclaim_pages_global(current->mm);
>>   }
>>
>
> Hmm.. Sorry for not pointing out in the previous version.
>
> Perhaps it makes more sense to do the rename in the patch ...
>
>    x86/sgx: Add basic EPC reclamation flow for cgroup
>
> ... where we have actually introduced the concept of per-cgroup reclaim,  
> and we literally have below change in that patch:
>
>   void sgx_reclaim_direct(void)
>   {
>   	if (sgx_should_reclaim(SGX_NR_LOW_PAGES))
> -		sgx_reclaim_pages();
> +		sgx_reclaim_pages_global();
>   }
>
> So in that patch, the sgx_should_reclaim() literally just means we  
> should do gloabl reclaim, but not the per-cgruop reclaim.  Thus, perhaps  
> we just do the renaming here together with the new  
> sgx_reclaim_pages_global().
>
> If there's a new version needed, please move the renaming to that patch?
>
Will do

BR
Haitao
diff mbox series

Patch

diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index f1bd15620b83..92bd3151a589 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -36,6 +36,14 @@  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)
+{
+	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. */
@@ -387,10 +395,10 @@  unsigned int sgx_reclaim_pages(struct sgx_epc_lru_list *lru, struct mm_struct *c
 	return cnt;
 }
 
-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(struct mm_struct *charge_mm)
@@ -405,7 +413,7 @@  static void sgx_reclaim_pages_global(struct mm_struct *charge_mm)
  */
 void sgx_reclaim_direct(void)
 {
-	if (sgx_should_reclaim(SGX_NR_LOW_PAGES))
+	if (sgx_should_reclaim_global(SGX_NR_LOW_PAGES))
 		sgx_reclaim_pages_global(current->mm);
 }
 
@@ -426,9 +434,9 @@  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))
+		if (sgx_should_reclaim_global(SGX_NR_HIGH_PAGES))
 			/* Indirect reclaim, no mm to charge, so NULL: */
 			sgx_reclaim_pages_global(NULL);
 
@@ -592,7 +600,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;
 		}
@@ -620,7 +628,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;