On Wed, Jun 28, 2023 at 6:43 AM Peter Xu <peterx@xxxxxxxxxx> wrote: > > On Wed, Jun 28, 2023 at 12:17:59AM -0700, Suren Baghdasaryan wrote: > > When page fault is handled under per-VMA lock protection, all swap page > > faults are retried with mmap_lock because folio_lock_or_retry has to drop > > and reacquire mmap_lock if folio could not be immediately locked. > > Follow the same pattern as mmap_lock to drop per-VMA lock when waiting > > for folio and retrying once folio is available. > > With this obstacle removed, enable do_swap_page to operate under > > per-VMA lock protection. Drivers implementing ops->migrate_to_ram might > > still rely on mmap_lock, therefore we have to fall back to mmap_lock in > > that particular case. > > Note that the only time do_swap_page calls synchronous swap_readpage > > is when SWP_SYNCHRONOUS_IO is set, which is only set for > > QUEUE_FLAG_SYNCHRONOUS devices: brd, zram and nvdimms (both btt and > > pmem). Therefore we don't sleep in this path, and there's no need to > > drop the mmap or per-VMA lock. > > > > Signed-off-by: Suren Baghdasaryan <surenb@xxxxxxxxxx> > > Acked-by: Peter Xu <peterx@xxxxxxxxxx> > > One nit below: > > > --- > > mm/filemap.c | 25 ++++++++++++++++--------- > > mm/memory.c | 16 ++++++++++------ > > 2 files changed, 26 insertions(+), 15 deletions(-) > > > > diff --git a/mm/filemap.c b/mm/filemap.c > > index 52bcf12dcdbf..7ee078e1a0d2 100644 > > --- a/mm/filemap.c > > +++ b/mm/filemap.c > > @@ -1699,31 +1699,38 @@ static int __folio_lock_async(struct folio *folio, struct wait_page_queue *wait) > > return ret; > > } > > > > +static void release_fault_lock(struct vm_fault *vmf) > > +{ > > + if (vmf->flags & FAULT_FLAG_VMA_LOCK) > > + vma_end_read(vmf->vma); > > + else > > + mmap_read_unlock(vmf->vma->vm_mm); > > +} > > + > > /* > > * Return values: > > * 0 - folio is locked. > > * VM_FAULT_RETRY - folio is not locked. > > - * mmap_lock has been released (mmap_read_unlock(), unless flags had both > > - * FAULT_FLAG_ALLOW_RETRY and FAULT_FLAG_RETRY_NOWAIT set, in > > - * which case mmap_lock is still held. > > + * mmap_lock or per-VMA lock has been released (mmap_read_unlock() or > > + * vma_end_read()), unless flags had both FAULT_FLAG_ALLOW_RETRY and > > + * FAULT_FLAG_RETRY_NOWAIT set, in which case the lock is still held. > > * > > * If neither ALLOW_RETRY nor KILLABLE are set, will always return 0 > > - * with the folio locked and the mmap_lock unperturbed. > > + * with the folio locked and the mmap_lock/per-VMA lock is left unperturbed. > > */ > > vm_fault_t __folio_lock_or_retry(struct folio *folio, struct vm_fault *vmf) > > { > > - struct mm_struct *mm = vmf->vma->vm_mm; > > unsigned int flags = vmf->flags; > > > > if (fault_flag_allow_retry_first(flags)) { > > /* > > - * CAUTION! In this case, mmap_lock is not released > > - * even though return VM_FAULT_RETRY. > > + * CAUTION! In this case, mmap_lock/per-VMA lock is not > > + * released even though returning VM_FAULT_RETRY. > > */ > > if (flags & FAULT_FLAG_RETRY_NOWAIT) > > return VM_FAULT_RETRY; > > > > - mmap_read_unlock(mm); > > + release_fault_lock(vmf); > > if (flags & FAULT_FLAG_KILLABLE) > > folio_wait_locked_killable(folio); > > else > > @@ -1735,7 +1742,7 @@ vm_fault_t __folio_lock_or_retry(struct folio *folio, struct vm_fault *vmf) > > > > ret = __folio_lock_killable(folio); > > if (ret) { > > - mmap_read_unlock(mm); > > + release_fault_lock(vmf); > > return VM_FAULT_RETRY; > > } > > } else { > > diff --git a/mm/memory.c b/mm/memory.c > > index 345080052003..76c7907e7286 100644 > > --- a/mm/memory.c > > +++ b/mm/memory.c > > @@ -3712,12 +3712,6 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) > > if (!pte_unmap_same(vmf)) > > goto out; > > > > - if (vmf->flags & FAULT_FLAG_VMA_LOCK) { > > - ret = VM_FAULT_RETRY; > > - vma_end_read(vma); > > - goto out; > > - } > > - > > entry = pte_to_swp_entry(vmf->orig_pte); > > if (unlikely(non_swap_entry(entry))) { > > if (is_migration_entry(entry)) { > > @@ -3727,6 +3721,16 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) > > vmf->page = pfn_swap_entry_to_page(entry); > > ret = remove_device_exclusive_entry(vmf); > > } else if (is_device_private_entry(entry)) { > > + if (vmf->flags & FAULT_FLAG_VMA_LOCK) { > > + /* > > + * migrate_to_ram is not yet ready to operate > > + * under VMA lock. > > + */ > > + vma_end_read(vma); > > + ret |= VM_FAULT_RETRY; > > Here IIUC ret==0 is guaranteed, so maybe "ret = VM_FAULT_RETRY" is slightly > clearer. Ack. > > > + goto out; > > + } > > + > > vmf->page = pfn_swap_entry_to_page(entry); > > vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd, > > vmf->address, &vmf->ptl); > > -- > > 2.41.0.162.gfafddb0af9-goog > > > > -- > Peter Xu > > -- > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@xxxxxxxxxxx. >