Re: [PATCH] mm/hugetlb: fix overbroad MMU notifiers for unshared PMDs

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

On 9/10/26 12:59 AM, David Hildenbrand (Arm) wrote:
> On 9/8/26 09:09, Li Zhe wrote:
>> On 9/7/26 11:29 PM, David Hildenbrand (Arm) wrote:
>>> On 9/1/26 02:32, Andrew Morton wrote:
>>>> Thanks.
>>>>
>>>> So a modest performance improvement?
>>> Is that worth the complexity, though?
>>
>> That is a fair concern.
> In you setup, are the page tables ever being shared? I suspect you just run a VM
> with no other processes actually sharing the memory?


Yes. In the tested setup, there is only one QEMU process mapping the
hugetlbfs file for the VM. The hugetlb page tables are not actually shared.

>
> One idea would be to just remember whether any sharing ever happened for a
> hugetlb file.


I reworked the patch based on your suggestion. Instead of walking the
page tables during unmap, this version records, per hugetlbfs inode,
whether PMD sharing has ever actually been established for the file. The
flag is set only after huge_pmd_share() successfully populates a shared
PMD table.

For the hugetlbfs zap/unmap paths, the conservative PUD-sized MMU
notifier expansion is skipped while the file has never seen PMD
sharing. Once PMD sharing has ever happened for the file, the code falls
back to the existing conservative behavior.

>
>> I tried to simplify the approach. Instead of computing the exact PUD
>> sub-ranges that contain shared PMD tables, this version keeps the
>> existing adjust_range_if_pmd_sharing_possible() logic unchanged and only
>> adds an actual shared-PMD check as a gate before it.
>>
>> So the behavior becomes:
>>
>>     - if no shared PMD table is found, keep the notifier range unchanged;
>>     - if any shared PMD table is found, fall back to the existing
>>       conservative PUD-sized expansion.
>>
>> This should still avoid the unnecessary 1G KVM invalidation for the
>> common unshared-PMD case, while keeping the range expansion policy
>> unchanged when a shared PMD is present.
>>
>> The resulting diff would look like this:
>>
>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> index 816dde7..7a61fc3 100644
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
>> @@ -5355,10 +5357,12 @@ void __hugetlb_zap_begin(struct vm_area_struct *vma,
>>        if (!vma->vm_file)    /* hugetlbfs_file_mmap error */
>>            return;
>>
>> -    adjust_range_if_pmd_sharing_possible(vma, start, end);
>>        hugetlb_vma_lock_write(vma);
>> -    if (vma->vm_file)
>> +    if (vma->vm_file) {
>>            i_mmap_lock_write(vma->vm_file->f_mapping);
>> +        if (range_has_shared_pmd(vma, *start, *end))
>> +            adjust_range_if_pmd_sharing_possible(vma, start, end);
>> +    }
>>    }
>>
>>    void __hugetlb_zap_end(struct vm_area_struct *vma,
>> @@ -5397,7 +5401,9 @@ void unmap_hugepage_range(struct vm_area_struct
>> *vma, unsigned long start,
>>
>>        mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma->vm_mm,
>>                    start, end);
>> -    adjust_range_if_pmd_sharing_possible(vma, &range.start, &range.end);
>> +    if (range_has_shared_pmd(vma, range.start, range.end))
>> +        adjust_range_if_pmd_sharing_possible(vma, &range.start,
>> +                             &range.end);
>>        mmu_notifier_invalidate_range_start(&range);
>>        tlb_gather_mmu(&tlb, vma->vm_mm);
>>
>> @@ -6958,6 +6964,52 @@ void adjust_range_if_pmd_sharing_possible(struct
>> vm_area_struct *vma,
>>            *end = ALIGN(*end, PUD_SIZE);
>>    }
>>
>> +static bool range_has_shared_pmd(struct vm_area_struct *vma,
>> +                 unsigned long start, unsigned long end)
>> +{
>> +    unsigned long v_start = ALIGN(vma->vm_start, PUD_SIZE);
>> +    unsigned long v_end = ALIGN_DOWN(vma->vm_end, PUD_SIZE);
>> +    struct hstate *h = hstate_vma(vma);
>> +    struct mm_struct *mm = vma->vm_mm;
>> +    unsigned long address;
>> +
>> +    if (huge_page_size(h) != PMD_SIZE)
>> +        return false;
>> +
>> +    /*
>> +     * First apply the same cheap test as
>> +     * adjust_range_if_pmd_sharing_possible(). Only the PUD-aligned
>> +     * intersection can contain shared PMD tables.
>> +     */
>> +    if (!(vma->vm_flags & VM_MAYSHARE) || !(v_end > v_start) ||
>> +        end <= v_start || start >= v_end)
>> +        return false;
>> +
>> +    start = max(ALIGN_DOWN(start, PUD_SIZE), v_start);
>> +    end = min(ALIGN(end, PUD_SIZE), v_end);
>> +
>> +    hugetlb_vma_assert_locked(vma);
>> +    i_mmap_assert_write_locked(vma->vm_file->f_mapping);
>> +
>> +    for (address = start; address < end; address += PUD_SIZE) {
>> +        pte_t *ptep;
>> +        bool shared;
>> +
>> +        ptep = hugetlb_walk(vma, address, PMD_SIZE);
>> +        if (!ptep)
>> +            continue;
>> +
>> +        spin_lock(huge_pte_lockptr(h, mm, ptep));
>> +        shared = ptdesc_pmd_is_shared(virt_to_ptdesc(ptep));
>> +        spin_unlock(huge_pte_lockptr(h, mm, ptep));
> I really don't like this piece of code to optimize something that is already
> questionable in practice: overcommiting hugetlb folios for VMs.
>
> Can you share some more details which mechanism ends up zapping hugetlb folios
> for the VM?
>
> If it's virtio-balloon's free-page-reporting, you should likely disable that for
> the VM.


It is not virtio-balloon free-page-reporting.

The setup uses a downstream, out-of-tree QEMU/KVM reclaim mechanism for
hugetlb-backed VM memory. KVM maintains hot/cold information for guest
memory. QEMU then asks KVM to select cold guest ranges and releases the
corresponding hugetlbfs-backed host ranges after taking care of the
required device-side unmapping.

The release path uses PUNCH_HOLE on the hugetlbfs file for 2M hugetlb
pages.

This is the case where the current conservative range expansion becomes
unnecessarily broad.

Does this patch look reasonable to you?

The updated patch looks like this:

---
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 7611a84..1e24b8d 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -921,6 +921,9 @@ static struct inode *hugetlbfs_get_inode(struct 
super_block *sb,
          simple_inode_init_ts(inode);
          info->resv_map = resv_map;
          info->seals = F_SEAL_SEAL;
+#ifdef CONFIG_HUGETLB_PMD_PAGE_TABLE_SHARING
+        info->pmd_sharing_seen = false;
+#endif
          switch (mode & S_IFMT) {
          default:
              init_special_inode(inode, mode, dev);
diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index 16c4c4c..8f1f899 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -509,6 +509,9 @@ struct hugetlbfs_inode_info {
      struct inode vfs_inode;
      struct resv_map *resv_map;
      unsigned int seals;
+#ifdef CONFIG_HUGETLB_PMD_PAGE_TABLE_SHARING
+    bool pmd_sharing_seen;
+#endif
  };

  static inline struct hugetlbfs_inode_info *HUGETLBFS_I(struct inode 
*inode)
@@ -516,6 +519,27 @@ static inline struct hugetlbfs_inode_info 
*HUGETLBFS_I(struct inode *inode)
      return container_of(inode, struct hugetlbfs_inode_info, vfs_inode);
  }

+#ifdef CONFIG_HUGETLB_PMD_PAGE_TABLE_SHARING
+static inline void hugetlbfs_set_pmd_sharing_seen(struct inode *inode)
+{
+    HUGETLBFS_I(inode)->pmd_sharing_seen = true;
+}
+
+static inline bool hugetlbfs_pmd_sharing_seen(struct inode *inode)
+{
+    return HUGETLBFS_I(inode)->pmd_sharing_seen;
+}
+#else
+static inline void hugetlbfs_set_pmd_sharing_seen(struct inode *inode)
+{
+}
+
+static inline bool hugetlbfs_pmd_sharing_seen(struct inode *inode)
+{
+    return false;
+}
+#endif
+
  extern const struct vm_operations_struct hugetlb_vm_ops;
  struct file *hugetlb_file_setup(const char *name, size_t size, 
vma_flags_t acct,
                  int creat_flags, int page_size_log);
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 7857728..e370960 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -5359,10 +5359,12 @@ void __hugetlb_zap_begin(struct vm_area_struct *vma,
      if (!vma->vm_file)    /* hugetlbfs_file_mmap error */
          return;

-    adjust_range_if_pmd_sharing_possible(vma, start, end);
      hugetlb_vma_lock_write(vma);
-    if (vma->vm_file)
+    if (vma->vm_file) {
          i_mmap_lock_write(vma->vm_file->f_mapping);
+        if (hugetlbfs_pmd_sharing_seen(file_inode(vma->vm_file)))
+            adjust_range_if_pmd_sharing_possible(vma, start, end);
+    }
  }

  void __hugetlb_zap_end(struct vm_area_struct *vma,
@@ -5401,7 +5403,9 @@ void unmap_hugepage_range(struct vm_area_struct 
*vma, unsigned long start,

      mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma->vm_mm,
                  start, end);
-    adjust_range_if_pmd_sharing_possible(vma, &range.start, &range.end);
+    if (hugetlbfs_pmd_sharing_seen(file_inode(vma->vm_file)))
+        adjust_range_if_pmd_sharing_possible(vma, &range.start,
+                             &range.end);
      mmu_notifier_invalidate_range_start(&range);
      tlb_gather_mmu(&tlb, vma->vm_mm);

@@ -7004,6 +7008,7 @@ pte_t *huge_pmd_share(struct mm_struct *mm, struct 
vm_area_struct *vma,
      if (pud_none(*pud)) {
          pud_populate(mm, pud,
                  (pmd_t *)((unsigned long)spte & PAGE_MASK));
+        hugetlbfs_set_pmd_sharing_seen(mapping->host);
          mm_inc_nr_pmds(mm);
      } else {
          ptdesc_pmd_pts_dec(virt_to_ptdesc(spte));
-- 


Thanks,
Zhe

>