| Message ID | 20250415092548.271718-1-ye.liu@linux.dev (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/rmap: Move anon_vma initialization to anon_vma_ctor() | expand |
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.
在 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
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)