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

[Zi Yan <ziy@nvidia.com>]

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

>
>> 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?

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