Re: [PATCH v6 04/12] x86/sgx: Implement basic EPC misc cgroup functionality

["Huang, Kai" <kai.huang@intel.com>]

> > > 
> > 
> > That's true. I was thinking no need to have them done in separate calls.  
> > The caller has to check the return value for epc_cg instance first, then  
> > check result of try_charge. But there is really only one caller,  
> > sgx_alloc_epc_page() below, so I don't have strong opinions now.
> > 
> > With them separate, the checks will look like this:
> > if (epc_cg = sgx_get_current_epc_cg()) // NULL means cgroup disabled,  
> > should continue for allocation
> > {
> > 	if (ret =  sgx_epc_cgroup_try_charge())
> > 		return ret
> > }
> > // continue...
> > 
> > I can go either way.

Let's keep this aligned with other _try_charge() variants: return 'int' to
indicate whether the charge is done or not.

> > 
> > > 
> > > > > >  struct sgx_epc_page *sgx_alloc_epc_page(void *owner, bool reclaim)
> > > > > >  {
> > > > > >  	struct sgx_epc_page *page;
> > > > > > +	struct sgx_epc_cgroup *epc_cg;
> > > > > > +
> > > > > > +	epc_cg = sgx_epc_cgroup_try_charge();
> > > > > > +	if (IS_ERR(epc_cg))
> > > > > > +		return ERR_CAST(epc_cg);
> > > > > > 
> > > > > >  	for ( ; ; ) {
> > > > > >  		page = __sgx_alloc_epc_page();
> > > > > > @@ -580,10 +587,21 @@ 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();
> > > > > 
> > > > > What's the final behaviour?  IIUC it should be reclaiming from the
> > > > > *current* EPC
> > > > > cgroup?  If so shouldn't we just pass the @epc_cg to it here?
> > > > > 
> > > > > I think we can make this patch as "structure" patch w/o actually  
> > > > having
> > > > > EPC
> > > > > cgroup enabled, i.e., sgx_get_current_epc_cg() always return NULL.
> > > > > 
> > > > > And we can have one patch to change sgx_reclaim_pages() to take the
> > > > > 'struct
> > > > > sgx_epc_lru_list *' as argument:
> > > > > 
> > > > > 	void sgx_reclaim_pages_lru(struct sgx_epc_lru_list * lru)
> > > > > 	{
> > > > > 		...
> > > > > 	}
> > > > > 
> > > > > Then here we can have something like:
> > > > > 
> > > > > 	void sgx_reclaim_pages(struct sgx_epc_cg *epc_cg)
> > > > > 	{
> > > > > 		struct sgx_epc_lru_list *lru =			epc_cg ? &epc_cg->lru :
> > > > > &sgx_global_lru;
> > > > > 
> > > > > 		sgx_reclaim_pages_lru(lru);
> > > > > 	}
> > > > > 
> > > > > Makes sense?
> > > > > 
> > > > 
> > > > This is purely global reclamation. No cgroup involved.
> > > 
> > > Again why?  Here you are allocating one EPC page for enclave in a  
> > > particular EPC
> > > cgroup.  When that fails, shouldn't you try only to reclaim from the  
> > > *current*
> > > EPC cgroup?  Or at least you should try to reclaim from the *current*  
> > > EPC cgroup
> > > first?
> > > 
> > 
> > Later sgx_epc_cg_try_charge will take a 'reclaim' flag, if true, cgroup  
> > reclaims synchronously, otherwise in background and returns -EBUSY in  
> > that case. This function also returns if no valid epc_cg pointer  
> > returned.
> > 
> > All reclamation for *current* cgroup is done in sgx_epc_cg_try_charge().

This is fine, but I believe my question above is about where to reclaim when
"allocation" fails,  but not "try charge" fails.

And for "reclaim for current cgroup when charge fails", I don't think its even
necessary in this initial implementation of EPC cgroup.  You can just fail the
allocation when charge fails (reaching the limit).  Trying to reclaim when limit
is hit can be done later.

Please see Dave and Michal's replies here:

https://lore.kernel.org/lkml/7a1a5125-9da2-47b6-ba0f-cf24d84df16b@intel.com/#t
https://lore.kernel.org/lkml/yz44wukoic3syy6s4fcrngagurkjhe2hzka6kvxbajdtro3fwu@zd2ilht7wcw3/

> > 
> > So, by reaching to this point,  a valid epc_cg pointer was returned,  
> > that means allocation is allowed for the cgroup (it has reclaimed if  
> > necessary, and its usage is not above limit after charging).

I found memory cgroup uses different logic -- allocation first and then charge:

For instance:

static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
{
	......

        folio = vma_alloc_zeroed_movable_folio(vma, vmf->address);             
        if (!folio)                                                            
                goto oom;
                        
        if (mem_cgroup_charge(folio, vma->vm_mm, GFP_KERNEL))                  
                goto oom_free_page;
	
	......     
}

Why EPC needs to "charge first" and "then allocate"?
                             
> > 
> > But the system level free count may be low (e.g., limits of all cgroups  
> > may add up to be more than capacity). so we need to do a global  
> > reclamation here, which may involve reclaiming a few pages (from current  
> > or other groups) so the system can be at a performant state with minimal  
> > free count. (current behavior of ksgxd).
> > 
> I should have sticked to the orignial comment added in code. Actually  
> __sgx_alloc_epc_page() can fail if system runs out of EPC. That's the  
> really reason for global reclaim. 
> 

I don't see how this can work.  With EPC cgroup likely all EPC pages will go to
the individual LRU of each cgroup, and the sgx_global_lru will basically empty.
How can you reclaim from the sgx_global_lru?

I am probably missing something, but anyway, looks some high level design
material is really missing in the changelog.