Re: [RFC PATCH 0/4] SGX shmem backing store issue

Jarkko Sakkinen
2022-05-07T17:46:38Z

On Sat, May 07, 2022 at 08:46:50PM +0300, Jarkko Sakkinen wrote:
> On Mon, May 02, 2022 at 02:33:06PM -0700, Dave Hansen wrote:
> > On 5/2/22 10:11, Reinette Chatre wrote:
> > > My goal was to prevent the page fault handler from doing a "is the PCMD
> > > page empty" if the reclaimer has a reference to it. Even if the PCMD page
> > > is empty when the page fault handler checks it the page is expected to
> > > get data right when reclaimer can get the mutex back since the reclaimer
> > > already has a reference to the page.
> >
> > I think shmem_truncate_range() might be the wrong operation. It
> > destroys data and, in the end, we don't want to destroy data.
> > Filesystems and the page cache already have nice ways to keep from
> > destroying data, we just need to use them.
> >
> > First, I think set_page_dirty(pcmd_page) before the EWB is a good start.
> > That's what filesystems do before important data that needs to be saved
> > goes into pages.
> >
> > Second, I think we need behavior like POSIX_FADV_DONTNEED, not
> > FALLOC_FL_PUNCH_HOLE. The DONTNEED operations end up in
> > mapping_evict_folio(), which has both page refcount *and* dirty page
> > checks. That means that either elevating a refcount or set_page_dirty()
> > will thwart DONTNEED-like behavior.
> >
> > There are two basic things we need to do:
> >
> > 1. Prevent page from being truncated around EWB time
> > 2. Prevent unreferenced page with all zeros from staying in shmem
> > forever or taking up swap space
> >
> > On the EWB (write to PCMD side) I think something like this works:
> >
> > sgx_encl_get_backing()
> > get_page(pcmd_page)
> >
> > ...
> > lock_page(pcmd_page);
> > // check for truncation since sgx_encl_get_backing()
> > if (pcmd_page->mapping != shmem)
> > goto retry;
> > // double check this is OK under lock_page():
> > set_page_dirty(pcmd_page);
> > __sgx_encl_ewb();
> > unlock_page(pcmd_page);
> >
> > That's basically what filesystems do. Get the page from the page cache,
> > lock it, then make sure it is consistent. If not, retry.
> >
> > On the "free" / read in (ELDU) side:
> >
> > // get pcmd_page ref
> > lock_page(pcmd_page);
> > // truncation is not a concern because that's only done
> > // on the read-in side, here, where we hold encl->lock
> >
> > memset();
> > if (!memchr_inv())
> > // clear the way for DONTNEED:
> > ClearPageDirty(pcmd_page);
> > unlock_page(pcmd_page);
> > // drop pcmd_page ref
> > ...
> > POSIX_FADV_DONTNEED
> >
> > There's one downside to this: it's _possible_ that an transient
> > get_page() would block POSIX_FADV_DONTNEED. Then the zeroed page would
> > stick around forever, or at least until the next ELDU operation did
> > another memchr_inv().
> >
> > I went looking around for some of those and could not find any that I
> > *know* apply to shmem.
> >
> > This doesn't feel like a great solution; it's more complicated than I
> > would like. Any other ideas?
>
> If we could do both truncation and swapping in one side, i.e. in ksgxd,
> that would simplify this process a lot. Then the whole synchronization
> problem would not exist.
>
> E.g. perhaps #PF handler could just zero PCMD and collect zeroed pages
> indices to a list and ksgxd would truncate them.

I.e. instead of immediate response, go for lazy response that is taken
care by ksgxd.

BR, Jarkko