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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <op.2f523elowjvjmi@hhuan26-mobl.amr.corp.intel.com>
Date: Mon, 18 Dec 2023 15:24:40 -0600
From: "Haitao Huang" <haitao.huang@...ux.intel.com>
To: "Mehta, Sohil" <sohil.mehta@...el.com>, "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>, "tj@...nel.org"
 <tj@...nel.org>, "mkoutny@...e.com" <mkoutny@...e.com>, "tglx@...utronix.de"
 <tglx@...utronix.de>, "linux-sgx@...r.kernel.org" <linux-sgx@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "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>, "anakrish@...rosoft.com"
 <anakrish@...rosoft.com>, "sean.j.christopherson@...el.com"
 <sean.j.christopherson@...el.com>, "Li, Zhiquan1" <zhiquan1.li@...el.com>,
 "yangjie@...rosoft.com" <yangjie@...rosoft.com>
Subject: Re: [PATCH v6 09/12] x86/sgx: Restructure top-level EPC reclaim
 function

On Sun, 17 Dec 2023 19:44:56 -0600, Huang, Kai <kai.huang@...el.com> wrote:

>
>> >
>> > 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?
>
> To me I won't bother to do that.  Having less than 16 pages in one LRU is
> *extremely rare* that should never happen in practice.  It's pointless  
> to make
> such code adjustment at this stage.
>
> Let's focus on enabling functionality first.  When you have some real
> performance issue that is related to this, we can come back then.
>
> Btw, I think you need to step back even further.  IIUC the whole  
> multiple LRU
> thing isn't mandatory in this initial support.
>
> Please (again) take a look at the comments from Dave and Michal:
>
> https://lore.kernel.org/lkml/7a1a5125-9da2-47b6-ba0f-cf24d84df16b@intel.com/#t
> https://lore.kernel.org/lkml/yz44wukoic3syy6s4fcrngagurkjhe2hzka6kvxbajdtro3fwu@zd2ilht7wcw3/

Thanks for raising this. Actually my understanding the above discussion  
was mainly about not doing reclaiming by killing enclaves, i.e., I assumed  
"reclaiming" within that context only meant for that particular kind.

As Mikko pointed out, without reclaiming per-cgroup, the max limit of each  
cgroup needs to accommodate the peak usage of enclaves within that cgroup.  
That may be inconvenient for practical usage and limits could be forced to  
be set larger than necessary to run enclaves performantly. For example, we  
can observe following undesired consequences comparing a system with  
current kernel loaded with enclaves whose total peak usage is greater than  
the EPC capacity.

1) If a user wants to load the same exact enclaves but in separate  
cgroups, then the sum of cgroup limits must be higher than the capacity  
and the system will end up doing the same old global reclaiming as it is  
currently doing. Cgroup is not useful at all for isolating EPC  
consumptions.

2) To isolate impact of usage of each cgroup on other cgroups and yet  
still being able to load each enclave, the user basically has to carefully  
plan to ensure the sum of cgroup max limits, i.e., the sum of peak usage  
of enclaves, is not reaching over the capacity. That means no  
over-commiting allowed and the same system may not be able to load as many  
enclaves as with current kernel.

@Dave and @Michal, Your thoughts? Or could you confirm we should not do  
reclaim per cgroup at
all?

If confirmed as desired, then this series can stop at patch 4.

Thanks
Haitao