On Thu, Dec 14, 2017 at 09:36:05PM +0000, Christopherson, Sean J wrote:
> On Thu, Dec 14, 2017 at 03:10:06PM +0200, Jarkko Sakkinen wrote:
> > On Tue, Dec 12, 2017 at 01:46:48PM -0800, Sean Christopherson wrote:
> > > So it looks like you avoid the described case by moving B to the head of
> > > the list in sgx_eldu. The bug I am seeing is still straightforward to
> > > theorize:
> > >
> > > 1. Three VA pages. List = A->B->C
> > > 2. Fill A and B, use one entry in C. List = C->B->A
> > > 3. ELDU, freeing a slot in B. List = B->C->A
> > > 4. EWB, consuming the last slot in B. List = B->C->A
> > > 5. ELDU, freeing a slot in A. List = A->B->C
> > > 6. EWB, consuming the last slot in A. List = A->B->C
> > > 7. ELDU, but both A and B are full
> > > 8. Explode
> >
> > I see. It is easy to fix by moving back to of the list immediately after
> > last allocation. Thanks for pointing this out.
>
> Why not keep it simple and iterate over all VA pages? You can still
> move full pages to the back of the list to reduce the number of times
> full pages are queried. IMO, juggling the pages on every EWB/ELDU
> adds complexity for little to no gain; there's no guarantee that the
> cache/TLB benefits of reusing a VA slot justifies the potential for
> thrashing the list, e.g. moving a previously-full VA page to the head
> of the list on ELDU will cause that page to get bounced back to the
> end of the list on the next EWB. Besides, whatever performance might
> be gained is a drop in the bucket compared to the performance hit of
> evicting enough EPC pages to fill multiple VA pages.
>
> e.g.
>
> list_for_each_entry_safe(va_page, tmp, &encl->va_pages, list) {
> va_offset = sgx_alloc_va_slot(va_page);
> if (va_offset < PAGE_SIZE)
> break;
>
> list_move_tail(&va_page->list, &full_pages);
> }
> list_splice_tail(&full_pages, &va_page->list);
It is easy to just to check whether VA page is full and move it back
of the list if it is.
/Jarkko
