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

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

On Fri, Nov 07, 2025 at 07:29:44AM +0000, Wei Yang wrote:
>On Thu, Nov 06, 2025 at 10:21:21PM -0500, Zi Yan wrote:
>>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:
>>
>
>Thanks for your analysis.
>
>>if (new_order < mapping_min_folio_order() || new_order > mapping_max_folio_order()).
>>
>
>This reminds me one thing, we don't check on max_order now.
>
>For example, the supported split order is [3, 5]. But new_order is set to 6.
>
>In current kernel, we don't do this. try_folio_split_or_unmap() pass
>min_order. But selftest will split from pmd_order - 1.
>
>>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)".
>>
>
>I am trying to think another way.
>
>For uniform split, after-split folio order is new_order.
>For non-uniform split, after-split folio order is [new_order, old_order - 1].
>
>So I come up following draft change.
>
>diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>index dee416b3f6ed..873680ab4cbb 100644
>--- a/mm/huge_memory.c
>+++ b/mm/huge_memory.c
>@@ -3703,28 +3703,18 @@ bool folio_split_supported(struct folio *folio, unsigned int new_order,
> 				"Cannot split to order-1 folio");
> 		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)) {
>-			/*
>-			 * We can always split a folio down to a single page
>-			 * (new_order == 0) uniformly.
>-			 *
>-			 * For any other scenario
>-			 *   a) uniform split targeting a large folio
>-			 *      (new_order > 0)
>-			 *   b) any non-uniform split
>-			 * we must confirm that the file system supports large
>-			 * folios.
>-			 *
>-			 * Note that we might still have THPs in such
>-			 * mappings, which is created from khugepaged when
>-			 * CONFIG_READ_ONLY_THP_FOR_FS is enabled. But in that
>-			 * case, the mapping does not actually support large
>-			 * folios properly.
>-			 */
>+	} else {
>+		/*
>+		 * Some explanation here.
>+		 */
>+		if (new_order && !mapping_folio_order_supported(new_order)) {
>+			VM_WARN_ONCE(warns,
>+				"Cannot split file folio to unsupported order: %d", new_order);
>+			return false;
>+		}
>+		if (split_type == SPLIT_TYPE_NON_UNIFORM && !mapping_folio_order_supported(old_order - 1)) {
> 			VM_WARN_ONCE(warns,
>-				"Cannot split file folio to non-0 order");
>+				"Cannot split file folio to unsupported order: %d", old_order - 1);
> 			return false;
> 		}
> 	}

Hi, Zi Yan

Does it look good to you?

-- 
Wei Yang
Help you, Help me