mm, sched: Rework lazy mm handling

We had a nice comment at the top of membarrier.c explaining why membarrier worked in a handful of scenarios, but that consisted more of a list of things not to forget than an actual description of the algorithm and why it should be expected to work.

Add a comment explaining my understanding of the algorithm. This exposes a couple of implementation issues that I will hopefully fix up in subsequent patches.

Cc Mathieu Desnoyers <[email protected]>

Cc Nicholas Piggin <[email protected]>

Cc Peter Zijlstra <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---
kernel/sched/membarrier.c | 60 +++++++++++++++++++++++++++++++++++++--

1 file changed, 58 insertions(+), 2 deletions(-)

kernel/sched/membarrier.c +58 -3

index b5add64d9698..30e964b9689d 100644
@@ -7,8 +7,64 @@
 #include "sched.h"
 
 /*
- * For documentation purposes, here are some membarrier ordering
- * scenarios to keep in mind:
+ * The basic principle behind the regular memory barrier mode of
+ * membarrier() is as follows.  membarrier() is called in one thread.  Tt
+ * iterates over all CPUs, and, for each CPU, it either sends an IPI to
+ * that CPU or it does not. If it sends an IPI, then we have the
+ * following sequence of events:
+ *
+ * 1. membarrier() does smp_mb().
+ * 2. membarrier() does a store (the IPI request payload) that is observed by
+ *    the target CPU.
+ * 3. The target CPU does smp_mb().
+ * 4. The target CPU does a store (the completion indication) that is observed
+ *    by membarrier()'s wait-for-IPIs-to-finish request.
+ * 5. membarrier() does smp_mb().
+ *
+ * So all pre-membarrier() local accesses are visible after the IPI on the
+ * target CPU and all pre-IPI remote accesses are visible after
+ * membarrier(). IOW membarrier() has synchronized both ways with the target
+ * CPU.
+ *
+ * (This has the caveat that membarrier() does not interrupt the CPU that it's
+ * running on at the time it sends the IPIs. However, if that is the CPU on
+ * which membarrier() starts and/or finishes, membarrier() does smp_mb() and,
+ * if not, then the scheduler's migration of membarrier() is a full barrier.)
+ *
+ * membarrier() skips sending an IPI only if membarrier() sees
+ * cpu_rq(cpu)->curr->mm != target mm.  The sequence of events is:
+ *
+ *           membarrier()            |          target CPU
+ * ---------------------------------------------------------------------
+ *                                   | 1. smp_mb()
+ *                                   | 2. set rq->curr->mm = other_mm
+ *                                   |    (by writing to ->curr or to ->mm)
+ * 3. smp_mb()                       |
+ * 4. read rq->curr->mm == other_mm  |
+ * 5. smp_mb()                       |
+ *                                   | 6. rq->curr->mm = target_mm
+ *                                   |    (by writing to ->curr or to ->mm)
+ *                                   | 7. smp_mb()
+ *                                   |
+ *
+ * All memory accesses on the target CPU prior to scheduling are visible
+ * to membarrier()'s caller after membarrier() returns due to steps 1, 2, 4
+ * and 5.
+ *
+ * All memory accesses by membarrier()'s caller prior to membarrier() are
+ * visible to the target CPU after scheduling due to steps 3, 4, 6, and 7.
+ *
+ * Note that, tasks can change their ->mm, e.g. via kthread_use_mm().  So
+ * tasks that switch their ->mm must follow the same rules as the scheduler
+ * changing rq->curr, and the membarrier() code needs to do both dereferences
+ * carefully.
+ *
+ * GLOBAL_EXPEDITED support works the same way except that all references
+ * to rq->curr->mm are replaced with references to rq->membarrier_state.
+ *
+ *
+ * Specific examples of how this produces the documented properties of
+ * membarrier():
 *
 * A) Userspace thread execution after IPI vs membarrier's memory
 *    barrier before sending the IPI
--

2.33.1

The core scheduler isn't a great place for membarrier_mm_sync_core_before_usermode() -- the core scheduler doesn't actually know whether we are lazy. With the old code, if a CPU is running a membarrier-registered task, goes idle, gets unlazied via a TLB shootdown IPI, and switches back to the membarrier-registered task, it will do an unnecessary core sync.

Conveniently, x86 is the only architecture that does anything in this sync_core_before_usermode(), so membarrier_mm_sync_core_before_usermode() is a no-op on all other architectures and we can just move the code.

(I am not claiming that the SYNC_CORE code was correct before or after this change on any non-x86 architecture. I merely claim that this change improves readability, is correct on x86, and makes no change on any other architecture.)

Cc Mathieu Desnoyers <[email protected]>

Cc Nicholas Piggin <[email protected]>

Cc Peter Zijlstra <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---

arch/x86/mm/tlb.c        | 58 +++++++++++++++++++++++++++++++---------
include/linux/sched/mm.h | 13 ---------
kernel/sched/core.c      | 14 +++++-----
3 files changed, 53 insertions(+), 32 deletions(-)

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

index 59ba2968af1b..1ae15172885e 100644
@@ -9,6 +9,7 @@
 #include <linux/cpu.h>
 #include <linux/debugfs.h>
 #include <linux/sched/smt.h>
+#include <linux/sched/mm.h>
 
 #include <asm/tlbflush.h>
 #include <asm/mmu_context.h>

@@ -485,6 +486,15 @@ void cr4_update_pce(void *ignored)
 static inline void cr4_update_pce_mm(struct mm_struct *mm) { }
 #endif
 
+static void sync_core_if_membarrier_enabled(struct mm_struct *next)
+{
+#ifdef CONFIG_MEMBARRIER
+	if (unlikely(atomic_read(&next->membarrier_state) &
+		     MEMBARRIER_STATE_PRIVATE_EXPEDITED_SYNC_CORE))
+		sync_core_before_usermode();
+#endif
+}
+
 void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
 			struct task_struct *tsk)
 {

@@ -539,16 +549,24 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct
 mm_struct *next,
 		this_cpu_write(cpu_tlbstate_shared.is_lazy, false);
 
 	/*
-	 * The membarrier system call requires a full memory barrier and
-	 * core serialization before returning to user-space, after
-	 * storing to rq->curr, when changing mm.  This is because
-	 * membarrier() sends IPIs to all CPUs that are in the target mm
-	 * to make them issue memory barriers.  However, if another CPU
-	 * switches to/from the target mm concurrently with
-	 * membarrier(), it can cause that CPU not to receive an IPI
-	 * when it really should issue a memory barrier.  Writing to CR3
-	 * provides that full memory barrier and core serializing
-	 * instruction.
+	 * membarrier() support requires that, when we change rq->curr->mm:
+	 *
+	 *  - If next->mm has membarrier registered, a full memory barrier
+	 *    after writing rq->curr (or rq->curr->mm if we switched the mm
+	 *    without switching tasks) and before returning to user mode.
+	 *
+	 *  - If next->mm has SYNC_CORE registered, then we sync core before
+	 *    returning to user mode.
+	 *
+	 * In the case where prev->mm == next->mm, membarrier() uses an IPI
+	 * instead, and no particular barriers are needed while context
+	 * switching.
+	 *
+	 * x86 gets all of this as a side-effect of writing to CR3 except
+	 * in the case where we unlazy without flushing.
+	 *
+	 * All other architectures are civilized and do all of this implicitly
+	 * when transitioning from kernel to user mode.
 	 */
 	if (real_prev == next) {

		VM_WARN_ON(this_cpu_read(cpu_tlbstate.ctxs[prev_asid].ctx_id) !=

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

@@ -566,7 +584,8 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct
 mm_struct *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.
+		 * process. No TLB flush or membarrier() synchronization
+		 * is required.
 		 */
 		if (!was_lazy)

			return;

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

@@ -576,16 +595,31 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct
 mm_struct *next,
 		 * If the TLB is up to date, just use it.
 		 * The barrier synchronizes with the tlb_gen increment in
 		 * the TLB shootdown code.
+		 *
+		 * As a future optimization opportunity, it's plausible
+		 * that the x86 memory model is strong enough that this
+		 * smp_mb() isn't needed.
 		 */
 		smp_mb();
 		next_tlb_gen = atomic64_read(&next->context.tlb_gen);
 		if (this_cpu_read(cpu_tlbstate.ctxs[prev_asid].tlb_gen) ==
-				next_tlb_gen)
+		    next_tlb_gen) {
+			/*
+			 * We switched logical mm but we're not going to
+			 * write to CR3.  We already did smp_mb() above,
+			 * but membarrier() might require a sync_core()
+			 * as well.
+			 */
+			sync_core_if_membarrier_enabled(next);
+
 			return;
+		}
 
 		/*
 		 * TLB contents went out of date while we were in lazy
 		 * mode. Fall through to the TLB switching code below.
+		 * No need for an explicit membarrier invocation -- the CR3
+		 * write will serialize.
 		 */
 		new_asid = prev_asid;

		need_flush = true;

include/linux/sched/mm.h +0 -10

index 5561486fddef..c256a7fc0423 100644
@@ -345,16 +345,6 @@ enum {
 #include <asm/membarrier.h>
 #endif
 
-static inline void membarrier_mm_sync_core_before_usermode(struct mm_struct
 *mm)
-{
-	if (current->mm != mm)
-		return;
-	if (likely(!(atomic_read(&mm->membarrier_state) &
-		     MEMBARRIER_STATE_PRIVATE_EXPEDITED_SYNC_CORE)))
-		return;
-	sync_core_before_usermode();
-}
-
 extern void membarrier_exec_mmap(struct mm_struct *mm);
 

extern void membarrier_update_current_mm(struct mm_struct *next_mm);

include/linux/sched/mm.h +0 -3

@@ -370,9 +360,6 @@ static inline void membarrier_arch_switch_mm(struct
 mm_struct *prev,
 static inline void membarrier_exec_mmap(struct mm_struct *mm)
 {
 }
-static inline void membarrier_mm_sync_core_before_usermode(struct mm_struct
 *mm)
-{
-}
 static inline void membarrier_update_current_mm(struct mm_struct *next_mm)

{
}

kernel/sched/core.c +7 -7

index f21714ea3db8..6a1db8264c7b 100644
@@ -4822,22 +4822,22 @@ static struct rq *finish_task_switch(struct task_struct
 *prev)
 	kmap_local_sched_in();
 
 	fire_sched_in_preempt_notifiers(current);
+
 	/*
 	 * When switching through a kernel thread, the loop in
 	 * membarrier_{private,global}_expedited() may have observed that
 	 * kernel thread and not issued an IPI. It is therefore possible to
 	 * schedule between user->kernel->user threads without passing though
 	 * switch_mm(). Membarrier requires a barrier after storing to
-	 * rq->curr, before returning to userspace, so provide them here:
+	 * rq->curr, before returning to userspace, and mmdrop() provides
+	 * this barrier.
 	 *
-	 * - a full memory barrier for {PRIVATE,GLOBAL}_EXPEDITED, implicitly
-	 *   provided by mmdrop(),
-	 * - a sync_core for SYNC_CORE.
+	 * If an architecture needs to take a specific action for
+	 * SYNC_CORE, it can do so in switch_mm_irqs_off().
 	 */
-	if (mm) {
-		membarrier_mm_sync_core_before_usermode(mm);
+	if (mm)
 		mmdrop(mm);
-	}
+
 	if (unlikely(prev_state == TASK_DEAD)) {
 		if (prev->sched_class->task_dead)

			prev->sched_class->task_dead(prev);

--
2.33.1

membarrier() needs a barrier after any CPU changes mm. There is currently a comment explaining why this barrier probably exists in all cases. The logic is based on ensuring that the barrier exists on every control flow path through the scheduler. It also relies on mmgrab() and mmdrop() being full barriers.

mmgrab() and mmdrop() would be better if they were not full barriers. As a trivial optimization, mmgrab() could use a relaxed atomic and mmdrop() could use a release on architectures that have these operations. Larger optimizations are also in the works. Doing any of these optimizations while preserving an unnecessary barrier will complicate the code and penalize non-membarrier-using tasks.

Simplify the logic by adding an explicit barrier, and allow architectures to override it as an optimization if they want to.

One of the deleted comments in this patch said "It is therefore possible to schedule between user->kernel->user threads without passing through switch_mm()". It is possible to do this without, say, writing to CR3 on x86, but the core scheduler indeed calls switch_mm_irqs_off() to tell the arch code to go back from lazy mode to no-lazy mode.

The membarrier_finish_switch_mm() call in exec_mmap() is a no-op so long as there is no way for a newly execed program to register for membarrier prior to running user code. Subsequent patches will merge the exec_mmap() code with the kthread_use_mm() code, though, and keeping the paths consistent will make the result more comprehensible.

Cc Mathieu Desnoyers <[email protected]>

Cc Nicholas Piggin <[email protected]>

Cc Peter Zijlstra <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---

fs/exec.c                |  1 +
include/linux/sched/mm.h | 18 ++++++++++++++++++
kernel/kthread.c         | 12 +-----------
kernel/sched/core.c      | 34 +++++++++-------------------------
4 files changed, 29 insertions(+), 36 deletions(-)

fs/exec.c +1 -0

index a098c133d8d7..3abbd0294e73 100644
@@ -1019,6 +1019,7 @@ static int exec_mmap(struct mm_struct *mm)
 	activate_mm(active_mm, mm);
 	if (IS_ENABLED(CONFIG_ARCH_WANT_IRQS_OFF_ACTIVATE_MM))
 		local_irq_enable();
+	membarrier_finish_switch_mm(mm);
 	tsk->mm->vmacache_seqnum = 0;
 	vmacache_flush(tsk);
 	task_unlock(tsk);

include/linux/sched/mm.h +18 -0

index 0df706c099e5..e8919995d8dd 100644
@@ -349,6 +349,20 @@ extern void membarrier_exec_mmap(struct mm_struct *mm);
 
 extern void membarrier_update_current_mm(struct mm_struct *next_mm);
 
+/*
+ * Called by the core scheduler after calling switch_mm_irqs_off().
+ * Architectures that have implicit barriers when switching mms can
+ * override this as an optimization.
+ */
+#ifndef membarrier_finish_switch_mm
+static inline void membarrier_finish_switch_mm(struct mm_struct *mm)
+{
+	if (atomic_read(&mm->membarrier_state) &
+	    (MEMBARRIER_STATE_GLOBAL_EXPEDITED | MEMBARRIER_STATE_PRIVATE_EXPEDITED))
+		smp_mb();
+}
+#endif
+
 #else
 static inline void membarrier_exec_mmap(struct mm_struct *mm)
 {

@@ -356,6 +370,10 @@ static inline void membarrier_exec_mmap(struct mm_struct
 *mm)
 static inline void membarrier_update_current_mm(struct mm_struct *next_mm)
 {
 }
+static inline void membarrier_finish_switch_mm(struct mm_struct *mm)
+{
+}
+
 #endif
 

#endif /* _LINUX_SCHED_MM_H */

kernel/kthread.c +1 -11

index 5b37a8567168..396ae78a1a34 100644
@@ -1361,25 +1361,15 @@ void kthread_use_mm(struct mm_struct *mm)
 	tsk->mm = mm;
 	membarrier_update_current_mm(mm);
 	switch_mm_irqs_off(active_mm, mm, tsk);
+	membarrier_finish_switch_mm(mm);
 	local_irq_enable();
 	task_unlock(tsk);
 #ifdef finish_arch_post_lock_switch
 	finish_arch_post_lock_switch();
 #endif
 
-	/*
-	 * When a kthread starts operating on an address space, the loop
-	 * in membarrier_{private,global}_expedited() may not observe
-	 * that tsk->mm, and not issue an IPI. Membarrier requires a
-	 * memory barrier after storing to tsk->mm, before accessing
-	 * user-space memory. A full memory barrier for membarrier
-	 * {PRIVATE,GLOBAL}_EXPEDITED is implicitly provided by
-	 * mmdrop(), or explicitly with smp_mb().
-	 */
 	if (active_mm != mm)
 		mmdrop(active_mm);
-	else
-		smp_mb();
 
 	to_kthread(tsk)->oldfs = force_uaccess_begin();
 }

kernel/sched/core.c +0 -8

index 6a1db8264c7b..917068b0a145 100644
@@ -4824,14 +4824,6 @@ static struct rq *finish_task_switch(struct task_struct
 *prev)
 	fire_sched_in_preempt_notifiers(current);
 
 	/*
-	 * When switching through a kernel thread, the loop in
-	 * membarrier_{private,global}_expedited() may have observed that
-	 * kernel thread and not issued an IPI. It is therefore possible to
-	 * schedule between user->kernel->user threads without passing though
-	 * switch_mm(). Membarrier requires a barrier after storing to
-	 * rq->curr, before returning to userspace, and mmdrop() provides
-	 * this barrier.
-	 *
 	 * If an architecture needs to take a specific action for
 	 * SYNC_CORE, it can do so in switch_mm_irqs_off().

	 */

kernel/sched/core.c +9 -18

@@ -4915,15 +4907,14 @@ context_switch(struct rq *rq, struct task_struct *prev,
 			prev->active_mm = NULL;
 	} else {                                        // to user
 		membarrier_switch_mm(rq, prev->active_mm, next->mm);
+		switch_mm_irqs_off(prev->active_mm, next->mm, next);
+
 		/*
 		 * sys_membarrier() requires an smp_mb() between setting
-		 * rq->curr / membarrier_switch_mm() and returning to userspace.
-		 *
-		 * The below provides this either through switch_mm(), or in
-		 * case 'prev->active_mm == next->mm' through
-		 * finish_task_switch()'s mmdrop().
+		 * rq->curr->mm to a membarrier-enabled mm and returning
+		 * to userspace.
 		 */
-		switch_mm_irqs_off(prev->active_mm, next->mm, next);
+		membarrier_finish_switch_mm(next->mm);
 
 		if (!prev->mm) {                        // from kernel
 			/* will mmdrop() in finish_task_switch(). */

@@ -6264,17 +6255,10 @@ static void __sched notrace __schedule(unsigned int
 sched_mode)
 		RCU_INIT_POINTER(rq->curr, next);
 		/*
 		 * The membarrier system call requires each architecture
-		 * to have a full memory barrier after updating
-		 * rq->curr, before returning to user-space.
-		 *
-		 * Here are the schemes providing that barrier on the
-		 * various architectures:
-		 * - mm ? switch_mm() : mmdrop() for x86, s390, sparc, PowerPC.
-		 *   switch_mm() rely on membarrier_arch_switch_mm() on PowerPC.
-		 * - finish_lock_switch() for weakly-ordered
-		 *   architectures where spin_unlock is a full barrier,
-		 * - switch_to() for arm64 (weakly-ordered, spin_unlock
-		 *   is a RELEASE barrier),
+		 * to have a full memory barrier before and after updating
+		 * rq->curr->mm, before returning to userspace.  This
+		 * is provided by membarrier_finish_switch_mm().  Architectures
+		 * that want to optimize this can override that function.
 		 */
 		++*switch_count;
 
--

2.33.1

membarrier reads cpu_rq(remote cpu)->curr->mm without locking. Use READ_ONCE() and WRITE_ONCE() to remove the data races.

Cc Mathieu Desnoyers <[email protected]>

Cc Nicholas Piggin <[email protected]>

Cc Peter Zijlstra <[email protected]>

Acked-by Nicholas Piggin <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---

fs/exec.c                 | 2 +-
kernel/exit.c             | 2 +-
kernel/kthread.c          | 4 ++--
kernel/sched/membarrier.c | 7 ++++---
4 files changed, 8 insertions(+), 7 deletions(-)

fs/exec.c +1 -1

index 3abbd0294e73..38b05e01c5bd 100644
@@ -1006,7 +1006,7 @@ static int exec_mmap(struct mm_struct *mm)
 	local_irq_disable();
 	active_mm = tsk->active_mm;
 	tsk->active_mm = mm;
-	tsk->mm = mm;
+	WRITE_ONCE(tsk->mm, mm);  /* membarrier reads this without locks */
 	/*
 	 * This prevents preemption while active_mm is being loaded and
 	 * it and mm are being updated, which could cause problems for

kernel/exit.c +1 -1

index 91a43e57a32e..70f2cbc42015 100644
@@ -491,7 +491,7 @@ static void exit_mm(void)
 	 */
 	smp_mb__after_spinlock();
 	local_irq_disable();
-	current->mm = NULL;
+	WRITE_ONCE(current->mm, NULL);
 	membarrier_update_current_mm(NULL);
 	enter_lazy_tlb(mm, current);
 	local_irq_enable();

kernel/kthread.c +2 -2

index 396ae78a1a34..3b18329f885c 100644
@@ -1358,7 +1358,7 @@ void kthread_use_mm(struct mm_struct *mm)
 		mmgrab(mm);
 		tsk->active_mm = mm;
 	}
-	tsk->mm = mm;
+	WRITE_ONCE(tsk->mm, mm);  /* membarrier reads this without locks */
 	membarrier_update_current_mm(mm);
 	switch_mm_irqs_off(active_mm, mm, tsk);
 	membarrier_finish_switch_mm(mm);

@@ -1399,7 +1399,7 @@ void kthread_unuse_mm(struct mm_struct *mm)
 	smp_mb__after_spinlock();
 	sync_mm_rss(mm);
 	local_irq_disable();
-	tsk->mm = NULL;
+	WRITE_ONCE(tsk->mm, NULL);  /* membarrier reads this without locks */
 	membarrier_update_current_mm(NULL);
 	/* active_mm is still 'mm' */
 	enter_lazy_tlb(mm, tsk);

kernel/sched/membarrier.c +1 -1

index 30e964b9689d..327830f89c37 100644
@@ -411,7 +411,7 @@ static int membarrier_private_expedited(int flags, int
 cpu_id)
 			goto out;
 		rcu_read_lock();
 		p = rcu_dereference(cpu_rq(cpu_id)->curr);
-		if (!p || p->mm != mm) {
+		if (!p || READ_ONCE(p->mm) != mm) {
 			rcu_read_unlock();
 			goto out;

		}

kernel/sched/membarrier.c +1 -1

@@ -424,7 +424,7 @@ static int membarrier_private_expedited(int flags, int
 cpu_id)
 			struct task_struct *p;
 
 			p = rcu_dereference(cpu_rq(cpu)->curr);
-			if (p && p->mm == mm)
+			if (p && READ_ONCE(p->mm) == mm)
 				__cpumask_set_cpu(cpu, tmpmask);
 		}

		rcu_read_unlock();

kernel/sched/membarrier.c +2 -1

@@ -522,7 +522,8 @@ static int sync_runqueues_membarrier_state(struct mm_struct
 *mm)
 		struct task_struct *p;
 
 		p = rcu_dereference(rq->curr);
-		if (p && p->mm == mm)
+		/* exec and kthread_use_mm() write ->mm without locks */
+		if (p && READ_ONCE(p->mm) == mm)
 			__cpumask_set_cpu(cpu, tmpmask);
 	}

	rcu_read_unlock();

--
2.33.1

powerpc did the following on some, but not all, paths through switch_mm_irqs_off():

/*
* Only need the full barrier when switching between processes.
* Barrier when switching from kernel to userspace is not
* required here, given that it is implied by mmdrop(). Barrier
* when switching from userspace to kernel is not needed after
* store to rq->curr.
*/
if (likely(!(atomic_read(&next->membarrier_state) &
(MEMBARRIER_STATE_PRIVATE_EXPEDITED |
MEMBARRIER_STATE_GLOBAL_EXPEDITED)) || !prev))
return;

This is puzzling: if !prev, then one might expect that we are switching from kernel to user, not user to kernel, which is inconsistent with the comment. But this is all nonsense, because the one and only caller would never have prev == NULL and would, in fact, OOPS if prev == NULL.

In any event, this code is unnecessary, since the new generic membarrier_finish_switch_mm() provides the same barrier without arch help.

arch/powerpc/include/asm/membarrier.h remains as an empty header, because a later patch in this series will add code to it.

Cc Michael Ellerman <[email protected]>

Cc Benjamin Herrenschmidt <[email protected]>

Cc Paul Mackerras <[email protected]>

Cc [email protected]

Cc Nicholas Piggin <[email protected]>

Cc Mathieu Desnoyers <[email protected]>

Cc Peter Zijlstra <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---
  arch/powerpc/include/asm/membarrier.h | 24 ------------------------
  arch/powerpc/mm/mmu_context.c         |  1 -
  2 files changed, 25 deletions(-)

arch/powerpc/include/asm/membarrier.h +0 -24

index de7f79157918..b90766e95bd1 100644
@@ -1,28 +1,4 @@
  #ifndef _ASM_POWERPC_MEMBARRIER_H
  #define _ASM_POWERPC_MEMBARRIER_H
 
-static inline void membarrier_arch_switch_mm(struct mm_struct *prev,
-					     struct mm_struct *next,
-					     struct task_struct *tsk)
-{
-	/*
-	 * Only need the full barrier when switching between processes.
-	 * Barrier when switching from kernel to userspace is not
-	 * required here, given that it is implied by mmdrop(). Barrier
-	 * when switching from userspace to kernel is not needed after
-	 * store to rq->curr.
-	 */
-	if (IS_ENABLED(CONFIG_SMP) &&
-	    likely(!(atomic_read(&next->membarrier_state) &
-		     (MEMBARRIER_STATE_PRIVATE_EXPEDITED |
-		      MEMBARRIER_STATE_GLOBAL_EXPEDITED)) || !prev))
-		return;
-
-	/*
-	 * The membarrier system call requires a full memory barrier
-	 * after storing to rq->curr, before going back to user-space.
-	 */
-	smp_mb();
-}
-
  #endif /* _ASM_POWERPC_MEMBARRIER_H */

arch/powerpc/mm/mmu_context.c +0 -0

index 74246536b832..5f2daa6b0497 100644

arch/powerpc/mm/mmu_context.c +0 -0

  	/*
  	 * The actual HW switching method differs between the various

powerpc did the following on some, but not all, paths through switch_mm_irqs_off():

/*
* Only need the full barrier when switching between processes.
* Barrier when switching from kernel to userspace is not
* required here, given that it is implied by mmdrop(). Barrier
* when switching from userspace to kernel is not needed after
* store to rq->curr.
*/
if (likely(!(atomic_read(&next->membarrier_state) &
(MEMBARRIER_STATE_PRIVATE_EXPEDITED |
MEMBARRIER_STATE_GLOBAL_EXPEDITED)) || !prev))
return;

This is puzzling: if !prev, then one might expect that we are switching from kernel to user, not user to kernel, which is inconsistent with the comment. But this is all nonsense, because the one and only caller would never have prev == NULL and would, in fact, OOPS if prev == NULL.

In any event, this code is unnecessary, since the new generic membarrier_finish_switch_mm() provides the same barrier without arch help.

arch/powerpc/include/asm/membarrier.h remains as an empty header, because a later patch in this series will add code to it.

Cc Michael Ellerman <[email protected]>

Cc Benjamin Herrenschmidt <[email protected]>

Cc Paul Mackerras <[email protected]>

Cc [email protected]

Cc Nicholas Piggin <[email protected]>

Cc Mathieu Desnoyers <[email protected]>

Cc Peter Zijlstra <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---
arch/powerpc/include/asm/membarrier.h | 24 ------------------------
arch/powerpc/mm/mmu_context.c | 1 -

2 files changed, 25 deletions(-)

arch/powerpc/include/asm/membarrier.h +0 -24

index de7f79157918..b90766e95bd1 100644
@@ -1,28 +1,4 @@
 #ifndef _ASM_POWERPC_MEMBARRIER_H
 #define _ASM_POWERPC_MEMBARRIER_H
 
-static inline void membarrier_arch_switch_mm(struct mm_struct *prev,
-					     struct mm_struct *next,
-					     struct task_struct *tsk)
-{
-	/*
-	 * Only need the full barrier when switching between processes.
-	 * Barrier when switching from kernel to userspace is not
-	 * required here, given that it is implied by mmdrop(). Barrier
-	 * when switching from userspace to kernel is not needed after
-	 * store to rq->curr.
-	 */
-	if (IS_ENABLED(CONFIG_SMP) &&
-	    likely(!(atomic_read(&next->membarrier_state) &
-		     (MEMBARRIER_STATE_PRIVATE_EXPEDITED |
-		      MEMBARRIER_STATE_GLOBAL_EXPEDITED)) || !prev))
-		return;
-
-	/*
-	 * The membarrier system call requires a full memory barrier
-	 * after storing to rq->curr, before going back to user-space.
-	 */
-	smp_mb();
-}
-
 #endif /* _ASM_POWERPC_MEMBARRIER_H */

arch/powerpc/mm/mmu_context.c +0 -1

index 74246536b832..5f2daa6b0497 100644
@@ -84,7 +84,6 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct
 mm_struct *next,
 		asm volatile ("dssall");
 
 	if (!new_on_cpu)
-		membarrier_arch_switch_mm(prev, next, tsk);
 
 	/*

* The actual HW switching method differs between the various
--
2.33.1

The old sync_core_before_usermode() comments suggested that a non-icache-syncing return-to-usermode instruction is x86-specific and that all other architectures automatically notice cross-modified code on return to userspace.

This is misleading. The incantation needed to modify code from one CPU and execute it on another CPU is highly architecture dependent. On x86, according to the SDM, one must modify the code, issue SFENCE if the modification was WC or nontemporal, and then issue a "serializing instruction" on the CPU that will execute the code. membarrier() can do the latter.

On arm, arm64 and powerpc, one must flush the icache and then flush the pipeline on the target CPU, although the CPU manuals don't necessarily use this language.

So let's drop any pretense that we can have a generic way to define or implement membarrier's SYNC_CORE operation and instead require all architectures to define the helper and supply their own documentation as to how to use it. This means x86, arm64, and powerpc for now. Let's also rename the function from sync_core_before_usermode() to membarrier_sync_core_before_usermode() because the precise flushing details may very well be specific to membarrier, and even the concept of "sync_core" in the kernel is mostly an x86-ism.

(It may well be the case that, on real x86 processors, synchronizing the icache (which requires no action at all) and "flushing the pipeline" is sufficient, but trying to use this language would be confusing at best. LFENCE does something awfully like "flushing the pipeline", but the SDM does not permit LFENCE as an alternative to a "serializing instruction" for this purpose.)

arch/x86/kernel/alternative.c +0 -0

 #include <asm/text-patching.h>
 #include <asm/alternative.h>
 #include <asm/sections.h>

arch/x86/kernel/cpu/mce/core.c +1 -1

index 193204aee880..a2529e09f620 100644
@@ -41,12 +41,12 @@
 #include <linux/irq_work.h>
 #include <linux/export.h>
 #include <linux/set_memory.h>
-#include <linux/sync_core.h>
 #include <linux/task_work.h>
 #include <linux/hardirq.h>
 
 #include <asm/intel-family.h>
 #include <asm/processor.h>
+#include <asm/sync_core.h>
 #include <asm/traps.h>
 #include <asm/tlbflush.h>
 #include <asm/mce.h>

drivers/misc/sgi-gru/grufault.c +1 -1

index d7ef61e602ed..462c667bd6c4 100644
@@ -20,8 +20,8 @@
 #include <linux/io.h>
 #include <linux/uaccess.h>
 #include <linux/security.h>
-#include <linux/sync_core.h>
 #include <linux/prefetch.h>
+#include <asm/sync_core.h>
 #include "gru.h"
 #include "grutables.h"
 #include "grulib.h"

drivers/misc/sgi-gru/gruhandles.c +1 -1

index 1d75d5e540bc..c8cba1c1b00f 100644
@@ -16,7 +16,7 @@
 #define GRU_OPERATION_TIMEOUT	(((cycles_t) local_cpu_data->itc_freq)*10)
 #define CLKS2NSEC(c)		((c) *1000000000 / local_cpu_data->itc_freq)
 #else
-#include <linux/sync_core.h>
+#include <asm/sync_core.h>
 #include <asm/tsc.h>
 #define GRU_OPERATION_TIMEOUT	((cycles_t) tsc_khz*10*1000)
 #define CLKS2NSEC(c)		((c) * 1000000 / tsc_khz)

drivers/misc/sgi-gru/grukservices.c +1 -1

index 0ea923fe6371..ce03ff3f7c3a 100644
@@ -16,10 +16,10 @@
 #include <linux/miscdevice.h>
 #include <linux/proc_fs.h>
 #include <linux/interrupt.h>
-#include <linux/sync_core.h>
 #include <linux/uaccess.h>
 #include <linux/delay.h>
 #include <linux/export.h>
+#include <asm/sync_core.h>
 #include <asm/io_apic.h>
 #include "gru.h"
 #include "grulib.h"

include/linux/sched/mm.h +0 -1

index e8919995d8dd..e107f292fc42 100644
@@ -7,7 +7,6 @@
 #include <linux/sched.h>
 #include <linux/mm_types.h>
 #include <linux/gfp.h>
-#include <linux/sync_core.h>
 
 /*
 * Routines for handling mm_structs

include/linux/sync_core.h deleted +0 -11

deleted file mode 100644
index 013da4b8b327..000000000000
@@ -1,21 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-#ifndef _LINUX_SYNC_CORE_H
-#define _LINUX_SYNC_CORE_H
-
-#ifdef CONFIG_ARCH_HAS_SYNC_CORE_BEFORE_USERMODE
-#include <asm/sync_core.h>
-#else
-/*
- * This is a dummy sync_core_before_usermode() implementation that can be used
- * on all architectures which return to user-space through core serializing
- * instructions.

exec_mmap() supplies a brand-new mm from mm_alloc(), and membarrier_state is already 0. There's no need to clear it again.

Signed-off-by Andy Lutomirski <[email protected]>

---
kernel/sched/membarrier.c | 1 -

1 file changed, 1 deletion(-)

kernel/sched/membarrier.c +0 -2

index eb73eeaedc7d..c38014c2ed66 100644
@@ -285,7 +285,6 @@ void membarrier_exec_mmap(struct mm_struct *mm)
 	 * clearing this state.
 	 */
 	smp_mb();
-	atomic_set(&mm->membarrier_state, 0);
 	/*
 	 * Keep the runqueue membarrier_state in sync with this mm
 	 * membarrier_state.
--

2.33.1

membarrier() requires a barrier before changes to rq->curr->mm, not just before writes to rq->membarrier_state. Move the barrier in exec_mmap() to the right place.

Add the barrier in kthread_use_mm() -- it was entirely missing before.

This patch makes exec_mmap() and kthread_use_mm() use the same membarrier hooks, which results in some code deletion.

As an added bonus, this will eliminate a redundant barrier in execve() on arches for which spinlock acquisition is a barrier.

Signed-off-by Andy Lutomirski <[email protected]>

---

fs/exec.c                 |  6 +++++-
include/linux/sched/mm.h  |  2 --
kernel/kthread.c          |  5 +++++
kernel/sched/membarrier.c | 15 ---------------
4 files changed, 10 insertions(+), 18 deletions(-)

fs/exec.c +5 -1

index 38b05e01c5bd..325dab98bc51 100644
@@ -1001,12 +1001,16 @@ static int exec_mmap(struct mm_struct *mm)
 	}
 
 	task_lock(tsk);
-	membarrier_exec_mmap(mm);
+	/*
+	 * membarrier() requires a full barrier before switching mm.
+	 */
+	smp_mb__after_spinlock();
 
 	local_irq_disable();
 	active_mm = tsk->active_mm;
 	tsk->active_mm = mm;
 	WRITE_ONCE(tsk->mm, mm);  /* membarrier reads this without locks */
+	membarrier_update_current_mm(mm);
 	/*
 	 * This prevents preemption while active_mm is being loaded and
 	 * it and mm are being updated, which could cause problems for

include/linux/sched/mm.h +0 -2

index e107f292fc42..f1d2beac464c 100644
@@ -344,8 +344,6 @@ enum {
 #include <asm/membarrier.h>
 #endif
 
-extern void membarrier_exec_mmap(struct mm_struct *mm);
-
 extern void membarrier_update_current_mm(struct mm_struct *next_mm);
 
 /*

kernel/kthread.c +5 -0

index 3b18329f885c..18b0a2e0e3b2 100644
@@ -1351,6 +1351,11 @@ void kthread_use_mm(struct mm_struct *mm)
 	WARN_ON_ONCE(tsk->mm);
 
 	task_lock(tsk);
+	/*
+	 * membarrier() requires a full barrier before switching mm.
+	 */
+	smp_mb__after_spinlock();
+
 	/* Hold off tlb flush IPIs while switching mm's */
 	local_irq_disable();
 	active_mm = tsk->active_mm;

kernel/sched/membarrier.c +0 -16

index c38014c2ed66..44fafa6e1efd 100644
@@ -277,21 +277,6 @@ static void ipi_sync_rq_state(void *info)
 	smp_mb();
 }
 
-void membarrier_exec_mmap(struct mm_struct *mm)
-{
-	/*
-	 * Issue a memory barrier before clearing membarrier_state to
-	 * guarantee that no memory access prior to exec is reordered after
-	 * clearing this state.
-	 */
-	smp_mb();
-	/*
-	 * Keep the runqueue membarrier_state in sync with this mm
-	 * membarrier_state.
-	 */
-	this_cpu_write(runqueues.membarrier_state, 0);
-}
-
 void membarrier_update_current_mm(struct mm_struct *next_mm)
 {
 	struct rq *rq = this_rq();
--

2.33.1

Quoted 3 lines · ----- On Jan 8, 2022, at 11:43 AM, Andy Lutomirski [email protected] wrote:

membarrier() requires a barrier before changes to rq->curr->mm, not just before writes to rq->membarrier_state. Move the barrier in exec_mmap() to the right place.

I don't see anything that was technically wrong with membarrier_exec_mmap before this patchset. membarrier_exec-mmap issued a smp_mb just after the task_lock(), and proceeded to clear the mm->membarrier_state and runqueue membarrier state. And then the tsk->mm is set *after* the smp_mb().

So from this commit message we could be led to think there was something wrong before, but I do not think it's true. This first part of the proposed change is merely a performance optimization that removes a useless memory barrier on architectures where smp_mb__after_spinlock() is a no-op, and removes a useless store to mm->membarrier_state because it is already zero-initialized. This is all very nice, but does not belong in a "Fix" patch.

Quoted 2 lines

Add the barrier in kthread_use_mm() -- it was entirely missing before.

This is correct. This second part of the patch is indeed a relevant fix.

Thanks,

Mathieu

Quoted 94 lines

This patch makes exec_mmap() and kthread_use_mm() use the same membarrier hooks, which results in some code deletion.

As an added bonus, this will eliminate a redundant barrier in execve() on arches for which spinlock acquisition is a barrier.

Signed-off-by Andy Lutomirski <[email protected]>

---

fs/exec.c                 |  6 +++++-
include/linux/sched/mm.h  |  2 --
kernel/kthread.c          |  5 +++++
kernel/sched/membarrier.c | 15 ---------------
4 files changed, 10 insertions(+), 18 deletions(-)

fs/exec.c +5 -1

index 38b05e01c5bd..325dab98bc51 100644
@@ -1001,12 +1001,16 @@ static int exec_mmap(struct mm_struct *mm)
 	}
 
 	task_lock(tsk);
-	membarrier_exec_mmap(mm);
+	/*
+	 * membarrier() requires a full barrier before switching mm.
+	 */
+	smp_mb__after_spinlock();
 
 	local_irq_disable();
 	active_mm = tsk->active_mm;
 	tsk->active_mm = mm;
 	WRITE_ONCE(tsk->mm, mm);  /* membarrier reads this without locks */
+	membarrier_update_current_mm(mm);
 	/*
 	 * This prevents preemption while active_mm is being loaded and
 	 * it and mm are being updated, which could cause problems for

include/linux/sched/mm.h +0 -2

index e107f292fc42..f1d2beac464c 100644
@@ -344,8 +344,6 @@ enum {
 #include <asm/membarrier.h>
 #endif
 
-extern void membarrier_exec_mmap(struct mm_struct *mm);
-
 extern void membarrier_update_current_mm(struct mm_struct *next_mm);
 
 /*

kernel/kthread.c +5 -0

index 3b18329f885c..18b0a2e0e3b2 100644
@@ -1351,6 +1351,11 @@ void kthread_use_mm(struct mm_struct *mm)
 	WARN_ON_ONCE(tsk->mm);
 
 	task_lock(tsk);
+	/*
+	 * membarrier() requires a full barrier before switching mm.
+	 */
+	smp_mb__after_spinlock();
+
 	/* Hold off tlb flush IPIs while switching mm's */
 	local_irq_disable();
 	active_mm = tsk->active_mm;

kernel/sched/membarrier.c +0 -16

index c38014c2ed66..44fafa6e1efd 100644
@@ -277,21 +277,6 @@ static void ipi_sync_rq_state(void *info)
 	smp_mb();
 }
 
-void membarrier_exec_mmap(struct mm_struct *mm)
-{
-	/*
-	 * Issue a memory barrier before clearing membarrier_state to
-	 * guarantee that no memory access prior to exec is reordered after
-	 * clearing this state.
-	 */
-	smp_mb();
-	/*
-	 * Keep the runqueue membarrier_state in sync with this mm
-	 * membarrier_state.
-	 */
-	this_cpu_write(runqueues.membarrier_state, 0);
-}
-
 void membarrier_update_current_mm(struct mm_struct *next_mm)
 {
 	struct rq *rq = this_rq();
--

2.33.1

Signature · 3 lines

Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Starting with commit 63acd42c0d49 ("sched/scs: Reset the shadow stack when idle_task_exit"), the idle thread's shadow stack was reset from the idle task's context during CPU hot-unplug. This was fragile: between resetting the shadow stack and actually stopping the idle task, the shadow stack did not match the actual call stack.

Clean this up by resetting the idle task's SCS in bringup_cpu().

init_idle() still does scs_task_reset() -- see the comments there. I leave this to an SCS maintainer to untangle further.

Cc Woody Lin <[email protected]>

Cc Valentin Schneider <[email protected]>

Cc Sami Tolvanen <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---
 kernel/cpu.c        | 3 +++
 kernel/sched/core.c | 9 ++++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

kernel/cpu.c +3 -0

index 192e43a87407..be16816bb87c 100644
@@ -33,6 +33,7 @@
 #include <linux/slab.h>
 #include <linux/percpu-rwsem.h>
 #include <linux/cpuset.h>
+#include <linux/scs.h>
 
 #include <trace/events/power.h>
 #define CREATE_TRACE_POINTS

@@ -587,6 +588,8 @@ static int bringup_cpu(unsigned int cpu)
        struct task_struct *idle = idle_thread_get(cpu);
        int ret;
 
+       scs_task_reset(idle);
+
        /*
         * Some architectures have to walk the irq descriptors to
         * setup the vector space for the cpu which comes online.

kernel/sched/core.c +8 -0

index 917068b0a145..acd52a7d1349 100644
@@ -8621,7 +8621,15 @@ void __init init_idle(struct task_struct *idle, int cpu)
        idle->flags |= PF_IDLE | PF_KTHREAD | PF_NO_SETAFFINITY;
        kthread_set_per_cpu(idle, cpu);
 
+       /*
+        * NB: This is called from sched_init() on the *current* idle thread.
+        * This seems fragile if not actively incorrect.
+        *
+        * Initializing SCS for about-to-be-brought-up CPU idle threads
+        * is in bringup_cpu(), but that does not cover the boot CPU.
+        */
        scs_task_reset(idle);
+
        kasan_unpoison_task_stack(idle);
 
 #ifdef CONFIG_SMP

To improve scalability, this patch adds a percpu hazard pointer scheme to keep lazily-used mms alive. Each CPU has a single pointer to an mm that must not be freed, and __mmput() checks the pointers belonging to all CPUs that might be lazily using the mm in question.

Quoted 5 lines · Andy Lutomirski

To improve scalability, this patch adds a percpu hazard pointer scheme to keep lazily-used mms alive. Each CPU has a single pointer to an mm that must not be freed, and __mmput() checks the pointers belonging to all CPUs that might be lazily using the mm in question.

Ugh. This feels horribly fragile to me, and also looks like it makes some common cases potentially quite expensive for machines with large CPU counts if they don't do that mm_cpumask optimization - which in turn feels quite fragile as well.

IOW, this just feels *complicated*.

And I think it's overly so. I get the strong feeling that we could make the rules much simpler and more straightforward.

For example, how about we make the rules be

- a lazy TLB mm reference requires that there's an actual active user
of that mm (ie "mm_users > 0")

- the last mm_users decrement (ie __mmput) forces a TLB flush, and
that TLB flush must make sure that no lazy users exist (which I think it does already anyway).

Doesn't that seem like a really simple set of rules?

And the nice thing about it is that we *already* do that required TLB flush in all normal circumstances. __mmput() already calls exit_mmap(), and exit_mm() already forces that TLB flush in every normal situation.

So we might have to make sure that every architecture really does that "drop lazy mms on TLB flush", and maybe add a flag to the existing 'struct mmu_gather tlb' to make sure that flush actually always happens (even if the process somehow managed to unmap all vma's even before exiting).

Is there something silly I'm missing? Somebody pat me on the head, and say "There, there, Linus, don't try to get involved with things you don't understand.." and explain to me in small words.

Linus

So this requires that all architectures actually walk all relevant CPUs to see if an IPI is needed and send that IPI. On architectures that actually need an IPI anyway (x86 bare metal, powerpc (I think) and others, fine. But on architectures with a broadcast-to-all-CPUs flush (ARM64 IIUC), then the extra IPI will be much much slower than a simple load-acquire in a loop.

So this requires that all architectures actually walk all relevant CPUs to see if an IPI is needed and send that IPI. On architectures that actually need an IPI anyway (x86 bare metal, powerpc (I think) and others, fine. But on architectures with a broadcast-to-all-CPUs flush (ARM64 IIUC), then the extra IPI will be much much slower than a simple load-acquire in a loop.

[ Let's try this again, without the html crud this time. Apologies to the people who see this reply twice ]

Quoted 7 lines · Andy Lutomirski

So this requires that all architectures actually walk all relevant CPUs to see if an IPI is needed and send that IPI. On architectures that actually need an IPI anyway (x86 bare metal, powerpc (I think) and others, fine. But on architectures with a broadcast-to-all-CPUs flush (ARM64 IIUC), then the extra IPI will be much much slower than a simple load-acquire in a loop.

... hmm. How about a hybrid scheme?

(a) architectures that already require that IPI anyway for TLB
invalidation (ie x86, but others too), just make the rule be that the TLB flush by exit_mmap() get rid of any lazy TLB mm references. Which they already do.

(b) architectures like arm64 that do hw-assisted TLB shootdown will
have an ASID allocator model, and what you do is to use that to either
(b') increment/decrement the mm_count at mm ASID allocation/freeing time (b'') use the existing ASID tracking data to find the CPU's that
have that ASID

(c) can you really imagine hardware TLB shootdown without ASID
allocation? That doesn't seem to make sense. But if it exists, maybe that kind of crazy case would do the percpu array walking.

(Honesty in advertising: I don't know the arm64 ASID code - I used to know the old alpha version I wrote in a previous lifetime - but afaik any ASID allocator has to be able to track CPU's that have a particular ASID in use and be able to invalidate it).

Hmm. The x86 maintainers are on this thread, but they aren't even the problem. Adding Catalin and Will to this, I think they should know if/how this would fit with the arm64 ASID allocator.

Will/Catalin, background here:

https://lore.kernel.org/all/CAHk-=wj4LZaFB5HjZmzf7xLFSCcQri-WWqOEJHwQg0QmPRSdQA@mail.gmail.com/

for my objection to that special "keep non-refcounted magic per-cpu pointer to lazy tlb mm".

Linus

Quoted 5 lines

Hmm. The x86 maintainers are on this thread, but they aren't even the problem. Adding Catalin and Will to this, I think they should know if/how this would fit with the arm64 ASID allocator.

Well, I am an x86 mm maintainer, and there is definitely a performance problem on large x86 systems right now. :)

Quoted 7 lines · Andy Lutomirski

Quoted 5 lines

Hmm. The x86 maintainers are on this thread, but they aren't even the problem. Adding Catalin and Will to this, I think they should know if/how this would fit with the arm64 ASID allocator.

Well, I am an x86 mm maintainer, and there is definitely a performance problem on large x86 systems right now. :)

Well, my point was that on x86, the complexities of the patch you posted are completely pointless.

So on x86, you can just remove the mmgrab/mmdrop reference counts from the lazy mm use entirely, and voila, that performance problem is gone. We don't _need_ reference counting on x86 at all, if we just say that the rule is that a lazy mm is always associated with a honest-to-goodness live mm.

So on x86 - and any platform with the IPI model - there is no need for hundreds of lines of complexity at all.

THAT is my point. Your patch adds complexity that buys you ABSOLUTELY NOTHING.

You then saying that the mmgrab/mmdrop is a performance problem is just trying to muddy the water. You can just remove it entirely.

Now, I do agree that that depends on the whole "TLB IPI will get rid of any lazy mm users on other cpus". So I agree that if you have hardware TLB invalidation that then doesn't have that software component to it, you need something else.

But my argument _then_ was that hardware TLB invalidation then needs the hardware ASID thing to be useful, and the ASID management code already effectively keeps track of "this ASID is used on other CPU's". And that's exactly the same kind of information that your patch basically added a separate percpu array for.

So I think that even for that hardware TLB shootdown case, your patch only adds overhead.

And it potentially adds a *LOT* of overhead, if you replace an atomic refcount with a "for_each_possible_cpu()" loop that has to do cmpxchg things too.

Btw, you don't even need to really solve the arm64 TLB invalidate thing - we could make the rule be that we only do the mmgrab/mmput at all on platforms that don't do that IPI flush.

I think that's basically exactly what Nick Piggin wanted to do on powerpc, no?

But you hated that patch, for non-obvious reasons, and are now introducing this new patch that is clearly non-optimal on x86.

So I think there's some intellectual dishonesty on your part here.

Are you *sure*? The ASID management code on x86 is (as mentioned before) completely unaware of whether an ASID is actually in use anywhere.

Quoted 2 lines

So I think that even for that hardware TLB shootdown case, your patch only adds overhead.

The overhead is literally:

exit_mmap();
for each cpu still in mm_cpumask:
smp_load_acquire

That's it, unless the mm is actually in use

On a very large arm64 system, I would believe there could be real overhead. But these very large systems are exactly the systems that currently ping-pong mm_count.

I'd much rather have a *much* smaller patch that says "on x86 and powerpc, we don't need this overhead at all".

[ Ugh, I actually went back and looked at Nick's patches again, to just verify my memory, and they weren't as pretty as I thought they were ]

Quoted 3 lines · Linus Torvalds

I'd much rather have a *much* smaller patch that says "on x86 and powerpc, we don't need this overhead at all".

Quoted 5 lines

exit_mmap();
for each cpu still in mm_cpumask:
smp_load_acquire

That's it, unless the mm is actually in use

Ok, now do this for a machine with 1024 CPU's.

And tell me it is "scalable".

Quoted 10 lines · Linus Torvalds

Quoted 5 lines

exit_mmap();
for each cpu still in mm_cpumask:
  smp_load_acquire

That's it, unless the mm is actually in use

Ok, now do this for a machine with 1024 CPU's.

And tell me it is "scalable".

Do you mean a machine with 1024 CPUs and 2 bits set in mm_cpumask or 1024 CPU with 800 bits set in mm_cpumask?  In the former case, this is fine.  In the latter case, *on x86*, sure it does 800 loads, but we're about to do 800 CR3 writes to tear the whole mess down, so the 800 loads should be in the noise.  (And this series won't actually do this anyway on bare metal, since exit_mmap does the shootdown.)

Also, while 800 loads is kinda expensive, it is a heck of a lot less expensive than 800 IPIs.

[ Ugh, I actually went back and looked at Nick's patches again, to just verify my memory, and they weren't as pretty as I thought they were ]

Quoted 3 lines · Linus Torvalds

I'd much rather have a *much* smaller patch that says "on x86 and powerpc, we don't need this overhead at all".

For some reason I thought Nick's patch worked at "last mmput" time and the TLB flush IPIs that happen at that point anyway would then make sure any lazy TLB is cleaned up.

But that's not actually what it does. It ties the MMU_LAZY_TLB_REFCOUNT to an explicit TLB shootdown triggered by the last mmdrop() instead. Because it really tied the whole logic to the mm_count logic (and made lazy tlb to not do mm_count) rather than the mm_users thing I mis-remembered it doing.

So at least some of my arguments were based on me just mis-remembering what Nick's patch actually did (mainly because I mentally recreated the patch from "Nick did something like this" and what I thought would be the way to do it on x86).

So I guess I have to recant my arguments.

I still think my "get rid of lazy at last mmput" model should work, and would be a perfect match for x86, but I can't really point to Nick having done that.

So I was full of BS.

Hmm. I'd love to try to actually create a patch that does that "Nick thing", but on last mmput() (ie when __mmput triggers). Because I think this is interesting. But then I look at my schedule for the upcoming week, and I go "I don't have a leg to stand on in this discussion, and I'm just all hot air".

Excerpts from Linus Torvalds's message of January 10, 2022 7:51 am:

Quoted 14 lines

[ Ugh, I actually went back and looked at Nick's patches again, to just verify my memory, and they weren't as pretty as I thought they were ]

Quoted 3 lines · Linus Torvalds

I'd much rather have a *much* smaller patch that says "on x86 and powerpc, we don't need this overhead at all".

For some reason I thought Nick's patch worked at "last mmput" time and the TLB flush IPIs that happen at that point anyway would then make sure any lazy TLB is cleaned up.

Quoted 5 lines

But that's not actually what it does. It ties the MMU_LAZY_TLB_REFCOUNT to an explicit TLB shootdown triggered by the last mmdrop() instead. Because it really tied the whole logic to the mm_count logic (and made lazy tlb to not do mm_count) rather than the mm_users thing I mis-remembered it doing.

It does this because on powerpc with hash MMU, we can't use IPIs for TLB shootdowns.

Quoted 4 lines

So at least some of my arguments were based on me just mis-remembering what Nick's patch actually did (mainly because I mentally recreated the patch from "Nick did something like this" and what I thought would be the way to do it on x86).

With powerpc with the radix MMU using IPI based shootdowns, we can actually do the switch-away-from-lazy on the final TLB flush and the final broadcast shootdown thing becomes a no-op. I didn't post that additional patch because it's powerpc-specific and I didn't want to post more code so widely.