Re: [PATCH] mm/rmap: Move anon_vma initialization to anon_vma_ctor()

[Mateusz Guzik <mjguzik@gmail.com>]

On Tue, Apr 15, 2025 at 05:25:48PM +0800, Ye Liu wrote:
> From: Ye Liu <liuye@kylinos.cn>
> 
> Currently, some initialization of anon_vma is performed in
> anon_vma_alloc(). Move the initialization to anon_vma_ctor()
> so that all object setup is handled in one place.
> 
> Signed-off-by: Ye Liu <liuye@kylinos.cn>
> ---
>  mm/rmap.c | 27 ++++++++++-----------------
>  1 file changed, 10 insertions(+), 17 deletions(-)
> 
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 67bb273dfb80..9802b1c27e4b 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -89,22 +89,7 @@ static struct kmem_cache *anon_vma_chain_cachep;
>  
>  static inline struct anon_vma *anon_vma_alloc(void)
>  {
> -	struct anon_vma *anon_vma;
> -
> -	anon_vma = kmem_cache_alloc(anon_vma_cachep, GFP_KERNEL);
> -	if (anon_vma) {
> -		atomic_set(&anon_vma->refcount, 1);
> -		anon_vma->num_children = 0;
> -		anon_vma->num_active_vmas = 0;
> -		anon_vma->parent = anon_vma;
> -		/*
> -		 * Initialise the anon_vma root to point to itself. If called
> -		 * from fork, the root will be reset to the parents anon_vma.
> -		 */
> -		anon_vma->root = anon_vma;
> -	}
> -
> -	return anon_vma;
> +	return kmem_cache_alloc(anon_vma_cachep, GFP_KERNEL);
>  }
>  
>  static inline void anon_vma_free(struct anon_vma *anon_vma)
> @@ -453,8 +438,16 @@ static void anon_vma_ctor(void *data)
>  	struct anon_vma *anon_vma = data;
>  
>  	init_rwsem(&anon_vma->rwsem);
> -	atomic_set(&anon_vma->refcount, 0);
> +	atomic_set(&anon_vma->refcount, 1);
>  	anon_vma->rb_root = RB_ROOT_CACHED;
> +	anon_vma->num_children = 0;
> +	anon_vma->num_active_vmas = 0;
> +	anon_vma->parent = anon_vma;
> +	/*
> +	 * Initialise the anon_vma root to point to itself. If called
> +	 * from fork, the root will be reset to the parents anon_vma.
> +	 */
> +	anon_vma->root = anon_vma;
>  }
>  
>  void __init anon_vma_init(void)

As was pointed out by Harry this changes behavior: by shifting to ctor
you make it only happen once instead of every time anon_vma_alloc() is
called. By extension this assumes ->refcount is 1 and so on. By any
chance did you assume ctor executes every time kmem_cache_alloc() is
called?

When making changes of the sort I find it beneficial to place debug-only
asserts in place of moved code at least during development.

So in particular in this case I would add checks like these:
	
VM_BUG_ON(anon_vma->num_children != 0);

and so on.

Then on a kernel compiled with DEBUG_MM this would validate the expected
value is in place and blow up early and loudly if not.