On 10/15/26 16:14, David Hildenbrand (Arm) wrote:
> I would have thought that unmap_mapping_pages() is only called in
> corner cases when we would have concurrent fork().
>
> Like vfio_pci_zap_bars().
>
> But yeah, likely there is a theoretical race.
Agreed, it needs a zap such as vfio_pci_zap_bars() racing with fork()
of the mapping owner. I only hit it with a delay injected into the
window.
> TBH, I think we can drop this comment entirely. That's just simply
> the right thing to do?
Will do. I kept the comment on the CoW warning below it, which is
about something else.
> But why do we do this early copy dance *at all*? I think I
> complained about this vma_is_anonymous handling in the past.
> vma_needs_copy() is the one deciding whether something should get
> copied or not.
>
> So, can't we just get rid of the vma_is_anonymous() handling and
> perform the weird special-pte check (that I hate entirely) when
> already holding the PTL?
I tried exactly that first, see "try 1" below. The early
vma_is_anonymous() return is gone, the locks are taken for every huge
PMD, and both the special check and the skip of non-anonymous VMAs
run on the value read under the lock. The lockless read is gone, and
the race with it. The skip itself has to stay, as the anonymous path
cannot copy a file THP.
That makes fork slower when it walks a VMA full of huge PMDs that it
then skips. vma_needs_copy() sends such VMAs here, for example a
MAP_PRIVATE file mapping with an anon_vma. fork() of a process with
4 GB of shmem THPs mapped MAP_PRIVATE and an anon_vma on the VMA
(2047 PMD-mapped THPs), median of 2000 forks, 4-vCPU KVM guest:
v7.3-rc7 86-87 us
try 1 131-148 us
That is about 25 ns per huge PMD, spent taking and dropping the two
PMD locks only to skip it.
So "try 2" keeps the early return, but decides it from the VMA flags
instead of a lockless read of the PMD: a non-anonymous VMA without
VM_PFNMAP or VM_MIXEDMAP cannot hold a special PMD other than the huge
zero one, as vmf_insert_pfn_pmd() requires one of the flags, and the
flags cannot change under the mmap write lock fork holds. Everything
else is decided under the PTL, as in try 1. fork() is back at
86-87 us.
I also tried vma_is_special_huge() for that check, but its
vma_is_dax() test on every huge PMD made fork cost about 17% more
kernel instructions than v7.3-rc7 here, against about 1% for the
flags test.
Both diffs are against v7.3-rc7 and leave out the deposit fix from
patch 2, which goes on top as before. If try 2 looks right to you, v4
will be try 2 plus the deposit patch.
Try 1:
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1958,39 +1958,36 @@ int copy_huge_pmd(struct mm_struct *dst_mm, struct
mm_struct *src_mm,
pgtable_t pgtable = NULL;
int ret = -ENOMEM;
- pmd = pmdp_get_lockless(src_pmd);
+ if (vma_is_anonymous(dst_vma)) {
+ pgtable = pte_alloc_one(dst_mm);
+ if (unlikely(!pgtable))
+ goto out;
+ }
+
+ dst_ptl = pmd_lock(dst_mm, dst_pmd);
+ src_ptl = pmd_lockptr(src_mm, src_pmd);
+ spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING);
+
+ ret = -EAGAIN;
+ pmd = *src_pmd;
+
if (unlikely(pmd_present(pmd) && pmd_special(pmd) &&
!is_huge_zero_pmd(pmd))) {
- dst_ptl = pmd_lock(dst_mm, dst_pmd);
- src_ptl = pmd_lockptr(src_mm, src_pmd);
- spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING);
/*
- * No need to recheck the pmd, it can't change with write
- * mmap lock held here.
- *
- * Meanwhile, making sure it's not a CoW VMA with writable
- * mapping, otherwise it means either the anon page wrongly
- * applied special bit, or we made the PRIVATE mapping be
- * able to wrongly write to the backend MMIO.
+ * Make sure it's not a CoW VMA with writable mapping,
+ * otherwise it means either the anon page wrongly applied
+ * special bit, or we made the PRIVATE mapping be able to
+ * wrongly write to the backend MMIO.
*/
VM_WARN_ON_ONCE(vma_is_cow_mapping(src_vma) && pmd_write(pmd));
goto set_pmd;
}
/* Skip if can be re-fill on fault */
- if (!vma_is_anonymous(dst_vma))
- return 0;
-
- pgtable = pte_alloc_one(dst_mm);
- if (unlikely(!pgtable))
- goto out;
-
- dst_ptl = pmd_lock(dst_mm, dst_pmd);
- src_ptl = pmd_lockptr(src_mm, src_pmd);
- spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING);
-
- ret = -EAGAIN;
- pmd = *src_pmd;
+ if (!vma_is_anonymous(dst_vma)) {
+ ret = 0;
+ goto out_unlock;
+ }
if (unlikely(thp_migration_supported() &&
pmd_is_valid_softleaf(pmd))) {
Try 2:
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1958,32 +1958,16 @@ int copy_huge_pmd(struct mm_struct *dst_mm, struct
mm_struct *src_mm,
pgtable_t pgtable = NULL;
int ret = -ENOMEM;
- pmd = pmdp_get_lockless(src_pmd);
- if (unlikely(pmd_present(pmd) && pmd_special(pmd) &&
- !is_huge_zero_pmd(pmd))) {
- dst_ptl = pmd_lock(dst_mm, dst_pmd);
- src_ptl = pmd_lockptr(src_mm, src_pmd);
- spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING);
- /*
- * No need to recheck the pmd, it can't change with write
- * mmap lock held here.
- *
- * Meanwhile, making sure it's not a CoW VMA with writable
- * mapping, otherwise it means either the anon page wrongly
- * applied special bit, or we made the PRIVATE mapping be
- * able to wrongly write to the backend MMIO.
- */
- VM_WARN_ON_ONCE(vma_is_cow_mapping(src_vma) && pmd_write(pmd));
- goto set_pmd;
- }
-
/* Skip if can be re-fill on fault */
- if (!vma_is_anonymous(dst_vma))
+ if (!vma_is_anonymous(dst_vma) &&
+ !vma_test_any(dst_vma, VMA_PFNMAP_BIT, VMA_MIXEDMAP_BIT))
return 0;
- pgtable = pte_alloc_one(dst_mm);
- if (unlikely(!pgtable))
- goto out;
+ if (vma_is_anonymous(dst_vma)) {
+ pgtable = pte_alloc_one(dst_mm);
+ if (unlikely(!pgtable))
+ goto out;
+ }
dst_ptl = pmd_lock(dst_mm, dst_pmd);
src_ptl = pmd_lockptr(src_mm, src_pmd);
@@ -1992,6 +1976,23 @@ int copy_huge_pmd(struct mm_struct *dst_mm, struct
mm_struct *src_mm,
ret = -EAGAIN;
pmd = *src_pmd;
+ if (unlikely(pmd_present(pmd) && pmd_special(pmd) &&
+ !is_huge_zero_pmd(pmd))) {
+ /*
+ * Make sure it's not a CoW VMA with writable mapping,
+ * otherwise it means either the anon page wrongly applied
+ * special bit, or we made the PRIVATE mapping be able to
+ * wrongly write to the backend MMIO.
+ */
+ VM_WARN_ON_ONCE(vma_is_cow_mapping(src_vma) && pmd_write(pmd));
+ goto set_pmd;
+ }
+
+ if (!vma_is_anonymous(dst_vma)) {
+ ret = 0;
+ goto out_unlock;
+ }
+
if (unlikely(thp_migration_supported() &&
pmd_is_valid_softleaf(pmd))) {
copy_huge_non_present_pmd(dst_mm, src_mm, dst_pmd, src_pmd,
addr,
--
Cheers,
Konstantin
