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

[David Hildenbrand (Arm)]

On 6/4/26 05:10, xu.xin16@xxxxxxxxxx wrote:
>>>
>>>  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.

I'm afraid, the overall complexity would increase in any case.

So to quote myself "But fundamentally, I think we want to find a new design that
is just naturally simpler."

-- 
Cheers,

David