Re: [PATCH] mm: synchronize with PTE table reclaim for all folio sizes

[Baolin Wang <baolin.wang@linux.alibaba.com>]

On 11/6/26 5:29 PM, Lance Yang wrote:
> 
> 
> On 2026/11/6 17:15, Lance Yang wrote:
>>
>> On Fri, Nov 06, 2026 at 04:54:44PM +0800, Baolin Wang wrote:
>>>
>>>
>>> On 11/6/26 2:24 PM, Barry Song wrote:
>>>> On Thu, Nov 5, 2026 at 5:00 PM Baolin Wang
>>>> <baolin.wang@linux.alibaba.com> wrote:
>>>>>
>>>>> CC Barry, who added the TTU_SYNC in shrink_folio_list().
>>>>>
>>>>> On 11/5/26 1:48 PM, Lance Yang wrote:
>>>>>>
>>>>>>
>>>>>> On 2026/11/5 12:54, Lance Yang wrote:
>>>>>>>
>>>>>>> On Thu, Nov 05, 2026 at 10:56:28AM +0800, Baolin Wang wrote:
>>>>>>>> Similar to commit 243cf1f67b36 ("mm: memory: fix truncation 
>>>>>>>> removing
>>>>>>>> mapped
>>>>>>>> folios"), the '!pmd_present()' check in page_vma_mapped_walk() can
>>>>>>>> also race
>>>>>>>> with concurrent page table reclamation via zap_empty_pte_table().
>>>>>>>
>>>>>>> Good catch!
>>>>>>>
>>>>>>>> page_vma_mapped_walk() is used by many functions including
>>>>>>>> try_to_unmap(),
>>>>>>>> try_to_migrate(), folio_mkclean(), pfn_mkclean_range(),
>>>>>>>> remove_migration_ptes(),
>>>>>>>> page_mapped_in_vma(), and mapping_wrprotect_range(). Except for
>>>>>>>> try_to_unmap()
>>>>>>>> and try_to_migrate(), the other callers do not perform post-walk
>>>>>>>> accounting
>>>>>>>> checks, so the race has no observable impact on them.
>>>>>>>>
>>>>>>>> For migration, the race can cause unnecessary retries. For 
>>>>>>>> reclaim via
>>>>>>>> try_to_unmap(), the race can cause failure to reclaim a folio 
>>>>>>>> that is in
>>>>>>>> fact already unmapped (as shown below), which may lead to 
>>>>>>>> unexpected OOM
>>>>>>>> or increased refaults.
>>>>>>>>
>>>>>>>> CPU 0 (reclaim)                      CPU 1 (MADV_DONTNEED)
>>>>>>>> --------------------------------    
>>>>>>>> --------------------------------
>>>>>>>> folio_lock(F)
>>>>>>>> try_to_unmap(F, TTU_SYNC)
>>>>>>>>                                       zap_pte_range(full PMD range)
>>>>>>>>                                         spin_lock(ptl)
>>>>>>>>                                         clear PTEs
>>>>>>>>                                         zap_empty_pte_table()
>>>>>>>>                                           spin_trylock(pml)
>>>>>>>>                                           pmd_clear()
>>>>>>>>                                           spin_unlock(pml)
>>>>>>>>
>>>>>>>>     page_vma_mapped_walk()
>>>>>>>>       observe !pmd_present(pmde)
>>>>>>>>       nr_pages < HPAGE_PMD_NR
>>>>>>>>       skip synchronization
>>>>>>>>       return false
>>>>>>>>
>>>>>>>> try_to_unmap() returns
>>>>>>>> folio_mapped(F) == true
>>>>>>>> nr_unmap_fail += folio_nr_pages(F)
>>>>>>>>                                       tlb_flush_mmu_tlbonly()
>>>>>>>>                                       tlb_flush_rmaps()
>>>>>>>>                                         remove F's rmaps
>>>>>>>>                                       spin_unlock(ptl)
>>>>>>>>
>>>>>>>> Commit 243cf1f67b36 ("mm: memory: fix truncation removing mapped
>>>>>>>> folios")
>>>>>>>> has already moved the PMD clear operation (via 
>>>>>>>> zap_empty_pte_table()) to
>>>>>>>> after tlb_flush_rmaps(). Similarly, page_vma_mapped_walk() also 
>>>>>>>> needs to
>>>>>>>> use sync_with_folio_pmd_zap() to synchronize and avoid memory 
>>>>>>>> ordering
>>>>>>>> issues on weakly ordered CPUs.
>>>>>>>>
>>>>>>>> Fixes: 6375e95f381e ("mm: pgtable: reclaim empty PTE page in
>>>>>>>> madvise(MADV_DONTNEED)")
>>>>>>>> Cc: stable@vger.kernel.org
>>>>>>>> Suggested-by: David Hildenbrand (Arm) <david@kernel.org>
>>>>>>>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>>>>>>>> ---
>>>>>>>> mm/page_vma_mapped.c | 5 +----
>>>>>>>> 1 file changed, 1 insertion(+), 4 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c
>>>>>>>> index e6b6170c2f4f..8797467b6f0c 100644
>>>>>>>> --- a/mm/page_vma_mapped.c
>>>>>>>> +++ b/mm/page_vma_mapped.c
>>>>>>>> @@ -293,10 +293,7 @@ bool page_vma_mapped_walk(struct
>>>>>>>> page_vma_mapped_walk *pvmw)
>>>>>>>>               spin_unlock(pvmw->ptl);
>>>>>>>>               pvmw->ptl = NULL;
>>>>>>>>           } else if (!pmd_present(pmde)) {
>>>>>>>> -            if ((pvmw->flags & PVMW_SYNC) &&
>>>>>>>> -                thp_vma_suitable_order(vma, pvmw->address,
>>>>>>>> -                           PMD_ORDER) &&
>>>>>>>> -                (pvmw->nr_pages >= HPAGE_PMD_NR))
>>>>>>>> +            if (pvmw->flags & PVMW_SYNC)
>>>>>>>
>>>>>>> Right. Whether the folio can be PMD-mapped doesn't tell us 
>>>>>>> whether its
>>>>>>> PTE table can be reclaimed, so those extra checks can skip
>>>>>>> synchronization
>>>>>>> that the caller explicitly asked for ;)
>>>>>>
>>>>>> Ouch ... one thing I missed ... what about order-0 reclaim?
>>>>>>
>>>>>> shrink_folio_list() only sets TTU_SYNC for large folio, so can't 
>>>>>> we still
>>>>>> hit the same race there?
>>>>>
>>>>> Thanks Lance. But this matches the purpose of the TTU_SYNC flag, which
>>>>> is used to avoid racy checks with PVMW_SYNC. If no TTU_SYNC flag is 
>>>>> set,
>>>>> it means the caller can tolerate some racy checks.
>>>>>
>>>>> For this case, I think we should set TTU_SYNC unconditionally in
>>>>> shrink_folio_list(). Barry, what do you think?
>>>>
>>>> Baolin, thanks for reaching out to me. I added that flag
>>>> mainly for large folios.
>>>>
>>>> I observed cases where PTEs for large folios are partially
>>>> cleared during certain transition states, for example, when
>>>> PTEs are cleared before being modified:
>>>>
>>>> pte = ptep_get_and_clear();
>>>> pte modification;
>>>> set_pte_at();
>>>>
>>>> In such cases, page_vma_mapped_walk() skips the empty
>>>> entries and starts from the next valid one.
>>>>
>>>> This may result in partially unmapping PTEs for large folios
>>>> and trigger unexpected folio splits. At the time, I was
>>>> particularly concerned about unnecessary folio splitting,
>>>> so my focus was mainly on large folios.
>>>
>>> Thanks for the information.
>>>
>>>> However, I also noticed that a similar race can occur with
>>>> small folios. In that case, the folio may simply remain
>>>> unreclaimed by the reclamation path.
>>>>
>>>> However, my main concern at the time was avoiding
>>>> unnecessary folio splits.
>>>>
>>>> Your new race also seems to amplify the same situation.
>>>> However, I don't think this problem necessarily depends on
>>>> the commit you mentioned. It has been around for a long time.
>>>
>>> Agree.
>>>
>>>> If the concern is reclaim failure, I agree that there is no
>>>> fundamental difference between large folios and small folios.
>>>
>>> OK. And I did a quick measurement to reclaim anon and file folios via
>>> the memory.reclaim interface.
>>>
>>>  From test results, reclaiming 1G small anonymous folios (using zram as
>>> swap) shows no significant performance change. However, reclaiming 1G
>>> mapped clean small file folios does show an observable regression if the
>>> TTU_SYNC flag is set.
>>>
>>> Without TTU_SYNC for small folios:
>>> time echo 1G > /sys/fs/cgroup/test/memory.reclaim
>>> real    0m0.178s
>>> user    0m0.000s
>>> sys    0m0.178s
>>>
>>> With TTU_SYNC for small folios:
>>> real    0m0.315s
>>> user    0m0.000s
>>> sys    0m0.314s
>>
>> Nice numbers, cool!
>>
>>> So for the current patch, I'd prefer to keep things as is: only sync
>>> when TTU_SYNC is set. Without setting the TTU_SYNC flag, keep the
>>> original logic, meaning the caller can tolerate some racy checks.
>>>
>>> Any other thoughts?
>>
>> One thing I want to pin down first, for the order-0 folios, was that run
>> with TTU_SYNC going through the pmd lock, or did it already go the
>> smp_rmb() way?
>>
>> My guess is the first one, right?

Right. I didn't add the memory barriers optimization (will do in a 
follow-up). The test changes are as follows:
diff --git a/mm/vmscan.c b/mm/vmscan.c
index daa7e37c2236..6a95a4fdc64c 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1434,11 +1434,6 @@ static unsigned int shrink_folio_list(struct 
list_head *folio_list,
                  * processes. Try to unmap it here.
                  */
                 if (folio_mapped(folio)) {
-                       enum ttu_flags flags = TTU_BATCH_FLUSH;
-                       bool was_swapbacked = folio_test_swapbacked(folio);
-
-                       if (folio_test_pmd_mappable(folio))
-                               flags |= TTU_SPLIT_HUGE_PMD;
                         /*
                          * Without TTU_SYNC, try_to_unmap will only 
begin to
                          * hold PTL from the first present PTE within a 
large
@@ -1451,8 +1446,11 @@ static unsigned int shrink_folio_list(struct 
list_head *folio_list,
                          * try_to_unmap acquire PTL from the first PTE,
                          * eliminating the influence of temporary PTE 
values.
                          */
-                       if (folio_test_large(folio))
-                               flags |= TTU_SYNC;
+                       enum ttu_flags flags = TTU_BATCH_FLUSH | TTU_SYNC;
+                       bool was_swapbacked = folio_test_swapbacked(folio);
+
+                       if (folio_test_pmd_mappable(folio))
+                               flags |= TTU_SPLIT_HUGE_PMD;

                         try_to_unmap(folio, flags);
                         if (folio_mapped(folio)) {

>> If so, please keep the fix as is and only sync when the caller asks for
>> it (at least, order-0 folios should not go the pmd lock way) :D
> 
> In addition:
> 
> For order-0 folios, we'd miss reclaim and reactivate a cold folio.
> 
> That doesn't sound too bad, especially for such a rare race.
> 
> so keeping the current behavior seems reasonable given the sync
> overhead ;)

That's also my thought:)