Re: [PATCH v3 1/2] mm/huge_memory: recheck a special PMD under the lock in copy_huge_pmd()

["David Hildenbrand (Arm)" <david@kernel.org>]

On 10/16/26 15:05, Konstantin Khorenko wrote:
> 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

Perfectly fine.

> 
> That is about 25 ns per huge PMD, spent taking and dropping the two
> PMD locks only to skip it.

We should likely do exactly what we do in the PTE path: copy PMDs that reference
file folios. See copy_present_ptes().

This THP/PMD handling inconsistency is annoying the crap out of me. We should
just consistently do the same thing for PTE vs. PMD.

> 
> 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))) {


There is a vm_normal_page_pmd(). It should be used instead of all of this
open-coded crap people added over the years.

Again, take a look at how the PTE scenario handles it in copy_present_ptes() etc.

> -               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.


Nobody should be checking pmd_special() bits manually. Use vm_normal_page_pmd().

Sorry that you'll have to clean up the mess people created over the years.

But it's all just waiting for more bugs to be discovered or introduces when
doing stuff completely different for PTE vs. PMD.

-- 
Cheers,

David