Hello!
Now that READ_ONCE() includes smp_read_barrier_depends(), almost nothing
else needs to, the exceptions being DEC Alpha architecture-specific
code and of course READ_ONCE() itself. This series therefore removes
smp_read_barrier_depends() and read_barrier_depends() from elsewhere,
as they no longer have any effect. Note that this patch series also
prohibits use of InfiniBand on DEC Alpha: (1) A poll of DEC Alpha
users revealed no use of InfiniBand and (2) It is not clear that
InfiniBand's memory ordering correctly handles DEC Alpha, even with the
smp_read_barrier_depends() invocations.
Note also that patch 4 moves to release-acquire ordering to ensure that
the name length is properly synchronized. An alternative approach would
be to place the length in with the name, so that dependency ordering
would cover both the length and the name, but doing so seemed a bit
intrusive for this series.
Thanx, Paul
------------------------------------------------------------------------
Documentation/RCU/Design/Requirements/Requirements.html | 3 -
Documentation/RCU/rcu_dereference.txt | 6 --
Documentation/RCU/whatisRCU.txt | 3 -
Documentation/circular-buffers.txt | 3 -
Documentation/memory-barriers.txt | 18 ++++---
arch/mn10300/kernel/mn10300-serial.c | 7 ++-
drivers/dma/ioat/dma.c | 2
drivers/infiniband/Kconfig | 1
drivers/infiniband/hw/hfi1/rc.c | 4 -
drivers/infiniband/hw/hfi1/ruc.c | 1
drivers/infiniband/hw/hfi1/sdma.c | 1
drivers/infiniband/hw/hfi1/uc.c | 2
drivers/infiniband/hw/hfi1/ud.c | 2
drivers/infiniband/hw/qib/qib_rc.c | 3 -
drivers/infiniband/hw/qib/qib_ruc.c | 1
drivers/infiniband/hw/qib/qib_uc.c | 2
drivers/infiniband/hw/qib/qib_ud.c | 2
drivers/infiniband/sw/rdmavt/qp.c | 1
drivers/net/ethernet/qlogic/qed/qed_spq.c | 4 -
drivers/vhost/vhost.c | 7 ---
fs/dcache.c | 10 +---
include/linux/genetlink.h | 3 -
include/linux/netfilter/nfnetlink.h | 3 -
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 | 37 +++++-----------
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 ---
scripts/checkpatch.pl | 6 ++
security/keys/keyring.c | 7 ---
38 files changed, 85 insertions(+), 160 deletions(-)
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 <[email protected]>
Cc: Ariel Elior <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
---
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
Now that READ_ONCE() implies smp_read_barrier_depends(), update the
rtnl_dereference() header comment accordingly.
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Vladislav Yasevich <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: David Ahern <[email protected]>
Cc: Vlad Yasevich <[email protected]>
---
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 2032ce2eb20b..1eadec3fc228 100644
--- a/include/linux/rtnetlink.h
+++ b/include/linux/rtnetlink.h
@@ -70,8 +70,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
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 <[email protected]>
Acked-by: Tejun Heo <[email protected]>
Cc: Christoph Lameter <[email protected]>
---
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 6658d9ee5257..864d167a1073 100644
--- a/include/linux/percpu-refcount.h
+++ b/include/linux/percpu-refcount.h
@@ -139,12 +139,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
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 <[email protected]>
---
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 a6ddc42f87a5..000432b87e5a 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
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 <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Ingo Molnar <[email protected]>
---
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
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 <[email protected]>
Cc: Ingo Molnar <[email protected]>
Acked-by: Steven Rostedt (VMware) <[email protected]>
Acked-by: Mathieu Desnoyers <[email protected]>
---
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
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 <[email protected]>
---
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 479ecec80593..2269e58fa073 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
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 <[email protected]>
Cc: David Howells <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: <[email protected]>
---
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
Because READ_ONCE() now implies read_barrier_depends(), the
read_barrier_depends() in next_desc() is now redundant. This commit
therefore removes it and the related comments.
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: "Michael S. Tsirkin" <[email protected]>
Cc: Jason Wang <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
---
drivers/vhost/vhost.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 33ac2b186b85..78b5940a415a 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1877,12 +1877,7 @@ static unsigned next_desc(struct vhost_virtqueue *vq, struct vring_desc *desc)
return -1U;
/* Check they're not leading us off end of descriptors. */
- next = vhost16_to_cpu(vq, desc->next);
- /* Make sure compiler knows to grab that: we don't want it changing! */
- /* We will use the result as an index in an array, so most
- * architectures only need a compiler barrier here. */
- read_barrier_depends();
-
+ next = vhost16_to_cpu(vq, READ_ONCE(desc->next));
return next;
}
--
2.5.2
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 <[email protected]>
Cc: Pablo Neira Ayuso <[email protected]>
Cc: Jozsef Kadlecsik <[email protected]>
Cc: Florian Westphal <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
---
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 f88221aebc9d..d242c2d29161 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 4cbe5e80f3bf..46866cc24a84 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 f06e25065a34..ac1db84722a7 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
The smp_read_barrier_depends() does nothing at all except on DEC Alpha,
and no current DEC Alpha systems use Infiniband:
lkml.kernel.org/r/20171023085921.jwbntptn6ictbnvj@tower
This commit therefore makes Infiniband depend on !ALPHA and removes
the now-ineffective invocations of smp_read_barrier_depends() from
the InfiniBand driver.
Please note that this patch should not be construed as my saying that
InfiniBand's memory ordering is correct, but rather that this patch does
not in any way affect InfiniBand's correctness. In other words, the
result of applying this patch is bug-for-bug compatible with the original.
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: Doug Ledford <[email protected]>
Cc: Jason Gunthorpe <[email protected]>
Cc: Richard Henderson <[email protected]>
Cc: Ivan Kokshaysky <[email protected]>
Cc: Matt Turner <[email protected]>
Cc: Michael Cree <[email protected]>
Cc: Andrea Parri <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
---
drivers/dma/ioat/dma.c | 2 --
drivers/infiniband/Kconfig | 1 +
drivers/infiniband/hw/hfi1/rc.c | 4 ----
drivers/infiniband/hw/hfi1/ruc.c | 1 -
drivers/infiniband/hw/hfi1/sdma.c | 1 -
drivers/infiniband/hw/hfi1/uc.c | 2 --
drivers/infiniband/hw/hfi1/ud.c | 2 --
drivers/infiniband/hw/qib/qib_rc.c | 3 ---
drivers/infiniband/hw/qib/qib_ruc.c | 1 -
drivers/infiniband/hw/qib/qib_uc.c | 2 --
drivers/infiniband/hw/qib/qib_ud.c | 2 --
drivers/infiniband/sw/rdmavt/qp.c | 1 -
12 files changed, 1 insertion(+), 21 deletions(-)
diff --git a/drivers/dma/ioat/dma.c b/drivers/dma/ioat/dma.c
index 58d4ccd33672..8b5b23a8ace9 100644
--- a/drivers/dma/ioat/dma.c
+++ b/drivers/dma/ioat/dma.c
@@ -597,7 +597,6 @@ static void __cleanup(struct ioatdma_chan *ioat_chan, dma_addr_t phys_complete)
for (i = 0; i < active && !seen_current; i++) {
struct dma_async_tx_descriptor *tx;
- smp_read_barrier_depends();
prefetch(ioat_get_ring_ent(ioat_chan, idx + i + 1));
desc = ioat_get_ring_ent(ioat_chan, idx + i);
dump_desc_dbg(ioat_chan, desc);
@@ -715,7 +714,6 @@ static void ioat_abort_descs(struct ioatdma_chan *ioat_chan)
for (i = 1; i < active; i++) {
struct dma_async_tx_descriptor *tx;
- smp_read_barrier_depends();
prefetch(ioat_get_ring_ent(ioat_chan, idx + i + 1));
desc = ioat_get_ring_ent(ioat_chan, idx + i);
diff --git a/drivers/infiniband/Kconfig b/drivers/infiniband/Kconfig
index 98ac46ed7214..3bb6e35b0bbf 100644
--- a/drivers/infiniband/Kconfig
+++ b/drivers/infiniband/Kconfig
@@ -4,6 +4,7 @@ menuconfig INFINIBAND
depends on NET
depends on INET
depends on m || IPV6 != m
+ depends on !ALPHA
select IRQ_POLL
---help---
Core support for InfiniBand (IB). Make sure to also select
diff --git a/drivers/infiniband/hw/hfi1/rc.c b/drivers/infiniband/hw/hfi1/rc.c
index fd01a760259f..f527bcda4650 100644
--- a/drivers/infiniband/hw/hfi1/rc.c
+++ b/drivers/infiniband/hw/hfi1/rc.c
@@ -302,7 +302,6 @@ int hfi1_make_rc_req(struct rvt_qp *qp, struct hfi1_pkt_state *ps)
if (!(ib_rvt_state_ops[qp->state] & RVT_FLUSH_SEND))
goto bail;
/* We are in the error state, flush the work request. */
- smp_read_barrier_depends(); /* see post_one_send() */
if (qp->s_last == READ_ONCE(qp->s_head))
goto bail;
/* If DMAs are in progress, we can't flush immediately. */
@@ -346,7 +345,6 @@ int hfi1_make_rc_req(struct rvt_qp *qp, struct hfi1_pkt_state *ps)
newreq = 0;
if (qp->s_cur == qp->s_tail) {
/* Check if send work queue is empty. */
- smp_read_barrier_depends(); /* see post_one_send() */
if (qp->s_tail == READ_ONCE(qp->s_head)) {
clear_ahg(qp);
goto bail;
@@ -900,7 +898,6 @@ void hfi1_send_rc_ack(struct hfi1_ctxtdata *rcd,
}
/* Ensure s_rdma_ack_cnt changes are committed */
- smp_read_barrier_depends();
if (qp->s_rdma_ack_cnt) {
hfi1_queue_rc_ack(qp, is_fecn);
return;
@@ -1562,7 +1559,6 @@ static void rc_rcv_resp(struct hfi1_packet *packet)
trace_hfi1_ack(qp, psn);
/* Ignore invalid responses. */
- smp_read_barrier_depends(); /* see post_one_send */
if (cmp_psn(psn, READ_ONCE(qp->s_next_psn)) >= 0)
goto ack_done;
diff --git a/drivers/infiniband/hw/hfi1/ruc.c b/drivers/infiniband/hw/hfi1/ruc.c
index 2c7fc6e331ea..13b994738f41 100644
--- a/drivers/infiniband/hw/hfi1/ruc.c
+++ b/drivers/infiniband/hw/hfi1/ruc.c
@@ -362,7 +362,6 @@ static void ruc_loopback(struct rvt_qp *sqp)
sqp->s_flags |= RVT_S_BUSY;
again:
- smp_read_barrier_depends(); /* see post_one_send() */
if (sqp->s_last == READ_ONCE(sqp->s_head))
goto clr_busy;
wqe = rvt_get_swqe_ptr(sqp, sqp->s_last);
diff --git a/drivers/infiniband/hw/hfi1/sdma.c b/drivers/infiniband/hw/hfi1/sdma.c
index 31c8f89b5fc8..61c130dbed10 100644
--- a/drivers/infiniband/hw/hfi1/sdma.c
+++ b/drivers/infiniband/hw/hfi1/sdma.c
@@ -553,7 +553,6 @@ static void sdma_hw_clean_up_task(unsigned long opaque)
static inline struct sdma_txreq *get_txhead(struct sdma_engine *sde)
{
- smp_read_barrier_depends(); /* see sdma_update_tail() */
return sde->tx_ring[sde->tx_head & sde->sdma_mask];
}
diff --git a/drivers/infiniband/hw/hfi1/uc.c b/drivers/infiniband/hw/hfi1/uc.c
index 991bbee04821..132b63e787d1 100644
--- a/drivers/infiniband/hw/hfi1/uc.c
+++ b/drivers/infiniband/hw/hfi1/uc.c
@@ -79,7 +79,6 @@ int hfi1_make_uc_req(struct rvt_qp *qp, struct hfi1_pkt_state *ps)
if (!(ib_rvt_state_ops[qp->state] & RVT_FLUSH_SEND))
goto bail;
/* We are in the error state, flush the work request. */
- smp_read_barrier_depends(); /* see post_one_send() */
if (qp->s_last == READ_ONCE(qp->s_head))
goto bail;
/* If DMAs are in progress, we can't flush immediately. */
@@ -119,7 +118,6 @@ int hfi1_make_uc_req(struct rvt_qp *qp, struct hfi1_pkt_state *ps)
RVT_PROCESS_NEXT_SEND_OK))
goto bail;
/* Check if send work queue is empty. */
- smp_read_barrier_depends(); /* see post_one_send() */
if (qp->s_cur == READ_ONCE(qp->s_head)) {
clear_ahg(qp);
goto bail;
diff --git a/drivers/infiniband/hw/hfi1/ud.c b/drivers/infiniband/hw/hfi1/ud.c
index beb5091eccca..deb184574395 100644
--- a/drivers/infiniband/hw/hfi1/ud.c
+++ b/drivers/infiniband/hw/hfi1/ud.c
@@ -486,7 +486,6 @@ int hfi1_make_ud_req(struct rvt_qp *qp, struct hfi1_pkt_state *ps)
if (!(ib_rvt_state_ops[qp->state] & RVT_FLUSH_SEND))
goto bail;
/* We are in the error state, flush the work request. */
- smp_read_barrier_depends(); /* see post_one_send */
if (qp->s_last == READ_ONCE(qp->s_head))
goto bail;
/* If DMAs are in progress, we can't flush immediately. */
@@ -500,7 +499,6 @@ int hfi1_make_ud_req(struct rvt_qp *qp, struct hfi1_pkt_state *ps)
}
/* see post_one_send() */
- smp_read_barrier_depends();
if (qp->s_cur == READ_ONCE(qp->s_head))
goto bail;
diff --git a/drivers/infiniband/hw/qib/qib_rc.c b/drivers/infiniband/hw/qib/qib_rc.c
index 8f5754fb8579..1a785c37ad0a 100644
--- a/drivers/infiniband/hw/qib/qib_rc.c
+++ b/drivers/infiniband/hw/qib/qib_rc.c
@@ -246,7 +246,6 @@ int qib_make_rc_req(struct rvt_qp *qp, unsigned long *flags)
if (!(ib_rvt_state_ops[qp->state] & RVT_FLUSH_SEND))
goto bail;
/* We are in the error state, flush the work request. */
- smp_read_barrier_depends(); /* see post_one_send() */
if (qp->s_last == READ_ONCE(qp->s_head))
goto bail;
/* If DMAs are in progress, we can't flush immediately. */
@@ -293,7 +292,6 @@ int qib_make_rc_req(struct rvt_qp *qp, unsigned long *flags)
newreq = 0;
if (qp->s_cur == qp->s_tail) {
/* Check if send work queue is empty. */
- smp_read_barrier_depends(); /* see post_one_send() */
if (qp->s_tail == READ_ONCE(qp->s_head))
goto bail;
/*
@@ -1340,7 +1338,6 @@ static void qib_rc_rcv_resp(struct qib_ibport *ibp,
goto ack_done;
/* Ignore invalid responses. */
- smp_read_barrier_depends(); /* see post_one_send */
if (qib_cmp24(psn, READ_ONCE(qp->s_next_psn)) >= 0)
goto ack_done;
diff --git a/drivers/infiniband/hw/qib/qib_ruc.c b/drivers/infiniband/hw/qib/qib_ruc.c
index 9a37e844d4c8..4662cc7bde92 100644
--- a/drivers/infiniband/hw/qib/qib_ruc.c
+++ b/drivers/infiniband/hw/qib/qib_ruc.c
@@ -367,7 +367,6 @@ static void qib_ruc_loopback(struct rvt_qp *sqp)
sqp->s_flags |= RVT_S_BUSY;
again:
- smp_read_barrier_depends(); /* see post_one_send() */
if (sqp->s_last == READ_ONCE(sqp->s_head))
goto clr_busy;
wqe = rvt_get_swqe_ptr(sqp, sqp->s_last);
diff --git a/drivers/infiniband/hw/qib/qib_uc.c b/drivers/infiniband/hw/qib/qib_uc.c
index bddcc37ace44..70c58b88192c 100644
--- a/drivers/infiniband/hw/qib/qib_uc.c
+++ b/drivers/infiniband/hw/qib/qib_uc.c
@@ -60,7 +60,6 @@ int qib_make_uc_req(struct rvt_qp *qp, unsigned long *flags)
if (!(ib_rvt_state_ops[qp->state] & RVT_FLUSH_SEND))
goto bail;
/* We are in the error state, flush the work request. */
- smp_read_barrier_depends(); /* see post_one_send() */
if (qp->s_last == READ_ONCE(qp->s_head))
goto bail;
/* If DMAs are in progress, we can't flush immediately. */
@@ -90,7 +89,6 @@ int qib_make_uc_req(struct rvt_qp *qp, unsigned long *flags)
RVT_PROCESS_NEXT_SEND_OK))
goto bail;
/* Check if send work queue is empty. */
- smp_read_barrier_depends(); /* see post_one_send() */
if (qp->s_cur == READ_ONCE(qp->s_head))
goto bail;
/*
diff --git a/drivers/infiniband/hw/qib/qib_ud.c b/drivers/infiniband/hw/qib/qib_ud.c
index 15962ed193ce..386c3c4da0c7 100644
--- a/drivers/infiniband/hw/qib/qib_ud.c
+++ b/drivers/infiniband/hw/qib/qib_ud.c
@@ -252,7 +252,6 @@ int qib_make_ud_req(struct rvt_qp *qp, unsigned long *flags)
if (!(ib_rvt_state_ops[qp->state] & RVT_FLUSH_SEND))
goto bail;
/* We are in the error state, flush the work request. */
- smp_read_barrier_depends(); /* see post_one_send */
if (qp->s_last == READ_ONCE(qp->s_head))
goto bail;
/* If DMAs are in progress, we can't flush immediately. */
@@ -266,7 +265,6 @@ int qib_make_ud_req(struct rvt_qp *qp, unsigned long *flags)
}
/* see post_one_send() */
- smp_read_barrier_depends();
if (qp->s_cur == READ_ONCE(qp->s_head))
goto bail;
diff --git a/drivers/infiniband/sw/rdmavt/qp.c b/drivers/infiniband/sw/rdmavt/qp.c
index 9177df60742a..eae84c216e2f 100644
--- a/drivers/infiniband/sw/rdmavt/qp.c
+++ b/drivers/infiniband/sw/rdmavt/qp.c
@@ -1684,7 +1684,6 @@ static inline int rvt_qp_is_avail(
/* non-reserved operations */
if (likely(qp->s_avail))
return 0;
- smp_read_barrier_depends(); /* see rc.c */
slast = READ_ONCE(qp->s_last);
if (qp->s_head >= slast)
avail = qp->s_size - (qp->s_head - slast);
--
2.5.2
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 <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Andrea Arcangeli <[email protected]>
Cc: Minchan Kim <[email protected]>
Cc: Michal Hocko <[email protected]>
Cc: "Kirill A. Shutemov" <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: "Aneesh Kumar K.V" <[email protected]>
Cc: Claudio Imbrenda <[email protected]>
Cc: <[email protected]>
---
mm/ksm.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/mm/ksm.c b/mm/ksm.c
index be8f4576f842..c406f75957ad 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
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 <[email protected]>
Cc: Alexander Viro <[email protected]>
Cc: <[email protected]>
---
fs/dcache.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/fs/dcache.c b/fs/dcache.c
index 5c7df1df81ff..379dce86f001 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;
@@ -3047,17 +3046,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
This commit keeps only the historical and low-level discussion of
smp_read_barrier_depends().
Signed-off-by: Paul E. McKenney <[email protected]>
---
Documentation/RCU/Design/Requirements/Requirements.html | 3 ++-
Documentation/RCU/rcu_dereference.txt | 6 +-----
Documentation/RCU/whatisRCU.txt | 3 +--
Documentation/circular-buffers.txt | 3 +--
Documentation/memory-barriers.txt | 7 ++++---
5 files changed, 9 insertions(+), 13 deletions(-)
diff --git a/Documentation/RCU/Design/Requirements/Requirements.html b/Documentation/RCU/Design/Requirements/Requirements.html
index 62e847bcdcdd..571c3d75510f 100644
--- a/Documentation/RCU/Design/Requirements/Requirements.html
+++ b/Documentation/RCU/Design/Requirements/Requirements.html
@@ -581,7 +581,8 @@ This guarantee was only partially premeditated.
DYNIX/ptx used an explicit memory barrier for publication, but had nothing
resembling <tt>rcu_dereference()</tt> for subscription, nor did it
have anything resembling the <tt>smp_read_barrier_depends()</tt>
-that was later subsumed into <tt>rcu_dereference()</tt>.
+that was later subsumed into <tt>rcu_dereference()</tt> and later
+still into <tt>READ_ONCE()</tt>.
The need for these operations made itself known quite suddenly at a
late-1990s meeting with the DEC Alpha architects, back in the days when
DEC was still a free-standing company.
diff --git a/Documentation/RCU/rcu_dereference.txt b/Documentation/RCU/rcu_dereference.txt
index 1acb26b09b48..ab96227bad42 100644
--- a/Documentation/RCU/rcu_dereference.txt
+++ b/Documentation/RCU/rcu_dereference.txt
@@ -122,11 +122,7 @@ o Be very careful about comparing pointers obtained from
Note that if checks for being within an RCU read-side
critical section are not required and the pointer is never
dereferenced, rcu_access_pointer() should be used in place
- of rcu_dereference(). The rcu_access_pointer() primitive
- does not require an enclosing read-side critical section,
- and also omits the smp_read_barrier_depends() included in
- rcu_dereference(), which in turn should provide a small
- performance gain in some CPUs (e.g., the DEC Alpha).
+ of rcu_dereference().
o The comparison is against a pointer that references memory
that was initialized "a long time ago." The reason
diff --git a/Documentation/RCU/whatisRCU.txt b/Documentation/RCU/whatisRCU.txt
index df62466da4e0..a27fbfb0efb8 100644
--- a/Documentation/RCU/whatisRCU.txt
+++ b/Documentation/RCU/whatisRCU.txt
@@ -600,8 +600,7 @@ don't forget about them when submitting patches making use of RCU!]
#define rcu_dereference(p) \
({ \
- typeof(p) _________p1 = p; \
- smp_read_barrier_depends(); \
+ typeof(p) _________p1 = READ_ONCE(p); \
(_________p1); \
})
diff --git a/Documentation/circular-buffers.txt b/Documentation/circular-buffers.txt
index d4628174b7c5..53e51caa3347 100644
--- a/Documentation/circular-buffers.txt
+++ b/Documentation/circular-buffers.txt
@@ -220,8 +220,7 @@ before it writes the new tail pointer, which will erase the item.
Note the use of READ_ONCE() and smp_load_acquire() to read the
opposition index. This prevents the compiler from discarding and
-reloading its cached value - which some compilers will do across
-smp_read_barrier_depends(). This isn't strictly needed if you can
+reloading its cached value. This isn't strictly needed if you can
be sure that the opposition index will _only_ be used the once.
The smp_load_acquire() additionally forces the CPU to order against
subsequent memory references. Similarly, smp_store_release() is used
diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
index 2269e58fa073..c21a9b25cacf 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -236,9 +236,10 @@ There are some minimal guarantees that may be expected of a CPU:
and always in that order. On most systems, 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
+ mischief. Please note that you should almost always use something
like READ_ONCE() or rcu_dereference() instead of open-coding
- smp_read_barrier_depends().
+ smp_read_barrier_depends(), the only exceptions being in DEC Alpha
+ architecture-specific code and in ACCESS_ONCE itself.
(*) Overlapping loads and stores within a particular CPU will appear to be
ordered within that CPU. This means that for:
@@ -1816,7 +1817,7 @@ The Linux kernel has eight basic CPU memory barriers:
GENERAL mb() smp_mb()
WRITE wmb() smp_wmb()
READ rmb() smp_rmb()
- DATA DEPENDENCY read_barrier_depends() smp_read_barrier_depends()
+ DATA DEPENDENCY READ_ONCE()
All memory barriers except the data dependency barriers imply a compiler
--
2.5.2
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 <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Alexander Shishkin <[email protected]>
---
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
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 <[email protected]>
Cc: David Howells <[email protected]>
Cc: James Morris <[email protected]>
Cc: "Serge E. Hallyn" <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
---
security/keys/keyring.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index d0bccebbd3b5..41bcf57e96f2 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -713,7 +713,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;
@@ -723,8 +722,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;
@@ -736,7 +733,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));
}
@@ -744,7 +740,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 */
@@ -792,14 +787,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
Now that smp_read_barrier_depends() has been de-emphasized, the less
said about it, the better.
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: Pablo Neira Ayuso <[email protected]>
Cc: Jozsef Kadlecsik <[email protected]>
Cc: Florian Westphal <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
---
include/linux/netfilter/nfnetlink.h | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/include/linux/netfilter/nfnetlink.h b/include/linux/netfilter/nfnetlink.h
index 495ba4dd9da5..34551f8aaf9d 100644
--- a/include/linux/netfilter/nfnetlink.h
+++ b/include/linux/netfilter/nfnetlink.h
@@ -67,8 +67,7 @@ static inline bool lockdep_nfnl_is_held(__u8 subsys_id)
* @ss: The nfnetlink subsystem ID
*
* Return the value of the specified RCU-protected pointer, but omit
- * both the smp_read_barrier_depends() and the READ_ONCE(), because
- * caller holds the NFNL subsystem mutex.
+ * the READ_ONCE(), because caller holds the NFNL subsystem mutex.
*/
#define nfnl_dereference(p, ss) \
rcu_dereference_protected(p, lockdep_nfnl_is_held(ss))
--
2.5.2
Now that both smp_read_barrier_depends() and read_barrier_depends()
are being de-emphasized, warn if any are added.
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: Andy Whitcroft <[email protected]>
Cc: Joe Perches <[email protected]>
---
scripts/checkpatch.pl | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 95cda3ecc66b..25f7098e2ad3 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5586,6 +5586,12 @@ sub process {
}
}
+# check for smp_read_barrier_depends and read_barrier_depends
+ if ($line =~ /\b(smp_|)read_barrier_depends\(/) {
+ WARN("READ_BARRIER_DEPENDS",
+ "Dependency barriers should only be used in READ_ONCE or DEC Alpha code" . $herecurr);
+ }
+
# check of hardware specific defines
if ($line =~ m@^.\s*\#\s*if.*\b(__i386__|__powerpc64__|__sun__|__s390x__)\b@ && $realfile !~ m@include/asm-@) {
CHK("ARCH_DEFINES",
--
2.5.2
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 <[email protected]>
Cc: Jonathan Corbet <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Alexander Kuleshov <[email protected]>
Cc: David Howells <[email protected]>
---
lib/assoc_array.c | 37 ++++++++++++-------------------------
1 file changed, 12 insertions(+), 25 deletions(-)
diff --git a/lib/assoc_array.c b/lib/assoc_array.c
index b77d51da8c73..c6659cb37033 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;
@@ -331,20 +320,18 @@ void *assoc_array_find(const struct assoc_array *array,
return NULL;
node = result.terminal_node.node;
- smp_read_barrier_depends();
/* 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;
}
--
2.5.2
Now that smp_read_barrier_depends() has been de-emphasized, the less
said about it, the better.
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Kate Stewart <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Philippe Ombredanne <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
---
include/linux/genetlink.h | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/include/linux/genetlink.h b/include/linux/genetlink.h
index ecc2928e8046..bc738504ab4a 100644
--- a/include/linux/genetlink.h
+++ b/include/linux/genetlink.h
@@ -31,8 +31,7 @@ extern wait_queue_head_t genl_sk_destructing_waitq;
* @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 genl mutex.
+ * the READ_ONCE(), because caller holds genl mutex.
*/
#define genl_dereference(p) \
rcu_dereference_protected(p, lockdep_genl_is_held())
--
2.5.2
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 <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Ingo Molnar <[email protected]>
---
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 f189a8a3bbb8..bcf4cf26b8c8 100644
--- a/include/linux/seqlock.h
+++ b/include/linux/seqlock.h
@@ -278,9 +278,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
On Fri, 2017-12-01 at 11:51 -0800, Paul E. McKenney wrote:
> Now that both smp_read_barrier_depends() and read_barrier_depends()
> are being de-emphasized, warn if any are added.
This would also warn on existing files when run
with ./scripts/checkpatch.pl -f <file>
Do you want it to check new patches only?
If so the test could become "if (!$file && etc...)
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 95cda3ecc66b..25f7098e2ad3 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -5586,6 +5586,12 @@ sub process {
> }
> }
>
> +# check for smp_read_barrier_depends and read_barrier_depends
> + if ($line =~ /\b(smp_|)read_barrier_depends\(/) {
Must become
+ if ($line =~ /\b(smp_|)read_barrier_depends\s*\(/) {
similar to the lines above this as there are sometimes
spaces between function name and argument parentheses.
On Fri, Dec 01, 2017 at 12:14:17PM -0800, Joe Perches wrote:
> On Fri, 2017-12-01 at 11:51 -0800, Paul E. McKenney wrote:
> > Now that both smp_read_barrier_depends() and read_barrier_depends()
> > are being de-emphasized, warn if any are added.
>
> This would also warn on existing files when run
> with ./scripts/checkpatch.pl -f <file>
>
> Do you want it to check new patches only?
>
> If so the test could become "if (!$file && etc...)
>
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 95cda3ecc66b..25f7098e2ad3 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -5586,6 +5586,12 @@ sub process {
> > }
> > }
> >
> > +# check for smp_read_barrier_depends and read_barrier_depends
> > + if ($line =~ /\b(smp_|)read_barrier_depends\(/) {
>
> Must become
>
> + if ($line =~ /\b(smp_|)read_barrier_depends\s*\(/) {
>
> similar to the lines above this as there are sometimes
> spaces between function name and argument parentheses.
Good points! Like this?
Thanx, Paul
------------------------------------------------------------------------
commit ff155ce179aab891dbe2ca80f82a453383fd165a
Author: Paul E. McKenney <[email protected]>
Date: Mon Nov 27 09:37:35 2017 -0800
checkpatch: Add warnings for {smp_,}read_barrier_depends()
Now that both smp_read_barrier_depends() and read_barrier_depends()
are being de-emphasized, warn if any are added.
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: Andy Whitcroft <[email protected]>
Cc: Joe Perches <[email protected]>
[ paulmck: Skipped checking files and handled whitespace per Joe Perches. ]
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 95cda3ecc66b..0d90518d4147 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5586,6 +5586,12 @@ sub process {
}
}
+# check for smp_read_barrier_depends and read_barrier_depends
+ if (!$file && $line =~ /\b(smp_|)read_barrier_depends\s*\(/) {
+ WARN("READ_BARRIER_DEPENDS",
+ "Dependency barriers should only be used in READ_ONCE or DEC Alpha code" . $herecurr);
+ }
+
# check of hardware specific defines
if ($line =~ m@^.\s*\#\s*if.*\b(__i386__|__powerpc64__|__sun__|__s390x__)\b@ && $realfile !~ m@include/asm-@) {
CHK("ARCH_DEFINES",
On Fri, Dec 01, 2017 at 11:51:11AM -0800, Paul E. McKenney wrote:
> The smp_read_barrier_depends() does nothing at all except on DEC Alpha,
> and no current DEC Alpha systems use Infiniband:
>
> lkml.kernel.org/r/20171023085921.jwbntptn6ictbnvj@tower
I understand DEC Alpha has PCI, and we continue to support several
RDMA devices that used the PCI bus.
However, the hif1, rdma vt and qib drivers modified in this patch are
PCI-Express ONLY. So I think this idea is fine, but would revise the
commit message to focus on PCI-E not Infiniband.
> drivers/dma/ioat/dma.c | 2 --
> drivers/infiniband/Kconfig | 1 +
Did you mean to have ioat/dma.c in this patch?
Jason
On Fri, Dec 01, 2017 at 05:11:09PM -0700, Jason Gunthorpe wrote:
> On Fri, Dec 01, 2017 at 11:51:11AM -0800, Paul E. McKenney wrote:
> > The smp_read_barrier_depends() does nothing at all except on DEC Alpha,
> > and no current DEC Alpha systems use Infiniband:
> >
> > lkml.kernel.org/r/20171023085921.jwbntptn6ictbnvj@tower
>
> I understand DEC Alpha has PCI, and we continue to support several
> RDMA devices that used the PCI bus.
>
> However, the hif1, rdma vt and qib drivers modified in this patch are
> PCI-Express ONLY. So I think this idea is fine, but would revise the
> commit message to focus on PCI-E not Infiniband.
Hmmm... It is not just the commit log, but also the Kconfig change.
I am not as worried as I might be about the few museum-piece DEC
Alpha systems suddenly sporting new RDMA PCI devices, but I am worried
about getting a more complex change right.
And if someone really does add a PCI RDMA device to their DEC Alpha,
and this patch causes them trouble, we can work out what to do about
it when and if that happens.
> > drivers/dma/ioat/dma.c | 2 --
> > drivers/infiniband/Kconfig | 1 +
>
> Did you mean to have ioat/dma.c in this patch?
Indeed I did not, thank you for catching this! Please see below for
updated ioat-free patch.
Thanx, Paul
------------------------------------------------------------------------
commit c389c98ec5f4a7aa4c36853e89801eb5ea81870e
Author: Paul E. McKenney <[email protected]>
Date: Mon Nov 27 09:04:22 2017 -0800
drivers/infiniband: Remove now-redundant smp_read_barrier_depends()
The smp_read_barrier_depends() does nothing at all except on DEC Alpha,
and no current DEC Alpha systems use Infiniband:
lkml.kernel.org/r/20171023085921.jwbntptn6ictbnvj@tower
This commit therefore makes Infiniband depend on !ALPHA and removes
the now-ineffective invocations of smp_read_barrier_depends() from
the InfiniBand driver.
Please note that this patch should not be construed as my saying that
InfiniBand's memory ordering is correct, but rather that this patch does
not in any way affect InfiniBand's correctness. In other words, the
result of applying this patch is bug-for-bug compatible with the original.
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: Doug Ledford <[email protected]>
Cc: Jason Gunthorpe <[email protected]>
Cc: Richard Henderson <[email protected]>
Cc: Ivan Kokshaysky <[email protected]>
Cc: Matt Turner <[email protected]>
Cc: Michael Cree <[email protected]>
Cc: Andrea Parri <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
[ paulmck: Removed drivers/dma/ioat/dma.c per Jason Gunthorpe's feedback. ]
diff --git a/drivers/infiniband/Kconfig b/drivers/infiniband/Kconfig
index 98ac46ed7214..3bb6e35b0bbf 100644
--- a/drivers/infiniband/Kconfig
+++ b/drivers/infiniband/Kconfig
@@ -4,6 +4,7 @@ menuconfig INFINIBAND
depends on NET
depends on INET
depends on m || IPV6 != m
+ depends on !ALPHA
select IRQ_POLL
---help---
Core support for InfiniBand (IB). Make sure to also select
diff --git a/drivers/infiniband/hw/hfi1/rc.c b/drivers/infiniband/hw/hfi1/rc.c
index fd01a760259f..f527bcda4650 100644
--- a/drivers/infiniband/hw/hfi1/rc.c
+++ b/drivers/infiniband/hw/hfi1/rc.c
@@ -302,7 +302,6 @@ int hfi1_make_rc_req(struct rvt_qp *qp, struct hfi1_pkt_state *ps)
if (!(ib_rvt_state_ops[qp->state] & RVT_FLUSH_SEND))
goto bail;
/* We are in the error state, flush the work request. */
- smp_read_barrier_depends(); /* see post_one_send() */
if (qp->s_last == READ_ONCE(qp->s_head))
goto bail;
/* If DMAs are in progress, we can't flush immediately. */
@@ -346,7 +345,6 @@ int hfi1_make_rc_req(struct rvt_qp *qp, struct hfi1_pkt_state *ps)
newreq = 0;
if (qp->s_cur == qp->s_tail) {
/* Check if send work queue is empty. */
- smp_read_barrier_depends(); /* see post_one_send() */
if (qp->s_tail == READ_ONCE(qp->s_head)) {
clear_ahg(qp);
goto bail;
@@ -900,7 +898,6 @@ void hfi1_send_rc_ack(struct hfi1_ctxtdata *rcd,
}
/* Ensure s_rdma_ack_cnt changes are committed */
- smp_read_barrier_depends();
if (qp->s_rdma_ack_cnt) {
hfi1_queue_rc_ack(qp, is_fecn);
return;
@@ -1562,7 +1559,6 @@ static void rc_rcv_resp(struct hfi1_packet *packet)
trace_hfi1_ack(qp, psn);
/* Ignore invalid responses. */
- smp_read_barrier_depends(); /* see post_one_send */
if (cmp_psn(psn, READ_ONCE(qp->s_next_psn)) >= 0)
goto ack_done;
diff --git a/drivers/infiniband/hw/hfi1/ruc.c b/drivers/infiniband/hw/hfi1/ruc.c
index 2c7fc6e331ea..13b994738f41 100644
--- a/drivers/infiniband/hw/hfi1/ruc.c
+++ b/drivers/infiniband/hw/hfi1/ruc.c
@@ -362,7 +362,6 @@ static void ruc_loopback(struct rvt_qp *sqp)
sqp->s_flags |= RVT_S_BUSY;
again:
- smp_read_barrier_depends(); /* see post_one_send() */
if (sqp->s_last == READ_ONCE(sqp->s_head))
goto clr_busy;
wqe = rvt_get_swqe_ptr(sqp, sqp->s_last);
diff --git a/drivers/infiniband/hw/hfi1/sdma.c b/drivers/infiniband/hw/hfi1/sdma.c
index 31c8f89b5fc8..61c130dbed10 100644
--- a/drivers/infiniband/hw/hfi1/sdma.c
+++ b/drivers/infiniband/hw/hfi1/sdma.c
@@ -553,7 +553,6 @@ static void sdma_hw_clean_up_task(unsigned long opaque)
static inline struct sdma_txreq *get_txhead(struct sdma_engine *sde)
{
- smp_read_barrier_depends(); /* see sdma_update_tail() */
return sde->tx_ring[sde->tx_head & sde->sdma_mask];
}
diff --git a/drivers/infiniband/hw/hfi1/uc.c b/drivers/infiniband/hw/hfi1/uc.c
index 991bbee04821..132b63e787d1 100644
--- a/drivers/infiniband/hw/hfi1/uc.c
+++ b/drivers/infiniband/hw/hfi1/uc.c
@@ -79,7 +79,6 @@ int hfi1_make_uc_req(struct rvt_qp *qp, struct hfi1_pkt_state *ps)
if (!(ib_rvt_state_ops[qp->state] & RVT_FLUSH_SEND))
goto bail;
/* We are in the error state, flush the work request. */
- smp_read_barrier_depends(); /* see post_one_send() */
if (qp->s_last == READ_ONCE(qp->s_head))
goto bail;
/* If DMAs are in progress, we can't flush immediately. */
@@ -119,7 +118,6 @@ int hfi1_make_uc_req(struct rvt_qp *qp, struct hfi1_pkt_state *ps)
RVT_PROCESS_NEXT_SEND_OK))
goto bail;
/* Check if send work queue is empty. */
- smp_read_barrier_depends(); /* see post_one_send() */
if (qp->s_cur == READ_ONCE(qp->s_head)) {
clear_ahg(qp);
goto bail;
diff --git a/drivers/infiniband/hw/hfi1/ud.c b/drivers/infiniband/hw/hfi1/ud.c
index beb5091eccca..deb184574395 100644
--- a/drivers/infiniband/hw/hfi1/ud.c
+++ b/drivers/infiniband/hw/hfi1/ud.c
@@ -486,7 +486,6 @@ int hfi1_make_ud_req(struct rvt_qp *qp, struct hfi1_pkt_state *ps)
if (!(ib_rvt_state_ops[qp->state] & RVT_FLUSH_SEND))
goto bail;
/* We are in the error state, flush the work request. */
- smp_read_barrier_depends(); /* see post_one_send */
if (qp->s_last == READ_ONCE(qp->s_head))
goto bail;
/* If DMAs are in progress, we can't flush immediately. */
@@ -500,7 +499,6 @@ int hfi1_make_ud_req(struct rvt_qp *qp, struct hfi1_pkt_state *ps)
}
/* see post_one_send() */
- smp_read_barrier_depends();
if (qp->s_cur == READ_ONCE(qp->s_head))
goto bail;
diff --git a/drivers/infiniband/hw/qib/qib_rc.c b/drivers/infiniband/hw/qib/qib_rc.c
index 8f5754fb8579..1a785c37ad0a 100644
--- a/drivers/infiniband/hw/qib/qib_rc.c
+++ b/drivers/infiniband/hw/qib/qib_rc.c
@@ -246,7 +246,6 @@ int qib_make_rc_req(struct rvt_qp *qp, unsigned long *flags)
if (!(ib_rvt_state_ops[qp->state] & RVT_FLUSH_SEND))
goto bail;
/* We are in the error state, flush the work request. */
- smp_read_barrier_depends(); /* see post_one_send() */
if (qp->s_last == READ_ONCE(qp->s_head))
goto bail;
/* If DMAs are in progress, we can't flush immediately. */
@@ -293,7 +292,6 @@ int qib_make_rc_req(struct rvt_qp *qp, unsigned long *flags)
newreq = 0;
if (qp->s_cur == qp->s_tail) {
/* Check if send work queue is empty. */
- smp_read_barrier_depends(); /* see post_one_send() */
if (qp->s_tail == READ_ONCE(qp->s_head))
goto bail;
/*
@@ -1340,7 +1338,6 @@ static void qib_rc_rcv_resp(struct qib_ibport *ibp,
goto ack_done;
/* Ignore invalid responses. */
- smp_read_barrier_depends(); /* see post_one_send */
if (qib_cmp24(psn, READ_ONCE(qp->s_next_psn)) >= 0)
goto ack_done;
diff --git a/drivers/infiniband/hw/qib/qib_ruc.c b/drivers/infiniband/hw/qib/qib_ruc.c
index 9a37e844d4c8..4662cc7bde92 100644
--- a/drivers/infiniband/hw/qib/qib_ruc.c
+++ b/drivers/infiniband/hw/qib/qib_ruc.c
@@ -367,7 +367,6 @@ static void qib_ruc_loopback(struct rvt_qp *sqp)
sqp->s_flags |= RVT_S_BUSY;
again:
- smp_read_barrier_depends(); /* see post_one_send() */
if (sqp->s_last == READ_ONCE(sqp->s_head))
goto clr_busy;
wqe = rvt_get_swqe_ptr(sqp, sqp->s_last);
diff --git a/drivers/infiniband/hw/qib/qib_uc.c b/drivers/infiniband/hw/qib/qib_uc.c
index bddcc37ace44..70c58b88192c 100644
--- a/drivers/infiniband/hw/qib/qib_uc.c
+++ b/drivers/infiniband/hw/qib/qib_uc.c
@@ -60,7 +60,6 @@ int qib_make_uc_req(struct rvt_qp *qp, unsigned long *flags)
if (!(ib_rvt_state_ops[qp->state] & RVT_FLUSH_SEND))
goto bail;
/* We are in the error state, flush the work request. */
- smp_read_barrier_depends(); /* see post_one_send() */
if (qp->s_last == READ_ONCE(qp->s_head))
goto bail;
/* If DMAs are in progress, we can't flush immediately. */
@@ -90,7 +89,6 @@ int qib_make_uc_req(struct rvt_qp *qp, unsigned long *flags)
RVT_PROCESS_NEXT_SEND_OK))
goto bail;
/* Check if send work queue is empty. */
- smp_read_barrier_depends(); /* see post_one_send() */
if (qp->s_cur == READ_ONCE(qp->s_head))
goto bail;
/*
diff --git a/drivers/infiniband/hw/qib/qib_ud.c b/drivers/infiniband/hw/qib/qib_ud.c
index 15962ed193ce..386c3c4da0c7 100644
--- a/drivers/infiniband/hw/qib/qib_ud.c
+++ b/drivers/infiniband/hw/qib/qib_ud.c
@@ -252,7 +252,6 @@ int qib_make_ud_req(struct rvt_qp *qp, unsigned long *flags)
if (!(ib_rvt_state_ops[qp->state] & RVT_FLUSH_SEND))
goto bail;
/* We are in the error state, flush the work request. */
- smp_read_barrier_depends(); /* see post_one_send */
if (qp->s_last == READ_ONCE(qp->s_head))
goto bail;
/* If DMAs are in progress, we can't flush immediately. */
@@ -266,7 +265,6 @@ int qib_make_ud_req(struct rvt_qp *qp, unsigned long *flags)
}
/* see post_one_send() */
- smp_read_barrier_depends();
if (qp->s_cur == READ_ONCE(qp->s_head))
goto bail;
diff --git a/drivers/infiniband/sw/rdmavt/qp.c b/drivers/infiniband/sw/rdmavt/qp.c
index 9177df60742a..eae84c216e2f 100644
--- a/drivers/infiniband/sw/rdmavt/qp.c
+++ b/drivers/infiniband/sw/rdmavt/qp.c
@@ -1684,7 +1684,6 @@ static inline int rvt_qp_is_avail(
/* non-reserved operations */
if (likely(qp->s_avail))
return 0;
- smp_read_barrier_depends(); /* see rc.c */
slast = READ_ONCE(qp->s_last);
if (qp->s_head >= slast)
avail = qp->s_size - (qp->s_head - slast);
On Fri, 2017-12-01 at 13:44 -0800, Paul E. McKenney wrote:
> On Fri, Dec 01, 2017 at 12:14:17PM -0800, Joe Perches wrote:
> > On Fri, 2017-12-01 at 11:51 -0800, Paul E. McKenney wrote:
> > > Now that both smp_read_barrier_depends() and read_barrier_depends()
> > > are being de-emphasized, warn if any are added.
> >
> > This would also warn on existing files when run
> > with ./scripts/checkpatch.pl -f <file>
> >
> > Do you want it to check new patches only?
> >
> > If so the test could become "if (!$file && etc...)
> >
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> > > @@ -5586,6 +5586,12 @@ sub process {
> > > }
> > > }
> > >
> > > +# check for smp_read_barrier_depends and read_barrier_depends
> > > + if ($line =~ /\b(smp_|)read_barrier_depends\(/) {
> >
> > Must become
> >
> > + if ($line =~ /\b(smp_|)read_barrier_depends\s*\(/) {
> >
> > similar to the lines above this as there are sometimes
> > spaces between function name and argument parentheses.
>
> Good points! Like this?
[]
> commit ff155ce179aab891dbe2ca80f82a453383fd165a
> Author: Paul E. McKenney <[email protected]>
> Date: Mon Nov 27 09:37:35 2017 -0800
>
> checkpatch: Add warnings for {smp_,}read_barrier_depends()
>
> Now that both smp_read_barrier_depends() and read_barrier_depends()
> are being de-emphasized, warn if any are added.
>
> Signed-off-by: Paul E. McKenney <[email protected]>
> Cc: Andy Whitcroft <[email protected]>
> Cc: Joe Perches <[email protected]>
> [ paulmck: Skipped checking files and handled whitespace per Joe Perches. ]
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -5586,6 +5586,12 @@ sub process {
> }
> }
>
> +# check for smp_read_barrier_depends and read_barrier_depends
> + if (!$file && $line =~ /\b(smp_|)read_barrier_depends\s*\(/) {
> + WARN("READ_BARRIER_DEPENDS",
> + "Dependency barriers should only be used in READ_ONCE or DEC Alpha code" . $herecurr);
Almost right. Missing a '\n' after "DEC Alpha code".
Without the '\n', running checkpatch with --terse outputs badly.
If you really wanted to optimize, you could make the first (smp_|)
become (?:smp_|) to avoid the unused capture group.
Or perhaps this could emit the actual type in the message like:
if (!$file && $line =~ /\b((?:smp_|)read_barrier_depends)\s*\(/ {
WARN("READ_BARRIER_DEPENDS",
"$1 should only be used in READ_ONCE or DEC Alpha code\n" . $herecurr);
On Fri, 1 Dec 2017, Paul E. McKenney wrote:
> 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 <[email protected]>
> Cc: David Howells <[email protected]>
> Cc: James Morris <[email protected]>
> Cc: "Serge E. Hallyn" <[email protected]>
> Cc: <[email protected]>
> Cc: <[email protected]>
Reviewed-by: James Morris <[email protected]>
--
James Morris
<[email protected]>
Paul E. McKenney <[email protected]> wrote:
> - 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
The CPU may now issue two barriers in addition to the loads, so should we show
this? E.g.:
Q = LOAD P, BARRIER, D = LOAD *Q, BARRIER
David
On Mon, Dec 04, 2017 at 03:38:56PM +0000, David Howells wrote:
> Paul E. McKenney <[email protected]> wrote:
>
> > - 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
>
> The CPU may now issue two barriers in addition to the loads, so should we show
> this? E.g.:
>
> Q = LOAD P, BARRIER, D = LOAD *Q, BARRIER
Good point! How about as shown in the updated patch below?
Thanx, Paul
------------------------------------------------------------------------
commit 40555946447a394889243e4393e312f65d847e1e
Author: Paul E. McKenney <[email protected]>
Date: Mon Oct 9 09:15:21 2017 -0700
doc: READ_ONCE() now implies smp_barrier_depends()
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 <[email protected]>
[ paulmck: Added MEMORY_BARRIER instructions from DEC Alpha from
READ_ONCE(), per David Howells's feedback. ]
diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
index 479ecec80593..13fd35b6a597 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -227,17 +227,20 @@ 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().
+ and always in that order. However, on DEC Alpha, READ_ONCE() also
+ emits a memory-barrier instruction, so that a DEC Alpha CPU will
+ instead issue the following memory operations:
+
+ Q = LOAD P, MEMORY_BARRIER, D = LOAD *Q, MEMORY_BARRIER
+
+ Whether on DEC Alpha or not, the READ_ONCE() also prevents compiler
+ mischief.
(*) Overlapping loads and stores within a particular CPU will appear to be
ordered within that CPU. This means that for:
On Mon, Dec 04, 2017 at 11:59:22AM +1100, James Morris wrote:
> On Fri, 1 Dec 2017, Paul E. McKenney wrote:
>
> > 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 <[email protected]>
> > Cc: David Howells <[email protected]>
> > Cc: James Morris <[email protected]>
> > Cc: "Serge E. Hallyn" <[email protected]>
> > Cc: <[email protected]>
> > Cc: <[email protected]>
>
> Reviewed-by: James Morris <[email protected]>
Applied, thank you for the review!
Thanx, Paul
On Fri, Dec 01, 2017 at 08:45:13PM -0800, Joe Perches wrote:
> On Fri, 2017-12-01 at 13:44 -0800, Paul E. McKenney wrote:
[ . . . ]
> > Good points! Like this?
> []
> > commit ff155ce179aab891dbe2ca80f82a453383fd165a
> > Author: Paul E. McKenney <[email protected]>
> > Date: Mon Nov 27 09:37:35 2017 -0800
> >
> > checkpatch: Add warnings for {smp_,}read_barrier_depends()
> >
> > Now that both smp_read_barrier_depends() and read_barrier_depends()
> > are being de-emphasized, warn if any are added.
> >
> > Signed-off-by: Paul E. McKenney <[email protected]>
> > Cc: Andy Whitcroft <[email protected]>
> > Cc: Joe Perches <[email protected]>
> > [ paulmck: Skipped checking files and handled whitespace per Joe Perches. ]
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -5586,6 +5586,12 @@ sub process {
> > }
> > }
> >
> > +# check for smp_read_barrier_depends and read_barrier_depends
> > + if (!$file && $line =~ /\b(smp_|)read_barrier_depends\s*\(/) {
> > + WARN("READ_BARRIER_DEPENDS",
> > + "Dependency barriers should only be used in READ_ONCE or DEC Alpha code" . $herecurr);
>
> Almost right. Missing a '\n' after "DEC Alpha code".
> Without the '\n', running checkpatch with --terse outputs badly.
>
> If you really wanted to optimize, you could make the first (smp_|)
> become (?:smp_|) to avoid the unused capture group.
> Or perhaps this could emit the actual type in the message like:
>
> if (!$file && $line =~ /\b((?:smp_|)read_barrier_depends)\s*\(/ {
> WARN("READ_BARRIER_DEPENDS",
> "$1 should only be used in READ_ONCE or DEC Alpha code\n" . $herecurr);
How about like this? (s/$1/$1read_barrier_depends/).
Thanx, Paul
------------------------------------------------------------------------
commit e4f1d781e33c150547c68e55099794b98ee14d5e
Author: Paul E. McKenney <[email protected]>
Date: Mon Nov 27 09:37:35 2017 -0800
checkpatch: Add warnings for {smp_,}read_barrier_depends()
Now that both smp_read_barrier_depends() and read_barrier_depends()
are being de-emphasized, warn if any are added.
Signed-off-by: Paul E. McKenney <[email protected]>
Cc: Andy Whitcroft <[email protected]>
Cc: Joe Perches <[email protected]>
[ paulmck: Skipped checking files and handled whitespace per Joe Perches. ]
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 95cda3ecc66b..9a384bfe2bd5 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5586,6 +5586,12 @@ sub process {
}
}
+# check for smp_read_barrier_depends and read_barrier_depends
+ if (!$file && $line =~ /\b(smp_|)read_barrier_depends\s*\(/) {
+ WARN("READ_BARRIER_DEPENDS",
+ "$1read_barrier_depends should only be used in READ_ONCE or DEC Alpha code\n" . $herecurr);
+ }
+
# check of hardware specific defines
if ($line =~ m@^.\s*\#\s*if.*\b(__i386__|__powerpc64__|__sun__|__s390x__)\b@ && $realfile !~ m@include/asm-@) {
CHK("ARCH_DEFINES",
On Mon, 2017-12-04 at 11:06 -0800, Paul E. McKenney wrote:
> On Fri, Dec 01, 2017 at 08:45:13PM -0800, Joe Perches wrote:
> > On Fri, 2017-12-01 at 13:44 -0800, Paul E. McKenney wrote:
>
> > If you really wanted to optimize, you could make the first (smp_|)
> > become (?:smp_|) to avoid the unused capture group.
> > Or perhaps this could emit the actual type in the message like:
> >
> > if (!$file && $line =~ /\b((?:smp_|)read_barrier_depends)\s*\(/ {
> > WARN("READ_BARRIER_DEPENDS",
> > "$1 should only be used in READ_ONCE or DEC Alpha code\n" . $herecurr);
>
> How about like this? (s/$1/$1read_barrier_depends/).
Works for me.
Acked-by: Joe Perches <[email protected]>
Are you going to merge this?
Normally akpm does checkpatch merging but whatever
works for you is OK by me.
cheers, Joe
> commit e4f1d781e33c150547c68e55099794b98ee14d5e
> Author: Paul E. McKenney <[email protected]>
> Date: Mon Nov 27 09:37:35 2017 -0800
> {smp_,}read_barrier_depends()
>
> Now that both smp_read_barrier_depends() and read_barrier_depends()
> are being de-emphasized, warn if any are added.
>
> Signed-off-by: Paul E. McKenney <[email protected]>
> Cc: Andy Whitcroft <[email protected]>
> Cc: Joe Perches <[email protected]>
> [ paulmck: Skipped checking files and handled whitespace per Joe Perches.
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 95cda3ecc66b..9a384bfe2bd5 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -5586,6 +5586,12 @@ sub process {
> }
> }
>
> +# check for smp_read_barrier_depends and read_barrier_depends
> + if (!$file && $line =~ /\b(smp_|)read_barrier_depends\s*\(/) {
> + WARN("READ_BARRIER_DEPENDS",
> + "$1read_barrier_depends should only be used in READ_ONCE or DEC Alpha code\n" . $herecurr);
> + }
> +
> # check of hardware specific defines
> if ($line =~ m@^.\s*\#\s*if.*\b(__i386__|__powerpc64__|__sun__|__s390x__)\b@ && $realfile !~ m@include/asm-@) {
> CHK("ARCH_DEFINES",
>
On Mon, Dec 04, 2017 at 10:52:15AM -0800, Paul E. McKenney wrote:
> On Mon, Dec 04, 2017 at 03:38:56PM +0000, David Howells wrote:
> > Paul E. McKenney <[email protected]> wrote:
> >
> > > - 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
> >
> > The CPU may now issue two barriers in addition to the loads, so should we show
> > this? E.g.:
> >
> > Q = LOAD P, BARRIER, D = LOAD *Q, BARRIER
>
> Good point! How about as shown in the updated patch below?
Humm, I thought the idea was to completely remove read_barrier_depends
from the lkmm and memory-barriers.txt, making it an Alpha implementation
detail.
On Mon, Dec 04, 2017 at 10:54:48PM +0100, Peter Zijlstra wrote:
> On Mon, Dec 04, 2017 at 10:52:15AM -0800, Paul E. McKenney wrote:
> > On Mon, Dec 04, 2017 at 03:38:56PM +0000, David Howells wrote:
> > > Paul E. McKenney <[email protected]> wrote:
> > >
> > > > - 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
> > >
> > > The CPU may now issue two barriers in addition to the loads, so should we show
> > > this? E.g.:
> > >
> > > Q = LOAD P, BARRIER, D = LOAD *Q, BARRIER
> >
> > Good point! How about as shown in the updated patch below?
>
> Humm, I thought the idea was to completely remove read_barrier_depends
> from the lkmm and memory-barriers.txt, making it an Alpha implementation
> detail.
That was indeed my hope, but a too-abrupt departure of DEC Alpha seemed
to be causing some confusion, so I jumped on David's suggested change. My
hope now is to slowly remove mention of DEC Alpha from the documentation.
Hmmm... Maybe we need an LWN article on how we are weaning the memory
model from its historical DEC Alpha influences? That might get the word
out more effectively.
Thanx, Paul
Peter Zijlstra <[email protected]> wrote:
> > Good point! How about as shown in the updated patch below?
>
> Humm, I thought the idea was to completely remove read_barrier_depends
> from the lkmm and memory-barriers.txt, making it an Alpha implementation
> detail.
memory-barriers.txt explains how the barriers used by the kernel work, amongst
other things.
Don't forget, btw, that Blackfin uses it also.
David
On Mon, Dec 04, 2017 at 10:39:05PM +0000, David Howells wrote:
> Peter Zijlstra <[email protected]> wrote:
>
> > > Good point! How about as shown in the updated patch below?
> >
> > Humm, I thought the idea was to completely remove read_barrier_depends
> > from the lkmm and memory-barriers.txt, making it an Alpha implementation
> > detail.
>
> memory-barriers.txt explains how the barriers used by the kernel work, amongst
> other things.
The thing is, read-barrier-depends will no longer be used. read-read
dependencies will henceforth be assumed to 'just work'.
> Don't forget, btw, that Blackfin uses it also.
We should delete that blackfin code, its beyond insane.
> commit c389c98ec5f4a7aa4c36853e89801eb5ea81870e
> Author: Paul E. McKenney <[email protected]>
> Date: Mon Nov 27 09:04:22 2017 -0800
>
> drivers/infiniband: Remove now-redundant smp_read_barrier_depends()
>
> The smp_read_barrier_depends() does nothing at all except on DEC Alpha,
> and no current DEC Alpha systems use Infiniband:
>
> lkml.kernel.org/r/20171023085921.jwbntptn6ictbnvj@tower
>
> This commit therefore makes Infiniband depend on !ALPHA and removes
> the now-ineffective invocations of smp_read_barrier_depends() from
> the InfiniBand driver.
>
> Please note that this patch should not be construed as my saying that
> InfiniBand's memory ordering is correct, but rather that this patch does
> not in any way affect InfiniBand's correctness. In other words, the
> result of applying this patch is bug-for-bug compatible with the original.
>
> Signed-off-by: Paul E. McKenney <[email protected]>
> Cc: Doug Ledford <[email protected]>
> Cc: Jason Gunthorpe <[email protected]>
> Cc: Richard Henderson <[email protected]>
> Cc: Ivan Kokshaysky <[email protected]>
> Cc: Matt Turner <[email protected]>
> Cc: Michael Cree <[email protected]>
> Cc: Andrea Parri <[email protected]>
> Cc: <[email protected]>
> Cc: <[email protected]>
> [ paulmck: Removed drivers/dma/ioat/dma.c per Jason Gunthorpe's feedback. ]
Let me know if you want this patch to flow through the rdma tree..
Acked-by: Jason Gunthorpe <[email protected]>
On Fri, Dec 01, 2017 at 11:51:16AM -0800, Paul E. McKenney wrote:
> Because READ_ONCE() now implies read_barrier_depends(), the
> read_barrier_depends() in next_desc() is now redundant. This commit
> therefore removes it and the related comments.
>
> Signed-off-by: Paul E. McKenney <[email protected]>
> Cc: "Michael S. Tsirkin" <[email protected]>
> Cc: Jason Wang <[email protected]>
> Cc: <[email protected]>
> Cc: <[email protected]>
> Cc: <[email protected]>
Apropos, READ_ONCE is now asymmetrical with WRITE_ONCE.
I can read a pointer with READ_ONCE and be sure the value
is sane, but only if I also remember to put in smp_wmb before
WRITE_ONCE. Otherwise the pointer is ok but no guarantees
about the data pointed to.
It would be better if the API reflected he assymetry somehow.
> ---
> drivers/vhost/vhost.c | 7 +------
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 33ac2b186b85..78b5940a415a 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -1877,12 +1877,7 @@ static unsigned next_desc(struct vhost_virtqueue *vq, struct vring_desc *desc)
> return -1U;
>
> /* Check they're not leading us off end of descriptors. */
> - next = vhost16_to_cpu(vq, desc->next);
> - /* Make sure compiler knows to grab that: we don't want it changing! */
> - /* We will use the result as an index in an array, so most
> - * architectures only need a compiler barrier here. */
> - read_barrier_depends();
> -
> + next = vhost16_to_cpu(vq, READ_ONCE(desc->next));
> return next;
> }
>
> --
> 2.5.2
On Tue, Dec 05, 2017 at 08:31:20PM +0200, Michael S. Tsirkin wrote:
> Apropos, READ_ONCE is now asymmetrical with WRITE_ONCE.
>
> I can read a pointer with READ_ONCE and be sure the value
> is sane, but only if I also remember to put in smp_wmb before
> WRITE_ONCE. Otherwise the pointer is ok but no guarantees
> about the data pointed to.
That was already the case on everything except Alpha. And the canonical
match do the data dependency is store_release, not wmb.
On Tue, Dec 05, 2017 at 07:39:46PM +0100, Peter Zijlstra wrote:
> On Tue, Dec 05, 2017 at 08:31:20PM +0200, Michael S. Tsirkin wrote:
>
> > Apropos, READ_ONCE is now asymmetrical with WRITE_ONCE.
> >
> > I can read a pointer with READ_ONCE and be sure the value
> > is sane, but only if I also remember to put in smp_wmb before
> > WRITE_ONCE. Otherwise the pointer is ok but no guarantees
> > about the data pointed to.
>
> That was already the case on everything except Alpha. And the canonical
> match do the data dependency is store_release, not wmb.
Oh, interesting
static __always_inline void __write_once_size(volatile void *p, void *res, int size)
{
switch (size) {
case 1: *(volatile __u8 *)p = *(__u8 *)res; break;
case 2: *(volatile __u16 *)p = *(__u16 *)res; break;
case 4: *(volatile __u32 *)p = *(__u32 *)res; break;
case 8: *(volatile __u64 *)p = *(__u64 *)res; break;
default:
barrier();
__builtin_memcpy((void *)p, (const void *)res, size);
barrier();
}
}
#define WRITE_ONCE(x, val) \
({ \
union { typeof(x) __val; char __c[1]; } __u = \
{ .__val = (__force typeof(x)) (val) }; \
__write_once_size(&(x), __u.__c, sizeof(x)); \
__u.__val; \
})
I don't see WRITE_ONCE inserting any barriers, release or
write.
So it seems that on an architecture where writes can be reordered,
if I do
*pointer = 0xa;
WRITE_ONCE(array[x], pointer);
array write might bypass the pointer write,
and readers will read a stale value.
--
MST
On Tue, Dec 05, 2017 at 08:57:46PM +0200, Michael S. Tsirkin wrote:
> I don't see WRITE_ONCE inserting any barriers, release or
> write.
Correct, never claimed there was.
Just saying that:
obj = READ_ONCE(*foo);
val = READ_ONCE(obj->val);
Never needs a barrier (except on Alpha and we want to make that go
away). Simply because a CPU needs to complete the load of @obj before it
can compute the address &obj->val. Thus the second load _must_ come
after the first load and we get LOAD-LOAD ordering.
Alpha messing that up is a royal pain, and Alpha not being an
active/living architecture is just not worth the pain of keeping this in
the generic model.
On Tue, Dec 05, 2017 at 08:17:33PM +0100, Peter Zijlstra wrote:
> On Tue, Dec 05, 2017 at 08:57:46PM +0200, Michael S. Tsirkin wrote:
>
> > I don't see WRITE_ONCE inserting any barriers, release or
> > write.
>
> Correct, never claimed there was.
>
> Just saying that:
>
> obj = READ_ONCE(*foo);
> val = READ_ONCE(obj->val);
>
> Never needs a barrier (except on Alpha and we want to make that go
> away). Simply because a CPU needs to complete the load of @obj before it
> can compute the address &obj->val. Thus the second load _must_ come
> after the first load and we get LOAD-LOAD ordering.
>
> Alpha messing that up is a royal pain, and Alpha not being an
> active/living architecture is just not worth the pain of keeping this in
> the generic model.
>
Right. What I am saying is that for writes you need
WRITE_ONCE(obj->val, 1);
smp_wmb();
WRITE_ONCE(*foo, obj);
and this barrier is no longer paired with anything until
you realize there's a dependency barrier within READ_ONCE.
Barrier pairing was a useful tool to check code validity,
maybe there are other, better tools now.
--
MST
On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin wrote:
> On Tue, Dec 05, 2017 at 08:17:33PM +0100, Peter Zijlstra wrote:
> > On Tue, Dec 05, 2017 at 08:57:46PM +0200, Michael S. Tsirkin wrote:
> >
> > > I don't see WRITE_ONCE inserting any barriers, release or
> > > write.
> >
> > Correct, never claimed there was.
> >
> > Just saying that:
> >
> > obj = READ_ONCE(*foo);
> > val = READ_ONCE(obj->val);
> >
> > Never needs a barrier (except on Alpha and we want to make that go
> > away). Simply because a CPU needs to complete the load of @obj before it
> > can compute the address &obj->val. Thus the second load _must_ come
> > after the first load and we get LOAD-LOAD ordering.
> >
> > Alpha messing that up is a royal pain, and Alpha not being an
> > active/living architecture is just not worth the pain of keeping this in
> > the generic model.
> >
>
> Right. What I am saying is that for writes you need
>
> WRITE_ONCE(obj->val, 1);
> smp_wmb();
> WRITE_ONCE(*foo, obj);
I believe Peter was instead suggesting:
WRITE_ONCE(obj->val, 1);
smp_store_release(foo, obj);
> and this barrier is no longer paired with anything until
> you realize there's a dependency barrier within READ_ONCE.
>
> Barrier pairing was a useful tool to check code validity,
> maybe there are other, better tools now.
There are quite a few people who say that smp_store_release() is
easier for the tools to analyze than is smp_wmb(). My experience with
smp_read_barrier_depends() and rcu_dereference() leads me to believe
that they are correct.
Thanx, Paul
On Tue, Dec 05, 2017 at 11:33:39AM -0800, Paul E. McKenney wrote:
> On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Dec 05, 2017 at 08:17:33PM +0100, Peter Zijlstra wrote:
> > > On Tue, Dec 05, 2017 at 08:57:46PM +0200, Michael S. Tsirkin wrote:
> > >
> > > > I don't see WRITE_ONCE inserting any barriers, release or
> > > > write.
> > >
> > > Correct, never claimed there was.
> > >
> > > Just saying that:
> > >
> > > obj = READ_ONCE(*foo);
> > > val = READ_ONCE(obj->val);
> > >
> > > Never needs a barrier (except on Alpha and we want to make that go
> > > away). Simply because a CPU needs to complete the load of @obj before it
> > > can compute the address &obj->val. Thus the second load _must_ come
> > > after the first load and we get LOAD-LOAD ordering.
> > >
> > > Alpha messing that up is a royal pain, and Alpha not being an
> > > active/living architecture is just not worth the pain of keeping this in
> > > the generic model.
> > >
> >
> > Right. What I am saying is that for writes you need
> >
> > WRITE_ONCE(obj->val, 1);
> > smp_wmb();
> > WRITE_ONCE(*foo, obj);
>
> I believe Peter was instead suggesting:
>
> WRITE_ONCE(obj->val, 1);
> smp_store_release(foo, obj);
Isn't that more expensive though?
> > and this barrier is no longer paired with anything until
> > you realize there's a dependency barrier within READ_ONCE.
> >
> > Barrier pairing was a useful tool to check code validity,
> > maybe there are other, better tools now.
>
> There are quite a few people who say that smp_store_release() is
> easier for the tools to analyze than is smp_wmb(). My experience with
> smp_read_barrier_depends() and rcu_dereference() leads me to believe
> that they are correct.
>
> Thanx, Paul
OK, but smp_store_release is still not paired with anything since we
rely on READ_ONCE to include the implicit dpendendency barrier.
--
MST
On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin wrote:
> On Tue, Dec 05, 2017 at 08:17:33PM +0100, Peter Zijlstra wrote:
> > On Tue, Dec 05, 2017 at 08:57:46PM +0200, Michael S. Tsirkin wrote:
> >
> > > I don't see WRITE_ONCE inserting any barriers, release or
> > > write.
> >
> > Correct, never claimed there was.
> >
> > Just saying that:
> >
> > obj = READ_ONCE(*foo);
> > val = READ_ONCE(obj->val);
> >
> > Never needs a barrier (except on Alpha and we want to make that go
> > away). Simply because a CPU needs to complete the load of @obj before it
> > can compute the address &obj->val. Thus the second load _must_ come
> > after the first load and we get LOAD-LOAD ordering.
> >
> > Alpha messing that up is a royal pain, and Alpha not being an
> > active/living architecture is just not worth the pain of keeping this in
> > the generic model.
> >
>
> Right. What I am saying is that for writes you need
>
> WRITE_ONCE(obj->val, 1);
> smp_wmb();
> WRITE_ONCE(*foo, obj);
You really should use smp_store_release() here instead of the smp_wmb().
But yes.
> and this barrier is no longer paired with anything until
> you realize there's a dependency barrier within READ_ONCE.
No, there isn't. read_dependecy barriers are no more. They don't exist.
They never did, except on Alpha anyway.
There were a ton of sites that relied on this but never had the
smp_read_barrier_depends() annotation, similarly there were quite a few
sites that had the barrier but nobody could explain wtf for.
What these patches do is return the sane rule that dependent loads are
ordered.
And like all memory ordering; it should come with comments. Any piece of
code that relies on memory ordering and doesn't have big fat comments
that explain the required ordering are broken per definition. Maybe not
now, but they will be after a few years because someone changed it and
didn't know.
> Barrier pairing was a useful tool to check code validity,
> maybe there are other, better tools now.
Same is true for the closely related control dependency, you can pair
against those just fine but they don't have an explicit barrier
annotation.
There are no tools, use brain.
On Tue, Dec 05, 2017 at 09:51:48PM +0200, Michael S. Tsirkin wrote:
> > > WRITE_ONCE(obj->val, 1);
> > > smp_wmb();
> > > WRITE_ONCE(*foo, obj);
> >
> > I believe Peter was instead suggesting:
> >
> > WRITE_ONCE(obj->val, 1);
> > smp_store_release(foo, obj);
>
> Isn't that more expensive though?
Depends on the architecture. The only architecture where it is more
expensive and people actually still care about is ARM I think.
On Tue, Dec 05, 2017 at 08:08:32AM -0700, Jason Gunthorpe wrote:
> > commit c389c98ec5f4a7aa4c36853e89801eb5ea81870e
> > Author: Paul E. McKenney <[email protected]>
> > Date: Mon Nov 27 09:04:22 2017 -0800
> >
> > drivers/infiniband: Remove now-redundant smp_read_barrier_depends()
> >
> > The smp_read_barrier_depends() does nothing at all except on DEC Alpha,
> > and no current DEC Alpha systems use Infiniband:
> >
> > lkml.kernel.org/r/20171023085921.jwbntptn6ictbnvj@tower
> >
> > This commit therefore makes Infiniband depend on !ALPHA and removes
> > the now-ineffective invocations of smp_read_barrier_depends() from
> > the InfiniBand driver.
> >
> > Please note that this patch should not be construed as my saying that
> > InfiniBand's memory ordering is correct, but rather that this patch does
> > not in any way affect InfiniBand's correctness. In other words, the
> > result of applying this patch is bug-for-bug compatible with the original.
> >
> > Signed-off-by: Paul E. McKenney <[email protected]>
> > Cc: Doug Ledford <[email protected]>
> > Cc: Jason Gunthorpe <[email protected]>
> > Cc: Richard Henderson <[email protected]>
> > Cc: Ivan Kokshaysky <[email protected]>
> > Cc: Matt Turner <[email protected]>
> > Cc: Michael Cree <[email protected]>
> > Cc: Andrea Parri <[email protected]>
> > Cc: <[email protected]>
> > Cc: <[email protected]>
> > [ paulmck: Removed drivers/dma/ioat/dma.c per Jason Gunthorpe's feedback. ]
>
> Let me know if you want this patch to flow through the rdma tree..
>
> Acked-by: Jason Gunthorpe <[email protected]>
I applied your ack, thank you!
Your choice as to what tree it flows through. By default, and if all
goes well, I will push this series through -rcu and -tip during the next
merge window.
Thanx, Paul
On Tue, Dec 05, 2017 at 09:51:48PM +0200, Michael S. Tsirkin wrote:
> On Tue, Dec 05, 2017 at 11:33:39AM -0800, Paul E. McKenney wrote:
> > On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin wrote:
[ . . . ]
> > > and this barrier is no longer paired with anything until
> > > you realize there's a dependency barrier within READ_ONCE.
> > >
> > > Barrier pairing was a useful tool to check code validity,
> > > maybe there are other, better tools now.
> >
> > There are quite a few people who say that smp_store_release() is
> > easier for the tools to analyze than is smp_wmb(). My experience with
> > smp_read_barrier_depends() and rcu_dereference() leads me to believe
> > that they are correct.
>
> OK, but smp_store_release is still not paired with anything since we
> rely on READ_ONCE to include the implicit dpendendency barrier.
Why wouldn't you consider the smp_store_release() to be paired with
the new improved READ_ONCE()?
Thanx, Paul
On Tue, Dec 05, 2017 at 08:57:52PM +0100, Peter Zijlstra wrote:
> On Tue, Dec 05, 2017 at 09:51:48PM +0200, Michael S. Tsirkin wrote:
> > > > WRITE_ONCE(obj->val, 1);
> > > > smp_wmb();
> > > > WRITE_ONCE(*foo, obj);
> > >
> > > I believe Peter was instead suggesting:
> > >
> > > WRITE_ONCE(obj->val, 1);
> > > smp_store_release(foo, obj);
> >
> > Isn't that more expensive though?
>
> Depends on the architecture. The only architecture where it is more
> expensive and people actually still care about is ARM I think.
Right. Why should I use the more expensive smp_store_release then?
--
MST
On Tue, Dec 05, 2017 at 10:28:38PM +0200, Michael S. Tsirkin wrote:
> On Tue, Dec 05, 2017 at 08:57:52PM +0100, Peter Zijlstra wrote:
> > On Tue, Dec 05, 2017 at 09:51:48PM +0200, Michael S. Tsirkin wrote:
> > > > > WRITE_ONCE(obj->val, 1);
> > > > > smp_wmb();
> > > > > WRITE_ONCE(*foo, obj);
> > > >
> > > > I believe Peter was instead suggesting:
> > > >
> > > > WRITE_ONCE(obj->val, 1);
> > > > smp_store_release(foo, obj);
> > >
> > > Isn't that more expensive though?
> >
> > Depends on the architecture. The only architecture where it is more
> > expensive and people actually still care about is ARM I think.
>
> Right. Why should I use the more expensive smp_store_release then?
Because it makes more sense. Memory ordering is hard enough, don't make
it harder still if you don't have to.
On Tue, Dec 05, 2017 at 12:08:01PM -0800, Paul E. McKenney wrote:
> On Tue, Dec 05, 2017 at 09:51:48PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Dec 05, 2017 at 11:33:39AM -0800, Paul E. McKenney wrote:
> > > On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin wrote:
>
> [ . . . ]
>
> > > > and this barrier is no longer paired with anything until
> > > > you realize there's a dependency barrier within READ_ONCE.
> > > >
> > > > Barrier pairing was a useful tool to check code validity,
> > > > maybe there are other, better tools now.
> > >
> > > There are quite a few people who say that smp_store_release() is
> > > easier for the tools to analyze than is smp_wmb(). My experience with
> > > smp_read_barrier_depends() and rcu_dereference() leads me to believe
> > > that they are correct.
> >
> > OK, but smp_store_release is still not paired with anything since we
> > rely on READ_ONCE to include the implicit dpendendency barrier.
>
> Why wouldn't you consider the smp_store_release() to be paired with
> the new improved READ_ONCE()?
>
> Thanx, Paul
READ_ONCE is really all over the place (some code literally replaced all
memory accesses with READ/WRITE ONCE).
And I also prefer smp_wmb as it seems to be cheaper on ARM.
Would an API like WRITE_POINTER()/smp_store_pointer make sense,
and READ_POINTER for symmetry?
--
MST
On Tue, Dec 05, 2017 at 11:24:49PM +0200, Michael S. Tsirkin wrote:
> On Tue, Dec 05, 2017 at 12:08:01PM -0800, Paul E. McKenney wrote:
> > On Tue, Dec 05, 2017 at 09:51:48PM +0200, Michael S. Tsirkin wrote:
> > > On Tue, Dec 05, 2017 at 11:33:39AM -0800, Paul E. McKenney wrote:
> > > > On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin wrote:
> >
> > [ . . . ]
> >
> > > > > and this barrier is no longer paired with anything until
> > > > > you realize there's a dependency barrier within READ_ONCE.
> > > > >
> > > > > Barrier pairing was a useful tool to check code validity,
> > > > > maybe there are other, better tools now.
> > > >
> > > > There are quite a few people who say that smp_store_release() is
> > > > easier for the tools to analyze than is smp_wmb(). My experience with
> > > > smp_read_barrier_depends() and rcu_dereference() leads me to believe
> > > > that they are correct.
> > >
> > > OK, but smp_store_release is still not paired with anything since we
> > > rely on READ_ONCE to include the implicit dpendendency barrier.
> >
> > Why wouldn't you consider the smp_store_release() to be paired with
> > the new improved READ_ONCE()?
>
> READ_ONCE is really all over the place (some code literally replaced all
> memory accesses with READ/WRITE ONCE).
>
> And I also prefer smp_wmb as it seems to be cheaper on ARM.
>
> Would an API like WRITE_POINTER()/smp_store_pointer make sense,
> and READ_POINTER for symmetry?
What we do in some code is to comment the pairings, allowing the other
side of the pairing to be easily located. Would that work for you?
Thanx, Paul
On Tue, Dec 05, 2017 at 10:17:35PM +0100, Peter Zijlstra wrote:
> On Tue, Dec 05, 2017 at 10:28:38PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Dec 05, 2017 at 08:57:52PM +0100, Peter Zijlstra wrote:
> > > On Tue, Dec 05, 2017 at 09:51:48PM +0200, Michael S. Tsirkin wrote:
> > > > > > WRITE_ONCE(obj->val, 1);
> > > > > > smp_wmb();
> > > > > > WRITE_ONCE(*foo, obj);
> > > > >
> > > > > I believe Peter was instead suggesting:
> > > > >
> > > > > WRITE_ONCE(obj->val, 1);
> > > > > smp_store_release(foo, obj);
> > > >
> > > > Isn't that more expensive though?
> > >
> > > Depends on the architecture. The only architecture where it is more
> > > expensive and people actually still care about is ARM I think.
> >
> > Right. Why should I use the more expensive smp_store_release then?
>
> Because it makes more sense. Memory ordering is hard enough, don't make
> it harder still if you don't have to.
I suspect I have to - ptr_ring is a very low level construct used by
netowrking on data path so making it a bit more complicated for
a bit of performance is probably justified.
--
MST
On Tue, Dec 05, 2017 at 01:36:44PM -0800, Paul E. McKenney wrote:
> On Tue, Dec 05, 2017 at 11:24:49PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Dec 05, 2017 at 12:08:01PM -0800, Paul E. McKenney wrote:
> > > On Tue, Dec 05, 2017 at 09:51:48PM +0200, Michael S. Tsirkin wrote:
> > > > On Tue, Dec 05, 2017 at 11:33:39AM -0800, Paul E. McKenney wrote:
> > > > > On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin wrote:
> > >
> > > [ . . . ]
> > >
> > > > > > and this barrier is no longer paired with anything until
> > > > > > you realize there's a dependency barrier within READ_ONCE.
> > > > > >
> > > > > > Barrier pairing was a useful tool to check code validity,
> > > > > > maybe there are other, better tools now.
> > > > >
> > > > > There are quite a few people who say that smp_store_release() is
> > > > > easier for the tools to analyze than is smp_wmb(). My experience with
> > > > > smp_read_barrier_depends() and rcu_dereference() leads me to believe
> > > > > that they are correct.
> > > >
> > > > OK, but smp_store_release is still not paired with anything since we
> > > > rely on READ_ONCE to include the implicit dpendendency barrier.
> > >
> > > Why wouldn't you consider the smp_store_release() to be paired with
> > > the new improved READ_ONCE()?
> >
> > READ_ONCE is really all over the place (some code literally replaced all
> > memory accesses with READ/WRITE ONCE).
> >
> > And I also prefer smp_wmb as it seems to be cheaper on ARM.
> >
> > Would an API like WRITE_POINTER()/smp_store_pointer make sense,
> > and READ_POINTER for symmetry?
>
> What we do in some code is to comment the pairings, allowing the other
> side of the pairing to be easily located. Would that work for you?
>
> Thanx, Paul
Yes, that's exactly what I did for now.
Thanks!
--
MST
On Tue, Dec 05, 2017 at 11:24:49PM +0200, Michael S. Tsirkin wrote:
> READ_ONCE is really all over the place (some code literally replaced all
> memory accesses with READ/WRITE ONCE).
Yeah, so? Complain to the compiler people for forcing us into that.
> Would an API like WRITE_POINTER()/smp_store_pointer make sense,
> and READ_POINTER for symmetry?
No, the whole point of the exercise was to get away from the fact that
dependent loads are special.
On Tue, Dec 05, 2017 at 11:43:41PM +0200, Michael S. Tsirkin wrote:
> On Tue, Dec 05, 2017 at 01:36:44PM -0800, Paul E. McKenney wrote:
> > On Tue, Dec 05, 2017 at 11:24:49PM +0200, Michael S. Tsirkin wrote:
> > > On Tue, Dec 05, 2017 at 12:08:01PM -0800, Paul E. McKenney wrote:
> > > > On Tue, Dec 05, 2017 at 09:51:48PM +0200, Michael S. Tsirkin wrote:
> > > > > On Tue, Dec 05, 2017 at 11:33:39AM -0800, Paul E. McKenney wrote:
> > > > > > On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin wrote:
> > > >
> > > > [ . . . ]
> > > >
> > > > > > > and this barrier is no longer paired with anything until
> > > > > > > you realize there's a dependency barrier within READ_ONCE.
> > > > > > >
> > > > > > > Barrier pairing was a useful tool to check code validity,
> > > > > > > maybe there are other, better tools now.
> > > > > >
> > > > > > There are quite a few people who say that smp_store_release() is
> > > > > > easier for the tools to analyze than is smp_wmb(). My experience with
> > > > > > smp_read_barrier_depends() and rcu_dereference() leads me to believe
> > > > > > that they are correct.
> > > > >
> > > > > OK, but smp_store_release is still not paired with anything since we
> > > > > rely on READ_ONCE to include the implicit dpendendency barrier.
> > > >
> > > > Why wouldn't you consider the smp_store_release() to be paired with
> > > > the new improved READ_ONCE()?
> > >
> > > READ_ONCE is really all over the place (some code literally replaced all
> > > memory accesses with READ/WRITE ONCE).
> > >
> > > And I also prefer smp_wmb as it seems to be cheaper on ARM.
> > >
> > > Would an API like WRITE_POINTER()/smp_store_pointer make sense,
> > > and READ_POINTER for symmetry?
> >
> > What we do in some code is to comment the pairings, allowing the other
> > side of the pairing to be easily located. Would that work for you?
>
> Yes, that's exactly what I did for now.
Very good, thank you!
Thanx, Paul
On Tue, Dec 05, 2017 at 10:57:00PM +0100, Peter Zijlstra wrote:
> On Tue, Dec 05, 2017 at 11:24:49PM +0200, Michael S. Tsirkin wrote:
> > READ_ONCE is really all over the place (some code literally replaced all
> > memory accesses with READ/WRITE ONCE).
>
> Yeah, so?
Oh my point was I can't just look for READ_ONCE and go
*that's the pair*. there are too many of these.
At Paul's suggestion I will document the pairing *this read once has a
barrier that is paired with that barrier*.
> Complain to the compiler people for forcing us into that.
In some cases when you end up with all accesses
going through read/write once volatile just might better.
> > Would an API like WRITE_POINTER()/smp_store_pointer make sense,
> > and READ_POINTER for symmetry?
>
> No, the whole point of the exercise was to get away from the fact that
> dependent loads are special.
It's a pity that dependent stores are still special.
--
MST
On Tue, Dec 05, 2017 at 01:36:44PM -0800, Paul E. McKenney wrote:
> What we do in some code is to comment the pairings, allowing the other
> side of the pairing to be easily located. Would that work for you?
I would say that that is mandatory for any memory ordering code ;-)
On Wed, Dec 06, 2017 at 12:09:36AM +0200, Michael S. Tsirkin wrote:
> On Tue, Dec 05, 2017 at 10:57:00PM +0100, Peter Zijlstra wrote:
> > On Tue, Dec 05, 2017 at 11:24:49PM +0200, Michael S. Tsirkin wrote:
> > > READ_ONCE is really all over the place (some code literally replaced all
> > > memory accesses with READ/WRITE ONCE).
> >
> > Yeah, so?
>
> Oh my point was I can't just look for READ_ONCE and go
> *that's the pair*. there are too many of these.
> At Paul's suggestion I will document the pairing *this read once has a
> barrier that is paired with that barrier*.
>
> > Complain to the compiler people for forcing us into that.
>
> In some cases when you end up with all accesses
> going through read/write once volatile just might better.
That is in fact what the jiffies counter does. But you lose READ_ONCE()'s
automatic handling of DEC Alpha when you take that approach.
> > > Would an API like WRITE_POINTER()/smp_store_pointer make sense,
> > > and READ_POINTER for symmetry?
> >
> > No, the whole point of the exercise was to get away from the fact that
> > dependent loads are special.
>
> It's a pity that dependent stores are still special.
We can make READ_ONCE() not be special at zero cost on non-Alpha
systems, but both smp_wmb() and smp_store_release() are decidedly
not free of added overhead.
Thanx, Paul