membarrier cleanups

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 | 55 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

kernel/sched/membarrier.c +0 -0

index b5add64d9698..3173b063d358 100644

Either it sends an IPI or it does not. If membarrier() 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 membarrier() scheduled, and scheduling had better include a
+ * full barrier somewhere for basic correctness regardless of membarrier.)
+ *
+ * If membarrier() does not send an IPI, this means that membarrier() reads
+ * cpu_rq(cpu)->curr->mm and that the result is not equal to the target
+ * mm.

Let's assume for now that tasks never change their mm field. The
+ * sequence of events is:
+ *
+ * 1. Target CPU switches away from the target mm (or goes lazy or has never
+ * run the target mm in the first place). This involves smp_mb() followed
+ * by a write to cpu_rq(cpu)->curr.
+ * 2. membarrier() does smp_mb(). (This is NOT synchronized with any action
+ * done by the target.)
+ * 3. membarrier() observes the value written in step 1 and does *not* observe
+ * the value written in step 5.
+ * 4. membarrier() does smp_mb().
+ * 5. Target CPU switches back to the target mm and writes to
+ * cpu_rq(cpu)->curr. (This is NOT synchronized with any action on
+ * membarrier()'s part.)
+ * 6. Target CPU executes smp_mb()
+ *
+ * All pre-schedule accesses on the remote CPU are visible after membarrier()
+ * because they all precede the target's write in step 1 and are synchronized
+ * to the local CPU by steps 3 and 4. All pre-membarrier() accesses on the

+ * local CPU are visible on the remote CPU after scheduling because they
+ * happen before the smp_mb(); read in steps 2 and 3 and that read preceeds
+ * the target's smp_mb() in step 6.
+ *
+ * However, 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.

+ *
+ *
* For documentation purposes, here are some membarrier ordering
* scenarios to keep in mind:

Excerpts from Andy Lutomirski's message of June 16, 2021 1:21 pm:

Quoted 17 lines

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 | 55 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

kernel/sched/membarrier.c line 8

Quoted 7 lines

kernel/sched/membarrier.c +0 -0

index b5add64d9698..3173b063d358 100644
@@ -7,6 +7,61 @@
 #include "sched.h"
 

Precisely describing the orderings is great, not a fan of the style of the comment though.

Quoted 18 lines · Nicholas Piggin

Excerpts from Andy Lutomirski's message of June 16, 2021 1:21 pm:

Quoted 17 lines

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 | 55 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

Quoted 10 lines

kernel/sched/membarrier.c line 8

Quoted 7 lines

kernel/sched/membarrier.c +0 -0

index b5add64d9698..3173b063d358 100644
@@ -7,6 +7,61 @@
 #include "sched.h"
 

Precisely describing the orderings is great, not a fan of the style of the comment though.

I'm with Nick on that; I can't read it :/ It only makes things more confusing. If you want precision, English (or any natural language) is your enemy.

To describe ordering use the diagrams and/or litmus tests.

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        | 53 +++++++++++++++++++++++++++++++---------
 include/linux/sched/mm.h | 13 ----------
 kernel/sched/core.c      | 13 ++++------
 3 files changed, 46 insertions(+), 33 deletions(-)

arch/x86/mm/tlb.c +41 -12

index 78804680e923..59488d663e68 100644
@@ -8,6 +8,7 @@
 #include <linux/export.h>
 #include <linux/cpu.h>
 #include <linux/debugfs.h>
+#include <linux/sched/mm.h>
 
 #include <asm/tlbflush.h>
 #include <asm/mmu_context.h>

@@ -473,16 +474,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) !=

@@ -500,7 +509,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;

@@ -510,16 +520,35 @@ 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) {
+#ifdef CONFIG_MEMBARRIER
+			/*
+			 * 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.
+			 */
+			if (unlikely(atomic_read(&next->membarrier_state) &
+				     MEMBARRIER_STATE_PRIVATE_EXPEDITED_SYNC_CORE))
+				sync_core_before_usermode();
+#endif
+
 			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 -13

index e24b1fe348e3..24d97d1b6252 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);

@@ -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 +5 -8

index 5226cc26a095..e4c122f8bf21 100644
@@ -4220,22 +4220,19 @@ 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:
-	 *
-	 * - a full memory barrier for {PRIVATE,GLOBAL}_EXPEDITED, implicitly
-	 *   provided by mmdrop(),
-	 * - a sync_core for SYNC_CORE.
+	 * rq->curr, before returning to userspace, and mmdrop() provides
+	 * this barrier.
 	 */
-	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);

Signature · 1 line

2.31.1

Excerpts from Andy Lutomirski's message of June 16, 2021 1:21 pm:

I'm fine with the patch though, except I would leave the comment in the core sched code saying any arch specific sequence to deal with SYNC_CORE is required for that case.

(quoted diff) +0 -0

@@ -473,16 +474,24 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct
 mm_struct *next,

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

@@ -510,16 +520,35 @@ 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) {
+#ifdef CONFIG_MEMBARRIER
+			/*
+			 * We switched logical mm but we're not going to
+			 * write to CR3.  We already did smp_mb() above,

----- On Jun 15, 2021, at 11:21 PM, Andy Lutomirski [email protected] wrote:
[...]

Quoted 2 lines

(quoted diff) +0 -0

@@ -473,16 +474,24 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct
 mm_struct *next,

[...]

Quoted 29 lines

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

@@ -510,16 +520,35 @@ 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) {
+#ifdef CONFIG_MEMBARRIER
+			/*
+			 * 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.
+			 */
+			if (unlikely(atomic_read(&next->membarrier_state) &
+				     MEMBARRIER_STATE_PRIVATE_EXPEDITED_SYNC_CORE))
+				sync_core_before_usermode();
+#endif
+
 			return;
+		}

[...]

I find that mixing up preprocessor #ifdef and code logic hurts readability. Can you lift this into a static function within the same compile unit, and provides an empty implementation for the #else case ?

Thanks,

Mathieu

	prev->sched_class->task_dead(prev);

membarrier_arch_switch_mm()'s sole implementation and caller are in arch/powerpc. Having a fallback implementation in include/linux is confusing -- remove it.

It's still mentioned in a comment, but a subsequent patch will remove it.

Cc Mathieu Desnoyers <[email protected]>

Cc Nicholas Piggin <[email protected]>

Cc Peter Zijlstra <[email protected]>

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

---
 include/linux/sched/mm.h | 7 -------
 1 file changed, 7 deletions(-)

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

index 24d97d1b6252..10aace21d25e 100644
@@ -350,13 +350,6 @@ extern void membarrier_exec_mmap(struct mm_struct *mm);
 extern void membarrier_update_current_mm(struct mm_struct *next_mm);
 
 #else
-#ifdef CONFIG_ARCH_HAS_MEMBARRIER_CALLBACKS
-static inline void membarrier_arch_switch_mm(struct mm_struct *prev,
-					     struct mm_struct *next,
-					     struct task_struct *tsk)
-{
-}
-#endif
 static inline void membarrier_exec_mmap(struct mm_struct *mm)
 {
 }

Signature · 1 line

2.31.1

membarrier_arch_switch_mm()'s sole implementation and caller are in arch/powerpc. Having a fallback implementation in include/linux is confusing -- remove it.

It's still mentioned in a comment, but a subsequent patch will remove it.

Cc Mathieu Desnoyers <[email protected]>

Cc Nicholas Piggin <[email protected]>

Cc Peter Zijlstra <[email protected]>

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

---
include/linux/sched/mm.h | 7 -------

1 file changed, 7 deletions(-)

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

index 24d97d1b6252..10aace21d25e 100644
@@ -350,13 +350,6 @@ extern void membarrier_exec_mmap(struct mm_struct *mm);
 extern void membarrier_update_current_mm(struct mm_struct *next_mm);
 
 #else
-#ifdef CONFIG_ARCH_HAS_MEMBARRIER_CALLBACKS
-static inline void membarrier_arch_switch_mm(struct mm_struct *prev,
-					     struct mm_struct *next,
-					     struct task_struct *tsk)
-{
-}
-#endif
 static inline void membarrier_exec_mmap(struct mm_struct *mm)
 {
 }
--

2.31.1

membarrier() needs a barrier after any CPU changes mm. There is currently a comment explaining why this barrier probably exists in all cases. This is very fragile -- any change to the relevant parts of the scheduler might get rid of these barriers, and it's not really clear to me that the barrier actually exists in all necessary cases.

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.

Cc Mathieu Desnoyers <[email protected]>

Cc Nicholas Piggin <[email protected]>

Cc Peter Zijlstra <[email protected]>

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

---
 include/linux/sched/mm.h | 21 +++++++++++++++++++++
 kernel/kthread.c         | 12 +-----------
 kernel/sched/core.c      | 35 +++++++++--------------------------
 3 files changed, 31 insertions(+), 37 deletions(-)

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

index 10aace21d25e..c6eebbafadb0 100644
@@ -341,6 +341,27 @@ enum {
 	MEMBARRIER_FLAG_RSEQ		= (1U << 1),
 };
 
+#ifdef CONFIG_MEMBARRIER
+
+/*
+ * 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(int membarrier_state)
+{
+	if (membarrier_state & (MEMBARRIER_STATE_GLOBAL_EXPEDITED | MEMBARRIER_STATE_PRIVATE_EXPEDITED))
+		smp_mb();
+}
+#endif
+
+#else
+
+static inline void membarrier_finish_switch_mm(int membarrier_state) {}
+
+#endif
+
 #ifdef CONFIG_ARCH_HAS_MEMBARRIER_CALLBACKS
 #include <asm/membarrier.h>
 #endif

kernel/kthread.c +1 -11

index fe3f2a40d61e..8275b415acec 100644
@@ -1325,25 +1325,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(atomic_read(&mm->membarrier_state));
 	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 +9 -26

index e4c122f8bf21..329a6d2a4e13 100644
@@ -4221,15 +4221,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 (mm)
 		mmdrop(mm);
 

@@ -4311,15 +4302,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(rq->membarrier_state);
 
 		if (!prev->mm) {                        // from kernel
 			/* will mmdrop() in finish_task_switch(). */

@@ -5121,17 +5111,10 @@ static void __sched notrace __schedule(bool preempt)
 		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;
 

Signature · 1 line

2.31.1

Excerpts from Andy Lutomirski's message of June 16, 2021 1:21 pm:

Quoted 5 lines

membarrier() needs a barrier after any CPU changes mm. There is currently a comment explaining why this barrier probably exists in all cases. This is very fragile -- any change to the relevant parts of the scheduler might get rid of these barriers, and it's not really clear to me that the barrier actually exists in all necessary cases.

The comments and barriers in the mmdrop() hunks? I don't see what is fragile or maybe-buggy about this. The barrier definitely exists.

And any change can change anything, that doesn't make it fragile. My lazy tlb refcounting change avoids the mmdrop in some cases, but it replaces it with smp_mb for example.

Quoted 7 lines · Nicholas Piggin

Excerpts from Andy Lutomirski's message of June 16, 2021 1:21 pm:

Quoted 5 lines

membarrier() needs a barrier after any CPU changes mm. There is currently a comment explaining why this barrier probably exists in all cases. This is very fragile -- any change to the relevant parts of the scheduler might get rid of these barriers, and it's not really clear to me that the barrier actually exists in all necessary cases.

Quoted 6 lines

The comments and barriers in the mmdrop() hunks? I don't see what is fragile or maybe-buggy about this. The barrier definitely exists.

And any change can change anything, that doesn't make it fragile. My lazy tlb refcounting change avoids the mmdrop in some cases, but it replaces it with smp_mb for example.

I'm with Nick again, on this. You're adding extra barriers for no discernible reason, that's not generally encouraged, seeing how extra barriers is extra slow.

Both mmdrop() itself, as well as the callsite have comments saying how membarrier relies on the implied barrier, what's fragile about that?

Quoted 15 lines · Nicholas Piggin

Quoted 7 lines

Excerpts from Andy Lutomirski's message of June 16, 2021 1:21 pm:

Quoted 5 lines

membarrier() needs a barrier after any CPU changes mm. There is currently a comment explaining why this barrier probably exists in all cases. This is very fragile -- any change to the relevant parts of the scheduler might get rid of these barriers, and it's not really clear to me that the barrier actually exists in all necessary cases.

Quoted 6 lines

The comments and barriers in the mmdrop() hunks? I don't see what is fragile or maybe-buggy about this. The barrier definitely exists.

And any change can change anything, that doesn't make it fragile. My lazy tlb refcounting change avoids the mmdrop in some cases, but it replaces it with smp_mb for example.

Quoted 7 lines

I'm with Nick again, on this. You're adding extra barriers for no discernible reason, that's not generally encouraged, seeing how extra barriers is extra slow.

Both mmdrop() itself, as well as the callsite have comments saying how membarrier relies on the implied barrier, what's fragile about that?

My real motivation is that mmgrab() and mmdrop() don't actually need to be full barriers. The current implementation has them being full barriers, and the current implementation is quite slow. So let's try that commit message again:

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.

Excerpts from Andy Lutomirski's message of June 17, 2021 4:41 am:

Quoted 35 lines · Peter Zijlstra

Quoted 15 lines · Nicholas Piggin

Quoted 7 lines

Excerpts from Andy Lutomirski's message of June 16, 2021 1:21 pm:

Quoted 5 lines

membarrier() needs a barrier after any CPU changes mm. There is currently a comment explaining why this barrier probably exists in all cases. This is very fragile -- any change to the relevant parts of the scheduler might get rid of these barriers, and it's not really clear to me that the barrier actually exists in all necessary cases.

Quoted 6 lines

The comments and barriers in the mmdrop() hunks? I don't see what is fragile or maybe-buggy about this. The barrier definitely exists.

And any change can change anything, that doesn't make it fragile. My lazy tlb refcounting change avoids the mmdrop in some cases, but it replaces it with smp_mb for example.

Quoted 7 lines

I'm with Nick again, on this. You're adding extra barriers for no discernible reason, that's not generally encouraged, seeing how extra barriers is extra slow.

Both mmdrop() itself, as well as the callsite have comments saying how membarrier relies on the implied barrier, what's fragile about that?

My real motivation is that mmgrab() and mmdrop() don't actually need to be full barriers. The current implementation has them being full barriers, and the current implementation is quite slow. So let's try that commit message again:

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.

Quoted 3 lines

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.

I'm not against the idea, I've looked at something similar before (not for mmdrop but a different primitive). Also my lazy tlb shootdown series could possibly take advantage of this, I might cherry pick it and test performance :)

I don't think it belongs in this series though. Should go together with something that takes advantage of it.

Thanks,
Nick

Quoted 45 lines · Nicholas Piggin

Excerpts from Andy Lutomirski's message of June 17, 2021 4:41 am:

Quoted 35 lines · Peter Zijlstra

Quoted 15 lines · Nicholas Piggin

Quoted 7 lines

Excerpts from Andy Lutomirski's message of June 16, 2021 1:21 pm:

Quoted 5 lines

membarrier() needs a barrier after any CPU changes mm. There is currently a comment explaining why this barrier probably exists in all cases. This is very fragile -- any change to the relevant parts of the scheduler might get rid of these barriers, and it's not really clear to me that the barrier actually exists in all necessary cases.

Quoted 6 lines

The comments and barriers in the mmdrop() hunks? I don't see what is fragile or maybe-buggy about this. The barrier definitely exists.

And any change can change anything, that doesn't make it fragile. My lazy tlb refcounting change avoids the mmdrop in some cases, but it replaces it with smp_mb for example.

Quoted 7 lines

I'm with Nick again, on this. You're adding extra barriers for no discernible reason, that's not generally encouraged, seeing how extra barriers is extra slow.

Both mmdrop() itself, as well as the callsite have comments saying how membarrier relies on the implied barrier, what's fragile about that?

My real motivation is that mmgrab() and mmdrop() don't actually need to be full barriers. The current implementation has them being full barriers, and the current implementation is quite slow. So let's try that commit message again:

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.

Quoted 3 lines

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.

I'm not against the idea, I've looked at something similar before (not for mmdrop but a different primitive). Also my lazy tlb shootdown series could possibly take advantage of this, I might cherry pick it and test performance :)

Quoted 2 lines

I don't think it belongs in this series though. Should go together with something that takes advantage of it.

I’m going to see if I can get hazard pointers into shape quickly.

Quoted 48 lines · Nicholas Piggin

Quoted 45 lines

Excerpts from Andy Lutomirski's message of June 17, 2021 4:41 am:

Quoted 35 lines · Peter Zijlstra

Quoted 15 lines · Nicholas Piggin

Quoted 7 lines

Excerpts from Andy Lutomirski's message of June 16, 2021 1:21 pm:

Quoted 5 lines

membarrier() needs a barrier after any CPU changes mm. There is currently a comment explaining why this barrier probably exists in all cases. This is very fragile -- any change to the relevant parts of the scheduler might get rid of these barriers, and it's not really clear to me that the barrier actually exists in all necessary cases.

Quoted 6 lines

The comments and barriers in the mmdrop() hunks? I don't see what is fragile or maybe-buggy about this. The barrier definitely exists.

And any change can change anything, that doesn't make it fragile. My lazy tlb refcounting change avoids the mmdrop in some cases, but it replaces it with smp_mb for example.

Quoted 7 lines

I'm with Nick again, on this. You're adding extra barriers for no discernible reason, that's not generally encouraged, seeing how extra barriers is extra slow.

Both mmdrop() itself, as well as the callsite have comments saying how membarrier relies on the implied barrier, what's fragile about that?

My real motivation is that mmgrab() and mmdrop() don't actually need to be full barriers. The current implementation has them being full barriers, and the current implementation is quite slow. So let's try that commit message again:

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.

Quoted 3 lines

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.

I'm not against the idea, I've looked at something similar before (not for mmdrop but a different primitive). Also my lazy tlb shootdown series could possibly take advantage of this, I might cherry pick it and test performance :)

Quoted 4 lines

Quoted 2 lines

I don't think it belongs in this series though. Should go together with something that takes advantage of it.

I’m going to see if I can get hazard pointers into shape quickly.

Here it is. Not even boot tested!

https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=sched/lazymm&id=ecc3992c36cb88087df9c537e2326efb51c95e31

Nick, I think you can accomplish much the same thing as your patch by:

#define for_each_possible_lazymm_cpu while (false)

although a more clever definition might be even more performant.

I would appreciate everyone's thoughts as to whether this scheme is sane.

Paul, I'm adding you for two reasons. First, you seem to enjoy bizarre locking schemes. Secondly, because maybe RCU could actually work here. The basic idea is that we want to keep an mm_struct from being freed at an inopportune time. The problem with naively using RCU is that each CPU can use one single mm_struct while in an idle extended quiescent state (but not a user extended quiescent state). So rcu_read_lock() is right out. If RCU could understand this concept, then maybe it could help us, but this seems a bit out of scope for RCU.

--Andy

Excerpts from Andy Lutomirski's message of June 17, 2021 3:32 pm:

Quoted 56 lines · Andy Lutomirski

Quoted 48 lines · Nicholas Piggin

Quoted 45 lines

Excerpts from Andy Lutomirski's message of June 17, 2021 4:41 am:

Quoted 35 lines · Peter Zijlstra

Quoted 15 lines · Nicholas Piggin

Quoted 7 lines

Excerpts from Andy Lutomirski's message of June 16, 2021 1:21 pm:

Quoted 5 lines

membarrier() needs a barrier after any CPU changes mm. There is currently a comment explaining why this barrier probably exists in all cases. This is very fragile -- any change to the relevant parts of the scheduler might get rid of these barriers, and it's not really clear to me that the barrier actually exists in all necessary cases.

Quoted 6 lines

The comments and barriers in the mmdrop() hunks? I don't see what is fragile or maybe-buggy about this. The barrier definitely exists.

And any change can change anything, that doesn't make it fragile. My lazy tlb refcounting change avoids the mmdrop in some cases, but it replaces it with smp_mb for example.

Quoted 7 lines

I'm with Nick again, on this. You're adding extra barriers for no discernible reason, that's not generally encouraged, seeing how extra barriers is extra slow.

Both mmdrop() itself, as well as the callsite have comments saying how membarrier relies on the implied barrier, what's fragile about that?

My real motivation is that mmgrab() and mmdrop() don't actually need to be full barriers. The current implementation has them being full barriers, and the current implementation is quite slow. So let's try that commit message again:

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.

Quoted 3 lines

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.

I'm not against the idea, I've looked at something similar before (not for mmdrop but a different primitive). Also my lazy tlb shootdown series could possibly take advantage of this, I might cherry pick it and test performance :)

Quoted 4 lines

Quoted 2 lines

I don't think it belongs in this series though. Should go together with something that takes advantage of it.

I’m going to see if I can get hazard pointers into shape quickly.

Here it is. Not even boot tested!

Quoted 5 lines

https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=sched/lazymm&id=ecc3992c36cb88087df9c537e2326efb51c95e31

Nick, I think you can accomplish much the same thing as your patch by:

#define for_each_possible_lazymm_cpu while (false)

I'm not sure what you mean? For powerpc, other CPUs can be using the mm as lazy at this point. I must be missing something.

Quoted 57 lines

Excerpts from Andy Lutomirski's message of June 17, 2021 3:32 pm:

Quoted 56 lines · Andy Lutomirski

Quoted 48 lines · Nicholas Piggin

Quoted 45 lines

Excerpts from Andy Lutomirski's message of June 17, 2021 4:41 am:

Quoted 35 lines · Peter Zijlstra

Quoted 15 lines · Nicholas Piggin

Quoted 7 lines

Excerpts from Andy Lutomirski's message of June 16, 2021 1:21 pm:

Quoted 5 lines

membarrier() needs a barrier after any CPU changes mm. There is currently a comment explaining why this barrier probably exists in all cases. This is very fragile -- any change to the relevant parts of the scheduler might get rid of these barriers, and it's not really clear to me that the barrier actually exists in all necessary cases.

Quoted 6 lines

The comments and barriers in the mmdrop() hunks? I don't see what is fragile or maybe-buggy about this. The barrier definitely exists.

And any change can change anything, that doesn't make it fragile. My lazy tlb refcounting change avoids the mmdrop in some cases, but it replaces it with smp_mb for example.

Quoted 7 lines

I'm with Nick again, on this. You're adding extra barriers for no discernible reason, that's not generally encouraged, seeing how extra barriers is extra slow.

Both mmdrop() itself, as well as the callsite have comments saying how membarrier relies on the implied barrier, what's fragile about that?

My real motivation is that mmgrab() and mmdrop() don't actually need to be full barriers. The current implementation has them being full barriers, and the current implementation is quite slow. So let's try that commit message again:

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.

Quoted 3 lines

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.

I'm not against the idea, I've looked at something similar before (not for mmdrop but a different primitive). Also my lazy tlb shootdown series could possibly take advantage of this, I might cherry pick it and test performance :)

Quoted 4 lines

Quoted 2 lines

I don't think it belongs in this series though. Should go together with something that takes advantage of it.

I’m going to see if I can get hazard pointers into shape quickly.

Here it is. Not even boot tested!

Quoted 8 lines

Quoted 5 lines

https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=sched/lazymm&id=ecc3992c36cb88087df9c537e2326efb51c95e31

Nick, I think you can accomplish much the same thing as your patch by:

#define for_each_possible_lazymm_cpu while (false)

I'm not sure what you mean? For powerpc, other CPUs can be using the mm as lazy at this point. I must be missing something.

What I mean is: if you want to shoot down lazies instead of doing the hazard pointer trick to track them, you could do:

#define for_each_possible_lazymm_cpu while (false)

which would promise to the core code that you don't have any lazies left by the time exit_mmap() is done. You might need a new hook in exit_mmap() depending on exactly how you implement the lazy shootdown.

Excerpts from Andy Lutomirski's message of June 18, 2021 9:49 am:

Quoted 72 lines · Nicholas Piggin

Quoted 57 lines

Excerpts from Andy Lutomirski's message of June 17, 2021 3:32 pm:

Quoted 56 lines · Andy Lutomirski

Quoted 48 lines · Nicholas Piggin

Quoted 45 lines

Excerpts from Andy Lutomirski's message of June 17, 2021 4:41 am:

Quoted 35 lines · Peter Zijlstra

Quoted 15 lines · Nicholas Piggin

Quoted 7 lines

Excerpts from Andy Lutomirski's message of June 16, 2021 1:21 pm:

Quoted 5 lines

membarrier() needs a barrier after any CPU changes mm. There is currently a comment explaining why this barrier probably exists in all cases. This is very fragile -- any change to the relevant parts of the scheduler might get rid of these barriers, and it's not really clear to me that the barrier actually exists in all necessary cases.

Quoted 6 lines

The comments and barriers in the mmdrop() hunks? I don't see what is fragile or maybe-buggy about this. The barrier definitely exists.

And any change can change anything, that doesn't make it fragile. My lazy tlb refcounting change avoids the mmdrop in some cases, but it replaces it with smp_mb for example.

Quoted 7 lines

I'm with Nick again, on this. You're adding extra barriers for no discernible reason, that's not generally encouraged, seeing how extra barriers is extra slow.

Both mmdrop() itself, as well as the callsite have comments saying how membarrier relies on the implied barrier, what's fragile about that?

My real motivation is that mmgrab() and mmdrop() don't actually need to be full barriers. The current implementation has them being full barriers, and the current implementation is quite slow. So let's try that commit message again:

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.

Quoted 3 lines

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.

I'm not against the idea, I've looked at something similar before (not for mmdrop but a different primitive). Also my lazy tlb shootdown series could possibly take advantage of this, I might cherry pick it and test performance :)

Quoted 4 lines

Quoted 2 lines

I don't think it belongs in this series though. Should go together with something that takes advantage of it.

I’m going to see if I can get hazard pointers into shape quickly.

Here it is. Not even boot tested!

Quoted 8 lines

Quoted 5 lines

https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=sched/lazymm&id=ecc3992c36cb88087df9c537e2326efb51c95e31

Nick, I think you can accomplish much the same thing as your patch by:

#define for_each_possible_lazymm_cpu while (false)

I'm not sure what you mean? For powerpc, other CPUs can be using the mm as lazy at this point. I must be missing something.

What I mean is: if you want to shoot down lazies instead of doing the hazard pointer trick to track them, you could do:

#define for_each_possible_lazymm_cpu while (false)

Quoted 3 lines

which would promise to the core code that you don't have any lazies left by the time exit_mmap() is done. You might need a new hook in exit_mmap() depending on exactly how you implement the lazy shootdown.

Oh for configuring it away entirely. I'll have to see how it falls out, I suspect we'd want to just no-op that entire function and avoid the 2 atomics if we are taking care of our lazy mms with shootdowns.

The more important thing would be the context switch fast path, but even there, there's really no reason why the two approaches couldn't be made to both work with some careful helper functions or structuring of the code.

Thanks,
Nick

Quoted 77 lines

Excerpts from Andy Lutomirski's message of June 18, 2021 9:49 am:

Quoted 75 lines · Nicholas Piggin

Quoted 57 lines

Excerpts from Andy Lutomirski's message of June 17, 2021 3:32 pm:

Quoted 56 lines · Andy Lutomirski

Quoted 48 lines · Nicholas Piggin

Quoted 45 lines

Excerpts from Andy Lutomirski's message of June 17, 2021 4:41 am:

Quoted 35 lines · Peter Zijlstra

Quoted 15 lines · Nicholas Piggin

Quoted 7 lines

Excerpts from Andy Lutomirski's message of June 16, 2021 1:21 pm:

Quoted 5 lines

membarrier() needs a barrier after any CPU changes mm. There is currently a comment explaining why this barrier probably exists in all cases. This is very fragile -- any change to the relevant parts of the scheduler might get rid of these barriers, and it's not really clear to me that the barrier actually exists in all necessary cases.

Quoted 6 lines

The comments and barriers in the mmdrop() hunks? I don't see what is fragile or maybe-buggy about this. The barrier definitely exists.

And any change can change anything, that doesn't make it fragile. My lazy tlb refcounting change avoids the mmdrop in some cases, but it replaces it with smp_mb for example.

Quoted 7 lines

I'm with Nick again, on this. You're adding extra barriers for no discernible reason, that's not generally encouraged, seeing how extra barriers is extra slow.

Both mmdrop() itself, as well as the callsite have comments saying how membarrier relies on the implied barrier, what's fragile about that?

My real motivation is that mmgrab() and mmdrop() don't actually need to be full barriers. The current implementation has them being full barriers, and the current implementation is quite slow. So let's try that commit message again:

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.

Quoted 3 lines

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.

I'm not against the idea, I've looked at something similar before (not for mmdrop but a different primitive). Also my lazy tlb shootdown series could possibly take advantage of this, I might cherry pick it and test performance :)

Quoted 4 lines

Quoted 2 lines

I don't think it belongs in this series though. Should go together with something that takes advantage of it.

I’m going to see if I can get hazard pointers into shape quickly.

Here it is. Not even boot tested!

Quoted 8 lines

Quoted 5 lines

https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=sched/lazymm&id=ecc3992c36cb88087df9c537e2326efb51c95e31

Nick, I think you can accomplish much the same thing as your patch by:

#define for_each_possible_lazymm_cpu while (false)

I'm not sure what you mean? For powerpc, other CPUs can be using the mm as lazy at this point. I must be missing something.

What I mean is: if you want to shoot down lazies instead of doing the hazard pointer trick to track them, you could do:

#define for_each_possible_lazymm_cpu while (false)

which would promise to the core code that you don't have any lazies left by the time exit_mmap() is done. You might need a new hook in exit_mmap() depending on exactly how you implement the lazy shootdown.

Quoted 3 lines

Oh for configuring it away entirely. I'll have to see how it falls out, I suspect we'd want to just no-op that entire function and avoid the 2 atomics if we are taking care of our lazy mms with shootdowns.

Do you mean the smp_store_release()? On x86 and similar architectures, that’s almost free. I’m also not convinced it needs to be a real release.

Here it is. Not even boot tested!

https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=sched/lazymm&id=ecc3992c36cb88087df9c537e2326efb51c95e31

On Wed, Jun 16, 2021 at 10:32:15PM -0700, Andy Lutomirski wrote:

arch/x86/include/asm/mmu.h +5 -0

@@ -66,4 +66,9 @@ typedef struct {
 void leave_mm(int cpu);
 #define leave_mm leave_mm
 
+/* On x86, mm_cpumask(mm) contains all CPUs that might be lazily using mm */
+#define for_each_possible_lazymm_cpu(cpu, mm) \
+	for_each_cpu((cpu), mm_cpumask((mm)))
+
+
 #endif /* _ASM_X86_MMU_H */

kernel/sched/core.c +0 -0

index 8ac693d542f6..e102ec53c2f6 100644

Quoted 21 lines · Andy Lutomirski

arch/x86/include/asm/mmu.h +5 -0

@@ -66,4 +66,9 @@ typedef struct {
 void leave_mm(int cpu);
 #define leave_mm leave_mm
 
+/* On x86, mm_cpumask(mm) contains all CPUs that might be lazily using mm */
+#define for_each_possible_lazymm_cpu(cpu, mm) \
+	for_each_cpu((cpu), mm_cpumask((mm)))
+
+
 #endif /* _ASM_X86_MMU_H */

kernel/sched/core.c +0 -0

index 8ac693d542f6..e102ec53c2f6 100644
@@ -19,6 +19,7 @@
 

kernel/sched/core.c prepare_task_switch(struct rq *rq, struct task_struct *prev, · line 4524

Quoted 5 lines

kernel/sched/core.c +5 -0

+
+#ifndef for_each_possible_lazymm_cpu
+#define for_each_possible_lazymm_cpu(cpu, mm) for_each_online_cpu((cpu))
+#endif
+

Why can't the x86 implementation be the default? IIRC the problem with mm_cpumask() is that (some) architectures don't clear bits, but IIRC they all should be setting bits, or were there archs that didn't even do that?

kernel/sched/core.c +5 -0

+static void mmdrop_lazy(struct rq *rq)
+{
+	struct mm_struct *old_mm;
+
+	if (likely(!READ_ONCE(rq->drop_mm)))

Quoted 11 lines · Peter Zijlstra

kernel/sched/core.c +11 -0

+static void mmdrop_lazy(struct rq *rq)
+{
+	struct mm_struct *old_mm;
+
+	if (likely(!READ_ONCE(rq->drop_mm)))
+		return;
+
+	/*
+	 * Slow path.  This only happens when we recently stopped using
+	 * an mm that is exiting.
+	 */

kernel/sched/core.c prepare_task_switch(struct rq *rq, struct task_struct *prev, · line 4519

Quoted 4 lines

kernel/sched/core.c +4 -0

+	old_mm = xchg(&rq->drop_mm, NULL);
+	if (old_mm)
+		mmdrop(old_mm);
+}

AFAICT if we observe a !NULL value on the load, the xchg() *MUST* also see !NULL (although it might see a different !NULL value). So do we want to write it something like so instead?

static void mmdrop_lazy(struct rq *rq)

{
	struct mm_struct *old_mm;

	if (likely(!READ_ONCE(rq->drop_mm)))
		return;

	/*
	 * Slow path.  This only happens when we recently stopped using
	 * an mm that is exiting.

	 */
	old_mm = xchg(&rq->drop_mm, NULL);
	if (WARN_ON_ONCE(!old_mm))
		return;

	mmdrop(old_mm);
}

kernel/fork.c +2 -0

index e595e77913eb..57415cca088c 100644
@@ -1104,6 +1104,8 @@ static inline void __mmput(struct mm_struct *mm)
 	}
 	if (mm->binfmt)
 		module_put(mm->binfmt->module);
+
+	mm_unlazy_mm_count(mm);
 	mmdrop(mm);
 }
 

kernel/sched/core.c +7 -0

index 8ac693d542f6..e102ec53c2f6 100644
@@ -19,6 +19,7 @@
 
+/*
+ * This converts all lazy_mm references to mm to mm_count refcounts.  Our
+ * caller holds an mm_count reference, so we don't need to worry about mm
+ * being freed out from under us.
+ */
+void mm_unlazy_mm_count(struct mm_struct *mm)
+{

+

kernel/sched/core.c +26 -0

+
+		/*
+		 * We know that old_mm != mm: when we did the xchg(), we were
+		 * the only cpu to be putting mm into any drop_mm variable.
+		 */
+		WARN_ON_ONCE(old_mm == mm);
+		if (unlikely(old_mm)) {
+			/*
+			 * We just stole an mm reference from the target CPU.
+			 *
+			 * drop_mm was set to old by another call to
+			 * mm_unlazy_mm_count().  After that call xchg'd old
+			 * into drop_mm, the target CPU did:
+			 *
+			 *  smp_store_release(&rq->lazy_mm, mm);
+			 *
+			 * which synchronized with our smp_load_acquire()
+			 * above, so we know that the target CPU is done with
+			 * old. Drop old on its behalf.
+			 */
+			mmdrop(old_mm);
+		}
+	}
+
+	atomic_sub(drop_count, &mm->mm_count);
+}

Quoted 26 lines · Peter Zijlstra

kernel/fork.c +2 -0

index e595e77913eb..57415cca088c 100644
@@ -1104,6 +1104,8 @@ static inline void __mmput(struct mm_struct *mm)
 	}
 	if (mm->binfmt)
 		module_put(mm->binfmt->module);
+
+	mm_unlazy_mm_count(mm);
 	mmdrop(mm);
 }
 

kernel/sched/core.c +7 -0

index 8ac693d542f6..e102ec53c2f6 100644
@@ -19,6 +19,7 @@
 
+/*
+ * This converts all lazy_mm references to mm to mm_count refcounts.  Our
+ * caller holds an mm_count reference, so we don't need to worry about mm
+ * being freed out from under us.
+ */
+void mm_unlazy_mm_count(struct mm_struct *mm)
+{

kernel/sched/core.c prepare_task_switch(struct rq *rq, struct task_struct *prev, · line 4541

Quoted 10 lines

kernel/sched/core.c +10 -0

+	unsigned int drop_count = num_possible_cpus();
+	int cpu;
+
+	/*
+	 * mm_users is zero, so no cpu will set its rq->lazy_mm to mm.
+	 */
+	WARN_ON_ONCE(atomic_read(&mm->mm_users) != 0);
+
+	/* Grab enough references for the rest of this function. */
+	atomic_add(drop_count, &mm->mm_count);

So that had me puzzled for a little while. Would something like this be a better comment?

/*
* Because this can race with mmdrop_lazy(), mm_count must be
* incremented before setting any rq->drop_mm value, otherwise
* it is possible to free mm early.
*/

Quoted 1 line · Andy Lutomirski

Here it is. Not even boot tested!

It is now, it even builds a kernel.. so it must be perfect :-)

Quoted 1 line

https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=sched/lazymm&id=ecc3992c36cb88087df9c537e2326efb51c95e31

Since I had to turn it into a patch to post, so that I could comment on it, I've cleaned it up a little for you.

I'll reply to self with some notes, but I think I like it.

---
 arch/x86/include/asm/mmu.h |   5 ++
 include/linux/sched/mm.h   |   3 +
 kernel/fork.c              |   2 +
 kernel/sched/core.c        | 138 ++++++++++++++++++++++++++++++++++++---------
 kernel/sched/sched.h       |  10 +++-
 5 files changed, 130 insertions(+), 28 deletions(-)

arch/x86/include/asm/mmu.h +5 -0

index 5d7494631ea9..ce94162168c2 100644
@@ -66,4 +66,9 @@ typedef struct {
 void leave_mm(int cpu);
 #define leave_mm leave_mm
 
+/* On x86, mm_cpumask(mm) contains all CPUs that might be lazily using mm */
+#define for_each_possible_lazymm_cpu(cpu, mm) \
+	for_each_cpu((cpu), mm_cpumask((mm)))
+
+
 #endif /* _ASM_X86_MMU_H */

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

index e24b1fe348e3..5c7eafee6fea 100644

Quoted 1 line · Peter Zijlstra

+extern void mm_unlazy_mm_count(struct mm_struct *mm);

You didn't like mm_many_words_in_the_name_of_the_function()? :)

kernel/sched/core.c static struct rq *finish_task_switch(struct task_struct *prev) · line 4592

Quoted 4 lines

kernel/sched/core.c +0 -4

-	if (mm) {
-		membarrier_mm_sync_core_before_usermode(mm);
-		mmdrop(mm);
-	}

What happened here?

Quoted 1 line · Andy Lutomirski

Here it is. Not even boot tested!

It is now, it even builds a kernel.. so it must be perfect :-)

Quoted 1 line

https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=sched/lazymm&id=ecc3992c36cb88087df9c537e2326efb51c95e31

Since I had to turn it into a patch to post, so that I could comment on it, I've cleaned it up a little for you.

I'll reply to self with some notes, but I think I like it.

---
 arch/x86/include/asm/mmu.h |   5 ++
 include/linux/sched/mm.h   |   3 +
 kernel/fork.c              |   2 +
 kernel/sched/core.c        | 138 ++++++++++++++++++++++++++++++++++++---------
 kernel/sched/sched.h       |  10 +++-
 5 files changed, 130 insertions(+), 28 deletions(-)

arch/x86/include/asm/mmu.h +5 -0

index 5d7494631ea9..ce94162168c2 100644
@@ -66,4 +66,9 @@ typedef struct {
 void leave_mm(int cpu);
 #define leave_mm leave_mm
 
+/* On x86, mm_cpumask(mm) contains all CPUs that might be lazily using mm */
+#define for_each_possible_lazymm_cpu(cpu, mm) \
+	for_each_cpu((cpu), mm_cpumask((mm)))
+
+
 #endif /* _ASM_X86_MMU_H */

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

index e24b1fe348e3..5c7eafee6fea 100644
@@ -77,6 +77,9 @@ static inline bool mmget_not_zero(struct mm_struct *mm)
 
 /* mmput gets rid of the mappings and all user-space */
 extern void mmput(struct mm_struct *);
+
+extern void mm_unlazy_mm_count(struct mm_struct *mm);
+
 #ifdef CONFIG_MMU
 /* same as above but performs the slow path from the async context. Can
  * be called from the atomic context as well

kernel/fork.c +2 -0

index e595e77913eb..57415cca088c 100644
@@ -1104,6 +1104,8 @@ static inline void __mmput(struct mm_struct *mm)
 	}
 	if (mm->binfmt)
 		module_put(mm->binfmt->module);
+
+	mm_unlazy_mm_count(mm);
 	mmdrop(mm);
 }
 

kernel/sched/core.c +111 -27

index 8ac693d542f6..e102ec53c2f6 100644
@@ -19,6 +19,7 @@
 
 #include <asm/switch_to.h>
 #include <asm/tlb.h>
+#include <asm/mmu.h>
 
 #include "../workqueue_internal.h"
 #include "../../fs/io-wq.h"

@@ -4501,6 +4502,81 @@ prepare_task_switch(struct rq *rq, struct task_struct *prev,
 	prepare_arch_switch(next);
 }
 
+static void mmdrop_lazy(struct rq *rq)
+{
+	struct mm_struct *old_mm;
+
+	if (likely(!READ_ONCE(rq->drop_mm)))
+		return;
+
+	/*
+	 * Slow path.  This only happens when we recently stopped using
+	 * an mm that is exiting.
+	 */
+	old_mm = xchg(&rq->drop_mm, NULL);
+	if (old_mm)
+		mmdrop(old_mm);
+}
+
+#ifndef for_each_possible_lazymm_cpu
+#define for_each_possible_lazymm_cpu(cpu, mm) for_each_online_cpu((cpu))
+#endif
+
+/*
+ * This converts all lazy_mm references to mm to mm_count refcounts.  Our
+ * caller holds an mm_count reference, so we don't need to worry about mm
+ * being freed out from under us.
+ */
+void mm_unlazy_mm_count(struct mm_struct *mm)
+{
+	unsigned int drop_count = num_possible_cpus();
+	int cpu;
+
+	/*
+	 * mm_users is zero, so no cpu will set its rq->lazy_mm to mm.
+	 */
+	WARN_ON_ONCE(atomic_read(&mm->mm_users) != 0);
+
+	/* Grab enough references for the rest of this function. */
+	atomic_add(drop_count, &mm->mm_count);
+
+	for_each_possible_lazymm_cpu(cpu, mm) {
+		struct rq *rq = cpu_rq(cpu);
+		struct mm_struct *old_mm;
+
+		if (smp_load_acquire(&rq->lazy_mm) != mm)
+			continue;
+
+		drop_count--;	/* grab a reference; cpu will drop it later. */
+
+		old_mm = xchg(&rq->drop_mm, mm);
+
+		/*
+		 * We know that old_mm != mm: when we did the xchg(), we were
+		 * the only cpu to be putting mm into any drop_mm variable.
+		 */
+		WARN_ON_ONCE(old_mm == mm);
+		if (unlikely(old_mm)) {
+			/*
+			 * We just stole an mm reference from the target CPU.
+			 *
+			 * drop_mm was set to old by another call to
+			 * mm_unlazy_mm_count().  After that call xchg'd old
+			 * into drop_mm, the target CPU did:
+			 *
+			 *  smp_store_release(&rq->lazy_mm, mm);
+			 *
+			 * which synchronized with our smp_load_acquire()
+			 * above, so we know that the target CPU is done with
+			 * old. Drop old on its behalf.
+			 */
+			mmdrop(old_mm);
+		}
+	}
+
+	atomic_sub(drop_count, &mm->mm_count);
+}
+
 /**
  * finish_task_switch - clean up after a task-switch
  * @prev: the thread we just switched away from.

@@ -4524,7 +4600,6 @@ static struct rq *finish_task_switch(struct task_struct *prev)
 	__releases(rq->lock)
 {
 	struct rq *rq = this_rq();
-	struct mm_struct *mm = rq->prev_mm;
 	long prev_state;
 
 	/*

@@ -4543,8 +4618,6 @@ static struct rq *finish_task_switch(struct task_struct *prev)
 		      current->comm, current->pid, preempt_count()))
 		preempt_count_set(FORK_PREEMPT_COUNT);
 
-	rq->prev_mm = NULL;
-
 	/*
 	 * A task struct has one reference for the use as "current".
 	 * If a task dies, then it sets TASK_DEAD in tsk->state and calls

@@ -4574,22 +4647,16 @@ 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:
-	 *
-	 * - a full memory barrier for {PRIVATE,GLOBAL}_EXPEDITED, implicitly
-	 *   provided by mmdrop(),
-	 * - a sync_core for SYNC_CORE.
+	 * Do this unconditionally.  There's a race in which a remote CPU
+	 * sees rq->lazy_mm != NULL and gives us an extra mm ref while we
+	 * are executing this code and we don't notice.  Instead of letting
+	 * that ref sit around until the next time we unlazy, do it on every
+	 * context switch.
 	 */
-	if (mm) {
-		membarrier_mm_sync_core_before_usermode(mm);
-		mmdrop(mm);
-	}
+	mmdrop_lazy(rq);
+
 	if (unlikely(prev_state == TASK_DEAD)) {
 		if (prev->sched_class->task_dead)
 			prev->sched_class->task_dead(prev);

@@ -4652,25 +4719,32 @@ context_switch(struct rq *rq, struct task_struct *prev,
 
 	/*
 	 * kernel -> kernel   lazy + transfer active
-	 *   user -> kernel   lazy + mmgrab() active
+	 *   user -> kernel   lazy + lazy_mm grab active
 	 *
-	 * kernel ->   user   switch + mmdrop() active
+	 * kernel ->   user   switch + lazy_mm release active
 	 *   user ->   user   switch
 	 */
 	if (!next->mm) {                                // to kernel
 		enter_lazy_tlb(prev->active_mm, next);
 
 		next->active_mm = prev->active_mm;
-		if (prev->mm)                           // from user
-			mmgrab(prev->active_mm);
-		else
+		if (prev->mm) {                         // from user
+			SCHED_WARN_ON(rq->lazy_mm);
+
+			/*
+			 * Acqure a lazy_mm reference to the active
+			 * (lazy) mm.  No explicit barrier needed: we still
+			 * hold an explicit (mm_users) reference.  __mmput()
+			 * can't be called until we call mmput() to drop
+			 * our reference, and __mmput() is a release barrier.
+			 */
+			WRITE_ONCE(rq->lazy_mm, next->active_mm);
+		} else {
 			prev->active_mm = NULL;
+		}
 	} else {                                        // to user
 		membarrier_switch_mm(rq, prev->active_mm, next->mm);
 		/*
-		 * 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().

@@ -4678,9 +4752,19 @@ context_switch(struct rq *rq, struct task_struct *prev,
 		switch_mm_irqs_off(prev->active_mm, next->mm, next);
 
 		if (!prev->mm) {                        // from kernel
-			/* will mmdrop() in finish_task_switch(). */
-			rq->prev_mm = prev->active_mm;
+			/*
+			 * Even though nothing should reference ->active_mm
+			 * for a non-current task, don't leave a stale pointer
+			 * to an mm that might be freed.
+			 */
 			prev->active_mm = NULL;
+
+			/*
+			 * Drop our lazy_mm reference to the old lazy mm.
+			 * After this, any CPU may free it if it is
+			 * unreferenced.
+			 */
+			smp_store_release(&rq->lazy_mm, NULL);
 		}
 	}
 

kernel/sched/sched.h +9 -1

index 8f0194cee0ba..703d95a4abd0 100644
@@ -966,7 +966,15 @@ struct rq {
 	struct task_struct	*idle;
 	struct task_struct	*stop;
 	unsigned long		next_balance;
-	struct mm_struct	*prev_mm;
+
+	/*
+	 * Fast refcounting scheme for lazy mm.  lazy_mm is a hazard pointer:
+	 * setting it to point to a lazily used mm keeps that mm from being
+	 * freed.  drop_mm points to am mm that needs an mmdrop() call
+	 * after the CPU owning the rq is done with it.
+	 */
+	struct mm_struct	*lazy_mm;
+	struct mm_struct	*drop_mm;
 
 	unsigned int		clock_update_flags;
 	u64			clock;

Quoted 11 lines · Peter Zijlstra

Quoted 1 line · Andy Lutomirski

Here it is. Not even boot tested!

It is now, it even builds a kernel.. so it must be perfect :-)

Quoted 1 line

https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=sched/lazymm&id=ecc3992c36cb88087df9c537e2326efb51c95e31

Since I had to turn it into a patch to post, so that I could comment on it, I've cleaned it up a little for you.

I'll reply to self with some notes, but I think I like it.

But rcutorture isn't too happy with it when applied to current mainline:

------------------------------------------------------------------------

[   32.559192] ------------[ cut here ]------------
[   32.559528] WARNING: CPU: 0 PID: 175 at kernel/fork.c:686 
__mmdrop+0x9f/0xb0
[   32.560197] Modules linked in:
[   32.560470] CPU: 0 PID: 175 Comm: torture_onoff Not tainted 
5.13.0-rc6+ #23
[   32.561077] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 
1.13.0-1ubuntu1.1 04/01/2014
[   32.561809] RIP: 0010:__mmdrop+0x9f/0xb0
[   32.562179] Code: fb 20 75 e6 48 8b 45 68 48 85 c0 0f 85 1e 48 ad 00 
48 8b 3d 93 e0 c3 01 5b 48 89 ee 5d 41 5c e9 97 45 18 00 0f 0b 0f 0b eb 
87 <0f> 0b eb 95 48 89 ef e8 a5 f1 17 00 eb a9 0f 1f 00 48 81 ef c0 03
[   32.563822] RSP: 0018:ffff944c40623d68 EFLAGS: 00010246
[   32.564331] RAX: ffff8e84c2339c00 RBX: ffff8e84df5572e0 RCX: 
00000000fffffffa
[   32.564978] RDX: 0000000000000000 RSI: 0000000000000033 RDI: 
ffff8e84c29a0000
[   32.565648] RBP: ffff8e84c29a0000 R08: ffff8e84c11c774a R09: 
0000000000000001
[   32.566256] R10: ffff8e85411c773f R11: ffff8e84c11c774a R12: 
0000000000000057
[   32.566909] R13: 0000000000000000 R14: ffffffffb0e487f8 R15: 

000000000000000d

[   32.567584] FS:  0000000000000000(0000) GS:ffff8e84df200000(0000) 
knlGS:0000000000000000
[   32.568321] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   32.568860] CR2: 0000000000000000 CR3: 00000000029ec000 CR4: 
00000000000006f0
[   32.569559] Call Trace:
[   32.569804]  ? takedown_cpu+0xd0/0xd0
[   32.570123]  finish_cpu+0x2e/0x40

[   32.570449]  cpuhp_invoke_callback+0xf6/0x3f0
[   32.570755]  cpuhp_invoke_callback_range+0x3b/0x80
[   32.571137]  _cpu_down+0xdf/0x2a0
[   32.571467]  cpu_down+0x2a/0x50
[   32.571771]  device_offline+0x80/0xb0
[   32.572101]  remove_cpu+0x1a/0x30
[   32.572393]  torture_offline+0x80/0x140
[   32.572730]  torture_onoff+0x147/0x260
[   32.573068]  ? torture_kthread_stopping+0xa0/0xa0
[   32.573488]  kthread+0xf9/0x130
[   32.573777]  ? kthread_park+0x80/0x80
[   32.574119]  ret_from_fork+0x22/0x30
[   32.574418] ---[ end trace b77effd8aab7f902 ]---
[   32.574819] BUG: Bad rss-counter state mm:00000000bccc5a55 
type:MM_ANONPAGES val:1
[   32.575450] BUG: non-zero pgtables_bytes on freeing mm: 24576

------------------------------------------------------------------------

Are we absolutely sure that the mmdrop()s are balanced in all cases?

Thanx, Paul

Quoted 275 lines

---
 arch/x86/include/asm/mmu.h |   5 ++
 include/linux/sched/mm.h   |   3 +
 kernel/fork.c              |   2 +
 kernel/sched/core.c        | 138 ++++++++++++++++++++++++++++++++++++---------
 kernel/sched/sched.h       |  10 +++-
 5 files changed, 130 insertions(+), 28 deletions(-)

arch/x86/include/asm/mmu.h +5 -0

index 5d7494631ea9..ce94162168c2 100644
@@ -66,4 +66,9 @@ typedef struct {
 void leave_mm(int cpu);
 #define leave_mm leave_mm
 
+/* On x86, mm_cpumask(mm) contains all CPUs that might be lazily using mm */
+#define for_each_possible_lazymm_cpu(cpu, mm) \
+	for_each_cpu((cpu), mm_cpumask((mm)))
+
+
 #endif /* _ASM_X86_MMU_H */

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

index e24b1fe348e3..5c7eafee6fea 100644
@@ -77,6 +77,9 @@ static inline bool mmget_not_zero(struct mm_struct *mm)
 
 /* mmput gets rid of the mappings and all user-space */
 extern void mmput(struct mm_struct *);
+
+extern void mm_unlazy_mm_count(struct mm_struct *mm);
+
 #ifdef CONFIG_MMU
 /* same as above but performs the slow path from the async context. Can
  * be called from the atomic context as well

kernel/fork.c +2 -0

index e595e77913eb..57415cca088c 100644
@@ -1104,6 +1104,8 @@ static inline void __mmput(struct mm_struct *mm)
 	}
 	if (mm->binfmt)
 		module_put(mm->binfmt->module);
+
+	mm_unlazy_mm_count(mm);
 	mmdrop(mm);
 }
 

kernel/sched/core.c +111 -27

index 8ac693d542f6..e102ec53c2f6 100644
@@ -19,6 +19,7 @@
 
 #include <asm/switch_to.h>
 #include <asm/tlb.h>
+#include <asm/mmu.h>
 
 #include "../workqueue_internal.h"
 #include "../../fs/io-wq.h"

@@ -4501,6 +4502,81 @@ prepare_task_switch(struct rq *rq, struct task_struct *prev,
 	prepare_arch_switch(next);
 }
 
+static void mmdrop_lazy(struct rq *rq)
+{
+	struct mm_struct *old_mm;
+
+	if (likely(!READ_ONCE(rq->drop_mm)))
+		return;
+
+	/*
+	 * Slow path.  This only happens when we recently stopped using
+	 * an mm that is exiting.
+	 */
+	old_mm = xchg(&rq->drop_mm, NULL);
+	if (old_mm)
+		mmdrop(old_mm);
+}
+
+#ifndef for_each_possible_lazymm_cpu
+#define for_each_possible_lazymm_cpu(cpu, mm) for_each_online_cpu((cpu))
+#endif
+
+/*
+ * This converts all lazy_mm references to mm to mm_count refcounts.  Our
+ * caller holds an mm_count reference, so we don't need to worry about mm
+ * being freed out from under us.
+ */
+void mm_unlazy_mm_count(struct mm_struct *mm)
+{
+	unsigned int drop_count = num_possible_cpus();
+	int cpu;
+
+	/*
+	 * mm_users is zero, so no cpu will set its rq->lazy_mm to mm.
+	 */
+	WARN_ON_ONCE(atomic_read(&mm->mm_users) != 0);
+
+	/* Grab enough references for the rest of this function. */
+	atomic_add(drop_count, &mm->mm_count);
+
+	for_each_possible_lazymm_cpu(cpu, mm) {
+		struct rq *rq = cpu_rq(cpu);
+		struct mm_struct *old_mm;
+
+		if (smp_load_acquire(&rq->lazy_mm) != mm)
+			continue;
+
+		drop_count--;	/* grab a reference; cpu will drop it later. */
+
+		old_mm = xchg(&rq->drop_mm, mm);
+
+		/*
+		 * We know that old_mm != mm: when we did the xchg(), we were
+		 * the only cpu to be putting mm into any drop_mm variable.
+		 */
+		WARN_ON_ONCE(old_mm == mm);
+		if (unlikely(old_mm)) {
+			/*
+			 * We just stole an mm reference from the target CPU.
+			 *
+			 * drop_mm was set to old by another call to
+			 * mm_unlazy_mm_count().  After that call xchg'd old
+			 * into drop_mm, the target CPU did:
+			 *
+			 *  smp_store_release(&rq->lazy_mm, mm);
+			 *
+			 * which synchronized with our smp_load_acquire()
+			 * above, so we know that the target CPU is done with
+			 * old. Drop old on its behalf.
+			 */
+			mmdrop(old_mm);
+		}
+	}
+
+	atomic_sub(drop_count, &mm->mm_count);
+}
+
 /**
  * finish_task_switch - clean up after a task-switch
  * @prev: the thread we just switched away from.

@@ -4524,7 +4600,6 @@ static struct rq *finish_task_switch(struct task_struct *prev)
 	__releases(rq->lock)
 {
 	struct rq *rq = this_rq();
-	struct mm_struct *mm = rq->prev_mm;
 	long prev_state;
 
 	/*

@@ -4543,8 +4618,6 @@ static struct rq *finish_task_switch(struct task_struct *prev)
 		      current->comm, current->pid, preempt_count()))
 		preempt_count_set(FORK_PREEMPT_COUNT);
 
-	rq->prev_mm = NULL;
-
 	/*
 	 * A task struct has one reference for the use as "current".
 	 * If a task dies, then it sets TASK_DEAD in tsk->state and calls

@@ -4574,22 +4647,16 @@ 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:
-	 *
-	 * - a full memory barrier for {PRIVATE,GLOBAL}_EXPEDITED, implicitly
-	 *   provided by mmdrop(),
-	 * - a sync_core for SYNC_CORE.
+	 * Do this unconditionally.  There's a race in which a remote CPU
+	 * sees rq->lazy_mm != NULL and gives us an extra mm ref while we
+	 * are executing this code and we don't notice.  Instead of letting
+	 * that ref sit around until the next time we unlazy, do it on every
+	 * context switch.
 	 */
-	if (mm) {
-		membarrier_mm_sync_core_before_usermode(mm);
-		mmdrop(mm);
-	}
+	mmdrop_lazy(rq);
+
 	if (unlikely(prev_state == TASK_DEAD)) {
 		if (prev->sched_class->task_dead)
 			prev->sched_class->task_dead(prev);

@@ -4652,25 +4719,32 @@ context_switch(struct rq *rq, struct task_struct *prev,
 
 	/*
 	 * kernel -> kernel   lazy + transfer active
-	 *   user -> kernel   lazy + mmgrab() active
+	 *   user -> kernel   lazy + lazy_mm grab active
 	 *
-	 * kernel ->   user   switch + mmdrop() active
+	 * kernel ->   user   switch + lazy_mm release active
 	 *   user ->   user   switch
 	 */
 	if (!next->mm) {                                // to kernel
 		enter_lazy_tlb(prev->active_mm, next);
 
 		next->active_mm = prev->active_mm;
-		if (prev->mm)                           // from user
-			mmgrab(prev->active_mm);
-		else
+		if (prev->mm) {                         // from user
+			SCHED_WARN_ON(rq->lazy_mm);
+
+			/*
+			 * Acqure a lazy_mm reference to the active
+			 * (lazy) mm.  No explicit barrier needed: we still
+			 * hold an explicit (mm_users) reference.  __mmput()
+			 * can't be called until we call mmput() to drop
+			 * our reference, and __mmput() is a release barrier.
+			 */
+			WRITE_ONCE(rq->lazy_mm, next->active_mm);
+		} else {
 			prev->active_mm = NULL;
+		}
 	} else {                                        // to user
 		membarrier_switch_mm(rq, prev->active_mm, next->mm);
 		/*
-		 * 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().

@@ -4678,9 +4752,19 @@ context_switch(struct rq *rq, struct task_struct *prev,
 		switch_mm_irqs_off(prev->active_mm, next->mm, next);
 
 		if (!prev->mm) {                        // from kernel
-			/* will mmdrop() in finish_task_switch(). */
-			rq->prev_mm = prev->active_mm;
+			/*
+			 * Even though nothing should reference ->active_mm
+			 * for a non-current task, don't leave a stale pointer
+			 * to an mm that might be freed.
+			 */
 			prev->active_mm = NULL;
+
+			/*
+			 * Drop our lazy_mm reference to the old lazy mm.
+			 * After this, any CPU may free it if it is
+			 * unreferenced.
+			 */
+			smp_store_release(&rq->lazy_mm, NULL);
 		}
 	}
 

kernel/sched/sched.h +9 -1

index 8f0194cee0ba..703d95a4abd0 100644
@@ -966,7 +966,15 @@ struct rq {
 	struct task_struct	*idle;
 	struct task_struct	*stop;
 	unsigned long		next_balance;
-	struct mm_struct	*prev_mm;
+
+	/*
+	 * Fast refcounting scheme for lazy mm.  lazy_mm is a hazard pointer:
+	 * setting it to point to a lazily used mm keeps that mm from being
+	 * freed.  drop_mm points to am mm that needs an mmdrop() call
+	 * after the CPU owning the rq is done with it.
+	 */
+	struct mm_struct	*lazy_mm;
+	struct mm_struct	*drop_mm;
 
 	unsigned int		clock_update_flags;
 	u64			clock;

Quoted 46 lines · Andy Lutomirski

Quoted 45 lines · Nicholas Piggin

Excerpts from Andy Lutomirski's message of June 17, 2021 4:41 am:

Quoted 35 lines · Peter Zijlstra

Quoted 15 lines · Nicholas Piggin

Quoted 7 lines

Excerpts from Andy Lutomirski's message of June 16, 2021 1:21 pm:

Quoted 5 lines

membarrier() needs a barrier after any CPU changes mm. There is currently a comment explaining why this barrier probably exists in all cases. This is very fragile -- any change to the relevant parts of the scheduler might get rid of these barriers, and it's not really clear to me that the barrier actually exists in all necessary cases.

Quoted 6 lines

The comments and barriers in the mmdrop() hunks? I don't see what is fragile or maybe-buggy about this. The barrier definitely exists.

And any change can change anything, that doesn't make it fragile. My lazy tlb refcounting change avoids the mmdrop in some cases, but it replaces it with smp_mb for example.

Quoted 7 lines

I'm with Nick again, on this. You're adding extra barriers for no discernible reason, that's not generally encouraged, seeing how extra barriers is extra slow.

Both mmdrop() itself, as well as the callsite have comments saying how membarrier relies on the implied barrier, what's fragile about that?

My real motivation is that mmgrab() and mmdrop() don't actually need to be full barriers. The current implementation has them being full barriers, and the current implementation is quite slow. So let's try that commit message again:

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.

Quoted 3 lines

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.

I'm not against the idea, I've looked at something similar before (not for mmdrop but a different primitive). Also my lazy tlb shootdown series could possibly take advantage of this, I might cherry pick it and test performance :)

Quoted 4 lines

Quoted 2 lines

I don't think it belongs in this series though. Should go together with something that takes advantage of it.

I’m going to see if I can get hazard pointers into shape quickly.

Here it is. Not even boot tested!

https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=sched/lazymm&id=ecc3992c36cb88087df9c537e2326efb51c95e31

Nick, I think you can accomplish much the same thing as your patch by:

#define for_each_possible_lazymm_cpu while (false)

although a more clever definition might be even more performant.

I would appreciate everyone's thoughts as to whether this scheme is sane.

Paul, I'm adding you for two reasons. First, you seem to enjoy bizarre locking schemes. Secondly, because maybe RCU could actually work here. The basic idea is that we want to keep an mm_struct from being freed at an inopportune time. The problem with naively using RCU is that each CPU can use one single mm_struct while in an idle extended quiescent state (but not a user extended quiescent state). So rcu_read_lock() is right out. If RCU could understand this concept, then maybe it could help us, but this seems a bit out of scope for RCU.

Quoted 3 lines · Andy Lutomirski

I would appreciate everyone's thoughts as to whether this scheme is sane.

Paul, I'm adding you for two reasons. First, you seem to enjoy bizarre locking schemes. Secondly, because maybe RCU could actually work here. The basic idea is that we want to keep an mm_struct from being freed at an inopportune time. The problem with naively using RCU is that each CPU can use one single mm_struct while in an idle extended quiescent state (but not a user extended quiescent state). So rcu_read_lock() is right out. If RCU could understand this concept, then maybe it could help us, but this seems a bit out of scope for RCU.

OK, I should look at your patch, but that will be after morning meetings.

On RCU and idle, much of the idle code now allows rcu_read_lock() to be directly, thanks to Peter's recent work. Any sort of interrupt or NMI from idle can also use rcu_read_lock(), including the IPIs that are now done directly from idle. RCU_NONIDLE() makes RCU pay attention to the code supplied as its sole argument.

Or is your patch really having the CPU expect a mm_struct to stick around across the full idle sojourn, and without the assistance of mmgrab() and mmdrop()?

Quoted 13 lines · Paul E. McKenney

Quoted 3 lines · Andy Lutomirski

I would appreciate everyone's thoughts as to whether this scheme is sane.

Paul, I'm adding you for two reasons. First, you seem to enjoy bizarre locking schemes. Secondly, because maybe RCU could actually work here. The basic idea is that we want to keep an mm_struct from being freed at an inopportune time. The problem with naively using RCU is that each CPU can use one single mm_struct while in an idle extended quiescent state (but not a user extended quiescent state). So rcu_read_lock() is right out. If RCU could understand this concept, then maybe it could help us, but this seems a bit out of scope for RCU.

OK, I should look at your patch, but that will be after morning meetings.

On RCU and idle, much of the idle code now allows rcu_read_lock() to be directly, thanks to Peter's recent work. Any sort of interrupt or NMI from idle can also use rcu_read_lock(), including the IPIs that are now done directly from idle. RCU_NONIDLE() makes RCU pay attention to the code supplied as its sole argument.

Quoted 3 lines

Or is your patch really having the CPU expect a mm_struct to stick around across the full idle sojourn, and without the assistance of mmgrab() and mmdrop()?

I really do expect it to stick around across the full idle sojourn. Unless RCU is more magical than I think it is, this means I can't use RCU.

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.

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]>

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

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

fs/exec.c +1 -1

index 18594f11c31f..2e63dea83411 100644
@@ -1007,7 +1007,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/kthread.c +2 -2

index 8275b415acec..4962794e02d5 100644
@@ -1322,7 +1322,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(atomic_read(&mm->membarrier_state));

@@ -1363,7 +1363,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 3173b063d358..c32c32a2441e 100644
@@ -410,7 +410,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

 				__cpumask_set_cpu(cpu, tmpmask);
 		}
 		rcu_read_unlock();

@@ -521,7 +521,7 @@ static int sync_runqueues_membarrier_state(struct mm_struct *mm)
 		struct task_struct *p;
 
 		p = rcu_dereference(rq->curr);
-		if (p && p->mm == mm)
+		if (p && READ_ONCE(p->mm) == mm)
 			__cpumask_set_cpu(cpu, tmpmask);
 	}
 	rcu_read_unlock();

Signature · 1 line

2.31.1

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

Quoted 2 lines · ----- On Jun 15, 2021, at 11:21 PM, Andy Lutomirski [email protected] wrote:

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

I notice that kernel/exit.c:exit_mm() also has:

        current->mm = NULL;

I suspect you may want to add a WRITE_ONCE() there as well ?

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.

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 | 27 ---------------------------
 arch/powerpc/mm/mmu_context.c         |  2 --
 2 files changed, 29 deletions(-)
 delete mode 100644 arch/powerpc/include/asm/membarrier.h

arch/powerpc/include/asm/membarrier.h deleted +0 -27

deleted file mode 100644
index 6e20bb5c74ea..000000000000
@@ -1,27 +0,0 @@
-#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 (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 -2

index a857af401738..8daa95b3162b 100644
@@ -85,8 +85,6 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
 
 	if (new_on_cpu)
 		radix_kvm_prefetch_workaround(next);
-	else
-		membarrier_arch_switch_mm(prev, next, tsk);
 
 	/*
 	 * The actual HW switching method differs between the various

Signature · 1 line

2.31.1

On arm32, the only way to safely flush icache from usermode is to call cacheflush(2). This also handles any required pipeline flushes, so membarrier's SYNC_CORE feature is useless on arm. Remove it.

On arm32, the only way to safely flush icache from usermode is to call cacheflush(2). This also handles any required pipeline flushes, so membarrier's SYNC_CORE feature is useless on arm. Remove it.

Quoted 3 lines · Andy Lutomirski

On arm32, the only way to safely flush icache from usermode is to call cacheflush(2). This also handles any required pipeline flushes, so membarrier's SYNC_CORE feature is useless on arm. Remove it.

So SYNC_CORE is there to help an architecture that needs to do something per CPU. If I$ invalidation is broadcast and I$ invalidation also triggers the flush of any uarch caches derived from it (if there are any).

Now arm_syscall() NR(cacheflush) seems to do flush_icache_user_range(), which, if I read things right, end up in arch/arm/mm/*.S, but that doesn't consider cache_ops_need_broadcast().

Will suggests that perhaps ARM 11MPCore might need this due to their I$ flush maybe not being broadcast

Quoted 5 lines · Peter Zijlstra

Quoted 3 lines · Andy Lutomirski

On arm32, the only way to safely flush icache from usermode is to call cacheflush(2). This also handles any required pipeline flushes, so membarrier's SYNC_CORE feature is useless on arm. Remove it.

Quoted 4 lines

So SYNC_CORE is there to help an architecture that needs to do something per CPU. If I$ invalidation is broadcast and I$ invalidation also triggers the flush of any uarch caches derived from it (if there are any).

Incomplete sentence there: + then we don't need SYNC_CORE.

Quoted 6 lines

Now arm_syscall() NR(cacheflush) seems to do flush_icache_user_range(), which, if I read things right, end up in arch/arm/mm/*.S, but that doesn't consider cache_ops_need_broadcast().

Will suggests that perhaps ARM 11MPCore might need this due to their I$ flush maybe not being broadcast

Quoted 6 lines · Peter Zijlstra

Quoted 5 lines · Peter Zijlstra

Quoted 3 lines · Andy Lutomirski

On arm32, the only way to safely flush icache from usermode is to call cacheflush(2). This also handles any required pipeline flushes, so membarrier's SYNC_CORE feature is useless on arm. Remove it.

Quoted 13 lines

Quoted 4 lines

So SYNC_CORE is there to help an architecture that needs to do something per CPU. If I$ invalidation is broadcast and I$ invalidation also triggers the flush of any uarch caches derived from it (if there are any).

Incomplete sentence there: + then we don't need SYNC_CORE.

Quoted 6 lines

Now arm_syscall() NR(cacheflush) seems to do flush_icache_user_range(), which, if I read things right, end up in arch/arm/mm/*.S, but that doesn't consider cache_ops_need_broadcast().

Will suggests that perhaps ARM 11MPCore might need this due to their I$ flush maybe not being broadcast

If it leaves other cores with incoherent I cache, then that's already a problem for SMP cores, since there could be no guarantee that the modifications made by one core will be visible to some other core that ends up running that code - and there is little option for userspace to work around that except by pinning the thread making the modifications and subsequently executing the code to a core.

The same is also true of flush_icache_range() - which is used when loading a kernel module. In the case Will is referring to, these alias to the same code.

Quoted 21 lines · Russell King (Oracle)

Quoted 6 lines · Peter Zijlstra

Quoted 5 lines · Peter Zijlstra

Quoted 3 lines · Andy Lutomirski

On arm32, the only way to safely flush icache from usermode is to call cacheflush(2). This also handles any required pipeline flushes, so membarrier's SYNC_CORE feature is useless on arm. Remove it.

Quoted 13 lines

Quoted 4 lines

So SYNC_CORE is there to help an architecture that needs to do something per CPU. If I$ invalidation is broadcast and I$ invalidation also triggers the flush of any uarch caches derived from it (if there are any).

Incomplete sentence there: + then we don't need SYNC_CORE.

Quoted 6 lines

Now arm_syscall() NR(cacheflush) seems to do flush_icache_user_range(), which, if I read things right, end up in arch/arm/mm/*.S, but that doesn't consider cache_ops_need_broadcast().

Will suggests that perhaps ARM 11MPCore might need this due to their I$ flush maybe not being broadcast

Quoted 6 lines

If it leaves other cores with incoherent I cache, then that's already a problem for SMP cores, since there could be no guarantee that the modifications made by one core will be visible to some other core that ends up running that code - and there is little option for userspace to work around that except by pinning the thread making the modifications and subsequently executing the code to a core.

That's where SYNC_CORE can help. Or you make sys_cacheflush() do a system wide IPI.

Quoted 3 lines

The same is also true of flush_icache_range() - which is used when loading a kernel module. In the case Will is referring to, these alias to the same code.

Yes, cache_ops_need_broadcast() seems to be missing in more places.

Quoted 29 lines · Peter Zijlstra

Quoted 27 lines · Russell King (Oracle)

Quoted 6 lines · Peter Zijlstra

Quoted 5 lines · Peter Zijlstra

Quoted 3 lines · Andy Lutomirski

On arm32, the only way to safely flush icache from usermode is to call cacheflush(2). This also handles any required pipeline flushes, so membarrier's SYNC_CORE feature is useless on arm. Remove it.

Quoted 13 lines

Quoted 4 lines

So SYNC_CORE is there to help an architecture that needs to do something per CPU. If I$ invalidation is broadcast and I$ invalidation also triggers the flush of any uarch caches derived from it (if there are any).

Incomplete sentence there: + then we don't need SYNC_CORE.

Quoted 6 lines

Now arm_syscall() NR(cacheflush) seems to do flush_icache_user_range(), which, if I read things right, end up in arch/arm/mm/*.S, but that doesn't consider cache_ops_need_broadcast().

Will suggests that perhaps ARM 11MPCore might need this due to their I$ flush maybe not being broadcast

If it leaves other cores with incoherent I cache, then that's already a problem for SMP cores, since there could be no guarantee that the modifications made by one core will be visible to some other core that ends up running that code - and there is little option for userspace to work around that except by pinning the thread making the modifications and subsequently executing the code to a core.

Quoted 2 lines

That's where SYNC_CORE can help. Or you make sys_cacheflush() do a system wide IPI.

If it's a problem, then it needs fixing. sys_cacheflush() is used to implement GCC's __builtin___clear_cache(). I'm not sure who added this to gcc.

Quoted 5 lines

Quoted 3 lines

The same is also true of flush_icache_range() - which is used when loading a kernel module. In the case Will is referring to, these alias to the same code.

Yes, cache_ops_need_broadcast() seems to be missing in more places.

Likely only in places where we care about I/D coherency - as the data cache is required to be PIPT on these SMP platforms.

Quoted 3 lines · Russell King

If it's a problem, then it needs fixing. sys_cacheflush() is used to implement GCC's __builtin___clear_cache(). I'm not sure who added this to gcc.

I'm surprised that it works. I guess it's just luck that the thread doing the code writing doesn't migrate before the sys_cacheflush() call.

Quoted 2 lines

Likely only in places where we care about I/D coherency - as the data cache is required to be PIPT on these SMP platforms.

We had similar issue with the cache maintenance for DMA. The hack we employed (in cache.S) is relying on the MESI protocol internals and forcing a read/write for ownership before the D-cache maintenance. Luckily ARM11MPCore doesn't do speculative data loads to trigger some migration back.

The simpler fix for flush_icache_range() is to disable preemption, read a word in a cacheline to force any dirty lines on another CPU to be evicted and then issue the D-cache maintenance (for those cache lines which are still dirty on the current CPU).

Quoted 7 lines · Catalin Marinas

Quoted 3 lines · Russell King

If it's a problem, then it needs fixing. sys_cacheflush() is used to implement GCC's __builtin___clear_cache(). I'm not sure who added this to gcc.

I'm surprised that it works. I guess it's just luck that the thread doing the code writing doesn't migrate before the sys_cacheflush() call.

Maybe the platforms that use ARM MPCore avoid the issue somehow (maybe by not using self-modifying code?)

Quoted 8 lines

Quoted 2 lines

Likely only in places where we care about I/D coherency - as the data cache is required to be PIPT on these SMP platforms.

We had similar issue with the cache maintenance for DMA. The hack we employed (in cache.S) is relying on the MESI protocol internals and forcing a read/write for ownership before the D-cache maintenance. Luckily ARM11MPCore doesn't do speculative data loads to trigger some migration back.

That's very similar to the hack that was originally implemented for MPCore DMA - see the DMA_CACHE_RWFO configuration option.

An interesting point here is that cache_ops_need_broadcast() reads MMFR3 bits 12..15, which in the MPCore TRM has nothing to with cache operation broadcasting - but luckily is documented as containing zero. So, cache_ops_need_broadcast() returns correctly (true) here.

Quoted 4 lines

The simpler fix for flush_icache_range() is to disable preemption, read a word in a cacheline to force any dirty lines on another CPU to be evicted and then issue the D-cache maintenance (for those cache lines which are still dirty on the current CPU).

Is just reading sufficient? If so, why do we do a read-then-write in the MPCore DMA cache ops? Don't we need the write to force exclusive ownership? If we don't have exclusive ownership of the dirty line, how can we be sure to write it out of the caches?

Quoted 10 lines · Russell King

Quoted 4 lines · Catalin Marinas

The simpler fix for flush_icache_range() is to disable preemption, read a word in a cacheline to force any dirty lines on another CPU to be evicted and then issue the D-cache maintenance (for those cache lines which are still dirty on the current CPU).

Is just reading sufficient? If so, why do we do a read-then-write in the MPCore DMA cache ops? Don't we need the write to force exclusive ownership? If we don't have exclusive ownership of the dirty line, how can we be sure to write it out of the caches?

For cleaning (which is the case for I/D coherency), we only need reading since we are fine with clean lines being left in the D-cache on other CPUs. For invalidation, we indeed need to force the exclusive ownership, hence the write.

Quoted 12 lines · Catalin Marinas

Quoted 10 lines · Russell King

Quoted 4 lines · Catalin Marinas

The simpler fix for flush_icache_range() is to disable preemption, read a word in a cacheline to force any dirty lines on another CPU to be evicted and then issue the D-cache maintenance (for those cache lines which are still dirty on the current CPU).

Is just reading sufficient? If so, why do we do a read-then-write in the MPCore DMA cache ops? Don't we need the write to force exclusive ownership? If we don't have exclusive ownership of the dirty line, how can we be sure to write it out of the caches?

Quoted 4 lines

For cleaning (which is the case for I/D coherency), we only need reading since we are fine with clean lines being left in the D-cache on other CPUs. For invalidation, we indeed need to force the exclusive ownership, hence the write.

Ah, I'm not sure the I-cache is broadcast in hardware on ARM11MPCore either. So fixing the D side won't be sufficient.

So it seems to come down to a question about CNS3xxx and OXNAS. If these aren't being used, maybe we can drop ARM11MPCore support and the associated platforms?

Arnd tells me that the current remaining ARM11MPCore users are:
- CNS3xxx (where there is some martinal interest in the Gateworks Laguna platform)
- Similar for OXNAS
- There used to be the Realview MPCore tile - I haven't turned that on in ages, and it may be that the 3V cell that backs up the encryption keys is dead so it may not even boot.

So it seems to come down to a question about CNS3xxx and OXNAS. If these aren't being used, maybe we can drop ARM11MPCore support and the associated platforms?

Linus, Krzysztof, Neil, any input?

Quoted 7 lines · Russell King (Oracle)

Arnd tells me that the current remaining ARM11MPCore users are:
- CNS3xxx (where there is some martinal interest in the Gateworks Laguna platform)
- Similar for OXNAS
- There used to be the Realview MPCore tile - I haven't turned that on in ages, and it may be that the 3V cell that backs up the encryption keys is dead so it may not even boot.

I have this machine with 4 x ARM11 MPCore, it works like a charm. I use it to test exactly this kind of stuff, I know if a kernel works on ARM11MPCore it works on anything because of how fragile it is.

Quoted 4 lines

So it seems to come down to a question about CNS3xxx and OXNAS. If these aren't being used, maybe we can drop ARM11MPCore support and the associated platforms?

Quoted 1 line

Linus, Krzysztof, Neil, any input?

I don't especially need to keep the ARM11MPCore machine alive, it is just a testchip after all. The Oxnas is another story, that has wide deployment and was contributed recently (2016) and has excellent support in OpenWrt so I wouldn't really want
to axe that.

Quoted 4 lines · Russell King (Oracle)

So it seems to come down to a question about CNS3xxx and OXNAS. If these aren't being used, maybe we can drop ARM11MPCore support and the associated platforms?

Quoted 1 line

Linus, Krzysztof, Neil, any input?

I don't especially need to keep the ARM11MPCore machine alive, it is just a testchip after all. The Oxnas is another story, that has wide deployment and was contributed recently (2016) and has excellent support in OpenWrt so I wouldn't really want to axe that.

On arm32, the only way to safely flush icache from usermode is to call cacheflush(2). This also handles any required pipeline flushes, so membarrier's SYNC_CORE feature is useless on arm. Remove it.

Cc Mathieu Desnoyers <[email protected]>

Cc Nicholas Piggin <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Russell King <[email protected]>

Cc [email protected]

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

---
 arch/arm/Kconfig | 1 -
 1 file changed, 1 deletion(-)

arch/arm/Kconfig +0 -1

index 24804f11302d..89a885fba724 100644
@@ -10,7 +10,6 @@ config ARM
 	select ARCH_HAS_FORTIFY_SOURCE
 	select ARCH_HAS_KEEPINITRD
 	select ARCH_HAS_KCOV
-	select ARCH_HAS_MEMBARRIER_SYNC_CORE
 	select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
 	select ARCH_HAS_PTE_SPECIAL if ARM_LPAE
 	select ARCH_HAS_PHYS_TO_DMA

Signature · 1 line

2.31.1

Quoted 3 lines · Andy Lutomirski

On arm32, the only way to safely flush icache from usermode is to call cacheflush(2). This also handles any required pipeline flushes, so membarrier's SYNC_CORE feature is useless on arm. Remove it.

Unfortunately, it's a bit more complicated than that, and these days SYNC_CORE is equally necessary on arm as on arm64. This is something that changed in the architecture over time, but since ARMv7 we generally need both the cache maintenance *and* a context synchronization event (the latter must occur on the CPU which will execute the instructions).

If you look at the latest ARMv7-AR manual (ARM DDI 406C.d), section A3.5.4 "Concurrent modification and execution of instructions" covers this. That manual can be found at:

https://developer.arm.com/documentation/ddi0406/latest/

Quoted 11 lines · Mark Rutland

Quoted 3 lines · Andy Lutomirski

On arm32, the only way to safely flush icache from usermode is to call cacheflush(2). This also handles any required pipeline flushes, so membarrier's SYNC_CORE feature is useless on arm. Remove it.

Unfortunately, it's a bit more complicated than that, and these days SYNC_CORE is equally necessary on arm as on arm64. This is something that changed in the architecture over time, but since ARMv7 we generally need both the cache maintenance *and* a context synchronization event (the latter must occur on the CPU which will execute the instructions).

Quoted 5 lines

If you look at the latest ARMv7-AR manual (ARM DDI 406C.d), section A3.5.4 "Concurrent modification and execution of instructions" covers this. That manual can be found at:

https://developer.arm.com/documentation/ddi0406/latest/

Looking at that, sys_cacheflush() meets this. The manual details a series of cache maintenance calls in "step 1" that the modifying thread must issue - this is exactly what sys_cacheflush() does. The same is true for ARMv6, except the "ISB" terminology is replaced by a "PrefetchFlush" terminology. (I checked DDI0100I).

"step 2" requires an ISB on the "other CPU" prior to executing that code. As I understand it, in ARMv7, userspace can issue an ISB itself.

For ARMv6K, it doesn't have ISB, but instead has a CP15 instruction for this that isn't availble to userspace. This is where we come to the situation about ARM 11MPCore, and whether we continue to support it or not.

So, I think we're completely fine with ARMv7 under 32-bit ARM kernels as userspace has everything that's required. ARMv6K is a different matter as we've already identified for several reasons.

Signature · 3 lines

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Quoted 27 lines · Russell King (Oracle)

Quoted 11 lines · Mark Rutland

Quoted 3 lines · Andy Lutomirski

On arm32, the only way to safely flush icache from usermode is to call cacheflush(2). This also handles any required pipeline flushes, so membarrier's SYNC_CORE feature is useless on arm. Remove it.

Unfortunately, it's a bit more complicated than that, and these days SYNC_CORE is equally necessary on arm as on arm64. This is something that changed in the architecture over time, but since ARMv7 we generally need both the cache maintenance *and* a context synchronization event (the latter must occur on the CPU which will execute the instructions).

Quoted 5 lines

If you look at the latest ARMv7-AR manual (ARM DDI 406C.d), section A3.5.4 "Concurrent modification and execution of instructions" covers this. That manual can be found at:

https://developer.arm.com/documentation/ddi0406/latest/

Looking at that, sys_cacheflush() meets this. The manual details a series of cache maintenance calls in "step 1" that the modifying thread must issue - this is exactly what sys_cacheflush() does. The same is true for ARMv6, except the "ISB" terminology is replaced by a "PrefetchFlush" terminology. (I checked DDI0100I).

"step 2" requires an ISB on the "other CPU" prior to executing that code. As I understand it, in ARMv7, userspace can issue an ISB itself.

Quoted 8 lines

For ARMv6K, it doesn't have ISB, but instead has a CP15 instruction for this that isn't availble to userspace. This is where we come to the situation about ARM 11MPCore, and whether we continue to support it or not.

So, I think we're completely fine with ARMv7 under 32-bit ARM kernels as userspace has everything that's required. ARMv6K is a different matter as we've already identified for several reasons.

Sure, and I agree we should not change cacheflush().

The point of membarrier(SYNC_CORE) is that you can move the cost of that ISB out of the fast-path in the executing thread(s) and into the slow-path on the thread which generated the code.

So e.g. rather than an executing thread always having to do:

	LDR	<reg>, [<funcptr>]
	ISB	// in case funcptr was just updated
	BLR	<reg>

... you have the thread generating the code use membarrier(SYNC_CORE) prior to plublishing the funcptr, and the fast-path on all the executing threads can be:

	LDR	<reg> [<funcptr>]
	BLR	<reg>

... and thus I think we still want membarrier(SYNC_CORE) so that people can do this, even if there are other means to achieve the same functionality.

Quoted 53 lines · Mark Rutland

Quoted 27 lines · Russell King (Oracle)

Quoted 11 lines · Mark Rutland

Quoted 3 lines · Andy Lutomirski

On arm32, the only way to safely flush icache from usermode is to call cacheflush(2). This also handles any required pipeline flushes, so membarrier's SYNC_CORE feature is useless on arm. Remove it.

Unfortunately, it's a bit more complicated than that, and these days SYNC_CORE is equally necessary on arm as on arm64. This is something that changed in the architecture over time, but since ARMv7 we generally need both the cache maintenance *and* a context synchronization event (the latter must occur on the CPU which will execute the instructions).

Quoted 5 lines

If you look at the latest ARMv7-AR manual (ARM DDI 406C.d), section A3.5.4 "Concurrent modification and execution of instructions" covers this. That manual can be found at:

https://developer.arm.com/documentation/ddi0406/latest/

Looking at that, sys_cacheflush() meets this. The manual details a series of cache maintenance calls in "step 1" that the modifying thread must issue - this is exactly what sys_cacheflush() does. The same is true for ARMv6, except the "ISB" terminology is replaced by a "PrefetchFlush" terminology. (I checked DDI0100I).

"step 2" requires an ISB on the "other CPU" prior to executing that code. As I understand it, in ARMv7, userspace can issue an ISB itself.

Quoted 8 lines

For ARMv6K, it doesn't have ISB, but instead has a CP15 instruction for this that isn't availble to userspace. This is where we come to the situation about ARM 11MPCore, and whether we continue to support it or not.

So, I think we're completely fine with ARMv7 under 32-bit ARM kernels as userspace has everything that's required. ARMv6K is a different matter as we've already identified for several reasons.

Sure, and I agree we should not change cacheflush().

The point of membarrier(SYNC_CORE) is that you can move the cost of that ISB out of the fast-path in the executing thread(s) and into the slow-path on the thread which generated the code.

So e.g. rather than an executing thread always having to do:

	LDR	<reg>, [<funcptr>]
	ISB	// in case funcptr was just updated
	BLR	<reg>

... you have the thread generating the code use membarrier(SYNC_CORE) prior to plublishing the funcptr, and the fast-path on all the executing threads can be:

Quoted 6 lines

	LDR	<reg> [<funcptr>]
	BLR	<reg>

... and thus I think we still want membarrier(SYNC_CORE) so that people can do this, even if there are other means to achieve the same functionality.

I had the impression that sys_cacheflush() did that. Am I wrong?

In any event, I’m even more convinced that no new SYNC_CORE arches should be added. We need a new API that just does the right thing.

Quoted 2 lines · Andy Lutomirski

In any event, I’m even more convinced that no new SYNC_CORE arches should be added. We need a new API that just does the right thing.

My intuition is the other way around, and that this is a gnereally useful thing for architectures that require context synchronization.

It's not clear to me what "the right thing" would mean specifically, and on architectures with userspace cache maintenance JITs can usually do the most optimal maintenance, and only need help for the context synchronization.

Quoted 1 line · Mark Rutland

On Thu, Jun 17, 2021 at 06:41:41AM -0700, Andy Lutomirski wrote:

Quoted 5 lines

Quoted 2 lines

In any event, I’m even more convinced that no new SYNC_CORE arches should be added. We need a new API that just does the right thing.

My intuition is the other way around, and that this is a gnereally useful thing for architectures that require context synchronization.

Except that you can't use it in a generic way. You have to know the specific rules for your arch.