[PATCH 0/2] mm/ksm: clean up scan_get_next_rmap_item() initialization

[PATCH 0/2] mm/ksm: clean up scan_get_next_rmap_item() initialization

[Xu Xin (vivo)]

Currently, scan_get_next_rmap_item() has grown quite large and complex.
When introducing new features, such as the only-merge-zero-page mode we
are currently working on, we don't want to add more complexity to it
and don't want to touch rmap_item structures to avoid unnecessary overhead,
so we need to extract some common logic.

With these changes, the entry of scan_get_next_rmap_item() becomes much
cleaner and easier to follow, instead of being cluttered with unrelated
boilerplate such as stale stable_node pruning and cycle initialization.

Let's start by cleaning up these two parts at the beginning (where I think
abstraction is necessary), and we can continue cleaning up further if
needed later.


Xu Xin (vivo) (2):
  mm/ksm: extract helper functions for scan cycle cleanup
  mm/ksm: add initialization helpers for scan_get_next_rmap_item()

 mm/ksm.c | 126 ++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 79 insertions(+), 47 deletions(-)

-- 
2.34.1

[PATCH 1/2] mm/ksm: extract helper functions for scan cycle cleanup

[Xu Xin (vivo)]

Since scan_get_next_rmap_item() is getting too large, extract two new
helper functions, ksm_prune_migrate_nodes() and ksm_reset_unstable_trees(),
to reduce indentation levels, isolate local variables, and improve the
readability of the main scan loop.

No functional change intended.

Signed-off-by: Xu Xin (vivo) <xu.xin@linux.dev>
---
 mm/ksm.c | 56 +++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 33 insertions(+), 23 deletions(-)

diff --git a/mm/ksm.c b/mm/ksm.c
index 5e24b5ca8dc8..49977bcf04f7 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -2609,6 +2609,37 @@ static struct mm_walk_ops ksm_next_page_ops = {
 	.walk_lock = PGWALK_RDLOCK,
 };
 
+static void ksm_prune_migrate_nodes(void)
+{
+	struct ksm_stable_node *stable_node, *next;
+	struct folio *folio;
+
+	if (ksm_merge_across_nodes)
+		return;
+
+	/*
+	 * Whereas stale stable_nodes on the stable_tree itself
+	 * get pruned in the regular course of stable_tree_search(),
+	 * those moved out to the migrate_nodes list can accumulate:
+	 * so prune them once before each full scan.
+	 */
+	list_for_each_entry_safe(stable_node, next, &migrate_nodes, list) {
+		folio = ksm_get_folio(stable_node, KSM_GET_FOLIO_NOLOCK);
+
+		if (folio)
+			folio_put(folio);
+		cond_resched();
+	}
+}
+
+static void ksm_reset_unstable_trees(void)
+{
+	int nid;
+
+	for (nid = 0; nid < ksm_nr_node_ids; nid++)
+		root_unstable_tree[nid] = RB_ROOT;
+}
+
 static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
 {
 	struct mm_struct *mm;
@@ -2617,7 +2648,6 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
 	struct vm_area_struct *vma;
 	struct ksm_rmap_item *rmap_item;
 	struct vma_iterator vmi;
-	int nid;
 
 	if (list_empty(&ksm_mm_head.slot.mm_node))
 		return NULL;
@@ -2639,28 +2669,8 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
 		 */
 		lru_add_drain_all();
 
-		/*
-		 * Whereas stale stable_nodes on the stable_tree itself
-		 * get pruned in the regular course of stable_tree_search(),
-		 * those moved out to the migrate_nodes list can accumulate:
-		 * so prune them once before each full scan.
-		 */
-		if (!ksm_merge_across_nodes) {
-			struct ksm_stable_node *stable_node, *next;
-			struct folio *folio;
-
-			list_for_each_entry_safe(stable_node, next,
-						 &migrate_nodes, list) {
-				folio = ksm_get_folio(stable_node,
-						      KSM_GET_FOLIO_NOLOCK);
-				if (folio)
-					folio_put(folio);
-				cond_resched();
-			}
-		}
-
-		for (nid = 0; nid < ksm_nr_node_ids; nid++)
-			root_unstable_tree[nid] = RB_ROOT;
+		ksm_prune_migrate_nodes();
+		ksm_reset_unstable_trees();
 
 		spin_lock(&ksm_mmlist_lock);
 		slot = list_entry(mm_slot->slot.mm_node.next,
-- 
2.34.1

[PATCH 2/2] mm/ksm: add initialization helpers for scan_get_next_rmap_item()

[Xu Xin (vivo)]

Extract the new scan cycle detection (including draining per-CPU LRU
caches) and advancing the scan cursor to the first mm_slot into
scan_begin_new_cycle() and scan_get_first_mm().

This keeps the exact execution order intact while making
scan_get_next_rmap_item() cleaner and easier to follow.

No functional change intended.

Signed-off-by: Xu Xin (vivo) <xu.xin@linux.dev>
---
 mm/ksm.c | 70 +++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 46 insertions(+), 24 deletions(-)

diff --git a/mm/ksm.c b/mm/ksm.c
index 49977bcf04f7..e79e64372e49 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -2640,6 +2640,49 @@ static void ksm_reset_unstable_trees(void)
 		root_unstable_tree[nid] = RB_ROOT;
 }
 
+static bool scan_begin_new_cycle(void)
+{
+	/* Check if the cursor points to the head. */
+	if (ksm_scan.mm_slot != &ksm_mm_head)
+		return false;
+
+	/*
+	 * A number of pages can hang around indefinitely in per-cpu
+	 * LRU cache, raised page count preventing write_protect_page
+	 * from merging them.  Though it doesn't really matter much,
+	 * it is puzzling to see some stuck in pages_volatile until
+	 * other activity jostles them out, and they also prevented
+	 * LTP's KSM test from succeeding deterministically; so drain
+	 * them here (here rather than on entry to ksm_do_scan(),
+	 * so we don't IPI too often when pages_to_scan is set low).
+	 */
+	lru_add_drain_all();
+	return true;
+}
+
+static struct ksm_mm_slot *scan_get_first_mm(void)
+{
+	struct ksm_mm_slot *mm_slot = ksm_scan.mm_slot;
+	struct mm_slot *slot;
+
+	spin_lock(&ksm_mmlist_lock);
+
+	slot = list_entry(mm_slot->slot.mm_node.next, struct mm_slot, mm_node);
+	mm_slot = mm_slot_entry(slot, struct ksm_mm_slot, slot);
+	ksm_scan.mm_slot = mm_slot;
+
+	spin_unlock(&ksm_mmlist_lock);
+
+	/*
+	 * Although we tested list_empty() above, a racing __ksm_exit
+	 * of the last mm on the list may have removed it since then.
+	 */
+	if (mm_slot == &ksm_mm_head)
+		return NULL;
+
+	return mm_slot;
+}
+
 static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
 {
 	struct mm_struct *mm;
@@ -2653,36 +2696,15 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
 		return NULL;
 
 	mm_slot = ksm_scan.mm_slot;
-	if (mm_slot == &ksm_mm_head) {
+	if (scan_begin_new_cycle()) {
 		advisor_start_scan();
 		trace_ksm_start_scan(ksm_scan.seqnr, ksm_rmap_items);
 
-		/*
-		 * A number of pages can hang around indefinitely in per-cpu
-		 * LRU cache, raised page count preventing write_protect_page
-		 * from merging them.  Though it doesn't really matter much,
-		 * it is puzzling to see some stuck in pages_volatile until
-		 * other activity jostles them out, and they also prevented
-		 * LTP's KSM test from succeeding deterministically; so drain
-		 * them here (here rather than on entry to ksm_do_scan(),
-		 * so we don't IPI too often when pages_to_scan is set low).
-		 */
-		lru_add_drain_all();
-
 		ksm_prune_migrate_nodes();
 		ksm_reset_unstable_trees();
 
-		spin_lock(&ksm_mmlist_lock);
-		slot = list_entry(mm_slot->slot.mm_node.next,
-				  struct mm_slot, mm_node);
-		mm_slot = mm_slot_entry(slot, struct ksm_mm_slot, slot);
-		ksm_scan.mm_slot = mm_slot;
-		spin_unlock(&ksm_mmlist_lock);
-		/*
-		 * Although we tested list_empty() above, a racing __ksm_exit
-		 * of the last mm on the list may have removed it since then.
-		 */
-		if (mm_slot == &ksm_mm_head)
+		mm_slot = scan_get_first_mm();
+		if (!mm_slot)
 			return NULL;
 next_mm:
 		ksm_scan.address = 0;
-- 
2.34.1

Re: [PATCH 1/2] mm/ksm: extract helper functions for scan cycle cleanup

[Lorenzo Stoakes (ARM)]

On Thu, Nov 12, 2026 at 08:59:28PM +0800, Xu Xin (vivo) wrote:
> Since scan_get_next_rmap_item() is getting too large, extract two new
> helper functions, ksm_prune_migrate_nodes() and ksm_reset_unstable_trees(),
> to reduce indentation levels, isolate local variables, and improve the
> readability of the main scan loop.
>
> No functional change intended.
>
> Signed-off-by: Xu Xin (vivo) <xu.xin@linux.dev>

Thanks for doing this! LGTM so:

Reviewed-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>

One small nit below, but it's not a big deal :)

> ---
>  mm/ksm.c | 56 +++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 33 insertions(+), 23 deletions(-)
>
> diff --git a/mm/ksm.c b/mm/ksm.c
> index 5e24b5ca8dc8..49977bcf04f7 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
[ ... ]
> +static void ksm_reset_unstable_trees(void)
> +{
> +	int nid;
> +
> +	for (nid = 0; nid < ksm_nr_node_ids; nid++)
> +		root_unstable_tree[nid] = RB_ROOT;
> +}

Recently I learned (and PeterZ reminded me :) that it's actually valid to
declare the var in the for-block in linux kernel C.

So this can be:

static void ksm_reset_unstable_trees(void)
{
	for (int nid = 0; nid < ksm_nr_node_ids; nid++)
		root_unstable_tree[nid] = RB_ROOT;
}

:)

--
Cheers, Lorenzo

Re: [PATCH 2/2] mm/ksm: add initialization helpers for scan_get_next_rmap_item()

[Lorenzo Stoakes (ARM)]

On Thu, Nov 12, 2026 at 08:59:59PM +0800, Xu Xin (vivo) wrote:
> Extract the new scan cycle detection (including draining per-CPU LRU
> caches) and advancing the scan cursor to the first mm_slot into
> scan_begin_new_cycle() and scan_get_first_mm().
>
> This keeps the exact execution order intact while making
> scan_get_next_rmap_item() cleaner and easier to follow.
>
> No functional change intended.
>
> Signed-off-by: Xu Xin (vivo) <xu.xin@linux.dev>

Again, thanks for doing this! LGTM and:

Reviewed-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>

> ---
>  mm/ksm.c | 70 +++++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 46 insertions(+), 24 deletions(-)
>
> diff --git a/mm/ksm.c b/mm/ksm.c
> index 49977bcf04f7..e79e64372e49 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -2640,6 +2640,49 @@ static void ksm_reset_unstable_trees(void)
>  		root_unstable_tree[nid] = RB_ROOT;
>  }
>
> +static bool scan_begin_new_cycle(void)
> +{
> +	/* Check if the cursor points to the head. */
> +	if (ksm_scan.mm_slot != &ksm_mm_head)
> +		return false;
> +
> +	/*
> +	 * A number of pages can hang around indefinitely in per-cpu
> +	 * LRU cache, raised page count preventing write_protect_page
> +	 * from merging them.  Though it doesn't really matter much,
> +	 * it is puzzling to see some stuck in pages_volatile until
> +	 * other activity jostles them out, and they also prevented
> +	 * LTP's KSM test from succeeding deterministically; so drain
> +	 * them here (here rather than on entry to ksm_do_scan(),
> +	 * so we don't IPI too often when pages_to_scan is set low).
> +	 */
> +	lru_add_drain_all();

The sashiko finding is silly, this reorders it after advisor_start_scan() which
calls... ktime_get(), yeah it's zero delta :)

> +	return true;
> +}
> +
> +static struct ksm_mm_slot *scan_get_first_mm(void)
> +{
> +	struct ksm_mm_slot *mm_slot = ksm_scan.mm_slot;
> +	struct mm_slot *slot;
> +
> +	spin_lock(&ksm_mmlist_lock);
> +
> +	slot = list_entry(mm_slot->slot.mm_node.next, struct mm_slot, mm_node);
> +	mm_slot = mm_slot_entry(slot, struct ksm_mm_slot, slot);
> +	ksm_scan.mm_slot = mm_slot;
> +
> +	spin_unlock(&ksm_mmlist_lock);

NIT: optional, but this could be:

	scoped_guard(mutex, &ksm_mmlist_lock) {
		slot = list_entry(mm_slot->slot.mm_node.next, struct mm_slot, mm_node);
		mm_slot = mm_slot_entry(slot, struct ksm_mm_slot, slot);
		ksm_scan.mm_slot = mm_slot;
	}

> +
> +	/*
> +	 * Although we tested list_empty() above, a racing __ksm_exit
> +	 * of the last mm on the list may have removed it since then.
> +	 */
> +	if (mm_slot == &ksm_mm_head)
> +		return NULL;
> +
> +	return mm_slot;
> +}
> +
>  static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
>  {
>  	struct mm_struct *mm;
> @@ -2653,36 +2696,15 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
>  		return NULL;
>
>  	mm_slot = ksm_scan.mm_slot;
> -	if (mm_slot == &ksm_mm_head) {
> +	if (scan_begin_new_cycle()) {
>  		advisor_start_scan();
>  		trace_ksm_start_scan(ksm_scan.seqnr, ksm_rmap_items);
>
> -		/*
> -		 * A number of pages can hang around indefinitely in per-cpu
> -		 * LRU cache, raised page count preventing write_protect_page
> -		 * from merging them.  Though it doesn't really matter much,
> -		 * it is puzzling to see some stuck in pages_volatile until
> -		 * other activity jostles them out, and they also prevented
> -		 * LTP's KSM test from succeeding deterministically; so drain
> -		 * them here (here rather than on entry to ksm_do_scan(),
> -		 * so we don't IPI too often when pages_to_scan is set low).
> -		 */
> -		lru_add_drain_all();
> -
>  		ksm_prune_migrate_nodes();
>  		ksm_reset_unstable_trees();
>
> -		spin_lock(&ksm_mmlist_lock);
> -		slot = list_entry(mm_slot->slot.mm_node.next,
> -				  struct mm_slot, mm_node);
> -		mm_slot = mm_slot_entry(slot, struct ksm_mm_slot, slot);
> -		ksm_scan.mm_slot = mm_slot;
> -		spin_unlock(&ksm_mmlist_lock);
> -		/*
> -		 * Although we tested list_empty() above, a racing __ksm_exit
> -		 * of the last mm on the list may have removed it since then.
> -		 */
> -		if (mm_slot == &ksm_mm_head)
> +		mm_slot = scan_get_first_mm();
> +		if (!mm_slot)
>  			return NULL;
>  next_mm:
>  		ksm_scan.address = 0;
> --
> 2.34.1
>

--
Cheers, Lorenzo

Re: [PATCH 1/2] mm/ksm: extract helper functions for scan cycle cleanup

[David Hildenbrand (Arm)]

On 11/12/26 13:59, Xu Xin (vivo) wrote:
> Since scan_get_next_rmap_item() is getting too large, extract two new
> helper functions, ksm_prune_migrate_nodes() and ksm_reset_unstable_trees(),
> to reduce indentation levels, isolate local variables, and improve the
> readability of the main scan loop.
> 
> No functional change intended.
> 
> Signed-off-by: Xu Xin (vivo) <xu.xin@linux.dev>
> ---

Acked-by: David Hildenbrand (Arm) <david@kernel.org>

-- 
Cheers,

David

Re: [PATCH 2/2] mm/ksm: add initialization helpers for scan_get_next_rmap_item()

[David Hildenbrand (Arm)]

On 11/12/26 13:59, Xu Xin (vivo) wrote:
> Extract the new scan cycle detection (including draining per-CPU LRU
> caches) and advancing the scan cursor to the first mm_slot into
> scan_begin_new_cycle() and scan_get_first_mm().
> 
> This keeps the exact execution order intact while making
> scan_get_next_rmap_item() cleaner and easier to follow.
> 
> No functional change intended.
> 
> Signed-off-by: Xu Xin (vivo) <xu.xin@linux.dev>
> ---
>  mm/ksm.c | 70 +++++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 46 insertions(+), 24 deletions(-)
> 
> diff --git a/mm/ksm.c b/mm/ksm.c
> index 49977bcf04f7..e79e64372e49 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -2640,6 +2640,49 @@ static void ksm_reset_unstable_trees(void)
>  		root_unstable_tree[nid] = RB_ROOT;
>  }
>  
> +static bool scan_begin_new_cycle(void)
> +{
> +	/* Check if the cursor points to the head. */
> +	if (ksm_scan.mm_slot != &ksm_mm_head)
> +		return false;
> +
> +	/*
> +	 * A number of pages can hang around indefinitely in per-cpu
> +	 * LRU cache, raised page count preventing write_protect_page
> +	 * from merging them.  Though it doesn't really matter much,
> +	 * it is puzzling to see some stuck in pages_volatile until
> +	 * other activity jostles them out, and they also prevented
> +	 * LTP's KSM test from succeeding deterministically; so drain
> +	 * them here (here rather than on entry to ksm_do_scan(),
> +	 * so we don't IPI too often when pages_to_scan is set low).
> +	 */
> +	lru_add_drain_all();
> +	return true;
> +}
> +
> +static struct ksm_mm_slot *scan_get_first_mm(void)
> +{
> +	struct ksm_mm_slot *mm_slot = ksm_scan.mm_slot;
> +	struct mm_slot *slot;
> +
> +	spin_lock(&ksm_mmlist_lock);
> +
> +	slot = list_entry(mm_slot->slot.mm_node.next, struct mm_slot, mm_node);
> +	mm_slot = mm_slot_entry(slot, struct ksm_mm_slot, slot);
> +	ksm_scan.mm_slot = mm_slot;
> +
> +	spin_unlock(&ksm_mmlist_lock);
> +
> +	/*
> +	 * Although we tested list_empty() above, a racing __ksm_exit
> +	 * of the last mm on the list may have removed it since then.
> +	 */
> +	if (mm_slot == &ksm_mm_head)
> +		return NULL;
> +
> +	return mm_slot;
> +}

I like this function cleanup here, but not sure about the other one.

Can we just leave the lru_add_drain_all() where it is, or what's the reason for
that change?

Note that that might soon get removed either way, so I wouldn't worry about it.

Then, I would rename the function into something like

"scan_should_start_new_cycle"

And keep it simple.

-- 
Cheers,

David