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

["Li Zhe" <lizhe.67@bytedance.com>]

On 10/16/26 3:33 AM, David Hildenbrand (Arm) wrote:
> 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);


Hi David,

Your patch looks good to me.

Sashiko pointed out one corner case around UFFDIO_POISON.  In the
destination-PMD-sharing path, the patch zaps the source range and lets
the destination fault path populate or share the PMD lazily.  That works
for normal hugetlb mappings, but if the source PTE contains
PTE_MARKER_POISONED, the marker is not copied to the destination. The
source zap uses ZAP_FLAG_DROP_MARKER, so the marker can be dropped, and a
later fault at the new address would see an empty entry instead of the
poisoned marker.

One possible way to avoid this is to make VM_UFFD_MISSING participate in
the existing uffd_disable_huge_pmd_share() logic, because UFFDIO_POISON
can install PTE_MARKER_POISONED and the poison state is stored in the
PTE, similar to the existing UFFD-WP/RWP per-PTE state.

For example:

---
diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_k.h
index a4351cffc60c..9a057d7531c3 100644
--- a/include/linux/userfaultfd_k.h
+++ b/include/linux/userfaultfd_k.h
@@ -183,8 +187,8 @@ static inline bool 
is_mergeable_vm_userfaultfd_ctx(struct
vm_area_struct *vma,
  static inline bool uffd_disable_huge_pmd_share(struct vm_area_struct *vma)
  {
      return vma_test_any_mask(vma,
-        mk_vma_flags_from_masks(VMA_UFFD_WP, VMA_UFFD_RWP,
-                    VMA_UFFD_MINOR));
+        mk_vma_flags_from_masks(VMA_UFFD_MISSING, VMA_UFFD_WP,
+                    VMA_UFFD_RWP, VMA_UFFD_MINOR));
  }

  /*
---
However, Sashiko also pointed out that this may still be incomplete,
because PTE_MARKER_POISONED can outlive VM_UFFD_MISSING.

I am not very familiar with this part of the userfaultfdsemantics.

Do you have any suggestions for this issue or for this direction?

Thanks,
Zhe