RE: [PATCH 03/15] mm: introduce anon_vma_tree_t for multiple anon_vma topologies

[wangtao]

> Subject: Re: [PATCH 03/15] mm: introduce anon_vma_tree_t for multiple
> anon_vma topologies
> 
> On Wed, May 27, 2026 at 07:01:35PM +0800, tao wrote:
> > Prepare for upcoming ANON_VMA_LAZY support and RCU-based lockless
> rmap
> > traversal by clearly separating anon_vma topology handling from the
> > anon_rmap semantics.
> 
> RCU is not 'lockless'... and if you truly get RCU semantics you break a bunch
> of stuff as I found out.
> 
RCU is required when acquiring anon_vma or vma. When calling
rmap_one, RCU lock is not required; the lock is obtained with
anon_rmap_lock_read() or folio_lock_anon_rmap_read().

For regular anon_vma, the anon_vma lock is still used. For
ANON_VMA_LAZY, there is only one vma, so the anon_vma lock is not
needed; we only need to ensure the vma is valid.

> >
> > Prepare for supporting multiple anon_vma topologies by introducing
> > lightweight abstractions used by the VMA and rmap code.
> >
> > Introduce anon_vma_tree_t as the type stored in vma->anon_vma:
> >
> >     typedef unsigned long anon_vma_tree_t;
> >
> > It represents a tagged pointer encoding a reference to the anon_vma
> > topology. The low bits are reserved as type tags to distinguish
> > different implementations (e.g. regular anon_vma and lazy anon_vma).
> > This keeps the VMA representation compact while allowing the topology
> > to evolve without changing the VMA layout.
> >
> > Signed-off-by: tao <tao.wangtao@xxxxxxxxx>
> 
> The commit message is at least better on this one, but this approach is again,
> predicated on extending a broken abstraction.
> 
> You could have saved time and effort by coming forward with this earlier to
> the community.
> 
> You're also adding a bunch more messy code on top of anon_vma. It's just
> the wrong direction.
> 
I will update the commit message to add more explanation.

> >
> > +/* anon_vma_tree_t APIs */
> > +
> > +static inline anon_vma_tree_t make_anon_vma_tree(struct anon_vma
> > +*anon_vma) {
> > +	return (anon_vma_tree_t)anon_vma;
> > +}
> 
> You're literally returning an unsigned long of an anon_vma here?
> 
> Why is the anon_rmap_t a wrapped struct and this an unsigned long?
> 
anon_vma_tree_t uses unsigned long because it is used internally
by rmap.c and vma.c. In other places it is mainly used to check
whether a fault has occurred.

> > +
> > +static inline struct anon_vma
> *anon_vma_tree_anon_vma(anon_vma_tree_t
> > +anon_tree) {
> > +	return (struct anon_vma *)anon_tree; }
> 
> The anon_tree is an anon_vma? What?
> 
> And it's a tagged pointer but we don't bother clearing any bits right?...!
> 
When supporting ANON_VMA_LAZY, the lower bits definitions are added.
I will add comments to clarify this.

> > +static inline void anon_vma_tree_unlock_read(anon_vma_tree_t
> > +anon_tree) {
> > +	struct anon_vma *anon_vma =
> anon_vma_tree_anon_vma(anon_tree);
> > +
> > +	anon_vma_unlock_read(anon_vma);
> > +}
> > +
> 
> You keep adding more and more code on top of the existing mess. This is
> NOT what we want.
> 
Additional handling is introduced when enabling ANON_VMA_LAZY;
I will add comments to clarify this.