Re: [Patch v3 2/2] mm/huge_memory: merge uniform_split_supported() and non_uniform_split_supported()

[Zi Yan <ziy@nvidia.com>]

On 6 Nov 2025, at 21:49, Wei Yang wrote:

> On Thu, Nov 06, 2025 at 09:07:22PM -0500, Zi Yan wrote:
>> On 6 Nov 2025, at 20:17, Wei Yang wrote:
>>
>>> On Thu, Nov 06, 2025 at 07:46:14PM -0500, Zi Yan wrote:
>>>> On 5 Nov 2025, at 22:41, Wei Yang wrote:
>>>>
>>>>> The functions uniform_split_supported() and
>>>>> non_uniform_split_supported() share significantly similar logic.
>>>>>
>>>>> The only functional difference is that uniform_split_supported()
>>>>> includes an additional check on the requested @new_order.
>>>>>
>>>>> The reason for this check comes from the following two aspects:
>>>>>
>>>>>   * some file system or swap cache just supports order-0 folio
>>>>>   * the behavioral difference between uniform/non-uniform split
>>>>>
>>>>> The behavioral difference between uniform split and non-uniform:
>>>>>
>>>>>   * uniform split splits folio directly to @new_order
>>>>>   * non-uniform split creates after-split folios with orders from
>>>>>     folio_order(folio) - 1 to new_order.
>>>>>
>>>>> This means for non-uniform split or !new_order split we should check the
>>>>> file system and swap cache respectively.
>>>>>
>>>>> This commit unifies the logic and merge the two functions into a single
>>>>> combined helper, removing redundant code and simplifying the split
>>>>> support checking mechanism.
>>>>>
>>>>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>>>>> Cc: Zi Yan <ziy@nvidia.com>
>>>>> Cc: "David Hildenbrand (Red Hat)" <david@kernel.org>
>>>>>
>>>>> ---
>>>>> v3:
>>>>>   * adjust to use split_type
>>>>>   * rebase on Zi Yan fix lkml.kernel.org/r/20251105162910.752266-1-ziy@nvidia.com
>>>>> v2:
>>>>>   * remove need_check
>>>>>   * update comment
>>>>>   * add more explanation in change log
>>>>> ---
>>>>>  include/linux/huge_mm.h |  8 ++---
>>>>>  mm/huge_memory.c        | 71 +++++++++++++++++------------------------
>>>>>  2 files changed, 33 insertions(+), 46 deletions(-)
>>>>>
>>>> LGTM. Thanks. Reviewed-by: Zi Yan <ziy@nvidia.com>
>>>
>>> Hi, Zi
>>>
>>> I am thinking whether it is proper to move the check (new_order < min_order)
>>> from __folio_split() to folio_split_supported(). So that we could bail out
>>> early if file system couldn't split to new_order.
>>>
>>> Not sure you like it or not.
>>
>> It sounds reasonable. My only concern is that that might add another
>> indentation to the else branch in folio_split_supported().
>>
>> You can send a patch, so we can see how it looks.
>>
>
> Here is what come up my mind.
>
> If !CONFIG_READ_ONLY_THP_FOR_FS, we directly compare new_order and min_order.
>
> If CONFIG_READ_ONLY_THP_FOR_FS, one thing I am not sure is for the khugepaged
> collapsed THP. If its min_order is 0, it looks we can cover it with following
> check.

1. mapping_large_folio_support() checks if mapping_max_folio_order() > 0, meaning
   !mapping_large_folio_support() is mapping_max_folio_order() == 0,
2. mapping_max_folio_order() >= mapping_min_folio_order(),
3. combining 1) and 2) means
   mapping_min_folio_order() <= mapping_max_folio_order() == 0,
   meaning mapping_min_folio_order() == 0.

so a FS without large folio support always has min_order == 0.

>
> Look forward your insight.
>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index dee416b3f6ed..ef05f246df73 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -3704,8 +3704,8 @@ bool folio_split_supported(struct folio *folio, unsigned int new_order,
>  		if (new_order == 1)
>  			return false;
>  	} else if (split_type == SPLIT_TYPE_NON_UNIFORM || new_order) {
> -		if (IS_ENABLED(CONFIG_READ_ONLY_THP_FOR_FS) &&
> -		    !mapping_large_folio_support(folio->mapping)) {
> +		unsigned int min_order = mapping_min_folio_order(folio->mapping);
> +		if (new_order < min_order) {

This check is good for !CONFIG_READ_ONLY_THP_FOR_FS, but
for CONFIG_READ_ONLY_THP_FOR_FS and !mapping_large_folio_support(),
min_order is always 0, how can new_order be smaller than min_order
to trigger the warning below? You will need to check new_order against
mapping_max_folio_order().

OK, basically the check should be:

if (new_order < mapping_min_folio_order() || new_order > mapping_max_folio_order()).

Then, you might want to add a helper function mapping_folio_order_supported()
instead and change the warning message below to "Cannot split file folio to
unsupported order [%d, %d]", min_order, max_order (showing min/max order
is optional since it kinda defeat the purpose of having the helper function).
Of course, the comment needs to be changed.

Hmm, but still how could the above check to trigger the warning when
split_type == SPLIT_TYPE_NON_UNIFORM and new_order is 0? It will not
trigger, since new_order (as 0) is supported by the mapping.

I guess the min_order check code has to be in the else branch along
with the existing "if (split_type == SPLIT_TYPE_NON_UNIFORM || new_order)".

>  			/*
>  			 * We can always split a folio down to a single page
>  			 * (new_order == 0) uniformly.
> @@ -3827,7 +3827,6 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
>  		}
>  		mapping = NULL;
>  	} else {
> -		unsigned int min_order;
>  		gfp_t gfp;
>
>  		mapping = folio->mapping;
> @@ -3843,12 +3842,6 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
>  			goto out;
>  		}
>
> -		min_order = mapping_min_folio_order(folio->mapping);
> -		if (new_order < min_order) {
> -			ret = -EINVAL;
> -			goto out;
> -		}
> -
>  		gfp = current_gfp_context(mapping_gfp_mask(mapping) &
>  							GFP_RECLAIM_MASK);
>
> -- 
> Wei Yang
> Help you, Help me


--
Best Regards,
Yan, Zi