On Mon, Jun 10, 2019 at 11:09:50AM -0700, Xing, Cedric wrote: > > From: Andy Lutomirski [mailto:luto@xxxxxxxxxxxxxx] > > Sent: Friday, June 07, 2019 12:32 PM > > > > > On Jun 6, 2019, at 10:32 AM, Sean Christopherson > > <sean.j.christopherson@xxxxxxxxx> wrote: > > > > > >> On Thu, Jun 06, 2019 at 10:20:38AM -0700, Andy Lutomirski wrote: > > >> On Wed, Jun 5, 2019 at 12:49 PM Sean Christopherson > > >> <sean.j.christopherson@xxxxxxxxx> wrote: > > >>> > > >>> For some enclaves, e.g. an enclave with a small code footprint and a > > >>> large working set, the vast majority of pages added to the enclave > > >>> are zero pages. Introduce a flag to denote such zero pages. The > > >>> major benefit of the flag will be realized in a future patch to use > > >>> Linux's actual zero page as the source, as opposed to explicitly > > >>> zeroing the enclave's backing memory. > > >>> > > >> > > >> I feel like I probably asked this at some point, but why is there a > > >> workqueue here at all? > > > > > > Performance. A while back I wrote a patch set to remove the worker > > > queue and discovered that it tanks enclave build time when the enclave > > > is being hosted by a Golang application. Here's a snippet from a mail > > > discussing the code. > > > > > > The bad news is that I don't think we can remove the add page > > worker > > > as applications with userspace schedulers, e.g. Go's M:N scheduler, > > > can see a 10x or more throughput improvement when using the worker > > > queue. I did a bit of digging for the Golang case to make sure I > > > wasn't doing something horribly stupid/naive and found that it's a > > > generic issue in Golang with blocking (or just long-running) system > > > calls. Because Golang multiplexes Goroutines on top of OS threads, > > > blocking syscalls introduce latency and context switching overhead, > > > e.g. Go's scheduler will spin up a new OS thread to service other > > > Goroutines after it realizes the syscall has blocked, and will > > later > > > destroy one of the OS threads so that it doesn't build up too many > > > unused. > > > > > > IIRC, the scenario is spinning up several goroutines, each building an > > > enclave. I played around with adding a flag to do a synchronous EADD > > > but didn't see a meaningful change in performance for the simple case. > > > Supporting both the worker queue and direct paths was complex enough > > > that I decided it wasn't worth the trouble for initial upstreaming. > > > > Sigh. > > > > It seems silly to add a workaround for a language that has trouble > > calling somewhat-but-not-too-slow syscalls or ioctls. > > > > How about fixing this in Go directly? Either convince the golang people > > to add a way to allocate a real thread for a particular region of code > > or have the Go SGX folks write a bit of C code to do a whole bunch of > > ioctls and have Go call *that*. Then the mess stays in Go where it > > belongs. > > The add page worker is less about performance but more about concurrency > restrictions in SGX ISA. EADD/EEXTEND are slow instructions and both take > lock on the SECS. So if there's another EADD/EEXTEND in progress then it will > #GP. > > In practice, no sane applications will EADD in multiple threads because that > would make MRENCLAVE unpredictable. But adversary may use that just to > trigger #GP in kernel. Therefore, the SGX module would have to lock around > EADD/EEXTEND anyway. And then we figured it would be better to have the add > page worker so that no lock would be necessary, plus (small) improvement in > performance. That may have been true years ago, but it doesn't reflect the current reality. The ADD_PAGE ioctl *and* the worker function both take the enclave's lock. The worker function actually takes it twice, once to pull the request off the queue, and once to do EADD/EEXTEND. The lock is temporarily released by the worker function to avoid deadlock in case EPC page allocation triggers reclaim.
