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
