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

Haitao Huang
2023-12-18T21:24:48Z

On Sun, 17 Dec 2023 19:44:56 -0600, Huang, Kai <kai.huang@intel.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