Re: [PATCH 0/15] mm: introduce ANON_VMA_LAZY for deferred anon_vma creation

[xu.xin16]

> > 
> >  arch/arm64/Kconfig         |   1 +
> >  arch/x86/Kconfig           |   1 +
> >  fs/proc/page.c             |   6 +-
> >  include/linux/mm.h         |  38 ++
> >  include/linux/mm_types.h   |   9 +-
> >  include/linux/page-flags.h |  34 +-
> >  include/linux/pagemap.h    |   2 +-
> >  include/linux/rmap.h       | 165 ++++++++-
> >  mm/Kconfig                 |  22 ++
> >  mm/damon/ops-common.c      |   4 +-
> >  mm/debug.c                 |   2 +-
> >  mm/debug_vm_pgtable.c      |   2 +-
> >  mm/gup.c                   |   6 +-
> >  mm/huge_memory.c           |  16 +-
> >  mm/internal.h              | 171 +++++++++
> >  mm/khugepaged.c            |  13 +-
> >  mm/ksm.c                   |  43 ++-
> >  mm/memory-failure.c        |  11 +-
> >  mm/memory.c                |  19 +-
> >  mm/migrate.c               | 126 ++++---
> >  mm/mmap.c                  |  15 +-
> >  mm/mremap.c                |   4 +-
> >  mm/page_idle.c             |   2 +-
> >  mm/rmap.c                  | 690 ++++++++++++++++++++++++++++++++++---
> >  mm/vma.c                   |  76 ++--
> >  mm/vma.h                   |   4 +-
> >  mm/vma_exec.c              |   2 +-
> >  mm/vma_init.c              |   1 +
> >  28 files changed, 1279 insertions(+), 206 deletions(-)
> 
> Hi!
> 
> When I saw the diffsat I was concerned. Going through the patches made me ...
> more concerned :)
> 
> This is a lot of complexity. On top of something that is already so complicated
> that I fail to grasp most details without regularly taking a look at the nice
> figures Lorenzo created recently.
> 
> For example, I read above "since child VMAs do not need to allocate anon_vma"
> and wondered how that could be part of something that is just done lazily. Then
> I had to learn in the patches that there is some additional "Child VMAs
> are created as ANON_VMA_TREE_PARENT and do not allocate anon_vma" -- excuse me,
> what? :)
> 
> Reading about VMA refcounts made me shiver. Reading "Holding only
> folio_lock(folio) cannot guarantee that the split
> operation completes atomically." confused me. Learning that we have to invent
> interesting ways to make page migration mutually exclusive to free_pgtables()
> concerned me. Figuring out that there are arch-specific config options and
> runtime toggles is a clear warning sign.
> 
> Seeing test_folio_unmapped() was funny, though (why?! :)).
> 
> I think this patch set has a noble goal of reducing anon_vma overhead when anon
> pages are not shared during fork. However, using anon_vma for them actually
> makes the overall implementation (e.g., rmap walks, locking) more consistent and
> simpler.
> 
> Even if we could be convinced that most of this here is correct, how should we
> reasonably maintain this increasing level of complexity here?

Indeed, it's very complex, but having the changes of 15 patches scattered across
various subsystems is really frustrating for reviewers. It took me a whole day to
read through the entire patch set, which made an already complicated matter even
more complex (maintaining such complex code in the future will be a pain).

However, overall, I think the original intention behind Tao's patch is innovative
and valuable, and Tao could definitely make this patch set simpler and more
readable, because the core changes actually start from PATCH 10.

I believe that if Tao had done the following, things might have gone better and easier
for reviewing. In fact, I understand the motivation behind the patch is quite simple
at its core (just wanting to avoid allocating the anon_vma structure when a VMA hasn't
been truly forked, and instead put the VMA information directly into folio->mapping):

1) You could actually simplify your patch significantly — without adding a lot of wrappers
and helper functions that introduce extra review overhead — and keep only the most essential elements.

2) Provide complete test code (in tools/testing/selftest) that covers the affected functionality,
such as VMA, huge pages, KSM, etc.

3) Use the RFC tag to start a discussion.


I would be very glad to see if Tao could post a simpler v2 version that does not alter the rmap
core data structures too much and does not introduce excessive complexity, no matter whether
it can be merged finaly.