Re: [PATCH 1/1] mm: handle swap page faults if the faulting page can be locked

[Suren Baghdasaryan <surenb@google.com>]

On Mon, Apr 17, 2023 at 4:50 PM Suren Baghdasaryan <surenb@google.com> wrote:
>
> On Mon, Apr 17, 2023 at 4:33 PM Alistair Popple <apopple@nvidia.com> wrote:
> >
> >
> > Suren Baghdasaryan <surenb@google.com> writes:
> >
> > > On Sun, Apr 16, 2023 at 6:06 PM Alistair Popple <apopple@nvidia.com> wrote:
> > >>
> > >>
> > >> Matthew Wilcox <willy@infradead.org> writes:
> > >>
> > >> > On Fri, Apr 14, 2023 at 11:00:43AM -0700, Suren Baghdasaryan wrote:
> > >> >> When page fault is handled under VMA lock protection, all swap page
> > >> >> faults are retried with mmap_lock because folio_lock_or_retry
> > >> >> implementation has to drop and reacquire mmap_lock if folio could
> > >> >> not be immediately locked.
> > >> >> Instead of retrying all swapped page faults, retry only when folio
> > >> >> locking fails.
> > >> >
> > >> > Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > >> >
> > >> > Let's just review what can now be handled under the VMA lock instead of
> > >> > the mmap_lock, in case somebody knows better than me that it's not safe.
> > >> >
> > >> >  - We can call migration_entry_wait().  This will wait for PG_locked to
> > >> >    become clear (in migration_entry_wait_on_locked()).  As previously
> > >> >    discussed offline, I think this is safe to do while holding the VMA
> > >> >    locked.
> > >>
> > >> Do we even need to be holding the VMA locked while in
> > >> migration_entry_wait()? My understanding is we're just waiting for
> > >> PG_locked to be cleared so we can return with a reasonable chance the
> > >> migration entry is gone. If for example it has been unmapped or
> > >> protections downgraded we will simply refault.
> > >
> > > If we drop VMA lock before migration_entry_wait() then we would need
> > > to lock_vma_under_rcu again after the wait. In which case it might be
> > > simpler to retry the fault with some special return code to indicate
> > > that VMA lock is not held anymore and we want to retry without taking
> > > mmap_lock. I think it's similar to the last options Matthew suggested
> > > earlier. In which case we can reuse the same retry mechanism for both
> > > cases, here and in __folio_lock_or_retry.
> >
> > Good point. Agree there is no reason to re-take the VMA lock after the
> > wait, although in this case we shouldn't need to retry the fault
> > (ie. return VM_FAULT_RETRY). Just skip calling vma_end_read() on the way
> > out to userspace.
>
> Actually, __collapse_huge_page_swapin() which calls do_swap_page() can
> use VMA reference again inside its loop unless we return
> VM_FAULT_RETRY or VM_FAULT_ERROR. That is not safe since we dropped
> the VMA lock and stability of the VMA is not guaranteed at that point.
> So, we do need to return VM_FAULT_RETRY maybe with another bit
> indicating that retry does not need to fallback to mmap_lock. Smth
> like "return VM_FAULT_RETRY | VM_FAULT_USE_VMA_LOCK".

False alarm. __collapse_huge_page_swapin is always called under
mmap_lock protection. I'll go over the code once more to make sure
nothing else would use VMA after we drop the VMA lock in page fault
path.


>
> >
> > >>
> > >> >  - We can call remove_device_exclusive_entry().  That calls
> > >> >    folio_lock_or_retry(), which will fail if it can't get the VMA lock.
> > >>
> > >> Looks ok to me.
> > >>
> > >> >  - We can call pgmap->ops->migrate_to_ram().  Perhaps somebody familiar
> > >> >    with Nouveau and amdkfd could comment on how safe this is?
> > >>
> > >> Currently this won't work because drives assume mmap_lock is held during
> > >> pgmap->ops->migrate_to_ram(). Primarily this is because
> > >> migrate_vma_setup()/migrate_vma_pages() is used to handle the fault and
> > >> that asserts mmap_lock is taken in walk_page_range() and also
> > >> migrate_vma_insert_page().
> > >>
> > >> So I don't think we can call that case without mmap_lock.
> > >>
> > >> At a glance it seems it should be relatively easy to move to using
> > >> lock_vma_under_rcu(). Drivers will need updating as well though because
> > >> migrate_vma_setup() is called outside of fault handling paths so drivers
> > >> will currently take mmap_lock rather than vma lock when looking up the
> > >> vma. See for example nouveau_svmm_bind().
> > >
> > > Thanks for the pointers, Alistair! It does look like we need to be
> > > more careful with the migrate_to_ram() path. For now I can fallback to
> > > retrying with mmap_lock for this case, like with do with all cases
> > > today. Afterwards this path can be made ready for working under VMA
> > > lock and we can remove that retry. Does that sound good?
> >
> > Sounds good to me. Fixing that shouldn't be too difficult but will need
> > changes to at least Nouveau and amdkfd (and hmm-tests obviously). Happy
> > to look at doing that if/when this change makes it in. Thanks.
> >
> > >>
> > >> >  - I believe we can't call handle_pte_marker() because we exclude UFFD
> > >> >    VMAs earlier.
> > >> >  - We can call swap_readpage() if we allocate a new folio.  I haven't
> > >> >    traced through all this code to tell if it's OK.
> > >> >
> > >> > So ... I believe this is all OK, but we're definitely now willing to
> > >> > wait for I/O from the swap device while holding the VMA lock when we
> > >> > weren't before.  And maybe we should make a bigger deal of it in the
> > >> > changelog.
> > >> >
> > >> > And maybe we shouldn't just be failing the folio_lock_or_retry(),
> > >> > maybe we should be waiting for the folio lock with the VMA locked.
> > >>
> >