Skip to contentSkip to conversation

mm/khugepaged: collapse_file() cleanups

PATCH Superseded Pedro Falcato 5 participants 22 messages Jul 20 – Jul 22

Subsystems (2)
Memory management THP

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Pedro Falcato

author
#0 lore raw

cover mm/khugepaged: collapse_file() cleanups

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 <[email protected]>
Cc: Baolin Wang <[email protected]>
Cc: "Liam R. Howlett" <[email protected]>
Cc: Nico Pache <[email protected]>
Cc: Ryan Roberts <[email protected]>
Cc: Dev Jain <[email protected]>
Cc: Barry Song <[email protected]>
Cc: Lance Yang <[email protected]>
Cc: Usama Arif <[email protected]>
Cc: [email protected]
Cc: [email protected]
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(-)
Signature · 1 line
2.55.0
author
#1 lore raw

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

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 <[email protected]>
---
 mm/khugepaged.c | 189 ++++++++++++++++++++++++++++--------------------
 1 file changed, 112 insertions(+), 77 deletions(-)
mm/khugepaged.c +112 -77
index 27e8f3077e80..d4de507ac001 100644
@@ -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.
Signature · 1 line
2.55.0
#2 lore raw

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

Quoted 5 lines · 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 <[email protected]>

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.

mm/khugepaged.c static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff) · line 2221

Quoted 11 lines
mm/khugepaged.c +1 -0
index 27e8f3077e80..d4de507ac001 100644
@@ -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 ;)

mm/khugepaged.c static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff) · line 2222

Quoted 1 line
mm/khugepaged.c +1 -0
+	/* 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...)

mm/khugepaged.c static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff) · line 2227

Quoted 5 lines
mm/khugepaged.c +5 -0
+	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.

mm/khugepaged.c static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff) · line 2228

Quoted 1 line
mm/khugepaged.c +1 -0
+	/* collapse end index */

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

mm/khugepaged.c static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff) · line 2233

Quoted 5 lines
mm/khugepaged.c +5 -0
+	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?

mm/khugepaged.c static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff) · line 2293

Quoted 7 lines

[ ... skip 54 lines ... ]

mm/khugepaged.c +6 -0
+		} 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.

mm/khugepaged.c static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff) · line 2254

Quoted 3 lines
mm/khugepaged.c +3 -0
+		} 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.

mm/khugepaged.c static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff) · line 2305

Quoted 9 lines
mm/khugepaged.c +9 -0
+		}
+	}
+
+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.

mm/khugepaged.c static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, · line 2414

Quoted 8 lines
mm/khugepaged.c +0 -0
@@ -2298,87 +2391,29 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr,

[ ... skip 95 lines ... ]

mm/khugepaged.c +4 -0
 		}
 
+		/* 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!

mm/khugepaged.c static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, · line 2419

Quoted 5 lines
mm/khugepaged.c +2 -0
+		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,

Signature · 1 line
Lorenzo Stoakes (ARM) <[email protected]>
#3 lore raw

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

Quoted 3 lines · Lorenzo Stoakes (ARM)

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

reviewer
#4 lore raw

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

In reply to Pedro Falcato
Quoted 3 lines

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.

Hey! Since we both have a similar patch, I will note our differences in approach here, and then comment inline for your changes.

Overall I think your series is a good v1, and tackles the most complex patch in my version. There are a few things your patch does that mine does not, and vise-versa.

You seperate all this code into a prepare + isolate stage, and call both from the parent collapse_file() function.

What I did in mine was a "check" stage that handles both of these stages together; however, it is also broken down further within the check stage into:

collapse_file_check_folio
--> collapse_file_regular_check_folio (filemap is a better name) --> collapse_file_shmem_check_folio

the two child function help reduce nesting and make the code much more readable. then the parent function handles all the common checks and isolation.

I didnt use a helper function, but I think its the right approach.

overall I think your approach is better but should match the same flow as mine.

I dont think you need a separate isolate stage, as if you match my flow the check/prepare keep the error handling much cleaner (see my patch for an example of this).

Quoted 183 lines
Signed-off-by Pedro Falcato <[email protected]>
mm/khugepaged.c +93 -47
index 27e8f3077e80..d4de507ac001 100644
@@ -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;

mm/khugepaged.c static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, · line 2403

Quoted 10 lines
mm/khugepaged.c +10 -0
+		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;

You can hoist this stuff into the new check/prepare function if you pass the nr_none through the helper. I think the ultimate goal here would be to breakdown collapse_file() into a very simple stage based calling convention.

Leaving this one hunk that checks for nr_none in the parent would then seem out of place.

ie) alloc() -> check/prepare() -> copy() -> validate_holes() -> finalize()

I also did a rollback() function to keep the collapse_file() clean.

Quoted 31 lines
mm/khugepaged.c +0 -30
 				}
-			} 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;

mm/khugepaged.c static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, · line 2414

Quoted 10 lines
mm/khugepaged.c +7 -0
 			}
+			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;

I think prepare is a better name than my "check" functions. But i think it should also handle the isolation code below.

Cheers,
-- Nico

Signature · 1 line
Nico Pache (Red Hat) <[email protected]>
author
#5 lore raw

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

In reply to 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 <[email protected]>
---
 mm/khugepaged.c | 67 ++++++++++++++++++++++++++++++++-----------------
 1 file changed, 44 insertions(+), 23 deletions(-)
mm/khugepaged.c +44 -23
index d4de507ac001..4cc6917a55c7 100644
@@ -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.
Signature · 1 line
2.55.0
#6 lore raw

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

Quoted 3 lines · 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.

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.

Quoted 2 lines
Signed-off-by Pedro Falcato <[email protected]>

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

mm/khugepaged.c struct collapse_file_state { · line 2232

Quoted 10 lines
mm/khugepaged.c +0 -0
index d4de507ac001..4cc6917a55c7 100644
@@ -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.

mm/khugepaged.c struct collapse_file_state { · line 2233

Quoted 1 line
mm/khugepaged.c +1 -0
+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?

mm/khugepaged.c struct collapse_file_state { · line 2235

Quoted 2 lines
mm/khugepaged.c +2 -0
+{
+	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)

mm/khugepaged.c struct collapse_file_state { · line 2239

Quoted 4 lines
mm/khugepaged.c +4 -0
+	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.

mm/khugepaged.c struct collapse_file_state { · line 2240

Quoted 1 line
mm/khugepaged.c +1 -0
+		xas_unlock_irq(state->xas);

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

mm/khugepaged.c struct collapse_file_state { · line 2242

Quoted 2 lines
mm/khugepaged.c +2 -0
+		if (state->is_shmem) {
+			/* swap in or instantiate fallocated page */

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

mm/khugepaged.c struct collapse_file_state { · line 2245

Quoted 3 lines
mm/khugepaged.c +3 -0
+			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.

mm/khugepaged.c struct collapse_file_state { · line 2266

Quoted 7 lines

[ ... skip 16 lines ... ]

mm/khugepaged.c +3 -1
 	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 :)

mm/khugepaged.c struct collapse_file_state { · line 2268

Quoted 2 lines
mm/khugepaged.c +2 -0
+	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.

mm/khugepaged.c struct collapse_file_state { · line 2269

Quoted 1 line
mm/khugepaged.c +1 -0
+	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.

mm/khugepaged.c struct collapse_file_state { · line 2275

Quoted 6 lines
mm/khugepaged.c +6 -0
+		/*
+		 * 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.
	 */

mm/khugepaged.c struct collapse_file_state { · line 2277

Quoted 2 lines
mm/khugepaged.c +2 -0
+		 */
+		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.

mm/khugepaged.c struct collapse_file_state { · line 2292

Quoted 7 lines

[ ... skip 31 lines ... ]

mm/khugepaged.c +1 -2
-			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

Signature · 1 line
Lorenzo Stoakes (ARM) <[email protected]>
reviewer
#7 lore raw

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

In reply to Pedro Falcato
Quoted 3 lines

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.

As I stated in my review on (1) I think you should first hoist the prepare+isolate into a helper, then later do cleanups like you do here.

The flow my patch follows, leaves for much cleaner (and shared) error handling.

Sashiko left a comment about some locking issues which might be why sysbot found.

https://sashiko.dev/#/patchset/20260720142913.846902-1-pfalcato%40suse.de?part=2

Quoted 110 lines
Signed-off-by Pedro Falcato <[email protected]>
mm/khugepaged.c +43 -17
index d4de507ac001..4cc6917a55c7 100644
@@ -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() */

mm/khugepaged.c struct collapse_file_state {

Quoted 10 lines
mm/khugepaged.c +1 -6
-			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.

I agree with Lorenzo that this leaves a very ugly intermediate state. As I recommended please combine prepare+isolate into one stage, with two helpers for filemap and shmem, then do these kind of cleanups.

-- Nico

Signature · 1 line
Nico Pache (Red Hat) <[email protected]>
author
#8 lore raw

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

In reply to 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 <[email protected]>
---
 mm/khugepaged.c | 46 ++++++++++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 22 deletions(-)
mm/khugepaged.c +24 -22
index 4cc6917a55c7..ed5d23317177 100644
@@ -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;
Signature · 1 line
2.55.0
author
#9 lore raw

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

In reply to 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 <[email protected]>
---
 mm/khugepaged.c | 36 +++++++++++++-----------------------
 1 file changed, 13 insertions(+), 23 deletions(-)
mm/khugepaged.c +13 -23
index ed5d23317177..d170b64378e6 100644
@@ -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;
 }
 
 /**
Signature · 1 line
2.55.0
author
#10 lore raw

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

In reply to Pedro Falcato

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

Signed-off-by Pedro Falcato <[email protected]>
---
 mm/khugepaged.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
mm/khugepaged.c +15 -0
index d170b64378e6..db1f765b6ead 100644
@@ -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;
Signature · 1 line
2.55.0
author
#11 lore raw

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

In reply to Pedro Falcato
Signed-off-by Pedro Falcato <[email protected]>
---
 mm/khugepaged.c | 118 +++++++++++++++++++++++++++---------------------
 1 file changed, 66 insertions(+), 52 deletions(-)
mm/khugepaged.c +66 -52
index db1f765b6ead..429e2c5833d0 100644
@@ -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);
 
 		/*
Signature · 1 line
2.55.0
reviewer
#12 lore raw

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

Quoted 62 lines
Signed-off-by Pedro Falcato <[email protected]>
mm/khugepaged.c +52 -0
index db1f765b6ead..429e2c5833d0 100644
@@ -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;

mm/khugepaged.c static enum scan_result prepare_collapse_file_folio(pgoff_t index, struct collap · line 2396

Quoted 10 lines
mm/khugepaged.c +10 -0
+	}
+
+	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;
+}

prepare and isolate now share multiple goto error paths. if you move this into the prepare function (combining the prepare+isolate). the goto labels at the bottom become very easy to follow and reduce the code bloat.

With the two additional helpers (shmem vs !shmem) the code becomes very clean, even without the additional cleanups you did in the previous few patches.

I would suggest doing the very simple restructuring first, then including any additonal cleanups that you did in this series.

That would not only make it easier to review, but also a lot of the mess already gets handled just by following my recommended flow.

Cheers,
-- Nico

Signature · 1 line
Nico Pache (Red Hat) <[email protected]>
author
#13 lore raw

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

In reply to Pedro Falcato

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

Signed-off-by Pedro Falcato <[email protected]>
---
 mm/khugepaged.c | 83 ++++++++++++++++++++++---------------------------
 1 file changed, 37 insertions(+), 46 deletions(-)
mm/khugepaged.c +37 -46
index 429e2c5833d0..a09e4e4e1943 100644
@@ -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:
Signature · 1 line
2.55.0
reviewer
#14 lore raw

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

Quoted 98 lines

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

Signed-off-by Pedro Falcato <[email protected]>
mm/khugepaged.c +37 -36
index 429e2c5833d0..a09e4e4e1943 100644
@@ -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;

mm/khugepaged.c static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, · line 2555

Quoted 10 lines
mm/khugepaged.c +0 -5
-			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);

This can/should also be hoisted up if you follow my recommended code flow. This requires also passing pagelist (and index, as lorenzo already suggested) into the helper struct but thats fine.

mm/khugepaged.c static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, · line 2558

Quoted 8 lines
mm/khugepaged.c +0 -5
-		continue;
-out_unlock:
-		folio_unlock(folio);
-		folio_put(folio);
-		goto xa_unlocked;
 	}
 
 xa_locked:

my recommendations also allow you to group all these gotos into the bottom of the prepare stage which drastically cleanups the amount of gotos in the collapse_file() function.

with my code recommendations the loop becomes.

for (index = start; index < end;) {
...
-out_unlock:

-               folio_unlock(folio);
-               folio_put(folio);
-               goto xa_unlocked;
+               result = collapse_file_check_folio(folio, mapping, &xas,
+                                                  file, &index, start,
+                                                  end, is_shmem, &nr_none,
+                                                  &pagelist);
+               if (result != SCAN_SUCCEED)
+                       break;
        }

-xa_locked:
- xas_unlock_irq(&xas);
-xa_unlocked:
+ if (result == SCAN_SUCCEED)
+ xas_unlock_irq(&xas);

        /*
         * 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();

        if (result == SCAN_SUCCEED && nr_none &&
            !shmem_charge(mapping->host, nr_none))
                result = SCAN_FAIL;
        if (result != SCAN_SUCCEED) {
                nr_none = 0;
                goto rollback;
        }

	//Copy stage

Now we have no weird goto labels in collapse_file and rely on results to control the follow.

If you think my approach is already useable perhaps you can just take my commit I pointed to you that does all the major code moving with no major refactoring and add your further improvements on top.

let me know what you think!

Cheers,
-- Nico

Signature · 1 line
Nico Pache (Red Hat) <[email protected]>
author
#15 lore raw

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

In reply to 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 <[email protected]>
---
 mm/khugepaged.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
mm/khugepaged.c +7 -3
index a09e4e4e1943..1999e094df20 100644
@@ -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;
Signature · 1 line
2.55.0
#16 lore raw
In reply to Pedro Falcato
Quoted 6 lines · 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.

Hi Pedro,

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

I was waiting for my work here
https://lore.kernel.org/lkml/[email protected]/
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

Quoted 30 lines

Based on top of latest mm-unstable.

Cc: Zi Yan <[email protected]>
Cc: Baolin Wang <[email protected]>
Cc: "Liam R. Howlett" <[email protected]>
Cc: Nico Pache <[email protected]>
Cc: Ryan Roberts <[email protected]>
Cc: Dev Jain <[email protected]>
Cc: Barry Song <[email protected]>
Cc: Lance Yang <[email protected]>
Cc: Usama Arif <[email protected]>
Cc: [email protected]
Cc: [email protected]
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

author
#17 lore raw
Quoted 10 lines · Nico Pache
Quoted 6 lines · 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.

Hi Pedro,

Quoted 8 lines

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

I was waiting for my work here
https://lore.kernel.org/lkml/[email protected]/
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.

Signature · 1 line
Pedro
#18 lore raw
Quoted 14 lines · Pedro Falcato
Quoted 12 lines · Nico Pache
Quoted 6 lines · 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.

Hi Pedro,

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

Quoted 8 lines
Quoted 6 lines

I was waiting for my work here
https://lore.kernel.org/lkml/[email protected]/
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.

Quoted 6 lines

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

Quoted 4 lines

--
Pedro

#19 lore raw
Quoted 24 lines · Nico Pache
Quoted 14 lines · Pedro Falcato
Quoted 12 lines · Nico Pache
Quoted 6 lines · 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.

Hi Pedro,

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

Quoted 8 lines
Quoted 6 lines

I was waiting for my work here
https://lore.kernel.org/lkml/[email protected]/
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?

Quoted 8 lines

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?

Quoted 11 lines
Quoted 7 lines

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?

Quoted 9 lines

Cheers,
-- Nico

Quoted 4 lines

--
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

#20 lore raw
Quoted 35 lines · Lorenzo Stoakes (ARM)
Quoted 24 lines · Nico Pache
Quoted 14 lines · Pedro Falcato
Quoted 12 lines · Nico Pache
Quoted 6 lines · 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.

Hi Pedro,

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

Quoted 8 lines
Quoted 6 lines

I was waiting for my work here
https://lore.kernel.org/lkml/[email protected]/
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?

Quoted 8 lines

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.

Quoted 5 lines

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.

Quoted 9 lines
Quoted 9 lines
Quoted 8 lines

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.

Quoted 6 lines
Quoted 3 lines

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

Quoted 18 lines
Quoted 10 lines

Cheers,
-- Nico

Quoted 4 lines

--
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 Tested-by [email protected] on the v1 series
#21 lore raw
In reply to Pedro Falcato
syzbot ci has tested the following series

syzbot ci has tested the following series

[v1] mm/khugepaged: collapse_file() cleanups
https://lore.kernel.org/all/[email protected]
* [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 [email protected]

---
This report is generated by a bot. It may contain errors. syzbot ci engineers can be reached at [email protected].

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.