Re: [PATCH 0/5] mm/huge_memory: cleanup __split_unmapped_folio()

[Zi Yan <ziy@nvidia.com>]

On 14 Oct 2025, at 9:46, Wei Yang wrote:

> This short patch series cleans up and optimizes the internal logic of folio
> splitting, particularly focusing on the __split_unmapped_folio() function.
>
> The goal is to improve clarity and efficiency by eliminating redundant
> checks, caching stable attribute values, and simplifying the iteration
> logic used for updating folio statistics.
>
> These changes make the code easier to follow and maintain.
>
> Wei Yang (5):
>   mm/huge_memory: cache folio attribute in __split_unmapped_folio()
>   mm/huge_memory: update folio stat after successful split
>   mm/huge_memory: Optimize and simplify folio stat update after split
>   mm/huge_memory: Optimize old_order derivation during folio splitting
>   mm/huge_memory: Remove redundant split_order != new_order check in
>     uniform_split
>
>  mm/huge_memory.c | 70 +++++++++++++-----------------------------------
>  1 file changed, 18 insertions(+), 52 deletions(-)
>
The final code looks good to me, but patch 2-5 could be merged into one.
The diff below is the patch 2-5 and is not that big. My comments are
added below inline:



> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index b2a48e8e4e08..46ed647f85c1 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -3528,9 +3528,7 @@ static int __split_unmapped_folio(struct folio *folio, int new_order,
>  		struct address_space *mapping, bool uniform_split)
>  {
>  	bool is_anon = folio_test_anon(folio);
> -	int order = folio_order(folio);
> -	int start_order = uniform_split ? new_order : order - 1;

I would like to retain this, no need to inflate the initialization part
of for loop.

> -	struct folio *next;
> +	int old_order = folio_order(folio);
>  	int split_order;
>  	folio_clear_has_hwpoisoned(folio);
> @@ -3539,18 +3537,14 @@ static int __split_unmapped_folio(struct folio *folio, int new_order,
>  	 * split to new_order one order at a time. For uniform split,
>  	 * folio is split to new_order directly.
>  	 */
> -	for (split_order = start_order;
> +	for (split_order = uniform_split ? new_order : old_order - 1;
>  	     split_order >= new_order;
>  	     split_order--) {
> -		struct folio *end_folio = folio_next(folio);
> -		int old_order = folio_order(folio);
> -		struct folio *new_folio;
> +		int new_folios = 1UL << (old_order - split_order);

nr_new_folios is better.

>  		/* order-1 anonymous folio is not supported */
>  		if (is_anon && split_order == 1)
>  			continue;
> -		if (uniform_split && split_order != new_order)
> -			continue;

This is probably dead code in my initial implementation.
>  		if (mapping) {
>  			/*
> @@ -3573,19 +3567,12 @@ static int __split_unmapped_folio(struct folio *folio, int new_order,
>  		pgalloc_tag_split(folio, old_order, split_order);
>  		__split_folio_to_order(folio, old_order, split_order);
> -		if (is_anon)
> +		if (is_anon) {
>  			mod_mthp_stat(old_order, MTHP_STAT_NR_ANON, -1);
> -		/*
> -		 * Iterate through after-split folios and update folio stats.
> -		 */
> -		for (new_folio = folio; new_folio != end_folio; new_folio = next) {
> -			next = folio_next(new_folio);
> -			if (new_folio == page_folio(split_at))
> -				folio = new_folio;
> -			if (is_anon)
> -				mod_mthp_stat(folio_order(new_folio),
> -					      MTHP_STAT_NR_ANON, 1);
> +			mod_mthp_stat(split_order, MTHP_STAT_NR_ANON, new_folios);
>  		}
> +		folio = page_folio(split_at);

This is where non-uniform split moves to next to-be-split folio.
For uniform split, the for loop only iterates once, so this one
and the one below do not affect anything.

A comment above this assignment would help reader understand the difference
between uniform split and non-uniform split.

> +		old_order = split_order;
>  	}
>  	return 0;
>

Otherwise, looks good to me. Thanks for the cleanup.

--
Best Regards,
Yan, Zi