Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230414180043.1839745-1-surenb@google.com> <ZDmetaUdmlEz/W8Q@casper.infradead.org>
 <87sfczuxkc.fsf@nvidia.com> <CAJuCfpEV1OiM423bykYQTxDC1=bQAqhAwd5fiKYifsk=seP6yw@mail.gmail.com>
 <877cuaulrm.fsf@nvidia.com>
In-Reply-To: <877cuaulrm.fsf@nvidia.com>
From:   Suren Baghdasaryan <surenb@google.com>
Date:   Mon, 17 Apr 2023 16:50:04 -0700
Message-ID: <CAJuCfpFEwdnpTXGRpKtXaZ+F4RW4+DkxaRRTiBVy4jE9cG=TEw@mail.gmail.com>
Subject: Re: [PATCH 1/1] mm: handle swap page faults if the faulting page can
 be locked
To:     Alistair Popple <apopple@nvidia.com>
Cc:     Matthew Wilcox <willy@infradead.org>, akpm@linux-foundation.org,
        hannes@cmpxchg.org, mhocko@suse.com, josef@toxicpanda.com,
        jack@suse.cz, ldufour@linux.ibm.com, laurent.dufour@fr.ibm.com,
        michel@lespinasse.org, liam.howlett@oracle.com, jglisse@google.com,
        vbabka@suse.cz, minchan@google.com, dave@stgolabs.net,
        punit.agrawal@bytedance.com, lstoakes@gmail.com,
        linux-mm@kvack.org, linux-fsdevel@vger.kernel.org,
        linux-kernel@vger.kernel.org, kernel-team@android.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Apr 17, 2023 at 4:33=E2=80=AFPM Alistair Popple <apopple@nvidia.com=
> wrote:
>
>
> Suren Baghdasaryan <surenb@google.com> writes:
>
> > On Sun, Apr 16, 2023 at 6:06=E2=80=AFPM 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 s=
afe.
> >> >
> >> >  - We can call migration_entry_wait().  This will wait for PG_locked=
 to
> >> >    become clear (in migration_entry_wait_on_locked()).  As previousl=
y
> >> >    discussed offline, I think this is safe to do while holding the V=
MA
> >> >    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".

>
> >>
> >> >  - We can call remove_device_exclusive_entry().  That calls
> >> >    folio_lock_or_retry(), which will fail if it can't get the VMA lo=
ck.
> >>
> >> Looks ok to me.
> >>
> >> >  - We can call pgmap->ops->migrate_to_ram().  Perhaps somebody famil=
iar
> >> >    with Nouveau and amdkfd could comment on how safe this is?
> >>
> >> Currently this won't work because drives assume mmap_lock is held duri=
ng
> >> pgmap->ops->migrate_to_ram(). Primarily this is because
> >> migrate_vma_setup()/migrate_vma_pages() is used to handle the fault an=
d
> >> 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 becaus=
e
> >> migrate_vma_setup() is called outside of fault handling paths so drive=
rs
> >> 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 UF=
FD
> >> >    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.
> >>
>