Re: [PATCH] mm/hugetlb: avoid recursive i_mmap_rwsem in PMD sharing

[David Hildenbrand (Arm)]

On 10/15/26 04:40, Li Zhe wrote:
> On 10/14/26 6:52 PM, David Hildenbrand (Arm) wrote:
>> On 10/14/26 08:39, Li Zhe wrote:
>>> Hi David,
>>>
>>> I thought more about the case where the destination page table was
>>> preallocated as a shared PMD.
>>>
>>> I do not think the current move_huge_pte() handles that case very well. The
>>> reason is that it assumes the destination PTE is not part of a shared PMD
>>> page table: it clears the source PTE and then installs that PTE into the
>>> destination. That works when the destination PMD page table is not shared,
>>> because the source removal and destination install have a net-zero effect on
>>> rmap and hugetlb accounting.
>>>
>>> If the destination PTE belongs to a shared PMD, that assumption does not
>>> hold. In that case, I think we should leave the shared destination page
>>> table untouched and treat the operation as a source-side unmap. Otherwise
>>> move_huge_pte() either writes through a shared page table or skips the
>>> source-side unmap accounting. That can make the rmap, hugetlb count and
>>> folio release handling inconsistent with the actual PTE update.
>>>
>>> So I think the change should be roughly:
>> IIUC, what you are saying is:
>>
>> A) If the source table is shared, unshare it.
>>
>> b) If the destination table is shared, there is nothing to do.
>>
>> ?
> 
> Yes, mostly.
> 
> If the source PMD table is shared, huge_pmd_unshare() already drops that
> shared PMD table from the source side and we skip moving individual PTEs
> from that table.
> 
> If the source PTE is not in a shared PMD table, but the destination PTE
> is in a shared PMD table, then I think we should not install the source
> PTE into the destination page table. The destination mapping is already
> represented by the shared PMD page table.
> 
> However, move_huge_pte() still has to handle the source side as an
> unmap: clear the source PTE, record the TLB removal, and update the
> source-side hugetlb accounting/rmap state. Otherwise the source mapping
> is removed from the page table without the matching source-side
> accounting. I was able to reproduce this as:
> 
>    BUG: Bad page cache in process init
>    page dumped because: still mapped when deleted
> 
> when keeping the preallocation approach but dropping the special handling
> for a destination PMD-shared page table.
> 
> So my current understanding is:
> 
>    1. source PMD shared: unshare it and skip the PMD-sized range;
>    2. destination PMD shared: do not modify the destination PTE, but
>       still clear and account the source PTE removal.
Yes, I think this makes a lot of sense. Given that this issue has been
in the works for more than one month without a fix I tried to see how to do
it cleaner than your version:

This is what I came up with quickly, using existing helpers by refactoring them.

Is this what you had in mind? I didn't actually test this yet. If it makes sense,
could you test it and possibly clean it up further?

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index fb51f7d02cebf..0c3aebea7a2f8 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -124,6 +124,9 @@ static int __huge_pmd_unshare(struct mmu_gather *tlb,
 		bool check_locks);
 static void hugetlb_unshare_pmds(struct vm_area_struct *vma,
 		unsigned long start, unsigned long end, bool take_locks);
+static void hugetlb_unmap_range(struct mmu_gather *tlb,
+		struct vm_area_struct *vma, unsigned long start, unsigned long end,
+		struct folio *folio, zap_flags_t zap_flags);
 static struct resv_map *vma_resv_map(struct vm_area_struct *vma);
 
 static inline bool subpool_is_free(struct hugepage_subpool *spool)
@@ -5172,6 +5175,8 @@ int move_hugetlb_page_tables(struct vm_area_struct *vma,
 	for (; old_addr < old_end; old_addr += sz, new_addr += sz) {
 		const unsigned long offset_to_last_entry =
 			(old_addr | last_addr_mask) - old_addr;
+		const unsigned long offset_to_last_dst_entry =
+			(new_addr | last_addr_mask) - new_addr;
 
 		src_pte = hugetlb_walk(vma, old_addr, sz);
 		if (!src_pte) {
@@ -5188,6 +5193,23 @@ int move_hugetlb_page_tables(struct vm_area_struct *vma,
 			continue;
 		}
 
+		/*
+		 * Don't try to install a shared PMD table and move stuff into
+		 * them. Instead, just zap the source PMDs and share the PMD
+		 * table later lazily.
+		 */
+		if (sz == PMD_SIZE && want_pmd_share(new_vma, new_addr)) {
+			const unsigned long unmap_size = min(old_end - old_addr,
+							     offset_to_last_dst_entry + sz);
+
+			hugetlb_unmap_range(&tlb, vma, old_addr,
+					    old_addr + unmap_size, NULL,
+					    ZAP_FLAG_DROP_MARKER);
+			old_addr += unmap_size - sz;
+			new_addr += unmap_size - sz;
+			continue;
+		}
+
 		dst_pte = huge_pte_alloc(mm, new_vma, new_addr, sz);
 		if (!dst_pte)
 			break;
@@ -5207,9 +5229,9 @@ int move_hugetlb_page_tables(struct vm_area_struct *vma,
 	return len + old_addr - old_end;
 }
 
-void __unmap_hugepage_range(struct mmu_gather *tlb, struct vm_area_struct *vma,
-			    unsigned long start, unsigned long end,
-			    struct folio *folio, zap_flags_t zap_flags)
+static void hugetlb_unmap_range(struct mmu_gather *tlb,
+		struct vm_area_struct *vma, unsigned long start, unsigned long end,
+		struct folio *folio, zap_flags_t zap_flags)
 {
 	struct mm_struct *mm = vma->vm_mm;
 	const bool folio_provided = !!folio;
@@ -5227,13 +5249,6 @@ void __unmap_hugepage_range(struct mmu_gather *tlb, struct vm_area_struct *vma,
 	BUG_ON(start & ~huge_page_mask(h));
 	BUG_ON(end & ~huge_page_mask(h));
 
-	/*
-	 * This is a hugetlb vma, all the pte entries should point
-	 * to huge page.
-	 */
-	tlb_change_page_size(tlb, sz);
-	tlb_start_vma(tlb, vma);
-
 	last_addr_mask = hugetlb_mask_last_page(h);
 	address = start;
 	for (; address < end; address += sz) {
@@ -5362,6 +5377,15 @@ void __unmap_hugepage_range(struct mmu_gather *tlb, struct vm_area_struct *vma,
 		if (folio_provided)
 			break;
 	}
+}
+
+void __unmap_hugepage_range(struct mmu_gather *tlb, struct vm_area_struct *vma,
+			    unsigned long start, unsigned long end,
+			    struct folio *folio, zap_flags_t zap_flags)
+{
+	tlb_change_page_size(tlb, huge_page_size(hstate_vma(vma)));
+	tlb_start_vma(tlb, vma);
+	hugetlb_unmap_range(tlb, vma, start, end, folio, zap_flags);
 	tlb_end_vma(tlb, vma);
 
 	huge_pmd_unshare_flush(tlb, vma);
-- 
2.43.0



-- 
Cheers,

David