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

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

On Wed, Feb 04, 2026 at 09:43:42PM +0100, David Hildenbrand (arm) wrote:
>On 2/4/26 21:02, Zi Yan wrote:
>> On 4 Feb 2026, at 14:36, David Hildenbrand (arm) wrote:
>> 
>> > Sorry for the late reply. I saw that I was CCed in v1 but I am only now catching up with mails ... slowly but steadily.
>> > 
>> > > 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 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.
>> > 
>> > Ah, that explains magic number 513.
>> > 
>> > > 
>> > > This patch fixes this by restart page_vma_mapped_walk() after
>> > > split_huge_pmd_locked(). Because split_huge_pmd_locked() may fall back to
>> > > (freeze = false) if folio_try_share_anon_rmap_pmd() fails and the PMD is
>> > > just split instead of split to migration entry.
>> > 
>> > Right, but folio_try_share_anon_rmap_pmd() should never fail on the folios that have already been shared? (above you write that it is shared with 512 children)
>> > 
>> > The only case where  folio_try_share_anon_rmap_pmd() could fail would be if the folio would not be shared, and there would only be a single PMD then, so there is nothing you can do -> abort.
>> > 
>> > Returning "false" from try_to_migrate_one() is the real issue, as it makes rmap_walk_anon() to just stop -> abort the walk.
>> > 
>> > 
>> > So I suspect v1 was actually sufficient, or what am I missing where the restart would actually be required?
>> 
>> The explanation is not for the shared case mentioned above. It is for unshared
>> folio. If an unshared folio’s PAE cannot be cleared, try_to_migrate_one() return
>> true, indicating a success.

Thanks Zi Yan for the explanation.

>
>Oh. You mean that should be something like
>
>"This patch fixes this by restart page_vma_mapped_walk() after
>split_huge_pmd_locked(). We cannot simply return "true" to fix the problem,
>as that would affect another case:
>split_huge_pmd_locked()->folio_try_share_anon_rmap_pmd() can failed and leave
>the folio mapped through PTEs; we would return "true" from
>try_to_migrate_one() in that case as well. While that is mostly harmless, we
>could end up walking the rmap, wasting some cycles.".
>

Change log is updated accordingly.

>
>> Yeah, since it is an unshared folio, the return
>> value of try_to_migrate_one() does not matter. This fix makes try_to_migrate_one()
>> return false.
>
>Right, it's not really problematic. We could end up walking the rmap and burn
>some cycles.
>
>> 
>> > 
>> > 
>> > (maybe we should get rid of the usage of booleans here at some point, an enum like abort/continue would have been much clearer)
>> > 
>> > > Restart
>> > > page_vma_mapped_walk() and let try_to_migrate_one() try on each PTE
>> > > again and fail try_to_migrate() early if it fails.
>> > > 
>> > > 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: Lance Yang <lance.yang@linux.dev>
>> > > Cc: <stable@vger.kernel.org>
>> > > 
>> > > ---
>> > > v2:
>> > >     * restart page_vma_mapped_walk() after split_huge_pmd_locked()
>> > > ---
>> > >    mm/rmap.c | 11 ++++++++---
>> > >    1 file changed, 8 insertions(+), 3 deletions(-)
>> > > 
>> > > diff --git a/mm/rmap.c b/mm/rmap.c
>> > > index 618df3385c8b..5b853ec8901d 100644
>> > > --- a/mm/rmap.c
>> > > +++ b/mm/rmap.c
>> > > @@ -2446,11 +2446,16 @@ static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma,
>> > >    			__maybe_unused pmd_t pmdval;
>> > >     			if (flags & TTU_SPLIT_HUGE_PMD) {
>> > > +				/*
>> > > +				 * After split_huge_pmd_locked(), restart the
>> > > +				 * walk to detect PageAnonExclusive handling
>> > > +				 * failure in __split_huge_pmd_locked().
>> > > +				 */
>> > >    				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;
>> > >    			}
>> > 
>> > The change looks more consistent to what we have in try_to_unmap().
>> > 
>> > But the explanation above is not quite right I think. And consequently the comment above as well.
>> > 
>> > PAE being set implies "single PMD" -> unshared.
>> 
>> The commit message might be improved with some additional context. The comment
>> above pairs with the comment in __split_huge_pmd_locked()
>> “In case we cannot clear PageAnonExclusive(), split the PMD
>> only and let try_to_migrate_one() fail later”. What is problem with it?
>
>With your explanation it's much clearer, thanks.
>
>I'd remove some details from the comments about PAE like:
>
>"split_huge_pmd_locked() might leave the folio mapped through PTEs. Retry the
>walk so we can detect this scenario and properly abort the walk."
>

Comment is updated accordingly.

>
>With some clarifications along those lines
>
>Acked-by: David Hildenbrand (arm) <david@kernel.org>
>
>-- 
>Cheers,
>
>David

-- 
Wei Yang
Help you, Help me