Re: [PATCH] mm/huge_memory: fix early failure try_to_migrate() when split huge pmd for shared thp

[Zi Yan <ziy@nvidia.com>]

On 2 Feb 2026, at 18:57, Wei Yang wrote:

> On Sat, Jan 31, 2026 at 10:39:40PM -0500, Zi Yan wrote:
>> On 31 Jan 2026, at 21:09, Wei Yang wrote:
>>
>>> On Fri, Jan 30, 2026 at 09:44:10PM -0500, Zi Yan wrote:
>>>> On 30 Jan 2026, at 18:00, Wei Yang wrote:
>>>>
>>>>> Commit 60fbb14396d5 ("mm/huge_memory: adjust try_to_migrate_one() and
>>>>> split_huge_pmd_locked()") return false unconditionally after
>>>>> split_huge_pmd_locked() which may fail early during try_to_migrate() for
>>>>> shared thp. This will lead to unexpected folio split failure.
>>>>>
>>>>> One way to reproduce:
>>>>>
>>>>>     Create an anonymous thp range and fork 512 children, so we have a
>>>>>     thp shared mapped in 513 processes. Then trigger folio split with
>>>>>     /sys/kernel/debug/split_huge_pages debugfs to split the thp folio to
>>>>>     order 0.
>>>>>
>>>>> Without the above commit, we can successfully split to order 0.
>>>>> With the above commit, the folio is still a large folio.
>>>>>
>>>>> The reason is the above commit return false after split pmd
>>>>> unconditionally in the first process and break try_to_migrate().
>>>>
>>>> The reasoning looks good to me.
>>>>
>>>>>
>>>>> The tricky thing in above reproduce method is current debugfs interface
>>>>> leverage function split_huge_pages_pid(), which will iterate the whole
>>>>> pmd range and do folio split on each base page address. This means it
>>>>> will try 512 times, and each time split one pmd from pmd mapped to pte
>>>>> mapped thp. If there are less than 512 shared mapped process,
>>>>> the folio is still split successfully at last. But in real world, we
>>>>> usually try it for once.
>>>>>
>>>>> This patch fixes this by removing the unconditional false return after
>>>>> split_huge_pmd_locked(). Later, we may introduce a true fail early if
>>>>> split_huge_pmd_locked() does fail.
>>>>>
>>>>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>>>>> Fixes: 60fbb14396d5 ("mm/huge_memory: adjust try_to_migrate_one() and split_huge_pmd_locked()")
>>>>> Cc: Gavin Guo <gavinguo@igalia.com>
>>>>> Cc: "David Hildenbrand (Red Hat)" <david@kernel.org>
>>>>> Cc: Zi Yan <ziy@nvidia.com>
>>>>> Cc: Baolin Wang <baolin.wang@linux.alibaba.com>
>>>>> Cc: <stable@vger.kernel.org>
>>>>> ---
>>>>>  mm/rmap.c | 1 -
>>>>>  1 file changed, 1 deletion(-)
>>>>>
>>>>> diff --git a/mm/rmap.c b/mm/rmap.c
>>>>> index 618df3385c8b..eed971568d65 100644
>>>>> --- a/mm/rmap.c
>>>>> +++ b/mm/rmap.c
>>>>> @@ -2448,7 +2448,6 @@ static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma,
>>>>>  			if (flags & TTU_SPLIT_HUGE_PMD) {
>>>>>  				split_huge_pmd_locked(vma, pvmw.address,
>>>>>  						      pvmw.pmd, true);
>>>>> -				ret = false;
>>>>>  				page_vma_mapped_walk_done(&pvmw);
>>>>>  				break;
>>>>>  			}
>>>>
>>>> How about the patch below? It matches the pattern of set_pmd_migration_entry() below.
>>>> Basically, continue if the operation is successful, break otherwise.
>>>>
>>>> diff --git a/mm/rmap.c b/mm/rmap.c
>>>> index 618df3385c8b..83cc9d98533e 100644
>>>> --- a/mm/rmap.c
>>>> +++ b/mm/rmap.c
>>>> @@ -2448,9 +2448,7 @@ static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma,
>>>> 			if (flags & TTU_SPLIT_HUGE_PMD) {
>>>> 				split_huge_pmd_locked(vma, pvmw.address,
>>>> 						      pvmw.pmd, true);
>>>> -				ret = false;
>>>> -				page_vma_mapped_walk_done(&pvmw);
>>>> -				break;
>>>> +				continue;
>>>> 			}
>>>
>>> Per my understanding if @freeze is trur, split_huge_pmd_locked() may "fail" as
>>> the comment says:
>>>
>>> 		 * Without "freeze", we'll simply split the PMD, propagating the
>>> 		 * PageAnonExclusive() flag for each PTE by setting it for
>>> 		 * each subpage -- no need to (temporarily) clear.
>>> 		 *
>>> 		 * With "freeze" we want to replace mapped pages by
>>> 		 * migration entries right away. This is only possible if we
>>> 		 * managed to clear PageAnonExclusive() -- see
>>> 		 * set_pmd_migration_entry().
>>> 		 *
>>> 		 * In case we cannot clear PageAnonExclusive(), split the PMD
>>> 		 * only and let try_to_migrate_one() fail later.
>>>
>>> While currently we don't return the status of split_huge_pmd_locked() to
>>> indicate whether it does replaced PMD with migration entries successfully. So
>>> we are not sure this operation succeed.
>>
>> This is the right reasoning. This means to properly handle it, split_huge_pmd_locked()
>> needs to return whether it inserts migration entries or not when freeze is true.
>>
>>>
>>> Another difference from set_pmd_migration_entry() is split_huge_pmd_locked()
>>> would change the page table from PMD mapped to PTE mapped.
>>> page_vma_mapped_walk() can handle it now for (pvmw->pmd && !pvmw->pte), but I
>>> am not sure this is what we expected. For example, in try_to_unmap_one(), we
>>> use page_vma_mapped_walk_restart() after pmd splitted.
>>>
>>> So I prefer just remove the "ret = false" for a fix. Not sure this is
>>> reasonable to you.
>>>
>>> I am thinking two things after this fix:
>>>
>>>   * add one similar test in selftests
>>>   * let split_huge_pmd_locked() return value to indicate freeze is degrade to
>>>     !freeze, and fail early on try_to_migrate() like the thp migration branch
>>>
>>> Look forward your opinion on whether it worth to do it.
>>
>> This is not the right fix, neither was mine above. Because before commit 60fbb14396d5,
>> the code handles PAE properly. If PAE is cleared, PMD is split into PTEs and each
>> PTE becomes a migration entry, page_vma_mapped_walk(&pvmw) returns false,
>> and try_to_migrate_one() returns true. If PAE is not cleared, PMD is split into PTEs
>> and each PTE is not a migration entry, inside while (page_vma_mapped_walk(&pvmw)),
>> PAE will be attempted to get cleared again and it will fail again, leading to
>> try_to_migrate_one() returns false. After commit 60fbb14396d5, no matter PAE is
>> cleared or not, try_to_migrate_one() always returns false. It causes folio split
>> failures for shared PMD THPs.
>>
>> Now with your fix (and mine above), no matter PAE is cleared or not, try_to_migrate_one()
>> always returns true. It just flips the code to a different issue. So the proper fix
>> is to let split_huge_pmd_locked() returns whether it inserts migration entries or not
>> and do the same pattern as THP migration code path.
>>
>
> You are right.
>
> BTW, I thought PAE stands for Physical Address Extension and confused a while :-(
>
>>
>> Hi David,
>>
>> In terms of unmap_folio(), which is the only user of split_huge_pmd_locked(..., freeze=true),
>> there is no folio_mapped() check afterwards. That might be causing an issue,
>> when the folio is pinned between the refcount check and unmap_folio(), unmap_folio()
>> fails, but folio split code proceeds. That means the folio is still accessible
>> via PTEs and later remove_migration_pte() will try to remove non migration PTEs.
>> It needs to be fixed separately, right?
>>
>
> Current __folio_split() logic is like below:
>
>     if (folio_expected_ref_count(folio) != folio_ref_count(folio) - 1) {     --- (1)
>     	ret = -EAGAIN;
> 	goto out_unlock;
>     }
>
>     unmap_folio(folio);                                                      --- (2)
>
>     ret = __folio_freeze_and_split_unmapped()
>         if (folio_ref_freeze(folio, folio_cache_ref_count(folio) + 1)) {     --- (3)
> 	} else {
> 	    return -EAGAIN;
> 	}
>
> You mean after (1) and (2), we don't check folio_mapped() and continue
> spliting? Hmm... before continue split we tried to freeze folio with expected
> refcount at (3). This makes sure there is not extra refcount except in
> pagecache or swapcache.
>
> You mean this is not enough? Not sure I follow you correctly.

Missed that. It works. Thanks for pointing this out.

>
>>
>>>
>>>> #ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
>>>> 			pmdval = pmdp_get(pvmw.pmd);
>>>>
>>>>
>>>>
>>>> --
>>>> Best Regards,
>>>> Yan, Zi
>>>
>>> -- 
>>> Wei Yang
>>> Help you, Help me
>>
>>
>> --
>> Best Regards,
>> Yan, Zi
>
> -- 
> Wei Yang
> Help you, Help me


Best Regards,
Yan, Zi