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

[Haitao Huang]

On Tue, 30 Jan 2024 09:22:14 -0600, Huang, Kai <kai.huang@xxxxxxxxx> 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