Re: [PATCH v2 1/2] mm/huge_memory: move unrelated code out of __split_unmapped_folio()

[Zi Yan <ziy@nvidia.com>]

On 14 Jul 2025, at 11:30, David Hildenbrand wrote:

> On 11.07.25 20:23, Zi Yan wrote:
>> remap(), folio_ref_unfreeze(), lru_add_split_folio() are not relevant to
>> splitting unmapped folio operations. Move them out to the caller so that
>> __split_unmapped_folio() only handles unmapped folio splits. This makes
>> __split_unmapped_folio() reusable.
>>
>> Convert VM_BUG_ON(mapping) to use VM_WARN_ON_ONCE_FOLIO().
>>
>> Signed-off-by: Zi Yan <ziy@nvidia.com>
>> ---
>
> [...]
>
>> -	if (folio_test_swapcache(folio)) {
>> -		VM_BUG_ON(mapping);
>> -
>> -		/* a swapcache folio can only be uniformly split to order-0 */
>> -		if (!uniform_split || new_order != 0)
>> -			return -EINVAL;
>> -
>> -		swap_cache = swap_address_space(folio->swap);
>> -		xa_lock(&swap_cache->i_pages);
>> -	}
>> -
>>   	if (folio_test_anon(folio))
>>   		mod_mthp_stat(order, MTHP_STAT_NR_ANON, -1);
>>  -	/* lock lru list/PageCompound, ref frozen by page_ref_freeze */
>> -	lruvec = folio_lruvec_lock(folio);
>>
>
> Nit: now double empty line.
Will fix it.

>
>>   	folio_clear_has_hwpoisoned(folio);
>>  @@ -3480,9 +3451,9 @@ static int __split_unmapped_folio(struct folio *folio, int new_order,
>>   	for (split_order = start_order;
>>   	     split_order >= new_order && !stop_split;
>>   	     split_order--) {
>> -		int old_order = folio_order(folio);
>> -		struct folio *release;
>>   		struct folio *end_folio = folio_next(folio);
>> +		int old_order = folio_order(folio);
>> +		struct folio *new_folio;
>>    		/* order-1 anonymous folio is not supported */
>>   		if (folio_test_anon(folio) && split_order == 1)
>> @@ -3517,113 +3488,34 @@ static int __split_unmapped_folio(struct folio *folio, int new_order,
>>    after_split:
>>   		/*
>> -		 * Iterate through after-split folios and perform related
>> -		 * operations. But in buddy allocator like split, the folio
>> +		 * 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 (release = folio; release != end_folio; release = next) {
>> -			next = folio_next(release);
>> +		for (new_folio = folio; new_folio != end_folio; new_folio = next) {
>> +			next = folio_next(new_folio);
>>   			/*
>> -			 * for buddy allocator like split, the folio containing
>> -			 * page will be split next and should not be released,
>> -			 * until the folio's order is new_order or stop_split
>> -			 * is set to true by the above xas_split() failure.
>> +			 * for buddy allocator like split, new_folio containing
>> +			 * 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 (release == page_folio(split_at)) {
>> -				folio = release;
>> +			if (new_folio == page_folio(split_at)) {
>> +				folio = new_folio;
>>   				if (split_order != new_order && !stop_split)
>>   					continue;
>>   			}
>> -			if (folio_test_anon(release)) {
>> -				mod_mthp_stat(folio_order(release),
>> +			if (folio_test_anon(new_folio)) {
>> +				mod_mthp_stat(folio_order(new_folio),
>>   						MTHP_STAT_NR_ANON, 1);
>>   			}
>
> Nit: {} can be dropped

Sure.

>
> Code is still confusing, so could be that I miss something, but in general
> looks like an improvement to me.
>
> I think we can easily get rid of the goto label in __split_unmapped_folio() doing something like
>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 14bc0b54cf9f0..db0ae957a0ba8 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -3435,18 +3435,18 @@ static int __split_unmapped_folio(struct folio *folio, int new_order,
>                                 if (xas_error(xas)) {
>                                         ret = xas_error(xas);
>                                         stop_split = true;
> -                                       goto after_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);
> +               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);
>  -               __split_folio_to_order(folio, old_order, split_order);
> +                       __split_folio_to_order(folio, old_order, split_order);
> +               }
>  -after_split:
>                 /*
>                  * Iterate through after-split folios and update folio stats.
>                  * But in buddy allocator like split, the folio
>

Yep, looks much better to me. Let me fix it in V3. Thank you for the review
and suggestions.

Best Regards,
Yan, Zi