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>
In-Reply-To: <87sfczuxkc.fsf@nvidia.com>
From:   Suren Baghdasaryan <surenb@google.com>
Date:   Mon, 17 Apr 2023 11:13:29 -0700
Message-ID: <CAJuCfpEV1OiM423bykYQTxDC1=bQAqhAwd5fiKYifsk=seP6yw@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 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 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.

>
> >  - 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?

>
> >  - 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.
>