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

[Lorenzo Stoakes <ljs@kernel.org>]

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.

>
> 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@honor.com>

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.

> ---
>  include/linux/mm_types.h |  3 +++
>  mm/internal.h            | 54 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 57 insertions(+)
>
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index a308e2c23b82..5f4961ea1572 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -917,6 +917,9 @@ struct vm_area_desc {
>  	struct mmap_action action;
>  };
>
> +/* Tagged pointer stored in vma->anon_vma. Low bits encode anon_vma type. */
> +typedef unsigned long anon_vma_tree_t;
> +
>  /*
>   * This struct describes a virtual memory area. There is one of these
>   * per VM-area/task. A VM area is any part of the process virtual memory
> diff --git a/mm/internal.h b/mm/internal.h
> index 5a2ddcf68e0b..76544ad44ff0 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -246,6 +246,60 @@ static inline void anon_vma_unlock_read(struct anon_vma *anon_vma)
>  	up_read(&anon_vma->root->rwsem);
>  }
>
> +/* 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?

> +
> +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?...!

> +
> +static inline void anon_vma_tree_lock_write(anon_vma_tree_t anon_tree)
> +{
> +	struct anon_vma *anon_vma = anon_vma_tree_anon_vma(anon_tree);
> +
> +	anon_vma_lock_write(anon_vma);
> +}
> +
> +static inline int anon_vma_tree_trylock_write(anon_vma_tree_t anon_tree)
> +{
> +	struct anon_vma *anon_vma = anon_vma_tree_anon_vma(anon_tree);
> +
> +	return anon_vma_trylock_write(anon_vma);
> +}
> +
> +static inline void anon_vma_tree_unlock_write(anon_vma_tree_t anon_tree)
> +{
> +	struct anon_vma *anon_vma = anon_vma_tree_anon_vma(anon_tree);
> +
> +	anon_vma_unlock_write(anon_vma);
> +}
> +
> +static inline void anon_vma_tree_lock_read(anon_vma_tree_t anon_tree)
> +{
> +	struct anon_vma *anon_vma = anon_vma_tree_anon_vma(anon_tree);
> +
> +	anon_vma_lock_read(anon_vma);
> +}
> +
> +static inline int anon_vma_tree_trylock_read(anon_vma_tree_t anon_tree)
> +{
> +	struct anon_vma *anon_vma = anon_vma_tree_anon_vma(anon_tree);
> +
> +	return anon_vma_trylock_read(anon_vma);
> +}
> +
> +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.

>  struct anon_vma *folio_get_anon_vma(const struct folio *folio);
>
>  /* Operations which modify VMAs. */
> --
> 2.17.1
>