On Mon, Apr 17, 2023 at 4:50 PM Suren Baghdasaryan <surenb@xxxxxxxxxx> wrote: > > On Mon, Apr 17, 2023 at 4:33 PM Alistair Popple <apopple@xxxxxxxxxx> wrote: > > > > > > Suren Baghdasaryan <surenb@xxxxxxxxxx> writes: > > > > > On Sun, Apr 16, 2023 at 6:06 PM Alistair Popple <apopple@xxxxxxxxxx> wrote: > > >> > > >> > > >> Matthew Wilcox <willy@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> > > >> > > > >> > 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. > > >> > >
