Received-SPF: pass (google.com: domain of linux-kernel+bounces-162648-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Content-Type: text/plain; charset=iso-8859-15; format=flowed; delsp=yes
To: "hpa@zytor.com" <hpa@zytor.com>, "tim.c.chen@linux.intel.com"
 <tim.c.chen@linux.intel.com>, "linux-sgx@vger.kernel.org"
 <linux-sgx@vger.kernel.org>, "x86@kernel.org" <x86@kernel.org>,
 "dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
 "jarkko@kernel.org" <jarkko@kernel.org>, "cgroups@vger.kernel.org"
 <cgroups@vger.kernel.org>, "linux-kernel@vger.kernel.org"
 <linux-kernel@vger.kernel.org>, "mkoutny@suse.com" <mkoutny@suse.com>,
 "tglx@linutronix.de" <tglx@linutronix.de>, "Mehta, Sohil"
 <sohil.mehta@intel.com>, "tj@kernel.org" <tj@kernel.org>, "mingo@redhat.com"
 <mingo@redhat.com>, "bp@alien8.de" <bp@alien8.de>, "Huang, Kai"
 <kai.huang@intel.com>
Cc: "mikko.ylinen@linux.intel.com" <mikko.ylinen@linux.intel.com>,
 "seanjc@google.com" <seanjc@google.com>, "anakrish@microsoft.com"
 <anakrish@microsoft.com>, "Zhang, Bo" <zhanb@microsoft.com>,
 "kristen@linux.intel.com" <kristen@linux.intel.com>, "yangjie@microsoft.com"
 <yangjie@microsoft.com>, "Li, Zhiquan1" <zhiquan1.li@intel.com>,
 "chrisyan@microsoft.com" <chrisyan@microsoft.com>
Subject: Re: [PATCH v12 12/14] x86/sgx: Turn on per-cgroup EPC reclamation
References: <20240416032011.58578-1-haitao.huang@linux.intel.com>
 <20240416032011.58578-13-haitao.huang@linux.intel.com>
 <524cf9b081d86ae61342fdfc370a3639d0010f94.camel@intel.com>
Date: Mon, 29 Apr 2024 11:05:21 -0500
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
From: "Haitao Huang" <haitao.huang@linux.intel.com>
Organization: Intel
Message-ID: <op.2mzyy7ktwjvjmi@hhuan26-mobl.amr.corp.intel.com>
In-Reply-To: <524cf9b081d86ae61342fdfc370a3639d0010f94.camel@intel.com>
User-Agent: Opera Mail/1.0 (Win32)

On Mon, 29 Apr 2024 05:49:13 -0500, Huang, Kai <kai.huang@intel.com> wrote:

>
>> +/*
>> + * Get the per-cgroup or global LRU list that tracks the given  
>> reclaimable page.
>> + */
>>  static inline struct sgx_epc_lru_list *sgx_lru_list(struct  
>> sgx_epc_page *epc_page)
>>  {
>> +#ifdef CONFIG_CGROUP_MISC
>> +	/*
>> +	 * epc_page->sgx_cg here is never NULL during a reclaimable epc_page's
>> +	 * life between sgx_alloc_epc_page() and sgx_free_epc_page():
>> +	 *
>> +	 * In sgx_alloc_epc_page(), epc_page->sgx_cg is set to the return from
>> +	 * sgx_get_current_cg() which is the misc cgroup of the current task,  
>> or
>> +	 * the root by default even if the misc cgroup is disabled by kernel
>> +	 * command line.
>> +	 *
>> +	 * epc_page->sgx_cg is only unset by sgx_free_epc_page().
>> +	 *
>> +	 * This function is never used before sgx_alloc_epc_page() or after
>> +	 * sgx_free_epc_page().
>> +	 */
>> +	return &epc_page->sgx_cg->lru;
>> +#else
>>  	return &sgx_global_lru;
>> +#endif
>>  }
>>
>>  /*
>> @@ -42,7 +63,8 @@ static inline struct sgx_epc_lru_list  
>> *sgx_lru_list(struct sgx_epc_page *epc_pag
>>   */
>>  static inline bool sgx_can_reclaim(void)
>>  {
>> -	return !list_empty(&sgx_global_lru.reclaimable);
>> +	return !sgx_cgroup_lru_empty(misc_cg_root()) ||
>> +	       !list_empty(&sgx_global_lru.reclaimable);
>>  }
>
> Shouldn't this be:
>
> 	if (IS_ENABLED(CONFIG_CGROUP_MISC))
> 		return !sgx_cgroup_lru_empty(misc_cg_root());
> 	else
> 		return !list_empty(&sgx_global_lru.reclaimable);
> ?
>
> In this way, it is consistent with the sgx_reclaim_pages_global() below.
>

I changed to this way because sgx_cgroup_lru_empty() is now defined in  
both KConfig cases.
And it seems better to minimize use of the KConfig variables based on  
earlier feedback (some are yours).
Don't really have strong preference here. So let me know one way of the  
other.

>>
>>  static atomic_long_t sgx_nr_free_pages = ATOMIC_LONG_INIT(0);
>> @@ -404,7 +426,10 @@ static bool sgx_should_reclaim(unsigned long  
>> watermark)
>>
>>  static void sgx_reclaim_pages_global(struct mm_struct *charge_mm)
>>  {
>> -	sgx_reclaim_pages(&sgx_global_lru, charge_mm);
>> +	if (IS_ENABLED(CONFIG_CGROUP_MISC))
>> +		sgx_cgroup_reclaim_pages(misc_cg_root(), charge_mm);
>> +	else
>> +		sgx_reclaim_pages(&sgx_global_lru, charge_mm);
>>  }
>>
>>  /*
>> @@ -414,6 +439,14 @@ static void sgx_reclaim_pages_global(struct  
>> mm_struct *charge_mm)
>>   */
>>  void sgx_reclaim_direct(void)
>>  {
>> +	struct sgx_cgroup *sgx_cg = sgx_get_current_cg();
>> +
>> +	/* Make sure there are some free pages at cgroup level */
>> +	if (sgx_cg && sgx_cgroup_should_reclaim(sgx_cg)) {
>> +		sgx_cgroup_reclaim_pages(misc_from_sgx(sgx_cg), current->mm);
>> +		sgx_put_cg(sgx_cg);
>> +	}
>
> Empty line.
>

Sure

>> +	/* Make sure there are some free pages at global level */
>>  	if (sgx_should_reclaim(SGX_NR_LOW_PAGES))
>
> Looking at the code, to me sgx_should_reclaim() is a little bit vague
> because from the name we don't know whether it interally checks the
> current cgroup or the global.  
> It's better to rename to sgx_should_reclaim_global().
>
> Ditto for sgx_can_reclaim() -> sgx_can_reclaim_global().
>
> And I think you can do the renaming in the previous patch, because in the
> changelog of your previous patch, it seems you have called out the two
> functions are for global reclaim.
>

ok

Thanks
Haitao