Received-SPF: pass (google.com: domain of linux-kernel+bounces-170264-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Content-Type: text/plain; charset=iso-8859-15; format=flowed; delsp=yes
To: jarkko@kernel.org, dave.hansen@linux.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, "Huang, Kai" <kai.huang@intel.com>
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 v13 11/14] x86/sgx: Abstract check for global reclaimable
 pages
References: <20240430195108.5676-1-haitao.huang@linux.intel.com>
 <20240430195108.5676-12-haitao.huang@linux.intel.com>
 <3eb37a6d-ff52-41b0-8f74-d9d049e6ce05@intel.com>
Date: Mon, 06 May 2024 13:00:46 -0500
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
From: "Haitao Huang" <haitao.huang@linux.intel.com>
Organization: Intel
Message-ID: <op.2nc2zkpywjvjmi@hhuan26-mobl.amr.corp.intel.com>
In-Reply-To: <3eb37a6d-ff52-41b0-8f74-d9d049e6ce05@intel.com>
User-Agent: Opera Mail/1.0 (Win32)

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