[PATCH 0/8] mm/khugepaged: collapse_file() cleanups

[PATCH 0/8] mm/khugepaged: collapse_file() cleanups

[Pedro Falcato]

Here are a bunch of cleanups for collapse_file() that attempt to aid
code readability and understanding. This is part 1 out of N, and only
attempts to address the main collapse loop. The exit path is still a
mess but I figure it's much easier to review if I send this batch as-is,
versus sending the whole thing.

Based on top of latest mm-unstable.

Cc: Zi Yan <ziy@nvidia.com>
Cc: Baolin Wang <baolin.wang@linux.alibaba.com>
Cc: "Liam R. Howlett" <liam@infradead.org>
Cc: Nico Pache <npache@redhat.com>
Cc: Ryan Roberts <ryan.roberts@arm.com>
Cc: Dev Jain <dev.jain@arm.com>
Cc: Barry Song <baohua@kernel.org>
Cc: Lance Yang <lance.yang@linux.dev>
Cc: Usama Arif <usama.arif@linux.dev>
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
Pedro Falcato (8):
  mm/khugepaged: separate out windy folio logic from collapse_file
  mm/khugepaged: factor out page cache folio reading
  mm/khugepaged: factor out and simplify dirty/writeback handling
  mm/khugepaged: simplify prepare folio locking and exit paths
  mm/khugepaged: add kerneldoc to prepare_collapse_file_folio()
  mm/khugepaged: hoist isolation into collapse_isolate_folio()
  mm/khugepaged: hoist more code into collapse_isolate_folio()
  mm/khugepaged: fix and flesh out try_to_unmap_flush() comment

 mm/khugepaged.c | 424 ++++++++++++++++++++++++++++--------------------
 1 file changed, 248 insertions(+), 176 deletions(-)

-- 
2.55.0

[PATCH 1/8] mm/khugepaged: separate out windy folio logic from collapse_file

[Pedro Falcato]

Separate out complex folio-related logic from the main collapse_file()
loop, and introduce a new helper struct to help marshal arguments back
and forth.

Signed-off-by: Pedro Falcato <pfalcato@suse.de>
---
 mm/khugepaged.c | 189 ++++++++++++++++++++++++++++--------------------
 1 file changed, 112 insertions(+), 77 deletions(-)

diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index 27e8f3077e80..d4de507ac001 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -2218,6 +2218,92 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
 	i_mmap_unlock_read(mapping);
 }
 
+struct collapse_file_state {
+	/* in-out parameter */
+	struct folio *folio;
+	/* in parameters */
+	struct address_space *mapping;
+	struct file *file;
+	struct xa_state *xas;
+	/* collapse end index */
+	pgoff_t end;
+	unsigned int is_shmem : 1;
+};
+
+static enum scan_result prepare_collapse_file_folio(pgoff_t index, struct collapse_file_state *state)
+{
+	struct address_space *mapping = state->mapping;
+	enum scan_result result = SCAN_SUCCEED;
+	const int is_shmem = state->is_shmem;
+	struct folio *folio = state->folio;
+
+	if (is_shmem) {
+		if (xa_is_value(folio) || !folio_test_uptodate(folio)) {
+			xas_unlock_irq(state->xas);
+			/* swap in or instantiate fallocated page */
+			if (shmem_get_folio(mapping->host, index, 0,
+					&folio, SGP_NOALLOC))
+				result = SCAN_FAIL;
+			/* drain lru cache to help folio_isolate_lru() */
+			lru_add_drain();
+			goto xa_unlocked;
+		} else if (folio_trylock(folio)) {
+			folio_get(folio);
+		} else {
+			result = SCAN_PAGE_LOCK;
+			goto xa_locked;
+		}
+	} else {	/* !is_shmem */
+		if (!folio || xa_is_value(folio)) {
+			xas_unlock_irq(state->xas);
+			page_cache_sync_readahead(mapping, &state->file->f_ra,
+						  state->file, index,
+						  state->end - index);
+			/* drain lru cache to help folio_isolate_lru() */
+			lru_add_drain();
+			folio = filemap_lock_folio(mapping, index);
+			if (IS_ERR(folio))
+				result = SCAN_FAIL;
+			goto xa_unlocked;
+		} else if (folio_test_dirty(folio)) {
+			/*
+			 * This page is dirty because it hasn't
+			 * been flushed since first write.
+			 *
+			 * Trigger async flush for read-only files and
+			 * hope the writeback is done when khugepaged
+			 * revisits this page. Writable files can have
+			 * their folios dirty at any time; blindly
+			 * flushing them would cause undesirable
+			 * system-wide writeback.
+			 *
+			 * This is a one-off situation. We are not
+			 * forcing writeback in loop.
+			 */
+			xas_unlock_irq(state->xas);
+			if (!inode_is_open_for_write(mapping->host))
+				filemap_flush(mapping);
+			result = SCAN_PAGE_DIRTY_OR_WRITEBACK;
+			goto xa_unlocked;
+		} else if (folio_test_writeback(folio)) {
+			xas_unlock_irq(state->xas);
+			result = SCAN_PAGE_DIRTY_OR_WRITEBACK;
+			goto xa_unlocked;
+		} else if (folio_trylock(folio)) {
+			folio_get(folio);
+		} else {
+			result = SCAN_PAGE_LOCK;
+			goto xa_locked;
+		}
+	}
+
+xa_locked:
+	xas_unlock_irq(state->xas);
+xa_unlocked:
+	state->folio = folio;
+	return result;
+}
+
 /**
  * collapse_file - collapse filemap/tmpfs/shmem pages into huge one.
  *
@@ -2255,6 +2341,13 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr,
 	enum scan_result result = SCAN_SUCCEED;
 	int nr_none = 0;
 	bool is_shmem = shmem_file(file);
+	struct collapse_file_state state = {
+		.is_shmem = is_shmem,
+		.xas = &xas,
+		.mapping = mapping,
+		.file = file,
+		.end = end,
+	};
 
 	/*
 	 * MADV_COLLAPSE ignores shmem huge config, so do not check shmem
@@ -2298,87 +2391,29 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr,
 		folio = xas_load(&xas);
 
 		VM_BUG_ON(index != xas.xa_index);
-		if (is_shmem) {
-			if (!folio) {
-				/*
-				 * Stop if extent has been truncated or
-				 * hole-punched, and is now completely
-				 * empty.
-				 */
-				if (index == start) {
-					if (!xas_next_entry(&xas, end - 1)) {
-						result = SCAN_TRUNCATED;
-						goto xa_locked;
-					}
-				}
-				nr_none++;
-				index++;
-				continue;
-			}
-
-			if (xa_is_value(folio) || !folio_test_uptodate(folio)) {
-				xas_unlock_irq(&xas);
-				/* swap in or instantiate fallocated page */
-				if (shmem_get_folio(mapping->host, index, 0,
-						&folio, SGP_NOALLOC)) {
-					result = SCAN_FAIL;
-					goto xa_unlocked;
-				}
-				/* drain lru cache to help folio_isolate_lru() */
-				lru_add_drain();
-			} else if (folio_trylock(folio)) {
-				folio_get(folio);
-				xas_unlock_irq(&xas);
-			} else {
-				result = SCAN_PAGE_LOCK;
-				goto xa_locked;
-			}
-		} else {	/* !is_shmem */
-			if (!folio || xa_is_value(folio)) {
-				xas_unlock_irq(&xas);
-				page_cache_sync_readahead(mapping, &file->f_ra,
-							  file, index,
-							  end - index);
-				/* drain lru cache to help folio_isolate_lru() */
-				lru_add_drain();
-				folio = filemap_lock_folio(mapping, index);
-				if (IS_ERR(folio)) {
-					result = SCAN_FAIL;
-					goto xa_unlocked;
+		if (is_shmem && !folio) {
+			/*
+			 * Stop if extent has been truncated or
+			 * hole-punched, and is now completely
+			 * empty.
+			 */
+			if (index == start) {
+				if (!xas_next_entry(&xas, end - 1)) {
+					result = SCAN_TRUNCATED;
+					goto xa_locked;
 				}
-			} else if (folio_test_dirty(folio)) {
-				/*
-				 * This page is dirty because it hasn't
-				 * been flushed since first write.
-				 *
-				 * Trigger async flush for read-only files and
-				 * hope the writeback is done when khugepaged
-				 * revisits this page. Writable files can have
-				 * their folios dirty at any time; blindly
-				 * flushing them would cause undesirable
-				 * system-wide writeback.
-				 *
-				 * This is a one-off situation. We are not
-				 * forcing writeback in loop.
-				 */
-				xas_unlock_irq(&xas);
-				if (!inode_is_open_for_write(mapping->host))
-					filemap_flush(mapping);
-				result = SCAN_PAGE_DIRTY_OR_WRITEBACK;
-				goto xa_unlocked;
-			} else if (folio_test_writeback(folio)) {
-				xas_unlock_irq(&xas);
-				result = SCAN_PAGE_DIRTY_OR_WRITEBACK;
-				goto xa_unlocked;
-			} else if (folio_trylock(folio)) {
-				folio_get(folio);
-				xas_unlock_irq(&xas);
-			} else {
-				result = SCAN_PAGE_LOCK;
-				goto xa_locked;
 			}
+			nr_none++;
+			index++;
+			continue;
 		}
 
+		/* At this point folio can be NULL, or a value. */
+		state.folio = folio;
+		result = prepare_collapse_file_folio(index, &state);
+		folio = state.folio;
+		if (result != SCAN_SUCCEED)
+			goto xa_unlocked;
 		/*
 		 * The folio must be locked, so we can drop the i_pages lock
 		 * without racing with truncate.
-- 
2.55.0

[PATCH 2/8] mm/khugepaged: factor out page cache folio reading

[Pedro Falcato]

The two mechanisms (shmem, filemap) are quite similar and only change in
subtleties. There is no need to have two variants. Factor this out into
its own separate function, shared by both paths.

Signed-off-by: Pedro Falcato <pfalcato@suse.de>
---
 mm/khugepaged.c | 67 ++++++++++++++++++++++++++++++++-----------------
 1 file changed, 44 insertions(+), 23 deletions(-)

diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index d4de507ac001..4cc6917a55c7 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -2230,42 +2230,63 @@ struct collapse_file_state {
 	unsigned int is_shmem : 1;
 };
 
+static struct folio *collapse_read_folio(pgoff_t index, struct collapse_file_state *state)
+{
+	const bool may_bring_uptodate = state->is_shmem;
+	struct folio *folio = state->folio;
+
+	if (!folio || xa_is_value(folio) ||
+	   (may_bring_uptodate && !folio_test_uptodate(folio))) {
+		xas_unlock_irq(state->xas);
+		if (state->is_shmem) {
+			/* swap in or instantiate fallocated page */
+			if (shmem_get_folio(state->mapping->host, index, 0,
+					    &folio, SGP_NOALLOC))
+				return NULL;
+		} else {
+			page_cache_sync_readahead(state->mapping, &state->file->f_ra,
+						  state->file, index,
+						  state->end - index);
+			folio = filemap_lock_folio(state->mapping, index);
+			if (IS_ERR(folio))
+				return NULL;
+		}
+	}
+
+	return folio;
+}
+
 static enum scan_result prepare_collapse_file_folio(pgoff_t index, struct collapse_file_state *state)
 {
 	struct address_space *mapping = state->mapping;
 	enum scan_result result = SCAN_SUCCEED;
 	const int is_shmem = state->is_shmem;
-	struct folio *folio = state->folio;
+	struct folio *folio;
+
+	folio = collapse_read_folio(index, state);
+	if (!folio)
+		return SCAN_FAIL;
+	if (folio != state->folio) {
+		/*
+		 * collapse_read_folio() got a new folio. This folio is locked
+		 * and ref'd up. Nothing tricky (dirty, writeback, etc) can
+		 * happen, so bail now. But before that, drain the local LRU
+		 * add batch. Otherwise, it's very possible folio_isolate_lru()
+		 * will not succeed.
+		 */
+		lru_add_drain();
+		goto xa_unlocked;
+	}
 
 	if (is_shmem) {
-		if (xa_is_value(folio) || !folio_test_uptodate(folio)) {
-			xas_unlock_irq(state->xas);
-			/* swap in or instantiate fallocated page */
-			if (shmem_get_folio(mapping->host, index, 0,
-					&folio, SGP_NOALLOC))
-				result = SCAN_FAIL;
-			/* drain lru cache to help folio_isolate_lru() */
-			lru_add_drain();
-			goto xa_unlocked;
-		} else if (folio_trylock(folio)) {
+		if (folio_trylock(folio)) {
 			folio_get(folio);
 		} else {
 			result = SCAN_PAGE_LOCK;
 			goto xa_locked;
 		}
 	} else {	/* !is_shmem */
-		if (!folio || xa_is_value(folio)) {
-			xas_unlock_irq(state->xas);
-			page_cache_sync_readahead(mapping, &state->file->f_ra,
-						  state->file, index,
-						  state->end - index);
-			/* drain lru cache to help folio_isolate_lru() */
-			lru_add_drain();
-			folio = filemap_lock_folio(mapping, index);
-			if (IS_ERR(folio))
-				result = SCAN_FAIL;
-			goto xa_unlocked;
-		} else if (folio_test_dirty(folio)) {
+		if (folio_test_dirty(folio)) {
 			/*
 			 * This page is dirty because it hasn't
 			 * been flushed since first write.
-- 
2.55.0

[PATCH 3/8] mm/khugepaged: factor out and simplify dirty/writeback handling

[Pedro Falcato]

Dirty/writeback folios can easily share most of the branch. These checks
are still gated under !is_shmem due to shmem not really having issues
with dirty folios.

While here, reflow the comment.

Signed-off-by: Pedro Falcato <pfalcato@suse.de>
---
 mm/khugepaged.c | 46 ++++++++++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 22 deletions(-)

diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index 4cc6917a55c7..ed5d23317177 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -2262,6 +2262,7 @@ static enum scan_result prepare_collapse_file_folio(pgoff_t index, struct collap
 	enum scan_result result = SCAN_SUCCEED;
 	const int is_shmem = state->is_shmem;
 	struct folio *folio;
+	bool dirty;
 
 	folio = collapse_read_folio(index, state);
 	if (!folio)
@@ -2278,39 +2279,40 @@ static enum scan_result prepare_collapse_file_folio(pgoff_t index, struct collap
 		goto xa_unlocked;
 	}
 
-	if (is_shmem) {
-		if (folio_trylock(folio)) {
-			folio_get(folio);
-		} else {
-			result = SCAN_PAGE_LOCK;
-			goto xa_locked;
-		}
-	} else {	/* !is_shmem */
-		if (folio_test_dirty(folio)) {
+	if (!is_shmem) {
+		dirty = folio_test_dirty(folio);
+		if (dirty || folio_test_writeback(folio)) {
 			/*
-			 * This page is dirty because it hasn't
-			 * been flushed since first write.
+			 * This folio is either dirty or under writeback.
+			 * khugepaged cannot operate on such folios.
 			 *
-			 * Trigger async flush for read-only files and
-			 * hope the writeback is done when khugepaged
-			 * revisits this page. Writable files can have
-			 * their folios dirty at any time; blindly
-			 * flushing them would cause undesirable
+			 * For dirty folios, trigger async flush for
+			 * read-only files and hope the writeback is done
+			 * when khugepaged revisits this page. Writable
+			 * files can have their folios dirty at any time;
+			 * blindly flushing them would cause undesirable
 			 * system-wide writeback.
 			 *
 			 * This is a one-off situation. We are not
 			 * forcing writeback in loop.
 			 */
 			xas_unlock_irq(state->xas);
-			if (!inode_is_open_for_write(mapping->host))
+			if (dirty && !inode_is_open_for_write(mapping->host))
 				filemap_flush(mapping);
 			result = SCAN_PAGE_DIRTY_OR_WRITEBACK;
 			goto xa_unlocked;
-		} else if (folio_test_writeback(folio)) {
-			xas_unlock_irq(state->xas);
-			result = SCAN_PAGE_DIRTY_OR_WRITEBACK;
-			goto xa_unlocked;
-		} else if (folio_trylock(folio)) {
+		}
+	}
+
+	if (is_shmem) {
+		if (folio_trylock(folio)) {
+			folio_get(folio);
+		} else {
+			result = SCAN_PAGE_LOCK;
+			goto xa_locked;
+		}
+	} else {	/* !is_shmem */
+		if (folio_trylock(folio)) {
 			folio_get(folio);
 		} else {
 			result = SCAN_PAGE_LOCK;
-- 
2.55.0

[PATCH 4/8] mm/khugepaged: simplify prepare folio locking and exit paths

[Pedro Falcato]

folio_trylock() + folio_get() do the exact same thing on both is_shmem
and !is_shmem paths. Unify them into one path.

At the end of the day, there are two large invariants for the function:
 - xarray lock is not held at exit. Either it was dropped on error, or
   dropped on success/IO.
 - success always means a locked-and-ref'd-up folio.

After this transformation, it's trivial to notice how simple the error
paths are, so do away with the gotos and hoist some code.

Signed-off-by: Pedro Falcato <pfalcato@suse.de>
---
 mm/khugepaged.c | 36 +++++++++++++-----------------------
 1 file changed, 13 insertions(+), 23 deletions(-)

diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index ed5d23317177..d170b64378e6 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -2259,8 +2259,8 @@ static struct folio *collapse_read_folio(pgoff_t index, struct collapse_file_sta
 static enum scan_result prepare_collapse_file_folio(pgoff_t index, struct collapse_file_state *state)
 {
 	struct address_space *mapping = state->mapping;
-	enum scan_result result = SCAN_SUCCEED;
 	const int is_shmem = state->is_shmem;
+	enum scan_result ret = SCAN_SUCCEED;
 	struct folio *folio;
 	bool dirty;
 
@@ -2276,7 +2276,8 @@ static enum scan_result prepare_collapse_file_folio(pgoff_t index, struct collap
 		 * will not succeed.
 		 */
 		lru_add_drain();
-		goto xa_unlocked;
+		state->folio = folio;
+		return SCAN_SUCCEED;
 	}
 
 	if (!is_shmem) {
@@ -2299,32 +2300,21 @@ static enum scan_result prepare_collapse_file_folio(pgoff_t index, struct collap
 			xas_unlock_irq(state->xas);
 			if (dirty && !inode_is_open_for_write(mapping->host))
 				filemap_flush(mapping);
-			result = SCAN_PAGE_DIRTY_OR_WRITEBACK;
-			goto xa_unlocked;
+			return SCAN_PAGE_DIRTY_OR_WRITEBACK;
 		}
 	}
 
-	if (is_shmem) {
-		if (folio_trylock(folio)) {
-			folio_get(folio);
-		} else {
-			result = SCAN_PAGE_LOCK;
-			goto xa_locked;
-		}
-	} else {	/* !is_shmem */
-		if (folio_trylock(folio)) {
-			folio_get(folio);
-		} else {
-			result = SCAN_PAGE_LOCK;
-			goto xa_locked;
-		}
-	}
 
-xa_locked:
+	/*
+	 * Note: trylock + simple folio_get() is safe here due to the i_pages
+	 * lock being held; this excludes truncation happening in parallel.
+	 */
+	if (!folio_trylock(folio))
+		ret = SCAN_PAGE_LOCK;
+	else
+		folio_get(folio);
 	xas_unlock_irq(state->xas);
-xa_unlocked:
-	state->folio = folio;
-	return result;
+	return ret;
 }
 
 /**
-- 
2.55.0

[PATCH 5/8] mm/khugepaged: add kerneldoc to prepare_collapse_file_folio()

[Pedro Falcato]

The input-output expectations are not entirely clear, thus document it.

Signed-off-by: Pedro Falcato <pfalcato@suse.de>
---
 mm/khugepaged.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index d170b64378e6..db1f765b6ead 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -2256,6 +2256,21 @@ static struct folio *collapse_read_folio(pgoff_t index, struct collapse_file_sta
 	return folio;
 }
 
+
+/**
+ * prepare_collapse_file_folio() - Prepare the collapsing of a single file folio
+ * @index: folio index in the page cache
+ * @state: helper struct of state being passed back and forth.
+ *
+ * prepare_collapse_file_folio() helps prepare the THP collapsing of file folios
+ * (both shmem and !shmem), including the reading of folios, initial tests for
+ * dirty/writeback, locking, etc.
+ *
+ * Context: Expects @state->mapping->i_pages->xa_lock to be held. Releases the same xa_lock.
+ *          On success, returns a refcounted and locked folio in @state->folio (that may
+ *          not be the same that was passed in).
+ * Return: Appropriate scan_result value (on success, SCAN_SUCCEED).
+ */
 static enum scan_result prepare_collapse_file_folio(pgoff_t index, struct collapse_file_state *state)
 {
 	struct address_space *mapping = state->mapping;
-- 
2.55.0

[PATCH 6/8] mm/khugepaged: hoist isolation into collapse_isolate_folio()

[Pedro Falcato]

Signed-off-by: Pedro Falcato <pfalcato@suse.de>
---
 mm/khugepaged.c | 118 +++++++++++++++++++++++++++---------------------
 1 file changed, 66 insertions(+), 52 deletions(-)

diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index db1f765b6ead..429e2c5833d0 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -2332,6 +2332,69 @@ static enum scan_result prepare_collapse_file_folio(pgoff_t index, struct collap
 	return ret;
 }
 
+static enum scan_result collapse_isolate_folio(struct collapse_file_state *state)
+{
+	struct folio *folio = state->folio;
+	enum scan_result result;
+
+	/*
+	 * The folio must be locked, so we can drop the i_pages lock
+	 * without racing with truncate.
+	 */
+	VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
+
+	/* make sure the folio is up to date */
+	if (unlikely(!folio_test_uptodate(folio))) {
+		result = SCAN_FAIL;
+		goto out_unlock;
+	}
+
+	/*
+	 * If file was truncated then extended, or hole-punched, before
+	 * we locked the first folio, then a THP might be there already.
+	 * This will be discovered on the first iteration.
+	 */
+	if (is_pmd_order(folio_order(folio))) {
+		result = SCAN_PTE_MAPPED_HUGEPAGE;
+		goto out_unlock;
+	}
+
+	if (folio_mapping(folio) != state->mapping) {
+		result = SCAN_TRUNCATED;
+		goto out_unlock;
+	}
+
+	if (!state->is_shmem && (folio_test_dirty(folio) ||
+			         folio_test_writeback(folio))) {
+		/*
+		 * khugepaged only works on clean file-backed folios,
+		 * so this folio is dirty because it hasn't been flushed
+		 * since first write.
+		 */
+		result = SCAN_PAGE_DIRTY_OR_WRITEBACK;
+		goto out_unlock;
+	}
+
+	if (!folio_isolate_lru(folio)) {
+		result = SCAN_DEL_PAGE_LRU;
+		goto out_unlock;
+	}
+
+	if (!filemap_release_folio(folio, GFP_KERNEL)) {
+		result = SCAN_PAGE_HAS_PRIVATE;
+		folio_putback_lru(folio);
+		goto out_unlock;
+	}
+
+	if (folio_mapped(folio))
+		try_to_unmap(folio, TTU_IGNORE_MLOCK | TTU_BATCH_FLUSH);
+	return SCAN_SUCCEED;
+out_unlock:
+	folio_unlock(folio);
+	folio_put(folio);
+	return result;
+}
+
 /**
  * collapse_file - collapse filemap/tmpfs/shmem pages into huge one.
  *
@@ -2442,61 +2505,12 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr,
 		folio = state.folio;
 		if (result != SCAN_SUCCEED)
 			goto xa_unlocked;
-		/*
-		 * The folio must be locked, so we can drop the i_pages lock
-		 * without racing with truncate.
-		 */
-		VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
-
-		/* make sure the folio is up to date */
-		if (unlikely(!folio_test_uptodate(folio))) {
-			result = SCAN_FAIL;
-			goto out_unlock;
-		}
-
-		/*
-		 * If file was truncated then extended, or hole-punched, before
-		 * we locked the first folio, then a THP might be there already.
-		 * This will be discovered on the first iteration.
-		 */
-		if (is_pmd_order(folio_order(folio))) {
-			result = SCAN_PTE_MAPPED_HUGEPAGE;
-			goto out_unlock;
-		}
-
-		if (folio_mapping(folio) != mapping) {
-			result = SCAN_TRUNCATED;
-			goto out_unlock;
-		}
-
-		if (!is_shmem && (folio_test_dirty(folio) ||
-				  folio_test_writeback(folio))) {
-			/*
-			 * khugepaged only works on clean file-backed folios,
-			 * so this folio is dirty because it hasn't been flushed
-			 * since first write.
-			 */
-			result = SCAN_PAGE_DIRTY_OR_WRITEBACK;
-			goto out_unlock;
-		}
 
-		if (!folio_isolate_lru(folio)) {
-			result = SCAN_DEL_PAGE_LRU;
-			goto out_unlock;
-		}
-
-		if (!filemap_release_folio(folio, GFP_KERNEL)) {
-			result = SCAN_PAGE_HAS_PRIVATE;
-			folio_putback_lru(folio);
-			goto out_unlock;
-		}
-
-		if (folio_mapped(folio))
-			try_to_unmap(folio,
-					TTU_IGNORE_MLOCK | TTU_BATCH_FLUSH);
+		result = collapse_isolate_folio(&state);
+		if (result != SCAN_SUCCEED)
+			goto xa_unlocked;
 
 		xas_lock_irq(&xas);
-
 		VM_BUG_ON_FOLIO(folio != xa_load(xas.xa, index), folio);
 
 		/*
-- 
2.55.0

[PATCH 7/8] mm/khugepaged: hoist more code into collapse_isolate_folio()

[Pedro Falcato]

Checking expected refcounts or dirtyness has no need for the i_pages
xarray lock.

Signed-off-by: Pedro Falcato <pfalcato@suse.de>
---
 mm/khugepaged.c | 83 ++++++++++++++++++++++---------------------------
 1 file changed, 37 insertions(+), 46 deletions(-)

diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index 429e2c5833d0..a09e4e4e1943 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -2382,13 +2382,48 @@ static enum scan_result collapse_isolate_folio(struct collapse_file_state *state
 
 	if (!filemap_release_folio(folio, GFP_KERNEL)) {
 		result = SCAN_PAGE_HAS_PRIVATE;
-		folio_putback_lru(folio);
-		goto out_unlock;
+		goto out_putback;
 	}
 
 	if (folio_mapped(folio))
 		try_to_unmap(folio, TTU_IGNORE_MLOCK | TTU_BATCH_FLUSH);
+	/*
+	 * We control 2 + nr_pages references to the folio:
+	 *  - we hold a pin on it;
+	 *  - nr_pages reference from page cache;
+	 *  - one from lru_isolate_folio;
+	 * If those are the only references, then any new usage
+	 * of the folio will have to fetch it from the page
+	 * cache. That requires locking the folio to handle
+	 * truncate, so any new usage will be blocked until we
+	 * unlock folio after collapse/during rollback.
+	 */
+	if (folio_ref_count(folio) != 2 + folio_nr_pages(folio)) {
+		result = SCAN_PAGE_COUNT;
+		goto out_putback;
+	}
+
+	/*
+	 * At this point, the folio is locked and unmapped. If the PTE
+	 * was dirty, try_to_unmap() has transferred the dirty bit to
+	 * the folio and we must not collapse it into a clean
+	 * file-backed folio.
+	 *
+	 * If the folio is clean here, no one can write it until we
+	 * drop the folio lock. A write through a stale TLB entry came
+	 * from a clean PTE and must fault because the PTE has been
+	 * cleared; the fault path has to take the folio lock before
+	 * installing a writable mapping. Buffered write paths also
+	 * have to take the folio lock before modifying file contents
+	 * without a mapping, typically via write_begin_get_folio().
+	 */
+	if (!state->is_shmem && folio_test_dirty(folio)) {
+		result = SCAN_PAGE_DIRTY_OR_WRITEBACK;
+		goto out_putback;
+	}
 	return SCAN_SUCCEED;
+out_putback:
+	folio_putback_lru(folio);
 out_unlock:
 	folio_unlock(folio);
 	folio_put(folio);
@@ -2513,55 +2548,11 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr,
 		xas_lock_irq(&xas);
 		VM_BUG_ON_FOLIO(folio != xa_load(xas.xa, index), folio);
 
-		/*
-		 * We control 2 + nr_pages references to the folio:
-		 *  - we hold a pin on it;
-		 *  - nr_pages reference from page cache;
-		 *  - one from lru_isolate_folio;
-		 * If those are the only references, then any new usage
-		 * of the folio will have to fetch it from the page
-		 * cache. That requires locking the folio to handle
-		 * truncate, so any new usage will be blocked until we
-		 * unlock folio after collapse/during rollback.
-		 */
-		if (folio_ref_count(folio) != 2 + folio_nr_pages(folio)) {
-			result = SCAN_PAGE_COUNT;
-			xas_unlock_irq(&xas);
-			folio_putback_lru(folio);
-			goto out_unlock;
-		}
-
-		/*
-		 * At this point, the folio is locked and unmapped. If the PTE
-		 * was dirty, try_to_unmap() has transferred the dirty bit to
-		 * the folio and we must not collapse it into a clean
-		 * file-backed folio.
-		 *
-		 * If the folio is clean here, no one can write it until we
-		 * drop the folio lock. A write through a stale TLB entry came
-		 * from a clean PTE and must fault because the PTE has been
-		 * cleared; the fault path has to take the folio lock before
-		 * installing a writable mapping. Buffered write paths also
-		 * have to take the folio lock before modifying file contents
-		 * without a mapping, typically via write_begin_get_folio().
-		 */
-		if (!is_shmem && folio_test_dirty(folio)) {
-			result = SCAN_PAGE_DIRTY_OR_WRITEBACK;
-			xas_unlock_irq(&xas);
-			folio_putback_lru(folio);
-			goto out_unlock;
-		}
-
 		/*
 		 * Accumulate the folios that are being collapsed.
 		 */
 		list_add_tail(&folio->lru, &pagelist);
 		index += folio_nr_pages(folio);
-		continue;
-out_unlock:
-		folio_unlock(folio);
-		folio_put(folio);
-		goto xa_unlocked;
 	}
 
 xa_locked:
-- 
2.55.0

[PATCH 8/8] mm/khugepaged: fix and flesh out try_to_unmap_flush() comment

[Pedro Falcato]

Instead of vaguely handwaving that it should probably be done, flesh out
any correctness concerns with try_to_unmap batching, flushing and its
interaction with the rest of collapse_file().

Signed-off-by: Pedro Falcato <pfalcato@suse.de>
---
 mm/khugepaged.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index a09e4e4e1943..1999e094df20 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -2560,15 +2560,19 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr,
 xa_unlocked:
 
 	/*
-	 * If collapse is successful, flush must be done now before copying.
-	 * If collapse is unsuccessful, does flush actually need to be done?
-	 * Do it anyway, to clear the state.
+	 * try_to_unmap() flush must be done now before copying, regardless
+	 * of success or not. In case of success, folios are about to be
+	 * copied and collapsed onto a single large folio. In that case,
+	 * stale TLB entries need to be flushed out, so no racing write may
+	 * get lost. In case of failure, stale TLB entries need to be flushed
+	 * out before putting the folio (which can possibly free it).
 	 */
 	try_to_unmap_flush();
 
 	if (result == SCAN_SUCCEED && nr_none &&
 	    !shmem_charge(mapping->host, nr_none))
 		result = SCAN_FAIL;
+
 	if (result != SCAN_SUCCEED) {
 		nr_none = 0;
 		goto rollback;
-- 
2.55.0

Re: [PATCH 0/8] mm/khugepaged: collapse_file() cleanups

[Nico Pache]

On Mon, Jul 20, 2026 at 8:33 AM Pedro Falcato <pfalcato@suse.de> wrote:
>
> Here are a bunch of cleanups for collapse_file() that attempt to aid
> code readability and understanding. This is part 1 out of N, and only
> attempts to address the main collapse loop. The exit path is still a
> mess but I figure it's much easier to review if I send this batch as-is,
> versus sending the whole thing.

Hi Pedro,

I have already prepped a cleanup series for collapse_file().

I was waiting for my work here
https://lore.kernel.org/lkml/20260715025941.1571316-1-npache@redhat.com/
to be reviewed (and in mm-unstable) before sending it out.

Our series have several similarities; for reference, here is my version.
https://gitlab.com/npache/linux/-/commits/khugepaged_file_refactor?ref_type=heads

Cheers,
-- Nico

>
> Based on top of latest mm-unstable.
>
> Cc: Zi Yan <ziy@nvidia.com>
> Cc: Baolin Wang <baolin.wang@linux.alibaba.com>
> Cc: "Liam R. Howlett" <liam@infradead.org>
> Cc: Nico Pache <npache@redhat.com>
> Cc: Ryan Roberts <ryan.roberts@arm.com>
> Cc: Dev Jain <dev.jain@arm.com>
> Cc: Barry Song <baohua@kernel.org>
> Cc: Lance Yang <lance.yang@linux.dev>
> Cc: Usama Arif <usama.arif@linux.dev>
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> Pedro Falcato (8):
>   mm/khugepaged: separate out windy folio logic from collapse_file
>   mm/khugepaged: factor out page cache folio reading
>   mm/khugepaged: factor out and simplify dirty/writeback handling
>   mm/khugepaged: simplify prepare folio locking and exit paths
>   mm/khugepaged: add kerneldoc to prepare_collapse_file_folio()
>   mm/khugepaged: hoist isolation into collapse_isolate_folio()
>   mm/khugepaged: hoist more code into collapse_isolate_folio()
>   mm/khugepaged: fix and flesh out try_to_unmap_flush() comment
>
>  mm/khugepaged.c | 424 ++++++++++++++++++++++++++++--------------------
>  1 file changed, 248 insertions(+), 176 deletions(-)
>
> --
> 2.55.0
>

Re: [PATCH 1/8] mm/khugepaged: separate out windy folio logic from collapse_file

[Lorenzo Stoakes (ARM)]

On Mon, 20 Jul 2026 15:29:06 +0100, Pedro Falcato <pfalcato@suse.de> wrote:
> Separate out complex folio-related logic from the main collapse_file()
> loop, and introduce a new helper struct to help marshal arguments back
> and forth.
> 
> Signed-off-by: Pedro Falcato <pfalcato@suse.de>

THANK YOU for doing this :) even looking into this bit of code is horrifying so
I can see you're putting yourself through it to make these changes... :)

I checked carefully and the logic looks to be identical, just various nits
etc. below.

>
>
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 27e8f3077e80..d4de507ac001 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -2218,6 +2218,92 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
>  	i_mmap_unlock_read(mapping);
>  }
>  
> +struct collapse_file_state {

You had me at helper struct ;)

> +	/* in-out parameter */

I think we can drop this comment, though hm it is useful to know this is updated
and the 'product' of the operation.

Maybe something like:

struct collapse_file_state {
	/* Inputs */
	struct file *file;
	struct address_space *mapping;
	struct xa_state *xas;
	const pgoff_t pgoff_start, pgoff_end;

	struct folio *folio;
};

Renaming index -> pgoff_start and end -> pgoff_end and keeping them both in the
struct rather than passing the index as a separate parameter implies the folio
is separate?

(Though any such renames should be a separate patch...)

> +	struct folio *folio;
> +	/* in parameters */
> +	struct address_space *mapping;
> +	struct file *file;
> +	struct xa_state *xas;

Can we just make these input fields const + have the code tell you they're input
only? Though perhaps callers expect non-const (esp. the pointers, ugh.

> +	/* collapse end index */

This feels a little superfluous? Feels odd not to have a start too but I guess
not necessary.

> +	pgoff_t end;
> +	unsigned int is_shmem : 1;
> +};
> +
> +static enum scan_result prepare_collapse_file_folio(pgoff_t index, struct collapse_file_state *state)

NIT - this is 101 chars :)

Either:

static enum scan_result
prepare_collapse_file_folio(pgoff_t index, struct collapse_file_state *state)

OR:

static enum scan_result prepare_collapse_file_folio(pgoff_t index,
		struct collapse_file_state *state)

Resolves this :)

Also - why are we passing index and not just putting that in the helper struct?

> [ ... skip 54 lines ... ]
> +		} else if (folio_test_writeback(folio)) {
> +			xas_unlock_irq(state->xas);
> +			result = SCAN_PAGE_DIRTY_OR_WRITEBACK;
> +			goto xa_unlocked;
> +		} else if (folio_trylock(folio)) {
> +			folio_get(folio);

I see you drop the xas_unlock_irq(&xas) here because xa_locked has you covered
below, slight inconsistency in terms of the mechnical change but I think
unavoidable.

> +		} else {
> +			result = SCAN_PAGE_LOCK;
> +			goto xa_locked;

I wonder if we could do away with this goto? That makes it match the
removed code less but you are already having to do that as above.

> +		}
> +	}
> +
> +xa_locked:
> +	xas_unlock_irq(state->xas);
> +xa_unlocked:
> +	state->folio = folio;
> +	return result;
> +}

I'm guessing this will get some more cleanup in subsequent patches? As a little
long as-is.

But obviously makes sense to separate out into broadly mechanical moves first.

> @@ -2298,87 +2391,29 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr,
> [ ... skip 95 lines ... ]
>  		}
>  
> +		/* At this point folio can be NULL, or a value. */
> +		state.folio = folio;
> +		result = prepare_collapse_file_folio(index, &state);
> +		folio = state.folio;

This is a little ugly, but I think probably necessarily at this point in the
operation!

> +		if (result != SCAN_SUCCEED)
> +			goto xa_unlocked;
>  		/*
>  		 * The folio must be locked, so we can drop the i_pages lock
>  		 * without racing with truncate.

I looked through the rest of the logic and it all seems to be correct!

Cheers, Lorenzo

NIT: This is 101 chars,

NIT: This is 101 chars,

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

Re: [PATCH 1/8] mm/khugepaged: separate out windy folio logic from collapse_file

[Lorenzo Stoakes (ARM)]

On Mon, Jul 20, 2026 at 04:38:55PM +0100, Lorenzo Stoakes (ARM) wrote:
> NIT: This is 101 chars,
>
> NIT: This is 101 chars,

(Please forgive this, b4 review converted my nits into tags and I couldn't undo
it so just sent it :P)

Cheers, Lorenzo

Re: [PATCH 2/8] mm/khugepaged: factor out page cache folio reading

[Lorenzo Stoakes (ARM)]

On Mon, 20 Jul 2026 15:29:07 +0100, Pedro Falcato <pfalcato@suse.de> wrote:
> The two mechanisms (shmem, filemap) are quite similar and only change in
> subtleties. There is no need to have two variants. Factor this out into
> its own separate function, shared by both paths.

I think this commit message could have some more detail, it's difficult to
figure out what's new, what's moved, etc. so mentioning that and the broader
purpose of why you're doing this (iteratively improving file collapse in
khugepaged) would be nice.

Also there are multiple subtleties in this patch as noted below, none of which
you've described here.

You should describe every bit of logic you're changing compared to the original
and why that's OK.

Or ideally really, separate out the mechnical changes from the logical ones.

> 
> Signed-off-by: Pedro Falcato <pfalcato@suse.de>

It's a little tricky to figure out what's moved, what's new, etc. don't be
afraid to break out into even more commits to help reviewers :)

>
>
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index d4de507ac001..4cc6917a55c7 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -2230,42 +2230,63 @@ struct collapse_file_state {
>  	unsigned int is_shmem : 1;
>  };
>  

It'd be nice to add some comments to these bits!

I noticed that you add them later in the series but it'd be good to add them
earlier and build them out as you go.

> +static struct folio *collapse_read_folio(pgoff_t index, struct collapse_file_state *state)

90 chars here be nice to try to stick to ~80 :) simliar comment to last patch.

Also the name seems to imply you're _always_ reading which isn't the case.

Not sure we need the collapse_ either. Something like 'maybe_read_folio()' with
the state implying what it's used for?

> +{
> +	const bool may_bring_uptodate = state->is_shmem;

It's nice to make this self-documenting, but we could make life easier for
ourselves and avoid the (apparent) inconsistentcy of using this and
state->is_shmem separately by just doing the full check here:

	const bool bring_uptodate = folio && state->is_shmem &&
				    !folio_test_uptodate(folio);

(putting the folio decl before it ofc)

> +	struct folio *folio = state->folio;
> +
> +	if (!folio || xa_is_value(folio) ||
> +	   (may_bring_uptodate && !folio_test_uptodate(folio))) {

This X or Y or (A and B) is confusing, separating out bring_uptodate makes it
easier.

Can make this a guard clause like:

	/* No reason to read the folio. */
	if (folio && !xa_is_value(folio) && !bring_uptodate)
		return folio;

Which becomes its own form of self-documentation.

But I'm concerned whether this is correct.

For the shmem case you're going from (xa_is_value(folio) ||
!folio_test_uptodate(folio)) to (!folio || xa_is_value(folio) ||
!folio_test_uptodate(folio).

Again you do need to justify these changes somewhere.

> +		xas_unlock_irq(state->xas);

Be nice to have a comment as to why we're unlocking this.

> +		if (state->is_shmem) {
> +			/* swap in or instantiate fallocated page */

I know it was in the original code but this comment feels superfluous.

> +			if (shmem_get_folio(state->mapping->host, index, 0,
> +					    &folio, SGP_NOALLOC))
> +				return NULL;

Looking through the logic I see that this returns NULL which gets checked first
below and results in SCAN_FAIL, but it's not obvious when first reading the
patch :)

Argues for more info in commit msg + separation of patches.

> [ ... skip 16 lines ... ]
>  	enum scan_result result = SCAN_SUCCEED;
>  	const int is_shmem = state->is_shmem;
> -	struct folio *folio = state->folio;
> +	struct folio *folio;
> +
> +	folio = collapse_read_folio(index, state);

Again would be nice to have index in state :)

> +	if (!folio)
> +		return SCAN_FAIL;

Previously we always did the LRU drain regardless of whether shmem_get_folio()
or page_cache_sync_readahead() failed or not, now you're not doing that.

If an intentional change, you should explain why.

It'd also be nice to separate out mechnical moves from logic changes, as it's
hard to keep track of the new stuff you're doing vs. the stuff you're just moving around.

> +	if (folio != state->folio) {

Is this always the case when the operation succeeds?

You're going from always doing LRU drain and exit early to - only if folio !=
state->folio which is a logic change AFAICT.

> +		/*
> +		 * collapse_read_folio() got a new folio. This folio is locked
> +		 * and ref'd up. Nothing tricky (dirty, writeback, etc) can
> +		 * happen, so bail now. But before that, drain the local LRU
> +		 * add batch. Otherwise, it's very possible folio_isolate_lru()
> +		 * will not succeed.

Hmm :)

I feel we could collapse this into something more succinct like:

	/*
	 * We read a new folio which is now locked and pinned. Drain so
         * folio_isolate_lru() sees it and bail.
	 */

> +		 */
> +		lru_add_drain();

This is now out of order compared to the code it replaces for the !is_shmem
case. Is that OK?

Another thing that'd be nice to cover in the commit msg, it does need to be
justified.

> [ ... skip 31 lines ... ]
> -			goto xa_unlocked;
> -		} else if (folio_test_dirty(folio)) {
> +		if (folio_test_dirty(folio)) {
>  			/*
>  			 * This page is dirty because it hasn't
>  			 * been flushed since first write.

Th resulting code in prepare_collapse_file_folio() at this point is pretty
horrible, I realise it might be an interim state but I'm sure it could be done
better, for instance separating out the is_shmem vs. !is_shmem logic into helper
functions etc.

Let's definitely try to split this up into more patches so it's easier to follow
on respin please!

Thanks, Lorenzo

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

Re: [PATCH 0/8] mm/khugepaged: collapse_file() cleanups

[Pedro Falcato]

On Mon, Jul 20, 2026 at 09:21:11AM -0600, Nico Pache wrote:
> On Mon, Jul 20, 2026 at 8:33 AM Pedro Falcato <pfalcato@suse.de> wrote:
> >
> > Here are a bunch of cleanups for collapse_file() that attempt to aid
> > code readability and understanding. This is part 1 out of N, and only
> > attempts to address the main collapse loop. The exit path is still a
> > mess but I figure it's much easier to review if I send this batch as-is,
> > versus sending the whole thing.
> 
> Hi Pedro,
> 
> I have already prepped a cleanup series for collapse_file().
> 
> I was waiting for my work here
> https://lore.kernel.org/lkml/20260715025941.1571316-1-npache@redhat.com/
> to be reviewed (and in mm-unstable) before sending it out.
> 
> Our series have several similarities; for reference, here is my version.
> https://gitlab.com/npache/linux/-/commits/khugepaged_file_refactor?ref_type=heads

Ugh, this is awkward. Do you have any suggestions on next steps?

I can try and take in some of your work on top of this series. But that might
be hairy.

This situation really does suck (I've definitely been there), but I would
prefer not to drop the patchset.

-- 
Pedro

Re: [PATCH 0/8] mm/khugepaged: collapse_file() cleanups

[Nico Pache]

On Mon, Jul 20, 2026 at 1:49 PM Pedro Falcato <pfalcato@suse.de> wrote:
>
> On Mon, Jul 20, 2026 at 09:21:11AM -0600, Nico Pache wrote:
> > On Mon, Jul 20, 2026 at 8:33 AM Pedro Falcato <pfalcato@suse.de> wrote:
> > >
> > > Here are a bunch of cleanups for collapse_file() that attempt to aid
> > > code readability and understanding. This is part 1 out of N, and only
> > > attempts to address the main collapse loop. The exit path is still a
> > > mess but I figure it's much easier to review if I send this batch as-is,
> > > versus sending the whole thing.
> >
> > Hi Pedro,
> >
> > I have already prepped a cleanup series for collapse_file().
> >
> > I was waiting for my work here
> > https://lore.kernel.org/lkml/20260715025941.1571316-1-npache@redhat.com/
> > to be reviewed (and in mm-unstable) before sending it out.
> >
> > Our series have several similarities; for reference, here is my version.
> > https://gitlab.com/npache/linux/-/commits/khugepaged_file_refactor?ref_type=heads
>
> Ugh, this is awkward. Do you have any suggestions on next steps?

Let me review your series and see what differences stand out. You
referenced part 1 of N; I think my series covers many of the other
parts. I basically broke down each stage of collapse_file() into
helpers, and cleaned up each part respectively.

Whatever we decide, as long as collapse_file() gets the proper love im
ok with it. I did, however, communicate that I was cleaning this up to
David, Lorenzo, and others.

>
> I can try and take in some of your work on top of this series. But that might
> be hairy.
>
> This situation really does suck (I've definitely been there), but I would
> prefer not to drop the patchset.

Yeah that might be hard. However, there are other parts I cleaned up
that you haven't touched in this series so perhaps some of those apply
cleanly.

Cheers,
-- Nico

>
> --
> Pedro
>

Re: [PATCH 0/8] mm/khugepaged: collapse_file() cleanups

[Lorenzo Stoakes (ARM)]

On Mon, Jul 20, 2026 at 02:14:45PM -0600, Nico Pache wrote:
> On Mon, Jul 20, 2026 at 1:49 PM Pedro Falcato <pfalcato@suse.de> wrote:
> >
> > On Mon, Jul 20, 2026 at 09:21:11AM -0600, Nico Pache wrote:
> > > On Mon, Jul 20, 2026 at 8:33 AM Pedro Falcato <pfalcato@suse.de> wrote:
> > > >
> > > > Here are a bunch of cleanups for collapse_file() that attempt to aid
> > > > code readability and understanding. This is part 1 out of N, and only
> > > > attempts to address the main collapse loop. The exit path is still a
> > > > mess but I figure it's much easier to review if I send this batch as-is,
> > > > versus sending the whole thing.
> > >
> > > Hi Pedro,
> > >
> > > I have already prepped a cleanup series for collapse_file().
> > >
> > > I was waiting for my work here
> > > https://lore.kernel.org/lkml/20260715025941.1571316-1-npache@redhat.com/
> > > to be reviewed (and in mm-unstable) before sending it out.
> > >
> > > Our series have several similarities; for reference, here is my version.
> > > https://gitlab.com/npache/linux/-/commits/khugepaged_file_refactor?ref_type=heads
> >
> > Ugh, this is awkward. Do you have any suggestions on next steps?
>
> Let me review your series and see what differences stand out. You
> referenced part 1 of N; I think my series covers many of the other
> parts. I basically broke down each stage of collapse_file() into
> helpers, and cleaned up each part respectively.
>
> Whatever we decide, as long as collapse_file() gets the proper love im
> ok with it. I did, however, communicate that I was cleaning this up to
> David, Lorenzo, and others.

Yeah I'm keen that we find a good compromise here, as I genuinely very much
empathise with both sides of this - being 'pipped' on a series is horrible and
we generally really want to avoid that.

But I hope that some collaborative effort is possible here?

>
> >
> > I can try and take in some of your work on top of this series. But that might
> > be hairy.
> >
> > This situation really does suck (I've definitely been there), but I would
> > prefer not to drop the patchset.
>
> Yeah that might be hard. However, there are other parts I cleaned up
> that you haven't touched in this series so perhaps some of those apply
> cleanly.

It'd be ideal if you guys could figure out a way to combine both in a series,
perhaps with Co-Dev-By or similar?

>
> Cheers,
> -- Nico
>
> >
> > --
> > Pedro
> >
>

I'll leave it to you two to figure things out, I've left some review here
already (the general thrust of the series is good and cleaning
this... 'organically grown' code is a wonderful thing), but will pause until you
figure out a way forwards.

Cheers, Lorenzo

Re: [PATCH 0/8] mm/khugepaged: collapse_file() cleanups

[Nico Pache]

On Tue, Jul 21, 2026 at 1:55 AM Lorenzo Stoakes (ARM) <ljs@kernel.org> wrote:
>
> On Mon, Jul 20, 2026 at 02:14:45PM -0600, Nico Pache wrote:
> > On Mon, Jul 20, 2026 at 1:49 PM Pedro Falcato <pfalcato@suse.de> wrote:
> > >
> > > On Mon, Jul 20, 2026 at 09:21:11AM -0600, Nico Pache wrote:
> > > > On Mon, Jul 20, 2026 at 8:33 AM Pedro Falcato <pfalcato@suse.de> wrote:
> > > > >
> > > > > Here are a bunch of cleanups for collapse_file() that attempt to aid
> > > > > code readability and understanding. This is part 1 out of N, and only
> > > > > attempts to address the main collapse loop. The exit path is still a
> > > > > mess but I figure it's much easier to review if I send this batch as-is,
> > > > > versus sending the whole thing.
> > > >
> > > > Hi Pedro,
> > > >
> > > > I have already prepped a cleanup series for collapse_file().
> > > >
> > > > I was waiting for my work here
> > > > https://lore.kernel.org/lkml/20260715025941.1571316-1-npache@redhat.com/
> > > > to be reviewed (and in mm-unstable) before sending it out.
> > > >
> > > > Our series have several similarities; for reference, here is my version.
> > > > https://gitlab.com/npache/linux/-/commits/khugepaged_file_refactor?ref_type=heads
> > >
> > > Ugh, this is awkward. Do you have any suggestions on next steps?
> >
> > Let me review your series and see what differences stand out. You
> > referenced part 1 of N; I think my series covers many of the other
> > parts. I basically broke down each stage of collapse_file() into
> > helpers, and cleaned up each part respectively.
> >
> > Whatever we decide, as long as collapse_file() gets the proper love im
> > ok with it. I did, however, communicate that I was cleaning this up to
> > David, Lorenzo, and others.
>
> Yeah I'm keen that we find a good compromise here, as I genuinely very much
> empathise with both sides of this - being 'pipped' on a series is horrible and
> we generally really want to avoid that.
>
> But I hope that some collaborative effort is possible here?

Yes definitely! Pedro's series covers only part of what my work
addresses. I think it will be pretty easy to take his work, and my
commits for the rest of the collapse_file() function.

>
> >
> > >
> > > I can try and take in some of your work on top of this series. But that might
> > > be hairy.
> > >
> > > This situation really does suck (I've definitely been there), but I would
> > > prefer not to drop the patchset.
> >
> > Yeah that might be hard. However, there are other parts I cleaned up
> > that you haven't touched in this series so perhaps some of those apply
> > cleanly.
>
> It'd be ideal if you guys could figure out a way to combine both in a series,
> perhaps with Co-Dev-By or similar?

I've reached out to him to coordinate this effort :)

Thanks,
-- Nico

>
> >
> > Cheers,
> > -- Nico
> >
> > >
> > > --
> > > Pedro
> > >
> >
>
> I'll leave it to you two to figure things out, I've left some review here
> already (the general thrust of the series is good and cleaning
> this... 'organically grown' code is a wonderful thing), but will pause until you
> figure out a way forwards.
>
> Cheers, Lorenzo
>

[syzbot ci] Re: mm/khugepaged: collapse_file() cleanups

[syzbot ci]

syzbot ci has tested the following series

[v1] mm/khugepaged: collapse_file() cleanups
https://lore.kernel.org/all/20260720142913.846902-1-pfalcato@suse.de
* [PATCH 1/8] mm/khugepaged: separate out windy folio logic from collapse_file
* [PATCH 2/8] mm/khugepaged: factor out page cache folio reading
* [PATCH 3/8] mm/khugepaged: factor out and simplify dirty/writeback handling
* [PATCH 4/8] mm/khugepaged: simplify prepare folio locking and exit paths
* [PATCH 5/8] mm/khugepaged: add kerneldoc to prepare_collapse_file_folio()
* [PATCH 6/8] mm/khugepaged: hoist isolation into collapse_isolate_folio()
* [PATCH 7/8] mm/khugepaged: hoist more code into collapse_isolate_folio()
* [PATCH 8/8] mm/khugepaged: fix and flesh out try_to_unmap_flush() comment

and found the following issue:
WARNING: bad unlock balance in collapse_scan_file

Full report is available here:
https://ci.syzbot.org/series/cc30d9d4-857a-45de-a8e8-b99f9346d0da

***

WARNING: bad unlock balance in collapse_scan_file

tree:      mm-new
URL:       https://kernel.googlesource.com/pub/scm/linux/kernel/git/akpm/mm.git
base:      c872b70f5d6c742ad34b8e838c92af81c8920b3e
arch:      amd64
compiler:  Debian clang version 22.1.8 (++20260613092233+e80beda6e255-1~exp1~20260613092250.77), Debian LLD 22.1.8
config:    https://ci.syzbot.org/builds/e6e237b7-5a64-4732-bb37-4e62c3103d4a/config
syz repro: https://ci.syzbot.org/findings/51ee94c3-7fe6-47f6-81ed-a53cf51bb3f9/syz_repro

=====================================
WARNING: bad unlock balance detected!
syzkaller #0 Not tainted
-------------------------------------
syz.0.17/5868 is trying to release lock (&xa->xa_lock) at:
[<ffffffff823ac70f>] spin_unlock_irq include/linux/spinlock.h:402 [inline]
[<ffffffff823ac70f>] prepare_collapse_file_folio mm/khugepaged.c:2331 [inline]
[<ffffffff823ac70f>] collapse_file mm/khugepaged.c:2539 [inline]
[<ffffffff823ac70f>] collapse_scan_file+0x2cbf/0x4dc0 mm/khugepaged.c:2849
but there are no more locks to release!

other info that might help us debug this:
no locks held by syz.0.17/5868.

stack backtrace:
CPU: 0 UID: 0 PID: 5868 Comm: syz.0.17 Not tainted syzkaller #0 PREEMPT(full) 
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
Call Trace:
 <TASK>
 dump_stack_lvl+0xe8/0x150 lib/dump_stack.c:120
 print_unlock_imbalance_bug+0xdc/0xf0 kernel/locking/lockdep.c:5299
 __lock_release kernel/locking/lockdep.c:5528 [inline]
 lock_release+0x1fb/0x3c0 kernel/locking/lockdep.c:5912
 __raw_spin_unlock_irq include/linux/spinlock_api_smp.h:185 [inline]
 _raw_spin_unlock_irq+0x16/0x50 kernel/locking/spinlock.c:206
 spin_unlock_irq include/linux/spinlock.h:402 [inline]
 prepare_collapse_file_folio mm/khugepaged.c:2331 [inline]
 collapse_file mm/khugepaged.c:2539 [inline]
 collapse_scan_file+0x2cbf/0x4dc0 mm/khugepaged.c:2849
 collapse_single_pmd+0x2b1/0x3da0 mm/khugepaged.c:2884
 madvise_collapse+0x2cf/0x790 mm/khugepaged.c:3310
 madvise_vma_behavior+0x115f/0x4170 mm/madvise.c:1363
 madvise_walk_vmas+0x576/0xb00 mm/madvise.c:1712
 madvise_do_behavior+0x385/0x540 mm/madvise.c:1907
 do_madvise+0x327/0x3a0 mm/madvise.c:2005
 __do_sys_madvise mm/madvise.c:2014 [inline]
 __se_sys_madvise mm/madvise.c:2012 [inline]
 __x64_sys_madvise+0xa6/0xc0 mm/madvise.c:2012
 do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
 do_syscall_64+0x174/0x580 arch/x86/entry/syscall_64.c:94
 entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7fb60079de99
Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 e8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007fb6015c0028 EFLAGS: 00000246 ORIG_RAX: 000000000000001c
RAX: ffffffffffffffda RBX: 00007fb600a25fa0 RCX: 00007fb60079de99
RDX: 0000000000000019 RSI: 0000000000600003 RDI: 0000200000000000
RBP: 00007fb600833eaf R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
R13: 00007fb600a26038 R14: 00007fb600a25fa0 R15: 00007ffc7db2f618
 </TASK>


***

If these findings have caused you to resend the series or submit a
separate fix, please add the following tag to your commit message:
  Tested-by: syzbot@syzkaller.appspotmail.com

---
This report is generated by a bot. It may contain errors.
syzbot ci engineers can be reached at syzkaller@googlegroups.com.

To test a patch for this bug, please reply with `#syz test`
(should be on a separate line).

The patch should be attached to the email.
Note: arguments like custom git repos and branches are not supported.