Message-ID: <op.2f0eo8r1wjvjmi@hhuan26-mobl.amr.corp.intel.com>
Date: Fri, 15 Dec 2023 13:49:46 -0600
From: "Haitao Huang" <haitao.huang@...ux.intel.com>
To: "tj@...nel.org" <tj@...nel.org>, "jarkko@...nel.org" <jarkko@...nel.org>,
 "x86@...nel.org" <x86@...nel.org>, "dave.hansen@...ux.intel.com"
 <dave.hansen@...ux.intel.com>, "cgroups@...r.kernel.org"
 <cgroups@...r.kernel.org>, "hpa@...or.com" <hpa@...or.com>, "mingo@...hat.com"
 <mingo@...hat.com>, "linux-kernel@...r.kernel.org"
 <linux-kernel@...r.kernel.org>, "mkoutny@...e.com" <mkoutny@...e.com>,
 "tglx@...utronix.de" <tglx@...utronix.de>, "linux-sgx@...r.kernel.org"
 <linux-sgx@...r.kernel.org>, "Mehta, Sohil" <sohil.mehta@...el.com>,
 "bp@...en8.de" <bp@...en8.de>, "Huang, Kai" <kai.huang@...el.com>
Cc: "mikko.ylinen@...ux.intel.com" <mikko.ylinen@...ux.intel.com>,
 "seanjc@...gle.com" <seanjc@...gle.com>, "Zhang, Bo" <zhanb@...rosoft.com>,
 "kristen@...ux.intel.com" <kristen@...ux.intel.com>, "yangjie@...rosoft.com"
 <yangjie@...rosoft.com>, "sean.j.christopherson@...el.com"
 <sean.j.christopherson@...el.com>, "Li, Zhiquan1" <zhiquan1.li@...el.com>,
 "anakrish@...rosoft.com" <anakrish@...rosoft.com>
Subject: Re: [PATCH v6 09/12] x86/sgx: Restructure top-level EPC reclaim
 function

On Wed, 13 Dec 2023 05:17:11 -0600, Huang, Kai <kai.huang@...el.com> wrote:

> 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@...el.com>  
>> 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@...el.com>
>> > > wrote:
>> > >
>> > > > On Mon, 2023-10-30 at 11:20 -0700, Haitao Huang wrote:
>> > > > > From: Sean Christopherson <sean.j.christopherson@...el.com>
>> > > > >
>> > > > > 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?
>

I skipped those as what really matters is to keep ewb loop separate and  
run in one shot for each reclaiming cycle, not dependent on number of  
LRUs.  All those loops in original sgx_reclaim_pages() except the  
"isolate" loop are not dealing with multiple LRUs of cgroups later. That's  
the reason to refactor out only the "isolate" part and loop it through  
cgroup LRUs in later patches.

>
>> 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.
>
>

I think I see your point. By capping pages reclaimed per cycle to 16,  
there is not much difference even if those 16 pages are spread in separate  
LRUs . The difference is only significant when we ever raise that cap. To  
preserve the current behavior of ewb loops independent on number of LRUs  
to loop through for each reclaiming cycle, regardless of the exact value  
of the page cap, I would still think current approach in the patch is  
reasonable choice.  What do you think?

Thanks
Haitao

Powered by blists - more mailing lists