Re: [PATCH v8 04/15] x86/sgx: Implement basic EPC misc cgroup functionality

["Haitao Huang" <haitao.huang@linux.intel.com>]

On Tue, 30 Jan 2024 09:22:14 -0600, Huang, Kai <kai.huang@intel.com> wrote:

>>  struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim)  {
>> +	struct sgx_epc_cgroup *epc_cg;
>>  	struct sgx_epc_page *page;
>> +	int ret;
>> +
>> +	epc_cg = sgx_get_current_epc_cg();
>> +	ret = sgx_epc_cgroup_try_charge(epc_cg);
>> +	if (ret) {
>> +		sgx_put_epc_cg(epc_cg);
>> +		return ERR_PTR(ret);
>> +	}
>>
>>  	for ( ; ; ) {
>>  		page = __sgx_alloc_epc_page();
>> @@ -567,8 +578,10 @@ struct sgx_epc_page *sgx_alloc_epc_page(void
>> *owner, bool reclaim)
>>  			break;
>>  		}
>>
>> -		if (list_empty(&sgx_active_page_list))
>> -			return ERR_PTR(-ENOMEM);
>> +		if (list_empty(&sgx_active_page_list)) {
>> +			page = ERR_PTR(-ENOMEM);
>> +			break;
>> +		}
>
> (Sorry for replying from Outlook because I am in travel for Chinese New  
> Year.)
>
> Perhaps I am missing something but I don't understand this change.
>
> An empty sgx_active_page_list means you cannot reclaim any page from it,  
> so why need to break?
>

This is to avoid any escape for sgx_put_epc_cg(), which is added in this  
version.

>>
>>  		if (!reclaim) {
>>  			page = ERR_PTR(-EBUSY);
>> @@ -580,10 +593,25 @@ struct sgx_epc_page *sgx_alloc_epc_page(void
>> *owner, bool reclaim)
>>  			break;
>>  		}
>>
>> +		/*
>> +		 * Need to do a global reclamation if cgroup was not full but
>> free
>> +		 * physical pages run out, causing __sgx_alloc_epc_page() to
>> fail.
>> +		 */
>>  		sgx_reclaim_pages();
>>  		cond_resched();
>>  	}
>
> And why adding this comment, especially in this patch?
>
> I don't see it brings additional clarity because there's only global  
> reclaim now, no matter whether cgroup is enabled or not.
>

True there is only global reclamation at the moment. The comment intended  
to help clarify why we can get here when try_charge() passes. It at least  
took me some thinking to realize that fact so I put it here to help remind  
this can happen even for cases that try_charge() passes. (In some earlier  
debugging for some concurrent issues, I actually tried mistakenly to  
remove this path hoping to narrow down some causes but made situation  
worse.)

I can remove if no one thinks this is needed.

Thanks
Haitao