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

[Zi Yan <ziy@nvidia.com>]

On 15 Oct 2025, at 4:15, Wei Yang wrote:

> On Tue, Oct 14, 2025 at 08:45:43PM -0400, Zi Yan wrote:
>> 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:
>>
>
> Sure, let me try to merge them. The challenge for me is how to merge the
> change log :-(

I do not think you need to explain how complicated the code looks like now.
You can focus on how your __split_unmapped_folio() works.

>
> Below commit log looks good to you?
>
>
>     mm/huge_memory: Optimize and simplify __split_unmapped_folio() logic

Existing __split_unmapped_folio() code splits the given folio and update stats,
but it is complicated to understand.

After simplification, __split_unmapped_folio() directly calculate and update
the folio statistics upon a successful split:

* All resulting folios are @split_order.

* The number of new folios are calculated directly from @old_order
  and @split_order.

* The folio for the next split is identified as the one containing @split_at.

* An xas_try_split() error is returned directly without worrying about stats updates.

The above commit log would be sufficient. Your code is quite easy to understand.

<snip>

>>
>>
>>> 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.
>
> Sure
>
>>
>>> -	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.
>>
>
> Sounds good.
>
>>>  		/* 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.
>>
>
> How about this?
>
> 		/*
> 		 * For uniform split, we have finished the job.
> 		 * For non-uniform split, we assign folio to the one the one
> 		 * containing @split_at and assign @old_order to @split_order.
> 		 */

Looks good to me.
>
>>> +		old_order = split_order;
>>>  	}
>>>  	return 0;
>>>
>>
>> Otherwise, looks good to me. Thanks for the cleanup.
>>

BTW, does split_huge_page selftest pass? If so, please write it on the cover letter.

With all these, feel free to add Reviewed-by: Zi Yan <ziy@nvidia.com>

Thanks.

--
Best Regards,
Yan, Zi