Re: [PATCH v2] mm: memory: fix truncation removing mapped folios
From: Baolin Wang
Date: Thu Oct 15 2026 - 21:47:33 EST
On 10/16/26 3:00 AM, David Hildenbrand (Arm) wrote:
On 10/15/26 08:41, Baolin Wang wrote:
On 10/14/26 9:25 PM, David Hildenbrand (Arm) wrote:
On 10/13/26 11:36, Baolin Wang wrote:
I had considered this approach before, but it is not feasible. IIUC, the
sync_with_folio_pmd_zap() is used to address the race with zap_huge_pmd(), and
both the PMD clear and the folio mapcount decrement are performed under the PMD
lock in zap_huge_pmd().
Just to be clear, I am thinking about something like the following on the
zap_pmd_range() side:
diff --git a/mm/internal.h b/mm/internal.h
index 38b1165212c94..a8820ad7e2f21 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -584,14 +584,15 @@ void free_pgtables(struct mmu_gather *tlb, struct
unmap_desc *desc);
void pmd_install(struct mm_struct *mm, pmd_t *pmd, pgtable_t *pte);
/**
- * sync_with_folio_pmd_zap - sync with concurrent zapping of a folio PMD
+ * sync_with_folio_pmd_zap - sync with concurrent zapping of folios
* @mm: The mm_struct.
* @pmdp: Pointer to the pmd that was found to be pmd_none().
*
* When we find a pmd_none() while unmapping a folio without holding the PTL,
- * zap_huge_pmd() may have cleared the PMD but not yet modified the folio to
- * indicate that it's unmapped. Skipping the PMD without synchronization could
- * make folio unmapping code assume that unmapping failed.
+ * zap_huge_pmd() or zap_empty_pte_table() may have cleared the PMD but not
+ * yet modified the mapped folios to indicate they are unmapped. Skipping the
+ * PMD without synchronization could make folio unmapping code assume that
+ * unmapping failed.
*
* Wait for concurrent zapping to complete by grabbing the PTL.
*/
diff --git a/mm/memory.c b/mm/memory.c
index 8b0c2c735d3de..c585f5eb255c9 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2139,9 +2139,7 @@ static inline unsigned long zap_pmd_range(struct
mmu_gather *tlb,
continue;
}
/* fall through */
- } else if (details && details->single_folio &&
- folio_test_pmd_mappable(details->single_folio) &&
- next - addr == HPAGE_PMD_SIZE && pmd_none(*pmd)) {
+ } else if (details && details->single_folio && pmd_none(*pmd)) {
sync_with_folio_pmd_zap(tlb->mm, pmd);
}
if (pmd_none(*pmd)) {
Yes, I've considered that. However, implicit memory barriers give me a headache,
so I ended up going with explicit memory barriers (suggested by Pedro).
If you don't like memory barriers, let me re-check whether the implicit memory
barriers here can ensure race safety with sync_with_folio_pmd_zap(). A
simplified logic is as follows:
CPU 0 (truncation) CPU 1 (zapping)
down_read() ->Acquire A1. pte spin_lock() ->Acquire A4
clear mapcount
pmd spin_lock() ->Acquire A5
clear pmd
pmd spin_unlock() ->Release R5
if (pmd_none()) { //read pmd
pte spin_unlock() ->Release R4
pmd spin_lock() ->Acquire A2
pmd spin_unlock() ->Release R2
}
up_read() -> Release R1
spin_lock(y) ->Acquire A3
folio_mapped() // read mapcount
spin_unlock(y) ->Release R3
CPU 0 constraints:
1. R2 is a release, so "read pmd" completes before R2.
2. A2 completes before A3 (ACQUIRE vs ACQUIRE).
3. "read mapcount" completes after A3, hence after A2.
CPU 1 constraints:
1. R5 is a release. Both "clear mapcount" and "clear pmd" complete before R5.
According to above constraints, if CPU0 sees pmd_none() caused by CPU1's clear
pmd, and CPU0 then acquires the same pmd lock released by CPU1 at R5, then
CPU1's clear mapcount happens before CPU0's later folio_mapped().
Therefore folio_mapped() is guaranteed not to read the old mapcount in this
case. So I think sync_with_folio_pmd_zap() can work, but needs a small change to
avoid the double pmd_none() check.
I'd be okay with some (follow-up?) memory barrier handling inside
sync_with_folio_pmd_zap(), for example, based on the folio size and with proper
documentation why we need the PT lock in one case and why the memory barrier is
ok in the other case.
I'm afraid that we can't fit all the memory barriers into sync_with_folio_pmd_zap(). As the current patch does, we need memory barriers on both the writer and reader sides.
David, does the following diff look good to you?
I think we should extend the sync_with_folio_pmd_zap() doc as suggested.
Ack.
diff --git a/mm/memory.c b/mm/memory.c
index 6a8e7772b8d6..c0aadab1b59e 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2036,6 +2036,15 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
}
} while (pte += nr, addr += PAGE_SIZE * nr, addr != end);
+ add_mm_rss_vec(mm, rss);
+ lazy_mmu_mode_disable();
+
+ /* Do the actual TLB flush before dropping ptl */
+ if (force_flush) {
+ tlb_flush_mmu_tlbonly(tlb);
+ tlb_flush_rmaps(tlb, vma);
+ }
+
/*
* Fast path: try to hold the pmd lock and unmap the PTE page.
*
@@ -2044,16 +2053,8 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
* to ensure they are still none, thereby preventing the pte entries
* from being repopulated by another thread.
*/
- if (can_reclaim_pt && direct_reclaim && addr == end)
+ if (can_reclaim_pt && direct_reclaim && addr == end) {
direct_reclaim = zap_empty_pte_table(mm, pmd, ptl, &pmdval);
-
- add_mm_rss_vec(mm, rss);
- lazy_mmu_mode_disable();
-
- /* Do the actual TLB flush before dropping ptl */
- if (force_flush) {
- tlb_flush_mmu_tlbonly(tlb);
- tlb_flush_rmaps(tlb, vma);
}
pte_unmap_unlock(start_pte, ptl);
@@ -2102,12 +2103,11 @@ static inline unsigned long zap_pmd_range(struct
mmu_gather *tlb,
continue;
}
/* fall through */
- } else if (details && details->single_folio &&
- folio_test_pmd_mappable(details->single_folio) &&
- next - addr == HPAGE_PMD_SIZE && pmd_none(*pmd)) {
- sync_with_folio_pmd_zap(tlb->mm, pmd);
}
+
if (pmd_none(*pmd)) {
+ if (details && details->single_folio)
+ sync_with_folio_pmd_zap(tlb->mm, pmd);
addr = next;
continue;
}
We do have another sync_with_folio_pmd_zap() user in page_vma_mapped_walk(),
does that one need care as well?
Good point. But I don't want to add page_vma_mapped_walk() changes into this fix patch. I need to analyze all callers of page_vma_mapped_walk() and the potential races. I'll look into whether it should be changed in a follow-up patch.
Thanks for your comments.