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

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

On Wed, Oct 15, 2025 at 09:34:39AM -0400, Zi Yan wrote:
>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.

Thanks

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

Yes. Will add it.

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

-- 
Wei Yang
Help you, Help me