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

[PATCH v2 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.

Changes in v2:
- Rename scan_begin_new_cycle() to scan_should_start_new_cycle() and keep
  it simple (David).
- Keep lru_add_drain_all() in scan_get_next_rmap_item() (David).
- Use scoped_guard(spinlock, &ksm_mmlist_lock) in scan_get_first_mm()
  (Lorenzo).
- Collect Reviewed-by and Acked-by tags from v1.

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 | 98 +++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 62 insertions(+), 36 deletions(-)

-- 
2.34.1

[PATCH v2 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>
Reviewed-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
Acked-by: David Hildenbrand (Arm) <david@kernel.org>
---
 mm/ksm.c | 54 +++++++++++++++++++++++++++++++-----------------------
 1 file changed, 31 insertions(+), 23 deletions(-)

diff --git a/mm/ksm.c b/mm/ksm.c
index 5e24b5ca8dc8..8db6743ece6d 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -2609,6 +2609,35 @@ 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)
+{
+	for (int 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 +2646,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 +2667,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 v2 2/2] mm/ksm: add initialization helpers for scan_get_next_rmap_item()

[Xu Xin (vivo)]

Extract the new scan cycle check into scan_should_start_new_cycle()
and add the helper scan_get_first_mm() that advances the scan
cursor to the first mm_slot.

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>
Reviewed-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
---
Changes in v2:
- Rename scan_begin_new_cycle() to scan_should_start_new_cycle() and keep
  it simple (David).
- Keep lru_add_drain_all() in scan_get_next_rmap_item() (David).
- Use scoped_guard(spinlock, &ksm_mmlist_lock) in scan_get_first_mm()
  (Lorenzo).
- Keep Lorenzo's Reviewed-by tag as the changes are minor and follow the
  suggested cleanups (please let me know if you prefer otherwise).

 mm/ksm.c | 44 +++++++++++++++++++++++++++++++-------------
 1 file changed, 31 insertions(+), 13 deletions(-)

diff --git a/mm/ksm.c b/mm/ksm.c
index 8db6743ece6d..337fcc9c6322 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -2638,6 +2638,34 @@ static void ksm_reset_unstable_trees(void)
 		root_unstable_tree[nid] = RB_ROOT;
 }
 
+static bool scan_should_start_new_cycle(void)
+{
+	/* Check if the cursor points to the head. */
+	return ksm_scan.mm_slot == &ksm_mm_head;
+}
+
+static struct ksm_mm_slot *scan_get_first_mm(void)
+{
+	struct ksm_mm_slot *mm_slot = &ksm_mm_head;
+	struct mm_slot *slot;
+
+	scoped_guard(spinlock, &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;
@@ -2650,8 +2678,7 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
 	if (list_empty(&ksm_mm_head.slot.mm_node))
 		return NULL;
 
-	mm_slot = ksm_scan.mm_slot;
-	if (mm_slot == &ksm_mm_head) {
+	if (scan_should_start_new_cycle()) {
 		advisor_start_scan();
 		trace_ksm_start_scan(ksm_scan.seqnr, ksm_rmap_items);
 
@@ -2670,17 +2697,8 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
 		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 v2 2/2] mm/ksm: add initialization helpers for scan_get_next_rmap_item()

[kernel test robot]

Hi Xu,

kernel test robot noticed the following build warnings:

[auto build test WARNING on akpm-mm/mm-everything]

url:    https://github.com/intel-lab-lkp/linux/commits/Xu-Xin-vivo/mm-ksm-extract-helper-functions-for-scan-cycle-cleanup/20261115-160032
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link:    https://lore.kernel.org/r/20261115080049.132515-1-xu.xin%40linux.dev
patch subject: [PATCH v2 2/2] mm/ksm: add initialization helpers for scan_get_next_rmap_item()
config: loongarch-defconfig (https://download.01.org/0day-ci/archive/20261115/202611152302.OUfZlgfW-lkp@intel.com/config)
compiler: clang version 24.0.0git (https://github.com/llvm/llvm-project ca14e3d05c4e63d6e8103945c86b958ed2f55d5f)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20261115/202611152302.OUfZlgfW-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202611152302.OUfZlgfW-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> mm/ksm.c:2693:6: warning: variable 'mm_slot' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
    2693 |         if (scan_should_start_new_cycle()) {
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   mm/ksm.c:2720:10: note: uninitialized use occurs here
    2720 |         slot = &mm_slot->slot;
         |                 ^~~~~~~
   mm/ksm.c:2693:2: note: remove the 'if' if its condition is always true
    2693 |         if (scan_should_start_new_cycle()) {
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   mm/ksm.c:2684:29: note: initialize the variable 'mm_slot' to silence this warning
    2684 |         struct ksm_mm_slot *mm_slot;
         |                                    ^
         |                                     = NULL
   1 warning generated.


vim +2693 mm/ksm.c

  2680	
  2681	static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
  2682	{
  2683		struct mm_struct *mm;
  2684		struct ksm_mm_slot *mm_slot;
  2685		struct mm_slot *slot;
  2686		struct vm_area_struct *vma;
  2687		struct ksm_rmap_item *rmap_item;
  2688		struct vma_iterator vmi;
  2689	
  2690		if (list_empty(&ksm_mm_head.slot.mm_node))
  2691			return NULL;
  2692	
> 2693		if (scan_should_start_new_cycle()) {
  2694			advisor_start_scan();
  2695			trace_ksm_start_scan(ksm_scan.seqnr, ksm_rmap_items);
  2696	
  2697			/*
  2698			 * A number of pages can hang around indefinitely in per-cpu
  2699			 * LRU cache, raised page count preventing write_protect_page
  2700			 * from merging them.  Though it doesn't really matter much,
  2701			 * it is puzzling to see some stuck in pages_volatile until
  2702			 * other activity jostles them out, and they also prevented
  2703			 * LTP's KSM test from succeeding deterministically; so drain
  2704			 * them here (here rather than on entry to ksm_do_scan(),
  2705			 * so we don't IPI too often when pages_to_scan is set low).
  2706			 */
  2707			lru_add_drain_all();
  2708	
  2709			ksm_prune_migrate_nodes();
  2710			ksm_reset_unstable_trees();
  2711	
  2712			mm_slot = scan_get_first_mm();
  2713			if (!mm_slot)
  2714				return NULL;
  2715	next_mm:
  2716			ksm_scan.address = 0;
  2717			ksm_scan.rmap_list = &mm_slot->rmap_list;
  2718		}
  2719	
  2720		slot = &mm_slot->slot;
  2721		mm = slot->mm;
  2722		vma_iter_init(&vmi, mm, ksm_scan.address);
  2723	
  2724		mmap_read_lock(mm);
  2725		if (ksm_test_exit(mm))
  2726			goto no_vmas;
  2727	
  2728		for_each_vma(vmi, vma) {
  2729			if (!(vma->vm_flags & VM_MERGEABLE))
  2730				continue;
  2731			if (ksm_scan.address < vma->vm_start)
  2732				ksm_scan.address = vma->vm_start;
  2733			if (!vma_has_anon_rmap(vma))
  2734				ksm_scan.address = vma->vm_end;
  2735	
  2736			while (ksm_scan.address < vma->vm_end) {
  2737				struct ksm_next_page_arg ksm_next_page_arg;
  2738				struct page *tmp_page = NULL;
  2739				struct folio *folio;
  2740	
  2741				if (ksm_test_exit(mm))
  2742					goto no_vmas;
  2743	
  2744				int found;
  2745	
  2746				found = walk_page_range_vma(vma, ksm_scan.address,
  2747							    vma->vm_end,
  2748							    &ksm_next_page_ops,
  2749							    &ksm_next_page_arg);
  2750	
  2751				if (found > 0) {
  2752					folio = ksm_next_page_arg.folio;
  2753					tmp_page = ksm_next_page_arg.page;
  2754					ksm_scan.address = ksm_next_page_arg.addr;
  2755				} else {
  2756					VM_WARN_ON_ONCE(found < 0);
  2757					ksm_scan.address = vma->vm_end - PAGE_SIZE;
  2758				}
  2759	
  2760				if (tmp_page) {
  2761					flush_anon_page(vma, tmp_page, ksm_scan.address);
  2762					flush_dcache_page(tmp_page);
  2763					rmap_item = get_next_rmap_item(mm_slot,
  2764						ksm_scan.rmap_list, ksm_scan.address);
  2765					if (rmap_item) {
  2766						ksm_scan.rmap_list =
  2767								&rmap_item->rmap_list;
  2768	
  2769						if (should_skip_rmap_item(folio, rmap_item)) {
  2770							folio_put(folio);
  2771							goto next_page;
  2772						}
  2773	
  2774						ksm_scan.address += PAGE_SIZE;
  2775						*page = tmp_page;
  2776					} else {
  2777						folio_put(folio);
  2778					}
  2779					mmap_read_unlock(mm);
  2780					return rmap_item;
  2781				}
  2782	next_page:
  2783				ksm_scan.address += PAGE_SIZE;
  2784				cond_resched();
  2785			}
  2786		}
  2787	
  2788		if (ksm_test_exit(mm)) {
  2789	no_vmas:
  2790			ksm_scan.address = 0;
  2791			ksm_scan.rmap_list = &mm_slot->rmap_list;
  2792		}
  2793		/*
  2794		 * Nuke all the rmap_items that are above this current rmap:
  2795		 * because there were no VM_MERGEABLE vmas with such addresses.
  2796		 */
  2797		remove_trailing_rmap_items(ksm_scan.rmap_list);
  2798	
  2799		spin_lock(&ksm_mmlist_lock);
  2800		slot = list_entry(mm_slot->slot.mm_node.next,
  2801				  struct mm_slot, mm_node);
  2802		ksm_scan.mm_slot = mm_slot_entry(slot, struct ksm_mm_slot, slot);
  2803		if (ksm_scan.address == 0) {
  2804			/*
  2805			 * We've completed a full scan of all vmas, holding mmap_lock
  2806			 * throughout, and found no VM_MERGEABLE: so do the same as
  2807			 * __ksm_exit does to remove this mm from all our lists now.
  2808			 * This applies either when cleaning up after __ksm_exit
  2809			 * (but beware: we can reach here even before __ksm_exit),
  2810			 * or when all VM_MERGEABLE areas have been unmapped (and
  2811			 * mmap_lock then protects against race with MADV_MERGEABLE).
  2812			 */
  2813			mm_slot_remove(&mm_slot->slot);
  2814			spin_unlock(&ksm_mmlist_lock);
  2815	
  2816			mm_slot_free(mm_slot_cache, mm_slot);
  2817			/*
  2818			 * Only clear MMF_VM_MERGEABLE. We must not clear
  2819			 * MMF_VM_MERGE_ANY, because for those MMF_VM_MERGE_ANY process,
  2820			 * perhaps their mm_struct has just been added to ksm_mm_slot
  2821			 * list, and its process has not yet officially started running
  2822			 * or has not yet performed mmap/brk to allocate anonymous VMAS.
  2823			 */
  2824			mm_flags_clear(MMF_VM_MERGEABLE, mm);
  2825			mmap_read_unlock(mm);
  2826			mmdrop(mm);
  2827		} else {
  2828			mmap_read_unlock(mm);
  2829			/*
  2830			 * mmap_read_unlock(mm) first because after
  2831			 * spin_unlock(&ksm_mmlist_lock) run, the "mm" may
  2832			 * already have been freed under us by __ksm_exit()
  2833			 * because the "mm_slot" is still hashed and
  2834			 * ksm_scan.mm_slot doesn't point to it anymore.
  2835			 */
  2836			spin_unlock(&ksm_mmlist_lock);
  2837		}
  2838	
  2839		/* Repeat until we've completed scanning the whole list */
  2840		mm_slot = ksm_scan.mm_slot;
  2841		if (mm_slot != &ksm_mm_head)
  2842			goto next_mm;
  2843	
  2844		advisor_stop_scan();
  2845	
  2846		trace_ksm_stop_scan(ksm_scan.seqnr, ksm_rmap_items);
  2847		ksm_scan.seqnr++;
  2848		return NULL;
  2849	}
  2850	

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki