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

Groups > linux.kernel > #1700165 > unrolled thread

Re: linux-next: manual merge of the rcu tree with the tip tree

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: linux-next: manual merge of the rcu tree with the tip tree "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-07-31 18:20 +0200
    Re: linux-next: manual merge of the rcu tree with the tip tree Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2017-08-01 02:10 +0200
      Re: linux-next: manual merge of the rcu tree with the tip tree "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-08-01 06:10 +0200
        Re: linux-next: manual merge of the rcu tree with the tip tree Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2017-08-01 06:30 +0200
          Re: linux-next: manual merge of the rcu tree with the tip tree "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-08-01 18:40 +0200
        Re: linux-next: manual merge of the rcu tree with the tip tree Andy Lutomirski <luto@kernel.org> - 2017-08-01 15:50 +0200
          Re: linux-next: manual merge of the rcu tree with the tip tree Peter Zijlstra <peterz@infradead.org> - 2017-08-01 16:00 +0200
            Re: linux-next: manual merge of the rcu tree with the tip tree Peter Zijlstra <peterz@infradead.org> - 2017-08-01 16:20 +0200
              Re: linux-next: manual merge of the rcu tree with the tip tree Andy Lutomirski <luto@kernel.org> - 2017-08-01 16:20 +0200
          Re: linux-next: manual merge of the rcu tree with the tip tree Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2017-08-01 16:10 +0200
            Re: linux-next: manual merge of the rcu tree with the tip tree Andy Lutomirski <luto@kernel.org> - 2017-08-01 16:20 +0200
              Re: linux-next: manual merge of the rcu tree with the tip tree Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2017-08-01 17:50 +0200
              Re: linux-next: manual merge of the rcu tree with the tip tree "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-08-01 23:40 +0200

#1700165 — Re: linux-next: manual merge of the rcu tree with the tip tree

On Mon, Jul 31, 2017 at 01:50:29PM +1000, Stephen Rothwell wrote:
> Hi Paul,
> 
> Today's linux-next merge of the rcu tree got a conflict in:
> 
>   arch/x86/mm/tlb.c
> 
> between commit:
> 
>   94b1b03b519b ("x86/mm: Rework lazy TLB mode and TLB freshness tracking")
> 
> from the tip tree and commit:
> 
>   d7713e8f8b23 ("membarrier: Expedited private command")
> 
> from the rcu tree.
> 
> I fixed it up (the former removed the comment and the load_cr3(), so I
> just dropped the commend change in the latter) and can carry the fix as
> necessary. This is now fixed as far as linux-next is concerned, but any
> non trivial conflicts should be mentioned to your upstream maintainer
> when your tree is submitted for merging.  You may also want to consider
> cooperating with the maintainer of the conflicting tree to minimise any
> particularly complex conflicts.

Thank you, Stephen!

Mathieu, Peter, our commit log reads as if removal of load_cr3() would
simply result in relying on the ordering provided by the atomic ops
in switch_mm() for mm_cpumask(), so that only the commit log and the
comment need changing.

Please let me know if I am missing something here.

							Thanx, Paul

[toc] | [next] | [standalone]

#1700530

----- On Jul 31, 2017, at 12:13 PM, Paul E. McKenney paulmck@linux.vnet.ibm.com wrote:

> On Mon, Jul 31, 2017 at 01:50:29PM +1000, Stephen Rothwell wrote:
>> Hi Paul,
>> 
>> Today's linux-next merge of the rcu tree got a conflict in:
>> 
>>   arch/x86/mm/tlb.c
>> 
>> between commit:
>> 
>>   94b1b03b519b ("x86/mm: Rework lazy TLB mode and TLB freshness tracking")
>> 
>> from the tip tree and commit:
>> 
>>   d7713e8f8b23 ("membarrier: Expedited private command")
>> 
>> from the rcu tree.
>> 
>> I fixed it up (the former removed the comment and the load_cr3(), so I
>> just dropped the commend change in the latter) and can carry the fix as
>> necessary. This is now fixed as far as linux-next is concerned, but any
>> non trivial conflicts should be mentioned to your upstream maintainer
>> when your tree is submitted for merging.  You may also want to consider
>> cooperating with the maintainer of the conflicting tree to minimise any
>> particularly complex conflicts.
> 
> Thank you, Stephen!
> 
> Mathieu, Peter, our commit log reads as if removal of load_cr3() would
> simply result in relying on the ordering provided by the atomic ops
> in switch_mm() for mm_cpumask(), so that only the commit log and the
> comment need changing.
> 
> Please let me know if I am missing something here.

I think you are right. Both load_cr3() and mm_cpumask update operations
(LOCK prefixed) provide the appropriate barriers on x86. So it's just a
matter of adapting the comment to the new reality.

Thanks,

Mathieu

> 
> 							Thanx, Paul

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

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

#1700606

On Tue, Aug 01, 2017 at 12:04:05AM +0000, Mathieu Desnoyers wrote:
> ----- On Jul 31, 2017, at 12:13 PM, Paul E. McKenney paulmck@linux.vnet.ibm.com wrote:
> 
> > On Mon, Jul 31, 2017 at 01:50:29PM +1000, Stephen Rothwell wrote:
> >> Hi Paul,
> >> 
> >> Today's linux-next merge of the rcu tree got a conflict in:
> >> 
> >>   arch/x86/mm/tlb.c
> >> 
> >> between commit:
> >> 
> >>   94b1b03b519b ("x86/mm: Rework lazy TLB mode and TLB freshness tracking")
> >> 
> >> from the tip tree and commit:
> >> 
> >>   d7713e8f8b23 ("membarrier: Expedited private command")
> >> 
> >> from the rcu tree.
> >> 
> >> I fixed it up (the former removed the comment and the load_cr3(), so I
> >> just dropped the commend change in the latter) and can carry the fix as
> >> necessary. This is now fixed as far as linux-next is concerned, but any
> >> non trivial conflicts should be mentioned to your upstream maintainer
> >> when your tree is submitted for merging.  You may also want to consider
> >> cooperating with the maintainer of the conflicting tree to minimise any
> >> particularly complex conflicts.
> > 
> > Thank you, Stephen!
> > 
> > Mathieu, Peter, our commit log reads as if removal of load_cr3() would
> > simply result in relying on the ordering provided by the atomic ops
> > in switch_mm() for mm_cpumask(), so that only the commit log and the
> > comment need changing.
> > 
> > Please let me know if I am missing something here.
> 
> I think you are right. Both load_cr3() and mm_cpumask update operations
> (LOCK prefixed) provide the appropriate barriers on x86. So it's just a
> matter of adapting the comment to the new reality.

Like this?

							Thanx, Paul

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

commit fde19879b6bd1abc0c1d4d5f945efed61bf7eb8c
Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Date:   Fri Jul 28 16:40:40 2017 -0400

    membarrier: Expedited private command
    
    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. It executes faster than the non-expedited
    variant (no blocking). It also works on NOHZ_FULL configurations.
    
    Scheduler-wise, it requires a memory barrier before and after context
    switching between processes (which have different mm). The memory
    barrier before context switch is already present. For the barrier after
    context switch:
    
    * Our TSO archs can do RELEASE without being a full barrier. Look at
      x86 spin_unlock() being a regular STORE for example.  But for those
      archs, all atomics imply smp_mb and all of them have atomic ops in
      switch_mm() for mm_cpumask().
    
    * From all weakly ordered machines, only ARM64 and PPC can do RELEASE,
      the rest does indeed do smp_mb(), so there the spin_unlock() is a full
      barrier and we're good.
    
    * ARM64 has a very heavy barrier in switch_to(), which suffices.
    
    * PPC just removed its barrier from switch_to(), but appears to be
      talking about adding something to switch_mm(). So add a
      smp_mb__after_unlock_lock() for now, until this is settled on the PPC
      side.
    
    Changes since v3:
    - Properly document the memory barriers provided by each architecture.
    
    Changes since v2:
    - Address comments from Peter Zijlstra,
    - Add smp_mb__after_unlock_lock() after finish_lock_switch() in
      finish_task_switch() to add the memory barrier we need after storing
      to rq->curr. This is much simpler than the previous approach relying
      on atomic_dec_and_test() in mmdrop(), which actually added a memory
      barrier in the common case of switching between userspace processes.
    - Return -EINVAL when MEMBARRIER_CMD_SHARED is used on a nohz_full
      kernel, rather than having the whole membarrier system call returning
      -ENOSYS. Indeed, CMD_PRIVATE_EXPEDITED is compatible with nohz_full.
      Adapt the CMD_QUERY mask accordingly.
    
    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>
    CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
    CC: Paul Mackerras <paulus@samba.org>
    CC: Michael Ellerman <mpe@ellerman.id.au>
    Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
    Tested-by: Dave Watson <davejwatson@fb.com>

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/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 659ae8094ed5..c8f7d98d8cb9 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -360,6 +360,8 @@ __notrace_funcgraph struct task_struct *__switch_to(struct task_struct *prev,
 	/*
 	 * Complete any pending TLB or cache maintenance on this CPU in case
 	 * the thread migrates to a different CPU.
+	 * This full barrier is also required by the membarrier system
+	 * call.
 	 */
 	dsb(ish);
 
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/membarrier.c b/kernel/membarrier.c
deleted file mode 100644
index 9f9284f37f8d..000000000000
--- a/kernel/membarrier.c
+++ /dev/null
@@ -1,70 +0,0 @@
-/*
- * Copyright (C) 2010, 2015 Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
- *
- * membarrier system call
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- */
-
-#include <linux/syscalls.h>
-#include <linux/membarrier.h>
-#include <linux/tick.h>
-
-/*
- * Bitmask made from a "or" of all commands within enum membarrier_cmd,
- * except MEMBARRIER_CMD_QUERY.
- */
-#define MEMBARRIER_CMD_BITMASK	(MEMBARRIER_CMD_SHARED)
-
-/**
- * sys_membarrier - issue memory barriers on a set of threads
- * @cmd:   Takes command values defined in enum membarrier_cmd.
- * @flags: Currently needs to be 0. For future extensions.
- *
- * If this system call is not implemented, -ENOSYS is returned. If the
- * command specified does not exist, or if the command argument is invalid,
- * this system call returns -EINVAL. For a given command, with flags argument
- * set to 0, this system call is guaranteed to always return the same value
- * until reboot.
- *
- * All memory accesses performed in program order from each targeted thread
- * is guaranteed to be ordered with respect to sys_membarrier(). If we use
- * the semantic "barrier()" to represent a compiler barrier forcing memory
- * accesses to be performed in program order across the barrier, and
- * smp_mb() to represent explicit memory barriers forcing full memory
- * ordering across the barrier, we have the following ordering table for
- * each pair of barrier(), sys_membarrier() and smp_mb():
- *
- * The pair ordering is detailed as (O: ordered, X: not ordered):
- *
- *                        barrier()   smp_mb() sys_membarrier()
- *        barrier()          X           X            O
- *        smp_mb()           X           O            O
- *        sys_membarrier()   O           O            O
- */
-SYSCALL_DEFINE2(membarrier, int, cmd, int, flags)
-{
-	/* MEMBARRIER_CMD_SHARED is not compatible with nohz_full. */
-	if (tick_nohz_full_enabled())
-		return -ENOSYS;
-	if (unlikely(flags))
-		return -EINVAL;
-	switch (cmd) {
-	case MEMBARRIER_CMD_QUERY:
-		return MEMBARRIER_CMD_BITMASK;
-	case MEMBARRIER_CMD_SHARED:
-		if (num_online_cpus() > 1)
-			synchronize_sched();
-		return 0;
-	default:
-		return -EINVAL;
-	}
-}
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 bfee6ea7db49..f77269c6c2f8 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2640,6 +2640,16 @@ static struct rq *finish_task_switch(struct task_struct *prev)
 	prev_state = prev->state;
 	vtime_task_switch(prev);
 	perf_event_task_sched_in(prev, current);
+	/*
+	 * The membarrier system call requires a full memory barrier
+	 * after storing to rq->curr, before going back to user-space.
+	 *
+	 * TODO: This smp_mb__after_unlock_lock can go away if PPC end
+	 * up adding a full barrier to switch_mm(), or we should figure
+	 * out if a smp_mb__after_unlock_lock is really the proper API
+	 * to use.
+	 */
+	smp_mb__after_unlock_lock();
 	finish_lock_switch(rq, prev);
 	finish_arch_post_lock_switch();
 
@@ -3329,6 +3339,21 @@ static void __sched notrace __schedule(bool preempt)
 	if (likely(prev != next)) {
 		rq->nr_switches++;
 		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. For TSO
+		 * (e.g. x86), the architecture must provide its own
+		 * barrier in switch_mm(). For weakly ordered machines
+		 * for which spin_unlock() acts as a full memory
+		 * barrier, finish_lock_switch() in common code takes
+		 * care of this barrier. For weakly ordered machines for
+		 * which spin_unlock() acts as a RELEASE barrier (only
+		 * arm64 and PowerPC), arm64 has a full barrier in
+		 * switch_to(), and PowerPC has
+		 * smp_mb__after_unlock_lock() before
+		 * finish_lock_switch().
+		 */
 		++*switch_count;
 
 		trace_sched_switch(preempt, prev, next);
diff --git a/kernel/sched/membarrier.c b/kernel/sched/membarrier.c
new file mode 100644
index 000000000000..a92fddc22747
--- /dev/null
+++ b/kernel/sched/membarrier.c
@@ -0,0 +1,152 @@
+/*
+ * Copyright (C) 2010-2017 Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
+ *
+ * membarrier system call
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#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 | 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;
+	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. */
+
+	/*
+	 * Expedited membarrier commands guarantee that they won't
+	 * block, hence the GFP_NOWAIT allocation flag and fallback
+	 * implementation.
+	 */
+	if (!zalloc_cpumask_var(&tmpmask, GFP_NOWAIT)) {
+		/* Fallback for OOM. */
+		fallback = true;
+	}
+
+	cpus_read_lock();
+	for_each_online_cpu(cpu) {
+		struct task_struct *p;
+
+		/*
+		 * 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.
+		 */
+		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();
+	}
+	if (!fallback) {
+		smp_call_function_many(tmpmask, ipi_mb, NULL, 1);
+		free_cpumask_var(tmpmask);
+	}
+	cpus_read_unlock();
+
+	/*
+	 * 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
+ * @cmd:   Takes command values defined in enum membarrier_cmd.
+ * @flags: Currently needs to be 0. For future extensions.
+ *
+ * If this system call is not implemented, -ENOSYS is returned. If the
+ * command specified does not exist, not available on the running
+ * kernel, or if the command argument is invalid, this system call
+ * returns -EINVAL. For a given command, with flags argument set to 0,
+ * this system call is guaranteed to always return the same value until
+ * reboot.
+ *
+ * All memory accesses performed in program order from each targeted thread
+ * is guaranteed to be ordered with respect to sys_membarrier(). If we use
+ * the semantic "barrier()" to represent a compiler barrier forcing memory
+ * accesses to be performed in program order across the barrier, and
+ * smp_mb() to represent explicit memory barriers forcing full memory
+ * ordering across the barrier, we have the following ordering table for
+ * each pair of barrier(), sys_membarrier() and smp_mb():
+ *
+ * The pair ordering is detailed as (O: ordered, X: not ordered):
+ *
+ *                        barrier()   smp_mb() sys_membarrier()
+ *        barrier()          X           X            O
+ *        smp_mb()           X           O            O
+ *        sys_membarrier()   O           O            O
+ */
+SYSCALL_DEFINE2(membarrier, int, cmd, int, flags)
+{
+	if (unlikely(flags))
+		return -EINVAL;
+	switch (cmd) {
+	case MEMBARRIER_CMD_QUERY:
+	{
+		int cmd_mask = MEMBARRIER_CMD_BITMASK;
+
+		if (tick_nohz_full_enabled())
+			cmd_mask &= ~MEMBARRIER_CMD_SHARED;
+		return cmd_mask;
+	}
+	case MEMBARRIER_CMD_SHARED:
+		/* MEMBARRIER_CMD_SHARED is not compatible with nohz_full. */
+		if (tick_nohz_full_enabled())
+			return -EINVAL;
+		if (num_online_cpus() > 1)
+			synchronize_sched();
+		return 0;
+	case MEMBARRIER_CMD_PRIVATE_EXPEDITED:
+		membarrier_private_expedited();
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}

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

#1700610

----- On Aug 1, 2017, at 12:03 AM, Paul E. McKenney paulmck@linux.vnet.ibm.com wrote:

> On Tue, Aug 01, 2017 at 12:04:05AM +0000, Mathieu Desnoyers wrote:
>> ----- On Jul 31, 2017, at 12:13 PM, Paul E. McKenney paulmck@linux.vnet.ibm.com
>> wrote:
>> 
>> > On Mon, Jul 31, 2017 at 01:50:29PM +1000, Stephen Rothwell wrote:
>> >> Hi Paul,
>> >> 
>> >> Today's linux-next merge of the rcu tree got a conflict in:
>> >> 
>> >>   arch/x86/mm/tlb.c
>> >> 
>> >> between commit:
>> >> 
>> >>   94b1b03b519b ("x86/mm: Rework lazy TLB mode and TLB freshness tracking")
>> >> 
>> >> from the tip tree and commit:
>> >> 
>> >>   d7713e8f8b23 ("membarrier: Expedited private command")
>> >> 
>> >> from the rcu tree.
>> >> 
>> >> I fixed it up (the former removed the comment and the load_cr3(), so I
>> >> just dropped the commend change in the latter) and can carry the fix as
>> >> necessary. This is now fixed as far as linux-next is concerned, but any
>> >> non trivial conflicts should be mentioned to your upstream maintainer
>> >> when your tree is submitted for merging.  You may also want to consider
>> >> cooperating with the maintainer of the conflicting tree to minimise any
>> >> particularly complex conflicts.
>> > 
>> > Thank you, Stephen!
>> > 
>> > Mathieu, Peter, our commit log reads as if removal of load_cr3() would
>> > simply result in relying on the ordering provided by the atomic ops
>> > in switch_mm() for mm_cpumask(), so that only the commit log and the
>> > comment need changing.
>> > 
>> > Please let me know if I am missing something here.
>> 
>> I think you are right. Both load_cr3() and mm_cpumask update operations
>> (LOCK prefixed) provide the appropriate barriers on x86. So it's just a
>> matter of adapting the comment to the new reality.
> 
> Like this?

The updated comment in the commit message looks good, but I would be
tempted to add a comment in x86 switch_mm_irqs_off() stating the
following requirement just before the line invoking cpumask_set_cpu():

/*
 * The full memory barrier implied by mm_cpumask update operations
 * is required by the membarrier system call.
 */

Thanks,

Mathieu

> 
>							Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
> commit fde19879b6bd1abc0c1d4d5f945efed61bf7eb8c
> Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Date:   Fri Jul 28 16:40:40 2017 -0400
> 
>    membarrier: Expedited private command
>    
>    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. It executes faster than the non-expedited
>    variant (no blocking). It also works on NOHZ_FULL configurations.
>    
>    Scheduler-wise, it requires a memory barrier before and after context
>    switching between processes (which have different mm). The memory
>    barrier before context switch is already present. For the barrier after
>    context switch:
>    
>    * Our TSO archs can do RELEASE without being a full barrier. Look at
>      x86 spin_unlock() being a regular STORE for example.  But for those
>      archs, all atomics imply smp_mb and all of them have atomic ops in
>      switch_mm() for mm_cpumask().
>    
>    * From all weakly ordered machines, only ARM64 and PPC can do RELEASE,
>      the rest does indeed do smp_mb(), so there the spin_unlock() is a full
>      barrier and we're good.
>    
>    * ARM64 has a very heavy barrier in switch_to(), which suffices.
>    
>    * PPC just removed its barrier from switch_to(), but appears to be
>      talking about adding something to switch_mm(). So add a
>      smp_mb__after_unlock_lock() for now, until this is settled on the PPC
>      side.
>    
>    Changes since v3:
>    - Properly document the memory barriers provided by each architecture.
>    
>    Changes since v2:
>    - Address comments from Peter Zijlstra,
>    - Add smp_mb__after_unlock_lock() after finish_lock_switch() in
>      finish_task_switch() to add the memory barrier we need after storing
>      to rq->curr. This is much simpler than the previous approach relying
>      on atomic_dec_and_test() in mmdrop(), which actually added a memory
>      barrier in the common case of switching between userspace processes.
>    - Return -EINVAL when MEMBARRIER_CMD_SHARED is used on a nohz_full
>      kernel, rather than having the whole membarrier system call returning
>      -ENOSYS. Indeed, CMD_PRIVATE_EXPEDITED is compatible with nohz_full.
>      Adapt the CMD_QUERY mask accordingly.
>    
>    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>
>    CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>    CC: Paul Mackerras <paulus@samba.org>
>    CC: Michael Ellerman <mpe@ellerman.id.au>
>    Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>    Tested-by: Dave Watson <davejwatson@fb.com>
> 
> 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/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 659ae8094ed5..c8f7d98d8cb9 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -360,6 +360,8 @@ __notrace_funcgraph struct task_struct *__switch_to(struct
> task_struct *prev,
> 	/*
> 	 * Complete any pending TLB or cache maintenance on this CPU in case
> 	 * the thread migrates to a different CPU.
> +	 * This full barrier is also required by the membarrier system
> +	 * call.
> 	 */
> 	dsb(ish);
> 
> 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/membarrier.c b/kernel/membarrier.c
> deleted file mode 100644
> index 9f9284f37f8d..000000000000
> --- a/kernel/membarrier.c
> +++ /dev/null
> @@ -1,70 +0,0 @@
> -/*
> - * Copyright (C) 2010, 2015 Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> - *
> - * membarrier system call
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License as published by
> - * the Free Software Foundation; either version 2 of the License, or
> - * (at your option) any later version.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> - * GNU General Public License for more details.
> - */
> -
> -#include <linux/syscalls.h>
> -#include <linux/membarrier.h>
> -#include <linux/tick.h>
> -
> -/*
> - * Bitmask made from a "or" of all commands within enum membarrier_cmd,
> - * except MEMBARRIER_CMD_QUERY.
> - */
> -#define MEMBARRIER_CMD_BITMASK	(MEMBARRIER_CMD_SHARED)
> -
> -/**
> - * sys_membarrier - issue memory barriers on a set of threads
> - * @cmd:   Takes command values defined in enum membarrier_cmd.
> - * @flags: Currently needs to be 0. For future extensions.
> - *
> - * If this system call is not implemented, -ENOSYS is returned. If the
> - * command specified does not exist, or if the command argument is invalid,
> - * this system call returns -EINVAL. For a given command, with flags argument
> - * set to 0, this system call is guaranteed to always return the same value
> - * until reboot.
> - *
> - * All memory accesses performed in program order from each targeted thread
> - * is guaranteed to be ordered with respect to sys_membarrier(). If we use
> - * the semantic "barrier()" to represent a compiler barrier forcing memory
> - * accesses to be performed in program order across the barrier, and
> - * smp_mb() to represent explicit memory barriers forcing full memory
> - * ordering across the barrier, we have the following ordering table for
> - * each pair of barrier(), sys_membarrier() and smp_mb():
> - *
> - * The pair ordering is detailed as (O: ordered, X: not ordered):
> - *
> - *                        barrier()   smp_mb() sys_membarrier()
> - *        barrier()          X           X            O
> - *        smp_mb()           X           O            O
> - *        sys_membarrier()   O           O            O
> - */
> -SYSCALL_DEFINE2(membarrier, int, cmd, int, flags)
> -{
> -	/* MEMBARRIER_CMD_SHARED is not compatible with nohz_full. */
> -	if (tick_nohz_full_enabled())
> -		return -ENOSYS;
> -	if (unlikely(flags))
> -		return -EINVAL;
> -	switch (cmd) {
> -	case MEMBARRIER_CMD_QUERY:
> -		return MEMBARRIER_CMD_BITMASK;
> -	case MEMBARRIER_CMD_SHARED:
> -		if (num_online_cpus() > 1)
> -			synchronize_sched();
> -		return 0;
> -	default:
> -		return -EINVAL;
> -	}
> -}
> 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 bfee6ea7db49..f77269c6c2f8 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -2640,6 +2640,16 @@ static struct rq *finish_task_switch(struct task_struct
> *prev)
> 	prev_state = prev->state;
> 	vtime_task_switch(prev);
> 	perf_event_task_sched_in(prev, current);
> +	/*
> +	 * The membarrier system call requires a full memory barrier
> +	 * after storing to rq->curr, before going back to user-space.
> +	 *
> +	 * TODO: This smp_mb__after_unlock_lock can go away if PPC end
> +	 * up adding a full barrier to switch_mm(), or we should figure
> +	 * out if a smp_mb__after_unlock_lock is really the proper API
> +	 * to use.
> +	 */
> +	smp_mb__after_unlock_lock();
> 	finish_lock_switch(rq, prev);
> 	finish_arch_post_lock_switch();
> 
> @@ -3329,6 +3339,21 @@ static void __sched notrace __schedule(bool preempt)
> 	if (likely(prev != next)) {
> 		rq->nr_switches++;
> 		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. For TSO
> +		 * (e.g. x86), the architecture must provide its own
> +		 * barrier in switch_mm(). For weakly ordered machines
> +		 * for which spin_unlock() acts as a full memory
> +		 * barrier, finish_lock_switch() in common code takes
> +		 * care of this barrier. For weakly ordered machines for
> +		 * which spin_unlock() acts as a RELEASE barrier (only
> +		 * arm64 and PowerPC), arm64 has a full barrier in
> +		 * switch_to(), and PowerPC has
> +		 * smp_mb__after_unlock_lock() before
> +		 * finish_lock_switch().
> +		 */
> 		++*switch_count;
> 
> 		trace_sched_switch(preempt, prev, next);
> diff --git a/kernel/sched/membarrier.c b/kernel/sched/membarrier.c
> new file mode 100644
> index 000000000000..a92fddc22747
> --- /dev/null
> +++ b/kernel/sched/membarrier.c
> @@ -0,0 +1,152 @@
> +/*
> + * Copyright (C) 2010-2017 Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> + *
> + * membarrier system call
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#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 | 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;
> +	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. */
> +
> +	/*
> +	 * Expedited membarrier commands guarantee that they won't
> +	 * block, hence the GFP_NOWAIT allocation flag and fallback
> +	 * implementation.
> +	 */
> +	if (!zalloc_cpumask_var(&tmpmask, GFP_NOWAIT)) {
> +		/* Fallback for OOM. */
> +		fallback = true;
> +	}
> +
> +	cpus_read_lock();
> +	for_each_online_cpu(cpu) {
> +		struct task_struct *p;
> +
> +		/*
> +		 * 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.
> +		 */
> +		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();
> +	}
> +	if (!fallback) {
> +		smp_call_function_many(tmpmask, ipi_mb, NULL, 1);
> +		free_cpumask_var(tmpmask);
> +	}
> +	cpus_read_unlock();
> +
> +	/*
> +	 * 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
> + * @cmd:   Takes command values defined in enum membarrier_cmd.
> + * @flags: Currently needs to be 0. For future extensions.
> + *
> + * If this system call is not implemented, -ENOSYS is returned. If the
> + * command specified does not exist, not available on the running
> + * kernel, or if the command argument is invalid, this system call
> + * returns -EINVAL. For a given command, with flags argument set to 0,
> + * this system call is guaranteed to always return the same value until
> + * reboot.
> + *
> + * All memory accesses performed in program order from each targeted thread
> + * is guaranteed to be ordered with respect to sys_membarrier(). If we use
> + * the semantic "barrier()" to represent a compiler barrier forcing memory
> + * accesses to be performed in program order across the barrier, and
> + * smp_mb() to represent explicit memory barriers forcing full memory
> + * ordering across the barrier, we have the following ordering table for
> + * each pair of barrier(), sys_membarrier() and smp_mb():
> + *
> + * The pair ordering is detailed as (O: ordered, X: not ordered):
> + *
> + *                        barrier()   smp_mb() sys_membarrier()
> + *        barrier()          X           X            O
> + *        smp_mb()           X           O            O
> + *        sys_membarrier()   O           O            O
> + */
> +SYSCALL_DEFINE2(membarrier, int, cmd, int, flags)
> +{
> +	if (unlikely(flags))
> +		return -EINVAL;
> +	switch (cmd) {
> +	case MEMBARRIER_CMD_QUERY:
> +	{
> +		int cmd_mask = MEMBARRIER_CMD_BITMASK;
> +
> +		if (tick_nohz_full_enabled())
> +			cmd_mask &= ~MEMBARRIER_CMD_SHARED;
> +		return cmd_mask;
> +	}
> +	case MEMBARRIER_CMD_SHARED:
> +		/* MEMBARRIER_CMD_SHARED is not compatible with nohz_full. */
> +		if (tick_nohz_full_enabled())
> +			return -EINVAL;
> +		if (num_online_cpus() > 1)
> +			synchronize_sched();
> +		return 0;
> +	case MEMBARRIER_CMD_PRIVATE_EXPEDITED:
> +		membarrier_private_expedited();
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +}

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

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

#1701228

On Tue, Aug 01, 2017 at 04:25:56AM +0000, Mathieu Desnoyers wrote:
> ----- On Aug 1, 2017, at 12:03 AM, Paul E. McKenney paulmck@linux.vnet.ibm.com wrote:
> 
> > On Tue, Aug 01, 2017 at 12:04:05AM +0000, Mathieu Desnoyers wrote:
> >> ----- On Jul 31, 2017, at 12:13 PM, Paul E. McKenney paulmck@linux.vnet.ibm.com
> >> wrote:
> >> 
> >> > On Mon, Jul 31, 2017 at 01:50:29PM +1000, Stephen Rothwell wrote:
> >> >> Hi Paul,
> >> >> 
> >> >> Today's linux-next merge of the rcu tree got a conflict in:
> >> >> 
> >> >>   arch/x86/mm/tlb.c
> >> >> 
> >> >> between commit:
> >> >> 
> >> >>   94b1b03b519b ("x86/mm: Rework lazy TLB mode and TLB freshness tracking")
> >> >> 
> >> >> from the tip tree and commit:
> >> >> 
> >> >>   d7713e8f8b23 ("membarrier: Expedited private command")
> >> >> 
> >> >> from the rcu tree.
> >> >> 
> >> >> I fixed it up (the former removed the comment and the load_cr3(), so I
> >> >> just dropped the commend change in the latter) and can carry the fix as
> >> >> necessary. This is now fixed as far as linux-next is concerned, but any
> >> >> non trivial conflicts should be mentioned to your upstream maintainer
> >> >> when your tree is submitted for merging.  You may also want to consider
> >> >> cooperating with the maintainer of the conflicting tree to minimise any
> >> >> particularly complex conflicts.
> >> > 
> >> > Thank you, Stephen!
> >> > 
> >> > Mathieu, Peter, our commit log reads as if removal of load_cr3() would
> >> > simply result in relying on the ordering provided by the atomic ops
> >> > in switch_mm() for mm_cpumask(), so that only the commit log and the
> >> > comment need changing.
> >> > 
> >> > Please let me know if I am missing something here.
> >> 
> >> I think you are right. Both load_cr3() and mm_cpumask update operations
> >> (LOCK prefixed) provide the appropriate barriers on x86. So it's just a
> >> matter of adapting the comment to the new reality.
> > 
> > Like this?
> 
> The updated comment in the commit message looks good, but I would be
> tempted to add a comment in x86 switch_mm_irqs_off() stating the
> following requirement just before the line invoking cpumask_set_cpu():
> 
> /*
>  * The full memory barrier implied by mm_cpumask update operations
>  * is required by the membarrier system call.
>  */

This looks good to me, but I will give the discussion another day or
so to settle out.  ;-)

							Thanx, Paul

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

#1701066

On Mon, Jul 31, 2017 at 9:03 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Tue, Aug 01, 2017 at 12:04:05AM +0000, Mathieu Desnoyers wrote:
>> ----- On Jul 31, 2017, at 12:13 PM, Paul E. McKenney paulmck@linux.vnet.ibm.com wrote:
>>

>                                                         Thanx, Paul
>
> ------------------------------------------------------------------------
>
> commit fde19879b6bd1abc0c1d4d5f945efed61bf7eb8c
> Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Date:   Fri Jul 28 16:40:40 2017 -0400
>
>     membarrier: Expedited private command
>
>     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. It executes faster than the non-expedited
>     variant (no blocking). It also works on NOHZ_FULL configurations.
>
>     Scheduler-wise, it requires a memory barrier before and after context
>     switching between processes (which have different mm). The memory
>     barrier before context switch is already present. For the barrier after
>     context switch:
>
>     * Our TSO archs can do RELEASE without being a full barrier. Look at
>       x86 spin_unlock() being a regular STORE for example.  But for those
>       archs, all atomics imply smp_mb and all of them have atomic ops in
>       switch_mm() for mm_cpumask().

I think that, on x86, context switches, even without mm changes, must
at least flush the store buffer (maybe SFENCE is okay) to avoid
visible inconsistency due to store-buffer forwarding.

Anyway, can you document whatever property you require with a comment
in switch_mm() or wherever you're finding that property so that future
arch changes don't break it?

> +static void membarrier_private_expedited(void)
> +{
> +       int 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. */
> +
> +       /*
> +        * Expedited membarrier commands guarantee that they won't
> +        * block, hence the GFP_NOWAIT allocation flag and fallback
> +        * implementation.
> +        */
> +       if (!zalloc_cpumask_var(&tmpmask, GFP_NOWAIT)) {
> +               /* Fallback for OOM. */
> +               fallback = true;
> +       }
> +
> +       cpus_read_lock();
> +       for_each_online_cpu(cpu) {
> +               struct task_struct *p;
> +
> +               /*
> +                * 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.
> +                */
> +               if (cpu == raw_smp_processor_id())
> +                       continue;
> +               rcu_read_lock();
> +               p = task_rcu_dereference(&cpu_rq(cpu)->curr);
> +               if (p && p->mm == current->mm) {

I'm a bit surprised you're iterating all CPUs instead of just CPUs in
mm_cpumask().

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

#1701079

On Tue, Aug 01, 2017 at 06:43:14AM -0700, Andy Lutomirski wrote:
> Anyway, can you document whatever property you require with a comment
> in switch_mm() or wherever you're finding that property so that future
> arch changes don't break it?

We need _a_ smp_mb after rq->curr store. x86 has plenty.

> > +static void membarrier_private_expedited(void)
> > +{
> > +       int 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. */
> > +
> > +       /*
> > +        * Expedited membarrier commands guarantee that they won't
> > +        * block, hence the GFP_NOWAIT allocation flag and fallback
> > +        * implementation.
> > +        */
> > +       if (!zalloc_cpumask_var(&tmpmask, GFP_NOWAIT)) {
> > +               /* Fallback for OOM. */
> > +               fallback = true;
> > +       }
> > +
> > +       cpus_read_lock();
> > +       for_each_online_cpu(cpu) {
> > +               struct task_struct *p;
> > +
> > +               /*
> > +                * 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.
> > +                */
> > +               if (cpu == raw_smp_processor_id())
> > +                       continue;
> > +               rcu_read_lock();
> > +               p = task_rcu_dereference(&cpu_rq(cpu)->curr);
> > +               if (p && p->mm == current->mm) {
> 
> I'm a bit surprised you're iterating all CPUs instead of just CPUs in
> mm_cpumask().

Because ARM64 doesn't set any bits at all in there.

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

#1701099

On Tue, Aug 01, 2017 at 03:58:49PM +0200, Peter Zijlstra wrote:
> On Tue, Aug 01, 2017 at 06:43:14AM -0700, Andy Lutomirski wrote:
> > Anyway, can you document whatever property you require with a comment
> > in switch_mm() or wherever you're finding that property so that future
> > arch changes don't break it?
> 
> We need _a_ smp_mb after rq->curr store. x86 has plenty.

That is, we need it when we change to a different !0 mm. And we have the
mm_cpumask() atomics at the very least, even if loading a new CR3 would
not be serializing.

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

#1701100

On Tue, Aug 1, 2017 at 7:15 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, Aug 01, 2017 at 03:58:49PM +0200, Peter Zijlstra wrote:
>> On Tue, Aug 01, 2017 at 06:43:14AM -0700, Andy Lutomirski wrote:
>> > Anyway, can you document whatever property you require with a comment
>> > in switch_mm() or wherever you're finding that property so that future
>> > arch changes don't break it?
>>
>> We need _a_ smp_mb after rq->curr store. x86 has plenty.
>
> That is, we need it when we change to a different !0 mm. And we have the
> mm_cpumask() atomics at the very least, even if loading a new CR3 would
> not be serializing.

I'm 99.5% sure that loading a new CR3 is always serializing even if it
doesn't flush the TLB.

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

#1701092

----- On Aug 1, 2017, at 9:43 AM, Andy Lutomirski luto@kernel.org wrote:

> On Mon, Jul 31, 2017 at 9:03 PM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
>> On Tue, Aug 01, 2017 at 12:04:05AM +0000, Mathieu Desnoyers wrote:
>>> ----- On Jul 31, 2017, at 12:13 PM, Paul E. McKenney paulmck@linux.vnet.ibm.com
>>> wrote:
>>>
> 
>>                                                         Thanx, Paul
>>
>> ------------------------------------------------------------------------
>>
>> commit fde19879b6bd1abc0c1d4d5f945efed61bf7eb8c
>> Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>> Date:   Fri Jul 28 16:40:40 2017 -0400
>>
>>     membarrier: Expedited private command
>>
>>     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. It executes faster than the non-expedited
>>     variant (no blocking). It also works on NOHZ_FULL configurations.
>>
>>     Scheduler-wise, it requires a memory barrier before and after context
>>     switching between processes (which have different mm). The memory
>>     barrier before context switch is already present. For the barrier after
>>     context switch:
>>
>>     * Our TSO archs can do RELEASE without being a full barrier. Look at
>>       x86 spin_unlock() being a regular STORE for example.  But for those
>>       archs, all atomics imply smp_mb and all of them have atomic ops in
>>       switch_mm() for mm_cpumask().
> 
> I think that, on x86, context switches, even without mm changes, must
> at least flush the store buffer (maybe SFENCE is okay) to avoid
> visible inconsistency due to store-buffer forwarding.
> 
> Anyway, can you document whatever property you require with a comment
> in switch_mm() or wherever you're finding that property so that future
> arch changes don't break it?

As I asked to Paul in my reply to his proposed manual merge,
we should indeed have a comment in switch_mm() stating something
like this just before the line invoking cpumask_set_cpu():

/*
 * The full memory barrier implied by mm_cpumask update operations
 * is required by the membarrier system call.
 */

What we want to order here is:

prev userspace memory accesses
schedule
  <full mb> (it's already there) [A]
  update to rq->curr changing the rq->curr->mm value
  <full mb> (provided by mm_cpumask updates in switch_mm on x86) [B]
next userspace memory accesses

wrt to:

userspace memory accesses
sys_membarrier
  <full mb> [C]
  iterate on each cpu's rq->curr, compare their "mm" to current->mm
    IPI each CPU that match
  <full mb> [D]
userspace memory accesses

[A] pairs with [D] and [B] pairs with [C].


> 
>> +static void membarrier_private_expedited(void)
>> +{
>> +       int 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. */
>> +
>> +       /*
>> +        * Expedited membarrier commands guarantee that they won't
>> +        * block, hence the GFP_NOWAIT allocation flag and fallback
>> +        * implementation.
>> +        */
>> +       if (!zalloc_cpumask_var(&tmpmask, GFP_NOWAIT)) {
>> +               /* Fallback for OOM. */
>> +               fallback = true;
>> +       }
>> +
>> +       cpus_read_lock();
>> +       for_each_online_cpu(cpu) {
>> +               struct task_struct *p;
>> +
>> +               /*
>> +                * 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.
>> +                */
>> +               if (cpu == raw_smp_processor_id())
>> +                       continue;
>> +               rcu_read_lock();
>> +               p = task_rcu_dereference(&cpu_rq(cpu)->curr);
>> +               if (p && p->mm == current->mm) {
> 
> I'm a bit surprised you're iterating all CPUs instead of just CPUs in
> mm_cpumask().

I see two reasons for this. The first is because architectures like
ARM64 don't even bother populating the mm_cpumask. The second reason
is because I don't think all architectures ensure that updates to
mm_cpumask imply full memory barriers. Therefore, we would need to revisit
each architecture switch_mm to ensure mm_cpumask bit set ops either imply
a full memory barrier, or are followed by an explicit one, if we
choose to use this bitmask as an optimization.

Thanks,

Mathieu


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

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

#1701105

On Tue, Aug 1, 2017 at 7:02 AM, Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
> /*
>  * The full memory barrier implied by mm_cpumask update operations
>  * is required by the membarrier system call.
>  */
>
> What we want to order here is:
>
> prev userspace memory accesses
> schedule
>   <full mb> (it's already there) [A]
>   update to rq->curr changing the rq->curr->mm value
>   <full mb> (provided by mm_cpumask updates in switch_mm on x86) [B]

If I understand this right, the issue with relying on CR3 writes is
that the target CPU could switch to a kernel thread and back to the
same user mm white the membarrier caller is reading its mm, right?

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

#1701173

----- On Aug 1, 2017, at 10:15 AM, Andy Lutomirski luto@kernel.org wrote:

> On Tue, Aug 1, 2017 at 7:02 AM, Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>> /*
>>  * The full memory barrier implied by mm_cpumask update operations
>>  * is required by the membarrier system call.
>>  */
>>
>> What we want to order here is:
>>
>> prev userspace memory accesses
>> schedule
>>   <full mb> (it's already there) [A]
>>   update to rq->curr changing the rq->curr->mm value
>>   <full mb> (provided by mm_cpumask updates in switch_mm on x86) [B]
> 
> If I understand this right, the issue with relying on CR3 writes is
> that the target CPU could switch to a kernel thread and back to the
> same user mm white the membarrier caller is reading its mm, right?

The current implementation of context_switch() does:

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

        if (!mm)
                next->active_mm = oldmm;

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

so basically the only way to have a non-null rq->prev_mm when we
reach finish_task_switch() is to have a non-null prev->active_mm
in context_switch (kernel thread).

finish_task_switch() has:

struct mm_struct *mm = rq->prev_mm;
[...]
if (mm)
        mmdrop(mm);

which issues a full memory barrier through atomic_dec_and_test(). This
happens to take care of this kthread->uthread scenario. I think it would
be important to document though.

Thanks,

Mathieu


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

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

#1701523

On Tue, Aug 01, 2017 at 07:15:40AM -0700, Andy Lutomirski wrote:
> On Tue, Aug 1, 2017 at 7:02 AM, Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
> > /*
> >  * The full memory barrier implied by mm_cpumask update operations
> >  * is required by the membarrier system call.
> >  */
> >
> > What we want to order here is:
> >
> > prev userspace memory accesses
> > schedule
> >   <full mb> (it's already there) [A]
> >   update to rq->curr changing the rq->curr->mm value
> >   <full mb> (provided by mm_cpumask updates in switch_mm on x86) [B]
> 
> If I understand this right, the issue with relying on CR3 writes is
> that the target CPU could switch to a kernel thread and back to the
> same user mm white the membarrier caller is reading its mm, right?

The thing that got my attention was your patch removing the load_cr3().
Ah, looking closer, it appears that you have not eliminated the CR3
load, but just renamed it to write_cr3().  So if there is always still
a CR3 load, you are right, I should be able to simply move the comment.
Or let you insert the comment into your patch?

So there is still always a CR3 load, correct?  (Hey, I thought that
maybe x86 was moving to ASIDs or some such.)

							Thanx, Paul

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web