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

[Wei Yang <richard.weiyang@gmail.com>]

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

Below commit log looks good to you?


    mm/huge_memory: Optimize and simplify __split_unmapped_folio() logic
    
    This commit refactors the statistic update and iteration logic within
    __split_unmapped_folio() to improve clarity and efficiency.
    
    The current implementation is overly complicated due to two main issues:
    
      1. It iterates over all resulting new folios to perform two combined
         tasks: update statistics and determine the folio for the next
         split.
    
      2. It uses confusing conditional logic (skipping the stat update for
         the folio at @split_at on success, only to attempt updating it
         later on a subsequent failure path).
    
    This refactoring removes the confusing iteration and conditional updates
    by leveraging information that is already known, allowing us to directly
    calculate and update the folio statistics upon a successful split:
    
      * All resulting folios have a known order: @split_order.
    
      * The number of new folios can be calculated directly from @old_order
        and @split_order.
    
      * The folio for the next split is easily identified as the one
        containing @split_at.
    
    This change results in a much cleaner and more efficient stat update
    without the complex looping logic.
    
    This commit also includes a related cleanup to the uniform splitting
    logic by removing the check for split_order != new_order, as this
    condition will not logically occur within the expected flow of a uniform
    split.

>
>
>> 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.
		 */

>> +		old_order = split_order;
>>  	}
>>  	return 0;
>>
>
>Otherwise, looks good to me. Thanks for the cleanup.
>
>--
>Best Regards,
>Yan, Zi

-- 
Wei Yang
Help you, Help me