Message-ID: <aUioS4dkTrKgsHGP@hyeyoo>
Date: Mon, 22 Dec 2025 11:09:15 +0900
From: Harry Yoo <harry.yoo@...cle.com>
To: "David Hildenbrand (Red Hat)" <david@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org,
        linux-mm@...ck.org, Will Deacon <will@...nel.org>,
        "Aneesh Kumar K.V" <aneesh.kumar@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Nick Piggin <npiggin@...il.com>, Peter Zijlstra <peterz@...radead.org>,
        Arnd Bergmann <arnd@...db.de>, Muchun Song <muchun.song@...ux.dev>,
        Oscar Salvador <osalvador@...e.de>,
        "Liam R. Howlett" <Liam.Howlett@...cle.com>,
        Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
        Vlastimil Babka <vbabka@...e.cz>, Jann Horn <jannh@...gle.com>,
        Pedro Falcato <pfalcato@...e.de>, Rik van Riel <riel@...riel.com>,
        Laurence Oberman <loberman@...hat.com>,
        Prakash Sangappa <prakash.sangappa@...cle.com>,
        Nadav Amit <nadav.amit@...il.com>, stable@...r.kernel.org,
        Ryan Roberts <ryan.roberts@....com>,
        Catalin Marinas <catalin.marinas@....com>,
        Christophe Leroy <christophe.leroy@...roup.eu>
Subject: Re: [PATCH v2 4/4] mm/hugetlb: fix excessive IPI broadcasts when
 unsharing PMD tables using mmu_gather

On Sun, Dec 21, 2025 at 01:24:44PM +0100, David Hildenbrand (Red Hat) wrote:
> On 12/19/25 14:59, David Hildenbrand (Red Hat) wrote:
> > On 12/19/25 14:52, David Hildenbrand (Red Hat) wrote:
> > > On 12/19/25 13:37, Harry Yoo wrote:
> > > > On Fri, Dec 12, 2025 at 08:10:19AM +0100, David Hildenbrand (Red Hat) wrote:
> > > > > As reported, ever since commit 1013af4f585f ("mm/hugetlb: fix
> > > > > huge_pmd_unshare() vs GUP-fast race") we can end up in some situations
> > > > > where we perform so many IPI broadcasts when unsharing hugetlb PMD page
> > > > > tables that it severely regresses some workloads.
> > > > > 
> > > > > In particular, when we fork()+exit(), or when we munmap() a large
> > > > > area backed by many shared PMD tables, we perform one IPI broadcast per
> > > > > unshared PMD table.
> > > > > 
> > > > 
> > > > [...snip...]
> > > > 
> > > > > Fixes: 1013af4f585f ("mm/hugetlb: fix huge_pmd_unshare() vs GUP-fast race")
> > > > > Reported-by: Uschakow, Stanislav" <suschako@...zon.de>
> > > > > Closes: https://lore.kernel.org/all/4d3878531c76479d9f8ca9789dc6485d@amazon.de/
> > > > > Tested-by: Laurence Oberman <loberman@...hat.com>
> > > > > Cc: <stable@...r.kernel.org>
> > > > > Signed-off-by: David Hildenbrand (Red Hat) <david@...nel.org>
> > > > > ---
> > > > >     include/asm-generic/tlb.h |  74 ++++++++++++++++++++++-
> > > > >     include/linux/hugetlb.h   |  19 +++---
> > > > >     mm/hugetlb.c              | 121 ++++++++++++++++++++++----------------
> > > > >     mm/mmu_gather.c           |   7 +++
> > > > >     mm/mprotect.c             |   2 +-
> > > > >     mm/rmap.c                 |  25 +++++---
> > > > >     6 files changed, 179 insertions(+), 69 deletions(-)
> > > > > 
> > > > > @@ -6522,22 +6511,16 @@ long hugetlb_change_protection(struct vm_area_struct *vma,
> > > > >     				pte = huge_pte_clear_uffd_wp(pte);
> > > > >     			huge_ptep_modify_prot_commit(vma, address, ptep, old_pte, pte);
> > > > >     			pages++;
> > > > > +			tlb_remove_huge_tlb_entry(h, tlb, ptep, address);
> > > > >     		}
> > > > >     next:
> > > > >     		spin_unlock(ptl);
> > > > >     		cond_resched();
> > > > >     	}
> > > > > -	/*
> > > > > -	 * There is nothing protecting a previously-shared page table that we
> > > > > -	 * unshared through huge_pmd_unshare() from getting freed after we
> > > > > -	 * release i_mmap_rwsem, so flush the TLB now. If huge_pmd_unshare()
> > > > > -	 * succeeded, flush the range corresponding to the pud.
> > > > > -	 */
> > > > > -	if (shared_pmd)
> > > > > -		flush_hugetlb_tlb_range(vma, range.start, range.end);
> > > > > -	else
> > > > > -		flush_hugetlb_tlb_range(vma, start, end);
> > > > > +
> > > > > +	tlb_flush_mmu_tlbonly(tlb);
> > > > > +	huge_pmd_unshare_flush(tlb, vma);
> > > > 
> > > > Shouldn't we teach mmu_gather that it has to call
> > > 
> > > I hope not :) In the worst case we could keep the
> > > flush_hugetlb_tlb_range() in the !shared case in. Suboptimal but I am
> > > sick and tired of dealing with this hugetlb mess.
> > > 
> > > 
> > > Let me CC Ryan and Catalin for the arm64 pieces and Christophe on the
> > > ppc pieces: See [1] where we convert away from some
> > > flush_hugetlb_tlb_range() users to operate on mmu_gather using
> > > * tlb_remove_huge_tlb_entry() for mremap() and mprotect(). Before we
> > >      would only use it in __unmap_hugepage_range().
> > > * tlb_flush_pmd_range() for unsharing of shared PMD tables. We already
> > >      used that in one call path.
> > 
> > To clarify, powerpc does not select ARCH_WANT_HUGE_PMD_SHARE, so the
> > second change does not apply to ppc.
> > 
> 
> Okay, the existing hugetlb mmu_gather integration is hell on earth.
> 
> I *think* to get everything right (work around all the hacks we have) we might have to do a
> 
> 	tlb_change_page_size(tlb, sz);
> 	tlb_start_vma(tlb, vma);
> 
> before adding something to the tlb and a tlb_end_vma(tlb, vma) if we
> don't immediately call tlb_finish_mmu() already.

Good point, indeed!

> tlb_change_page_size() will set page_size accordingly (as required for
> ppc IIUC).

Right. PPC wants to flush TLB when the page size changes.

> tlb_start_vma()->tlb_update_vma_flags() will set tlb->vma_huge for ...
> some very good reason I am sure.

:)

> So something like the following might do the trick:
> 
> From b0b854c2f91ce0931e1462774c92015183fb5b52 Mon Sep 17 00:00:00 2001
> From: "David Hildenbrand (Red Hat)" <david@...nel.org>
> Date: Sun, 21 Dec 2025 12:57:43 +0100
> Subject: [PATCH] tmp
> 
> Signed-off-by: David Hildenbrand (Red Hat) <david@...nel.org>
> ---
>  mm/hugetlb.c | 12 +++++++++++-
>  mm/rmap.c    |  4 ++++
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 7fef0b94b5d1e..14521210181c9 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -5113,6 +5113,9 @@ int move_hugetlb_page_tables(struct vm_area_struct *vma,
>  	/* Prevent race with file truncation */
>  	hugetlb_vma_lock_write(vma);
>  	i_mmap_lock_write(mapping);
> +
> +	tlb_change_page_size(&tlb, sz);
> +	tlb_start_vma(&tlb, vma);
>  	for (; old_addr < old_end; old_addr += sz, new_addr += sz) {
>  		src_pte = hugetlb_walk(vma, old_addr, sz);
>  		if (!src_pte) {
> @@ -5128,13 +5131,13 @@ int move_hugetlb_page_tables(struct vm_area_struct *vma,
>  			new_addr |= last_addr_mask;
>  			continue;
>  		}
> -		tlb_remove_huge_tlb_entry(h, &tlb, src_pte, old_addr);
>  		dst_pte = huge_pte_alloc(mm, new_vma, new_addr, sz);
>  		if (!dst_pte)
>  			break;
>  		move_huge_pte(vma, old_addr, new_addr, src_pte, dst_pte, sz);
> +		tlb_remove_huge_tlb_entry(h, &tlb, src_pte, old_addr);
>  	}
>  	tlb_flush_mmu_tlbonly(&tlb);
> @@ -6416,6 +6419,8 @@ long hugetlb_change_protection(struct mmu_gather *tlb, struct vm_area_struct *vm
>  	BUG_ON(address >= end);
>  	flush_cache_range(vma, range.start, range.end);
> +	tlb_change_page_size(tlb, psize);
> +	tlb_start_vma(tlb, vma);
>  	mmu_notifier_invalidate_range_start(&range);
>  	hugetlb_vma_lock_write(vma);
> @@ -6532,6 +6537,8 @@ long hugetlb_change_protection(struct mmu_gather *tlb, struct vm_area_struct *vm
>  	hugetlb_vma_unlock_write(vma);
>  	mmu_notifier_invalidate_range_end(&range);
> +	tlb_end_vma(tlb, vma);
> +
>  	return pages > 0 ? (pages << h->order) : pages;
>  }
> @@ -7259,6 +7266,9 @@ static void hugetlb_unshare_pmds(struct vm_area_struct *vma,
>  	} else {
>  		i_mmap_assert_write_locked(vma->vm_file->f_mapping);
>  	}
> +
> +	tlb_change_page_size(&tlb, sz);
> +	tlb_start_vma(&tlb, vma);
>  	for (address = start; address < end; address += PUD_SIZE) {
>  		ptep = hugetlb_walk(vma, address, sz);
>  		if (!ptep)
> diff --git a/mm/rmap.c b/mm/rmap.c
> index d6799afe11147..27210bc6fb489 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -2015,6 +2015,8 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>  					goto walk_abort;
>  				tlb_gather_mmu(&tlb, mm);
> +				tlb_change_page_size(&tlb, huge_page_size(hstate_vma(vma)));
> +				tlb_start_vma(&tlb, vma);
>  				if (huge_pmd_unshare(&tlb, vma, address, pvmw.pte)) {
>  					hugetlb_vma_unlock_write(vma);
>  					huge_pmd_unshare_flush(&tlb, vma);
> @@ -2413,6 +2415,8 @@ static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma,
>  				}
>  				tlb_gather_mmu(&tlb, mm);
> +				tlb_change_page_size(&tlb, huge_page_size(hstate_vma(vma)));
> +				tlb_start_vma(&tlb, vma);
>  				if (huge_pmd_unshare(&tlb, vma, address, pvmw.pte)) {
>  					hugetlb_vma_unlock_write(vma);
>  					huge_pmd_unshare_flush(&tlb, vma);
> -- 
> 2.52.0
> 
> 
> 
> But now I'm staring at it and wonder whether we should just defer the TLB flushing changes
> to a later point and only focus on the IPI flushes.

You mean defer TLB flushing to which point? For unmapping or
changing permission of VMAs, flushing at VMA boundary already makes sense?

Or if you meant batching TLB flushes in try_to_{migrate,unmap}_one()...

/me starts wondering...

"Hmm... for RMAP, we already have TLB flush batching
 via struct tlbflush_unmap_batch. Why not use this framework
 when unmapping shared hugetlb pages as well?"

> Doing only that with mmu_gather looks *really* weird, and I don't want to
> introduce some other mechanism just for that batching purpose.
> 
> Hm ...
> 
> -- 
> Cheers
> 
> David

-- 
Cheers,
Harry / Hyeonggon

Powered by blists - more mailing lists