Re: [Patch v2 2/2] mm/huge_memory: Optimize and simplify __split_unmapped_folio() logic

[Zi Yan <ziy@nvidia.com>]

On 17 Oct 2025, at 5:44, Lorenzo Stoakes wrote:

> On Thu, Oct 16, 2025 at 12:46:13AM +0000, Wei Yang wrote:
>> 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.
>
> You seem to be doing two things at once, a big refactoring where you move stuff
> about AND changing functionality.

No function change is done in this patchset. The wording might be
confusing here, it should be read like:

After simplification, __split_unmapped_folio() directly calculate and
update the folio statistics upon a successful split, so An xas_try_split()
error is returned directly without worrying about stats updates.

David suggested a change[1] to make it clear:
Stats fixup is no longer needed for an xas_try_split() error,
since we now update the stats only after a successful split.


[1] https://lore.kernel.org/linux-mm/518dedb8-d379-47c3-a4c1-f4afc789f1b4@redhat.com/

>
> Can we split this out please? It makes review so much harder.

I asked Wei to use a single patch for this change, since the original
code was complicated due to the initial implementation. After my
recent change (first commit 6c7de9c83)[1], __split_unmmaped_folio()
can be simplified like Wei did here.



[1] https://lore.kernel.org/all/20250718023000.4044406-7-ziy@nvidia.com/

>
>>
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> Cc: Zi Yan <ziy@nvidia.com>
>> Reviewed-by: Zi Yan <ziy@nvidia.com>
>>
>> ---
>> v2:
>>   * merge patch 2-5
>>   * retain start_order
>>   * new_folios -> nr_new_folios
>>   * add a comment at the end of the loop
>> ---
>>  mm/huge_memory.c | 66 ++++++++++++++----------------------------------
>>  1 file changed, 19 insertions(+), 47 deletions(-)
>>
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index 4b2d5a7e5c8e..68e851f5fcb2 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -3528,15 +3528,9 @@ 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;
>> -	bool stop_split = false;
>> -	struct folio *next;
>> +	int old_order = folio_order(folio);
>> +	int start_order = uniform_split ? new_order : old_order - 1;
>>  	int split_order;
>> -	int ret = 0;
>> -
>> -	if (is_anon)
>> -		mod_mthp_stat(order, MTHP_STAT_NR_ANON, -1);
>>
>>  	folio_clear_has_hwpoisoned(folio);
>>
>> @@ -3545,17 +3539,13 @@ static int __split_unmapped_folio(struct folio *folio, int new_order,
>>  	 * folio is split to new_order directly.
>>  	 */
>>  	for (split_order = start_order;
>> -	     split_order >= new_order && !stop_split;
>> +	     split_order >= new_order;
>>  	     split_order--) {
>> -		struct folio *end_folio = folio_next(folio);
>> -		int old_order = folio_order(folio);
>> -		struct folio *new_folio;
>> +		int nr_new_folios = 1UL << (old_order - split_order);
>>
>>  		/* order-1 anonymous folio is not supported */
>>  		if (is_anon && split_order == 1)
>>  			continue;
>> -		if (uniform_split && split_order != new_order)
>> -			continue;
>>
>>  		if (mapping) {
>>  			/*
>> @@ -3568,49 +3558,31 @@ static int __split_unmapped_folio(struct folio *folio, int new_order,
>>  			else {
>>  				xas_set_order(xas, folio->index, split_order);
>>  				xas_try_split(xas, folio, old_order);
>> -				if (xas_error(xas)) {
>> -					ret = xas_error(xas);
>> -					stop_split = true;
>> -				}
>> +				if (xas_error(xas))
>> +					return xas_error(xas);
>>  			}
>>  		}
>>
>> -		if (!stop_split) {
>> -			folio_split_memcg_refs(folio, old_order, split_order);
>> -			split_page_owner(&folio->page, old_order, split_order);
>> -			pgalloc_tag_split(folio, old_order, split_order);
>> +		folio_split_memcg_refs(folio, old_order, split_order);
>> +		split_page_owner(&folio->page, old_order, split_order);
>> +		pgalloc_tag_split(folio, old_order, split_order);
>> +		__split_folio_to_order(folio, old_order, split_order);
>>
>> -			__split_folio_to_order(folio, old_order, split_order);
>> +		if (is_anon) {
>> +			mod_mthp_stat(old_order, MTHP_STAT_NR_ANON, -1);
>> +			mod_mthp_stat(split_order, MTHP_STAT_NR_ANON, nr_new_folios);
>>  		}
>>
>>  		/*
>> -		 * Iterate through after-split folios and update folio stats.
>> -		 * But in buddy allocator like split, the folio
>> -		 * containing the specified page is skipped until its order
>> -		 * is new_order, since the folio will be worked on in next
>> -		 * iteration.
>> +		 * For uniform split, we have finished the job.
>
> Finsihed what job? This is unclear.
>
>> +		 * For non-uniform split, we assign folio to the one the one
>
> 'To the one the one' you're duplicating that, and I have no idea what 'the one'
> means?
>
>> +		 * containing @split_at and assign @old_order to @split_order.
>
> Now you're just describing code, and why are you making it kdoc-like in a
> non-kdoc comment?
>
> I mean you're now unconditionally assigning folio to page_folio(split_at) and
> reassigning split-order to old_order so you really need to be clearer about what
> you mean here, given there is no e.g.:
>
> if (is uniform split)
> 	break;
>
> Something simpler would probably work better here.
>
>>  		 */
>> -		for (new_folio = folio; new_folio != end_folio; new_folio = next) {
>> -			next = folio_next(new_folio);
>> -			/*
>> -			 * for buddy allocator like split, new_folio containing
>> -			 * @split_at page could be split again, thus do not
>> -			 * change stats yet. Wait until new_folio's order is
>> -			 * @new_order or stop_split is set to true by the above
>> -			 * xas_split() failure.
>> -			 */
>> -			if (new_folio == page_folio(split_at)) {
>> -				folio = new_folio;
>> -				if (split_order != new_order && !stop_split)
>> -					continue;
>> -			}
>> -			if (is_anon)
>> -				mod_mthp_stat(folio_order(new_folio),
>> -					      MTHP_STAT_NR_ANON, 1);
>> -		}
>> +		folio = page_folio(split_at);
>> +		old_order = split_order;
>>  	}
>>
>> -	return ret;
>> +	return 0;
>>  }
>>
>>  bool non_uniform_split_supported(struct folio *folio, unsigned int new_order,
>> --
>> 2.34.1
>>
>>


--
Best Regards,
Yan, Zi