Re: [PATCH v6 09/12] x86/sgx: Restructure top-level EPC reclaim function

[Huang, Kai]

On Mon, 2023-12-11 at 22:04 -0600, Haitao Huang wrote:
> Hi Kai
> 
> On Mon, 27 Nov 2023 03:57:03 -0600, Huang, Kai <kai.huang@xxxxxxxxx> wrote:
> 
> > On Mon, 2023-11-27 at 00:27 +0800, Haitao Huang wrote:
> > > On Mon, 20 Nov 2023 11:45:46 +0800, Huang, Kai <kai.huang@xxxxxxxxx>  
> > > wrote:
> > > 
> > > > On Mon, 2023-10-30 at 11:20 -0700, Haitao Huang wrote:
> > > > > From: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> > > > > 
> > > > > To prepare for per-cgroup reclamation, separate the top-level  
> > > reclaim
> > > > > function, sgx_reclaim_epc_pages(), into two separate functions:
> > > > > 
> > > > > - sgx_isolate_epc_pages() scans and isolates reclaimable pages from  
> > > a
> > > > > given LRU list.
> > > > > - sgx_do_epc_reclamation() performs the real reclamation for the
> > > > > already isolated pages.
> > > > > 
> > > > > Create a new function, sgx_reclaim_epc_pages_global(), calling  
> > > those two
> > > > > in succession, to replace the original sgx_reclaim_epc_pages(). The
> > > > > above two functions will serve as building blocks for the  
> > > reclamation
> > > > > flows in later EPC cgroup implementation.
> > > > > 
> > > > > sgx_do_epc_reclamation() returns the number of reclaimed pages. The  
> > > EPC
> > > > > cgroup will use the result to track reclaiming progress.
> > > > > 
> > > > > sgx_isolate_epc_pages() returns the additional number of pages to  
> > > scan
> > > > > for current epoch of reclamation. The EPC cgroup will use the  
> > > result to
> > > > > determine if more scanning to be done in LRUs in its children  
> > > groups.
> > > > 
> > > > This changelog says nothing about "why", but only mentions the
> > > > "implementation".
> > > > 
> > > > For instance, assuming we need to reclaim @npages_to_reclaim from the
> > > > @epc_cgrp_to_reclaim and its descendants, why cannot we do:
> > > > 
> > > > 	for_each_cgroup_and_descendants(&epc_cgrp_to_reclaim, &epc_cgrp) {
> > > > 		if (npages_to_reclaim <= 0)
> > > > 			return;
> > > > 
> > > > 		npages_to_reclaim -= sgx_reclaim_pages_lru(&epc_cgrp->lru,
> > > > 					npages_to_reclaim);
> > > > 	}
> > > > 
> > > > Is there any difference to have "isolate" + "reclaim"?
> > > > 
> > > 
> > > This is to optimize "reclaim". See how etrack was done in sgx_encl_ewb.
> > > Here we just follow the same design as ksgxd for each reclamation cycle.
> > 
> > I don't see how did you "follow" ksgxd.  If I am guessing correctly, you  
> > are
> > afraid of there might be less than 16 pages in a given EPC cgroup, thus  
> > w/o
> > splitting into "isolate" + "reclaim" you might feed the "reclaim" less  
> > than 16
> > pages, which might cause some performance degrade?
> > 
> > But is this a common case?  Should we even worry about this?
> > 
> > I suppose for such new feature we should bring functionality first and  
> > then
> > optimization if you have real performance data to show.
> > 
> The concern is not about "reclaim less than 16".
> I mean this is just refactoring with exactly the same design of ksgxd  
> preserved, 
> 

I literally have no idea what you are talking about here.  ksgxd() just calls
sgx_reclaim_pages(), which tries to reclaim 16 pages at once.

> in that we first isolate as many candidate EPC pages (up  to  
> 16, ignore the unneeded SGX_NR_TO_SCAN_MAX for now), then does the ewb in  
> one shot without anything else done in between. 
> 

Assuming you are referring the implementation of sgx_reclaim_pages(), and
assuming the "isolate" you mean removing EPC pages from the list (which is
exactly what the sgx_isolate_epc_pages() in this patch does), what happens to
the loops of "backing store allocation" and "EBLOCK", before the loop of EWB? 
Eaten by you?


> As described in original  
> comments for the function sgx_reclaim_pages and sgx_encl_ewb, this is to  
> finish all ewb quickly while minimizing impact of IPI.
> 
> The way you proposed will work but alters the current design and behavior  
> if cgroups is enabled and EPCs of an enclave are tracked across multiple  
> LRUs within the descendant cgroups, in that you will have isolation loop,  
> backing store allocation loop, eblock loop interleaved with the ewb loop.
> 

I have no idea what you are talking about.

The point is, with or w/o this patch, you can only reclaim 16 EPC pages in one
function call (as you have said you are going to remove SGX_NR_TO_SCAN_MAX,
which is a cipher to both of us).  The only difference I can see is, with this
patch, you can have multiple calls of "isolate" and then call the "do_reclaim"
once.

But what's the good of having the "isolate" if the "do_reclaim" can only reclaim
16 pages anyway?

Back to my last reply, are you afraid of any LRU has less than 16 pages to
"isolate", therefore you need to loop LRUs of descendants to get 16?  Cause I
really cannot think of any other reason why you are doing this.


> >