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

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