[PATCH] mm/mm_slot.h: add a helper function mm_slot_remove

[PATCH] mm/mm_slot.h: add a helper function mm_slot_remove

[xu.xin16]

From 69923b1aa7d702eeea7364a95d7b05ae82ebd3e0 Mon Sep 17 00:00:00 2001
From: xu xin <xu.xin16@zte.com.cn>
Date: Fri, 10 Jul 2026 12:29:57 +0800
Subject: [PATCH] mm/mm_slot.h: add a helper function mm_slot_remove

Both khugepaged (THP collapsing) and KSM manage per-mm scanning slots
using the mm_slot structure. The slot is kept in a hash table and a
list, and removal from both containers requires the same two operations:
hash_del() and list_del().

Currently, this two‑step removal is open‑coded in multiple places
across khugepaged.c and ksm.c. This duplication makes the code slightly
harder to read and maintain, and creates the risk that future changes to
the removal logic might miss one of the call sites.

Introduce a helper function mm_slot_remove() that encapsulates both
deletions. This reduces redundancy, improves code clarity, and ensures
that the removal sequence stays consistent.

No functional change is intended.

Signed-off-by: xu xin <xu.xin16@zte.com.cn>
---
 mm/khugepaged.c | 6 ++----
 mm/ksm.c        | 9 +++------
 mm/mm_slot.h    | 7 +++++++
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index 58e14d1543ec..5f6eb1bd9a67 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -606,8 +606,7 @@ void __khugepaged_exit(struct mm_struct *mm)
 	spin_lock(&khugepaged_mm_lock);
 	slot = mm_slot_lookup(mm_slots_hash, mm);
 	if (slot && khugepaged_scan.mm_slot != slot) {
-		hash_del(&slot->hash);
-		list_del(&slot->mm_node);
+		mm_slot_remove(slot);
 		free = 1;
 	}
 	spin_unlock(&khugepaged_mm_lock);
@@ -1802,8 +1801,7 @@ static void collect_mm_slot(struct mm_slot *slot)

 	if (collapse_test_exit(mm)) {
 		/* free mm_slot */
-		hash_del(&slot->hash);
-		list_del(&slot->mm_node);
+		mm_slot_remove(slot);

 		/*
 		 * Not strictly needed because the mm exited already.
diff --git a/mm/ksm.c b/mm/ksm.c
index 2791ce5bd44b..443dab513da6 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -1257,8 +1257,7 @@ static int unmerge_and_remove_all_rmap_items(void)
 				  struct mm_slot, mm_node);
 		ksm_scan.mm_slot = mm_slot_entry(slot, struct ksm_mm_slot, slot);
 		if (ksm_test_exit(mm)) {
-			hash_del(&mm_slot->slot.hash);
-			list_del(&mm_slot->slot.mm_node);
+			mm_slot_remove(&mm_slot->slot);
 			spin_unlock(&ksm_mmlist_lock);

 			mm_slot_free(mm_slot_cache, mm_slot);
@@ -2772,8 +2771,7 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
 		 * or when all VM_MERGEABLE areas have been unmapped (and
 		 * mmap_lock then protects against race with MADV_MERGEABLE).
 		 */
-		hash_del(&mm_slot->slot.hash);
-		list_del(&mm_slot->slot.mm_node);
+		mm_slot_remove(&mm_slot->slot);
 		spin_unlock(&ksm_mmlist_lock);

 		mm_slot_free(mm_slot_cache, mm_slot);
@@ -3116,8 +3114,7 @@ void __ksm_exit(struct mm_struct *mm)
 	if (ksm_scan.mm_slot == mm_slot)
 		goto unlock;
 	if (!mm_slot->rmap_list) {
-		hash_del(&slot->hash);
-		list_del(&slot->mm_node);
+		mm_slot_remove(slot);
 		easy_to_free = 1;
 	} else {
 		list_move(&slot->mm_node,
diff --git a/mm/mm_slot.h b/mm/mm_slot.h
index 83f18ed1c4bd..bc3b9ae8fdbc 100644
--- a/mm/mm_slot.h
+++ b/mm/mm_slot.h
@@ -52,4 +52,11 @@ static inline void mm_slot_free(struct kmem_cache *cache, void *objp)
 	hash_add(_hashtable, &_mm_slot->hash, (unsigned long)_mm);	       \
 })

+static inline void mm_slot_remove(struct mm_slot *_mm_slot)
+{
+	hash_del(&_mm_slot->hash);
+	list_del(&_mm_slot->mm_node);
+}
+
+
 #endif /* _LINUX_MM_SLOT_H */
-- 
2.25.1

Re: [PATCH] mm/mm_slot.h: add a helper function mm_slot_remove

[Lorenzo Stoakes]

On Fri, 10 Jul 2026 13:15:36 +0800, xu.xin16@zte.com.cn <xu.xin16@zte.com.cn> wrote:
> Both khugepaged (THP collapsing) and KSM manage per-mm scanning slots
> using the mm_slot structure. The slot is kept in a hash table and a
> list, and removal from both containers requires the same two operations:
> hash_del() and list_del().
> 
> Currently, this two‑step removal is open‑coded in multiple places

Let's not have em-dots in the commit message ;)

> across khugepaged.c and ksm.c. This duplication makes the code slightly
> harder to read and maintain, and creates the risk that future changes to
> the removal logic might miss one of the call sites.
> 
> Introduce a helper function mm_slot_remove() that encapsulates both
> deletions. This reduces redundancy, improves code clarity, and ensures
> that the removal sequence stays consistent.

You're talking far too much about a trivial change, just say something like
'this patch introduces mm_remove() to abstract this'.

> 
> No functional change is intended.
> 
> Signed-off-by: xu xin <xu.xin16@zte.com.cn>

In general it's fine but I want more :) see below.

>
>
> diff --git a/mm/mm_slot.h b/mm/mm_slot.h
> index 83f18ed1c4bd..bc3b9ae8fdbc 100644
> --- a/mm/mm_slot.h
> +++ b/mm/mm_slot.h
> @@ -52,4 +52,11 @@ static inline void mm_slot_free(struct kmem_cache *cache, void *objp)
>  	hash_add(_hashtable, &_mm_slot->hash, (unsigned long)_mm);	       \
>  })
>  

mm_slot.h also inexplicably implements mm_slot_lookup() and mm_slot_insert() as
macros. Can you change these to static inline functions in the respin?

Also rename the horrible names from _foo -> foo too please.

And we don't really need 'mm_slot' in the var name for a slot just 'slot' works
fine.

> +static inline void mm_slot_remove(struct mm_slot *_mm_slot)

Yeah this name is gross just 'slot' please.

> +{
> +	hash_del(&_mm_slot->hash);
> +	list_del(&_mm_slot->mm_node);
> +}
> +
> +
>  #endif /* _LINUX_MM_SLOT_H */

Thanks, Lorenzo

-- 
Lorenzo Stoakes <ljs@kernel.org>

Re: [PATCH] mm/mm_slot.h: add a helper function mm_slot_remove

[Matthew Wilcox]

On Fri, Jul 10, 2026 at 01:17:37PM +0100, Lorenzo Stoakes wrote:
> In general it's fine but I want more :) see below.

... and these functions all need kernel-doc.
Not sure where to suggest that it be added to the Documentation/
Thoughts?

Re: [PATCH] mm/mm_slot.h: add a helper function mm_slot_remove

[Lorenzo Stoakes]

On Sat, Jul 11, 2026 at 05:30:41AM +0100, Matthew Wilcox wrote:
> On Fri, Jul 10, 2026 at 01:17:37PM +0100, Lorenzo Stoakes wrote:
> > In general it's fine but I want more :) see below.
>
> ... and these functions all need kernel-doc.
> Not sure where to suggest that it be added to the Documentation/
> Thoughts?

Well could just send out to the KSM/THP people? If somebody has some bandwidth
for it.

Or... are you volunteering yourself? ;)

It'd be unfair to ask Xu to also add kdoc I think, perhaps do that as a
follow-up?

Just need to get those nasty pointless (AFAICT) macros gone.

Cheers, Lorenzo

Re: [PATCH] mm/mm_slot.h: add a helper function mm_slot_remove

[xu.xin16]

> > No functional change is intended.
> > 
> > Signed-off-by: xu xin <xu.xin16@zte.com.cn>
> 
> In general it's fine but I want more :) see below.
> 
> >
> >
> > diff --git a/mm/mm_slot.h b/mm/mm_slot.h
> > index 83f18ed1c4bd..bc3b9ae8fdbc 100644
> > --- a/mm/mm_slot.h
> > +++ b/mm/mm_slot.h
> > @@ -52,4 +52,11 @@ static inline void mm_slot_free(struct kmem_cache *cache, void *objp)
> >  	hash_add(_hashtable, &_mm_slot->hash, (unsigned long)_mm);	       \
> >  })
> >  
> 
> mm_slot.h also inexplicably implements mm_slot_lookup() and mm_slot_insert() as
> macros. Can you change these to static inline functions in the respin?

Sorry, I looked into turning mm_slot_lookup() and mm_slot_insert() into
static inline functions, but unfortunately it’s not possible because
they rely on hash_for_each_possible() and hash_add(), which expect an
actual array instead of pointer so that sizeof() can determine the
hash table size. Passing a struct hlist_head * would make sizeof() return
the pointer size instead of the array size, leading to incorrect bucket
indexing and memory corruption.

> 
> Also rename the horrible names from _foo -> foo too please.
> 
> And we don't really need 'mm_slot' in the var name for a slot just 'slot' works
> fine.

Ok, I can clean up these stuff.

> 
> > +static inline void mm_slot_remove(struct mm_slot *_mm_slot)
> 
> Yeah this name is gross just 'slot' please.
> 
> > +{
> > +	hash_del(&_mm_slot->hash);
> > +	list_del(&_mm_slot->mm_node);
> > +}
> > +
> > +
> >  #endif /* _LINUX_MM_SLOT_H */
> 
> Thanks, Lorenzo
>

Re: [PATCH] mm/mm_slot.h: add a helper function mm_slot_remove

[Lorenzo Stoakes]

On Mon, Jul 13, 2026 at 01:44:54PM +0800, xu.xin16@zte.com.cn wrote:
> > > No functional change is intended.
> > >
> > > Signed-off-by: xu xin <xu.xin16@zte.com.cn>
> >
> > In general it's fine but I want more :) see below.
> >
> > >
> > >
> > > diff --git a/mm/mm_slot.h b/mm/mm_slot.h
> > > index 83f18ed1c4bd..bc3b9ae8fdbc 100644
> > > --- a/mm/mm_slot.h
> > > +++ b/mm/mm_slot.h
> > > @@ -52,4 +52,11 @@ static inline void mm_slot_free(struct kmem_cache *cache, void *objp)
> > >  	hash_add(_hashtable, &_mm_slot->hash, (unsigned long)_mm);	       \
> > >  })
> > >
> >
> > mm_slot.h also inexplicably implements mm_slot_lookup() and mm_slot_insert() as
> > macros. Can you change these to static inline functions in the respin?
>
> Sorry, I looked into turning mm_slot_lookup() and mm_slot_insert() into
> static inline functions, but unfortunately it’s not possible because
> they rely on hash_for_each_possible() and hash_add(), which expect an
> actual array instead of pointer so that sizeof() can determine the
> hash table size. Passing a struct hlist_head * would make sizeof() return
> the pointer size instead of the array size, leading to incorrect bucket
> indexing and memory corruption.

Ah that's fair enough then, explains why they're macros there :)


>
> >
> > Also rename the horrible names from _foo -> foo too please.
> >
> > And we don't really need 'mm_slot' in the var name for a slot just 'slot' works
> > fine.
>
> Ok, I can clean up these stuff.

Thanks!

>
> >
> > > +static inline void mm_slot_remove(struct mm_slot *_mm_slot)
> >
> > Yeah this name is gross just 'slot' please.
> >
> > > +{
> > > +	hash_del(&_mm_slot->hash);
> > > +	list_del(&_mm_slot->mm_node);
> > > +}
> > > +
> > > +
> > >  #endif /* _LINUX_MM_SLOT_H */
> >
> > Thanks, Lorenzo
> >

Cheers, Lorenzo