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. 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@xxxxxxxxxx> > >> --- > >> mm/gup.c | 9 ++++++--- > >> mm/ksm.c | 7 ++++--- > >> mm/memcontrol.c | 8 ++++---- > >> mm/memory-failure.c | 8 +++++--- > >> mm/migrate.c | 3 +++ > >> mm/swap_state.c | 3 +++ > >> 6 files changed, 25 insertions(+), 13 deletions(-) > >> > > > > [...] > > > >> 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, pmd_t > >> *pmd, > >> swp_entry_t entry; > >> ptep = pte_offset_map_lock(mm, pmd, address, &ptl); > >> + if (!ptep) > >> + return; > > > > 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. > > > >> + > >> pte = *ptep; > >> 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, > >> /* Copy the PTEs because the page table may be unmapped */ > >> orig_pte = pte = pte_offset_map(vmf->pmd, faddr); > >> + if (!pte) > >> + return; > > > > Ditto? > > 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
