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 19:00, Wei Yang wrote:

> On Sun, Feb 01, 2026 at 09:20:35AM -0500, Zi Yan wrote:
>> On 1 Feb 2026, at 8:04, Gavin Guo wrote:
>>
>>> On 2/1/26 11:39, 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.
>>>
>>> How about aligning with the try_to_unmap_one()? The behavior would be the same before applying the commit 60fbb14396d5:
>>>
>>> diff --git a/mm/rmap.c b/mm/rmap.c
>>> index 7b9879ef442d..0c96f0883013 100644
>>> --- a/mm/rmap.c
>>> +++ b/mm/rmap.c
>>> @@ -2333,9 +2333,9 @@ 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;
>>> +                               flags &= ~TTU_SPLIT_HUGE_PMD;
>>> +                               page_vma_mapped_walk_restart(&pvmw);
>>> +                               continue;
>>>                         }
>>>  #ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
>>>                         pmdval = pmdp_get(pvmw.pmd);
>>
>> Yes, it works and definitely needs a comment like "After split_huge_pmd_locked(), restart
>> the walk to detect PageAnonExclusive handling failure in __split_huge_pmd_locked()".
>> The change is good for backporting, but an additional patch to fix it properly by adding
>> a return value to split_huge_pmd_locked() is also necessary.
>>
>
> If my understanding is correct, this approach is good for backporting.
>
> And yes, we could further improve it by return a value to indicate whether
> split_huge_pmd_locked() do split to migration entry.
>
> Thanks both for your thoughtful inputs.

Are you going to send two patches in a series, one is the above fix with a comment
and the other changes split_huge_pmd_locked() to return a value?

Best Regards,
Yan, Zi