Re: [PATCH] mm/huge_memory: consolidate order-related checks into folio_split_supported()

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

On Fri, Nov 14, 2025 at 03:53:42PM -0500, Zi Yan wrote:
>On 14 Nov 2025, at 9:30, Wei Yang wrote:
>
>> On Fri, Nov 14, 2025 at 07:43:38AM -0500, Zi Yan wrote:
>>> On 14 Nov 2025, at 3:49, David Hildenbrand (Red Hat) wrote:
>>>
>> [...]
>>>>> +
>>>>> +	if (new_order >= old_order)
>>>>> +		return -EINVAL;
>>>>> +
>>>>>   	if (folio_test_anon(folio)) {
>>>>>   		/* order-1 is not supported for anonymous THP. */
>>>>>   		VM_WARN_ONCE(warns && new_order == 1,
>>>>>   				"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 {
>>>>> +		const struct address_space *mapping = NULL;
>>>>> +
>>>>> +		mapping = folio->mapping;
>>>>
>>>> const struct address_space *mapping = folio->mapping;
>>>>
>>>>> +
>>>>> +		/* Truncated ? */
>>>>> +		/*
>>>>> +		 * TODO: add support for large shmem folio in swap cache.
>>>>> +		 * When shmem is in swap cache, mapping is NULL and
>>>>> +		 * folio_test_swapcache() is true.
>>>>> +		 */
>>>>> +		if (!mapping)
>>>>> +			return false;
>>>>> +
>>>>> +		/*
>>>>> +		 * We have two types of split:
>>>>> +		 *
>>>>> +		 *   a) uniform split: split folio directly to new_order.
>>>>> +		 *   b) non-uniform split: create after-split folios with
>>>>> +		 *      orders from (old_order - 1) to new_order.
>>>>> +		 *
>>>>> +		 * For file system, we encodes it supported folio order in
>>>>> +		 * mapping->flags, which could be checked by
>>>>> +		 * mapping_folio_order_supported().
>>>>> +		 *
>>>>> +		 * With these knowledge, we can know whether folio support
>>>>> +		 * split to new_order by:
>>>>> +		 *
>>>>> +		 *   1. check new_order is supported first
>>>>> +		 *   2. check (old_order - 1) is supported if
>>>>> +		 *      SPLIT_TYPE_NON_UNIFORM
>>>>> +		 */
>>>>> +		if (!mapping_folio_order_supported(mapping, new_order)) {
>>>>> +			VM_WARN_ONCE(warns,
>>>>> +				"Cannot split file folio to unsupported order: %d", new_order);
>>>>
>>>> Is that really worth a VM_WARN_ONCE? We didn't have that previously IIUC, we would only return
>>>> -EINVAL.
>>>
>>
>> Sorry for introducing this unpleasant affair.
>>
>> Hope I can explain what I have done.
>>
>>> No, and it causes undesired warning when LBS folio is enabled. I explicitly
>>> removed this warning one month ago in the LBS related patch[1].
>>>
>>
>> Yes, I see you removal of a warning in [1].
>>
>> While in the discussion in [2], you mentioned:
>>
>>   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.
>>
>> I thought you agree to print a warning message here. So I am confused.
>
>This is exactly my point. You need to know what you are doing. You should not
>write a patch because of what I said. And my above comment is to
>CONFIG_READ_ONLY_THP_FOR_FS part of code. It has nothing
>to do with the check pulled into folio_split_supported().
>

Yes, I thought they serve the same purpose(checking supported folio order), so
print the warning both.

If should not, I will remove it.

>>
>>> It is so frustrating to see this part of patch. Wei has RB in the aforementioned
>>> patch and still add this warning blindly. I am not sure if Wei understands
>>> what he is doing, since he threw the idea to me and I told him to just
>>> move the code without changing the logic, but he insisted doing it in his
>>> own way and failed[2]. This retry is still wrong.
>>>
>>
>> I think we are still discussing the problem and a patch maybe more convenient
>> to proceed. I didn't insist anything and actually I am looking forward your
>> option and always respect your insight. Never thought to offend you.
>
>Not offended.
>>
>> In discussion [2], you pointed out two concerns:
>>
>>   1) new_order < min_order is meaning less if min_order is 0
>>   2) how to do the check if new_order is 0 for non-uniform split
>>
>> For 1), you suggested to add mapping_folio_order_supported().
>> For 2), I come up an idea to check (old_order - 1) <= max_order. Originally,
>> we just check !max_order. I think this could cover it.
>>
>> So I gather them together here to see whether it is suitable.
>>
>> If I missed some part, hope you could let me know.
>
>Based on the discussion in [2], your patch mixes the checks for FS does not
>support large folio and FS supporting large folio has min_order requirement
>and I told you that it does not work well and suggested you to just move
>“if (new_order < min_order) {“ part into folio_split_supported() as an
>easy approach. Why not do that?
>

I may not follow you. Let me try to clear my mind.

My first approach is :

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) {
 			/*
 			 * We can always split a folio down to a single page
 			 * (new_order == 0) uniformly.


Current patch does 3 major changes.

1)

Your comment to above change:

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

   ...
   
   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().
```

So in this patch, I introduce mapping_folio_order_supported() and replace the
(new_order < min_order) check. Still use (new_order < min_order) looks not
correct. Or I misunderstand your suggestion?

2)

And then you mentioned this check is not enough:

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

What come up my mind is to check with old_orer - 1 is also supported. So for
SPLIT_TYPE_NON_UNIFORM, the after-split folio range [min_order, old_order - 1]
are all checked.

3)

Also I remove the first "if (split_type == SPLIT_TYPE_NON_UNIFORM || new_order)".

Because in original logic, when split_type == SPLIT_TYPE_UNIFORM && new_order
== 0, we should still check (new_order < min_order). If not we would miss
that.

But sounds I misunderstand you insight, I am sorry for that. If not too
bother, would you mind sharing it again?

>>
>>> Wei, please make sure you understand the code before sending any patch.
>>>
>>> [1] https://lore.kernel.org/linux-mm/20251017013630.139907-1-ziy@nvidia.com/
>>> [2] https://lore.kernel.org/linux-mm/20251114030301.hkestzrk534ik7q4@master/
>>>
>>> Best Regards,
>>> Yan, Zi
>>
>> -- 
>> Wei Yang
>> Help you, Help me
>
>
>Best Regards,
>Yan, Zi

-- 
Wei Yang
Help you, Help me