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

[Harry Yoo <harry.yoo@oracle.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>
> ---

NACK unless the patch explains how the object's initial state
('constructed state') is preserved between uses.

anon_vma_ctor() is a slab constructor. That means it is called only once
when a slab (folio) is allocated, and not called again when an anon_vma
is allocated from an existing slab (folio). In other words it is not called
everytime an object allocated via kmem_cache_alloc() interface.

This patch looks very dangerous to me and makes me question whether you
tested it before submission.

-- 
Cheers,
Harry / Hyeonggon

>  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)
> -- 
> 2.25.1
> 
>