x86,tlb,mm: make lazy TLB mode even lazier

The mm_struct always contains a cpumask bitmap, regardless of CONFIG_CPUMASK_OFFSTACK. That means the first step can be to simplify things, and simply have one bitmask at the end of the mm_struct for the mm_cpumask.

This does necessitate moving everything else in mm_struct into an anonymous sub-structure, which can be randomized when struct randomization is enabled.

The second step is to determine the correct size for the mm_struct slab object from the size of the mm_struct (excluding the cpu bitmap) and the size the cpumask.

For init_mm we can simply allocate the maximum size this kernel is compiled for, since we only have one init_mm in the system, anyway.

Pointer magic by Mike Galbraith, to evade -Wstringop-overflow getting confused by the dynamically sized array.

Signed-off-by Rik van Riel <[email protected]>

Signed-off-by Mike Galbraith <[email protected]>

Signed-off-by Rik van Riel <[email protected]>

Acked-by Dave Hansen <[email protected]>

Tested-by Song Liu <[email protected]>

include/asm-generic/tlb.h +0 -0

index 3063125197ad..e811ef7b8350 100644
@@ -303,4 +303,14 @@ static inline void tlb_remove_check_page_size_change(struct mmu_gather *tlb,
 
 #define tlb_migrate_finish(mm) do {} while (0)

Quoted 8 lines · tip-bot for Rik van Riel

include/asm-generic/tlb.h +0 -0

index 3063125197ad..e811ef7b8350 100644
@@ -303,4 +303,14 @@ static inline void tlb_remove_check_page_size_change(struct mmu_gather *tlb,
 
 #define tlb_migrate_finish(mm) do {} while (0)
 

include/asm-generic/tlb.h static inline void tlb_remove_check_page_size_change(struct mmu_gather *tlb, · line 314

Quoted 9 lines

include/asm-generic/tlb.h +9 -0

+/*
+ * Used to flush the TLB when page tables are removed, when lazy
+ * TLB mode may cause a CPU to retain intermediate translations
+ * pointing to about-to-be-freed page table memory.
+ */
+#ifndef HAVE_TLB_FLUSH_REMOVE_TABLES
+#define tlb_flush_remove_tables(mm) do {} while (0)
+#define tlb_flush_remove_tables_local(mm) do {} while (0)
+#endif

Is there a reason these are not inline functions, which gets us type checking and the like?

Andy discovered that speculative memory accesses while in lazy TLB mode can crash a system, when a CPU tries to dereference a speculative access using memory contents that used to be valid page table memory, but have since been reused for something else and point into la-la land.

On Mon, Jul 16, 2018 at 12:03 PM, Rik van Riel <[email protected]> wrote:

Hi Rik-

I was looking through this, and I see:

Quoted 13 lines

(quoted diff) +1 -1

-static void tlb_remove_table_one(void *table)
+static void tlb_remove_table_one(void *table, struct mmu_gather

*tlb)
{
/*
* This isn't an RCU grace period and hence the page-tables
cannot be

(quoted diff) +0 -0

@@ -344,7 +348,7 @@ static void tlb_remove_table_one(void *table)
         * It is however sufficient for software page-table walkers

that rely on
* IRQ disabling. See the comment near struct
mmu_table_batch.

         */

mm/memory.c static void tlb_remove_table_one(void *table) · line 353

Quoted 4 lines

mm/memory.c +1 -1

-       smp_call_function(tlb_remove_table_smp_sync, NULL, 1);
+       smp_call_function(tlb_remove_table_smp_sync, tlb->mm, 1);
        __tlb_remove_table(table);
 }

But tlb_remove_table() doesn't always call tlb_remove_table_one(). Do
the other paths through tlb_remove_table() do the right thing?

arch/x86/mm/tlb.c +8 -0

@@ -242,17 +244,40 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
 			   next->context.ctx_id);
 
 		/*
+		 * Even in lazy TLB mode, the CPU should stay set in the
+		 * mm_cpumask. The TLB shootdown code can figure out from
+		 * from cpu_tlbstate.is_lazy whether or not to send an IPI.
 		 */
 		if (WARN_ON_ONCE(real_prev != &init_mm &&
 				 !cpumask_test_cpu(cpu, mm_cpumask(next))))
 			cpumask_set_cpu(cpu, mm_cpumask(next));
 
+		/*
+		 * If the CPU is not in lazy TLB mode, we are just switching
+		 * from one thread in a process to another thread in the same
+		 * process. No TLB flush required.
+		 */

arch/x86/mm/tlb.c +11 -0

+		next_tlb_gen = atomic64_read(&next->context.tlb_gen);
+		if (this_cpu_read(cpu_tlbstate.ctxs[prev_asid].tlb_gen) ==
+				next_tlb_gen)
+			return;
+
+		/*
+		 * TLB contents went out of date while we were in lazy
+		 * mode. Fall through to the TLB switching code below.
+		 */
+		new_asid = prev_asid;
+		need_flush = true;

Quoted 17 lines · tip-bot for Rik van Riel

arch/x86/mm/tlb.c +8 -0

@@ -242,17 +244,40 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
 			   next->context.ctx_id);
 
 		/*
+		 * Even in lazy TLB mode, the CPU should stay set in the
+		 * mm_cpumask. The TLB shootdown code can figure out from
+		 * from cpu_tlbstate.is_lazy whether or not to send an IPI.
 		 */
 		if (WARN_ON_ONCE(real_prev != &init_mm &&
 				 !cpumask_test_cpu(cpu, mm_cpumask(next))))
 			cpumask_set_cpu(cpu, mm_cpumask(next));
 
+		/*
+		 * If the CPU is not in lazy TLB mode, we are just switching
+		 * from one thread in a process to another thread in the same
+		 * process. No TLB flush required.
+		 */

arch/x86/mm/tlb.c void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next, · line 269

Quoted 10 lines

arch/x86/mm/tlb.c +10 -0

+		if (!was_lazy)
+			return;
+
+		/*
+		 * Read the tlb_gen to check whether a flush is needed.
+		 * If the TLB is up to date, just use it.
+		 * The barrier synchronizes with the tlb_gen increment in
+		 * the TLB shootdown code.
+		 */
+		smp_mb();

What exactly is this smp_mb() ordering? The above comment is insufficient. Is it the cpumask_set_cpu() vs the atomic64_read() ?

If so, should this not be smp_mb__after_atomic() (iow a NO-OP on x86)

If it is not so, please fix the comment to explain things.

The tlb flush code first increments mm->context.tlb_gen, and then sends

shootdown IPIs to CPUs that have this mm loaded and are not in lazy TLB mode.

At context switch time, we have to ensure that we check the tlb_gen after we load the old is_lazy state.

Maybe something like this?

/*
* Read the tlb_gen to check whether a flush is needed.
* If the TLB is up to date, just use it.
* The TLB shootdown code first increments tlb_gen, and then
* sends IPIs to CPUs that have this CPU loaded and are not
* in lazy TLB mode. The barrier ensures we handle
* cpu_tlbstate.is_lazy before tlb_gen, keeping this code
* synchronized with the TLB flush code.
*/

Quoted 4 lines · Peter Zijlstra

Also, I don't suppose you've looked at the paravirt instances of flush_tlb_other() ? They don't elide the flushes because of lazy.

Let me look at those now :)

kvm_flush_tlb_other takes out preempted VCPUs from the flush mask, before calling native_flush_tlb_others, so it should get the optimization automatically.

Hyperv does ... sad magic.

Quoted 6 lines · Rik van Riel

Quoted 4 lines · Peter Zijlstra

Also, I don't suppose you've looked at the paravirt instances of flush_tlb_other() ? They don't elide the flushes because of lazy.

Let me look at those now :)

<snip xen>

Quoted 3 lines

kvm_flush_tlb_other takes out preempted VCPUs from the flush mask, before calling native_flush_tlb_others, so it should get the optimization automatically.

Ah, ok. I wasn't entirely sure the new lazy was purely for the idle case. But yes, the KVM paravirt thing should get the idle case right.

Quoted 3 lines

Ah, ok. I wasn't entirely sure the new lazy was purely for the idle case. But yes, the KVM paravirt thing should get the idle case right.

Not just idle, but also running in kernel threads like ksoftirqd, kworker, kswapd, etc.

However, kvm_flush_tlb_other calls
native_flush_tlb_other, so it should get that optimization automatically from my patch series.

Lazy TLB mode can result in an idle CPU being woken up by a TLB flush, when all it really needs to do is reload %CR3 at the next context switch, assuming no page table pages got freed.

Memory ordering is used to prevent race conditions between switch_mm_irqs_off, which checks whether .tlb_gen changed, and the TLB invalidation code, which increments .tlb_gen whenever page table entries get invalidated.

The atomic increment in inc_mm_tlb_gen is its own barrier; the context switch code adds an explicit barrier between reading tlbstate.is_lazy and next->context.tlb_gen.

Unlike the 2016 version of this patch, CPUs with cpu_tlbstate.is_lazy set are not removed from the mm_cpumask(mm), since that would prevent the TLB flush IPIs at page table free time from being sent to all the CPUs that need them.

This patch reduces total CPU use in the system by about 1-2% for a memcache workload on two socket systems, and by about 1% for a heavily multi-process netperf between two systems.

Can I skip both the cr4 and let switches when the TLB contents are no longer valid and got reloaded?

If the TLB contents are still valid, either because we never went into lazy TLB mode, or because no invalidates happened while we were lazy, we immediately return.

The cr4 and ldt reloads only happen if the TLB was invalidated while we were in lazy TLB mode.

Quoted 2 lines

Hmm. load_mm_cr4() should bypass itself when mm == &init_mm. Want to fix that part or should I?

I would be happy to send in a patch for this, and one for the above optimization you pointed out.

Quoted 9 lines · Rik van Riel

Can I skip both the cr4 and let switches when the TLB contents are no longer valid and got reloaded?

If the TLB contents are still valid, either because we never went into lazy TLB mode, or because no invalidates happened while we were lazy, we immediately return.

The cr4 and ldt reloads only happen if the TLB was invalidated while we were in lazy TLB mode.

Yes, since the only events that would change the LDT or the required CR4 value will unconditionally broadcast to every CPU in mm_cpumask regardless of whether they're lazy. The interesting case is that you go lazy, you miss an invalidation IPI because you were lazy, then you go unlazy, notice the tlb_gen change, and flush. If this happens, you know that you only missed a page table update and not an LDT update or a CR4 update, because the latter would have sent the IPI even though you were lazy. So you should skip the CR4 and LDT updates.

I suppose a different approach would be to fix the issue below and to try to track when the LDT actually needs reloading. But that latter part seems a bit complicated for minimal gain.

(Do you believe me? If not, please argue back!)

Quoted 6 lines

Quoted 2 lines

Hmm. load_mm_cr4() should bypass itself when mm == &init_mm. Want to fix that part or should I?

I would be happy to send in a patch for this, and one for the above optimization you pointed out.

Yes please!

Quoted 25 lines · Andy Lutomirski

Quoted 9 lines · Rik van Riel

Can I skip both the cr4 and let switches when the TLB contents are no longer valid and got reloaded?

If the TLB contents are still valid, either because we never went into lazy TLB mode, or because no invalidates happened while we were lazy, we immediately return.

The cr4 and ldt reloads only happen if the TLB was invalidated while we were in lazy TLB mode.

Yes, since the only events that would change the LDT or the required CR4 value will unconditionally broadcast to every CPU in mm_cpumask regardless of whether they're lazy. The interesting case is that you go lazy, you miss an invalidation IPI because you were lazy, then you go unlazy, notice the tlb_gen change, and flush. If this happens, you know that you only missed a page table update and not an LDT update or a CR4 update, because the latter would have sent the IPI even though you were lazy. So you should skip the CR4 and LDT updates.

Quoted 6 lines

I suppose a different approach would be to fix the issue below and to try to track when the LDT actually needs reloading. But that latter part seems a bit complicated for minimal gain.

(Do you believe me? If not, please argue back!)

I believe you :)

Quoted 9 lines

Quoted 6 lines

Quoted 2 lines

Hmm. load_mm_cr4() should bypass itself when mm == &init_mm. Want to fix that part or should I?

I would be happy to send in a patch for this, and one for the above optimization you pointed out.

Yes please!

There is a third optimization left to do. Currently every time we switch into lazy tlb mode, we take a refcount on the mm, even when switching from one kernel thread to another, or when repeatedly switching between the same mm and kernel threads.

We could keep that refcount (on a per cpu basis) from the time we first switch to that mm in lazy tlb mode, to when we switch the CPU to a different mm.

That would allow us to not bounce the cache line with the mm_struct reference count on every lazy TLB context switch.

Does that seem like a reasonable optimization?

Am I overlooking anything?

I'll try to get all three optimizations working, and will run them through some testing here before posting upstream.

I think you've introduced a minor-ish performance regression due to changing the old (admittedly terribly documented) control flow a bit. Before, if real_prev == next, we would skip:

load_mm_cr4(next);
switch_ldt(real_prev, next);

Now we don't any more. I think you should reinstate that optimization. It's probably as simple as wrapping them in an if (real_priv != next) with a comment like /* Remote changes that would require a cr4 or ldt reload will unconditionally send an IPI even to lazy CPUs. So, if we aren't changing our mm, we don't need to refresh cr4 or the ldt */

Hmm. load_mm_cr4() should bypass itself when mm == &init_mm. Want to fix that part or should I?

Quoted 13 lines · Andy Lutomirski

I think you've introduced a minor-ish performance regression due to changing the old (admittedly terribly documented) control flow a bit. Before, if real_prev == next, we would skip:

load_mm_cr4(next);
switch_ldt(real_prev, next);

Quoted 6 lines

Now we don't any more. I think you should reinstate that optimization. It's probably as simple as wrapping them in an if (real_priv != next) with a comment like /* Remote changes that would require a cr4 or ldt reload will unconditionally send an IPI even to lazy CPUs. So, if we aren't changing our mm, we don't need to refresh cr4 or the ldt */

Looks like switch_ldt already skips reloading the LDT when prev equals next, or when they simply have the same LDT values:

       if (unlikely((unsigned long)prev->context.ldt |
                    (unsigned long)next->context.ldt))
               load_mm_ldt(next);

It appears that the cr4 bits have a similar optimization:

static inline void cr4_set_bits(unsigned long mask)

{
       unsigned long cr4, flags;

       local_irq_save(flags);
       cr4 = this_cpu_read(cpu_tlbstate.cr4);
       if ((cr4 | mask) != cr4)
               __cr4_set(cr4 | mask);
       local_irq_restore(flags);
}

Quoted 1 line

Quoted 3 lines

Hmm. load_mm_cr4() should bypass itself when mm == &init_mm. Want to fix that part or should I?

Looks like there might not be anything to do here, after all.

On to the lazy TLB mm_struct refcounting stuff :)

I guess we can skip both switch_ldt and load_mm_cr4 if real_prev equals next?

On to the lazy TLB mm_struct refcounting stuff :)

Quoted 4 lines

Which refcount? mm_users shouldn’t be hot, so I assume you’re talking about mm_count. My suggestion is to get rid of mm_count instead of trying to optimize it.

Do you have any suggestions on how? :)

The TLB shootdown sent at __exit_mm time does not get rid of the kernelthread->active_mm
pointer pointing at the mm that is exiting.

[I added PeterZ and Vitaly -- can you see any way in which this would break something obscure? I don't.]

Quoted 2 lines · Rik van Riel

I guess we can skip both switch_ldt and load_mm_cr4 if real_prev equals next?

Yes, AFAICS.

Quoted 9 lines

On to the lazy TLB mm_struct refcounting stuff :)

Quoted 4 lines

Which refcount? mm_users shouldn’t be hot, so I assume you’re talking about mm_count. My suggestion is to get rid of mm_count instead of trying to optimize it.

Quoted 6 lines

Do you have any suggestions on how? :)

The TLB shootdown sent at __exit_mm time does not get rid of the kernelthread->active_mm
pointer pointing at the mm that is exiting.

Ah, but that's conceptually very easy to fix. Add a #define like ARCH_NO_TASK_ACTIVE_MM. Then just get rid of active_mm if that #define is set. After some grepping, there are very few users. The only nontrivial ones are the ones in kernel/ and mm/mmu_context.c that are involved in the rather complicated dance of refcounting active_mm. If that field goes away, it doesn't need to be refcounted. Instead, I think the refcounting can get replaced with something like:

/*
* Release any arch-internal references to mm. Only called when
mm_users is zero
* and all tasks using mm have either been switch_mm()'d away or have had
* enter_lazy_tlb() called.
*/
extern void arch_shoot_down_dead_mm(struct mm_struct *mm);

which the kernel calls in __mmput() after tearing down all the page tables. The body can be something like:

if (WARN_ON(cpumask_any_but(mm_cpumask(...), ...)) {
/* send an IPI. Maybe just call tlb_flush_remove_tables() */
}

(You'll also have to fix up the highly questionable users in arch/x86/platform/efi/efi_64.c, but that's easy.)

Does all that make sense? Basically, as I understand it, the expensive atomic ops you're seeing are all pointless because they're enabling an optimization that hasn't actually worked for a long time, if ever.

Quoted 2 lines · Andy Lutomirski

[I added PeterZ and Vitaly -- can you see any way in which this would break something obscure? I don't.]

Quoted 45 lines · Rik van Riel

Quoted 2 lines

I guess we can skip both switch_ldt and load_mm_cr4 if real_prev equals next?

Yes, AFAICS.

Quoted 9 lines

On to the lazy TLB mm_struct refcounting stuff :)

Quoted 4 lines

Which refcount? mm_users shouldn’t be hot, so I assume you’re talking about mm_count. My suggestion is to get rid of mm_count instead of trying to optimize it.

Quoted 6 lines

Do you have any suggestions on how? :)

The TLB shootdown sent at __exit_mm time does not get rid of the kernelthread->active_mm
pointer pointing at the mm that is exiting.

Ah, but that's conceptually very easy to fix. Add a #define like ARCH_NO_TASK_ACTIVE_MM. Then just get rid of active_mm if that #define is set. After some grepping, there are very few users. The only nontrivial ones are the ones in kernel/ and mm/mmu_context.c that are involved in the rather complicated dance of refcounting active_mm. If that field goes away, it doesn't need to be refcounted. Instead, I think the refcounting can get replaced with something like:

/*
* Release any arch-internal references to mm. Only called when
mm_users is zero
* and all tasks using mm have either been switch_mm()'d away or have had
* enter_lazy_tlb() called.
*/
extern void arch_shoot_down_dead_mm(struct mm_struct *mm);

which the kernel calls in __mmput() after tearing down all the page tables. The body can be something like:

if (WARN_ON(cpumask_any_but(mm_cpumask(...), ...)) {
/* send an IPI. Maybe just call tlb_flush_remove_tables() */
}

Quoted 7 lines

(You'll also have to fix up the highly questionable users in arch/x86/platform/efi/efi_64.c, but that's easy.)

Does all that make sense? Basically, as I understand it, the expensive atomic ops you're seeing are all pointless because they're enabling an optimization that hasn't actually worked for a long time, if ever.

Hmm. Xen PV has a big hack in xen_exit_mmap(), which is called from arch_exit_mmap(), I think. It's a heavier weight version of more or less the same thing that arch_shoot_down_dead_mm() would be, except that it happens before exit_mmap(). But maybe Xen actually has the right idea. In other words, rather doing the big pagetable free in exit_mmap() while there may still be other CPUs pointing at the page tables, the other order might make more sense. So maybe, if ARCH_NO_TASK_ACTIVE_MM is set, arch_exit_mmap() should be responsible for getting rid of all secret arch references to the mm.

Hmm. ARCH_FREE_UNUSED_MM_IMMEDIATELY might be a better name.

I added some more arch maintainers. The idea here is that, on x86 at least, task->active_mm and all its refcounting is pure overhead. When a process exits, __mmput() gets called, but the core kernel has a longstanding "optimization" in which other tasks (kernel threads and idle tasks) may have ->active_mm pointing at this mm. This is nasty, complicated, and hurts performance on large systems, since it requires extra atomic operations whenever a CPU switches between real users threads and idle/kernel threads.

It's also almost completely worthless on x86 at least, since __mmput() frees pagetables, and that operation *already* forces a remote TLB flush, so we might as well zap all the active_mm references at the same time.

But arm64 has real HW remote flushes. Does arm64 actually benefit from the active_mm optimization? What happens on arm64 when a process exits? How about s390? I suspect that x390 has rather larger systems than arm64, where the cost of the reference counting can be much higher.

(Also, Rik, x86 on Hyper-V has remote flushes, too. How does that interact with your previous patch set?)

I added some more arch maintainers. The idea here is that, on x86 at least, task->active_mm and all its refcounting is pure overhead. When a process exits, __mmput() gets called, but the core kernel has a longstanding "optimization" in which other tasks (kernel threads and idle tasks) may have ->active_mm pointing at this mm. This is nasty, complicated, and hurts performance on large systems, since it requires extra atomic operations whenever a CPU switches between real users threads and idle/kernel threads.

It's also almost completely worthless on x86 at least, since __mmput() frees pagetables, and that operation *already* forces a remote TLB flush, so we might as well zap all the active_mm references at the same time.

Quoted 14 lines · Andy Lutomirski

I added some more arch maintainers. The idea here is that, on x86 at
least, task->active_mm and all its refcounting is pure overhead. When
a process exits, __mmput() gets called, but the core kernel has a longstanding "optimization" in which other tasks (kernel threads and
idle tasks) may have ->active_mm pointing at this mm. This is nasty,
complicated, and hurts performance on large systems, since it requires
extra atomic operations whenever a CPU switches between real users threads and idle/kernel threads.

Quoted 5 lines

It's also almost completely worthless on x86 at least, since __mmput()
frees pagetables, and that operation *already* forces a remote TLB flush, so we might as well zap all the active_mm references at the same time.

So I disagree that active_mm is complicated (the code is less than ideal
but that is actually fixable). And aside from the process exit case, it
does avoid CR3 writes when switching between user and kernel threads (which can be far more often than exit if you have longer running tasks).

Now agreed, recent x86 work has made that less important.

And I of course also agree that not doing those refcount atomics is better.

Quoted 48 lines · Andy Lutomirski

[I added PeterZ and Vitaly -- can you see any way in which this would break something obscure? I don't.]

Quoted 2 lines · Rik van Riel

I guess we can skip both switch_ldt and load_mm_cr4 if real_prev equals next?

Yes, AFAICS.

Quoted 9 lines

On to the lazy TLB mm_struct refcounting stuff :)

Quoted 4 lines

Which refcount? mm_users shouldn’t be hot, so I assume you’re talking about mm_count. My suggestion is to get rid of mm_count instead of trying to optimize it.

Quoted 6 lines

Do you have any suggestions on how? :)

The TLB shootdown sent at __exit_mm time does not get rid of the kernelthread->active_mm
pointer pointing at the mm that is exiting.

Ah, but that's conceptually very easy to fix. Add a #define like ARCH_NO_TASK_ACTIVE_MM. Then just get rid of active_mm if that #define is set. After some grepping, there are very few users. The only nontrivial ones are the ones in kernel/ and mm/mmu_context.c that are involved in the rather complicated dance of refcounting active_mm. If that field goes away, it doesn't need to be refcounted. Instead, I think the refcounting can get replaced with something like:

/*
* Release any arch-internal references to mm. Only called when
mm_users is zero
* and all tasks using mm have either been switch_mm()'d away or have had
* enter_lazy_tlb() called.
*/
extern void arch_shoot_down_dead_mm(struct mm_struct *mm);

which the kernel calls in __mmput() after tearing down all the page tables. The body can be something like:

if (WARN_ON(cpumask_any_but(mm_cpumask(...), ...)) {
/* send an IPI. Maybe just call tlb_flush_remove_tables() */
}

Quoted 7 lines

(You'll also have to fix up the highly questionable users in arch/x86/platform/efi/efi_64.c, but that's easy.)

Does all that make sense? Basically, as I understand it, the expensive atomic ops you're seeing are all pointless because they're enabling an optimization that hasn't actually worked for a long time, if ever.

Hmm. Xen PV has a big hack in xen_exit_mmap(), which is called from arch_exit_mmap(), I think. It's a heavier weight version of more or less the same thing that arch_shoot_down_dead_mm() would be, except that it happens before exit_mmap(). But maybe Xen actually has the right idea. In other words, rather doing the big pagetable free in exit_mmap() while there may still be other CPUs pointing at the page tables, the other order might make more sense. So maybe, if ARCH_NO_TASK_ACTIVE_MM is set, arch_exit_mmap() should be responsible for getting rid of all secret arch references to the mm.

Hmm. ARCH_FREE_UNUSED_MM_IMMEDIATELY might be a better name.

I added some more arch maintainers. The idea here is that, on x86 at least, task->active_mm and all its refcounting is pure overhead. When a process exits, __mmput() gets called, but the core kernel has a longstanding "optimization" in which other tasks (kernel threads and idle tasks) may have ->active_mm pointing at this mm. This is nasty, complicated, and hurts performance on large systems, since it requires extra atomic operations whenever a CPU switches between real users threads and idle/kernel threads.

It's also almost completely worthless on x86 at least, since __mmput() frees pagetables, and that operation *already* forces a remote TLB flush, so we might as well zap all the active_mm references at the same time.

But arm64 has real HW remote flushes. Does arm64 actually benefit from the active_mm optimization? What happens on arm64 when a process exits? How about s390? I suspect that x390 has rather larger systems than arm64, where the cost of the reference counting can be much higher.

Given that CPUs in lazy TLB mode stay part of the mm_cpumask, that WARN_ON seems misplaced. You are right though, that the mm_cpumask alone should provide enough information for us to avoid a need for both tsk->active_mm and the refcounting.

Does all that make sense? Basically, as I understand it, the expensive atomic ops you're seeing are all pointless because they're enabling an optimization that hasn't actually worked for a long time, if ever.

Our benchmark results suggest that lazy TLB mode works, and makes a measurable performance difference. Getting rid of the atomic ops should make it a little better, though :)

[I added PeterZ and Vitaly -- can you see any way in which this would break something obscure? I don't.]

Quoted 2 lines · Andy Lutomirski

[I added PeterZ and Vitaly -- can you see any way in which this would break something obscure? I don't.]

Thanks for CCing me,

I don't see how this can break things either. At first glance, however, I'm afraid we can add performance penalty to virtualized guests which don't use native_flush_tlb_others() (Hyper-V, KVM): we will be reloading CR3 without a need as we don't look at lazy mode in PV tlb flush functions.

We can either check to switch_mm_irqs_off() that native_flush_tlb_others() is in use or teach PV tlb flush functions to look at lazy mode too.

Quoted 13 lines · Vitaly Kuznetsov

Quoted 2 lines · Andy Lutomirski

[I added PeterZ and Vitaly -- can you see any way in which this would break something obscure? I don't.]

Thanks for CCing me,

I don't see how this can break things either. At first glance, however, I'm afraid we can add performance penalty to virtualized guests which don't use native_flush_tlb_others() (Hyper-V, KVM): we will be reloading CR3 without a need as we don't look at lazy mode in PV tlb flush functions.

Quoted 3 lines

We can either check to switch_mm_irqs_off() that native_flush_tlb_others() is in use or teach PV tlb flush functions to look at lazy mode too.

As Rik noted elsewhere in the thread, kvm_flush_tlb_others() ends up calling native_tlb_flush_others() for all running vcpu threads.

The Hyper-V thing is magical, we can't really do anything about it there. Let them worry about it.

After some grepping, there are very few users. The
only nontrivial ones are the ones in kernel/ and mm/mmu_context.c that are involved in the rather complicated dance of refcounting active_mm.

kernel/sched/core.c +11 -0

+#endif
+	} else {					// to user
+		switch_mm_irqs_off(prev->active_mm, next->mm, next);
+
+#ifdef ARCH_NO_ACTIVE_MM
+		if (!prev->mm) {			// from kernel
+			/* will mmdrop() in finish_task_switch(). */
+			rq->prev_mm = prev->active_mm;
+			prev->active_mm = NULL;
+		}
+#endif
 	}

arch/x86/mm/tlb.c +0 -0

index 26542cc17043..e4156e37aa71 100644
@@ -712,15 +712,50 @@ void tlb_flush_remove_tables_local(void *arg)
 	}

arch/x86/mm/tlb.c +27 -1

+	}
+}
+
 void tlb_flush_remove_tables(struct mm_struct *mm)
 {
 	int cpu = get_cpu();
+	cpumask_var_t lazy_cpus;
+
+	if (cpumask_any_but(mm_cpumask(mm), cpu) >= nr_cpu_ids) {
+		put_cpu();
+		return;
+	}
+
+	if (!zalloc_cpumask_var(&lazy_cpus, GFP_ATOMIC)) {
+		/*
+		 * If the cpumask allocation fails, do a brute force flush
+		 * on all the CPUs that have this mm loaded.
+		 */
+		smp_call_function_many(mm_cpumask(mm),
+				tlb_flush_remove_tables_local, (void *)mm, 1);
+		put_cpu();
+		return;
+	}
+
 	/*
-	 * XXX: this really only needs to be called for CPUs in lazy TLB mode.
+	 * CPUs with !is_lazy either received a TLB flush IPI while the user
+	 * pages in this address range were unmapped, or have context switched
+	 * and reloaded %CR3 since then.
+	 *
+	 * Shootdown IPIs at page table freeing time only need to be sent to
+	 * CPUs that may have out of date TLB contents.

Quoted 8 lines · Rik van Riel

Quoted 4 lines · On Jul 17, 2018, at 7:39 AM, Peter Zijlstra <[email protected]…

Also, was there a reason to re-implement on_each_cpu_cond() ? (which btw also wants that __cpumask_set_bit fix).

I did not use on_each_cpu_cond() because I had no idea it existed. A quick grep suggests very few users of that function :)

Quoted 2 lines

I'll make sure things are done the right way.

OK, looking at it some more, I think open coding may be faster in case of the TLB shootdown code, because that way we only iterate over the CPUs in the mm_cpumask, instead of iterating over every single online CPU in the system, and calling the helper function for every CPU, like on_each_cpu_cond() does.

However, the difference in overhead might be small enough that we might not notice. Preferences?