[PATCH RFC tip/core/rcu 0/15] Remove to-be-unneeded smp_read_barrier_depends()

[PATCH RFC tip/core/rcu 0/15] Remove to-be-unneeded smp_read_barrier_depends()

[Paul E. McKenney]

Hello!

Will Deacon has proposed adding smp_read_barrier_depends() to READ_ONCE(),
which would mean that quite a few instances of smp_read_barrier_depends()
would become redundant.  This series depends on Will's change and removes
those smp_read_barrier_depends(), while fixing a bug or two along the
way.  Some of these bugs are subtle, hence posting this sooner rather
than later.

This patch series depends on an in-progress series from Mark Rutland that
changes all calls to ACCESS_ONCE() to either READ_ONCE() or WRITE_ONCE(),
depending.  This patch series also depends on an upcoming patch from Will
Deacon that adds smp_read_barrier_depends() to Alpha's relaxed/release
RMW atomic operations.

As Will pointed out, once all of these patch series are in place, the
core kernel will no longer need any Alpha-specific code, with the sole
exception of READ_ONCE() and of course smp_read_barrier_depends() itself.

Pretty cool, huh?  ;-)

							Thanx, Paul

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

 Documentation/memory-barriers.txt         |   11 ++++++-----
 arch/mn10300/kernel/mn10300-serial.c      |    7 +++++--
 drivers/net/ethernet/qlogic/qed/qed_spq.c |    4 +---
 fs/dcache.c                               |   10 +++-------
 include/linux/assoc_array_priv.h          |    5 +++--
 include/linux/percpu-refcount.h           |    6 +++---
 include/linux/rcupdate.h                  |   23 +++++++++++------------
 include/linux/rtnetlink.h                 |    3 +--
 include/linux/seqlock.h                   |    3 +--
 kernel/events/uprobes.c                   |   12 ++++++------
 kernel/locking/qspinlock.c                |   12 +++++-------
 kernel/tracepoint.c                       |    9 ++++-----
 lib/assoc_array.c                         |   20 ++------------------
 lib/percpu-refcount.c                     |    8 ++++----
 mm/ksm.c                                  |    9 +--------
 net/ipv4/netfilter/arp_tables.c           |    7 +------
 net/ipv4/netfilter/ip_tables.c            |    7 +------
 net/ipv6/netfilter/ip6_tables.c           |    7 +------
 security/keys/keyring.c                   |    7 -------
 19 files changed, 59 insertions(+), 111 deletions(-)

[PATCH RFC tip/core/rcu 01/15] doc: READ_ONCE() now implies smp_barrier_depends()

[Paul E. McKenney]

This commit updates an example in memory-barriers.txt to account for
the fact that READ_ONCE() now implies smp_barrier_depends().

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 Documentation/memory-barriers.txt | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
index b759a60624fd..b592bb265e5c 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -227,17 +227,18 @@ There are some minimal guarantees that may be expected of a CPU:
  (*) On any given CPU, dependent memory accesses will be issued in order, with
      respect to itself.  This means that for:
 
-	Q = READ_ONCE(P); smp_read_barrier_depends(); D = READ_ONCE(*Q);
+	Q = READ_ONCE(P); D = READ_ONCE(*Q);
 
      the CPU will issue the following memory operations:
 
 	Q = LOAD P, D = LOAD *Q
 
      and always in that order.  On most systems, smp_read_barrier_depends()
-     does nothing, but it is required for DEC Alpha.  The READ_ONCE()
-     is required to prevent compiler mischief.  Please note that you
-     should normally use something like rcu_dereference() instead of
-     open-coding smp_read_barrier_depends().
+     does nothing, but it is required for DEC Alpha, and is supplied by
+     READ_ONCE().  The READ_ONCE() is also required to prevent compiler
+     mischief.  Please note that you should normally use something
+     like READ_ONCE() or rcu_dereference() instead of open-coding
+     smp_read_barrier_depends().
 
  (*) Overlapping loads and stores within a particular CPU will appear to be
      ordered within that CPU.  This means that for:
-- 
2.5.2

[PATCH RFC tip/core/rcu 02/15] mn10300: READ_ONCE() now implies smp_read_barrier_depends()

Given that READ_ONCE() now implies smp_read_barrier_depends(),
there is no need for the open-coded smp_read_barrier_depends() in
mn10300_serial_receive_interrupt() and mn10300_serial_poll_get_char().
This commit therefore removes them, but replaces them with comments
calling out that carrying dependencies through non-pointers is quite
dangerous.  Compilers simply know too much about integers.

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: <linux-am33-list@redhat.com>
---
 arch/mn10300/kernel/mn10300-serial.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/mn10300/kernel/mn10300-serial.c b/arch/mn10300/kernel/mn10300-serial.c
index d7ef1232a82a..4994b570dfd9 100644
--- a/arch/mn10300/kernel/mn10300-serial.c
+++ b/arch/mn10300/kernel/mn10300-serial.c
@@ -550,7 +550,7 @@ static void mn10300_serial_receive_interrupt(struct mn10300_serial_port *port)
 		return;
 	}
 
-	smp_read_barrier_depends();
+	/* READ_ONCE() enforces dependency, but dangerous through integer!!! */
 	ch = port->rx_buffer[ix++];
 	st = port->rx_buffer[ix++];
 	smp_mb();
@@ -1728,7 +1728,10 @@ static int mn10300_serial_poll_get_char(struct uart_port *_port)
 			if (CIRC_CNT(port->rx_inp, ix, MNSC_BUFFER_SIZE) == 0)
 				return NO_POLL_CHAR;
 
-			smp_read_barrier_depends();
+			/*
+			 * READ_ONCE() enforces dependency, but dangerous
+			 * through integer!!!
+			 */
 			ch = port->rx_buffer[ix++];
 			st = port->rx_buffer[ix++];
 			smp_mb();
-- 
2.5.2

[PATCH RFC tip/core/rcu 02/15] mn10300: READ_ONCE() now implies smp_read_barrier_depends()

[Paul E. McKenney]

Given that READ_ONCE() now implies smp_read_barrier_depends(),
there is no need for the open-coded smp_read_barrier_depends() in
mn10300_serial_receive_interrupt() and mn10300_serial_poll_get_char().
This commit therefore removes them, but replaces them with comments
calling out that carrying dependencies through non-pointers is quite
dangerous.  Compilers simply know too much about integers.

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: <linux-am33-list@redhat.com>
---
 arch/mn10300/kernel/mn10300-serial.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/mn10300/kernel/mn10300-serial.c b/arch/mn10300/kernel/mn10300-serial.c
index d7ef1232a82a..4994b570dfd9 100644
--- a/arch/mn10300/kernel/mn10300-serial.c
+++ b/arch/mn10300/kernel/mn10300-serial.c
@@ -550,7 +550,7 @@ static void mn10300_serial_receive_interrupt(struct mn10300_serial_port *port)
 		return;
 	}
 
-	smp_read_barrier_depends();
+	/* READ_ONCE() enforces dependency, but dangerous through integer!!! */
 	ch = port->rx_buffer[ix++];
 	st = port->rx_buffer[ix++];
 	smp_mb();
@@ -1728,7 +1728,10 @@ static int mn10300_serial_poll_get_char(struct uart_port *_port)
 			if (CIRC_CNT(port->rx_inp, ix, MNSC_BUFFER_SIZE) == 0)
 				return NO_POLL_CHAR;
 
-			smp_read_barrier_depends();
+			/*
+			 * READ_ONCE() enforces dependency, but dangerous
+			 * through integer!!!
+			 */
 			ch = port->rx_buffer[ix++];
 			st = port->rx_buffer[ix++];
 			smp_mb();
-- 
2.5.2

[PATCH RFC tip/core/rcu 03/15] drivers/net/ethernet/qlogic/qed: Fix __qed_spq_block() ordering

[Paul E. McKenney]

The __qed_spq_block() function expects an smp_read_barrier_depends()
to order a prior READ_ONCE() against a later load that does not depend
on the prior READ_ONCE(), an expectation that can fail to be met.
This commit therefore replaces the READ_ONCE() with smp_load_acquire()
and removes the smp_read_barrier_depends().

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Ariel Elior <Ariel.Elior@cavium.com>
Cc: <everest-linux-l2@cavium.com>
Cc: <netdev@vger.kernel.org>
---
 drivers/net/ethernet/qlogic/qed/qed_spq.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qed/qed_spq.c b/drivers/net/ethernet/qlogic/qed/qed_spq.c
index be48d9abd001..c1237ec58b6c 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_spq.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_spq.c
@@ -97,9 +97,7 @@ static int __qed_spq_block(struct qed_hwfn *p_hwfn,
 
 	while (iter_cnt--) {
 		/* Validate we receive completion update */
-		if (READ_ONCE(comp_done->done) == 1) {
-			/* Read updated FW return value */
-			smp_read_barrier_depends();
+		if (smp_load_acquire(&comp_done->done) == 1) { /* ^^^ */
 			if (p_fw_ret)
 				*p_fw_ret = comp_done->fw_return_code;
 			return 0;
-- 
2.5.2

[PATCH RFC tip/core/rcu 03/15] drivers/net/ethernet/qlogic/qed: Fix __qed_spq_block() ordering

[Paul E. McKenney]

The __qed_spq_block() function expects an smp_read_barrier_depends()
to order a prior READ_ONCE() against a later load that does not depend
on the prior READ_ONCE(), an expectation that can fail to be met.
This commit therefore replaces the READ_ONCE() with smp_load_acquire()
and removes the smp_read_barrier_depends().

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Ariel Elior <Ariel.Elior@cavium.com>
Cc: <everest-linux-l2@cavium.com>
Cc: <netdev@vger.kernel.org>
---
 drivers/net/ethernet/qlogic/qed/qed_spq.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qed/qed_spq.c b/drivers/net/ethernet/qlogic/qed/qed_spq.c
index be48d9abd001..c1237ec58b6c 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_spq.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_spq.c
@@ -97,9 +97,7 @@ static int __qed_spq_block(struct qed_hwfn *p_hwfn,
 
 	while (iter_cnt--) {
 		/* Validate we receive completion update */
-		if (READ_ONCE(comp_done->done) == 1) {
-			/* Read updated FW return value */
-			smp_read_barrier_depends();
+		if (smp_load_acquire(&comp_done->done) == 1) { /* ^^^ */
 			if (p_fw_ret)
 				*p_fw_ret = comp_done->fw_return_code;
 			return 0;
-- 
2.5.2

[PATCH RFC tip/core/rcu 04/15] fs/dcache: Use release-acquire for name/length update

[Paul E. McKenney]

The code in __d_alloc() carefully orders filling in the NUL character
of the name (and the length, hash, and the name itself) with assigning
of the name itself.  However, prepend_name() does not order the accesses
to the ->name and ->len fields, other than on TSO systems.  This commit
therefore replaces prepend_name()'s READ_ONCE() of ->name with an
smp_load_acquire(), which orders against the subsequent READ_ONCE() of
->len.  Because READ_ONCE() now incorporates smp_read_barrier_depends(),
prepend_name()'s smp_read_barrier_depends() is removed.  Finally,
to save a line, the smp_wmb()/store pair in __d_alloc() is replaced
by smp_store_release().

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: <linux-fsdevel@vger.kernel.org>
---
 fs/dcache.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index f283cd31a491..c40ae213fd2f 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1636,8 +1636,7 @@ struct dentry *__d_alloc(struct super_block *sb, const struct qstr *name)
 	dname[name->len] = 0;
 
 	/* Make sure we always see the terminating NUL character */
-	smp_wmb();
-	dentry->d_name.name = dname;
+	smp_store_release(&dentry->d_name.name, dname); /* ^^^ */
 
 	dentry->d_lockref.count = 1;
 	dentry->d_flags = 0;
@@ -3049,17 +3048,14 @@ static int prepend(char **buffer, int *buflen, const char *str, int namelen)
  * retry it again when a d_move() does happen. So any garbage in the buffer
  * due to mismatched pointer and length will be discarded.
  *
- * Data dependency barrier is needed to make sure that we see that terminating
- * NUL.  Alpha strikes again, film at 11...
+ * Load acquire is needed to make sure that we see that terminating NUL.
  */
 static int prepend_name(char **buffer, int *buflen, const struct qstr *name)
 {
-	const char *dname = READ_ONCE(name->name);
+	const char *dname = smp_load_acquire(&name->name); /* ^^^ */
 	u32 dlen = READ_ONCE(name->len);
 	char *p;
 
-	smp_read_barrier_depends();
-
 	*buflen -= dlen + 1;
 	if (*buflen < 0)
 		return -ENAMETOOLONG;
-- 
2.5.2

[PATCH RFC tip/core/rcu 04/15] fs/dcache: Use release-acquire for name/length update

[Paul E. McKenney]

The code in __d_alloc() carefully orders filling in the NUL character
of the name (and the length, hash, and the name itself) with assigning
of the name itself.  However, prepend_name() does not order the accesses
to the ->name and ->len fields, other than on TSO systems.  This commit
therefore replaces prepend_name()'s READ_ONCE() of ->name with an
smp_load_acquire(), which orders against the subsequent READ_ONCE() of
->len.  Because READ_ONCE() now incorporates smp_read_barrier_depends(),
prepend_name()'s smp_read_barrier_depends() is removed.  Finally,
to save a line, the smp_wmb()/store pair in __d_alloc() is replaced
by smp_store_release().

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: <linux-fsdevel@vger.kernel.org>
---
 fs/dcache.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index f283cd31a491..c40ae213fd2f 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1636,8 +1636,7 @@ struct dentry *__d_alloc(struct super_block *sb, const struct qstr *name)
 	dname[name->len] = 0;
 
 	/* Make sure we always see the terminating NUL character */
-	smp_wmb();
-	dentry->d_name.name = dname;
+	smp_store_release(&dentry->d_name.name, dname); /* ^^^ */
 
 	dentry->d_lockref.count = 1;
 	dentry->d_flags = 0;
@@ -3049,17 +3048,14 @@ static int prepend(char **buffer, int *buflen, const char *str, int namelen)
  * retry it again when a d_move() does happen. So any garbage in the buffer
  * due to mismatched pointer and length will be discarded.
  *
- * Data dependency barrier is needed to make sure that we see that terminating
- * NUL.  Alpha strikes again, film at 11...
+ * Load acquire is needed to make sure that we see that terminating NUL.
  */
 static int prepend_name(char **buffer, int *buflen, const struct qstr *name)
 {
-	const char *dname = READ_ONCE(name->name);
+	const char *dname = smp_load_acquire(&name->name); /* ^^^ */
 	u32 dlen = READ_ONCE(name->len);
 	char *p;
 
-	smp_read_barrier_depends();
-
 	*buflen -= dlen + 1;
 	if (*buflen < 0)
 		return -ENAMETOOLONG;
-- 
2.5.2

[PATCH RFC tip/core/rcu 05/15] percpu: READ_ONCE() now implies smp_read_barrier_depends()

[Paul E. McKenney]

Because READ_ONCE() now implies smp_read_barrier_depends(), this commit
removes the now-redundant smp_read_barrier_depends() following the
READ_ONCE() in __ref_is_percpu().

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Christoph Lameter <cl@linux.com>
---
 include/linux/percpu-refcount.h | 6 +++---
 lib/percpu-refcount.c           | 8 ++++----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/linux/percpu-refcount.h b/include/linux/percpu-refcount.h
index c13dceb87b60..1d0c18c59304 100644
--- a/include/linux/percpu-refcount.h
+++ b/include/linux/percpu-refcount.h
@@ -138,12 +138,12 @@ static inline bool __ref_is_percpu(struct percpu_ref *ref,
 	 * when using it as a pointer, __PERCPU_REF_ATOMIC may be set in
 	 * between contaminating the pointer value, meaning that
 	 * READ_ONCE() is required when fetching it.
+	 *
+	 * The smp_read_barrier_depends() implied by READ_ONCE() pairs
+	 * with smp_store_release() in __percpu_ref_switch_to_percpu().
 	 */
 	percpu_ptr = READ_ONCE(ref->percpu_count_ptr);
 
-	/* paired with smp_store_release() in __percpu_ref_switch_to_percpu() */
-	smp_read_barrier_depends();
-
 	/*
 	 * Theoretically, the following could test just ATOMIC; however,
 	 * then we'd have to mask off DEAD separately as DEAD may be
diff --git a/lib/percpu-refcount.c b/lib/percpu-refcount.c
index fe03c6d52761..30e7dd88148b 100644
--- a/lib/percpu-refcount.c
+++ b/lib/percpu-refcount.c
@@ -197,10 +197,10 @@ static void __percpu_ref_switch_to_percpu(struct percpu_ref *ref)
 	atomic_long_add(PERCPU_COUNT_BIAS, &ref->count);
 
 	/*
-	 * Restore per-cpu operation.  smp_store_release() is paired with
-	 * smp_read_barrier_depends() in __ref_is_percpu() and guarantees
-	 * that the zeroing is visible to all percpu accesses which can see
-	 * the following __PERCPU_REF_ATOMIC clearing.
+	 * Restore per-cpu operation.  smp_store_release() is paired
+	 * with READ_ONCE() in __ref_is_percpu() and guarantees that the
+	 * zeroing is visible to all percpu accesses which can see the
+	 * following __PERCPU_REF_ATOMIC clearing.
 	 */
 	for_each_possible_cpu(cpu)
 		*per_cpu_ptr(percpu_count, cpu) = 0;
-- 
2.5.2

Re: [PATCH RFC tip/core/rcu 05/15] percpu: READ_ONCE() now implies smp_read_barrier_depends()

[Tejun Heo]

On Mon, Oct 09, 2017 at 05:22:39PM -0700, Paul E. McKenney wrote:
> Because READ_ONCE() now implies smp_read_barrier_depends(), this commit
> removes the now-redundant smp_read_barrier_depends() following the
> READ_ONCE() in __ref_is_percpu().
> 
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Christoph Lameter <cl@linux.com>

Acked-by: Tejun Heo <tj@kernel.org>

Please feel free to route with other patches.  If this should be
routed through the percpu tree, please let me know.

Thanks.

-- 
tejun

Re: [PATCH RFC tip/core/rcu 05/15] percpu: READ_ONCE() now implies smp_read_barrier_depends()

[Paul E. McKenney]

On Tue, Oct 10, 2017 at 07:08:13AM -0700, Tejun Heo wrote:
> On Mon, Oct 09, 2017 at 05:22:39PM -0700, Paul E. McKenney wrote:
> > Because READ_ONCE() now implies smp_read_barrier_depends(), this commit
> > removes the now-redundant smp_read_barrier_depends() following the
> > READ_ONCE() in __ref_is_percpu().
> > 
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > Cc: Tejun Heo <tj@kernel.org>
> > Cc: Christoph Lameter <cl@linux.com>
> 
> Acked-by: Tejun Heo <tj@kernel.org>

Thank you, applied!

> Please feel free to route with other patches.  If this should be
> routed through the percpu tree, please let me know.

This depends on some other not-yet-finalized patch series, so if you
are OK with it, I will route them with that group of serieses.

							Thanx, Paul

Re: [PATCH RFC tip/core/rcu 05/15] percpu: READ_ONCE() now implies smp_read_barrier_depends()

[Tejun Heo]

On Tue, Oct 10, 2017 at 08:30:47AM -0700, Paul E. McKenney wrote:
> > Please feel free to route with other patches.  If this should be
> > routed through the percpu tree, please let me know.
> 
> This depends on some other not-yet-finalized patch series, so if you
> are OK with it, I will route them with that group of serieses.

Sure thing.  Thanks for the much needed simplification!

-- 
tejun

[PATCH RFC tip/core/rcu 06/15] rcu: Adjust read-side accessor comments for READ_ONCE()

[Paul E. McKenney]

Now that READ_ONCE() implies smp_read_barrier_depends(), the commit
updates now-misleading comments to account for this change.

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 include/linux/rcupdate.h | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index de50d8a4cf41..de386d82e2f1 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -433,12 +433,12 @@ static inline void rcu_preempt_sleep_check(void) { }
  * @p: The pointer to read
  *
  * Return the value of the specified RCU-protected pointer, but omit the
- * smp_read_barrier_depends() and keep the READ_ONCE().  This is useful
- * when the value of this pointer is accessed, but the pointer is not
- * dereferenced, for example, when testing an RCU-protected pointer against
- * NULL.  Although rcu_access_pointer() may also be used in cases where
- * update-side locks prevent the value of the pointer from changing, you
- * should instead use rcu_dereference_protected() for this use case.
+ * lockdep checks for being in an RCU read-side critical section.  This is
+ * useful when the value of this pointer is accessed, but the pointer is
+ * not dereferenced, for example, when testing an RCU-protected pointer
+ * against NULL.  Although rcu_access_pointer() may also be used in cases
+ * where update-side locks prevent the value of the pointer from changing,
+ * you should instead use rcu_dereference_protected() for this use case.
  *
  * It is also permissible to use rcu_access_pointer() when read-side
  * access to the pointer was removed at least one grace period ago, as
@@ -521,12 +521,11 @@ static inline void rcu_preempt_sleep_check(void) { }
  * @c: The conditions under which the dereference will take place
  *
  * Return the value of the specified RCU-protected pointer, but omit
- * both the smp_read_barrier_depends() and the READ_ONCE().  This
- * is useful in cases where update-side locks prevent the value of the
- * pointer from changing.  Please note that this primitive does -not-
- * prevent the compiler from repeating this reference or combining it
- * with other references, so it should not be used without protection
- * of appropriate locks.
+ * the READ_ONCE().  This is useful in cases where update-side locks
+ * prevent the value of the pointer from changing.  Please note that this
+ * primitive does -not- prevent the compiler from repeating this reference
+ * or combining it with other references, so it should not be used without
+ * protection of appropriate locks.
  *
  * This function is only for update-side use.  Using this function
  * when protected only by rcu_read_lock() will result in infrequent
-- 
2.5.2

[PATCH RFC tip/core/rcu 07/15] rtnetlink: Update now-misleading smp_read_barrier_depends() comment

[Paul E. McKenney]

Now that READ_ONCE() implies smp_read_barrier_depends(), update the
rtnl_dereference() header comment accordingly.

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Vladislav Yasevich <vyasevic@redhat.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Vlad Yasevich <vyasevich@gmail.com>
---
 include/linux/rtnetlink.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/linux/rtnetlink.h b/include/linux/rtnetlink.h
index 765f7b915475..107720d6b978 100644
--- a/include/linux/rtnetlink.h
+++ b/include/linux/rtnetlink.h
@@ -67,8 +67,7 @@ static inline bool lockdep_rtnl_is_held(void)
  * @p: The pointer to read, prior to dereferencing
  *
  * Return the value of the specified RCU-protected pointer, but omit
- * both the smp_read_barrier_depends() and the READ_ONCE(), because
- * caller holds RTNL.
+ * the READ_ONCE(), because caller holds RTNL.
  */
 #define rtnl_dereference(p)					\
 	rcu_dereference_protected(p, lockdep_rtnl_is_held())
-- 
2.5.2

[PATCH RFC tip/core/rcu 08/15] seqlock: Remove now-redundant smp_read_barrier_depends()

[Paul E. McKenney]

READ_ONCE() now implies smp_read_barrier_depends(), so this patch
removes the now-redundant smp_read_barrier_depends() from
raw_read_seqcount_latch().

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
---
 include/linux/seqlock.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h
index ead97654c4e9..27364347ff68 100644
--- a/include/linux/seqlock.h
+++ b/include/linux/seqlock.h
@@ -277,9 +277,8 @@ static inline void raw_write_seqcount_barrier(seqcount_t *s)
 
 static inline int raw_read_seqcount_latch(seqcount_t *s)
 {
-	int seq = READ_ONCE(s->sequence);
 	/* Pairs with the first smp_wmb() in raw_write_seqcount_latch() */
-	smp_read_barrier_depends();
+	int seq = READ_ONCE(s->sequence); /* ^^^ */
 	return seq;
 }
 
-- 
2.5.2

[PATCH RFC tip/core/rcu 09/15] uprobes: Remove now-redundant smp_read_barrier_depends()

[Paul E. McKenney]

Now that READ_ONCE() implies smp_read_barrier_depends(), the
get_xol_area() and get_trampoline_vaddr() no longer need their
smp_read_barrier_depends() calls, which this commit removes.
While we are here, convert the corresponding smp_wmb() to an
smp_store_release().

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 kernel/events/uprobes.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 267f6ef91d97..ce6848e46e94 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1167,8 +1167,8 @@ static int xol_add_vma(struct mm_struct *mm, struct xol_area *area)
 	}
 
 	ret = 0;
-	smp_wmb();	/* pairs with get_xol_area() */
-	mm->uprobes_state.xol_area = area;
+	/* pairs with get_xol_area() */
+	smp_store_release(&mm->uprobes_state.xol_area, area); /* ^^^ */
  fail:
 	up_write(&mm->mmap_sem);
 
@@ -1230,8 +1230,8 @@ static struct xol_area *get_xol_area(void)
 	if (!mm->uprobes_state.xol_area)
 		__create_xol_area(0);
 
-	area = mm->uprobes_state.xol_area;
-	smp_read_barrier_depends();	/* pairs with wmb in xol_add_vma() */
+	/* Pairs with xol_add_vma() smp_store_release() */
+	area = READ_ONCE(mm->uprobes_state.xol_area); /* ^^^ */
 	return area;
 }
 
@@ -1528,8 +1528,8 @@ static unsigned long get_trampoline_vaddr(void)
 	struct xol_area *area;
 	unsigned long trampoline_vaddr = -1;
 
-	area = current->mm->uprobes_state.xol_area;
-	smp_read_barrier_depends();
+	/* Pairs with xol_add_vma() smp_store_release() */
+	area = READ_ONCE(current->mm->uprobes_state.xol_area); /* ^^^ */
 	if (area)
 		trampoline_vaddr = area->vaddr;
 
-- 
2.5.2

[PATCH RFC tip/core/rcu 10/15] locking: Remove smp_read_barrier_depends() from queued_spin_lock_slowpath()

[Paul E. McKenney]

Queued spinlocks are not used by DEC Alpha, and furthermore operations
such as READ_ONCE() and release/relaxed RMW atomics are being changed
to imply smp_read_barrier_depends().  This commit therefore removes the
now-redundant smp_read_barrier_depends() from queued_spin_lock_slowpath(),
and adjusts the comments accordingly.

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
---
 kernel/locking/qspinlock.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
index 294294c71ba4..38ece035039e 100644
--- a/kernel/locking/qspinlock.c
+++ b/kernel/locking/qspinlock.c
@@ -170,7 +170,7 @@ static __always_inline void clear_pending_set_locked(struct qspinlock *lock)
  * @tail : The new queue tail code word
  * Return: The previous queue tail code word
  *
- * xchg(lock, tail)
+ * xchg(lock, tail), which heads an address dependency
  *
  * p,*,* -> n,*,* ; prev = xchg(lock, node)
  */
@@ -409,13 +409,11 @@ void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)
 	if (old & _Q_TAIL_MASK) {
 		prev = decode_tail(old);
 		/*
-		 * The above xchg_tail() is also a load of @lock which generates,
-		 * through decode_tail(), a pointer.
-		 *
-		 * The address dependency matches the RELEASE of xchg_tail()
-		 * such that the access to @prev must happen after.
+		 * The above xchg_tail() is also a load of @lock which
+		 * generates, through decode_tail(), a pointer.  The address
+		 * dependency matches the RELEASE of xchg_tail() such that
+		 * the subsequent access to @prev happens after.
 		 */
-		smp_read_barrier_depends();
 
 		WRITE_ONCE(prev->next, node);
 
-- 
2.5.2

[PATCH RFC tip/core/rcu 11/15] tracepoint: Remove smp_read_barrier_depends() from comment

[Paul E. McKenney]

The comment in tracepoint_add_func() mentions smp_read_barrier_depends(),
whose use should be quite restricted.  This commit updates the comment
to instead mention the smp_store_release() and rcu_dereference_sched()
that the current code actually uses.

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/tracepoint.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
index 685c50ae6300..671b13457387 100644
--- a/kernel/tracepoint.c
+++ b/kernel/tracepoint.c
@@ -212,11 +212,10 @@ static int tracepoint_add_func(struct tracepoint *tp,
 	}
 
 	/*
-	 * rcu_assign_pointer has a smp_wmb() which makes sure that the new
-	 * probe callbacks array is consistent before setting a pointer to it.
-	 * This array is referenced by __DO_TRACE from
-	 * include/linux/tracepoints.h. A matching smp_read_barrier_depends()
-	 * is used.
+	 * rcu_assign_pointer has as smp_store_release() which makes sure
+	 * that the new probe callbacks array is consistent before setting
+	 * a pointer to it.  This array is referenced by __DO_TRACE from
+	 * include/linux/tracepoint.h using rcu_dereference_sched().
 	 */
 	rcu_assign_pointer(tp->funcs, tp_funcs);
 	if (!static_key_enabled(&tp->key))
-- 
2.5.2

Re: [PATCH RFC tip/core/rcu 11/15] tracepoint: Remove smp_read_barrier_depends() from comment

[Steven Rostedt]

[ added Mathieu ]

On Mon,  9 Oct 2017 17:22:45 -0700
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:

> The comment in tracepoint_add_func() mentions smp_read_barrier_depends(),
> whose use should be quite restricted.  This commit updates the comment
> to instead mention the smp_store_release() and rcu_dereference_sched()
> that the current code actually uses.
> 
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>

Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

> ---
>  kernel/tracepoint.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> index 685c50ae6300..671b13457387 100644
> --- a/kernel/tracepoint.c
> +++ b/kernel/tracepoint.c
> @@ -212,11 +212,10 @@ static int tracepoint_add_func(struct tracepoint *tp,
>  	}
>  
>  	/*
> -	 * rcu_assign_pointer has a smp_wmb() which makes sure that the new
> -	 * probe callbacks array is consistent before setting a pointer to it.
> -	 * This array is referenced by __DO_TRACE from
> -	 * include/linux/tracepoints.h. A matching smp_read_barrier_depends()
> -	 * is used.
> +	 * rcu_assign_pointer has as smp_store_release() which makes sure
> +	 * that the new probe callbacks array is consistent before setting
> +	 * a pointer to it.  This array is referenced by __DO_TRACE from
> +	 * include/linux/tracepoint.h using rcu_dereference_sched().
>  	 */
>  	rcu_assign_pointer(tp->funcs, tp_funcs);
>  	if (!static_key_enabled(&tp->key))

Re: [PATCH RFC tip/core/rcu 11/15] tracepoint: Remove smp_read_barrier_depends() from comment

[Mathieu Desnoyers]

----- On Oct 9, 2017, at 8:31 PM, rostedt rostedt@goodmis.org wrote:

> [ added Mathieu ]
> 
> On Mon,  9 Oct 2017 17:22:45 -0700
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> 
>> The comment in tracepoint_add_func() mentions smp_read_barrier_depends(),
>> whose use should be quite restricted.  This commit updates the comment
>> to instead mention the smp_store_release() and rcu_dereference_sched()
>> that the current code actually uses.
>> 
>> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>> Cc: Ingo Molnar <mingo@kernel.org>
>> Cc: Steven Rostedt <rostedt@goodmis.org>
> 
> Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> 

Yeah I think we don't need to spell out the implementation of
rcu_dereference_sched() there (which actually uses lockless_dereference(),
which indeed ends up relying on smp_read_barrier_depends()). The comment
was a bit too low-level for the benefit of its audience.

I agree with the change.

Acked-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

Thanks,

Mathieu

> -- Steve
> 
>> ---
>>  kernel/tracepoint.c | 9 ++++-----
>>  1 file changed, 4 insertions(+), 5 deletions(-)
>> 
>> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
>> index 685c50ae6300..671b13457387 100644
>> --- a/kernel/tracepoint.c
>> +++ b/kernel/tracepoint.c
>> @@ -212,11 +212,10 @@ static int tracepoint_add_func(struct tracepoint *tp,
>>  	}
>>  
>>  	/*
>> -	 * rcu_assign_pointer has a smp_wmb() which makes sure that the new
>> -	 * probe callbacks array is consistent before setting a pointer to it.
>> -	 * This array is referenced by __DO_TRACE from
>> -	 * include/linux/tracepoints.h. A matching smp_read_barrier_depends()
>> -	 * is used.
>> +	 * rcu_assign_pointer has as smp_store_release() which makes sure
>> +	 * that the new probe callbacks array is consistent before setting
>> +	 * a pointer to it.  This array is referenced by __DO_TRACE from
>> +	 * include/linux/tracepoint.h using rcu_dereference_sched().
>>  	 */
>>  	rcu_assign_pointer(tp->funcs, tp_funcs);
> >  	if (!static_key_enabled(&tp->key))

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

Re: [PATCH RFC tip/core/rcu 11/15] tracepoint: Remove smp_read_barrier_depends() from comment

[Paul E. McKenney]

On Tue, Oct 10, 2017 at 01:12:38AM +0000, Mathieu Desnoyers wrote:
> ----- On Oct 9, 2017, at 8:31 PM, rostedt rostedt@goodmis.org wrote:
> 
> > [ added Mathieu ]
> > 
> > On Mon,  9 Oct 2017 17:22:45 -0700
> > "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> > 
> >> The comment in tracepoint_add_func() mentions smp_read_barrier_depends(),
> >> whose use should be quite restricted.  This commit updates the comment
> >> to instead mention the smp_store_release() and rcu_dereference_sched()
> >> that the current code actually uses.
> >> 
> >> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> >> Cc: Ingo Molnar <mingo@kernel.org>
> >> Cc: Steven Rostedt <rostedt@goodmis.org>
> > 
> > Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > 
> 
> Yeah I think we don't need to spell out the implementation of
> rcu_dereference_sched() there (which actually uses lockless_dereference(),
> which indeed ends up relying on smp_read_barrier_depends()). The comment
> was a bit too low-level for the benefit of its audience.
> 
> I agree with the change.
> 
> Acked-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

Applied, thank you both!

							Thanx, Paul

> Thanks,
> 
> Mathieu
> 
> > -- Steve
> > 
> >> ---
> >>  kernel/tracepoint.c | 9 ++++-----
> >>  1 file changed, 4 insertions(+), 5 deletions(-)
> >> 
> >> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> >> index 685c50ae6300..671b13457387 100644
> >> --- a/kernel/tracepoint.c
> >> +++ b/kernel/tracepoint.c
> >> @@ -212,11 +212,10 @@ static int tracepoint_add_func(struct tracepoint *tp,
> >>  	}
> >>  
> >>  	/*
> >> -	 * rcu_assign_pointer has a smp_wmb() which makes sure that the new
> >> -	 * probe callbacks array is consistent before setting a pointer to it.
> >> -	 * This array is referenced by __DO_TRACE from
> >> -	 * include/linux/tracepoints.h. A matching smp_read_barrier_depends()
> >> -	 * is used.
> >> +	 * rcu_assign_pointer has as smp_store_release() which makes sure
> >> +	 * that the new probe callbacks array is consistent before setting
> >> +	 * a pointer to it.  This array is referenced by __DO_TRACE from
> >> +	 * include/linux/tracepoint.h using rcu_dereference_sched().
> >>  	 */
> >>  	rcu_assign_pointer(tp->funcs, tp_funcs);
> > >  	if (!static_key_enabled(&tp->key))
> 
> -- 
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com
> 

[PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[Paul E. McKenney]

Now that smp_read_barrier_depends() is implied by READ_ONCE(), adding
READ_ONCE() to assoc_array_ptr_to_leaf() and __assoc_array_ptr_to_meta()
allows the several smp_read_barrier_depends() calls to be removed from
lib/assoc_array.c.  This commit makes this change.

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Kuleshov <kuleshovmail@gmail.com>
---
 include/linux/assoc_array_priv.h |  5 +++--
 lib/assoc_array.c                | 20 ++------------------
 2 files changed, 5 insertions(+), 20 deletions(-)

diff --git a/include/linux/assoc_array_priv.h b/include/linux/assoc_array_priv.h
index 711275e6681c..b47308bf1f99 100644
--- a/include/linux/assoc_array_priv.h
+++ b/include/linux/assoc_array_priv.h
@@ -135,13 +135,14 @@ static inline bool assoc_array_ptr_is_node(const struct assoc_array_ptr *x)
 
 static inline void *assoc_array_ptr_to_leaf(const struct assoc_array_ptr *x)
 {
-	return (void *)((unsigned long)x & ~ASSOC_ARRAY_PTR_TYPE_MASK);
+	return (void *)((unsigned long)READ_ONCE(x) & /* Address dependency. */
+		~ASSOC_ARRAY_PTR_TYPE_MASK);
 }
 
 static inline
 unsigned long __assoc_array_ptr_to_meta(const struct assoc_array_ptr *x)
 {
-	return (unsigned long)x &
+	return (unsigned long)READ_ONCE(x) & /* Address dependency. */
 		~(ASSOC_ARRAY_PTR_SUBTYPE_MASK | ASSOC_ARRAY_PTR_TYPE_MASK);
 }
 static inline
diff --git a/lib/assoc_array.c b/lib/assoc_array.c
index fe7953aead82..52845f5cbf19 100644
--- a/lib/assoc_array.c
+++ b/lib/assoc_array.c
@@ -38,12 +38,10 @@ static int assoc_array_subtree_iterate(const struct assoc_array_ptr *root,
 	if (assoc_array_ptr_is_shortcut(cursor)) {
 		/* Descend through a shortcut */
 		shortcut = assoc_array_ptr_to_shortcut(cursor);
-		smp_read_barrier_depends();
 		cursor = READ_ONCE(shortcut->next_node);
 	}
 
 	node = assoc_array_ptr_to_node(cursor);
-	smp_read_barrier_depends();
 	slot = 0;
 
 	/* We perform two passes of each node.
@@ -55,15 +53,9 @@ static int assoc_array_subtree_iterate(const struct assoc_array_ptr *root,
 	 */
 	has_meta = 0;
 	for (; slot < ASSOC_ARRAY_FAN_OUT; slot++) {
-		ptr = READ_ONCE(node->slots[slot]);
+		ptr = READ_ONCE(node->slots[slot]); /* Address dependency. */
 		has_meta |= (unsigned long)ptr;
 		if (ptr && assoc_array_ptr_is_leaf(ptr)) {
-			/* We need a barrier between the read of the pointer
-			 * and dereferencing the pointer - but only if we are
-			 * actually going to dereference it.
-			 */
-			smp_read_barrier_depends();
-
 			/* Invoke the callback */
 			ret = iterator(assoc_array_ptr_to_leaf(ptr),
 				       iterator_data);
@@ -86,8 +78,6 @@ static int assoc_array_subtree_iterate(const struct assoc_array_ptr *root,
 
 continue_node:
 	node = assoc_array_ptr_to_node(cursor);
-	smp_read_barrier_depends();
-
 	for (; slot < ASSOC_ARRAY_FAN_OUT; slot++) {
 		ptr = READ_ONCE(node->slots[slot]);
 		if (assoc_array_ptr_is_meta(ptr)) {
@@ -105,7 +95,6 @@ static int assoc_array_subtree_iterate(const struct assoc_array_ptr *root,
 
 	if (assoc_array_ptr_is_shortcut(parent)) {
 		shortcut = assoc_array_ptr_to_shortcut(parent);
-		smp_read_barrier_depends();
 		cursor = parent;
 		parent = READ_ONCE(shortcut->back_pointer);
 		slot = shortcut->parent_slot;
@@ -216,8 +205,6 @@ assoc_array_walk(const struct assoc_array *array,
 
 consider_node:
 	node = assoc_array_ptr_to_node(cursor);
-	smp_read_barrier_depends();
-
 	slot = segments >> (level & ASSOC_ARRAY_KEY_CHUNK_MASK);
 	slot &= ASSOC_ARRAY_FAN_MASK;
 	ptr = READ_ONCE(node->slots[slot]);
@@ -254,7 +241,6 @@ assoc_array_walk(const struct assoc_array *array,
 	cursor = ptr;
 follow_shortcut:
 	shortcut = assoc_array_ptr_to_shortcut(cursor);
-	smp_read_barrier_depends();
 	pr_devel("shortcut to %d\n", shortcut->skip_to_level);
 	sc_level = level + ASSOC_ARRAY_LEVEL_STEP;
 	BUG_ON(sc_level > shortcut->skip_to_level);
@@ -330,8 +316,7 @@ void *assoc_array_find(const struct assoc_array *array,
 	    assoc_array_walk_found_terminal_node)
 		return NULL;
 
-	node = result.terminal_node.node;
-	smp_read_barrier_depends();
+	node = READ_ONCE(result.terminal_node.node);  /* Address dependency. */
 
 	/* If the target key is available to us, it's has to be pointed to by
 	 * the terminal node.
@@ -344,7 +329,6 @@ void *assoc_array_find(const struct assoc_array *array,
 			 * actually going to dereference it.
 			 */
 			leaf = assoc_array_ptr_to_leaf(ptr);
-			smp_read_barrier_depends();
 			if (ops->compare_object(leaf, index_key))
 				return (void *)leaf;
 		}
-- 
2.5.2

Re: [PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[Peter Zijlstra]

On Mon, Oct 09, 2017 at 05:22:46PM -0700, Paul E. McKenney wrote:
> Now that smp_read_barrier_depends() is implied by READ_ONCE(), adding
> READ_ONCE() to assoc_array_ptr_to_leaf() and __assoc_array_ptr_to_meta()
> allows the several smp_read_barrier_depends() calls to be removed from
> lib/assoc_array.c.  This commit makes this change.

So arguably this code was broken for not already having READ_ONCE().

Re: [PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[David Howells]

Peter Zijlstra <peterz@infradead.org> wrote:

> > Now that smp_read_barrier_depends() is implied by READ_ONCE(), adding
> > READ_ONCE() to assoc_array_ptr_to_leaf() and __assoc_array_ptr_to_meta()
> > allows the several smp_read_barrier_depends() calls to be removed from
> > lib/assoc_array.c.  This commit makes this change.
> 
> So arguably this code was broken for not already having READ_ONCE().

No, the code is right; this is the wrong place to use READ_ONCE().  The
callers already call READ_ONCE() or ACCESS_ONCE().

David

[PATCH RFC tip/core/rcu 13/15] mm/ksm: Remove now-redundant smp_read_barrier_depends()

[Paul E. McKenney]

Because READ_ONCE() now implies smp_read_barrier_depends(), the
smp_read_barrier_depends() in get_ksm_page() is now redundant.
This commit removes it and updates the comments.

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andrea Arcangeli <aarcange@redhat.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Michal Hocko <mhocko@suse.com>
Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
Cc: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
Cc: <linux-mm@kvack.org>
---
 mm/ksm.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/mm/ksm.c b/mm/ksm.c
index 6cb60f46cce5..7d70f923bc45 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -675,15 +675,8 @@ static struct page *get_ksm_page(struct stable_node *stable_node, bool lock_it)
 	expected_mapping = (void *)((unsigned long)stable_node |
 					PAGE_MAPPING_KSM);
 again:
-	kpfn = READ_ONCE(stable_node->kpfn);
+	kpfn = READ_ONCE(stable_node->kpfn); /* Address dependency. */
 	page = pfn_to_page(kpfn);
-
-	/*
-	 * page is computed from kpfn, so on most architectures reading
-	 * page->mapping is naturally ordered after reading node->kpfn,
-	 * but on Alpha we need to be more careful.
-	 */
-	smp_read_barrier_depends();
 	if (READ_ONCE(page->mapping) != expected_mapping)
 		goto stale;
 
-- 
2.5.2

[PATCH RFC tip/core/rcu 13/15] mm/ksm: Remove now-redundant smp_read_barrier_depends()

[Paul E. McKenney]

Because READ_ONCE() now implies smp_read_barrier_depends(), the
smp_read_barrier_depends() in get_ksm_page() is now redundant.
This commit removes it and updates the comments.

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andrea Arcangeli <aarcange@redhat.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Michal Hocko <mhocko@suse.com>
Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
Cc: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
Cc: <linux-mm@kvack.org>
---
 mm/ksm.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/mm/ksm.c b/mm/ksm.c
index 6cb60f46cce5..7d70f923bc45 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -675,15 +675,8 @@ static struct page *get_ksm_page(struct stable_node *stable_node, bool lock_it)
 	expected_mapping = (void *)((unsigned long)stable_node |
 					PAGE_MAPPING_KSM);
 again:
-	kpfn = READ_ONCE(stable_node->kpfn);
+	kpfn = READ_ONCE(stable_node->kpfn); /* Address dependency. */
 	page = pfn_to_page(kpfn);
-
-	/*
-	 * page is computed from kpfn, so on most architectures reading
-	 * page->mapping is naturally ordered after reading node->kpfn,
-	 * but on Alpha we need to be more careful.
-	 */
-	smp_read_barrier_depends();
 	if (READ_ONCE(page->mapping) != expected_mapping)
 		goto stale;
 
-- 
2.5.2

[PATCH RFC tip/core/rcu 13/15] mm/ksm: Remove now-redundant smp_read_barrier_depends()

[Paul E. McKenney]

Because READ_ONCE() now implies smp_read_barrier_depends(), the
smp_read_barrier_depends() in get_ksm_page() is now redundant.
This commit removes it and updates the comments.

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andrea Arcangeli <aarcange@redhat.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Michal Hocko <mhocko@suse.com>
Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
Cc: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
Cc: <linux-mm@kvack.org>
---
 mm/ksm.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/mm/ksm.c b/mm/ksm.c
index 6cb60f46cce5..7d70f923bc45 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -675,15 +675,8 @@ static struct page *get_ksm_page(struct stable_node *stable_node, bool lock_it)
 	expected_mapping = (void *)((unsigned long)stable_node |
 					PAGE_MAPPING_KSM);
 again:
-	kpfn = READ_ONCE(stable_node->kpfn);
+	kpfn = READ_ONCE(stable_node->kpfn); /* Address dependency. */
 	page = pfn_to_page(kpfn);
-
-	/*
-	 * page is computed from kpfn, so on most architectures reading
-	 * page->mapping is naturally ordered after reading node->kpfn,
-	 * but on Alpha we need to be more careful.
-	 */
-	smp_read_barrier_depends();
 	if (READ_ONCE(page->mapping) != expected_mapping)
 		goto stale;
 
-- 
2.5.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH RFC tip/core/rcu 14/15] netfilter: Remove now-redundant smp_read_barrier_depends()

[Paul E. McKenney]

READ_ONCE() now implies smp_read_barrier_depends(), which means that
the instances in arpt_do_table(), ipt_do_table(), and ip6t_do_table()
are now redundant.  This commit removes them and adjusts the comments.

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Pablo Neira Ayuso <pablo@netfilter.org>
Cc: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
Cc: Florian Westphal <fw@strlen.de>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: <netfilter-devel@vger.kernel.org>
Cc: <coreteam@netfilter.org>
Cc: <netdev@vger.kernel.org>
---
 net/ipv4/netfilter/arp_tables.c | 7 +------
 net/ipv4/netfilter/ip_tables.c  | 7 +------
 net/ipv6/netfilter/ip6_tables.c | 7 +------
 3 files changed, 3 insertions(+), 18 deletions(-)

diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index 9e2770fd00be..d555b3b31c49 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -202,13 +202,8 @@ unsigned int arpt_do_table(struct sk_buff *skb,
 
 	local_bh_disable();
 	addend = xt_write_recseq_begin();
-	private = table->private;
+	private = READ_ONCE(table->private); /* Address dependency. */
 	cpu     = smp_processor_id();
-	/*
-	 * Ensure we load private-> members after we've fetched the base
-	 * pointer.
-	 */
-	smp_read_barrier_depends();
 	table_base = private->entries;
 	jumpstack  = (struct arpt_entry **)private->jumpstack[cpu];
 
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index 39286e543ee6..f63752bec442 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -260,13 +260,8 @@ ipt_do_table(struct sk_buff *skb,
 	WARN_ON(!(table->valid_hooks & (1 << hook)));
 	local_bh_disable();
 	addend = xt_write_recseq_begin();
-	private = table->private;
+	private = READ_ONCE(table->private); /* Address dependency. */
 	cpu        = smp_processor_id();
-	/*
-	 * Ensure we load private-> members after we've fetched the base
-	 * pointer.
-	 */
-	smp_read_barrier_depends();
 	table_base = private->entries;
 	jumpstack  = (struct ipt_entry **)private->jumpstack[cpu];
 
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index 01bd3ee5ebc6..52afcab9b0d6 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -282,12 +282,7 @@ ip6t_do_table(struct sk_buff *skb,
 
 	local_bh_disable();
 	addend = xt_write_recseq_begin();
-	private = table->private;
-	/*
-	 * Ensure we load private-> members after we've fetched the base
-	 * pointer.
-	 */
-	smp_read_barrier_depends();
+	private = READ_ONCE(table->private); /* Address dependency. */
 	cpu        = smp_processor_id();
 	table_base = private->entries;
 	jumpstack  = (struct ip6t_entry **)private->jumpstack[cpu];
-- 
2.5.2

[PATCH RFC tip/core/rcu 14/15] netfilter: Remove now-redundant smp_read_barrier_depends()

[Paul E. McKenney]

READ_ONCE() now implies smp_read_barrier_depends(), which means that
the instances in arpt_do_table(), ipt_do_table(), and ip6t_do_table()
are now redundant.  This commit removes them and adjusts the comments.

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Pablo Neira Ayuso <pablo@netfilter.org>
Cc: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
Cc: Florian Westphal <fw@strlen.de>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: <netfilter-devel@vger.kernel.org>
Cc: <coreteam@netfilter.org>
Cc: <netdev@vger.kernel.org>
---
 net/ipv4/netfilter/arp_tables.c | 7 +------
 net/ipv4/netfilter/ip_tables.c  | 7 +------
 net/ipv6/netfilter/ip6_tables.c | 7 +------
 3 files changed, 3 insertions(+), 18 deletions(-)

diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index 9e2770fd00be..d555b3b31c49 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -202,13 +202,8 @@ unsigned int arpt_do_table(struct sk_buff *skb,
 
 	local_bh_disable();
 	addend = xt_write_recseq_begin();
-	private = table->private;
+	private = READ_ONCE(table->private); /* Address dependency. */
 	cpu     = smp_processor_id();
-	/*
-	 * Ensure we load private-> members after we've fetched the base
-	 * pointer.
-	 */
-	smp_read_barrier_depends();
 	table_base = private->entries;
 	jumpstack  = (struct arpt_entry **)private->jumpstack[cpu];
 
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index 39286e543ee6..f63752bec442 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -260,13 +260,8 @@ ipt_do_table(struct sk_buff *skb,
 	WARN_ON(!(table->valid_hooks & (1 << hook)));
 	local_bh_disable();
 	addend = xt_write_recseq_begin();
-	private = table->private;
+	private = READ_ONCE(table->private); /* Address dependency. */
 	cpu        = smp_processor_id();
-	/*
-	 * Ensure we load private-> members after we've fetched the base
-	 * pointer.
-	 */
-	smp_read_barrier_depends();
 	table_base = private->entries;
 	jumpstack  = (struct ipt_entry **)private->jumpstack[cpu];
 
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index 01bd3ee5ebc6..52afcab9b0d6 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -282,12 +282,7 @@ ip6t_do_table(struct sk_buff *skb,
 
 	local_bh_disable();
 	addend = xt_write_recseq_begin();
-	private = table->private;
-	/*
-	 * Ensure we load private-> members after we've fetched the base
-	 * pointer.
-	 */
-	smp_read_barrier_depends();
+	private = READ_ONCE(table->private); /* Address dependency. */
 	cpu        = smp_processor_id();
 	table_base = private->entries;
 	jumpstack  = (struct ip6t_entry **)private->jumpstack[cpu];
-- 
2.5.2

Re: [PATCH RFC tip/core/rcu 14/15] netfilter: Remove now-redundant smp_read_barrier_depends()

[Peter Zijlstra]

On Mon, Oct 09, 2017 at 05:22:48PM -0700, Paul E. McKenney wrote:
> READ_ONCE() now implies smp_read_barrier_depends(), which means that
> the instances in arpt_do_table(), ipt_do_table(), and ip6t_do_table()
> are now redundant.  This commit removes them and adjusts the comments.

Similar to the previous patch, the lack of READ_ONCE() in the original
code is a pre-existing bug. It would allow the compiler to tear the load
and observe a composite of two difference pointer values, or reload the
private pointer and result in table_base and jumpstacl being part of
different objects.

It would be good to point out this actually fixes a bug in the code.

Re: [PATCH RFC tip/core/rcu 14/15] netfilter: Remove now-redundant smp_read_barrier_depends()

[Paul E. McKenney]

On Tue, Oct 10, 2017 at 10:43:34AM +0200, Peter Zijlstra wrote:
> On Mon, Oct 09, 2017 at 05:22:48PM -0700, Paul E. McKenney wrote:
> > READ_ONCE() now implies smp_read_barrier_depends(), which means that
> > the instances in arpt_do_table(), ipt_do_table(), and ip6t_do_table()
> > are now redundant.  This commit removes them and adjusts the comments.
> 
> Similar to the previous patch, the lack of READ_ONCE() in the original
> code is a pre-existing bug. It would allow the compiler to tear the load
> and observe a composite of two difference pointer values, or reload the
> private pointer and result in table_base and jumpstacl being part of
> different objects.
> 
> It would be good to point out this actually fixes a bug in the code.

Assuming that these changes actually fixed something, agreed.  ;-)

							Thanx, Paul

[PATCH RFC tip/core/rcu 15/15] keyring: Remove now-redundant smp_read_barrier_depends()

[Paul E. McKenney]

Now that the associative-array library properly heads dependency chains,
the various smp_read_barrier_depends() calls in security/keys/keyring.c
are no longer needed.  This commit therefore removes them.

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: David Howells <dhowells@redhat.com>
Cc: James Morris <james.l.morris@oracle.com>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: <keyrings@vger.kernel.org>
Cc: <linux-security-module@vger.kernel.org>
---
 security/keys/keyring.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 4fa82a8a9c0e..fb44c9169125 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -712,7 +712,6 @@ static bool search_nested_keyrings(struct key *keyring,
 		 * doesn't contain any keyring pointers.
 		 */
 		shortcut = assoc_array_ptr_to_shortcut(ptr);
-		smp_read_barrier_depends();
 		if ((shortcut->index_key[0] & ASSOC_ARRAY_FAN_MASK) != 0)
 			goto not_this_keyring;
 
@@ -722,8 +721,6 @@ static bool search_nested_keyrings(struct key *keyring,
 	}
 
 	node = assoc_array_ptr_to_node(ptr);
-	smp_read_barrier_depends();
-
 	ptr = node->slots[0];
 	if (!assoc_array_ptr_is_meta(ptr))
 		goto begin_node;
@@ -735,7 +732,6 @@ static bool search_nested_keyrings(struct key *keyring,
 	kdebug("descend");
 	if (assoc_array_ptr_is_shortcut(ptr)) {
 		shortcut = assoc_array_ptr_to_shortcut(ptr);
-		smp_read_barrier_depends();
 		ptr = READ_ONCE(shortcut->next_node);
 		BUG_ON(!assoc_array_ptr_is_node(ptr));
 	}
@@ -743,7 +739,6 @@ static bool search_nested_keyrings(struct key *keyring,
 
 begin_node:
 	kdebug("begin_node");
-	smp_read_barrier_depends();
 	slot = 0;
 ascend_to_node:
 	/* Go through the slots in a node */
@@ -791,14 +786,12 @@ static bool search_nested_keyrings(struct key *keyring,
 
 	if (ptr && assoc_array_ptr_is_shortcut(ptr)) {
 		shortcut = assoc_array_ptr_to_shortcut(ptr);
-		smp_read_barrier_depends();
 		ptr = READ_ONCE(shortcut->back_pointer);
 		slot = shortcut->parent_slot;
 	}
 	if (!ptr)
 		goto not_this_keyring;
 	node = assoc_array_ptr_to_node(ptr);
-	smp_read_barrier_depends();
 	slot++;
 
 	/* If we've ascended to the root (zero backpointer), we must have just
-- 
2.5.2

[PATCH RFC tip/core/rcu 15/15] keyring: Remove now-redundant smp_read_barrier_depends()

[Paul E. McKenney]

Now that the associative-array library properly heads dependency chains,
the various smp_read_barrier_depends() calls in security/keys/keyring.c
are no longer needed.  This commit therefore removes them.

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: David Howells <dhowells@redhat.com>
Cc: James Morris <james.l.morris@oracle.com>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: <keyrings@vger.kernel.org>
Cc: <linux-security-module@vger.kernel.org>
---
 security/keys/keyring.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 4fa82a8a9c0e..fb44c9169125 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -712,7 +712,6 @@ static bool search_nested_keyrings(struct key *keyring,
 		 * doesn't contain any keyring pointers.
 		 */
 		shortcut = assoc_array_ptr_to_shortcut(ptr);
-		smp_read_barrier_depends();
 		if ((shortcut->index_key[0] & ASSOC_ARRAY_FAN_MASK) != 0)
 			goto not_this_keyring;
 
@@ -722,8 +721,6 @@ static bool search_nested_keyrings(struct key *keyring,
 	}
 
 	node = assoc_array_ptr_to_node(ptr);
-	smp_read_barrier_depends();
-
 	ptr = node->slots[0];
 	if (!assoc_array_ptr_is_meta(ptr))
 		goto begin_node;
@@ -735,7 +732,6 @@ static bool search_nested_keyrings(struct key *keyring,
 	kdebug("descend");
 	if (assoc_array_ptr_is_shortcut(ptr)) {
 		shortcut = assoc_array_ptr_to_shortcut(ptr);
-		smp_read_barrier_depends();
 		ptr = READ_ONCE(shortcut->next_node);
 		BUG_ON(!assoc_array_ptr_is_node(ptr));
 	}
@@ -743,7 +739,6 @@ static bool search_nested_keyrings(struct key *keyring,
 
 begin_node:
 	kdebug("begin_node");
-	smp_read_barrier_depends();
 	slot = 0;
 ascend_to_node:
 	/* Go through the slots in a node */
@@ -791,14 +786,12 @@ static bool search_nested_keyrings(struct key *keyring,
 
 	if (ptr && assoc_array_ptr_is_shortcut(ptr)) {
 		shortcut = assoc_array_ptr_to_shortcut(ptr);
-		smp_read_barrier_depends();
 		ptr = READ_ONCE(shortcut->back_pointer);
 		slot = shortcut->parent_slot;
 	}
 	if (!ptr)
 		goto not_this_keyring;
 	node = assoc_array_ptr_to_node(ptr);
-	smp_read_barrier_depends();
 	slot++;
 
 	/* If we've ascended to the root (zero backpointer), we must have just
-- 
2.5.2

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RFC tip/core/rcu 15/15] keyring: Remove now-redundant smp_read_barrier_depends()

[Paul E. McKenney]

Now that the associative-array library properly heads dependency chains,
the various smp_read_barrier_depends() calls in security/keys/keyring.c
are no longer needed.  This commit therefore removes them.

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: David Howells <dhowells@redhat.com>
Cc: James Morris <james.l.morris@oracle.com>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: <keyrings@vger.kernel.org>
Cc: <linux-security-module@vger.kernel.org>
---
 security/keys/keyring.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 4fa82a8a9c0e..fb44c9169125 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -712,7 +712,6 @@ static bool search_nested_keyrings(struct key *keyring,
 		 * doesn't contain any keyring pointers.
 		 */
 		shortcut = assoc_array_ptr_to_shortcut(ptr);
-		smp_read_barrier_depends();
 		if ((shortcut->index_key[0] & ASSOC_ARRAY_FAN_MASK) != 0)
 			goto not_this_keyring;
 
@@ -722,8 +721,6 @@ static bool search_nested_keyrings(struct key *keyring,
 	}
 
 	node = assoc_array_ptr_to_node(ptr);
-	smp_read_barrier_depends();
-
 	ptr = node->slots[0];
 	if (!assoc_array_ptr_is_meta(ptr))
 		goto begin_node;
@@ -735,7 +732,6 @@ static bool search_nested_keyrings(struct key *keyring,
 	kdebug("descend");
 	if (assoc_array_ptr_is_shortcut(ptr)) {
 		shortcut = assoc_array_ptr_to_shortcut(ptr);
-		smp_read_barrier_depends();
 		ptr = READ_ONCE(shortcut->next_node);
 		BUG_ON(!assoc_array_ptr_is_node(ptr));
 	}
@@ -743,7 +739,6 @@ static bool search_nested_keyrings(struct key *keyring,
 
 begin_node:
 	kdebug("begin_node");
-	smp_read_barrier_depends();
 	slot = 0;
 ascend_to_node:
 	/* Go through the slots in a node */
@@ -791,14 +786,12 @@ static bool search_nested_keyrings(struct key *keyring,
 
 	if (ptr && assoc_array_ptr_is_shortcut(ptr)) {
 		shortcut = assoc_array_ptr_to_shortcut(ptr);
-		smp_read_barrier_depends();
 		ptr = READ_ONCE(shortcut->back_pointer);
 		slot = shortcut->parent_slot;
 	}
 	if (!ptr)
 		goto not_this_keyring;
 	node = assoc_array_ptr_to_node(ptr);
-	smp_read_barrier_depends();
 	slot++;
 
 	/* If we've ascended to the root (zero backpointer), we must have just
-- 
2.5.2

[PATCH RFC tip/core/rcu 15/15] keyring: Remove now-redundant smp_read_barrier_depends()

[Paul E. McKenney]

Now that the associative-array library properly heads dependency chains,
the various smp_read_barrier_depends() calls in security/keys/keyring.c
are no longer needed.  This commit therefore removes them.

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: David Howells <dhowells@redhat.com>
Cc: James Morris <james.l.morris@oracle.com>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: <keyrings@vger.kernel.org>
Cc: <linux-security-module@vger.kernel.org>
---
 security/keys/keyring.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 4fa82a8a9c0e..fb44c9169125 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -712,7 +712,6 @@ static bool search_nested_keyrings(struct key *keyring,
 		 * doesn't contain any keyring pointers.
 		 */
 		shortcut = assoc_array_ptr_to_shortcut(ptr);
-		smp_read_barrier_depends();
 		if ((shortcut->index_key[0] & ASSOC_ARRAY_FAN_MASK) != 0)
 			goto not_this_keyring;
 
@@ -722,8 +721,6 @@ static bool search_nested_keyrings(struct key *keyring,
 	}
 
 	node = assoc_array_ptr_to_node(ptr);
-	smp_read_barrier_depends();
-
 	ptr = node->slots[0];
 	if (!assoc_array_ptr_is_meta(ptr))
 		goto begin_node;
@@ -735,7 +732,6 @@ static bool search_nested_keyrings(struct key *keyring,
 	kdebug("descend");
 	if (assoc_array_ptr_is_shortcut(ptr)) {
 		shortcut = assoc_array_ptr_to_shortcut(ptr);
-		smp_read_barrier_depends();
 		ptr = READ_ONCE(shortcut->next_node);
 		BUG_ON(!assoc_array_ptr_is_node(ptr));
 	}
@@ -743,7 +739,6 @@ static bool search_nested_keyrings(struct key *keyring,
 
 begin_node:
 	kdebug("begin_node");
-	smp_read_barrier_depends();
 	slot = 0;
 ascend_to_node:
 	/* Go through the slots in a node */
@@ -791,14 +786,12 @@ static bool search_nested_keyrings(struct key *keyring,
 
 	if (ptr && assoc_array_ptr_is_shortcut(ptr)) {
 		shortcut = assoc_array_ptr_to_shortcut(ptr);
-		smp_read_barrier_depends();
 		ptr = READ_ONCE(shortcut->back_pointer);
 		slot = shortcut->parent_slot;
 	}
 	if (!ptr)
 		goto not_this_keyring;
 	node = assoc_array_ptr_to_node(ptr);
-	smp_read_barrier_depends();
 	slot++;
 
 	/* If we've ascended to the root (zero backpointer), we must have just
-- 
2.5.2

Re: [PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[David Howells]

Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:

>  static inline void *assoc_array_ptr_to_leaf(const struct assoc_array_ptr *x)
>  {
> -	return (void *)((unsigned long)x & ~ASSOC_ARRAY_PTR_TYPE_MASK);
> +	return (void *)((unsigned long)READ_ONCE(x) & /* Address dependency. */
> +		~ASSOC_ARRAY_PTR_TYPE_MASK);
>  }

This is the wrong place to do this.  assoc_array_ptr_to_leaf() is effectively
no more than a special cast; it removes a metadata bit from a pointer.  x is
the value we're modifying, not *x, and x was read by the caller.

>  static inline
>  unsigned long __assoc_array_ptr_to_meta(const struct assoc_array_ptr *x)
>  {
> -	return (unsigned long)x &
> +	return (unsigned long)READ_ONCE(x) & /* Address dependency. */
>  		~(ASSOC_ARRAY_PTR_SUBTYPE_MASK | ASSOC_ARRAY_PTR_TYPE_MASK);
>  }

Ditto.

> -		ptr = READ_ONCE(node->slots[slot]);
> +		ptr = READ_ONCE(node->slots[slot]); /* Address dependency. */
>  		has_meta |= (unsigned long)ptr;
>  		if (ptr && assoc_array_ptr_is_leaf(ptr)) {
> -			/* We need a barrier between the read of the pointer
> -			 * and dereferencing the pointer - but only if we are
> -			 * actually going to dereference it.
> -			 */
> -			smp_read_barrier_depends();
> -

For example, you can see the READ_ONCE() here; that is already done.

Can you also not just delete the comment, but rephrase it to indicate that a
barrier is necessary and it's done by READ_ONCE(), please?

David

Re: [PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[Paul E. McKenney]

On Tue, Oct 10, 2017 at 10:35:46AM +0100, David Howells wrote:
> Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> 
> >  static inline void *assoc_array_ptr_to_leaf(const struct assoc_array_ptr *x)
> >  {
> > -	return (void *)((unsigned long)x & ~ASSOC_ARRAY_PTR_TYPE_MASK);
> > +	return (void *)((unsigned long)READ_ONCE(x) & /* Address dependency. */
> > +		~ASSOC_ARRAY_PTR_TYPE_MASK);
> >  }
> 
> This is the wrong place to do this.  assoc_array_ptr_to_leaf() is effectively
> no more than a special cast; it removes a metadata bit from a pointer.  x is
> the value we're modifying, not *x, and x was read by the caller.

OK, so in at least one of these cases, the barrier is already provided
by the ACCESS_ONCE(array->root) in assoc_array_walk().  I took a quick
look at the other instances of ->root and convinced myself that they
were update-side accesses, but please let me know if I am missing
something.

I marked the ACCESS_ONCE() instances that seemed to me to be heading
address-dependency chains, please let me know how I did.  ;-)

> >  static inline
> >  unsigned long __assoc_array_ptr_to_meta(const struct assoc_array_ptr *x)
> >  {
> > -	return (unsigned long)x &
> > +	return (unsigned long)READ_ONCE(x) & /* Address dependency. */
> >  		~(ASSOC_ARRAY_PTR_SUBTYPE_MASK | ASSOC_ARRAY_PTR_TYPE_MASK);
> >  }
> 
> Ditto.
> 
> > -		ptr = READ_ONCE(node->slots[slot]);
> > +		ptr = READ_ONCE(node->slots[slot]); /* Address dependency. */
> >  		has_meta |= (unsigned long)ptr;
> >  		if (ptr && assoc_array_ptr_is_leaf(ptr)) {
> > -			/* We need a barrier between the read of the pointer
> > -			 * and dereferencing the pointer - but only if we are
> > -			 * actually going to dereference it.
> > -			 */
> > -			smp_read_barrier_depends();
> > -
> 
> For example, you can see the READ_ONCE() here; that is already done.
> 
> Can you also not just delete the comment, but rephrase it to indicate that a
> barrier is necessary and it's done by READ_ONCE(), please?

So, is the patch below at least somewhat less confused?  ;-)

							Thanx, Paul

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

commit a25170d6998c587b296c75be09b49b5e31bc515b
Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Date:   Mon Oct 9 11:39:57 2017 -0700

    lib/assoc_array: Remove smp_read_barrier_depends()
    
    Now that smp_read_barrier_depends() is implied by READ_ONCE(), adding
    READ_ONCE() to assoc_array_ptr_to_leaf() and __assoc_array_ptr_to_meta()
    allows the several smp_read_barrier_depends() calls to be removed from
    lib/assoc_array.c.  This commit makes this change.
    
    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
    Cc: Jonathan Corbet <corbet@lwn.net>
    Cc: Mark Rutland <mark.rutland@arm.com>
    Cc: Alexander Kuleshov <kuleshovmail@gmail.com>

diff --git a/lib/assoc_array.c b/lib/assoc_array.c
index fe7953aead82..7e483294f7d7 100644
--- a/lib/assoc_array.c
+++ b/lib/assoc_array.c
@@ -38,12 +38,10 @@ static int assoc_array_subtree_iterate(const struct assoc_array_ptr *root,
 	if (assoc_array_ptr_is_shortcut(cursor)) {
 		/* Descend through a shortcut */
 		shortcut = assoc_array_ptr_to_shortcut(cursor);
-		smp_read_barrier_depends();
-		cursor = READ_ONCE(shortcut->next_node);
+		cursor = READ_ONCE(shortcut->next_node); /* Address dependency. */
 	}
 
 	node = assoc_array_ptr_to_node(cursor);
-	smp_read_barrier_depends();
 	slot = 0;
 
 	/* We perform two passes of each node.
@@ -55,15 +53,12 @@ static int assoc_array_subtree_iterate(const struct assoc_array_ptr *root,
 	 */
 	has_meta = 0;
 	for (; slot < ASSOC_ARRAY_FAN_OUT; slot++) {
-		ptr = READ_ONCE(node->slots[slot]);
+		ptr = READ_ONCE(node->slots[slot]); /* Address dependency. */
 		has_meta |= (unsigned long)ptr;
 		if (ptr && assoc_array_ptr_is_leaf(ptr)) {
-			/* We need a barrier between the read of the pointer
-			 * and dereferencing the pointer - but only if we are
-			 * actually going to dereference it.
+			/* We need a barrier between the read of the pointer,
+			 * which is supplied by the above READ_ONCE().
 			 */
-			smp_read_barrier_depends();
-
 			/* Invoke the callback */
 			ret = iterator(assoc_array_ptr_to_leaf(ptr),
 				       iterator_data);
@@ -86,10 +81,8 @@ static int assoc_array_subtree_iterate(const struct assoc_array_ptr *root,
 
 continue_node:
 	node = assoc_array_ptr_to_node(cursor);
-	smp_read_barrier_depends();
-
 	for (; slot < ASSOC_ARRAY_FAN_OUT; slot++) {
-		ptr = READ_ONCE(node->slots[slot]);
+		ptr = READ_ONCE(node->slots[slot]); /* Address dependency. */
 		if (assoc_array_ptr_is_meta(ptr)) {
 			cursor = ptr;
 			goto begin_node;
@@ -98,16 +91,15 @@ static int assoc_array_subtree_iterate(const struct assoc_array_ptr *root,
 
 finished_node:
 	/* Move up to the parent (may need to skip back over a shortcut) */
-	parent = READ_ONCE(node->back_pointer);
+	parent = READ_ONCE(node->back_pointer); /* Address dependency. */
 	slot = node->parent_slot;
 	if (parent == stop)
 		return 0;
 
 	if (assoc_array_ptr_is_shortcut(parent)) {
 		shortcut = assoc_array_ptr_to_shortcut(parent);
-		smp_read_barrier_depends();
 		cursor = parent;
-		parent = READ_ONCE(shortcut->back_pointer);
+		parent = READ_ONCE(shortcut->back_pointer); /* Address dependency. */
 		slot = shortcut->parent_slot;
 		if (parent == stop)
 			return 0;
@@ -147,7 +139,7 @@ int assoc_array_iterate(const struct assoc_array *array,
 					void *iterator_data),
 			void *iterator_data)
 {
-	struct assoc_array_ptr *root = READ_ONCE(array->root);
+	struct assoc_array_ptr *root = READ_ONCE(array->root); /* Address dependency. */
 
 	if (!root)
 		return 0;
@@ -194,7 +186,7 @@ assoc_array_walk(const struct assoc_array *array,
 
 	pr_devel("-->%s()\n", __func__);
 
-	cursor = READ_ONCE(array->root);
+	cursor = READ_ONCE(array->root);  /* Address dependency. */
 	if (!cursor)
 		return assoc_array_walk_tree_empty;
 
@@ -216,11 +208,9 @@ assoc_array_walk(const struct assoc_array *array,
 
 consider_node:
 	node = assoc_array_ptr_to_node(cursor);
-	smp_read_barrier_depends();
-
 	slot = segments >> (level & ASSOC_ARRAY_KEY_CHUNK_MASK);
 	slot &= ASSOC_ARRAY_FAN_MASK;
-	ptr = READ_ONCE(node->slots[slot]);
+	ptr = READ_ONCE(node->slots[slot]); /* Address dependency. */
 
 	pr_devel("consider slot %x [ix=%d type=%lu]\n",
 		 slot, level, (unsigned long)ptr & 3);
@@ -254,7 +244,6 @@ assoc_array_walk(const struct assoc_array *array,
 	cursor = ptr;
 follow_shortcut:
 	shortcut = assoc_array_ptr_to_shortcut(cursor);
-	smp_read_barrier_depends();
 	pr_devel("shortcut to %d\n", shortcut->skip_to_level);
 	sc_level = level + ASSOC_ARRAY_LEVEL_STEP;
 	BUG_ON(sc_level > shortcut->skip_to_level);
@@ -294,7 +283,7 @@ assoc_array_walk(const struct assoc_array *array,
 	} while (sc_level < shortcut->skip_to_level);
 
 	/* The shortcut matches the leaf's index to this point. */
-	cursor = READ_ONCE(shortcut->next_node);
+	cursor = READ_ONCE(shortcut->next_node); /* Address dependency. */
 	if (((level ^ sc_level) & ~ASSOC_ARRAY_KEY_CHUNK_MASK) != 0) {
 		level = sc_level;
 		goto jumped;
@@ -330,21 +319,19 @@ void *assoc_array_find(const struct assoc_array *array,
 	    assoc_array_walk_found_terminal_node)
 		return NULL;
 
-	node = result.terminal_node.node;
-	smp_read_barrier_depends();
+	node = READ_ONCE(result.terminal_node.node); /* Address dependency. */
 
 	/* If the target key is available to us, it's has to be pointed to by
 	 * the terminal node.
 	 */
 	for (slot = 0; slot < ASSOC_ARRAY_FAN_OUT; slot++) {
-		ptr = READ_ONCE(node->slots[slot]);
+		ptr = READ_ONCE(node->slots[slot]); /* Address dependency. */
 		if (ptr && assoc_array_ptr_is_leaf(ptr)) {
 			/* We need a barrier between the read of the pointer
 			 * and dereferencing the pointer - but only if we are
 			 * actually going to dereference it.
 			 */
 			leaf = assoc_array_ptr_to_leaf(ptr);
-			smp_read_barrier_depends();
 			if (ops->compare_object(leaf, index_key))
 				return (void *)leaf;
 		}

Re: [PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[Peter Zijlstra]

On Tue, Oct 10, 2017 at 08:50:42AM -0700, Paul E. McKenney wrote:
> +			/* We need a barrier between the read of the pointer,
> +			 * which is supplied by the above READ_ONCE().
>  			 */

>  			/* We need a barrier between the read of the pointer
>  			 * and dereferencing the pointer - but only if we are
>  			 * actually going to dereference it.
>  			 */

Could you also fix those "disgusting drug-induced crap" comment styles
while you're there anyway?

  http://lkml.iu.edu/hypermail/linux/kernel/1607.1/00627.html

Re: [PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[Paul E. McKenney]

On Tue, Oct 10, 2017 at 05:54:51PM +0200, Peter Zijlstra wrote:
> On Tue, Oct 10, 2017 at 08:50:42AM -0700, Paul E. McKenney wrote:
> > +			/* We need a barrier between the read of the pointer,
> > +			 * which is supplied by the above READ_ONCE().
> >  			 */
> 
> >  			/* We need a barrier between the read of the pointer
> >  			 * and dereferencing the pointer - but only if we are
> >  			 * actually going to dereference it.
> >  			 */
> 
> Could you also fix those "disgusting drug-induced crap" comment styles
> while you're there anyway?
> 
>   http://lkml.iu.edu/hypermail/linux/kernel/1607.1/00627.html

If Dave is OK with it, I will do so on my next pass.

							Thanx, Paul

Re: [PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[David Howells]

Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:

> -	node = result.terminal_node.node;
> -	smp_read_barrier_depends();
> +	node = READ_ONCE(result.terminal_node.node); /* Address dependency. */

The main problem I have with this method of annotation is that it's not
obvious there's a barrier there or which side the barrier is.

I think one of the trickiest issues is that a barrier is typically between two
things and we're not making it clear what those two things actually are.

Also, I would say that the most natural interpretation of READ_ONCE() is that
the implicit barrier comes after the read, e.g.:

	f = READ_ONCE(stuff->foo);
	/* Implied barrier */
	look_at(f->a);
	look_at(f->b);

I.e. READ_ONCE() prevents stuff->foo from being reread whilst you access f and
orders LOAD(stuff->foo) before LOAD(f->a) and LOAD(f->b).

David

Re: [PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[Will Deacon]

On Wed, Oct 11, 2017 at 01:19:59PM +0100, David Howells wrote:
> Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> 
> > -	node = result.terminal_node.node;
> > -	smp_read_barrier_depends();
> > +	node = READ_ONCE(result.terminal_node.node); /* Address dependency. */
> 
> The main problem I have with this method of annotation is that it's not
> obvious there's a barrier there or which side the barrier is.
> 
> I think one of the trickiest issues is that a barrier is typically between two
> things and we're not making it clear what those two things actually are.
> 
> Also, I would say that the most natural interpretation of READ_ONCE() is that
> the implicit barrier comes after the read, e.g.:
> 
> 	f = READ_ONCE(stuff->foo);
> 	/* Implied barrier */
> 	look_at(f->a);
> 	look_at(f->b);
> 
> I.e. READ_ONCE() prevents stuff->foo from being reread whilst you access f and
> orders LOAD(stuff->foo) before LOAD(f->a) and LOAD(f->b).

FWIW, that's exactly what my patches do, this fixup looks a bit weird
because it removes a prior barrier which suggests that either (a) it's in
the wrong place to start with, or (b) we're annotating the wrong load.

Will

Re: [PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[Paul E. McKenney]

On Wed, Oct 11, 2017 at 01:22:17PM +0100, Will Deacon wrote:
> On Wed, Oct 11, 2017 at 01:19:59PM +0100, David Howells wrote:
> > Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> > 
> > > -	node = result.terminal_node.node;
> > > -	smp_read_barrier_depends();
> > > +	node = READ_ONCE(result.terminal_node.node); /* Address dependency. */
> > 
> > The main problem I have with this method of annotation is that it's not
> > obvious there's a barrier there or which side the barrier is.
> > 
> > I think one of the trickiest issues is that a barrier is typically between two
> > things and we're not making it clear what those two things actually are.
> > 
> > Also, I would say that the most natural interpretation of READ_ONCE() is that
> > the implicit barrier comes after the read, e.g.:
> > 
> > 	f = READ_ONCE(stuff->foo);
> > 	/* Implied barrier */
> > 	look_at(f->a);
> > 	look_at(f->b);
> > 
> > I.e. READ_ONCE() prevents stuff->foo from being reread whilst you access f and
> > orders LOAD(stuff->foo) before LOAD(f->a) and LOAD(f->b).
> 
> FWIW, that's exactly what my patches do, this fixup looks a bit weird
> because it removes a prior barrier which suggests that either (a) it's in
> the wrong place to start with, or (b) we're annotating the wrong load.

You lost me on this one.  Here is the side-by-side change, minus the
comment:

node = result.terminal_node.node;		 node = READ_ONCE(result.terminal_node.node);
smp_read_barrier_depends();

The barrier was after the load that got annotated.

Or are you talking about some other fixup?

								Thanx, Paul

Re: [PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[Will Deacon]

On Wed, Oct 11, 2017 at 05:54:51AM -0700, Paul E. McKenney wrote:
> On Wed, Oct 11, 2017 at 01:22:17PM +0100, Will Deacon wrote:
> > On Wed, Oct 11, 2017 at 01:19:59PM +0100, David Howells wrote:
> > > Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> > > 
> > > > -	node = result.terminal_node.node;
> > > > -	smp_read_barrier_depends();
> > > > +	node = READ_ONCE(result.terminal_node.node); /* Address dependency. */
> > > 
> > > The main problem I have with this method of annotation is that it's not
> > > obvious there's a barrier there or which side the barrier is.
> > > 
> > > I think one of the trickiest issues is that a barrier is typically between two
> > > things and we're not making it clear what those two things actually are.
> > > 
> > > Also, I would say that the most natural interpretation of READ_ONCE() is that
> > > the implicit barrier comes after the read, e.g.:
> > > 
> > > 	f = READ_ONCE(stuff->foo);
> > > 	/* Implied barrier */
> > > 	look_at(f->a);
> > > 	look_at(f->b);
> > > 
> > > I.e. READ_ONCE() prevents stuff->foo from being reread whilst you access f and
> > > orders LOAD(stuff->foo) before LOAD(f->a) and LOAD(f->b).
> > 
> > FWIW, that's exactly what my patches do, this fixup looks a bit weird
> > because it removes a prior barrier which suggests that either (a) it's in
> > the wrong place to start with, or (b) we're annotating the wrong load.
> 
> You lost me on this one.  Here is the side-by-side change, minus the
> comment:
> 
> node = result.terminal_node.node;		 node = READ_ONCE(result.terminal_node.node);
> smp_read_barrier_depends();
> 
> The barrier was after the load that got annotated.

Yes, sorry, I completely lost my ability to read diff. Looking again, I
don't actually know what's being ordered by the smp_read_barrier_depends()
in the snippet above, given that assigning "node" is a load from the stack
afaict.

Will

Re: [PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[Paul E. McKenney]

On Wed, Oct 11, 2017 at 03:18:17PM +0100, Will Deacon wrote:
> On Wed, Oct 11, 2017 at 05:54:51AM -0700, Paul E. McKenney wrote:
> > On Wed, Oct 11, 2017 at 01:22:17PM +0100, Will Deacon wrote:
> > > On Wed, Oct 11, 2017 at 01:19:59PM +0100, David Howells wrote:
> > > > Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> > > > 
> > > > > -	node = result.terminal_node.node;
> > > > > -	smp_read_barrier_depends();
> > > > > +	node = READ_ONCE(result.terminal_node.node); /* Address dependency. */
> > > > 
> > > > The main problem I have with this method of annotation is that it's not
> > > > obvious there's a barrier there or which side the barrier is.
> > > > 
> > > > I think one of the trickiest issues is that a barrier is typically between two
> > > > things and we're not making it clear what those two things actually are.
> > > > 
> > > > Also, I would say that the most natural interpretation of READ_ONCE() is that
> > > > the implicit barrier comes after the read, e.g.:
> > > > 
> > > > 	f = READ_ONCE(stuff->foo);
> > > > 	/* Implied barrier */
> > > > 	look_at(f->a);
> > > > 	look_at(f->b);
> > > > 
> > > > I.e. READ_ONCE() prevents stuff->foo from being reread whilst you access f and
> > > > orders LOAD(stuff->foo) before LOAD(f->a) and LOAD(f->b).
> > > 
> > > FWIW, that's exactly what my patches do, this fixup looks a bit weird
> > > because it removes a prior barrier which suggests that either (a) it's in
> > > the wrong place to start with, or (b) we're annotating the wrong load.
> > 
> > You lost me on this one.  Here is the side-by-side change, minus the
> > comment:
> > 
> > node = result.terminal_node.node;		 node = READ_ONCE(result.terminal_node.node);
> > smp_read_barrier_depends();
> > 
> > The barrier was after the load that got annotated.
> 
> Yes, sorry, I completely lost my ability to read diff. Looking again, I
> don't actually know what's being ordered by the smp_read_barrier_depends()
> in the snippet above, given that assigning "node" is a load from the stack
> afaict.

Good point, and in fact the required READ_ONCE() already exists off
in assoc_array_walk().  Updated.

							Thanx, Paul

Re: [PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[Paul E. McKenney]

On Wed, Oct 11, 2017 at 01:19:59PM +0100, David Howells wrote:
> Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> 
> > -	node = result.terminal_node.node;
> > -	smp_read_barrier_depends();
> > +	node = READ_ONCE(result.terminal_node.node); /* Address dependency. */
> 
> The main problem I have with this method of annotation is that it's not
> obvious there's a barrier there or which side the barrier is.
> 
> I think one of the trickiest issues is that a barrier is typically between two
> things and we're not making it clear what those two things actually are.
> 
> Also, I would say that the most natural interpretation of READ_ONCE() is that
> the implicit barrier comes after the read, e.g.:
> 
> 	f = READ_ONCE(stuff->foo);
> 	/* Implied barrier */
> 	look_at(f->a);
> 	look_at(f->b);
> 
> I.e. READ_ONCE() prevents stuff->foo from being reread whilst you access f and
> orders LOAD(stuff->foo) before LOAD(f->a) and LOAD(f->b).

Placing the comment on the same line makes it less likely that some
later change will move the comment away from the load that it applies to.
Which appears to have happened on some of the other instances of
smp_read_barrier_depends() in other parts of the kernel.  It is not at
all clear what load they go with, or if that load is even still present
in the kernel.  :-/

							Thanx, Paul

Re: [PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[David Howells]

Will Deacon <will.deacon@arm.com> wrote:

> FWIW, that's exactly what my patches do, this fixup looks a bit weird
> because it removes a prior barrier which suggests that either (a) it's in
> the wrong place to start with, or (b) we're annotating the wrong load.

There is a loop involved.  The barrier is against the read in the previous
iteration of the loop.  IIRC, the reason I did it this way is to avoid the
need for the barrier if there's nothing on the 'after-side' - ie. we examine
the pointer and see that it's NULL or a leaf.  However, I'm not sure that's a
particularly necessary optimisation.

So if READ_ONCE() issues a smp_read_barrier_depends() after the read, then
I've no problem with the removal of these explicit barriers.

I will, however, quibble with the appropriateness of the name READ_ONCE()...
I still think it's not sufficiently obvious that this is a barrier and the
barrier is after.  Maybe READ_AND_BARRIER()?

Also, does WRITE_ONCE() imply a preceding barrier?

David

Re: [PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[Paul E. McKenney]

On Wed, Oct 11, 2017 at 04:17:25PM +0100, David Howells wrote:
> Will Deacon <will.deacon@arm.com> wrote:
> 
> > FWIW, that's exactly what my patches do, this fixup looks a bit weird
> > because it removes a prior barrier which suggests that either (a) it's in
> > the wrong place to start with, or (b) we're annotating the wrong load.
> 
> There is a loop involved.  The barrier is against the read in the previous
> iteration of the loop.  IIRC, the reason I did it this way is to avoid the
> need for the barrier if there's nothing on the 'after-side' - ie. we examine
> the pointer and see that it's NULL or a leaf.  However, I'm not sure that's a
> particularly necessary optimisation.

Given that smp_read_barrier_depends() is nothingness on anything other
than DEC Alpha, I would argue that this optimization is not necessary.

> So if READ_ONCE() issues a smp_read_barrier_depends() after the read, then
> I've no problem with the removal of these explicit barriers.

Very good!

> I will, however, quibble with the appropriateness of the name READ_ONCE()...
> I still think it's not sufficiently obvious that this is a barrier and the
> barrier is after.  Maybe READ_AND_BARRIER()?

Linus was unhappy with READ_ONCE_CTRL() to tag control dependencies, but
indicated that he might consider it if it helped code-analysis tools.
Adding Dmitry Vyukov for his thoughts on whether tagging READ_ONCE()
for dependencies would help.  Me, I would suggest READ_ONCE_DEP(), but
let's figure out if the bikeshed needs to be painted before arguing over
the color.  ;-)

> Also, does WRITE_ONCE() imply a preceding barrier?

It does not.  In most cases, the barriered version would be
smp_store_release().

							Thanx, Paul

Re: [PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[Peter Zijlstra]

On Wed, Oct 11, 2017 at 08:59:48AM -0700, Paul E. McKenney wrote:
> On Wed, Oct 11, 2017 at 04:17:25PM +0100, David Howells wrote:
> > I will, however, quibble with the appropriateness of the name READ_ONCE()...
> > I still think it's not sufficiently obvious that this is a barrier and the
> > barrier is after.  Maybe READ_AND_BARRIER()?
> 
> Linus was unhappy with READ_ONCE_CTRL() to tag control dependencies, but
> indicated that he might consider it if it helped code-analysis tools.
> Adding Dmitry Vyukov for his thoughts on whether tagging READ_ONCE()
> for dependencies would help.  Me, I would suggest READ_ONCE_DEP(), but
> let's figure out if the bikeshed needs to be painted before arguing over
> the color.  ;-)

Count me one vote for the READ_ONCE() name. This is about dependent
reads, which are nothing special on anything except Alpha.

We want to remove the exception/specialness from the memory model; and
therefore have to fix up all primitives that could possibly be used for
these reads to unconditionally issue the barrier (on Alpha). The
alternative is: rm -rf arch/alpha.

Adding something like READ_ONCE_DEP() does not rid us of the idea that
dependent reads are special and thus defeats the purpose, we might as
well retain lockless_dereference().

Now; any user of dependent reads must use READ_ONCE() in any case, to
avoid load tearing and reloads. So using READ_ONCE() for the dependent
reads is not extra or additional (note we'll also have to add the
barrier to all our relaxed and release atomics and anything else that
implies READ_ONCE and doesn't already imply smp_mb() after).

Re: [PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[Peter Zijlstra]

On Wed, Oct 11, 2017 at 06:12:20PM +0200, Peter Zijlstra wrote:
> On Wed, Oct 11, 2017 at 08:59:48AM -0700, Paul E. McKenney wrote:
> > On Wed, Oct 11, 2017 at 04:17:25PM +0100, David Howells wrote:
> > > I will, however, quibble with the appropriateness of the name READ_ONCE()...
> > > I still think it's not sufficiently obvious that this is a barrier and the
> > > barrier is after.  Maybe READ_AND_BARRIER()?
> > 
> > Linus was unhappy with READ_ONCE_CTRL() to tag control dependencies, but
> > indicated that he might consider it if it helped code-analysis tools.
> > Adding Dmitry Vyukov for his thoughts on whether tagging READ_ONCE()
> > for dependencies would help.  Me, I would suggest READ_ONCE_DEP(), but
> > let's figure out if the bikeshed needs to be painted before arguing over
> > the color.  ;-)
> 
> Count me one vote for the READ_ONCE() name. This is about dependent
> reads, which are nothing special on anything except Alpha.
> 
> We want to remove the exception/specialness from the memory model; and
> therefore have to fix up all primitives that could possibly be used for
> these reads to unconditionally issue the barrier (on Alpha). The
> alternative is: rm -rf arch/alpha.
> 
> Adding something like READ_ONCE_DEP() does not rid us of the idea that
> dependent reads are special and thus defeats the purpose, we might as
> well retain lockless_dereference().
> 
> Now; any user of dependent reads must use READ_ONCE() in any case, to
> avoid load tearing and reloads. So using READ_ONCE() for the dependent
> reads is not extra or additional (note we'll also have to add the
> barrier to all our relaxed and release atomics and anything else that
> implies READ_ONCE and doesn't already imply smp_mb() after).

Add the per-cpu ops to that list, they imply READ_ONCE(). Consider for
example this example:


	for_each_possible_cpu(cpu)
		smp_store_release(per_cpu_ptr(&foo, cpu), obj);

-vs-

	obj = this_cpu_read(foo);
	if (obj->ponies)
		fart_rainbow(obj);

Re: [PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[Paul E. McKenney]

On Wed, Oct 11, 2017 at 06:24:12PM +0200, Peter Zijlstra wrote:
> On Wed, Oct 11, 2017 at 06:12:20PM +0200, Peter Zijlstra wrote:
> > On Wed, Oct 11, 2017 at 08:59:48AM -0700, Paul E. McKenney wrote:
> > > On Wed, Oct 11, 2017 at 04:17:25PM +0100, David Howells wrote:
> > > > I will, however, quibble with the appropriateness of the name READ_ONCE()...
> > > > I still think it's not sufficiently obvious that this is a barrier and the
> > > > barrier is after.  Maybe READ_AND_BARRIER()?
> > > 
> > > Linus was unhappy with READ_ONCE_CTRL() to tag control dependencies, but
> > > indicated that he might consider it if it helped code-analysis tools.
> > > Adding Dmitry Vyukov for his thoughts on whether tagging READ_ONCE()
> > > for dependencies would help.  Me, I would suggest READ_ONCE_DEP(), but
> > > let's figure out if the bikeshed needs to be painted before arguing over
> > > the color.  ;-)
> > 
> > Count me one vote for the READ_ONCE() name. This is about dependent
> > reads, which are nothing special on anything except Alpha.
> > 
> > We want to remove the exception/specialness from the memory model; and
> > therefore have to fix up all primitives that could possibly be used for
> > these reads to unconditionally issue the barrier (on Alpha). The
> > alternative is: rm -rf arch/alpha.
> > 
> > Adding something like READ_ONCE_DEP() does not rid us of the idea that
> > dependent reads are special and thus defeats the purpose, we might as
> > well retain lockless_dereference().
> > 
> > Now; any user of dependent reads must use READ_ONCE() in any case, to
> > avoid load tearing and reloads. So using READ_ONCE() for the dependent
> > reads is not extra or additional (note we'll also have to add the
> > barrier to all our relaxed and release atomics and anything else that
> > implies READ_ONCE and doesn't already imply smp_mb() after).
> 
> Add the per-cpu ops to that list, they imply READ_ONCE(). Consider for
> example this example:
> 
> 
> 	for_each_possible_cpu(cpu)
> 		smp_store_release(per_cpu_ptr(&foo, cpu), obj);
> 
> -vs-
> 
> 	obj = this_cpu_read(foo);
> 	if (obj->ponies)
> 		fart_rainbow(obj);

Interesting.  Do we currently have any dependencies headed by
this_cpu_read()?

							Thanx, Paul

Re: [PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[Peter Zijlstra]

On Wed, Oct 11, 2017 at 09:47:48AM -0700, Paul E. McKenney wrote:
> Interesting.  Do we currently have any dependencies headed by
> this_cpu_read()?

Nope, but almost, look for: cpufreq_update_util_data.

Re: [PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[Paul E. McKenney]

On Wed, Oct 11, 2017 at 06:54:05PM +0200, Peter Zijlstra wrote:
> On Wed, Oct 11, 2017 at 09:47:48AM -0700, Paul E. McKenney wrote:
> > Interesting.  Do we currently have any dependencies headed by
> > this_cpu_read()?
> 
> Nope, but almost, look for: cpufreq_update_util_data.

This, you mean?

data = rcu_dereference_sched(*this_cpu_ptr(&cpufreq_update_util_data));

True, a non-RCU dependency use case would want to use ACCESS_ONCE()
in the new regime.

							Thanx, Paul

Re: [PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[Peter Zijlstra]

On Wed, Oct 11, 2017 at 10:06:31AM -0700, Paul E. McKenney wrote:

> This, you mean?
> 
> data = rcu_dereference_sched(*this_cpu_ptr(&cpufreq_update_util_data));

Yep, that one. Although in my tree it now appears to look like:

        data = rcu_dereference_sched(*per_cpu_ptr(&cpufreq_update_util_data,
                                                  cpu_of(rq)));

Re: [PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[Paul E. McKenney]

On Wed, Oct 11, 2017 at 07:11:37PM +0200, Peter Zijlstra wrote:
> On Wed, Oct 11, 2017 at 10:06:31AM -0700, Paul E. McKenney wrote:
> 
> > This, you mean?
> > 
> > data = rcu_dereference_sched(*this_cpu_ptr(&cpufreq_update_util_data));
> 
> Yep, that one. Although in my tree it now appears to look like:
> 
>         data = rcu_dereference_sched(*per_cpu_ptr(&cpufreq_update_util_data,
>                                                   cpu_of(rq)));

So in the non-RCU case, we could simply replace rcu_dereference_sched()
with ACCESS_ONCE(), right?  Thus no need to change the per-CPU
primitives.  Especially given that most uses of per-CPU variables
don't even need protection from load/store tearing, let alone
protection from Alpha.

Or am I missing something subtle here?

							Thanx, Paul

Re: [PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[Dmitry Vyukov]

On Wed, Oct 11, 2017 at 7:34 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Wed, Oct 11, 2017 at 07:11:37PM +0200, Peter Zijlstra wrote:
>> On Wed, Oct 11, 2017 at 10:06:31AM -0700, Paul E. McKenney wrote:
>>
>> > This, you mean?
>> >
>> > data = rcu_dereference_sched(*this_cpu_ptr(&cpufreq_update_util_data));
>>
>> Yep, that one. Although in my tree it now appears to look like:
>>
>>         data = rcu_dereference_sched(*per_cpu_ptr(&cpufreq_update_util_data,
>>                                                   cpu_of(rq)));
>
> So in the non-RCU case, we could simply replace rcu_dereference_sched()
> with ACCESS_ONCE(), right?  Thus no need to change the per-CPU
> primitives.  Especially given that most uses of per-CPU variables
> don't even need protection from load/store tearing, let alone
> protection from Alpha.



A long thread and I lost track somewhat, but, yes, from KTSAN (data
race detector) point of view we will need a way to understand when
things are ordered due to data and control dependencies
(i.e.effectively acquire but only wrt the dependent stuff).
There is a logic level and physical level, it's perfectly fine if on
physical level all these _DEP/_CTRL variants are no-op (the same as
READ_ONCE, at least on todays archs with todays compilers). But on
logical level they represent a well defined, meaningful thing. Say,
when you proof-read code for correctness, the fact that there is a
dependency can be important and crucial. But tools can's always figure
that out, and even if they can it's still useful to distinguish
between normal dependencies (that are all over the place) and the ones
that are important for correctness. It also makes code more explicit,
rather then relying on some tricky implicit guarantees. Say, like
__user annotations which we could wipe out from codegen point of view.

Re: [PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[Linus Torvalds]

On Wed, Oct 11, 2017 at 11:43 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>
> A long thread and I lost track somewhat, but, yes, from KTSAN (data
> race detector) point of view we will need a way to understand when
> things are ordered due to data and control dependencies
> (i.e.effectively acquire but only wrt the dependent stuff).
> There is a logic level and physical level, it's perfectly fine if on
> physical level all these _DEP/_CTRL variants are no-op (the same as
> READ_ONCE, at least on todays archs with todays compilers). But on
> logical level they represent a well defined, meaningful thing.

No, they do not.

Do you believe in fairies and Santa Claus?

Because quite frankly, the likelihood of either of those being true is
_way_ higher than the likelihood of any normal human ever getting
those things right.

So asking a programmer to annotate whether two memory accesses have a
data dependency or a control dependency is completely inappropriate.
You won't get people understanding it to begin with, much less then
figure out subtle things like whether a control dependency is an
actual branch, or might be turned into a data dependency through
select, or whatever.

We've had really smart people who wrote core code that couldn't get it
right, and that weren't sure if a control dependency was really
guaranteed or not.

That is *exactly* the kinds of thing that _automation_ should handle.
Not some human. Figure the data/control dependencies out from the
code, not from some logical level.

I saw the contortions that the ISO C people tried to go through just
to describe control and data dependencies. It was awful.  It should
have never been described on that kind of level to begin with, when it
would have been much easier to just describe it to compiler people as
"this is a consume relationship". The same rules apply here. Don't
make it about some high-level thing and humans annontating things.
Make it about the actual generated code.

                Linus

Re: [PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[Paul E. McKenney]

On Wed, Oct 11, 2017 at 11:56:54AM -0700, Linus Torvalds wrote:
> On Wed, Oct 11, 2017 at 11:43 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
> >
> > A long thread and I lost track somewhat, but, yes, from KTSAN (data
> > race detector) point of view we will need a way to understand when
> > things are ordered due to data and control dependencies
> > (i.e.effectively acquire but only wrt the dependent stuff).
> > There is a logic level and physical level, it's perfectly fine if on
> > physical level all these _DEP/_CTRL variants are no-op (the same as
> > READ_ONCE, at least on todays archs with todays compilers). But on
> > logical level they represent a well defined, meaningful thing.
> 
> No, they do not.
> 
> Do you believe in fairies and Santa Claus?
> 
> Because quite frankly, the likelihood of either of those being true is
> _way_ higher than the likelihood of any normal human ever getting
> those things right.
> 
> So asking a programmer to annotate whether two memory accesses have a
> data dependency or a control dependency is completely inappropriate.
> You won't get people understanding it to begin with, much less then
> figure out subtle things like whether a control dependency is an
> actual branch, or might be turned into a data dependency through
> select, or whatever.
> 
> We've had really smart people who wrote core code that couldn't get it
> right, and that weren't sure if a control dependency was really
> guaranteed or not.
> 
> That is *exactly* the kinds of thing that _automation_ should handle.
> Not some human. Figure the data/control dependencies out from the
> code, not from some logical level.
> 
> I saw the contortions that the ISO C people tried to go through just
> to describe control and data dependencies. It was awful.  It should
> have never been described on that kind of level to begin with, when it
> would have been much easier to just describe it to compiler people as
> "this is a consume relationship". The same rules apply here. Don't
> make it about some high-level thing and humans annontating things.
> Make it about the actual generated code.

Speaking as one of the ISO C people...  What exactly do you have in
mind when you say to just describe it to compiler people as "this is a
consume relationship"?

							Thanx, Paul

Re: [PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[Dmitry Vyukov]

On Wed, Oct 11, 2017 at 8:56 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Oct 11, 2017 at 11:43 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>
>> A long thread and I lost track somewhat, but, yes, from KTSAN (data
>> race detector) point of view we will need a way to understand when
>> things are ordered due to data and control dependencies
>> (i.e.effectively acquire but only wrt the dependent stuff).
>> There is a logic level and physical level, it's perfectly fine if on
>> physical level all these _DEP/_CTRL variants are no-op (the same as
>> READ_ONCE, at least on todays archs with todays compilers). But on
>> logical level they represent a well defined, meaningful thing.
>
> No, they do not.
>
> Do you believe in fairies and Santa Claus?
>
> Because quite frankly, the likelihood of either of those being true is
> _way_ higher than the likelihood of any normal human ever getting
> those things right.
>
> So asking a programmer to annotate whether two memory accesses have a
> data dependency or a control dependency is completely inappropriate.
> You won't get people understanding it to begin with, much less then
> figure out subtle things like whether a control dependency is an
> actual branch, or might be turned into a data dependency through
> select, or whatever.
>
> We've had really smart people who wrote core code that couldn't get it
> right, and that weren't sure if a control dependency was really
> guaranteed or not.
>
> That is *exactly* the kinds of thing that _automation_ should handle.
> Not some human. Figure the data/control dependencies out from the
> code, not from some logical level.
>
> I saw the contortions that the ISO C people tried to go through just
> to describe control and data dependencies. It was awful.  It should
> have never been described on that kind of level to begin with, when it
> would have been much easier to just describe it to compiler people as
> "this is a consume relationship". The same rules apply here. Don't
> make it about some high-level thing and humans annontating things.
> Make it about the actual generated code.


First of all, I am not asking for a big overhaul and nothing changes
wrt codegen (i.e. the code works the same way it used to work).
If/when it is done, it will be guided by the race detector. It is
similar to enabling a new compiler warning and cleaning up the code
base incrementally. Compiler warnings don't affect code behavior, and
one doesn't need to proactively guess where they will fire (a compiler
warnings points you to the line what compiler does not like).

Second, we would very much like to just analyze the code as is. And in
fact we gone to great lengths teaching it about standalone memory
barriers (rmb/wmb), per-cpu variables, seqlocks, rcu, etc (none of
that we had in user-space). But this is just where we need help
(basically the same problem you mentioned with ISO C, it's effectively
not defineable/analyzable by compiler).

As far as I remember we added just few of READ_ONCE_CTRL (<5) to get
the prototype running. So it's not that we are talking about
hundreds/thousands of them. And again the tool will point to the
places it does not like. So I am just asking for a practical tradeoff:
few annotations in exchange for hundreds/thousands of bugs killed.

I did not want to revive this discussion right now, we already had it
and decided that we need a clear story for READ_ONCE_CTRL first. Our
plan is to revive the tool, find more real bugs and show what are the
actual code changes required for the tool. Hope we can get to some
compromise.

Re: [PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[Paul E. McKenney]

On Wed, Oct 11, 2017 at 10:06:31AM -0700, Paul E. McKenney wrote:
> On Wed, Oct 11, 2017 at 06:54:05PM +0200, Peter Zijlstra wrote:
> > On Wed, Oct 11, 2017 at 09:47:48AM -0700, Paul E. McKenney wrote:
> > > Interesting.  Do we currently have any dependencies headed by
> > > this_cpu_read()?
> > 
> > Nope, but almost, look for: cpufreq_update_util_data.
> 
> This, you mean?
> 
> data = rcu_dereference_sched(*this_cpu_ptr(&cpufreq_update_util_data));
> 
> True, a non-RCU dependency use case would want to use ACCESS_ONCE()
> in the new regime.

Right...  s/ACCESS_ONCE()/READ_ONCE()/

							Thanx, Paul

Re: [PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[Mark Rutland]

On Wed, Oct 11, 2017 at 06:24:12PM +0200, Peter Zijlstra wrote:
> On Wed, Oct 11, 2017 at 06:12:20PM +0200, Peter Zijlstra wrote:
> > On Wed, Oct 11, 2017 at 08:59:48AM -0700, Paul E. McKenney wrote:
> > > On Wed, Oct 11, 2017 at 04:17:25PM +0100, David Howells wrote:
> > > > I will, however, quibble with the appropriateness of the name READ_ONCE()...
> > > > I still think it's not sufficiently obvious that this is a barrier and the
> > > > barrier is after.  Maybe READ_AND_BARRIER()?
> > > 
> > > Linus was unhappy with READ_ONCE_CTRL() to tag control dependencies, but
> > > indicated that he might consider it if it helped code-analysis tools.
> > > Adding Dmitry Vyukov for his thoughts on whether tagging READ_ONCE()
> > > for dependencies would help.  Me, I would suggest READ_ONCE_DEP(), but
> > > let's figure out if the bikeshed needs to be painted before arguing over
> > > the color.  ;-)
> > 
> > Count me one vote for the READ_ONCE() name. This is about dependent
> > reads, which are nothing special on anything except Alpha.
> > 
> > We want to remove the exception/specialness from the memory model; and
> > therefore have to fix up all primitives that could possibly be used for
> > these reads to unconditionally issue the barrier (on Alpha). The
> > alternative is: rm -rf arch/alpha.
> > 
> > Adding something like READ_ONCE_DEP() does not rid us of the idea that
> > dependent reads are special and thus defeats the purpose, we might as
> > well retain lockless_dereference().
> > 
> > Now; any user of dependent reads must use READ_ONCE() in any case, to
> > avoid load tearing and reloads. So using READ_ONCE() for the dependent
> > reads is not extra or additional (note we'll also have to add the
> > barrier to all our relaxed and release atomics and anything else that
> > implies READ_ONCE and doesn't already imply smp_mb() after).
> 
> Add the per-cpu ops to that list, they imply READ_ONCE(). Consider for
> example this example:
> 
> 
> 	for_each_possible_cpu(cpu)
> 		smp_store_release(per_cpu_ptr(&foo, cpu), obj);
> 
> -vs-
> 
> 	obj = this_cpu_read(foo);
> 	if (obj->ponies)
> 		fart_rainbow(obj);

Sorry to derail things, but per the docs, this_cpu_read() (as with other
this_cpu_*() ops) is not atomic w.r.t. stores from another CPU.

Today in practice, that only matters for accesses larger than the native
word size, but the API explicitly doesn't guarantee a lack of tearing
for the above example. I'm sure we have bugs...

Thanks,
Mark.

Re: [PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[Paul E. McKenney]

On Wed, Oct 11, 2017 at 06:12:20PM +0200, Peter Zijlstra wrote:
> On Wed, Oct 11, 2017 at 08:59:48AM -0700, Paul E. McKenney wrote:
> > On Wed, Oct 11, 2017 at 04:17:25PM +0100, David Howells wrote:
> > > I will, however, quibble with the appropriateness of the name READ_ONCE()...
> > > I still think it's not sufficiently obvious that this is a barrier and the
> > > barrier is after.  Maybe READ_AND_BARRIER()?
> > 
> > Linus was unhappy with READ_ONCE_CTRL() to tag control dependencies, but
> > indicated that he might consider it if it helped code-analysis tools.
> > Adding Dmitry Vyukov for his thoughts on whether tagging READ_ONCE()
> > for dependencies would help.  Me, I would suggest READ_ONCE_DEP(), but
> > let's figure out if the bikeshed needs to be painted before arguing over
> > the color.  ;-)
> 
> Count me one vote for the READ_ONCE() name. This is about dependent
> reads, which are nothing special on anything except Alpha.

Agreed, unless specially marking them makes it easier for tools to
find bugs.  In which case, we should definitely specially mark them.

							Thanx, Paul

Re: [PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[David Howells]

Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:

> It does not.  In most cases, the barriered version would be
> smp_store_release().

Ummm... Is that good enough?  Is:

	WRITE_ONCE(x, 1);
	WRITE_ONCE(x, 2);

equivalent to:

	smp_store_release(x, 1);
	smp_store_release(x, 2);

if CONFIG_SMP=n?

(Consider what happens if an interrupt messes with x).

If it is good enough, should we be using smp_load_acquire() rather than
READ_ONCE()?

David

Re: [PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[Peter Zijlstra]

On Wed, Oct 11, 2017 at 05:07:05PM +0100, David Howells wrote:
> Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> 
> > It does not.  In most cases, the barriered version would be
> > smp_store_release().
> 
> Ummm... Is that good enough?  Is:
> 
> 	WRITE_ONCE(x, 1);
> 	WRITE_ONCE(x, 2);
> 
> equivalent to:
> 
> 	smp_store_release(x, 1);
> 	smp_store_release(x, 2);
> 
> if CONFIG_SMP=n?

Almost; it ends up being:

	barrier();
	WRITE_ONCE(x, 1);
	barrier();
	WRITE_ONCE(x, 2);

> (Consider what happens if an interrupt messes with x).
> 
> If it is good enough, should we be using smp_load_acquire() rather than
> READ_ONCE()?

No, smp_load_acquire() is strictly stronger (and far more expensive on
!Alpha).

Dependent loads do not require barriers (except Alpha, and we want to
kill that special case).

Re: [PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[Paul E. McKenney]

On Wed, Oct 11, 2017 at 05:07:05PM +0100, David Howells wrote:
> Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> 
> > It does not.  In most cases, the barriered version would be
> > smp_store_release().
> 
> Ummm... Is that good enough?  Is:
> 
> 	WRITE_ONCE(x, 1);
> 	WRITE_ONCE(x, 2);
> 
> equivalent to:
> 
> 	smp_store_release(x, 1);
> 	smp_store_release(x, 2);
> 
> if CONFIG_SMP=n?

	smp_store_release(&x, 1);
	smp_store_release(&x, 2);

But yes, give or take that smp_store_release() potentially disables
more compiler optimizations than does WRITE_ONCE().

> (Consider what happens if an interrupt messes with x).

OK, I will bite...  What is your scenario in which an interrupt
gives different results for CONFIG_SMP=n?  The barriers

> If it is good enough, should we be using smp_load_acquire() rather than
> READ_ONCE()?

On x86, that might be OK, give or take that smp_load_acquire() potentially
disables more optimizations than does READ_ONCE().  But on ARM, PowerPC,
MIPS, and so on, smp_load_acquire() emits a memory-barrier instruction
and READ_ONCE() does not.

							Thanx, Paul

Re: [PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[David Howells]

Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:

> Placing the comment on the same line makes it less likely that some
> later change will move the comment away from the load that it applies to.

The problem with your 'address dep' comment is that it's not particularly
useful.

Either your comment needs to say "dep between X and Y", but if the following is
always the dep:

	Y = READ_ONCE(X)
	access(*Y)

then the comment is superfluous.

If it's not always true then your comment needs to indicate what the dependency
is.

The other thing your comment could/should say is where the other barrier is -
barriers always have to be paired as a general rule.  (I know I haven't put
these comments in here - but I've been doing that recently).

David

Re: [PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[Paul E. McKenney]

On Wed, Oct 11, 2017 at 04:28:24PM +0100, David Howells wrote:
> Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> 
> > Placing the comment on the same line makes it less likely that some
> > later change will move the comment away from the load that it applies to.
> 
> The problem with your 'address dep' comment is that it's not particularly
> useful.
> 
> Either your comment needs to say "dep between X and Y", but if the following is
> always the dep:
> 
> 	Y = READ_ONCE(X)
> 	access(*Y)
> 
> then the comment is superfluous.

In assoc_array.c, the access is often quite some distance from the
corresponding READ_ONCE().

> If it's not always true then your comment needs to indicate what the dependency
> is.

Given that most READ_ONCE() calls aren't heading dependency chains,
a comment indicating that a particular READ_ONCE() does head a dependency
chain does provide at least some information.  But, as you say below...

> The other thing your comment could/should say is where the other barrier is -
> barriers always have to be paired as a general rule.  (I know I haven't put
> these comments in here - but I've been doing that recently).

I would welcome a patch that added the comments or help with what
the comments should say.

							Thanx, Paul

Re: [PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[David Howells]

David Howells <dhowells@redhat.com> wrote:

> Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> 
> >  static inline void *assoc_array_ptr_to_leaf(const struct assoc_array_ptr *x)
> >  {
> > -	return (void *)((unsigned long)x & ~ASSOC_ARRAY_PTR_TYPE_MASK);
> > +	return (void *)((unsigned long)READ_ONCE(x) & /* Address dependency. */
> > +		~ASSOC_ARRAY_PTR_TYPE_MASK);
> >  }
> 
> This is the wrong place to do this.  assoc_array_ptr_to_leaf() is effectively
> no more than a special cast; it removes a metadata bit from a pointer.  x is
> the value we're modifying, not *x, and x was read by the caller.

Also, x is not a pointer you can read from, so if READ_ONCE(x) ever effects a
memory access, you might get an oops.

David

Re: [PATCH RFC tip/core/rcu 12/15] lib/assoc_array: Remove smp_read_barrier_depends()

[Paul E. McKenney]

On Tue, Oct 10, 2017 at 10:59:39AM +0100, David Howells wrote:
> David Howells <dhowells@redhat.com> wrote:
> 
> > Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> > 
> > >  static inline void *assoc_array_ptr_to_leaf(const struct assoc_array_ptr *x)
> > >  {
> > > -	return (void *)((unsigned long)x & ~ASSOC_ARRAY_PTR_TYPE_MASK);
> > > +	return (void *)((unsigned long)READ_ONCE(x) & /* Address dependency. */
> > > +		~ASSOC_ARRAY_PTR_TYPE_MASK);
> > >  }
> > 
> > This is the wrong place to do this.  assoc_array_ptr_to_leaf() is effectively
> > no more than a special cast; it removes a metadata bit from a pointer.  x is
> > the value we're modifying, not *x, and x was read by the caller.
> 
> Also, x is not a pointer you can read from, so if READ_ONCE(x) ever effects a
> memory access, you might get an oops.

Agreed, and I got rid of it.

And thank you for looking this over!

And the patch again below, but this time also updating the commit log.

							Thanx, Paul

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

commit 472850f320e8eb47cfb06420c128c3f2bb8ccf27
Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Date:   Mon Oct 9 11:39:57 2017 -0700

    lib/assoc_array: Remove smp_read_barrier_depends()
    
    Now that smp_read_barrier_depends() is implied by READ_ONCE(), the several
    smp_read_barrier_depends() calls may be removed from lib/assoc_array.c.
    This commit makes this change and marks the READ_ONCE() calls that head
    address dependencies.
    
    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
    Cc: Jonathan Corbet <corbet@lwn.net>
    Cc: Mark Rutland <mark.rutland@arm.com>
    Cc: Alexander Kuleshov <kuleshovmail@gmail.com>

diff --git a/lib/assoc_array.c b/lib/assoc_array.c
index fe7953aead82..7e483294f7d7 100644
--- a/lib/assoc_array.c
+++ b/lib/assoc_array.c
@@ -38,12 +38,10 @@ static int assoc_array_subtree_iterate(const struct assoc_array_ptr *root,
 	if (assoc_array_ptr_is_shortcut(cursor)) {
 		/* Descend through a shortcut */
 		shortcut = assoc_array_ptr_to_shortcut(cursor);
-		smp_read_barrier_depends();
-		cursor = READ_ONCE(shortcut->next_node);
+		cursor = READ_ONCE(shortcut->next_node); /* Address dependency. */
 	}
 
 	node = assoc_array_ptr_to_node(cursor);
-	smp_read_barrier_depends();
 	slot = 0;
 
 	/* We perform two passes of each node.
@@ -55,15 +53,12 @@ static int assoc_array_subtree_iterate(const struct assoc_array_ptr *root,
 	 */
 	has_meta = 0;
 	for (; slot < ASSOC_ARRAY_FAN_OUT; slot++) {
-		ptr = READ_ONCE(node->slots[slot]);
+		ptr = READ_ONCE(node->slots[slot]); /* Address dependency. */
 		has_meta |= (unsigned long)ptr;
 		if (ptr && assoc_array_ptr_is_leaf(ptr)) {
-			/* We need a barrier between the read of the pointer
-			 * and dereferencing the pointer - but only if we are
-			 * actually going to dereference it.
+			/* We need a barrier between the read of the pointer,
+			 * which is supplied by the above READ_ONCE().
 			 */
-			smp_read_barrier_depends();
-
 			/* Invoke the callback */
 			ret = iterator(assoc_array_ptr_to_leaf(ptr),
 				       iterator_data);
@@ -86,10 +81,8 @@ static int assoc_array_subtree_iterate(const struct assoc_array_ptr *root,
 
 continue_node:
 	node = assoc_array_ptr_to_node(cursor);
-	smp_read_barrier_depends();
-
 	for (; slot < ASSOC_ARRAY_FAN_OUT; slot++) {
-		ptr = READ_ONCE(node->slots[slot]);
+		ptr = READ_ONCE(node->slots[slot]); /* Address dependency. */
 		if (assoc_array_ptr_is_meta(ptr)) {
 			cursor = ptr;
 			goto begin_node;
@@ -98,16 +91,15 @@ static int assoc_array_subtree_iterate(const struct assoc_array_ptr *root,
 
 finished_node:
 	/* Move up to the parent (may need to skip back over a shortcut) */
-	parent = READ_ONCE(node->back_pointer);
+	parent = READ_ONCE(node->back_pointer); /* Address dependency. */
 	slot = node->parent_slot;
 	if (parent == stop)
 		return 0;
 
 	if (assoc_array_ptr_is_shortcut(parent)) {
 		shortcut = assoc_array_ptr_to_shortcut(parent);
-		smp_read_barrier_depends();
 		cursor = parent;
-		parent = READ_ONCE(shortcut->back_pointer);
+		parent = READ_ONCE(shortcut->back_pointer); /* Address dependency. */
 		slot = shortcut->parent_slot;
 		if (parent == stop)
 			return 0;
@@ -147,7 +139,7 @@ int assoc_array_iterate(const struct assoc_array *array,
 					void *iterator_data),
 			void *iterator_data)
 {
-	struct assoc_array_ptr *root = READ_ONCE(array->root);
+	struct assoc_array_ptr *root = READ_ONCE(array->root); /* Address dependency. */
 
 	if (!root)
 		return 0;
@@ -194,7 +186,7 @@ assoc_array_walk(const struct assoc_array *array,
 
 	pr_devel("-->%s()\n", __func__);
 
-	cursor = READ_ONCE(array->root);
+	cursor = READ_ONCE(array->root);  /* Address dependency. */
 	if (!cursor)
 		return assoc_array_walk_tree_empty;
 
@@ -216,11 +208,9 @@ assoc_array_walk(const struct assoc_array *array,
 
 consider_node:
 	node = assoc_array_ptr_to_node(cursor);
-	smp_read_barrier_depends();
-
 	slot = segments >> (level & ASSOC_ARRAY_KEY_CHUNK_MASK);
 	slot &= ASSOC_ARRAY_FAN_MASK;
-	ptr = READ_ONCE(node->slots[slot]);
+	ptr = READ_ONCE(node->slots[slot]); /* Address dependency. */
 
 	pr_devel("consider slot %x [ix=%d type=%lu]\n",
 		 slot, level, (unsigned long)ptr & 3);
@@ -254,7 +244,6 @@ assoc_array_walk(const struct assoc_array *array,
 	cursor = ptr;
 follow_shortcut:
 	shortcut = assoc_array_ptr_to_shortcut(cursor);
-	smp_read_barrier_depends();
 	pr_devel("shortcut to %d\n", shortcut->skip_to_level);
 	sc_level = level + ASSOC_ARRAY_LEVEL_STEP;
 	BUG_ON(sc_level > shortcut->skip_to_level);
@@ -294,7 +283,7 @@ assoc_array_walk(const struct assoc_array *array,
 	} while (sc_level < shortcut->skip_to_level);
 
 	/* The shortcut matches the leaf's index to this point. */
-	cursor = READ_ONCE(shortcut->next_node);
+	cursor = READ_ONCE(shortcut->next_node); /* Address dependency. */
 	if (((level ^ sc_level) & ~ASSOC_ARRAY_KEY_CHUNK_MASK) != 0) {
 		level = sc_level;
 		goto jumped;
@@ -330,21 +319,19 @@ void *assoc_array_find(const struct assoc_array *array,
 	    assoc_array_walk_found_terminal_node)
 		return NULL;
 
-	node = result.terminal_node.node;
-	smp_read_barrier_depends();
+	node = READ_ONCE(result.terminal_node.node); /* Address dependency. */
 
 	/* If the target key is available to us, it's has to be pointed to by
 	 * the terminal node.
 	 */
 	for (slot = 0; slot < ASSOC_ARRAY_FAN_OUT; slot++) {
-		ptr = READ_ONCE(node->slots[slot]);
+		ptr = READ_ONCE(node->slots[slot]); /* Address dependency. */
 		if (ptr && assoc_array_ptr_is_leaf(ptr)) {
 			/* We need a barrier between the read of the pointer
 			 * and dereferencing the pointer - but only if we are
 			 * actually going to dereference it.
 			 */
 			leaf = assoc_array_ptr_to_leaf(ptr);
-			smp_read_barrier_depends();
 			if (ops->compare_object(leaf, index_key))
 				return (void *)leaf;
 		}

Re: [PATCH RFC tip/core/rcu 0/15] Remove to-be-unneeded smp_read_barrier_depends()

[David Howells]

Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:

> Will Deacon has proposed adding smp_read_barrier_depends() to READ_ONCE(),
> which would mean that quite a few instances of smp_read_barrier_depends()
> would become redundant.

It's not clear from you description where the barrier is added in relation to
the read: before, after or both?

David

Re: [PATCH RFC tip/core/rcu 0/15] Remove to-be-unneeded smp_read_barrier_depends()

[Paul E. McKenney]

On Wed, Oct 11, 2017 at 01:21:03PM +0100, David Howells wrote:
> Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> 
> > Will Deacon has proposed adding smp_read_barrier_depends() to READ_ONCE(),
> > which would mean that quite a few instances of smp_read_barrier_depends()
> > would become redundant.
> 
> It's not clear from you description where the barrier is added in relation to
> the read: before, after or both?

After, similar to lockless_dereference().  Of course, there is not
really a barrier except for on Alpha.

							Thanx, Paul