[v2,0/5] mm: A few cleanup patches around zap, shmem and uffd

Message

Peter Xu Sept. 2, 2021, 8:17 p.m. UTC

[Based on tag v5.14, but it should still apply to -mm too.  If not, I can
 repost anytime]

Hugh,

So I found one thing that I feel like a bug of commit 22061a1ffabdb9c3, but I'm
not sure.  If that's the case, patch 5 of this series may be the fix for it.

The problem is unmap_mapping_page() in current tree is calling
unmap_mapping_range_tree() with a details pointer, while by default when detail
pointer is specified, it means "we want to skip zapping swap entries".

I didn't mention this in v1 simply because I thought it was fine, e.g., swap
entry won't be kept in shmem ptes so skipped is okay (it is never okay with
shmem uffd-wp but uffd-wp code is not landed yet).  However I just remembered
there could also be e.g. shmem migration entries if I'm not wrong.  From that
pov, skipping swap entries for unmap_mapping_page() seems wrong.  Would you
please help check?

It'll be great if you can comment on patch 1 too, to see whether that's okay
and what I could have been missing there (e.g., if you have other concern on
breaking stuff, I'll be happy to test).

v2:
- Patch "mm: Clear vmf->pte after pte_unmap_same() returns"
  - Remove one comment [David]
- Collect r-b for patch 2/3
- Rewrite the last two patches to drop ZAP_FLAG_CHECK_MAPPING, dropping
  Alistair's r-b on patch 5 because it changed [David, Matthew]

===== v1 cover letter =====

I picked up these patches from uffd-wp v5 series here:

https://lore.kernel.org/lkml/[e-mail]/

IMHO all of them are very nice cleanups to existing code already, they're all
small and self-contained.  They'll be needed by uffd-wp coming series.  I would
appreciate if they can be accepted earlier, so as to not carry them over always
with the uffd-wp series.

I removed some CC from the uffd-wp v5 series to reduce the noise, and added a
few more into it.

Reviews are greatly welcomed, thanks.

Peter Xu (5):
  mm/shmem: Unconditionally set pte dirty in mfill_atomic_install_pte
  mm: Clear vmf->pte after pte_unmap_same() returns
  mm: Drop first_index/last_index in zap_details
  mm: Add zap_skip_check_mapping() helper
  mm: Add ZAP_FLAG_SKIP_SWAP and zap_flags

 include/linux/mm.h | 33 ++++++++++++++++++--
 mm/memory.c        | 76 +++++++++++++++++++---------------------------
 mm/shmem.c         |  1 -
 mm/userfaultfd.c   |  3 +-
 4 files changed, 62 insertions(+), 51 deletions(-)

Comments

Peter Xu Sept. 8, 2021, 12:35 a.m. UTC | #1

On Thu, Sep 02, 2021 at 04:17:16PM -0400, Peter Xu wrote:
> [Based on tag v5.14, but it should still apply to -mm too.  If not, I can
>  repost anytime]
> 
> Hugh,
> 
> So I found one thing that I feel like a bug of commit 22061a1ffabdb9c3, but I'm
> not sure.  If that's the case, patch 5 of this series may be the fix for it.
> 
> The problem is unmap_mapping_page() in current tree is calling
> unmap_mapping_range_tree() with a details pointer, while by default when detail
> pointer is specified, it means "we want to skip zapping swap entries".
> 
> I didn't mention this in v1 simply because I thought it was fine, e.g., swap
> entry won't be kept in shmem ptes so skipped is okay (it is never okay with
> shmem uffd-wp but uffd-wp code is not landed yet).  However I just remembered
> there could also be e.g. shmem migration entries if I'm not wrong.  From that
> pov, skipping swap entries for unmap_mapping_page() seems wrong.  Would you
> please help check?

I figured this seems to have no problem: firstly commit 22061a1ffabdb9c3 didn't
really change the behavior of the code because previously it was using
unmap_mapping_range(), which will always pass in a "details" pointer anyway.

Meanwhile there won't be migration entry for this page because the new helper
unmap_mapping_page() has page lock held, while migration requires that too
during moving the pages.

Anyway, sorry for the noise. I'll respin but drop these paragraphs, also in the
last patch's commit message.