Skip to contentSkip to conversation

skip redundant sync IPIs when TLB flush sent them

PATCH Superseded Lance Yang 3 participants 6 messages Apr 20 – Apr 24

Subsystems (8)

v9 of 10 Superseded Latest is v10 →

lore
Share & tools
lore

Lance Yang

author
#0 lore raw

v9 cover skip redundant sync IPIs when TLB flush sent them

Hi all,

When page table operations require synchronization with software/lockless walkers, they call tlb_remove_table_sync_{one,rcu}() after flushing the TLB (tlb->freed_tables or tlb->unshared_tables).

On architectures where the TLB flush already sends IPIs to all target CPUs, the subsequent sync IPI broadcast is redundant. This is not only costly on large systems where it disrupts all CPUs even for single-process page table operations, but has also been reported to hurt RT workloads[1].

This series introduces tlb_table_flush_implies_ipi_broadcast() to check if the prior TLB flush already provided the necessary synchronization. When true, the sync calls can early-return.

A few cases rely on this synchronization:

1) hugetlb PMD unshare[2]: The problem is not the freeing but the reuse of the PMD table for other purposes in the last remaining user after unsharing.

2) khugepaged collapse[3]: Ensure no concurrent GUP-fast before collapsing and (possibly) freeing the page table / re-depositing it.

Two-step plan as David suggested[4]:

Step 1 (this series): Skip redundant sync when we're 100% certain the TLB flush sent IPIs. INVLPGB is excluded because when supported, we cannot guarantee IPIs were sent, keeping it clean and simple.

Step 2 (future work): Send targeted IPIs only to CPUs actually doing software/lockless page table walks, benefiting all architectures.

Regarding Step 2, it obviously only applies to setups where Step 1 does not
apply: like x86 with INVLPGB or arm64. Step 2 work is ongoing; early attempts showed ~3% GUP-fast overhead. Reducing the overhead requires more work and tuning; it will be submitted separately once ready.

On a 64-core Intel x86 server, the CAL interrupt count in /proc/interrupts dropped from 646,316 to 785 when collapsing a 20 GiB range with this series applied.

David Hildenbrand did the initial implementation. I built on his work and relied on off-list discussions to push it further - thanks a lot David!

[1] https://lore.kernel.org/linux-mm/[email protected]/
[2] https://lore.kernel.org/linux-mm/[email protected]/
[3] https://lore.kernel.org/linux-mm/[email protected]/
[4] https://lore.kernel.org/linux-mm/[email protected]/

v8 -> v9:
- Rebase on mm-new; re-tested, no code changes.
- https://lore.kernel.org/linux-mm/[email protected]/

v7 -> v8:
- Pick up Acked-by tags from David, thanks!
- Add CAL interrupt numbers to the cover letter (per Andrew, thanks!)
- Rewrite the [2/2] changelog and reword the comment (per David, thanks!)
- https://lore.kernel.org/linux-mm/[email protected]/

v6 -> v7:
- Simplify init logic and eliminate duplicated X86_FEATURE_INVLPGB checks (per Dave, thanks!)
- Remove flush_tlb_multi_implies_ipi_broadcast property because no PV backend sets it today.
- https://lore.kernel.org/linux-mm/[email protected]/

v5 -> v6:
- Use static_branch to eliminate the branch overhead (per Peter, thanks!)
- https://lore.kernel.org/linux-mm/[email protected]/

v4 -> v5:
- Drop per-CPU tracking (active_lockless_pt_walk_mm) from this series; defer to Step 2 as it adds ~3% GUP-fast overhead
- Keep pv_ops property false for PV backends like KVM: preempted vCPUs cannot be assumed safe (per Sean, thanks!)
https://lore.kernel.org/linux-mm/[email protected]/
- https://lore.kernel.org/linux-mm/[email protected]/

v3 -> v4:
- Rework based on David's two-step direction and per-CPU idea:
1) Targeted IPIs: per-CPU variable when entering/leaving lockless page table walk; tlb_remove_table_sync_mm() IPIs only those CPUs.
2) On x86, pv_mmu_ops property set at init to skip the extra sync when flush_tlb_multi() already sends IPIs.
https://lore.kernel.org/linux-mm/[email protected]/
- https://lore.kernel.org/linux-mm/[email protected]/

v2 -> v3:
- Complete rewrite: use dynamic IPI tracking instead of static checks (per Dave Hansen, thanks!)
- Track IPIs via mmu_gather: native_flush_tlb_multi() sets flag when actually sending IPIs
- Motivation for skipping redundant IPIs explained by David:
https://lore.kernel.org/linux-mm/[email protected]/
- https://lore.kernel.org/linux-mm/[email protected]/

v1 -> v2:
- Fix cover letter encoding to resolve send-email issues. Apologies for any email flood caused by the failed send attempts :(

RFC -> v1:
- Use a callback function in pv_mmu_ops instead of comparing function pointers (per David)
- Embed the check directly in tlb_remove_table_sync_one() instead of requiring every caller to check explicitly (per David)
- Move tlb_table_flush_implies_ipi_broadcast() outside of CONFIG_MMU_GATHER_RCU_TABLE_FREE to fix build error on architectures that don't enable this config.
https://lore.kernel.org/oe-kbuild-all/[email protected]/
- https://lore.kernel.org/linux-mm/[email protected]/

Lance Yang (2):
mm/mmu_gather: prepare to skip redundant sync IPIs x86/tlb: skip redundant sync IPIs for native TLB flush

 arch/x86/include/asm/tlb.h      | 18 +++++++++++++++++-
 arch/x86/include/asm/tlbflush.h |  2 ++
 arch/x86/kernel/smpboot.c       |  1 +
 arch/x86/mm/tlb.c               | 15 +++++++++++++++
 include/asm-generic/tlb.h       | 17 +++++++++++++++++
 mm/mmu_gather.c                 | 15 +++++++++++++++
 6 files changed, 67 insertions(+), 1 deletion(-)
Signature · 1 line
2.49.0
author
#1 lore raw

PATCH v9 1/2 mm/mmu_gather: prepare to skip redundant sync IPIs

From: Lance Yang <[email protected]>

When page table operations require synchronization with software/lockless walkers, they call tlb_remove_table_sync_{one,rcu}() after flushing the TLB (tlb->freed_tables or tlb->unshared_tables).

On architectures where the TLB flush already sends IPIs to all target CPUs, the subsequent sync IPI broadcast is redundant. This is not only costly on large systems where it disrupts all CPUs even for single-process page table operations, but has also been reported to hurt RT workloads[1].

Introduce tlb_table_flush_implies_ipi_broadcast() to check if the prior TLB flush already provided the necessary synchronization. When true, the sync calls can early-return.

A few cases rely on this synchronization:

1) hugetlb PMD unshare[2]: The problem is not the freeing but the reuse of the PMD table for other purposes in the last remaining user after unsharing.

2) khugepaged collapse[3]: Ensure no concurrent GUP-fast before collapsing and (possibly) freeing the page table / re-depositing it.

Currently always returns false (no behavior change). The follow-up patch will enable the optimization for x86.

[1] https://lore.kernel.org/linux-mm/[email protected]/
[2] https://lore.kernel.org/linux-mm/[email protected]/
[3] https://lore.kernel.org/linux-mm/[email protected]/

---
 include/asm-generic/tlb.h | 17 +++++++++++++++++
 mm/mmu_gather.c           | 15 +++++++++++++++
 2 files changed, 32 insertions(+)
include/asm-generic/tlb.h +17 -0
index bdcc2778ac64..cb41cc6a0024 100644
@@ -240,6 +240,23 @@ static inline void tlb_remove_table(struct mmu_gather *tlb, void *table)
 }
 #endif /* CONFIG_MMU_GATHER_TABLE_FREE */
 
+/**
+ * tlb_table_flush_implies_ipi_broadcast - does TLB flush imply IPI sync
+ *
+ * When page table operations require synchronization with software/lockless
+ * walkers, they flush the TLB (tlb->freed_tables or tlb->unshared_tables)
+ * then call tlb_remove_table_sync_{one,rcu}(). If the flush already sent
+ * IPIs to all CPUs, the sync call is redundant.
+ *
+ * Returns false by default. Architectures can override by defining this.
+ */
+#ifndef tlb_table_flush_implies_ipi_broadcast
+static inline bool tlb_table_flush_implies_ipi_broadcast(void)
+{
+	return false;
+}
+#endif
+
 #ifdef CONFIG_MMU_GATHER_RCU_TABLE_FREE
 /*
  * This allows an architecture that does not use the linux page-tables for
mm/mmu_gather.c +15 -0
index 3985d856de7f..37a6a711c37e 100644
@@ -283,6 +283,14 @@ void tlb_remove_table_sync_one(void)
 	 * It is however sufficient for software page-table walkers that rely on
 	 * IRQ disabling.
 	 */
+
+	/*
+	 * Skip IPI if the preceding TLB flush already synchronized with
+	 * all CPUs that could be doing software/lockless page table walks.
+	 */
+	if (tlb_table_flush_implies_ipi_broadcast())
+		return;
+
 	smp_call_function(tlb_remove_table_smp_sync, NULL, 1);
 }
 
@@ -312,6 +320,13 @@ static void tlb_remove_table_free(struct mmu_table_batch *batch)
  */
 void tlb_remove_table_sync_rcu(void)
 {
+	/*
+	 * Skip RCU wait if the preceding TLB flush already synchronized
+	 * with all CPUs that could be doing software/lockless page table walks.
+	 */
+	if (tlb_table_flush_implies_ipi_broadcast())
+		return;
+
 	synchronize_rcu();
 }
 
Signature · 1 line
2.49.0
author
#2 lore raw

PATCH v9 2/2 x86/tlb: skip redundant sync IPIs for native TLB flush

In reply to Lance Yang

From: Lance Yang <[email protected]>

Some page table operations need to synchronize with software/lockless walkers after a TLB flush by calling tlb_remove_table_sync_{one,rcu}(). On x86, that extra synchronization is redundant when the preceding TLB flush already broadcast IPIs to all relevant CPUs.

native_pv_tlb_init() checks whether native_flush_tlb_multi() is in use. On CONFIG_PARAVIRT systems, it checks pv_ops; on non-PARAVIRT, native flush is always in use.

It decides once at boot whether to enable the optimization: if using native TLB flush and INVLPGB is not supported, we know IPIs were sent and can skip the redundant sync. The decision is fixed via a static key as Peter suggested[1].

PV backends (KVM, Xen, Hyper-V) typically have their own implementations and don't call native_flush_tlb_multi() directly, so they cannot be trusted to provide the IPI guarantees we need.

Also treat unshared_tables like freed_tables when issuing the TLB flush, so lazy-TLB CPUs receive IPIs during unsharing of page tables as well. This allows us to safely implement tlb_table_flush_implies_ipi_broadcast().

Two-step plan as David suggested[2]:

Step 1 (this patch): Skip redundant sync when we're 100% certain the TLB flush sent IPIs. INVLPGB is excluded because when supported, we cannot guarantee IPIs were sent, keeping it clean and simple.

Step 2 (future work): Send targeted IPIs only to CPUs actually doing software/lockless page table walks, benefiting all architectures.

Regarding Step 2, it obviously only applies to setups where Step 1 does not apply: like x86 with INVLPGB or arm64.

[1] https://lore.kernel.org/linux-mm/[email protected]/
[2] https://lore.kernel.org/linux-mm/[email protected]/

---
 arch/x86/include/asm/tlb.h      | 18 +++++++++++++++++-
 arch/x86/include/asm/tlbflush.h |  2 ++
 arch/x86/kernel/smpboot.c       |  1 +
 arch/x86/mm/tlb.c               | 15 +++++++++++++++
 4 files changed, 35 insertions(+), 1 deletion(-)
arch/x86/include/asm/tlb.h +17 -1
index 866ea78ba156..fc586ec8e768 100644
@@ -5,11 +5,21 @@
 #define tlb_flush tlb_flush
 static inline void tlb_flush(struct mmu_gather *tlb);
 
+#define tlb_table_flush_implies_ipi_broadcast tlb_table_flush_implies_ipi_broadcast
+static inline bool tlb_table_flush_implies_ipi_broadcast(void);
+
 #include <asm-generic/tlb.h>
 #include <linux/kernel.h>
 #include <vdso/bits.h>
 #include <vdso/page.h>
 
+DECLARE_STATIC_KEY_FALSE(tlb_ipi_broadcast_key);
+
+static inline bool tlb_table_flush_implies_ipi_broadcast(void)
+{
+	return static_branch_likely(&tlb_ipi_broadcast_key);
+}
+
 static inline void tlb_flush(struct mmu_gather *tlb)
 {
 	unsigned long start = 0UL, end = TLB_FLUSH_ALL;
@@ -20,7 +30,13 @@ static inline void tlb_flush(struct mmu_gather *tlb)
 		end = tlb->end;
 	}
 
-	flush_tlb_mm_range(tlb->mm, start, end, stride_shift, tlb->freed_tables);
+	/*
+	 * Treat unshared_tables just like freed_tables, such that lazy-TLB
+	 * CPUs also receive IPIs during unsharing of page tables, allowing
+	 * us to safely implement tlb_table_flush_implies_ipi_broadcast().
+	 */
+	flush_tlb_mm_range(tlb->mm, start, end, stride_shift,
+			   tlb->freed_tables || tlb->unshared_tables);
 }
 
 static inline void invlpg(unsigned long addr)
arch/x86/include/asm/tlbflush.h +2 -0
index 5a3cdc439e38..8ba853154b46 100644
@@ -18,6 +18,8 @@
 
 DECLARE_PER_CPU(u64, tlbstate_untag_mask);
 
+void __init native_pv_tlb_init(void);
+
 void __flush_tlb_all(void);
 
 #define TLB_FLUSH_ALL	-1UL
arch/x86/kernel/smpboot.c +1 -0
index 294a8ea60298..df776b645a9c 100644
@@ -1256,6 +1256,7 @@ void __init native_smp_prepare_boot_cpu(void)
 		switch_gdt_and_percpu_base(me);
 
 	native_pv_lock_init();
+	native_pv_tlb_init();
 }
 
 void __init native_smp_cpus_done(unsigned int max_cpus)
arch/x86/mm/tlb.c +15 -0
index 621e09d049cb..8f5585ebaf09 100644
@@ -26,6 +26,8 @@
 
 #include "mm_internal.h"
 
+DEFINE_STATIC_KEY_FALSE(tlb_ipi_broadcast_key);
+
 #ifdef CONFIG_PARAVIRT
 # define STATIC_NOPV
 #else
@@ -1834,3 +1836,16 @@ static int __init create_tlb_single_page_flush_ceiling(void)
 	return 0;
 }
 late_initcall(create_tlb_single_page_flush_ceiling);
+
+void __init native_pv_tlb_init(void)
+{
+#ifdef CONFIG_PARAVIRT
+	if (pv_ops.mmu.flush_tlb_multi != native_flush_tlb_multi)
+		return;
+#endif
+
+	if (cpu_feature_enabled(X86_FEATURE_INVLPGB))
+		return;
+
+	static_branch_enable(&tlb_ipi_broadcast_key);
+}
Signature · 1 line
2.49.0
#3 lore raw

Re: [PATCH 7.2 v9 2/2] x86/tlb: skip redundant sync IPIs for native TLB flush

Review

Quoted 9 lines · Lance Yang
(quoted diff) +7 -1
-	flush_tlb_mm_range(tlb->mm, start, end, stride_shift, tlb->freed_tables);
+	/*
+	 * Treat unshared_tables just like freed_tables, such that lazy-TLB
+	 * CPUs also receive IPIs during unsharing of page tables, allowing
+	 * us to safely implement tlb_table_flush_implies_ipi_broadcast().
+	 */
+	flush_tlb_mm_range(tlb->mm, start, end, stride_shift,
+			   tlb->freed_tables || tlb->unshared_tables);
 }

I've been staring at this trying to make sense of it for too long.

Right now, flush_tlb_mm_range() literally has an argument named "freed_tables" and "tlb->freed_tables" is passed there. That seems totally sane. It's 100% straightforward to follow.

But it makes zero logical sense to me to now mix "tlb->unshared_tables" in there. Sure, what you _want_ is the freed_tables==1 behavior from tlb->unshared_tables==1, and this obviously hacks that in there, but it's not explained well enough and not maintainable like this. IOW, it's still just hack.

I think what's happened here is that info->freed_tables is being modified from being strictly related to page table freeing, and moved over to a bit which tells TLB flushing implementations whether they can respect CPUs in lazy TLB mode.

It's mentioned in the comment, but then ever reflected into the code.

Shouldn't we be doing something like the attached patch? Look at how that maps over to the flushing side, like in the hyperv code:

Review static u64 hyperv_flush_tlb_others_ex(co · line 201

Quoted 11 lines
(quoted diff) +2 -2
-       bool do_lazy = !info->freed_tables;
+       bool do_lazy = !info->wake_lazy_cpus;
 
        trace_hyperv_mmu_flush_tlb_multi(cpus, info);
@@ -198,7 +198,7 @@ static u64 hyperv_flush_tlb_others_ex(co
 
        flush->hv_vp_set.format = HV_GENERIC_SET_SPARSE_4K;
        nr_bank = cpumask_to_vpset_skip(&flush->hv_vp_set, cpus,
-                       info->freed_tables ? NULL : cpu_is_lazy);
+                       info->wake_lazy_cpus ? NULL : cpu_is_lazy);

That even makes the hyperv code easier to read over what was there before, IMNHO.

Thoughts?

---

 b/arch/x86/hyperv/mmu.c           |    4 ++--
 b/arch/x86/include/asm/tlb.h      |   11 ++++++++++-
 b/arch/x86/include/asm/tlbflush.h |    4 ++--
 b/arch/x86/mm/tlb.c               |   29 +++++++++++++----------------
 4 files changed, 27 insertions(+), 21 deletions(-)
arch/x86/mm/tlb.c renamed from arch/x86/mm/tlb.c~flush_tlb_mm_range-lazy +13 -16
diff -puN arch/x86/mm/tlb.c~flush_tlb_mm_range-lazy arch/x86/mm/tlb.c
@@ -1339,16 +1339,12 @@ STATIC_NOPV void native_flush_tlb_multi(
 				(info->end - info->start) >> PAGE_SHIFT);
 
 	/*
-	 * If no page tables were freed, we can skip sending IPIs to
-	 * CPUs in lazy TLB mode. They will flush the CPU themselves
-	 * at the next context switch.
-	 *
-	 * However, if page tables are getting freed, we need to send the
-	 * IPI everywhere, to prevent CPUs in lazy TLB mode from tripping
-	 * up on the new contents of what used to be page tables, while
-	 * doing a speculative memory access.
+	 * Simple TLB flushes can avoid sending IPIs to CPUs in lazy
+	 * TLB mode. But some operations like freeing page tables
+	 * could leave dangerous state in paging structure caches.
+	 * Send IPIs even to lazy CPUs when necessary.
 	 */
-	if (info->freed_tables || mm_in_asid_transition(info->mm))
+	if (info->wake_lazy_cpus || mm_in_asid_transition(info->mm))
 		on_each_cpu_mask(cpumask, flush_tlb_func, (void *)info, true);
 	else
 		on_each_cpu_cond_mask(should_flush_tlb, flush_tlb_func,
@@ -1381,7 +1377,7 @@ static DEFINE_PER_CPU(unsigned int, flus
 
 static struct flush_tlb_info *get_flush_tlb_info(struct mm_struct *mm,
 			unsigned long start, unsigned long end,
-			unsigned int stride_shift, bool freed_tables,
+			unsigned int stride_shift, bool wake_lazy_cpus,
 			u64 new_tlb_gen)
 {
 	struct flush_tlb_info *info = this_cpu_ptr(&flush_tlb_info);
@@ -1408,7 +1404,7 @@ static struct flush_tlb_info *get_flush_
 	info->end		= end;
 	info->mm		= mm;
 	info->stride_shift	= stride_shift;
-	info->freed_tables	= freed_tables;
+	info->wake_lazy_cpus	= wake_lazy_cpus;
 	info->new_tlb_gen	= new_tlb_gen;
 	info->initiating_cpu	= smp_processor_id();
 	info->trim_cpumask	= 0;
@@ -1427,7 +1423,7 @@ static void put_flush_tlb_info(void)
 
 void flush_tlb_mm_range(struct mm_struct *mm, unsigned long start,
 				unsigned long end, unsigned int stride_shift,
-				bool freed_tables)
+				bool wake_lazy_cpus)
 {
 	struct flush_tlb_info *info;
 	int cpu = get_cpu();
@@ -1436,7 +1432,7 @@ void flush_tlb_mm_range(struct mm_struct
 	/* This is also a barrier that synchronizes with switch_mm(). */
 	new_tlb_gen = inc_mm_tlb_gen(mm);
 
-	info = get_flush_tlb_info(mm, start, end, stride_shift, freed_tables,
+	info = get_flush_tlb_info(mm, start, end, stride_shift, wake_lazy_cpus,
 				  new_tlb_gen);
 
 	/*
@@ -1528,10 +1524,11 @@ static void kernel_tlb_flush_range(struc
 void flush_tlb_kernel_range(unsigned long start, unsigned long end)
 {
 	struct flush_tlb_info *info;
+	bool wake_lazy_cpus = false;
 
 	guard(preempt)();
 
-	info = get_flush_tlb_info(NULL, start, end, PAGE_SHIFT, false,
+	info = get_flush_tlb_info(NULL, start, end, PAGE_SHIFT, wake_lazy_cpus,
 				  TLB_GENERATION_INVALID);
 
 	if (info->end == TLB_FLUSH_ALL)
@@ -1708,10 +1705,10 @@ EXPORT_SYMBOL_FOR_KVM(__flush_tlb_all);
 void arch_tlbbatch_flush(struct arch_tlbflush_unmap_batch *batch)
 {
 	struct flush_tlb_info *info;
-
+	bool wake_lazy_cpus = false;
 	int cpu = get_cpu();
 
-	info = get_flush_tlb_info(NULL, 0, TLB_FLUSH_ALL, 0, false,
+	info = get_flush_tlb_info(NULL, 0, TLB_FLUSH_ALL, 0, wake_lazy_cpus,
 				  TLB_GENERATION_INVALID);
 	/*
 	 * flush_tlb_multi() is not optimized for the common case in which only
arch/x86/include/asm/tlbflush.h renamed from arch/x86/include/asm/tlbflush.h~flush_tlb_mm_range-lazy +2 -2
diff -puN arch/x86/include/asm/tlbflush.h~flush_tlb_mm_range-lazy arch/x86/include/asm/tlbflush.h
@@ -247,7 +247,7 @@ struct flush_tlb_info {
 	u64			new_tlb_gen;
 	unsigned int		initiating_cpu;
 	u8			stride_shift;
-	u8			freed_tables;
+	u8			wake_lazy_cpus;
 	u8			trim_cpumask;
 };
 
@@ -337,7 +337,7 @@ static inline bool mm_in_asid_transition
 extern void flush_tlb_all(void);
 extern void flush_tlb_mm_range(struct mm_struct *mm, unsigned long start,
 				unsigned long end, unsigned int stride_shift,
-				bool freed_tables);
+				bool wake_lazy_cpus);
 extern void flush_tlb_kernel_range(unsigned long start, unsigned long end);
 
 static inline void flush_tlb_page(struct vm_area_struct *vma, unsigned long a)
arch/x86/include/asm/tlb.h renamed from arch/x86/include/asm/tlb.h~flush_tlb_mm_range-lazy +10 -1
diff -puN arch/x86/include/asm/tlb.h~flush_tlb_mm_range-lazy arch/x86/include/asm/tlb.h
@@ -14,13 +14,22 @@ static inline void tlb_flush(struct mmu_
 {
 	unsigned long start = 0UL, end = TLB_FLUSH_ALL;
 	unsigned int stride_shift = tlb_get_unmap_shift(tlb);
+	bool wake_lazy_cpus;
 
 	if (!tlb->fullmm && !tlb->need_flush_all) {
 		start = tlb->start;
 		end = tlb->end;
 	}
 
-	flush_tlb_mm_range(tlb->mm, start, end, stride_shift, tlb->freed_tables);
+	/*
+	 * Ensure all paging structure caches on all CPUs are flushed
+	 * when freeing page tables. Otherwise, a lazy CPU might wake
+	 * up and start walking previously-freed page tables and
+	 * caching garbage.
+	 */
+	wake_lazy_cpus = tlb->freed_tables;
+
+	flush_tlb_mm_range(tlb->mm, start, end, stride_shift, wake_lazy_cpus);
 }
 
 static inline void invlpg(unsigned long addr)
arch/x86/hyperv/mmu.c renamed from arch/x86/hyperv/mmu.c~flush_tlb_mm_range-lazy +2 -2
diff -puN arch/x86/hyperv/mmu.c~flush_tlb_mm_range-lazy arch/x86/hyperv/mmu.c
@@ -63,7 +63,7 @@ static void hyperv_flush_tlb_multi(const
 	struct hv_tlb_flush *flush;
 	u64 status;
 	unsigned long flags;
-	bool do_lazy = !info->freed_tables;
+	bool do_lazy = !info->wake_lazy_cpus;
 
 	trace_hyperv_mmu_flush_tlb_multi(cpus, info);
 
@@ -198,7 +198,7 @@ static u64 hyperv_flush_tlb_others_ex(co
 
 	flush->hv_vp_set.format = HV_GENERIC_SET_SPARSE_4K;
 	nr_bank = cpumask_to_vpset_skip(&flush->hv_vp_set, cpus,
-			info->freed_tables ? NULL : cpu_is_lazy);
+			info->wake_lazy_cpus ? NULL : cpu_is_lazy);
 	if (nr_bank < 0)
 		return HV_STATUS_INVALID_PARAMETER;
 

_

#4 lore raw

Re: [PATCH 7.2 v9 2/2] x86/tlb: skip redundant sync IPIs for native TLB flush

Quoted 48 lines · Dave Hansen

Review

Quoted 9 lines · Lance Yang
(quoted diff) +7 -1
-	flush_tlb_mm_range(tlb->mm, start, end, stride_shift, tlb->freed_tables);
+	/*
+	 * Treat unshared_tables just like freed_tables, such that lazy-TLB
+	 * CPUs also receive IPIs during unsharing of page tables, allowing
+	 * us to safely implement tlb_table_flush_implies_ipi_broadcast().
+	 */
+	flush_tlb_mm_range(tlb->mm, start, end, stride_shift,
+			   tlb->freed_tables || tlb->unshared_tables);
 }

I've been staring at this trying to make sense of it for too long.

Right now, flush_tlb_mm_range() literally has an argument named "freed_tables" and "tlb->freed_tables" is passed there. That seems totally sane. It's 100% straightforward to follow.

But it makes zero logical sense to me to now mix "tlb->unshared_tables" in there. Sure, what you _want_ is the freed_tables==1 behavior from tlb->unshared_tables==1, and this obviously hacks that in there, but it's not explained well enough and not maintainable like this. IOW, it's still just hack.

I think what's happened here is that info->freed_tables is being modified from being strictly related to page table freeing, and moved over to a bit which tells TLB flushing implementations whether they can respect CPUs in lazy TLB mode.

It's mentioned in the comment, but then ever reflected into the code.

Shouldn't we be doing something like the attached patch? Look at how that maps over to the flushing side, like in the hyperv code:

arch/x86/hyperv/mmu.c static u64 hyperv_flush_tlb_others_ex(co · line 201

Quoted 11 lines
arch/x86/hyperv/mmu.c renamed from arch/x86/hyperv/mmu.c~flush_tlb_mm_range-lazy +2 -2
-       bool do_lazy = !info->freed_tables;
+       bool do_lazy = !info->wake_lazy_cpus;
 
        trace_hyperv_mmu_flush_tlb_multi(cpus, info);
@@ -198,7 +198,7 @@ static u64 hyperv_flush_tlb_others_ex(co
 
        flush->hv_vp_set.format = HV_GENERIC_SET_SPARSE_4K;
        nr_bank = cpumask_to_vpset_skip(&flush->hv_vp_set, cpus,
-                       info->freed_tables ? NULL : cpu_is_lazy);
+                       info->wake_lazy_cpus ? NULL : cpu_is_lazy);

That even makes the hyperv code easier to read over what was there before, IMNHO.

Quoted 1 line

Thoughts?

Looks better!

Signature · 3 lines
Cheers,

David
author
#5 lore raw

Re: [PATCH 7.2 v9 2/2] x86/tlb: skip redundant sync IPIs for native TLB flush

In reply to Dave Hansen

Hi Dave,

On Thu, Apr 23, 2026 at 10:56:03AM -0700, Dave Hansen wrote:
[...]

Quoted 28 lines · Lance Yang

Review

Quoted 9 lines
(quoted diff) +7 -1
-	flush_tlb_mm_range(tlb->mm, start, end, stride_shift, tlb->freed_tables);
+	/*
+	 * Treat unshared_tables just like freed_tables, such that lazy-TLB
+	 * CPUs also receive IPIs during unsharing of page tables, allowing
+	 * us to safely implement tlb_table_flush_implies_ipi_broadcast().
+	 */
+	flush_tlb_mm_range(tlb->mm, start, end, stride_shift,
+			   tlb->freed_tables || tlb->unshared_tables);
 }

I've been staring at this trying to make sense of it for too long.

Right now, flush_tlb_mm_range() literally has an argument named "freed_tables" and "tlb->freed_tables" is passed there. That seems totally sane. It's 100% straightforward to follow.

But it makes zero logical sense to me to now mix "tlb->unshared_tables" in there. Sure, what you _want_ is the freed_tables==1 behavior from tlb->unshared_tables==1, and this obviously hacks that in there, but it's not explained well enough and not maintainable like this. IOW, it's still just hack.

I think what's happened here is that info->freed_tables is being modified from being strictly related to page table freeing, and moved over to a bit which tells TLB flushing implementations whether they can respect CPUs in lazy TLB mode.

Quoted 4 lines

It's mentioned in the comment, but then ever reflected into the code.

Shouldn't we be doing something like the attached patch? Look at how that maps over to the flushing side, like in the hyperv code:

Cool, thanks!

I was trying to keep the change small by passing unshared_tables through the exsiting freed_tables argument, but that made the code a bit harder to follow ...

Quoted 16 lines

arch/x86/hyperv/mmu.c static u64 hyperv_flush_tlb_others_ex(co · line 201

Quoted 12 lines
arch/x86/hyperv/mmu.c renamed from arch/x86/hyperv/mmu.c~flush_tlb_mm_range-lazy +2 -2
-       bool do_lazy = !info->freed_tables;
+       bool do_lazy = !info->wake_lazy_cpus;
 
        trace_hyperv_mmu_flush_tlb_multi(cpus, info);
@@ -198,7 +198,7 @@ static u64 hyperv_flush_tlb_others_ex(co
 
        flush->hv_vp_set.format = HV_GENERIC_SET_SPARSE_4K;
        nr_bank = cpumask_to_vpset_skip(&flush->hv_vp_set, cpus,
-                       info->freed_tables ? NULL : cpu_is_lazy);
+                       info->wake_lazy_cpus ? NULL : cpu_is_lazy);

That even makes the hyperv code easier to read over what was there before, IMNHO.

Quoted 1 line

Thoughts?

[...]

Yeah, renaming the flush_tlb_info bit to wake_lazy_cpus reads much better. Will fold this into v10, Thanks for spelling it out!

Lance