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

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.

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

在 2025/4/15 19:28, Harry Yoo 写道:
> 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.
Thank you for the feedback. You're absolutely right — I misunderstood
how the slab constructor (ctor) works. I had assumed it would be called
every time an object is allocated via kmem_cache_alloc(), but I now
realize it is only called once when a new slab is initialized, not on
every object allocation.
> This patch looks very dangerous to me and makes me question whether you
> tested it before submission.
>
Appreciate you catching this — and yes, I'll test it more thoroughly
before submitting other patches.

Drop it.

Thanks,
Ye                                                                     

+cc David, Liam

On Wed, Apr 16, 2025 at 09:34:30AM +0800, Ye Liu wrote:
>
> 在 2025/4/15 19:28, Harry Yoo 写道:
> > 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.
> Thank you for the feedback. You're absolutely right — I misunderstood
> how the slab constructor (ctor) works. I had assumed it would be called
> every time an object is allocated via kmem_cache_alloc(), but I now
> realize it is only called once when a new slab is initialized, not on
> every object allocation.
> > This patch looks very dangerous to me and makes me question whether you
> > tested it before submission.
> >
> Appreciate you catching this — and yes, I'll test it more thoroughly
> before submitting other patches.
>
> Drop it.
>
> Thanks,
> Ye
>

I agree with Harry's assessment, also this is something that needs to be cc'd to
other developers - I am more generally working on anon_vma at the moment and
this is vma-adjacent even if slab-ish :)

It isn't your fault that you didn't, we need a MAINTAINERS entry for this which
I will submit, but for any future revision, please do cc Harry, David, Liam,
Vlastimil + myself.

I should be sending a MAINTAINERS patch later today which should correct this
going forward!

Cheers, Lorenzo

On 16.04.25 14:59, Lorenzo Stoakes wrote:
> +cc David, Liam
> 
> On Wed, Apr 16, 2025 at 09:34:30AM +0800, Ye Liu wrote:
>>
>> 在 2025/4/15 19:28, Harry Yoo 写道:
>>> 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.
>> Thank you for the feedback. You're absolutely right — I misunderstood
>> how the slab constructor (ctor) works. I had assumed it would be called
>> every time an object is allocated via kmem_cache_alloc(), but I now
>> realize it is only called once when a new slab is initialized, not on
>> every object allocation.
>>> This patch looks very dangerous to me and makes me question whether you
>>> tested it before submission.
>>>
>> Appreciate you catching this — and yes, I'll test it more thoroughly
>> before submitting other patches.
>>
>> Drop it.
>>
>> Thanks,
>> Ye
>>
> 
> I agree with Harry's assessment, also this is something that needs to be cc'd to
> other developers - I am more generally working on anon_vma at the moment and
> this is vma-adjacent even if slab-ish :)

I stumbled over this patch on linux-mm and agreed with Harry's 
assessment as well. :)

-- 
Cheers,

David / dhildenb