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

[Huang, Kai]

On 1/05/2024 7:51 am, Haitao Huang wrote:
> From: Kristen Carlson Accardi <kristen@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxx>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> Signed-off-by: Kristen Carlson Accardi <kristen@xxxxxxxxxxxxxxx>
> Co-developed-by: Haitao Huang <haitao.huang@xxxxxxxxxxxxxxx>
> Signed-off-by: Haitao Huang <haitao.huang@xxxxxxxxxxxxxxx>
> Tested-by: Jarkko Sakkinen <jarkko@xxxxxxxxxx>
> ---

Free free to add:

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

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?