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

Jarkko Sakkinen
2022-05-07T17:45:19Z

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.

BR, Jarkko