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;
Date:   Tue, 23 May 2023 20:20:41 -0700 (PDT)
From:   Hugh Dickins <hughd@google.com>
To:     Qi Zheng <qi.zheng@linux.dev>
cc:     Hugh Dickins <hughd@google.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Mike Kravetz <mike.kravetz@oracle.com>,
        Mike Rapoport <rppt@kernel.org>,
        "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
        Matthew Wilcox <willy@infradead.org>,
        David Hildenbrand <david@redhat.com>,
        Suren Baghdasaryan <surenb@google.com>,
        Qi Zheng <zhengqi.arch@bytedance.com>,
        Yang Shi <shy828301@gmail.com>,
        Mel Gorman <mgorman@techsingularity.net>,
        Peter Xu <peterx@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Will Deacon <will@kernel.org>, Yu Zhao <yuzhao@google.com>,
        Alistair Popple <apopple@nvidia.com>,
        Ralph Campbell <rcampbell@nvidia.com>,
        Ira Weiny <ira.weiny@intel.com>,
        Steven Price <steven.price@arm.com>,
        SeongJae Park <sj@kernel.org>,
        Naoya Horiguchi <naoya.horiguchi@nec.com>,
        Christophe Leroy <christophe.leroy@csgroup.eu>,
        Zack Rusin <zackr@vmware.com>, Jason Gunthorpe <jgg@ziepe.ca>,
        Axel Rasmussen <axelrasmussen@google.com>,
        Anshuman Khandual <anshuman.khandual@arm.com>,
        Pasha Tatashin <pasha.tatashin@soleen.com>,
        Miaohe Lin <linmiaohe@huawei.com>,
        Minchan Kim <minchan@kernel.org>,
        Christoph Hellwig <hch@infradead.org>,
        Song Liu <song@kernel.org>,
        Thomas Hellstrom <thomas.hellstrom@linux.intel.com>,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [PATCH 17/31] mm/various: give up if pte_offset_map[_lock]()
 fails
In-Reply-To: <07317766-c901-34a9-360a-e916db4b9045@linux.dev>
Message-ID: <4df7a2a5-e2b9-6c9e-9bbf-27e1dbdace8@google.com>
References: <68a97fbe-5c1e-7ac6-72c-7b9c6290b370@google.com> <c299eba-4e17-c645-1115-ccd1fd9956bd@google.com> <f8468e50-9f54-f22d-59a7-3a48369d2f05@linux.dev> <07317766-c901-34a9-360a-e916db4b9045@linux.dev>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="-1463760895-1197784731-1684898444=:7491"
Precedence: bulk

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

---1463760895-1197784731-1684898444=:7491
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: QUOTED-PRINTABLE

On Mon, 22 May 2023, Qi Zheng wrote:
> On 2023/5/22 20:24, Qi Zheng wrote:
> > On 2023/5/22 13:10, Hugh Dickins wrote:
> >> Following the examples of nearby code, various functions can just give
> >> up if pte_offset_map() or pte_offset_map_lock() fails.=C2=A0 And there=
's no
> >> need for a preliminary pmd_trans_unstable() or other such check, since
> >> such cases are now safely handled inside.
> >>
> >> Signed-off-by: Hugh Dickins <hughd@google.com>
> >> ---
> >> =C2=A0 mm/gup.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0 | 9 ++++++---
> >> =C2=A0 mm/ksm.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0 | 7 ++++---
> >> =C2=A0 mm/memcontrol.c=C2=A0=C2=A0=C2=A0=C2=A0 | 8 ++++----
> >> =C2=A0 mm/memory-failure.c | 8 +++++---
> >> =C2=A0 mm/migrate.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 | 3 +++
> >> =C2=A0 mm/swap_state.c=C2=A0=C2=A0=C2=A0=C2=A0 | 3 +++
> >> =C2=A0 6 files changed, 25 insertions(+), 13 deletions(-)
> >>
> >=20
> > [...]
> >=20
> >> diff --git a/mm/migrate.c b/mm/migrate.c
> >> index 3ecb7a40075f..308a56f0b156 100644
> >> --- a/mm/migrate.c
> >> +++ b/mm/migrate.c
> >> @@ -305,6 +305,9 @@ void migration_entry_wait(struct mm_struct *mm, pm=
d_t
> >> *pmd,
> >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 swp_entry_t entry;
> >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ptep =3D pte_offset_map_lock(mm, pmd, a=
ddress, &ptl);
> >> +=C2=A0=C2=A0=C2=A0 if (!ptep)
> >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return;
> >=20
> > Maybe we should return false and let the caller handle the failure.

We have not needed to do that before, it's normal for migration_entry_wait(=
)
not to wait sometimes: it just goes back out to userspace to try again (by
which time the situation is usually resolved).  I don't think we want to
trouble the callers with a new case to handle in some other way.

> >=20
> >> +
> >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pte =3D *ptep;
> >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pte_unmap(ptep);
> >> diff --git a/mm/swap_state.c b/mm/swap_state.c
> >> index b76a65ac28b3..db2ec85ef332 100644
> >> --- a/mm/swap_state.c
> >> +++ b/mm/swap_state.c
> >> @@ -734,6 +734,9 @@ static void swap_ra_info(struct vm_fault *vmf,
> >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* Copy the PTEs because the page table=
 may be unmapped */
> >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 orig_pte =3D pte =3D pte_offset_map(vmf=
->pmd, faddr);
> >> +=C2=A0=C2=A0=C2=A0 if (!pte)
> >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return;
> >=20
> > Ditto?
>=20
> Oh, I see that you handle it in the PATCH[22/31].

I don't think 22/31 (about swapoff "unuse") relates to this one.
Here swap_vma_readahead() is doing an interesting calculation for
how big the readaround window should be, and my thinking was, who cares?
just read 1, in the rare case that the page table vanishes underneath us.

But thank you for making me look again: it looks like I was not careful
enough before, ra_info->win is definitely *not* 1 on this line, and I
wonder if something bad might result from not following through on the
ensuing calculations - see how !CONFIG_64BIT is copying ptes (and that
implies CONFIG_64BIT is accessing the page table after pte_unmap()).

This swap_ra_info() code looks like it will need a patch all it own:
I must come back to it.

Thanks!
Hugh
---1463760895-1197784731-1684898444=:7491--