| 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
+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. :)
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.
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)