Re: [PATCH] mm/rmap: don't discard lazyfree THPs at PMD level

[Zi Yan <ziy@nvidia.com>]

On 12 Oct 2026, at 8:07, Lorenzo Stoakes (ARM) wrote:

> On Fri, Oct 09, 2026 at 10:08:18PM -0400, Zi Yan wrote:
>> __discard_anon_folio_pmd_locked() clears a lazyfree THP PMD before it knows
>> whether the folio can be discarded, and restores it if the folio was
>> redirtied or has extra references. A concurrent munmap() or
>> MREMAP_DONTUNMAP skips the temporary none PMD and unlinks the VMA from its
>> anon_vma, so the folio stays mapped after the anon_vma is freed and a later
>
> Yeah this seems pretty horrible, instating a page table entry under the rmap
> lock is pretty gross in general, is there anywhere else where we actually
> _install_ (even if 'reinstate') page table entries under the rmap lock?
>
> I think this commit message needs to be clearer/more detailed though, I've been
> sat here working this through in my head for a while.

Sorry for being vague here, let me clarify it below.

>
> I'm confused about the locking, which is really the key point about this change
> isn't it?
>
> unlink_anon_vmas() takes the anon rmap lock so that can't happen concurrently,
> so is it _just_ the case that:
>
> - The folio is not properly freed/unmapped and ends up with a dangling anon_vma
>   pointer
>
> In which case, nobody can actually access that folio since it's not mapped
> anywhere, or is it the far more terrible case of the above and:
>
> - set_pmd_at() in __discard_anon_folio_pmd_locked() reinstates the pmd entry
>   regardless of the fact that the VMA and surrounding page tables could go away
>   concurrently
>
> ?
>
> But I'm confused here because unlink_anon_vmas() takes the anon_vma lock?
>
> EDIT: See below, I figured out what it was.
>
> Looking at the code:
>
> <anon_vma lock>
> [reclaim]
> < PMD lock >
> -> try_to_unmap_one()
>     -> unmap_huge_pmd_locked()
>       -> __discard_anon_folio_pmd_locked()
>         [ clears PMD entry ]

The PMD becomes pmd_none().

> 	...

There are two readers making decisions based on “pmd_none()” locklessly:

1. munmap case is via zap_pmd_range() in mm/memory.c: it skips pmd_none(),
so the above folio is not zapped;

2. MREMAP_DONTUNMAP case is via move_page_tables() in mm/mremap.c calls
get_old_pmd(): get_old_pmd() returns NULL for pmd_none(),
so move_page_tables() skips the above folio.

> 	[ reinstates PMD entry later ]


1. for munmap, unmap_region() calls free_pgtables(). In free_pmd_range(),
the restored PMD is treated as pmd_bad() and cleared. And the folio’s
rmap is not removed.

2. for meremap, dontunmap_complete() unlinks anon_vma but the source VMA
still maps the folio.

>
> Concurrent munmap():
>
> <mmap write lock, downgraded to mmap read lock later>
> do_vmi_align_munmap()
>   -> vms_complete_munmap_vmas()
>     -> vms_clear_ptes()
>       -> unmap_region()
>         -> unmap_vmas() < zaps > <----- EDIT: the real issue, see below
> 	-> free_pgtables()
> 	   -> unlink_anon_vmas()
> 	     -> < anon_vma lock taken! >
> 	   ... clears down page tables (no rmap lock) ...
>
> Is the issue actually the zapping in unmap_vmas(), which doesn't seem to be
> protected by rmap lock at all, meaning that _that_ can happen with
> concurrent munmap()?

No. zap and rmap walkers are serialized by PTL. The reason is described above.

>
> So unmap_vmas() doesn't correctly populate the mmu_gather struct for the
> range, meaning that the underlying page isn't freed and the refcount,
> mapcount etc. are all left as they were?

Yes to the underlying page isn’t freed.

>
> MREMAP_DONTUNMAP seems a bit of a bizarre callout, but I suppose it is the
> other case in which (under certain, likely, circumstances
> e.g. non-partial/self-merging remap) an unlink_anon_vmas()'s call is done.
>
> With normal mremap() that'd happen via an unmap of the original VMA.
>
>> rmap walk uses the freed anon_vma.
>>
>> Using an invalidated PMD instead of a cleared one requires additional arch
>> code fixes. Instead, disable the PMD level discard of lazyfree THPs, as
>> before commit 735ecdfaf4e8 ("mm/vmscan: avoid split lazyfree THP during
>> shrink_folio_list()").
>
> Again it feels like this commit message assumes a bunch of information that
> isn't presented here.

1. SPARC has thp_pte_count-- in its pmdp_invalidate(), but set_pmd_at()
does not add it back, since the code assumes pmdp_invalidate() is only
used in THP splitting code.

2. GUP-fast accepts an invalidate PMD on s390, risk, loongarch. They
need to fix it to prevent GUP-fast from succeeding for this new
pmdp_invalidate().

>
> Why would you need to invalidate the PMD? Is the issue actually TLB flush?
> How does that get around the fact that a PMD entry can be 'reinstated'
> concurrent with a zap?
>
> Or is it that you put something not-pmd_none() there? Hmm yeah I think so.
>
> Doesn't all this cause pmd_none_or_clear_bad() to fire too?
>
> All-round terrible.
>
> I've not dug into the reports in detail, but I think the commit message
> should also talk about what exactly is missing from pmdp_invalidate() that
> is broken here? Are some arches not doing things quite right?
>
> OK reading the code more and it seems the _real_ issue is actually
> pmd_is_huge() in zap_pmd_range() right?
>
> It's the -> pmd_none() in general. By doing that, you are breaking a
> contract with PMD leaf entries.
>
> So you get:
>
> zap_pmd_range()
>   -> pmd_is_huge() ? [ no ]
>      -> zap_huge_pmd() [ WOULD HAVE taken PMD lock that blocks against reclaim ]
>
> So using something like pmdp_invalidate() avoids this, because it keeps the
> entry huge but invalid.
>
> But I guess not every arch does this properly somehow?
>
>>
>> Fixes: 735ecdfaf4e8 ("mm/vmscan: avoid split lazyfree THP during shrink_folio_list()")
>> Reported-by: Kyle Zeng <kylebot@openai.com>
>> Closes: https://lore.kernel.org/r/20261009165214.40212-2-kylebot@openai.com
>> Cc: stable@vger.kernel.org
>> Assisted-by: LLM
>> Signed-off-by: Zi Yan <ziy@nvidia.com>
>
> With the commit message made a lot clearer and calling out the specific
> locking scenarios that allow this + getting rid of the dead code:
>
> Reviewed-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>

Thanks.

>
>> ---
>> The proposed fix[1] requires additional arch code fixes[2]. Disable it
>> first for stable fix. The function will be re-enabled along with arch code
>> fixes.
>>
>> Link: https://lore.kernel.org/all/20261009165214.40212-2-kylebot@openai.com/ [1]
>> Link: https://lore.kernel.org/all/DM0II2QHWWS1.2RXEJXC8S0101@nvidia.com/ [2]
>> ---
>>  mm/rmap.c | 11 -----------
>>  1 file changed, 11 deletions(-)
>>
>> diff --git a/mm/rmap.c b/mm/rmap.c
>> index 805db93fe0428..1131b76bbbc28 100644
>> --- a/mm/rmap.c
>> +++ b/mm/rmap.c
>> @@ -2275,17 +2275,6 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>>  		}
>>
>>  		if (!pvmw.pte) {
>> -			if (folio_test_lazyfree(folio)) {
>> -				if (unmap_huge_pmd_locked(vma, pvmw.address, pvmw.pmd, folio))
>
> This is the only caller of unmap_huge_pmd_locked(), making that and
> __discard_anon_folio_pmd_locked() dead code, shouldn't we remove all of it?
>
> Otherwise we're leaving dead code around.

1) it keeps the backport small, 2) I am going to restore the function and
this reduce the code churn.

I can remove the dead code if you think that is better.

>
>
>> -					goto walk_done;
>> -				/*
>> -				 * unmap_huge_pmd_locked has either already marked
>> -				 * the folio as swap-backed or decided to retain it
>> -				 * due to GUP or speculative references.
>> -				 */
>> -				goto walk_abort;
>> -			}
>> -
>>  			if (flags & TTU_SPLIT_HUGE_PMD) {
>>  				/*
>>  				 * We temporarily have to drop the PTL and
>>
>> ---
>> base-commit: 8b38ed9ab5b09c8ba168cbcc49524e9b380ee5c4
>> change-id: 20261009-lazyfree-stable-fix-67712b2049dd
>>
>> Best regards,
>> --
>> Yan, Zi
>>
>
> --
> Cheers, Lorenzo


Best Regards,
Yan, Zi