Re: [PATCH v38 16/24] x86/sgx: Add a page reclaimer

[Sean Christopherson <sean.j.christopherson@intel.com>]

On Tue, Sep 22, 2020 at 05:03:23PM +0300, Jarkko Sakkinen wrote:
> On Tue, Sep 22, 2020 at 12:45:38PM +0200, Borislav Petkov wrote:
> > > +	spin_lock(&sgx_active_page_list_lock);
> > > +	for (i = 0; i < SGX_NR_TO_SCAN; i++) {
> > > +		if (list_empty(&sgx_active_page_list))
> > 
> > Isn't it enough to do this once, i.e., not in the loop? You're holding
> > sgx_active_page_list_lock...

Argh, I missed this until I looked at Jarkko's updated tree.

The reason for checking list_empty() on every iteration is that the loop is
greedy, i.e. it tries to grab and reclaim up to 16 (SGX_NR_TO_SCAN) EPC pages
at a time.

> I think that would make sense. Distantly analogous to the EINIT
> discussion. Too complex code for yet to be known problem workloads I'd
> say.

Nooooo.  Please no.

This will most definitely impact workloads running a single large enclave, or a
process running multiple enclaves, as we'll lose the batching of ETRACK and IPIs.
ETRACK isn't a big deal, but the IPIs are painful and could thrash the system.
E.g. in the current code, reclaiming 10 pages from an enclave with actively
running threads will generate one round of IPIs to CPUs associated with the
enclave (based on the mm struct).  Going to per-page reclaim will likely result
in 10 rounds of IPIs as threads will reenter the enclave immediately after each
IPI wave.

Having to grab the section spinlock for every page is also (very) problematic.
Reclaiming one page at a time makes it nigh impossible for the reclaimer to
keep up as every allocation attempt acquires the spinlock.  I.e. the reclaimer
has to contend with every CPU that is faulting in an EPC page or is adding a
page to the enclave.  In my experiments with the EPC cgroup, even batching 16
pages may be insufficient to make "forward progress".  That's not an apples to
apples comparison, but it's a rough equivalent of what will happen with the
reclaim thread versus EPC page faults.  We can (and should) play with scaling
the number of reclaim threads, but at this stage, that's an exercise best left
to post-upstreaming.

I can't do A/B testing to provide numbers, because the changes in Jarkko's tree
break basic reclaim.  Which is a nice segue into my last point: this is by far
the most subtle code in the SGX code base; I really, really don't want to be
making last minute changes in this area unless they are absolutely necessary or
obviously benign.  Even if/when we get the basic reclaim functional again, and
assuming it doesn't have performance issues, I wouldn't be comfortable merging
the code without hammering it with the EPC cgroup tests for multiple days (on
top of the 1+ days to rebased the EPC cgroup).  Past testing with the cgroup
found multiple bugs with races between CPUs that required hitting a window that
was open for less than 10 instructions.