Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1698332 > unrolled thread

[RFC PATCH v2] membarrier: expedited private command

Started byMathieu Desnoyers <mathieu.desnoyers@efficios.com>
First post2017-07-27 23:20 +0200
Last post2017-07-28 17:40 +0200
Articles 16 — 4 participants

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH v2] membarrier: expedited private command Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2017-07-27 23:20 +0200
    Re: [RFC PATCH v2] membarrier: expedited private command "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-07-28 00:20 +0200
      Re: [RFC PATCH v2] membarrier: expedited private command Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2017-07-28 00:40 +0200
        Re: [RFC PATCH v2] membarrier: expedited private command "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-07-28 01:00 +0200
    Re: [RFC PATCH v2] membarrier: expedited private command Peter Zijlstra <peterz@infradead.org> - 2017-07-28 11:00 +0200
      Re: [RFC PATCH v2] membarrier: expedited private command Peter Zijlstra <peterz@infradead.org> - 2017-07-28 13:20 +0200
      Re: [RFC PATCH v2] membarrier: expedited private command Peter Zijlstra <peterz@infradead.org> - 2017-07-28 14:00 +0200
        Re: [RFC PATCH v2] membarrier: expedited private command Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2017-07-28 17:40 +0200
          Re: [RFC PATCH v2] membarrier: expedited private command Peter Zijlstra <peterz@infradead.org> - 2017-07-28 18:50 +0200
            Re: [RFC PATCH v2] membarrier: expedited private command Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2017-07-28 19:10 +0200
              Re: [RFC PATCH v2] membarrier: expedited private command Nicholas Piggin <npiggin@gmail.com> - 2017-07-29 04:00 +0200
                Re: [RFC PATCH v2] membarrier: expedited private command Peter Zijlstra <peterz@infradead.org> - 2017-07-29 11:30 +0200
                  Re: [RFC PATCH v2] membarrier: expedited private command Nicholas Piggin <npiggin@gmail.com> - 2017-07-29 11:50 +0200
                    Re: [RFC PATCH v2] membarrier: expedited private command Peter Zijlstra <peterz@infradead.org> - 2017-07-29 13:00 +0200
                  Re: [RFC PATCH v2] membarrier: expedited private command Nicholas Piggin <npiggin@gmail.com> - 2017-07-29 11:50 +0200
      Re: [RFC PATCH v2] membarrier: expedited private command Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2017-07-28 17:40 +0200

#1698332 — [RFC PATCH v2] membarrier: expedited private command

FromMathieu Desnoyers <mathieu.desnoyers@efficios.com>
Date2017-07-27 23:20 +0200
Subject[RFC PATCH v2] membarrier: expedited private command
Message-ID<u7Ymm-2WL-29@gated-at.bofh.it>
Implement MEMBARRIER_CMD_PRIVATE_EXPEDITED with IPIs using cpumask built
from all runqueues for which current thread's mm is the same as the
thread calling sys_membarrier.

Scheduler-wise, it requires that we add a memory barrier after context
switching between processes (which have different mm). Interestingly,
there is already a memory barrier in mmdrop(), so we only need to add
a barrier when switching from a kernel thread to a userspace thread.
We also don't need to add the barrier when switching to a kernel thread,
because it has no userspace memory mapping, which makes ordering of
user-space memory accesses pretty much useless.

* Benchmark

A stress-test benchmark of sched pipe shows that it does not add
significant overhead to the scheduler switching between processes:

100 runs of:

taskset 01 ./perf bench sched pipe

Running 'sched/pipe' benchmark:
Executed 1000000 pipe operations between two processes

Hardware: CPU: Intel(R) Xeon(R) CPU E5-2630 v3 @ 2.40GHz

A) With 4.13.0-rc2+
   at commit a97fb594bc7d ("virtio-net: fix module unloading")

avg.:     2.923 usecs/op
std.dev:  0.057 usecs/op

B) With this commit:

avg.:     2.916 usecs/op
std.dev:  0.043 usecs/op

Changes since v1:
- move membarrier code under kernel/sched/ because it uses the
  scheduler runqueue,
- only add the barrier when we switch from a kernel thread. The case
  where we switch from a user-space thread is already handled by
  the atomic_dec_and_test() in mmdrop().
- add a comment to mmdrop() documenting the requirement on the implicit
  memory barrier.

CC: Peter Zijlstra <peterz@infradead.org>
CC: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
CC: Boqun Feng <boqun.feng@gmail.com>
CC: Andrew Hunter <ahh@google.com>
CC: Maged Michael <maged.michael@gmail.com>
CC: gromer@google.com
CC: Avi Kivity <avi@scylladb.com>
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
---
 MAINTAINERS                     |  2 +-
 include/linux/sched/mm.h        |  5 +++
 include/uapi/linux/membarrier.h | 23 +++++++++++--
 kernel/Makefile                 |  1 -
 kernel/sched/Makefile           |  1 +
 kernel/sched/core.c             | 27 ++++++++++++++++
 kernel/{ => sched}/membarrier.c | 72 ++++++++++++++++++++++++++++++++++++++++-
 7 files changed, 126 insertions(+), 5 deletions(-)
 rename kernel/{ => sched}/membarrier.c (59%)

diff --git a/MAINTAINERS b/MAINTAINERS
index f66488dfdbc9..3b035584272f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8621,7 +8621,7 @@ M:	Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
 M:	"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
 L:	linux-kernel@vger.kernel.org
 S:	Supported
-F:	kernel/membarrier.c
+F:	kernel/sched/membarrier.c
 F:	include/uapi/linux/membarrier.h
 
 MEMORY MANAGEMENT
diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h
index 2b24a6974847..5c5384d9ae0f 100644
--- a/include/linux/sched/mm.h
+++ b/include/linux/sched/mm.h
@@ -38,6 +38,11 @@ static inline void mmgrab(struct mm_struct *mm)
 extern void __mmdrop(struct mm_struct *);
 static inline void mmdrop(struct mm_struct *mm)
 {
+	/*
+	 * Implicit full memory barrier provided by
+	 * atomic_dec_and_test() is required by membarrier. See comments
+	 * around membarrier_expedited_mb_after_set_current().
+	 */
 	if (unlikely(atomic_dec_and_test(&mm->mm_count)))
 		__mmdrop(mm);
 }
diff --git a/include/uapi/linux/membarrier.h b/include/uapi/linux/membarrier.h
index e0b108bd2624..6d47b3249d8a 100644
--- a/include/uapi/linux/membarrier.h
+++ b/include/uapi/linux/membarrier.h
@@ -40,14 +40,33 @@
  *                          (non-running threads are de facto in such a
  *                          state). This covers threads from all processes
  *                          running on the system. This command returns 0.
+ * @MEMBARRIER_CMD_PRIVATE_EXPEDITED:
+ *                          Execute a memory barrier on each running
+ *                          thread belonging to the same process as the current
+ *                          thread. Upon return from system call, the
+ *                          caller thread is ensured that all its running
+ *                          threads siblings have passed through a state
+ *                          where all memory accesses to user-space
+ *                          addresses match program order between entry
+ *                          to and return from the system call
+ *                          (non-running threads are de facto in such a
+ *                          state). This only covers threads from the
+ *                          same processes as the caller thread. This
+ *                          command returns 0. The "expedited" commands
+ *                          complete faster than the non-expedited ones,
+ *                          they never block, but have the downside of
+ *                          causing extra overhead.
  *
  * Command to be passed to the membarrier system call. The commands need to
  * be a single bit each, except for MEMBARRIER_CMD_QUERY which is assigned to
  * the value 0.
  */
 enum membarrier_cmd {
-	MEMBARRIER_CMD_QUERY = 0,
-	MEMBARRIER_CMD_SHARED = (1 << 0),
+	MEMBARRIER_CMD_QUERY			= 0,
+	MEMBARRIER_CMD_SHARED			= (1 << 0),
+	/* reserved for MEMBARRIER_CMD_SHARED_EXPEDITED (1 << 1) */
+	/* reserved for MEMBARRIER_CMD_PRIVATE (1 << 2) */
+	MEMBARRIER_CMD_PRIVATE_EXPEDITED	= (1 << 3),
 };
 
 #endif /* _UAPI_LINUX_MEMBARRIER_H */
diff --git a/kernel/Makefile b/kernel/Makefile
index 4cb8e8b23c6e..9c323a6daa46 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -108,7 +108,6 @@ obj-$(CONFIG_CRASH_DUMP) += crash_dump.o
 obj-$(CONFIG_JUMP_LABEL) += jump_label.o
 obj-$(CONFIG_CONTEXT_TRACKING) += context_tracking.o
 obj-$(CONFIG_TORTURE_TEST) += torture.o
-obj-$(CONFIG_MEMBARRIER) += membarrier.o
 
 obj-$(CONFIG_HAS_IOMEM) += memremap.o
 
diff --git a/kernel/sched/Makefile b/kernel/sched/Makefile
index 53f0164ed362..78f54932ea1d 100644
--- a/kernel/sched/Makefile
+++ b/kernel/sched/Makefile
@@ -25,3 +25,4 @@ obj-$(CONFIG_SCHED_DEBUG) += debug.o
 obj-$(CONFIG_CGROUP_CPUACCT) += cpuacct.o
 obj-$(CONFIG_CPU_FREQ) += cpufreq.o
 obj-$(CONFIG_CPU_FREQ_GOV_SCHEDUTIL) += cpufreq_schedutil.o
+obj-$(CONFIG_MEMBARRIER) += membarrier.o
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 17c667b427b4..01e3b881ab3a 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2724,6 +2724,32 @@ asmlinkage __visible void schedule_tail(struct task_struct *prev)
 		put_user(task_pid_vnr(current), current->set_child_tid);
 }
 
+static void membarrier_expedited_mb_after_set_current(struct mm_struct *mm,
+		struct mm_struct *oldmm)
+{
+	if (!IS_ENABLED(CONFIG_MEMBARRIER))
+		return;
+	/*
+	 * __schedule()->
+	 *   finish_task_switch()->
+	 *    if (mm)
+	 *      mmdrop(mm) ->
+	 *        atomic_dec_and_test()
+	 * takes care of issuing a memory barrier when oldmm is
+	 * non-NULL. We also don't need the barrier when switching to a
+	 * kernel thread, nor when we switch between threads belonging
+	 * to the same process.
+	 */
+	if (likely(oldmm || !mm || mm == oldmm))
+		return;
+	/*
+	 * When switching between processes, membarrier expedited
+	 * private requires a memory barrier after we set the current
+	 * task.
+	 */
+	smp_mb();
+}
+
 /*
  * context_switch - switch to the new MM and the new thread's register state.
  */
@@ -2737,6 +2763,7 @@ context_switch(struct rq *rq, struct task_struct *prev,
 
 	mm = next->mm;
 	oldmm = prev->active_mm;
+	membarrier_expedited_mb_after_set_current(mm, oldmm);
 	/*
 	 * For paravirt, this is coupled with an exit in switch_to to
 	 * combine the page table reload and the switch backend into
diff --git a/kernel/membarrier.c b/kernel/sched/membarrier.c
similarity index 59%
rename from kernel/membarrier.c
rename to kernel/sched/membarrier.c
index 9f9284f37f8d..f80828b0b607 100644
--- a/kernel/membarrier.c
+++ b/kernel/sched/membarrier.c
@@ -17,12 +17,79 @@
 #include <linux/syscalls.h>
 #include <linux/membarrier.h>
 #include <linux/tick.h>
+#include <linux/cpumask.h>
+
+#include "sched.h"	/* for cpu_rq(). */
 
 /*
  * Bitmask made from a "or" of all commands within enum membarrier_cmd,
  * except MEMBARRIER_CMD_QUERY.
  */
-#define MEMBARRIER_CMD_BITMASK	(MEMBARRIER_CMD_SHARED)
+#define MEMBARRIER_CMD_BITMASK	\
+	(MEMBARRIER_CMD_SHARED | MEMBARRIER_CMD_PRIVATE_EXPEDITED)
+
+static void ipi_mb(void *info)
+{
+	smp_mb();	/* IPIs should be serializing but paranoid. */
+}
+
+static void membarrier_private_expedited(void)
+{
+	int cpu, this_cpu;
+	bool fallback = false;
+	cpumask_var_t tmpmask;
+
+	if (num_online_cpus() == 1)
+		return;
+
+	/*
+	 * Matches memory barriers around rq->curr modification in
+	 * scheduler.
+	 */
+	smp_mb();	/* system call entry is not a mb. */
+
+	if (!alloc_cpumask_var(&tmpmask, GFP_NOWAIT)) {
+		/* Fallback for OOM. */
+		fallback = true;
+	}
+
+	/*
+	 * Skipping the current CPU is OK even through we can be
+	 * migrated at any point. The current CPU, at the point where we
+	 * read raw_smp_processor_id(), is ensured to be in program
+	 * order with respect to the caller thread. Therefore, we can
+	 * skip this CPU from the iteration.
+	 */
+	this_cpu = raw_smp_processor_id();
+	cpus_read_lock();
+	for_each_online_cpu(cpu) {
+		struct task_struct *p;
+
+		if (cpu == this_cpu)
+			continue;
+		rcu_read_lock();
+		p = task_rcu_dereference(&cpu_rq(cpu)->curr);
+		if (p && p->mm == current->mm) {
+			if (!fallback)
+				__cpumask_set_cpu(cpu, tmpmask);
+			else
+				smp_call_function_single(cpu, ipi_mb, NULL, 1);
+		}
+		rcu_read_unlock();
+	}
+	cpus_read_unlock();
+	if (!fallback) {
+		smp_call_function_many(tmpmask, ipi_mb, NULL, 1);
+		free_cpumask_var(tmpmask);
+	}
+
+	/*
+	 * Memory barrier on the caller thread _after_ we finished
+	 * waiting for the last IPI. Matches memory barriers around
+	 * rq->curr modification in scheduler.
+	 */
+	smp_mb();	/* exit from system call is not a mb */
+}
 
 /**
  * sys_membarrier - issue memory barriers on a set of threads
@@ -64,6 +131,9 @@ SYSCALL_DEFINE2(membarrier, int, cmd, int, flags)
 		if (num_online_cpus() > 1)
 			synchronize_sched();
 		return 0;
+	case MEMBARRIER_CMD_PRIVATE_EXPEDITED:
+		membarrier_private_expedited();
+		return 0;
 	default:
 		return -EINVAL;
 	}
-- 
2.11.0

[toc] | [next] | [standalone]

#1698349

From"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date2017-07-28 00:20 +0200
Message-ID<u7Ziq-3zP-7@gated-at.bofh.it>
In reply to#1698332
On Thu, Jul 27, 2017 at 05:13:14PM -0400, Mathieu Desnoyers wrote:
> Implement MEMBARRIER_CMD_PRIVATE_EXPEDITED with IPIs using cpumask built
> from all runqueues for which current thread's mm is the same as the
> thread calling sys_membarrier.
> 
> Scheduler-wise, it requires that we add a memory barrier after context
> switching between processes (which have different mm). Interestingly,
> there is already a memory barrier in mmdrop(), so we only need to add
> a barrier when switching from a kernel thread to a userspace thread.
> We also don't need to add the barrier when switching to a kernel thread,
> because it has no userspace memory mapping, which makes ordering of
> user-space memory accesses pretty much useless.
> 
> * Benchmark
> 
> A stress-test benchmark of sched pipe shows that it does not add
> significant overhead to the scheduler switching between processes:
> 
> 100 runs of:
> 
> taskset 01 ./perf bench sched pipe
> 
> Running 'sched/pipe' benchmark:
> Executed 1000000 pipe operations between two processes
> 
> Hardware: CPU: Intel(R) Xeon(R) CPU E5-2630 v3 @ 2.40GHz
> 
> A) With 4.13.0-rc2+
>    at commit a97fb594bc7d ("virtio-net: fix module unloading")
> 
> avg.:     2.923 usecs/op
> std.dev:  0.057 usecs/op
> 
> B) With this commit:
> 
> avg.:     2.916 usecs/op
> std.dev:  0.043 usecs/op
> 
> Changes since v1:
> - move membarrier code under kernel/sched/ because it uses the
>   scheduler runqueue,
> - only add the barrier when we switch from a kernel thread. The case
>   where we switch from a user-space thread is already handled by
>   the atomic_dec_and_test() in mmdrop().
> - add a comment to mmdrop() documenting the requirement on the implicit
>   memory barrier.
> 
> CC: Peter Zijlstra <peterz@infradead.org>
> CC: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> CC: Boqun Feng <boqun.feng@gmail.com>
> CC: Andrew Hunter <ahh@google.com>
> CC: Maged Michael <maged.michael@gmail.com>
> CC: gromer@google.com
> CC: Avi Kivity <avi@scylladb.com>
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

Looks much better, thank you!

I have queued this in place of my earlier patch for the moment.  If there
are no objections, I will push this into the upcoming v4.14 merge window.
If someone else wants to push it into v4.14, I am of course fine with
that, and am happy to give it a reviewed-by along the way.  But if there
are objections to your patch (suitably modified based on additional
review and testing, of course) going into v4.14, I can always fall back
to pushing my earlier simpler but less housebroken patch.  ;-)

							Thanx, Paul

> ---
>  MAINTAINERS                     |  2 +-
>  include/linux/sched/mm.h        |  5 +++
>  include/uapi/linux/membarrier.h | 23 +++++++++++--
>  kernel/Makefile                 |  1 -
>  kernel/sched/Makefile           |  1 +
>  kernel/sched/core.c             | 27 ++++++++++++++++
>  kernel/{ => sched}/membarrier.c | 72 ++++++++++++++++++++++++++++++++++++++++-
>  7 files changed, 126 insertions(+), 5 deletions(-)
>  rename kernel/{ => sched}/membarrier.c (59%)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f66488dfdbc9..3b035584272f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8621,7 +8621,7 @@ M:	Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>  M:	"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
>  L:	linux-kernel@vger.kernel.org
>  S:	Supported
> -F:	kernel/membarrier.c
> +F:	kernel/sched/membarrier.c
>  F:	include/uapi/linux/membarrier.h
> 
>  MEMORY MANAGEMENT
> diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h
> index 2b24a6974847..5c5384d9ae0f 100644
> --- a/include/linux/sched/mm.h
> +++ b/include/linux/sched/mm.h
> @@ -38,6 +38,11 @@ static inline void mmgrab(struct mm_struct *mm)
>  extern void __mmdrop(struct mm_struct *);
>  static inline void mmdrop(struct mm_struct *mm)
>  {
> +	/*
> +	 * Implicit full memory barrier provided by
> +	 * atomic_dec_and_test() is required by membarrier. See comments
> +	 * around membarrier_expedited_mb_after_set_current().
> +	 */
>  	if (unlikely(atomic_dec_and_test(&mm->mm_count)))
>  		__mmdrop(mm);
>  }
> diff --git a/include/uapi/linux/membarrier.h b/include/uapi/linux/membarrier.h
> index e0b108bd2624..6d47b3249d8a 100644
> --- a/include/uapi/linux/membarrier.h
> +++ b/include/uapi/linux/membarrier.h
> @@ -40,14 +40,33 @@
>   *                          (non-running threads are de facto in such a
>   *                          state). This covers threads from all processes
>   *                          running on the system. This command returns 0.
> + * @MEMBARRIER_CMD_PRIVATE_EXPEDITED:
> + *                          Execute a memory barrier on each running
> + *                          thread belonging to the same process as the current
> + *                          thread. Upon return from system call, the
> + *                          caller thread is ensured that all its running
> + *                          threads siblings have passed through a state
> + *                          where all memory accesses to user-space
> + *                          addresses match program order between entry
> + *                          to and return from the system call
> + *                          (non-running threads are de facto in such a
> + *                          state). This only covers threads from the
> + *                          same processes as the caller thread. This
> + *                          command returns 0. The "expedited" commands
> + *                          complete faster than the non-expedited ones,
> + *                          they never block, but have the downside of
> + *                          causing extra overhead.
>   *
>   * Command to be passed to the membarrier system call. The commands need to
>   * be a single bit each, except for MEMBARRIER_CMD_QUERY which is assigned to
>   * the value 0.
>   */
>  enum membarrier_cmd {
> -	MEMBARRIER_CMD_QUERY = 0,
> -	MEMBARRIER_CMD_SHARED = (1 << 0),
> +	MEMBARRIER_CMD_QUERY			= 0,
> +	MEMBARRIER_CMD_SHARED			= (1 << 0),
> +	/* reserved for MEMBARRIER_CMD_SHARED_EXPEDITED (1 << 1) */
> +	/* reserved for MEMBARRIER_CMD_PRIVATE (1 << 2) */
> +	MEMBARRIER_CMD_PRIVATE_EXPEDITED	= (1 << 3),
>  };
> 
>  #endif /* _UAPI_LINUX_MEMBARRIER_H */
> diff --git a/kernel/Makefile b/kernel/Makefile
> index 4cb8e8b23c6e..9c323a6daa46 100644
> --- a/kernel/Makefile
> +++ b/kernel/Makefile
> @@ -108,7 +108,6 @@ obj-$(CONFIG_CRASH_DUMP) += crash_dump.o
>  obj-$(CONFIG_JUMP_LABEL) += jump_label.o
>  obj-$(CONFIG_CONTEXT_TRACKING) += context_tracking.o
>  obj-$(CONFIG_TORTURE_TEST) += torture.o
> -obj-$(CONFIG_MEMBARRIER) += membarrier.o
> 
>  obj-$(CONFIG_HAS_IOMEM) += memremap.o
> 
> diff --git a/kernel/sched/Makefile b/kernel/sched/Makefile
> index 53f0164ed362..78f54932ea1d 100644
> --- a/kernel/sched/Makefile
> +++ b/kernel/sched/Makefile
> @@ -25,3 +25,4 @@ obj-$(CONFIG_SCHED_DEBUG) += debug.o
>  obj-$(CONFIG_CGROUP_CPUACCT) += cpuacct.o
>  obj-$(CONFIG_CPU_FREQ) += cpufreq.o
>  obj-$(CONFIG_CPU_FREQ_GOV_SCHEDUTIL) += cpufreq_schedutil.o
> +obj-$(CONFIG_MEMBARRIER) += membarrier.o
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 17c667b427b4..01e3b881ab3a 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -2724,6 +2724,32 @@ asmlinkage __visible void schedule_tail(struct task_struct *prev)
>  		put_user(task_pid_vnr(current), current->set_child_tid);
>  }
> 
> +static void membarrier_expedited_mb_after_set_current(struct mm_struct *mm,
> +		struct mm_struct *oldmm)
> +{
> +	if (!IS_ENABLED(CONFIG_MEMBARRIER))
> +		return;
> +	/*
> +	 * __schedule()->
> +	 *   finish_task_switch()->
> +	 *    if (mm)
> +	 *      mmdrop(mm) ->
> +	 *        atomic_dec_and_test()
> +	 * takes care of issuing a memory barrier when oldmm is
> +	 * non-NULL. We also don't need the barrier when switching to a
> +	 * kernel thread, nor when we switch between threads belonging
> +	 * to the same process.
> +	 */
> +	if (likely(oldmm || !mm || mm == oldmm))
> +		return;
> +	/*
> +	 * When switching between processes, membarrier expedited
> +	 * private requires a memory barrier after we set the current
> +	 * task.
> +	 */
> +	smp_mb();
> +}
> +
>  /*
>   * context_switch - switch to the new MM and the new thread's register state.
>   */
> @@ -2737,6 +2763,7 @@ context_switch(struct rq *rq, struct task_struct *prev,
> 
>  	mm = next->mm;
>  	oldmm = prev->active_mm;
> +	membarrier_expedited_mb_after_set_current(mm, oldmm);
>  	/*
>  	 * For paravirt, this is coupled with an exit in switch_to to
>  	 * combine the page table reload and the switch backend into
> diff --git a/kernel/membarrier.c b/kernel/sched/membarrier.c
> similarity index 59%
> rename from kernel/membarrier.c
> rename to kernel/sched/membarrier.c
> index 9f9284f37f8d..f80828b0b607 100644
> --- a/kernel/membarrier.c
> +++ b/kernel/sched/membarrier.c
> @@ -17,12 +17,79 @@
>  #include <linux/syscalls.h>
>  #include <linux/membarrier.h>
>  #include <linux/tick.h>
> +#include <linux/cpumask.h>
> +
> +#include "sched.h"	/* for cpu_rq(). */
> 
>  /*
>   * Bitmask made from a "or" of all commands within enum membarrier_cmd,
>   * except MEMBARRIER_CMD_QUERY.
>   */
> -#define MEMBARRIER_CMD_BITMASK	(MEMBARRIER_CMD_SHARED)
> +#define MEMBARRIER_CMD_BITMASK	\
> +	(MEMBARRIER_CMD_SHARED | MEMBARRIER_CMD_PRIVATE_EXPEDITED)
> +
> +static void ipi_mb(void *info)
> +{
> +	smp_mb();	/* IPIs should be serializing but paranoid. */
> +}
> +
> +static void membarrier_private_expedited(void)
> +{
> +	int cpu, this_cpu;
> +	bool fallback = false;
> +	cpumask_var_t tmpmask;
> +
> +	if (num_online_cpus() == 1)
> +		return;
> +
> +	/*
> +	 * Matches memory barriers around rq->curr modification in
> +	 * scheduler.
> +	 */
> +	smp_mb();	/* system call entry is not a mb. */
> +
> +	if (!alloc_cpumask_var(&tmpmask, GFP_NOWAIT)) {
> +		/* Fallback for OOM. */
> +		fallback = true;
> +	}
> +
> +	/*
> +	 * Skipping the current CPU is OK even through we can be
> +	 * migrated at any point. The current CPU, at the point where we
> +	 * read raw_smp_processor_id(), is ensured to be in program
> +	 * order with respect to the caller thread. Therefore, we can
> +	 * skip this CPU from the iteration.
> +	 */
> +	this_cpu = raw_smp_processor_id();
> +	cpus_read_lock();
> +	for_each_online_cpu(cpu) {
> +		struct task_struct *p;
> +
> +		if (cpu == this_cpu)
> +			continue;
> +		rcu_read_lock();
> +		p = task_rcu_dereference(&cpu_rq(cpu)->curr);
> +		if (p && p->mm == current->mm) {
> +			if (!fallback)
> +				__cpumask_set_cpu(cpu, tmpmask);
> +			else
> +				smp_call_function_single(cpu, ipi_mb, NULL, 1);
> +		}
> +		rcu_read_unlock();
> +	}
> +	cpus_read_unlock();
> +	if (!fallback) {
> +		smp_call_function_many(tmpmask, ipi_mb, NULL, 1);
> +		free_cpumask_var(tmpmask);
> +	}
> +
> +	/*
> +	 * Memory barrier on the caller thread _after_ we finished
> +	 * waiting for the last IPI. Matches memory barriers around
> +	 * rq->curr modification in scheduler.
> +	 */
> +	smp_mb();	/* exit from system call is not a mb */
> +}
> 
>  /**
>   * sys_membarrier - issue memory barriers on a set of threads
> @@ -64,6 +131,9 @@ SYSCALL_DEFINE2(membarrier, int, cmd, int, flags)
>  		if (num_online_cpus() > 1)
>  			synchronize_sched();
>  		return 0;
> +	case MEMBARRIER_CMD_PRIVATE_EXPEDITED:
> +		membarrier_private_expedited();
> +		return 0;
>  	default:
>  		return -EINVAL;
>  	}
> -- 
> 2.11.0
>

[toc] | [prev] | [next] | [standalone]

#1698353

FromMathieu Desnoyers <mathieu.desnoyers@efficios.com>
Date2017-07-28 00:40 +0200
Message-ID<u7ZBL-3Gi-13@gated-at.bofh.it>
In reply to#1698349
----- On Jul 27, 2017, at 6:13 PM, Paul E. McKenney paulmck@linux.vnet.ibm.com wrote:

> On Thu, Jul 27, 2017 at 05:13:14PM -0400, Mathieu Desnoyers wrote:
>> Implement MEMBARRIER_CMD_PRIVATE_EXPEDITED with IPIs using cpumask built
>> from all runqueues for which current thread's mm is the same as the
>> thread calling sys_membarrier.
>> 
>> Scheduler-wise, it requires that we add a memory barrier after context
>> switching between processes (which have different mm). Interestingly,
>> there is already a memory barrier in mmdrop(), so we only need to add
>> a barrier when switching from a kernel thread to a userspace thread.
>> We also don't need to add the barrier when switching to a kernel thread,
>> because it has no userspace memory mapping, which makes ordering of
>> user-space memory accesses pretty much useless.
>> 
>> * Benchmark
>> 
>> A stress-test benchmark of sched pipe shows that it does not add
>> significant overhead to the scheduler switching between processes:
>> 
>> 100 runs of:
>> 
>> taskset 01 ./perf bench sched pipe
>> 
>> Running 'sched/pipe' benchmark:
>> Executed 1000000 pipe operations between two processes
>> 
>> Hardware: CPU: Intel(R) Xeon(R) CPU E5-2630 v3 @ 2.40GHz
>> 
>> A) With 4.13.0-rc2+
>>    at commit a97fb594bc7d ("virtio-net: fix module unloading")
>> 
>> avg.:     2.923 usecs/op
>> std.dev:  0.057 usecs/op
>> 
>> B) With this commit:
>> 
>> avg.:     2.916 usecs/op
>> std.dev:  0.043 usecs/op
>> 
>> Changes since v1:
>> - move membarrier code under kernel/sched/ because it uses the
>>   scheduler runqueue,
>> - only add the barrier when we switch from a kernel thread. The case
>>   where we switch from a user-space thread is already handled by
>>   the atomic_dec_and_test() in mmdrop().
>> - add a comment to mmdrop() documenting the requirement on the implicit
>>   memory barrier.
>> 
>> CC: Peter Zijlstra <peterz@infradead.org>
>> CC: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>> CC: Boqun Feng <boqun.feng@gmail.com>
>> CC: Andrew Hunter <ahh@google.com>
>> CC: Maged Michael <maged.michael@gmail.com>
>> CC: gromer@google.com
>> CC: Avi Kivity <avi@scylladb.com>
>> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> 
> Looks much better, thank you!
> 
> I have queued this in place of my earlier patch for the moment.  If there
> are no objections, I will push this into the upcoming v4.14 merge window.
> If someone else wants to push it into v4.14, I am of course fine with
> that, and am happy to give it a reviewed-by along the way.  But if there
> are objections to your patch (suitably modified based on additional
> review and testing, of course) going into v4.14, I can always fall back
> to pushing my earlier simpler but less housebroken patch.  ;-)

I'm fine about you picking up this patch, even though it's tagged "RFC".
I'm sure concerns will have plenty of time to be voiced by others until
it reaches mainline anyway, at which point I'll address them and resubmit
new versions.

Thanks!

Mathieu

> 
>							Thanx, Paul
> 
>> ---
>>  MAINTAINERS                     |  2 +-
>>  include/linux/sched/mm.h        |  5 +++
>>  include/uapi/linux/membarrier.h | 23 +++++++++++--
>>  kernel/Makefile                 |  1 -
>>  kernel/sched/Makefile           |  1 +
>>  kernel/sched/core.c             | 27 ++++++++++++++++
>>  kernel/{ => sched}/membarrier.c | 72 ++++++++++++++++++++++++++++++++++++++++-
>>  7 files changed, 126 insertions(+), 5 deletions(-)
>>  rename kernel/{ => sched}/membarrier.c (59%)
>> 
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index f66488dfdbc9..3b035584272f 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -8621,7 +8621,7 @@ M:	Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>>  M:	"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
>>  L:	linux-kernel@vger.kernel.org
>>  S:	Supported
>> -F:	kernel/membarrier.c
>> +F:	kernel/sched/membarrier.c
>>  F:	include/uapi/linux/membarrier.h
>> 
>>  MEMORY MANAGEMENT
>> diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h
>> index 2b24a6974847..5c5384d9ae0f 100644
>> --- a/include/linux/sched/mm.h
>> +++ b/include/linux/sched/mm.h
>> @@ -38,6 +38,11 @@ static inline void mmgrab(struct mm_struct *mm)
>>  extern void __mmdrop(struct mm_struct *);
>>  static inline void mmdrop(struct mm_struct *mm)
>>  {
>> +	/*
>> +	 * Implicit full memory barrier provided by
>> +	 * atomic_dec_and_test() is required by membarrier. See comments
>> +	 * around membarrier_expedited_mb_after_set_current().
>> +	 */
>>  	if (unlikely(atomic_dec_and_test(&mm->mm_count)))
>>  		__mmdrop(mm);
>>  }
>> diff --git a/include/uapi/linux/membarrier.h b/include/uapi/linux/membarrier.h
>> index e0b108bd2624..6d47b3249d8a 100644
>> --- a/include/uapi/linux/membarrier.h
>> +++ b/include/uapi/linux/membarrier.h
>> @@ -40,14 +40,33 @@
>>   *                          (non-running threads are de facto in such a
>>   *                          state). This covers threads from all processes
>>   *                          running on the system. This command returns 0.
>> + * @MEMBARRIER_CMD_PRIVATE_EXPEDITED:
>> + *                          Execute a memory barrier on each running
>> + *                          thread belonging to the same process as the current
>> + *                          thread. Upon return from system call, the
>> + *                          caller thread is ensured that all its running
>> + *                          threads siblings have passed through a state
>> + *                          where all memory accesses to user-space
>> + *                          addresses match program order between entry
>> + *                          to and return from the system call
>> + *                          (non-running threads are de facto in such a
>> + *                          state). This only covers threads from the
>> + *                          same processes as the caller thread. This
>> + *                          command returns 0. The "expedited" commands
>> + *                          complete faster than the non-expedited ones,
>> + *                          they never block, but have the downside of
>> + *                          causing extra overhead.
>>   *
>>   * Command to be passed to the membarrier system call. The commands need to
>>   * be a single bit each, except for MEMBARRIER_CMD_QUERY which is assigned to
>>   * the value 0.
>>   */
>>  enum membarrier_cmd {
>> -	MEMBARRIER_CMD_QUERY = 0,
>> -	MEMBARRIER_CMD_SHARED = (1 << 0),
>> +	MEMBARRIER_CMD_QUERY			= 0,
>> +	MEMBARRIER_CMD_SHARED			= (1 << 0),
>> +	/* reserved for MEMBARRIER_CMD_SHARED_EXPEDITED (1 << 1) */
>> +	/* reserved for MEMBARRIER_CMD_PRIVATE (1 << 2) */
>> +	MEMBARRIER_CMD_PRIVATE_EXPEDITED	= (1 << 3),
>>  };
>> 
>>  #endif /* _UAPI_LINUX_MEMBARRIER_H */
>> diff --git a/kernel/Makefile b/kernel/Makefile
>> index 4cb8e8b23c6e..9c323a6daa46 100644
>> --- a/kernel/Makefile
>> +++ b/kernel/Makefile
>> @@ -108,7 +108,6 @@ obj-$(CONFIG_CRASH_DUMP) += crash_dump.o
>>  obj-$(CONFIG_JUMP_LABEL) += jump_label.o
>>  obj-$(CONFIG_CONTEXT_TRACKING) += context_tracking.o
>>  obj-$(CONFIG_TORTURE_TEST) += torture.o
>> -obj-$(CONFIG_MEMBARRIER) += membarrier.o
>> 
>>  obj-$(CONFIG_HAS_IOMEM) += memremap.o
>> 
>> diff --git a/kernel/sched/Makefile b/kernel/sched/Makefile
>> index 53f0164ed362..78f54932ea1d 100644
>> --- a/kernel/sched/Makefile
>> +++ b/kernel/sched/Makefile
>> @@ -25,3 +25,4 @@ obj-$(CONFIG_SCHED_DEBUG) += debug.o
>>  obj-$(CONFIG_CGROUP_CPUACCT) += cpuacct.o
>>  obj-$(CONFIG_CPU_FREQ) += cpufreq.o
>>  obj-$(CONFIG_CPU_FREQ_GOV_SCHEDUTIL) += cpufreq_schedutil.o
>> +obj-$(CONFIG_MEMBARRIER) += membarrier.o
>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>> index 17c667b427b4..01e3b881ab3a 100644
>> --- a/kernel/sched/core.c
>> +++ b/kernel/sched/core.c
>> @@ -2724,6 +2724,32 @@ asmlinkage __visible void schedule_tail(struct
>> task_struct *prev)
>>  		put_user(task_pid_vnr(current), current->set_child_tid);
>>  }
>> 
>> +static void membarrier_expedited_mb_after_set_current(struct mm_struct *mm,
>> +		struct mm_struct *oldmm)
>> +{
>> +	if (!IS_ENABLED(CONFIG_MEMBARRIER))
>> +		return;
>> +	/*
>> +	 * __schedule()->
>> +	 *   finish_task_switch()->
>> +	 *    if (mm)
>> +	 *      mmdrop(mm) ->
>> +	 *        atomic_dec_and_test()
>> +	 * takes care of issuing a memory barrier when oldmm is
>> +	 * non-NULL. We also don't need the barrier when switching to a
>> +	 * kernel thread, nor when we switch between threads belonging
>> +	 * to the same process.
>> +	 */
>> +	if (likely(oldmm || !mm || mm == oldmm))
>> +		return;
>> +	/*
>> +	 * When switching between processes, membarrier expedited
>> +	 * private requires a memory barrier after we set the current
>> +	 * task.
>> +	 */
>> +	smp_mb();
>> +}
>> +
>>  /*
>>   * context_switch - switch to the new MM and the new thread's register state.
>>   */
>> @@ -2737,6 +2763,7 @@ context_switch(struct rq *rq, struct task_struct *prev,
>> 
>>  	mm = next->mm;
>>  	oldmm = prev->active_mm;
>> +	membarrier_expedited_mb_after_set_current(mm, oldmm);
>>  	/*
>>  	 * For paravirt, this is coupled with an exit in switch_to to
>>  	 * combine the page table reload and the switch backend into
>> diff --git a/kernel/membarrier.c b/kernel/sched/membarrier.c
>> similarity index 59%
>> rename from kernel/membarrier.c
>> rename to kernel/sched/membarrier.c
>> index 9f9284f37f8d..f80828b0b607 100644
>> --- a/kernel/membarrier.c
>> +++ b/kernel/sched/membarrier.c
>> @@ -17,12 +17,79 @@
>>  #include <linux/syscalls.h>
>>  #include <linux/membarrier.h>
>>  #include <linux/tick.h>
>> +#include <linux/cpumask.h>
>> +
>> +#include "sched.h"	/* for cpu_rq(). */
>> 
>>  /*
>>   * Bitmask made from a "or" of all commands within enum membarrier_cmd,
>>   * except MEMBARRIER_CMD_QUERY.
>>   */
>> -#define MEMBARRIER_CMD_BITMASK	(MEMBARRIER_CMD_SHARED)
>> +#define MEMBARRIER_CMD_BITMASK	\
>> +	(MEMBARRIER_CMD_SHARED | MEMBARRIER_CMD_PRIVATE_EXPEDITED)
>> +
>> +static void ipi_mb(void *info)
>> +{
>> +	smp_mb();	/* IPIs should be serializing but paranoid. */
>> +}
>> +
>> +static void membarrier_private_expedited(void)
>> +{
>> +	int cpu, this_cpu;
>> +	bool fallback = false;
>> +	cpumask_var_t tmpmask;
>> +
>> +	if (num_online_cpus() == 1)
>> +		return;
>> +
>> +	/*
>> +	 * Matches memory barriers around rq->curr modification in
>> +	 * scheduler.
>> +	 */
>> +	smp_mb();	/* system call entry is not a mb. */
>> +
>> +	if (!alloc_cpumask_var(&tmpmask, GFP_NOWAIT)) {
>> +		/* Fallback for OOM. */
>> +		fallback = true;
>> +	}
>> +
>> +	/*
>> +	 * Skipping the current CPU is OK even through we can be
>> +	 * migrated at any point. The current CPU, at the point where we
>> +	 * read raw_smp_processor_id(), is ensured to be in program
>> +	 * order with respect to the caller thread. Therefore, we can
>> +	 * skip this CPU from the iteration.
>> +	 */
>> +	this_cpu = raw_smp_processor_id();
>> +	cpus_read_lock();
>> +	for_each_online_cpu(cpu) {
>> +		struct task_struct *p;
>> +
>> +		if (cpu == this_cpu)
>> +			continue;
>> +		rcu_read_lock();
>> +		p = task_rcu_dereference(&cpu_rq(cpu)->curr);
>> +		if (p && p->mm == current->mm) {
>> +			if (!fallback)
>> +				__cpumask_set_cpu(cpu, tmpmask);
>> +			else
>> +				smp_call_function_single(cpu, ipi_mb, NULL, 1);
>> +		}
>> +		rcu_read_unlock();
>> +	}
>> +	cpus_read_unlock();
>> +	if (!fallback) {
>> +		smp_call_function_many(tmpmask, ipi_mb, NULL, 1);
>> +		free_cpumask_var(tmpmask);
>> +	}
>> +
>> +	/*
>> +	 * Memory barrier on the caller thread _after_ we finished
>> +	 * waiting for the last IPI. Matches memory barriers around
>> +	 * rq->curr modification in scheduler.
>> +	 */
>> +	smp_mb();	/* exit from system call is not a mb */
>> +}
>> 
>>  /**
>>   * sys_membarrier - issue memory barriers on a set of threads
>> @@ -64,6 +131,9 @@ SYSCALL_DEFINE2(membarrier, int, cmd, int, flags)
>>  		if (num_online_cpus() > 1)
>>  			synchronize_sched();
>>  		return 0;
>> +	case MEMBARRIER_CMD_PRIVATE_EXPEDITED:
>> +		membarrier_private_expedited();
>> +		return 0;
>>  	default:
>>  		return -EINVAL;
>>  	}
>> --
>> 2.11.0

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

[toc] | [prev] | [next] | [standalone]

#1698357

From"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date2017-07-28 01:00 +0200
Message-ID<u7ZV8-3N8-7@gated-at.bofh.it>
In reply to#1698353
On Thu, Jul 27, 2017 at 10:41:25PM +0000, Mathieu Desnoyers wrote:
> ----- On Jul 27, 2017, at 6:13 PM, Paul E. McKenney paulmck@linux.vnet.ibm.com wrote:
> 
> > On Thu, Jul 27, 2017 at 05:13:14PM -0400, Mathieu Desnoyers wrote:
> >> Implement MEMBARRIER_CMD_PRIVATE_EXPEDITED with IPIs using cpumask built
> >> from all runqueues for which current thread's mm is the same as the
> >> thread calling sys_membarrier.
> >> 
> >> Scheduler-wise, it requires that we add a memory barrier after context
> >> switching between processes (which have different mm). Interestingly,
> >> there is already a memory barrier in mmdrop(), so we only need to add
> >> a barrier when switching from a kernel thread to a userspace thread.
> >> We also don't need to add the barrier when switching to a kernel thread,
> >> because it has no userspace memory mapping, which makes ordering of
> >> user-space memory accesses pretty much useless.
> >> 
> >> * Benchmark
> >> 
> >> A stress-test benchmark of sched pipe shows that it does not add
> >> significant overhead to the scheduler switching between processes:
> >> 
> >> 100 runs of:
> >> 
> >> taskset 01 ./perf bench sched pipe
> >> 
> >> Running 'sched/pipe' benchmark:
> >> Executed 1000000 pipe operations between two processes
> >> 
> >> Hardware: CPU: Intel(R) Xeon(R) CPU E5-2630 v3 @ 2.40GHz
> >> 
> >> A) With 4.13.0-rc2+
> >>    at commit a97fb594bc7d ("virtio-net: fix module unloading")
> >> 
> >> avg.:     2.923 usecs/op
> >> std.dev:  0.057 usecs/op
> >> 
> >> B) With this commit:
> >> 
> >> avg.:     2.916 usecs/op
> >> std.dev:  0.043 usecs/op
> >> 
> >> Changes since v1:
> >> - move membarrier code under kernel/sched/ because it uses the
> >>   scheduler runqueue,
> >> - only add the barrier when we switch from a kernel thread. The case
> >>   where we switch from a user-space thread is already handled by
> >>   the atomic_dec_and_test() in mmdrop().
> >> - add a comment to mmdrop() documenting the requirement on the implicit
> >>   memory barrier.
> >> 
> >> CC: Peter Zijlstra <peterz@infradead.org>
> >> CC: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> >> CC: Boqun Feng <boqun.feng@gmail.com>
> >> CC: Andrew Hunter <ahh@google.com>
> >> CC: Maged Michael <maged.michael@gmail.com>
> >> CC: gromer@google.com
> >> CC: Avi Kivity <avi@scylladb.com>
> >> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > 
> > Looks much better, thank you!
> > 
> > I have queued this in place of my earlier patch for the moment.  If there
> > are no objections, I will push this into the upcoming v4.14 merge window.
> > If someone else wants to push it into v4.14, I am of course fine with
> > that, and am happy to give it a reviewed-by along the way.  But if there
> > are objections to your patch (suitably modified based on additional
> > review and testing, of course) going into v4.14, I can always fall back
> > to pushing my earlier simpler but less housebroken patch.  ;-)
> 
> I'm fine about you picking up this patch, even though it's tagged "RFC".
> I'm sure concerns will have plenty of time to be voiced by others until
> it reaches mainline anyway, at which point I'll address them and resubmit
> new versions.

Works for me!  My -rcu tree is subject to rebasing, so I can easily
replace the current patch with an updated one.

							Thanx, Paul

> Thanks!
> 
> Mathieu
> 
> > 
> >							Thanx, Paul
> > 
> >> ---
> >>  MAINTAINERS                     |  2 +-
> >>  include/linux/sched/mm.h        |  5 +++
> >>  include/uapi/linux/membarrier.h | 23 +++++++++++--
> >>  kernel/Makefile                 |  1 -
> >>  kernel/sched/Makefile           |  1 +
> >>  kernel/sched/core.c             | 27 ++++++++++++++++
> >>  kernel/{ => sched}/membarrier.c | 72 ++++++++++++++++++++++++++++++++++++++++-
> >>  7 files changed, 126 insertions(+), 5 deletions(-)
> >>  rename kernel/{ => sched}/membarrier.c (59%)
> >> 
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index f66488dfdbc9..3b035584272f 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -8621,7 +8621,7 @@ M:	Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> >>  M:	"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> >>  L:	linux-kernel@vger.kernel.org
> >>  S:	Supported
> >> -F:	kernel/membarrier.c
> >> +F:	kernel/sched/membarrier.c
> >>  F:	include/uapi/linux/membarrier.h
> >> 
> >>  MEMORY MANAGEMENT
> >> diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h
> >> index 2b24a6974847..5c5384d9ae0f 100644
> >> --- a/include/linux/sched/mm.h
> >> +++ b/include/linux/sched/mm.h
> >> @@ -38,6 +38,11 @@ static inline void mmgrab(struct mm_struct *mm)
> >>  extern void __mmdrop(struct mm_struct *);
> >>  static inline void mmdrop(struct mm_struct *mm)
> >>  {
> >> +	/*
> >> +	 * Implicit full memory barrier provided by
> >> +	 * atomic_dec_and_test() is required by membarrier. See comments
> >> +	 * around membarrier_expedited_mb_after_set_current().
> >> +	 */
> >>  	if (unlikely(atomic_dec_and_test(&mm->mm_count)))
> >>  		__mmdrop(mm);
> >>  }
> >> diff --git a/include/uapi/linux/membarrier.h b/include/uapi/linux/membarrier.h
> >> index e0b108bd2624..6d47b3249d8a 100644
> >> --- a/include/uapi/linux/membarrier.h
> >> +++ b/include/uapi/linux/membarrier.h
> >> @@ -40,14 +40,33 @@
> >>   *                          (non-running threads are de facto in such a
> >>   *                          state). This covers threads from all processes
> >>   *                          running on the system. This command returns 0.
> >> + * @MEMBARRIER_CMD_PRIVATE_EXPEDITED:
> >> + *                          Execute a memory barrier on each running
> >> + *                          thread belonging to the same process as the current
> >> + *                          thread. Upon return from system call, the
> >> + *                          caller thread is ensured that all its running
> >> + *                          threads siblings have passed through a state
> >> + *                          where all memory accesses to user-space
> >> + *                          addresses match program order between entry
> >> + *                          to and return from the system call
> >> + *                          (non-running threads are de facto in such a
> >> + *                          state). This only covers threads from the
> >> + *                          same processes as the caller thread. This
> >> + *                          command returns 0. The "expedited" commands
> >> + *                          complete faster than the non-expedited ones,
> >> + *                          they never block, but have the downside of
> >> + *                          causing extra overhead.
> >>   *
> >>   * Command to be passed to the membarrier system call. The commands need to
> >>   * be a single bit each, except for MEMBARRIER_CMD_QUERY which is assigned to
> >>   * the value 0.
> >>   */
> >>  enum membarrier_cmd {
> >> -	MEMBARRIER_CMD_QUERY = 0,
> >> -	MEMBARRIER_CMD_SHARED = (1 << 0),
> >> +	MEMBARRIER_CMD_QUERY			= 0,
> >> +	MEMBARRIER_CMD_SHARED			= (1 << 0),
> >> +	/* reserved for MEMBARRIER_CMD_SHARED_EXPEDITED (1 << 1) */
> >> +	/* reserved for MEMBARRIER_CMD_PRIVATE (1 << 2) */
> >> +	MEMBARRIER_CMD_PRIVATE_EXPEDITED	= (1 << 3),
> >>  };
> >> 
> >>  #endif /* _UAPI_LINUX_MEMBARRIER_H */
> >> diff --git a/kernel/Makefile b/kernel/Makefile
> >> index 4cb8e8b23c6e..9c323a6daa46 100644
> >> --- a/kernel/Makefile
> >> +++ b/kernel/Makefile
> >> @@ -108,7 +108,6 @@ obj-$(CONFIG_CRASH_DUMP) += crash_dump.o
> >>  obj-$(CONFIG_JUMP_LABEL) += jump_label.o
> >>  obj-$(CONFIG_CONTEXT_TRACKING) += context_tracking.o
> >>  obj-$(CONFIG_TORTURE_TEST) += torture.o
> >> -obj-$(CONFIG_MEMBARRIER) += membarrier.o
> >> 
> >>  obj-$(CONFIG_HAS_IOMEM) += memremap.o
> >> 
> >> diff --git a/kernel/sched/Makefile b/kernel/sched/Makefile
> >> index 53f0164ed362..78f54932ea1d 100644
> >> --- a/kernel/sched/Makefile
> >> +++ b/kernel/sched/Makefile
> >> @@ -25,3 +25,4 @@ obj-$(CONFIG_SCHED_DEBUG) += debug.o
> >>  obj-$(CONFIG_CGROUP_CPUACCT) += cpuacct.o
> >>  obj-$(CONFIG_CPU_FREQ) += cpufreq.o
> >>  obj-$(CONFIG_CPU_FREQ_GOV_SCHEDUTIL) += cpufreq_schedutil.o
> >> +obj-$(CONFIG_MEMBARRIER) += membarrier.o
> >> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> >> index 17c667b427b4..01e3b881ab3a 100644
> >> --- a/kernel/sched/core.c
> >> +++ b/kernel/sched/core.c
> >> @@ -2724,6 +2724,32 @@ asmlinkage __visible void schedule_tail(struct
> >> task_struct *prev)
> >>  		put_user(task_pid_vnr(current), current->set_child_tid);
> >>  }
> >> 
> >> +static void membarrier_expedited_mb_after_set_current(struct mm_struct *mm,
> >> +		struct mm_struct *oldmm)
> >> +{
> >> +	if (!IS_ENABLED(CONFIG_MEMBARRIER))
> >> +		return;
> >> +	/*
> >> +	 * __schedule()->
> >> +	 *   finish_task_switch()->
> >> +	 *    if (mm)
> >> +	 *      mmdrop(mm) ->
> >> +	 *        atomic_dec_and_test()
> >> +	 * takes care of issuing a memory barrier when oldmm is
> >> +	 * non-NULL. We also don't need the barrier when switching to a
> >> +	 * kernel thread, nor when we switch between threads belonging
> >> +	 * to the same process.
> >> +	 */
> >> +	if (likely(oldmm || !mm || mm == oldmm))
> >> +		return;
> >> +	/*
> >> +	 * When switching between processes, membarrier expedited
> >> +	 * private requires a memory barrier after we set the current
> >> +	 * task.
> >> +	 */
> >> +	smp_mb();
> >> +}
> >> +
> >>  /*
> >>   * context_switch - switch to the new MM and the new thread's register state.
> >>   */
> >> @@ -2737,6 +2763,7 @@ context_switch(struct rq *rq, struct task_struct *prev,
> >> 
> >>  	mm = next->mm;
> >>  	oldmm = prev->active_mm;
> >> +	membarrier_expedited_mb_after_set_current(mm, oldmm);
> >>  	/*
> >>  	 * For paravirt, this is coupled with an exit in switch_to to
> >>  	 * combine the page table reload and the switch backend into
> >> diff --git a/kernel/membarrier.c b/kernel/sched/membarrier.c
> >> similarity index 59%
> >> rename from kernel/membarrier.c
> >> rename to kernel/sched/membarrier.c
> >> index 9f9284f37f8d..f80828b0b607 100644
> >> --- a/kernel/membarrier.c
> >> +++ b/kernel/sched/membarrier.c
> >> @@ -17,12 +17,79 @@
> >>  #include <linux/syscalls.h>
> >>  #include <linux/membarrier.h>
> >>  #include <linux/tick.h>
> >> +#include <linux/cpumask.h>
> >> +
> >> +#include "sched.h"	/* for cpu_rq(). */
> >> 
> >>  /*
> >>   * Bitmask made from a "or" of all commands within enum membarrier_cmd,
> >>   * except MEMBARRIER_CMD_QUERY.
> >>   */
> >> -#define MEMBARRIER_CMD_BITMASK	(MEMBARRIER_CMD_SHARED)
> >> +#define MEMBARRIER_CMD_BITMASK	\
> >> +	(MEMBARRIER_CMD_SHARED | MEMBARRIER_CMD_PRIVATE_EXPEDITED)
> >> +
> >> +static void ipi_mb(void *info)
> >> +{
> >> +	smp_mb();	/* IPIs should be serializing but paranoid. */
> >> +}
> >> +
> >> +static void membarrier_private_expedited(void)
> >> +{
> >> +	int cpu, this_cpu;
> >> +	bool fallback = false;
> >> +	cpumask_var_t tmpmask;
> >> +
> >> +	if (num_online_cpus() == 1)
> >> +		return;
> >> +
> >> +	/*
> >> +	 * Matches memory barriers around rq->curr modification in
> >> +	 * scheduler.
> >> +	 */
> >> +	smp_mb();	/* system call entry is not a mb. */
> >> +
> >> +	if (!alloc_cpumask_var(&tmpmask, GFP_NOWAIT)) {
> >> +		/* Fallback for OOM. */
> >> +		fallback = true;
> >> +	}
> >> +
> >> +	/*
> >> +	 * Skipping the current CPU is OK even through we can be
> >> +	 * migrated at any point. The current CPU, at the point where we
> >> +	 * read raw_smp_processor_id(), is ensured to be in program
> >> +	 * order with respect to the caller thread. Therefore, we can
> >> +	 * skip this CPU from the iteration.
> >> +	 */
> >> +	this_cpu = raw_smp_processor_id();
> >> +	cpus_read_lock();
> >> +	for_each_online_cpu(cpu) {
> >> +		struct task_struct *p;
> >> +
> >> +		if (cpu == this_cpu)
> >> +			continue;
> >> +		rcu_read_lock();
> >> +		p = task_rcu_dereference(&cpu_rq(cpu)->curr);
> >> +		if (p && p->mm == current->mm) {
> >> +			if (!fallback)
> >> +				__cpumask_set_cpu(cpu, tmpmask);
> >> +			else
> >> +				smp_call_function_single(cpu, ipi_mb, NULL, 1);
> >> +		}
> >> +		rcu_read_unlock();
> >> +	}
> >> +	cpus_read_unlock();
> >> +	if (!fallback) {
> >> +		smp_call_function_many(tmpmask, ipi_mb, NULL, 1);
> >> +		free_cpumask_var(tmpmask);
> >> +	}
> >> +
> >> +	/*
> >> +	 * Memory barrier on the caller thread _after_ we finished
> >> +	 * waiting for the last IPI. Matches memory barriers around
> >> +	 * rq->curr modification in scheduler.
> >> +	 */
> >> +	smp_mb();	/* exit from system call is not a mb */
> >> +}
> >> 
> >>  /**
> >>   * sys_membarrier - issue memory barriers on a set of threads
> >> @@ -64,6 +131,9 @@ SYSCALL_DEFINE2(membarrier, int, cmd, int, flags)
> >>  		if (num_online_cpus() > 1)
> >>  			synchronize_sched();
> >>  		return 0;
> >> +	case MEMBARRIER_CMD_PRIVATE_EXPEDITED:
> >> +		membarrier_private_expedited();
> >> +		return 0;
> >>  	default:
> >>  		return -EINVAL;
> >>  	}
> >> --
> >> 2.11.0
> 
> -- 
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com
>

[toc] | [prev] | [next] | [standalone]

#1698602

FromPeter Zijlstra <peterz@infradead.org>
Date2017-07-28 11:00 +0200
Message-ID<u89hL-1q4-11@gated-at.bofh.it>
In reply to#1698332
On Thu, Jul 27, 2017 at 05:13:14PM -0400, Mathieu Desnoyers wrote:
> +static void membarrier_expedited_mb_after_set_current(struct mm_struct *mm,
> +		struct mm_struct *oldmm)

That is a bit of a mouth-full...

> +{
> +	if (!IS_ENABLED(CONFIG_MEMBARRIER))
> +		return;
> +	/*
> +	 * __schedule()
> +	 *   finish_task_switch()
> +	 *    if (mm)
> +	 *      mmdrop(mm)
> +	 *        atomic_dec_and_test()
         *
> +	 * takes care of issuing a memory barrier when oldmm is
> +	 * non-NULL. We also don't need the barrier when switching to a
> +	 * kernel thread, nor when we switch between threads belonging
> +	 * to the same process.
> +	 */
> +	if (likely(oldmm || !mm || mm == oldmm))
> +		return;
> +	/*
> +	 * When switching between processes, membarrier expedited
> +	 * private requires a memory barrier after we set the current
> +	 * task.
> +	 */
> +	smp_mb();
> +}

And because of what it complements, I would have expected the callsite:

> @@ -2737,6 +2763,7 @@ context_switch(struct rq *rq, struct task_struct *prev,
>  
>  	mm = next->mm;
>  	oldmm = prev->active_mm;
> +	membarrier_expedited_mb_after_set_current(mm, oldmm);
>  	/*
>  	 * For paravirt, this is coupled with an exit in switch_to to
>  	 * combine the page table reload and the switch backend into

to be in finish_task_switch(), something like:

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index e9785f7aed75..33f34a201255 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2641,8 +2641,18 @@ static struct rq *finish_task_switch(struct task_struct *prev)
 	finish_arch_post_lock_switch();
 
 	fire_sched_in_preempt_notifiers(current);
+
+	/*
+	 * For CONFIG_MEMBARRIER we need a full memory barrier after the
+	 * rq->curr assignment. Not all architectures have one in either
+	 * switch_to() or switch_mm() so we use (and complement) the one
+	 * implied by mmdrop()'s atomic_dec_and_test().
+	 */
 	if (mm)
 		mmdrop(mm);
+	else if (IS_ENABLED(CONFIG_MEMBARRIER))
+		smp_mb();
+
 	if (unlikely(prev_state == TASK_DEAD)) {
 		if (prev->sched_class->task_dead)
 			prev->sched_class->task_dead(prev);


I realize this is sub-optimal if we're switching to a kernel thread, so
it might want some work, then again, a whole bunch of architectures
don't in fact need this extra barrier at all.

> +static void membarrier_private_expedited(void)
> +{
> +	int cpu, this_cpu;
> +	bool fallback = false;
> +	cpumask_var_t tmpmask;
> +
> +	if (num_online_cpus() == 1)
> +		return;
> +
> +	/*
> +	 * Matches memory barriers around rq->curr modification in
> +	 * scheduler.
> +	 */
> +	smp_mb();	/* system call entry is not a mb. */
> +

Weren't you going to put in a comment on that GFP_NOWAIT thing?

> +	if (!alloc_cpumask_var(&tmpmask, GFP_NOWAIT)) {

You really want: zalloc_cpumask_var().

> +		/* Fallback for OOM. */
> +		fallback = true;
> +	}
> +
> +	/*
> +	 * Skipping the current CPU is OK even through we can be
> +	 * migrated at any point. The current CPU, at the point where we
> +	 * read raw_smp_processor_id(), is ensured to be in program
> +	 * order with respect to the caller thread. Therefore, we can
> +	 * skip this CPU from the iteration.
> +	 */
> +	this_cpu = raw_smp_processor_id();

So if instead you do the below, that is still true, but you have the
opportunity to skip moar CPUs, then again, if you migrate the wrong way
you'll end up not skipping yourself.. a well.

> +	cpus_read_lock();
> +	for_each_online_cpu(cpu) {
> +		struct task_struct *p;
> +
		if (cpu == raw_smp_processor_id())
			continue;

> +		rcu_read_lock();
> +		p = task_rcu_dereference(&cpu_rq(cpu)->curr);
> +		if (p && p->mm == current->mm) {
> +			if (!fallback)
> +				__cpumask_set_cpu(cpu, tmpmask);
> +			else
> +				smp_call_function_single(cpu, ipi_mb, NULL, 1);
> +		}
> +		rcu_read_unlock();
> +	}
> +	cpus_read_unlock();

This ^, wants to go after that v

> +	if (!fallback) {
> +		smp_call_function_many(tmpmask, ipi_mb, NULL, 1);
> +		free_cpumask_var(tmpmask);
> +	}

Because otherwise the bits in your tmpmask might no longer match the
online state.

> +
> +	/*
> +	 * Memory barrier on the caller thread _after_ we finished
> +	 * waiting for the last IPI. Matches memory barriers around
> +	 * rq->curr modification in scheduler.
> +	 */
> +	smp_mb();	/* exit from system call is not a mb */
> +}

[toc] | [prev] | [next] | [standalone]

#1698702

FromPeter Zijlstra <peterz@infradead.org>
Date2017-07-28 13:20 +0200
Message-ID<u8btg-2ZD-9@gated-at.bofh.it>
In reply to#1698602
On Fri, Jul 28, 2017 at 10:55:32AM +0200, Peter Zijlstra wrote:
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index e9785f7aed75..33f34a201255 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -2641,8 +2641,18 @@ static struct rq *finish_task_switch(struct task_struct *prev)
>  	finish_arch_post_lock_switch();
>  
>  	fire_sched_in_preempt_notifiers(current);
> +
> +	/*
> +	 * For CONFIG_MEMBARRIER we need a full memory barrier after the
> +	 * rq->curr assignment. Not all architectures have one in either
> +	 * switch_to() or switch_mm() so we use (and complement) the one
> +	 * implied by mmdrop()'s atomic_dec_and_test().
> +	 */
>  	if (mm)
>  		mmdrop(mm);
> +	else if (IS_ENABLED(CONFIG_MEMBARRIER))
> +		smp_mb();
> +
>  	if (unlikely(prev_state == TASK_DEAD)) {
>  		if (prev->sched_class->task_dead)
>  			prev->sched_class->task_dead(prev);

Hurm.. so going over this again, I'm not sure this is as good as we
thought it was..


context_switch()

  mm = next->mm
  old_mm = prev->active_mm;

  if (!mm) {
    next->active_mm = old_mm;
    mmgrab(oldmm);
    enter_lazy_tlb(oldmm, next);
  } else
    switch_mm(oldmm, mm, next);

  if (!prev->mm) {
    prev->active_mm = NULL;
    rq->prev_mm = old_mm;
  }

  /* ... */

  finish_task_switch()

    mm = rq->prev_mm; // oldmm when !prev->mm
    rq->prev_mm = NULL;

    if (mm)
      mmdrop(mm);



That mmdrop() is to balance the mmgrab() for when we switch between
kthreads. Also, it looks to me that if we do kthread->kthread switches,
we do a superfluous mmgrab() / mmdrop().

Something like the below perhaps would optimize and clarify things.

---
 kernel/sched/core.c | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index e9785f7aed75..7924b4cc2bff 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2733,12 +2733,8 @@ static __always_inline struct rq *
 context_switch(struct rq *rq, struct task_struct *prev,
 	       struct task_struct *next, struct rq_flags *rf)
 {
-	struct mm_struct *mm, *oldmm;
-
 	prepare_task_switch(rq, prev, next);
 
-	mm = next->mm;
-	oldmm = prev->active_mm;
 	/*
 	 * For paravirt, this is coupled with an exit in switch_to to
 	 * combine the page table reload and the switch backend into
@@ -2746,16 +2742,29 @@ context_switch(struct rq *rq, struct task_struct *prev,
 	 */
 	arch_start_context_switch(prev);
 
-	if (!mm) {
-		next->active_mm = oldmm;
-		mmgrab(oldmm);
-		enter_lazy_tlb(oldmm, next);
-	} else
-		switch_mm_irqs_off(oldmm, mm, next);
+	/*
+	 * kernel -> kernel   transfer active
+	 *   user -> kernel   mmgrab()
+	 *
+	 * kernel ->   user   mmdrop()
+	 *   user ->   user   switch_mm()
+	 */
+	if (!next->mm) {				// to kernel
+		next->active_mm = prev->active_mm;
+		enter_lazy_tlb(prev->active_mm, next);
+
+		if (prev->mm)				// from user
+			mmgrab(prev->active_mm);
+
+	} else {					// to user
+		switch_mm_irqs_off(prev->active_mm, next->mm, next);
 
-	if (!prev->mm) {
-		prev->active_mm = NULL;
-		rq->prev_mm = oldmm;
+		if (!prev->mm) {			// from kernel
+			rq->prev_mm = prev->active_mm;
+			prev->active_mm = NULL;
+
+			/* will mmdrop() in finish_task_switch(). */
+		}
 	}
 
 	rq->clock_update_flags &= ~(RQCF_ACT_SKIP|RQCF_REQ_SKIP);

[toc] | [prev] | [next] | [standalone]

#1698714

FromPeter Zijlstra <peterz@infradead.org>
Date2017-07-28 14:00 +0200
Message-ID<u8c5Y-3fm-7@gated-at.bofh.it>
In reply to#1698602
On Fri, Jul 28, 2017 at 10:55:32AM +0200, Peter Zijlstra wrote:
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index e9785f7aed75..33f34a201255 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -2641,8 +2641,18 @@ static struct rq *finish_task_switch(struct task_struct *prev)
>  	finish_arch_post_lock_switch();
>  
>  	fire_sched_in_preempt_notifiers(current);
> +
> +	/*
> +	 * For CONFIG_MEMBARRIER we need a full memory barrier after the
> +	 * rq->curr assignment. Not all architectures have one in either
> +	 * switch_to() or switch_mm() so we use (and complement) the one
> +	 * implied by mmdrop()'s atomic_dec_and_test().
> +	 */
>  	if (mm)
>  		mmdrop(mm);
> +	else if (IS_ENABLED(CONFIG_MEMBARRIER))
> +		smp_mb();
> +
>  	if (unlikely(prev_state == TASK_DEAD)) {
>  		if (prev->sched_class->task_dead)
>  			prev->sched_class->task_dead(prev);
> 
> 

> a whole bunch of architectures don't in fact need this extra barrier at all.

In fact, I'm fairly sure its only PPC.

Because only ARM64 and PPC actually implement ACQUIRE/RELEASE with
anything other than smp_mb() (for now, Risc-V is in this same boat and
MIPS could be if they ever sort out their fancy barriers).

TSO archs use a regular STORE for RELEASE, but all their atomics imply a
smp_mb() and there are enough around to make one happen (typically
mm_cpumask updates).

Everybody else, aside from ARM64 and PPC must use smp_mb() for
ACQUIRE/RELEASE.

ARM64 has a super duper barrier in switch_to().

Which only leaves PPC stranded.. but the 'good' news is that mpe says
they'll probably need a barrier in switch_mm() in any case.

[toc] | [prev] | [next] | [standalone]

#1698907

FromMathieu Desnoyers <mathieu.desnoyers@efficios.com>
Date2017-07-28 17:40 +0200
Message-ID<u8fwS-5yW-11@gated-at.bofh.it>
In reply to#1698714
----- On Jul 28, 2017, at 7:57 AM, Peter Zijlstra peterz@infradead.org wrote:

> On Fri, Jul 28, 2017 at 10:55:32AM +0200, Peter Zijlstra wrote:
>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>> index e9785f7aed75..33f34a201255 100644
>> --- a/kernel/sched/core.c
>> +++ b/kernel/sched/core.c
>> @@ -2641,8 +2641,18 @@ static struct rq *finish_task_switch(struct task_struct
>> *prev)
>>  	finish_arch_post_lock_switch();
>>  
>>  	fire_sched_in_preempt_notifiers(current);
>> +
>> +	/*
>> +	 * For CONFIG_MEMBARRIER we need a full memory barrier after the
>> +	 * rq->curr assignment. Not all architectures have one in either
>> +	 * switch_to() or switch_mm() so we use (and complement) the one
>> +	 * implied by mmdrop()'s atomic_dec_and_test().
>> +	 */
>>  	if (mm)
>>  		mmdrop(mm);
>> +	else if (IS_ENABLED(CONFIG_MEMBARRIER))
>> +		smp_mb();
>> +
>>  	if (unlikely(prev_state == TASK_DEAD)) {
>>  		if (prev->sched_class->task_dead)
>>  			prev->sched_class->task_dead(prev);
>> 
>> 
> 
>> a whole bunch of architectures don't in fact need this extra barrier at all.
> 
> In fact, I'm fairly sure its only PPC.
> 
> Because only ARM64 and PPC actually implement ACQUIRE/RELEASE with
> anything other than smp_mb() (for now, Risc-V is in this same boat and
> MIPS could be if they ever sort out their fancy barriers).
> 
> TSO archs use a regular STORE for RELEASE, but all their atomics imply a
> smp_mb() and there are enough around to make one happen (typically
> mm_cpumask updates).
> 
> Everybody else, aside from ARM64 and PPC must use smp_mb() for
> ACQUIRE/RELEASE.
> 
> ARM64 has a super duper barrier in switch_to().
> 
> Which only leaves PPC stranded.. but the 'good' news is that mpe says
> they'll probably need a barrier in switch_mm() in any case.

As I pointed out in my other email, I plan to do this:

--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2636,6 +2636,11 @@ static struct rq *finish_task_switch(struct task_struct *prev)
        vtime_task_switch(prev);
        perf_event_task_sched_in(prev, current);
        finish_lock_switch(rq, prev);
+       /*
+        * The membarrier system call requires a full memory barrier
+        * after storing to rq->curr, before going back to user-space.
+        */
+       smp_mb__after_unlock_lock();
        finish_arch_post_lock_switch();
 
        fire_sched_in_preempt_notifiers(current);

Thoughts ?

Thanks,

Mathieu

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

[toc] | [prev] | [next] | [standalone]

#1698941

FromPeter Zijlstra <peterz@infradead.org>
Date2017-07-28 18:50 +0200
Message-ID<u8gCC-6cX-3@gated-at.bofh.it>
In reply to#1698907
On Fri, Jul 28, 2017 at 03:38:15PM +0000, Mathieu Desnoyers wrote:
> > Which only leaves PPC stranded.. but the 'good' news is that mpe says
> > they'll probably need a barrier in switch_mm() in any case.
> 
> As I pointed out in my other email, I plan to do this:
> 
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -2636,6 +2636,11 @@ static struct rq *finish_task_switch(struct task_struct *prev)
>         vtime_task_switch(prev);
>         perf_event_task_sched_in(prev, current);

Here would place it _inside_ the rq->lock, which seems to make more
sense given the purpose of the barrier, but either way works given its
definition.

>         finish_lock_switch(rq, prev);

You could put the whole thing inside IS_ENABLED(CONFIG_SYSMEMBARRIER) or
something.

> +       /*
> +        * The membarrier system call requires a full memory barrier
> +        * after storing to rq->curr, before going back to user-space.
> +        */
> +       smp_mb__after_unlock_lock();
>         finish_arch_post_lock_switch();
>  
>         fire_sched_in_preempt_notifiers(current);

[toc] | [prev] | [next] | [standalone]

#1698964

FromMathieu Desnoyers <mathieu.desnoyers@efficios.com>
Date2017-07-28 19:10 +0200
Message-ID<u8gVY-6za-37@gated-at.bofh.it>
In reply to#1698941
----- On Jul 28, 2017, at 12:46 PM, Peter Zijlstra peterz@infradead.org wrote:

> On Fri, Jul 28, 2017 at 03:38:15PM +0000, Mathieu Desnoyers wrote:
>> > Which only leaves PPC stranded.. but the 'good' news is that mpe says
>> > they'll probably need a barrier in switch_mm() in any case.
>> 
>> As I pointed out in my other email, I plan to do this:
>> 
>> --- a/kernel/sched/core.c
>> +++ b/kernel/sched/core.c
>> @@ -2636,6 +2636,11 @@ static struct rq *finish_task_switch(struct task_struct
>> *prev)
>>         vtime_task_switch(prev);
>>         perf_event_task_sched_in(prev, current);
> 
> Here would place it _inside_ the rq->lock, which seems to make more
> sense given the purpose of the barrier, but either way works given its
> definition.

Given its naming "...after_unlock_lock", I thought it would be clearer to put
it after the unlock. Anyway, this barrier does not seem to be used to ensure
the release barrier per se (unlock already has release semantic), but rather
ensures a full memory barrier wrt memory accesses that are synchronized by
means other than this this lock.

> 
>>         finish_lock_switch(rq, prev);
> 
> You could put the whole thing inside IS_ENABLED(CONFIG_SYSMEMBARRIER) or
> something.

I'm tempted to wait until we hear from powerpc maintainers, so we learn
whether they deeply care about this extra barrier in finish_task_switch()
before making it conditional on CONFIG_MEMBARRIER.

Having a guaranteed barrier after context switch on all architectures may
have other uses.

Thanks,

Mathieu

> 
>> +       /*
>> +        * The membarrier system call requires a full memory barrier
>> +        * after storing to rq->curr, before going back to user-space.
>> +        */
>> +       smp_mb__after_unlock_lock();
>>         finish_arch_post_lock_switch();
>>  
> >         fire_sched_in_preempt_notifiers(current);

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

[toc] | [prev] | [next] | [standalone]

#1699202

FromNicholas Piggin <npiggin@gmail.com>
Date2017-07-29 04:00 +0200
Message-ID<u8pcS-3Ah-15@gated-at.bofh.it>
In reply to#1698964
On Fri, 28 Jul 2017 17:06:53 +0000 (UTC)
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:

> ----- On Jul 28, 2017, at 12:46 PM, Peter Zijlstra peterz@infradead.org wrote:
> 
> > On Fri, Jul 28, 2017 at 03:38:15PM +0000, Mathieu Desnoyers wrote:  
> >> > Which only leaves PPC stranded.. but the 'good' news is that mpe says
> >> > they'll probably need a barrier in switch_mm() in any case.  
> >> 
> >> As I pointed out in my other email, I plan to do this:
> >> 
> >> --- a/kernel/sched/core.c
> >> +++ b/kernel/sched/core.c
> >> @@ -2636,6 +2636,11 @@ static struct rq *finish_task_switch(struct task_struct
> >> *prev)
> >>         vtime_task_switch(prev);
> >>         perf_event_task_sched_in(prev, current);  
> > 
> > Here would place it _inside_ the rq->lock, which seems to make more
> > sense given the purpose of the barrier, but either way works given its
> > definition.  
> 
> Given its naming "...after_unlock_lock", I thought it would be clearer to put
> it after the unlock. Anyway, this barrier does not seem to be used to ensure
> the release barrier per se (unlock already has release semantic), but rather
> ensures a full memory barrier wrt memory accesses that are synchronized by
> means other than this this lock.
> 
> >   
> >>         finish_lock_switch(rq, prev);  
> > 
> > You could put the whole thing inside IS_ENABLED(CONFIG_SYSMEMBARRIER) or
> > something.  
> 
> I'm tempted to wait until we hear from powerpc maintainers, so we learn
> whether they deeply care about this extra barrier in finish_task_switch()
> before making it conditional on CONFIG_MEMBARRIER.
> 
> Having a guaranteed barrier after context switch on all architectures may
> have other uses.

I haven't had time to read the thread and understand exactly why you need
this extra barrier, I'll do it next week. Thanks for cc'ing us on it.

A smp_mb is pretty expensive on powerpc CPUs. Removing the sync from
switch_to increased thread switch performance by 2-3%. Putting it in
switch_mm may be a little less painful, but still we have to weigh it
against the benefit of this new functionality. Would that be a net win
for the average end-user? Seems unlikely.

But we also don't want to lose sys_membarrier completely. Would it be too
painful to make  MEMBARRIER_CMD_PRIVATE_EXPEDITED return error, or make it
fall back to a slower case if we decide not to implement it?

Thanks,
Nick

[toc] | [prev] | [next] | [standalone]

#1699277

FromPeter Zijlstra <peterz@infradead.org>
Date2017-07-29 11:30 +0200
Message-ID<u8wem-br-23@gated-at.bofh.it>
In reply to#1699202
On Sat, Jul 29, 2017 at 11:58:40AM +1000, Nicholas Piggin wrote:
> I haven't had time to read the thread and understand exactly why you need
> this extra barrier, I'll do it next week. Thanks for cc'ing us on it.

Bottom of here:

https://lkml.kernel.org/r/20170727135610.jwjfvyuacqzj5e4u@hirez.programming.kicks-ass.net

is probably the fastest way towards understanding the need for a barrier
after rq->curr assignment.

Any barrier after that assignment is good for us, but so far it looks
like PPC doesn't (and PPC only afaict) provide any smp_mb() after that
point.

> A smp_mb is pretty expensive on powerpc CPUs. Removing the sync from
> switch_to increased thread switch performance by 2-3%. Putting it in
> switch_mm may be a little less painful, but still we have to weigh it
> against the benefit of this new functionality. Would that be a net win
> for the average end-user? Seems unlikely.
> 
> But we also don't want to lose sys_membarrier completely. Would it be too
> painful to make  MEMBARRIER_CMD_PRIVATE_EXPEDITED return error, or make it
> fall back to a slower case if we decide not to implement it?

One ugly thing we've thought of is tagging each mm that has used
sys_membarrier() and only issue the smp_mb() for those. That way only
those tasks that actually rely on the syscall get to pay the price.

[toc] | [prev] | [next] | [standalone]

#1699282

FromNicholas Piggin <npiggin@gmail.com>
Date2017-07-29 11:50 +0200
Message-ID<u8wxI-ju-19@gated-at.bofh.it>
In reply to#1699277
On Sat, 29 Jul 2017 19:45:43 +1000
Nicholas Piggin <npiggin@gmail.com> wrote:

> hmm, we might be able to restrict iteration
> to mm_cpumask(current->mm), no? 

Oh that's been discussed too. I'll read back over it too.

[toc] | [prev] | [next] | [standalone]

#1699295

FromPeter Zijlstra <peterz@infradead.org>
Date2017-07-29 13:00 +0200
Message-ID<u8xDr-140-1@gated-at.bofh.it>
In reply to#1699282
On Sat, Jul 29, 2017 at 07:48:56PM +1000, Nicholas Piggin wrote:
> On Sat, 29 Jul 2017 19:45:43 +1000
> Nicholas Piggin <npiggin@gmail.com> wrote:
> 
> > hmm, we might be able to restrict iteration
> > to mm_cpumask(current->mm), no? 
> 
> Oh that's been discussed too. I'll read back over it too.

Right, the main problem is that some architectures (arm64 for instance,
although Will is looking at it) don't use mm_cpumask() at all.

Other architectures (PPC for instance) only ever set bits.

[toc] | [prev] | [next] | [standalone]

#1699283

FromNicholas Piggin <npiggin@gmail.com>
Date2017-07-29 11:50 +0200
Message-ID<u8wxI-ju-15@gated-at.bofh.it>
In reply to#1699277
On Sat, 29 Jul 2017 11:23:33 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Sat, Jul 29, 2017 at 11:58:40AM +1000, Nicholas Piggin wrote:
> > I haven't had time to read the thread and understand exactly why you need
> > this extra barrier, I'll do it next week. Thanks for cc'ing us on it.  
> 
> Bottom of here:
> 
> https://lkml.kernel.org/r/20170727135610.jwjfvyuacqzj5e4u@hirez.programming.kicks-ass.net
> 
> is probably the fastest way towards understanding the need for a barrier
> after rq->curr assignment.
> 
> Any barrier after that assignment is good for us, but so far it looks
> like PPC doesn't (and PPC only afaict) provide any smp_mb() after that
> point.

Thanks, yeah that's relatively straightforward.

> > A smp_mb is pretty expensive on powerpc CPUs. Removing the sync from
> > switch_to increased thread switch performance by 2-3%. Putting it in
> > switch_mm may be a little less painful, but still we have to weigh it
> > against the benefit of this new functionality. Would that be a net win
> > for the average end-user? Seems unlikely.
> > 
> > But we also don't want to lose sys_membarrier completely. Would it be too
> > painful to make  MEMBARRIER_CMD_PRIVATE_EXPEDITED return error, or make it
> > fall back to a slower case if we decide not to implement it?  
> 
> One ugly thing we've thought of is tagging each mm that has used
> sys_membarrier() and only issue the smp_mb() for those. That way only
> those tasks that actually rely on the syscall get to pay the price.

The biggest hammer that puts everything on the syscall side I think would
be to lock each runqueue to while iterating over them, right?

That could be pretty expensive but it would be interesting to know how
bad that is for real apps... hmm, we might be able to restrict iteration
to mm_cpumask(current->mm), no?

[toc] | [prev] | [next] | [standalone]

#1698904

FromMathieu Desnoyers <mathieu.desnoyers@efficios.com>
Date2017-07-28 17:40 +0200
Message-ID<u8fwR-5yW-1@gated-at.bofh.it>
In reply to#1698602
----- On Jul 28, 2017, at 4:55 AM, Peter Zijlstra peterz@infradead.org wrote:

> On Thu, Jul 27, 2017 at 05:13:14PM -0400, Mathieu Desnoyers wrote:
>> +static void membarrier_expedited_mb_after_set_current(struct mm_struct *mm,
>> +		struct mm_struct *oldmm)
> 
> That is a bit of a mouth-full...
> 
>> +{
>> +	if (!IS_ENABLED(CONFIG_MEMBARRIER))
>> +		return;
>> +	/*
>> +	 * __schedule()
>> +	 *   finish_task_switch()
>> +	 *    if (mm)
>> +	 *      mmdrop(mm)
>> +	 *        atomic_dec_and_test()
>         *
>> +	 * takes care of issuing a memory barrier when oldmm is
>> +	 * non-NULL. We also don't need the barrier when switching to a
>> +	 * kernel thread, nor when we switch between threads belonging
>> +	 * to the same process.
>> +	 */
>> +	if (likely(oldmm || !mm || mm == oldmm))
>> +		return;
>> +	/*
>> +	 * When switching between processes, membarrier expedited
>> +	 * private requires a memory barrier after we set the current
>> +	 * task.
>> +	 */
>> +	smp_mb();
>> +}
> 
> And because of what it complements, I would have expected the callsite:
> 
>> @@ -2737,6 +2763,7 @@ context_switch(struct rq *rq, struct task_struct *prev,
>>  
>>  	mm = next->mm;
>>  	oldmm = prev->active_mm;
>> +	membarrier_expedited_mb_after_set_current(mm, oldmm);
>>  	/*
>>  	 * For paravirt, this is coupled with an exit in switch_to to
>>  	 * combine the page table reload and the switch backend into
> 
> to be in finish_task_switch(), something like:
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index e9785f7aed75..33f34a201255 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -2641,8 +2641,18 @@ static struct rq *finish_task_switch(struct task_struct
> *prev)
> 	finish_arch_post_lock_switch();
> 
> 	fire_sched_in_preempt_notifiers(current);
> +
> +	/*
> +	 * For CONFIG_MEMBARRIER we need a full memory barrier after the
> +	 * rq->curr assignment. Not all architectures have one in either
> +	 * switch_to() or switch_mm() so we use (and complement) the one
> +	 * implied by mmdrop()'s atomic_dec_and_test().
> +	 */
> 	if (mm)
> 		mmdrop(mm);
> +	else if (IS_ENABLED(CONFIG_MEMBARRIER))
> +		smp_mb();
> +
> 	if (unlikely(prev_state == TASK_DEAD)) {
> 		if (prev->sched_class->task_dead)
> 			prev->sched_class->task_dead(prev);
> 
> 
> I realize this is sub-optimal if we're switching to a kernel thread, so
> it might want some work, then again, a whole bunch of architectures
> don't in fact need this extra barrier at all.

As discussed on IRC, I plan to go instead for:

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 01e3b881ab3a..dd677fb2ee92 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2636,6 +2636,11 @@ static struct rq *finish_task_switch(struct task_struct *
prev)
        vtime_task_switch(prev);
        perf_event_task_sched_in(prev, current);
        finish_lock_switch(rq, prev);
+       /*
+        * The membarrier system call requires a full memory barrier
+        * after storing to rq->curr, before going back to user-space.
+        */
+       smp_mb__after_unlock_lock();
        finish_arch_post_lock_switch();
 
        fire_sched_in_preempt_notifiers(current);

Which is free on most architectures, except those defining
CONFIG_ARCH_WEAK_RELEASE_ACQUIRE. CCing PPC maintainers.

> 
>> +static void membarrier_private_expedited(void)
>> +{
>> +	int cpu, this_cpu;
>> +	bool fallback = false;
>> +	cpumask_var_t tmpmask;
>> +
>> +	if (num_online_cpus() == 1)
>> +		return;
>> +
>> +	/*
>> +	 * Matches memory barriers around rq->curr modification in
>> +	 * scheduler.
>> +	 */
>> +	smp_mb();	/* system call entry is not a mb. */
>> +
> 
> Weren't you going to put in a comment on that GFP_NOWAIT thing?

I only added it to the uapi header. Adding this to the implementation
too:

+       /*
+        * Expedited membarrier commands guarantee that they won't
+        * block, hence the GFP_NOWAIT allocation flag and fallback
+        * implementation.
+        */



> 
>> +	if (!alloc_cpumask_var(&tmpmask, GFP_NOWAIT)) {
> 
> You really want: zalloc_cpumask_var().

ok

> 
>> +		/* Fallback for OOM. */
>> +		fallback = true;
>> +	}
>> +
>> +	/*
>> +	 * Skipping the current CPU is OK even through we can be
>> +	 * migrated at any point. The current CPU, at the point where we
>> +	 * read raw_smp_processor_id(), is ensured to be in program
>> +	 * order with respect to the caller thread. Therefore, we can
>> +	 * skip this CPU from the iteration.
>> +	 */
>> +	this_cpu = raw_smp_processor_id();
> 
> So if instead you do the below, that is still true, but you have the
> opportunity to skip moar CPUs, then again, if you migrate the wrong way
> you'll end up not skipping yourself.. a well.

Chances are better to skip more CPUs in face of migration if we do it
in the loop as you suggest. Will do.

> 
>> +	cpus_read_lock();
>> +	for_each_online_cpu(cpu) {
>> +		struct task_struct *p;
>> +
>		if (cpu == raw_smp_processor_id())
>			continue;
> 
>> +		rcu_read_lock();
>> +		p = task_rcu_dereference(&cpu_rq(cpu)->curr);
>> +		if (p && p->mm == current->mm) {
>> +			if (!fallback)
>> +				__cpumask_set_cpu(cpu, tmpmask);
>> +			else
>> +				smp_call_function_single(cpu, ipi_mb, NULL, 1);
>> +		}
>> +		rcu_read_unlock();
>> +	}
>> +	cpus_read_unlock();
> 
> This ^, wants to go after that v
> 
>> +	if (!fallback) {
>> +		smp_call_function_many(tmpmask, ipi_mb, NULL, 1);
>> +		free_cpumask_var(tmpmask);
>> +	}
> 
> Because otherwise the bits in your tmpmask might no longer match the
> online state.

Good point, thanks!

Mathieu

> 
>> +
>> +	/*
>> +	 * Memory barrier on the caller thread _after_ we finished
>> +	 * waiting for the last IPI. Matches memory barriers around
>> +	 * rq->curr modification in scheduler.
>> +	 */
>> +	smp_mb();	/* exit from system call is not a mb */
> > +}

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web