| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c66cb4e4-820e-419a-ae9f-efd2c15aa570@kernel.org>
Date: Mon, 22 Dec 2025 11:10:32 +0100
From: "David Hildenbrand (Red Hat)" <david@...nel.org>
To: Harry Yoo <harry.yoo@...cle.com>
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
>> 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?
Defer converting to mmu_gather to a later patch set :)
I gave it a try yesterday, but it's also a bit ugly.
In the code above, primarily the rmap change is nasty.
>
> 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?"
Hm, also not what we really want in most cases. I don't think we should
be using that outside of rmap.c (and I have the gut feeling that we
should maybe make use of mmu_gather in there instead at some point).
Let me try a bit to see if I can clean the code here up, or if I just
add a temporary custom batching data structure.
Thanks for bringing this up!
--
Cheers
David
Powered by blists - more mailing lists