De-emphasize {smp_,}read_barrier_depends
Subsystems (27)
cover De-emphasize {smp_,}read_barrier_depends
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(-)
PATCH 1/21 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().
--- Documentation/memory-barriers.txt | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
Documentation/memory-barriers.txt +6 -5
@@ -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:
Signature · 1 line
2.5.2
David Howells
maintainerRe: [PATCH tip/core/rcu 01/21] doc: READ_ONCE() now implies smp_barrier_depends()
Documentation/memory-barriers.txt There are some minimal guarantees that may be expected of a CPU: · line 234
Quoted 6 lines · Paul E. McKenney
Documentation/memory-barriers.txt +1
-1- 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
Re: [PATCH tip/core/rcu 01/21] doc: READ_ONCE() now implies smp_barrier_depends()
Quoted 5 lines · David Howells
Quoted 3 lines · Paul E. McKenney
(quoted diff) +1
-1- Q = READ_ONCE(P); smp_read_barrier_depends(); D = READ_ONCE(*Q); + Q = READ_ONCE(P); D = READ_ONCE(*Q);
Quoted 8 lines
Quoted 3 lines
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().
[ paulmck: Added MEMORY_BARRIER instructions from DEC Alpha from
READ_ONCE(), per David Howells's feedback. ]
Documentation/memory-barriers.txt +9 -6
@@ -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:
Peter Zijlstra
maintainerRe: [PATCH tip/core/rcu 01/21] doc: READ_ONCE() now implies smp_barrier_depends()
Quoted 8 lines · Paul E. McKenney
Quoted 7 lines · David Howells
Quoted 5 lines · Paul E. McKenney
Documentation/memory-barriers.txt +1
-1- 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:
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
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.
Re: [PATCH tip/core/rcu 01/21] doc: READ_ONCE() now implies smp_barrier_depends()
Quoted 14 lines · Peter Zijlstra
Quoted 13 lines · Paul E. McKenney
Quoted 12 lines · David Howells
Documentation/memory-barriers.txt There are some minimal guarantees that may be expected of a CPU: · line 234
Quoted 6 lines · Paul E. McKenney
Documentation/memory-barriers.txt +1
-1- 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 *QThe CPU may now issue two barriers in addition to the loads, so should we show this? E.g.:
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
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_dependsfrom 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
David Howells
maintainerRe: [PATCH tip/core/rcu 01/21] doc: READ_ONCE() now implies smp_barrier_depends()
In reply to Peter ZijlstraQuoted 5 lines · Peter Zijlstra
Quoted 1 line
Good point! How about as shown in the updated patch below?
Humm, I thought the idea was to completely remove
read_barrier_dependsfrom 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
Peter Zijlstra
maintainerRe: [PATCH tip/core/rcu 01/21] doc: READ_ONCE() now implies smp_barrier_depends()
Quoted 10 lines · David Howells
Quoted 5 lines · Peter Zijlstra
Quoted 1 line
Good point! How about as shown in the updated patch below?
Humm, I thought the idea was to completely remove
read_barrier_dependsfrom 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'.
Quoted 1 line
Don't forget, btw, that Blackfin uses it also.
We should delete that blackfin code, its beyond insane.
PATCH 2/21 mn10300: READ_ONCE() now implies smp_read_barrier_depends()
In reply to Paul E. McKenneyGiven 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.
--- arch/mn10300/kernel/mn10300-serial.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
arch/mn10300/kernel/mn10300-serial.c +5 -2
@@ -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 ();
Signature · 1 line
2.5.2
PATCH 3/21 drivers/net/ethernet/qlogic/qed: Fix __qed_spq_block() ordering
In reply to Paul E. McKenneyThe __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().
--- drivers/net/ethernet/qlogic/qed/qed_spq.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/net/ethernet/qlogic/qed/qed_spq.c +1 -3
@@ -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 ;
Signature · 1 line
2.5.2
PATCH 4/21 fs/dcache: Use release-acquire for name/length update
In reply to Paul E. McKenneyThe 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().
--- fs/dcache.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
fs/dcache.c +3 -7
@@ -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 ;
Signature · 1 line
2.5.2
PATCH 5/21 percpu: READ_ONCE() now implies smp_read_barrier_depends()
In reply to Paul E. McKenneyBecause 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().
--- include/linux/percpu-refcount.h | 6 +++--- lib/percpu-refcount.c | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-)
include/linux/percpu-refcount.h +3 -3
@@ -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
lib/percpu-refcount.c +4 -4
@@ -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 ;
Signature · 1 line
2.5.2
PATCH 6/21 rcu: Adjust read-side accessor comments for READ_ONCE()
In reply to Paul E. McKenneyNow that READ_ONCE() implies smp_read_barrier_depends(), the commit updates now-misleading comments to account for this change.
--- include/linux/rcupdate.h | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-)
include/linux/rcupdate.h +11 -12
@@ -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
Signature · 1 line
2.5.2
PATCH 7/21 rtnetlink: Update now-misleading smp_read_barrier_depends() comment
In reply to Paul E. McKenneyNow that READ_ONCE() implies smp_read_barrier_depends(), update the rtnl_dereference() header comment accordingly.
--- include/linux/rtnetlink.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
include/linux/rtnetlink.h +1 -2
@@ -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())
Signature · 1 line
2.5.2
PATCH 8/21 seqlock: Remove now-redundant smp_read_barrier_depends()
In reply to Paul E. McKenneyREAD_ONCE() now implies smp_read_barrier_depends(), so this patch removes the now-redundant smp_read_barrier_depends() from raw_read_seqcount_latch().
--- include/linux/seqlock.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
include/linux/seqlock.h +1 -2
@@ -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; }
Signature · 1 line
2.5.2
PATCH 9/21 uprobes: Remove now-redundant smp_read_barrier_depends()
In reply to Paul E. McKenneyNow 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().
--- kernel/events/uprobes.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
kernel/events/uprobes.c +6 -6
@@ -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;
Signature · 1 line
2.5.2
PATCH 10/21 locking: Remove smp_read_barrier_depends() from queued_spin_lock_slowpath()
In reply to Paul E. McKenneyQueued 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.
--- kernel/locking/qspinlock.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
kernel/locking/qspinlock.c +5 -7
@@ -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);
Signature · 1 line
2.5.2
PATCH 11/21 tracepoint: Remove smp_read_barrier_depends() from comment
In reply to Paul E. McKenneyThe 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.
--- kernel/tracepoint.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
kernel/tracepoint.c +4 -5
@@ -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))
Signature · 1 line
2.5.2
PATCH 12/21 lib/assoc_array: Remove smp_read_barrier_depends()
In reply to Paul E. McKenneyNow 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.
--- lib/assoc_array.c | 37 ++++++++++++------------------------- 1 file changed, 12 insertions(+), 25 deletions(-)
lib/assoc_array.c +12 -25
@@ -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; }
Signature · 1 line
2.5.2
PATCH 13/21 mm/ksm: Remove now-redundant smp_read_barrier_depends()
In reply to Paul E. McKenneyBecause 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.
--- mm/ksm.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)
mm/ksm.c +1 -8
@@ -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;
Signature · 7 lines
2.5.2 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
PATCH 14/21 netfilter: Remove now-redundant smp_read_barrier_depends()
In reply to Paul E. McKenneyREAD_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.
--- 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(-)
net/ipv4/netfilter/arp_tables.c +1 -6
@@ -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];
net/ipv4/netfilter/ip_tables.c +1 -6
@@ -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];
net/ipv6/netfilter/ip6_tables.c +1 -6
@@ -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];
Signature · 1 line
2.5.2
PATCH 15/21 keyring: Remove now-redundant smp_read_barrier_depends()
In reply to Paul E. McKenneyNow 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.
--- security/keys/keyring.c | 7 ------- 1 file changed, 7 deletions(-)
security/keys/keyring.c +0 -7
@@ -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
Signature · 1 line
2.5.2
Re: [PATCH tip/core/rcu 15/21] keyring: Remove now-redundant smp_read_barrier_depends()
Quoted 10 lines · Paul E. McKenney
Now that the associative-array library properly heads dependency chains, the various
smp_read_barrier_depends()calls insecurity/keys/keyring.care 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]>
Signature · 2 lines
James Morris <[email protected]>
Paul E. McKenney
AppliedRe: [PATCH tip/core/rcu 15/21] keyring: Remove now-redundant smp_read_barrier_depends()
Quoted 6 lines · James Morris
Quoted 4 lines · Paul E. McKenney
Now that the associative-array library properly heads dependency chains, the various
smp_read_barrier_depends()calls insecurity/keys/keyring.care no longer needed. This commit therefore removes them.
Quoted 8 lines
Quoted 6 lines
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
PATCH 16/21 drivers/infiniband: Remove now-redundant smp_read_barrier_depends()
In reply to Paul E. McKenneyThe 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.
--- 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(-)
drivers/dma/ioat/dma.c +0 -2
@@ -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);
drivers/infiniband/Kconfig +1 -0
@@ -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
drivers/infiniband/hw/hfi1/rc.c +0 -4
@@ -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;
drivers/infiniband/hw/hfi1/ruc.c +0 -1
@@ -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);
drivers/infiniband/hw/hfi1/sdma.c +0 -1
@@ -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]; }
drivers/infiniband/hw/hfi1/uc.c +0 -2
@@ -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;
drivers/infiniband/hw/hfi1/ud.c +0 -2
@@ -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;
drivers/infiniband/hw/qib/qib_rc.c +0 -3
@@ -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;
drivers/infiniband/hw/qib/qib_ruc.c +0 -1
@@ -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);
drivers/infiniband/hw/qib/qib_uc.c +0 -2
@@ -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;/*
drivers/infiniband/hw/qib/qib_ud.c +0 -2
@@ -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;
drivers/infiniband/sw/rdmavt/qp.c +0 -1
@@ -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);
Signature · 1 line
2.5.2
Re: [PATCH tip/core/rcu 16/21] drivers/infiniband: Remove now-redundant smp_read_barrier_depends()
Quoted 4 lines · Paul E. McKenney
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.
Quoted 2 lines
drivers/dma/ioat/dma.c | 2 -- drivers/infiniband/Kconfig | 1 +
Did you mean to have ioat/dma.c in this patch?
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH tip/core/rcu 16/21] drivers/infiniband: Remove now-redundant smp_read_barrier_depends()
Quoted 4 lines · Jason Gunthorpe
Quoted 3 lines · Paul E. McKenney
The
smp_read_barrier_depends()does nothing at all except on DEC Alpha, and no current DEC Alpha systems use Infiniband:
Quoted 8 lines
Quoted 1 line
lkml.kernel.org/r/20171023085921.jwbntptn6ictbnvj@towerI 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.
Quoted 4 lines
Quoted 2 lines
drivers/dma/ioat/dma.c | 2 -- drivers/infiniband/Kconfig | 1 +Did you mean to have
ioat/dma.cin 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.
drivers/infiniband/Kconfig +1 -0
@@ -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
drivers/infiniband/hw/hfi1/rc.c +0 -4
@@ -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;
drivers/infiniband/hw/hfi1/ruc.c +0 -1
@@ -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);
drivers/infiniband/hw/hfi1/sdma.c +0 -1
@@ -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]; }
drivers/infiniband/hw/hfi1/uc.c +0 -2
@@ -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;
drivers/infiniband/hw/hfi1/ud.c +0 -2
@@ -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;
drivers/infiniband/hw/qib/qib_rc.c +0 -3
@@ -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;
drivers/infiniband/hw/qib/qib_ruc.c +0 -1
@@ -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);
drivers/infiniband/hw/qib/qib_uc.c +0 -2
@@ -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;/*
drivers/infiniband/hw/qib/qib_ud.c +0 -2
@@ -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;
drivers/infiniband/sw/rdmavt/qp.c +0 -1
@@ -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);
Jason Gunthorpe
Acked-byRe: [PATCH tip/core/rcu 16/21] drivers/infiniband: Remove now-redundant smp_read_barrier_depends()
Quoted 20 lines
commit
c389c98ec5f4a7aa4c36853e89801eb5ea81870eAuthor: Paul E. McKenney <[email protected]> Date: Mon Nov 27 09:04:22 2017 -0800
drivers/infiniband: Remove now-redundantsmp_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.
Quoted 11 lines
Signed-off-by Paul E. McKenney <[email protected]>Cc Doug Ledford <[email protected]>Cc Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>Cc Richard Henderson <[email protected]>Cc Ivan Kokshaysky <ink-biIs/[email protected]>Cc Matt Turner <[email protected]>Cc Michael Cree <mcree-q3Ck4f9/[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..
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Paul E. McKenney
AppliedRe: [PATCH tip/core/rcu 16/21] drivers/infiniband: Remove now-redundant smp_read_barrier_depends()
Quoted 32 lines · Jason Gunthorpe
Quoted 20 lines
commit
c389c98ec5f4a7aa4c36853e89801eb5ea81870eAuthor: Paul E. McKenney <[email protected]> Date: Mon Nov 27 09:04:22 2017 -0800
drivers/infiniband: Remove now-redundantsmp_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.
Quoted 11 lines
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. ]
Quoted 3 lines
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
PATCH 17/21 doc: De-emphasize smp_read_barrier_depends
In reply to Paul E. McKenneyThis commit keeps only the historical and low-level discussion of smp_read_barrier_depends().
--- 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(-)
Documentation/RCU/Design/Requirements/Requirements.html +2 -1
@@ -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.
Documentation/RCU/rcu_dereference.txt +1 -5
@@ -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
Documentation/RCU/whatisRCU.txt +1 -2
@@ -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); \ })
Documentation/circular-buffers.txt +1 -2
@@ -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
Documentation/memory-barriers.txt +4 -3
@@ -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
Signature · 1 line
2.5.2
PATCH 18/21 genetlink: Remove smp_read_barrier_depends() from comment
In reply to Paul E. McKenneyNow that smp_read_barrier_depends() has been de-emphasized, the less said about it, the better.
--- include/linux/genetlink.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
include/linux/genetlink.h +1 -2
@@ -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())
Signature · 1 line
2.5.2
PATCH 19/21 netlink: Remove smp_read_barrier_depends() from comment
In reply to Paul E. McKenneyNow that smp_read_barrier_depends() has been de-emphasized, the less said about it, the better.
--- include/linux/netfilter/nfnetlink.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
include/linux/netfilter/nfnetlink.h +1 -2
@@ -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))
Signature · 1 line
2.5.2
PATCH 20/21 checkpatch: Add warnings for {smp_,}read_barrier_depends()
In reply to Paul E. McKenneyNow that both smp_read_barrier_depends() and read_barrier_depends() are being de-emphasized, warn if any are added.
--- scripts/checkpatch.pl | 6 ++++++ 1 file changed, 6 insertions(+)
scripts/checkpatch.pl +6 -0
@@ -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",
Signature · 1 line
2.5.2
Joe Perches
maintainerRe: [PATCH tip/core/rcu 20/21] checkpatch: Add warnings for {smp_,}read_barrier_depends()
Quoted 2 lines · Paul E. McKenney
Now that both
smp_read_barrier_depends()andread_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...)
scripts/checkpatch.pl sub process { · line 5590
Quoted 10 lines
scripts/checkpatch.pl +2
-0@@ -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.
Re: [PATCH tip/core/rcu 20/21] checkpatch: Add warnings for {smp_,}read_barrier_depends()
Quoted 26 lines · Joe Perches
Quoted 2 lines · Paul E. McKenney
Now that both
smp_read_barrier_depends()andread_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...)
scripts/checkpatch.pl sub process { · line 5590
Quoted 10 lines
scripts/checkpatch.pl +2
-0@@ -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
(quoted diff) +1
-0+ if ($line =~ /\b(smp_|)read_barrier_depends\s*\(/) {
Quoted 2 lines
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.
scripts/checkpatch.pl +6 -0
@@ -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",
Joe Perches
maintainerRe: [PATCH tip/core/rcu 20/21] checkpatch: Add warnings for {smp_,}read_barrier_depends()
Quoted 5 lines · Paul E. McKenney
Quoted 4 lines · Joe Perches
Quoted 2 lines · Paul E. McKenney
Now that both
smp_read_barrier_depends()andread_barrier_depends()are being de-emphasized, warn if any are added.
Quoted 8 lines
Quoted 8 lines
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...)
scripts/checkpatch.pl
Quoted 1 line
scripts/checkpatch.pl +0
-0
[]
Quoted 9 lines
Quoted 9 lines
scripts/checkpatch.pl sub process { · line 5590
Quoted 6 lines
scripts/checkpatch.pl +2
-0@@ -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
Quoted 6 lines
Quoted 4 lines
(quoted diff) +1
-0+ 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?
[]
scripts/checkpatch.pl
Quoted 15 lines
commit
ff155ce179aab891dbe2ca80f82a453383fd165aAuthor: Paul E. McKenney <[email protected]> Date: Mon Nov 27 09:37:35 2017 -0800checkpatch: Add warnings for {smp_,}read_barrier_depends()
Now that both
smp_read_barrier_depends()andread_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. ]scripts/checkpatch.pl +0
-0
[]
scripts/checkpatch.pl sub process { · line 5592
Quoted 8 lines
scripts/checkpatch.pl +4
-0@@ -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);Re: [PATCH tip/core/rcu 20/21] checkpatch: Add warnings for {smp_,}read_barrier_depends()
Quoted 1 line · Joe Perches
On Fri, 2017-12-01 at 13:44 -0800, Paul E. McKenney wrote:
[ . . . ]
Quoted 30 lines
Quoted 1 line
Good points! Like this?
[]
scripts/checkpatch.pl
Quoted 15 lines
commit
ff155ce179aab891dbe2ca80f82a453383fd165aAuthor: Paul E. McKenney <[email protected]> Date: Mon Nov 27 09:37:35 2017 -0800checkpatch: Add warnings for {smp_,}read_barrier_depends()
Now that both
smp_read_barrier_depends()andread_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. ]scripts/checkpatch.pl +0
-0[]
scripts/checkpatch.pl sub process { · line 5592
Quoted 8 lines
scripts/checkpatch.pl +4
-0@@ -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.
Quoted 7 lines
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.
scripts/checkpatch.pl +6 -0
@@ -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",
Re: [PATCH tip/core/rcu 20/21] checkpatch: Add warnings for {smp_,}read_barrier_depends()
Quoted 7 lines · Paul E. McKenney
Quoted 6 lines · Joe Perches
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:
Quoted 5 lines
Quoted 3 lines
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.
Are you going to merge this?
Normally akpm does checkpatch merging but whatever works for you is OK by me.
cheers, Joe
Quoted 30 lines
commit
e4f1d781e33c150547c68e55099794b98ee14d5eAuthor: 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()andread_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.
scripts/checkpatch.pl +6
-0@@ -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",
PATCH 21/21 drivers/vhost: Remove now-redundant read_barrier_depends()
In reply to Paul E. McKenneyBecause 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.
--- drivers/vhost/vhost.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
drivers/vhost/vhost.c +1 -6
@@ -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; }
Signature · 1 line
2.5.2
Michael S. Tsirkin
maintainerRe: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
Quoted 10 lines · Paul E. McKenney
Because
READ_ONCE()now impliesread_barrier_depends(), theread_barrier_depends()innext_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.
Quoted 24 lines
--- drivers/vhost/vhost.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)drivers/vhost/vhost.c +1
-6@@ -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; }Signature · 1 line
2.5.2
Peter Zijlstra
maintainerRe: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
Quoted 6 lines · Michael S. Tsirkin
Apropos,
READ_ONCEis now asymmetrical withWRITE_ONCE.I can read a pointer with
READ_ONCEand be sure the value is sane, but only if I also remember to put insmp_wmbbeforeWRITE_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.
Michael S. Tsirkin
maintainerRe: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
Quoted 11 lines · Peter Zijlstra
Quoted 6 lines · Michael S. Tsirkin
Apropos,
READ_ONCEis now asymmetrical withWRITE_ONCE.I can read a pointer with
READ_ONCEand be sure the value is sane, but only if I also remember to put insmp_wmbbeforeWRITE_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.
Signature · 1 line
MST
Peter Zijlstra
maintainerRe: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
Quoted 2 lines · Michael S. Tsirkin
I don't see
WRITE_ONCEinserting 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.
Michael S. Tsirkin
maintainerRe: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
Quoted 12 lines · Peter Zijlstra
Quoted 2 lines · Michael S. Tsirkin
I don't see
WRITE_ONCEinserting any barriers, release or write.Correct, never claimed there was.
Just saying that:
obj = READ_ONCE(*foo); val = READ_ONCE(obj->val);
Quoted 9 lines
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.
Signature · 1 line
MST
Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
Quoted 23 lines · Michael S. Tsirkin
Quoted 12 lines · Peter Zijlstra
Quoted 2 lines · Michael S. Tsirkin
I don't see
WRITE_ONCEinserting any barriers, release or write.Correct, never claimed there was.
Just saying that:
obj = READ_ONCE(*foo); val = READ_ONCE(obj->val);Quoted 9 lines
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.
Quoted 5 lines
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);
Quoted 5 lines
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
Michael S. Tsirkin
maintainerRe: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
Quoted 26 lines · Paul E. McKenney
Quoted 25 lines · Michael S. Tsirkin
Quoted 12 lines · Peter Zijlstra
Quoted 2 lines · Michael S. Tsirkin
I don't see
WRITE_ONCEinserting any barriers, release or write.Correct, never claimed there was.
Just saying that:
obj = READ_ONCE(*foo); val = READ_ONCE(obj->val);Quoted 9 lines
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
Quoted 8 lines
Quoted 3 lines
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?
Quoted 6 lines
Quoted 5 lines
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.
Quoted 6 lines
There are quite a few people who say that
smp_store_release()is easier for the tools to analyze than issmp_wmb(). My experience withsmp_read_barrier_depends()andrcu_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.
Signature · 1 line
MST
Peter Zijlstra
maintainerRe: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
Quoted 10 lines · Michael S. Tsirkin
Quoted 8 lines
Quoted 3 lines
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.
Michael S. Tsirkin
maintainerRe: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
Quoted 7 lines · Peter Zijlstra
Quoted 6 lines · Michael S. Tsirkin
Quoted 6 lines
Quoted 3 lines
WRITE_ONCE(obj->val, 1);smp_wmb();WRITE_ONCE(*foo, obj);I believe Peter was instead suggesting:
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
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?
Signature · 1 line
MST
Peter Zijlstra
maintainerRe: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
Quoted 11 lines · Michael S. Tsirkin
Quoted 10 lines · Peter Zijlstra
Quoted 9 lines · Michael S. Tsirkin
Quoted 8 lines
Quoted 3 lines
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);
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
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_releasethen?
Because it makes more sense. Memory ordering is hard enough, don't make it harder still if you don't have to.
Michael S. Tsirkin
maintainerRe: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
Quoted 14 lines · Peter Zijlstra
Quoted 13 lines · Michael S. Tsirkin
Quoted 12 lines · Peter Zijlstra
Quoted 10 lines · Michael S. Tsirkin
Quoted 8 lines
Quoted 3 lines
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?
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
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_releasethen?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.
Signature · 1 line
MST
Michael S. Tsirkin
maintainerRe: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
In reply to Peter ZijlstraQuoted 14 lines · Peter Zijlstra
Quoted 13 lines · Michael S. Tsirkin
Quoted 12 lines · Peter Zijlstra
Quoted 10 lines · Michael S. Tsirkin
Quoted 8 lines
Quoted 3 lines
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?
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
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_releasethen?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.
Signature · 1 line
MST
Peter Zijlstra
maintainerRe: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
In reply to Michael S. TsirkinQuoted 11 lines · Michael S. Tsirkin
Quoted 10 lines · Peter Zijlstra
Quoted 9 lines · Michael S. Tsirkin
Quoted 8 lines
Quoted 3 lines
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);
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
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_releasethen?
Because it makes more sense. Memory ordering is hard enough, don't make it harder still if you don't have to.
Michael S. Tsirkin
maintainerRe: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
In reply to Peter ZijlstraQuoted 7 lines · Peter Zijlstra
Quoted 6 lines · Michael S. Tsirkin
Quoted 6 lines
Quoted 3 lines
WRITE_ONCE(obj->val, 1);smp_wmb();WRITE_ONCE(*foo, obj);I believe Peter was instead suggesting:
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
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?
Signature · 1 line
MST
Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
In reply to Michael S. TsirkinQuoted 2 lines · Michael S. Tsirkin
Quoted 1 line · Paul E. McKenney
On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin wrote:
[ . . . ]
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
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.
Quoted 7 lines
Quoted 4 lines
There are quite a few people who say that
smp_store_release()is easier for the tools to analyze than issmp_wmb(). My experience withsmp_read_barrier_depends()andrcu_dereference()leads me to believe that they are correct.OK, but
smp_store_releaseis still not paired with anything since we rely onREAD_ONCEto 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
Michael S. Tsirkin
maintainerRe: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
Quoted 17 lines · Paul E. McKenney
Quoted 2 lines · Michael S. Tsirkin
Quoted 1 line · Paul E. McKenney
On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin wrote:
[ . . . ]
Quoted 11 lines
Quoted 10 lines
Quoted 5 lines
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 issmp_wmb(). My experience withsmp_read_barrier_depends()andrcu_dereference()leads me to believe that they are correct.
Quoted 7 lines
Quoted 2 lines
OK, but
smp_store_releaseis still not paired with anything since we rely onREAD_ONCEto include the implicit dpendendency barrier.Why wouldn't you consider the
smp_store_release()to be paired with the new improvedREAD_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?
Signature · 1 line
MST
Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
Quoted 24 lines · Michael S. Tsirkin
Quoted 17 lines · Paul E. McKenney
Quoted 2 lines · Michael S. Tsirkin
Quoted 1 line · Paul E. McKenney
On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin wrote:
[ . . . ]
Quoted 11 lines
Quoted 10 lines
Quoted 5 lines
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 issmp_wmb(). My experience withsmp_read_barrier_depends()andrcu_dereference()leads me to believe that they are correct.Quoted 5 lines
Quoted 2 lines
OK, but
smp_store_releaseis still not paired with anything since we rely onREAD_ONCEto include the implicit dpendendency barrier.Why wouldn't you consider the
smp_store_release()to be paired with the new improvedREAD_ONCE()?
Quoted 7 lines
READ_ONCEis really all over the place (some code literally replaced all memory accesses with READ/WRITE ONCE).And I also prefer
smp_wmbas it seems to be cheaper on ARM.Would an API like WRITE_POINTER()/smp_store_pointer make sense, and
READ_POINTERfor 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
Michael S. Tsirkin
maintainerRe: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
Quoted 30 lines · Paul E. McKenney
Quoted 29 lines · Michael S. Tsirkin
Quoted 17 lines · Paul E. McKenney
Quoted 2 lines · Michael S. Tsirkin
Quoted 1 line · Paul E. McKenney
On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin wrote:
[ . . . ]
Quoted 11 lines
Quoted 10 lines
Quoted 5 lines
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 issmp_wmb(). My experience withsmp_read_barrier_depends()andrcu_dereference()leads me to believe that they are correct.Quoted 5 lines
Quoted 2 lines
OK, but
smp_store_releaseis still not paired with anything since we rely onREAD_ONCEto include the implicit dpendendency barrier.Why wouldn't you consider the
smp_store_release()to be paired with the new improvedREAD_ONCE()?
READ_ONCEis really all over the place (some code literally replaced all memory accesses with READ/WRITE ONCE).And I also prefer
smp_wmbas it seems to be cheaper on ARM.
Quoted 7 lines
Quoted 2 lines
Would an API like WRITE_POINTER()/smp_store_pointer make sense, and
READ_POINTERfor 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!
Signature · 1 line
MST
Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
Quoted 31 lines · Michael S. Tsirkin
Quoted 30 lines · Paul E. McKenney
Quoted 29 lines · Michael S. Tsirkin
Quoted 17 lines · Paul E. McKenney
Quoted 2 lines · Michael S. Tsirkin
Quoted 1 line · Paul E. McKenney
On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin wrote:
[ . . . ]
Quoted 11 lines
Quoted 10 lines
Quoted 5 lines
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 issmp_wmb(). My experience withsmp_read_barrier_depends()andrcu_dereference()leads me to believe that they are correct.Quoted 5 lines
Quoted 2 lines
OK, but
smp_store_releaseis still not paired with anything since we rely onREAD_ONCEto include the implicit dpendendency barrier.Why wouldn't you consider the
smp_store_release()to be paired with the new improvedREAD_ONCE()?
READ_ONCEis really all over the place (some code literally replaced all memory accesses with READ/WRITE ONCE).And I also prefer
smp_wmbas it seems to be cheaper on ARM.
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
Would an API like WRITE_POINTER()/smp_store_pointer make sense, and
READ_POINTERfor 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
Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
In reply to Michael S. TsirkinQuoted 31 lines · Michael S. Tsirkin
Quoted 30 lines · Paul E. McKenney
Quoted 29 lines · Michael S. Tsirkin
Quoted 17 lines · Paul E. McKenney
Quoted 2 lines · Michael S. Tsirkin
Quoted 1 line · Paul E. McKenney
On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin wrote:
[ . . . ]
Quoted 11 lines
Quoted 10 lines
Quoted 5 lines
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 issmp_wmb(). My experience withsmp_read_barrier_depends()andrcu_dereference()leads me to believe that they are correct.Quoted 5 lines
Quoted 2 lines
OK, but
smp_store_releaseis still not paired with anything since we rely onREAD_ONCEto include the implicit dpendendency barrier.Why wouldn't you consider the
smp_store_release()to be paired with the new improvedREAD_ONCE()?
READ_ONCEis really all over the place (some code literally replaced all memory accesses with READ/WRITE ONCE).And I also prefer
smp_wmbas it seems to be cheaper on ARM.
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
Would an API like WRITE_POINTER()/smp_store_pointer make sense, and
READ_POINTERfor 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
Michael S. Tsirkin
maintainerRe: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
In reply to Paul E. McKenneyQuoted 30 lines · Paul E. McKenney
Quoted 29 lines · Michael S. Tsirkin
Quoted 17 lines · Paul E. McKenney
Quoted 2 lines · Michael S. Tsirkin
Quoted 1 line · Paul E. McKenney
On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin wrote:
[ . . . ]
Quoted 11 lines
Quoted 10 lines
Quoted 5 lines
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 issmp_wmb(). My experience withsmp_read_barrier_depends()andrcu_dereference()leads me to believe that they are correct.Quoted 5 lines
Quoted 2 lines
OK, but
smp_store_releaseis still not paired with anything since we rely onREAD_ONCEto include the implicit dpendendency barrier.Why wouldn't you consider the
smp_store_release()to be paired with the new improvedREAD_ONCE()?
READ_ONCEis really all over the place (some code literally replaced all memory accesses with READ/WRITE ONCE).And I also prefer
smp_wmbas it seems to be cheaper on ARM.
Quoted 7 lines
Quoted 2 lines
Would an API like WRITE_POINTER()/smp_store_pointer make sense, and
READ_POINTERfor 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!
Signature · 1 line
MST
Peter Zijlstra
maintainerRe: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
In reply to Paul E. McKenneyQuoted 2 lines · Paul E. McKenney
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 ;-)
Peter Zijlstra
maintainerRe: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
In reply to Paul E. McKenneyQuoted 2 lines · Paul E. McKenney
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 ;-)
Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
In reply to Michael S. TsirkinQuoted 24 lines · Michael S. Tsirkin
Quoted 17 lines · Paul E. McKenney
Quoted 2 lines · Michael S. Tsirkin
Quoted 1 line · Paul E. McKenney
On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin wrote:
[ . . . ]
Quoted 11 lines
Quoted 10 lines
Quoted 5 lines
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 issmp_wmb(). My experience withsmp_read_barrier_depends()andrcu_dereference()leads me to believe that they are correct.Quoted 5 lines
Quoted 2 lines
OK, but
smp_store_releaseis still not paired with anything since we rely onREAD_ONCEto include the implicit dpendendency barrier.Why wouldn't you consider the
smp_store_release()to be paired with the new improvedREAD_ONCE()?
Quoted 7 lines
READ_ONCEis really all over the place (some code literally replaced all memory accesses with READ/WRITE ONCE).And I also prefer
smp_wmbas it seems to be cheaper on ARM.Would an API like WRITE_POINTER()/smp_store_pointer make sense, and
READ_POINTERfor 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
Peter Zijlstra
maintainerRe: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
In reply to Michael S. TsirkinQuoted 2 lines · Michael S. Tsirkin
READ_ONCEis 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.
Quoted 2 lines
Would an API like WRITE_POINTER()/smp_store_pointer make sense, and
READ_POINTERfor symmetry?
No, the whole point of the exercise was to get away from the fact that dependent loads are special.
Michael S. Tsirkin
maintainerRe: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
Quoted 5 lines · Peter Zijlstra
Quoted 2 lines · Michael S. Tsirkin
READ_ONCEis 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*.
Quoted 1 line
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.
Quoted 5 lines
Quoted 2 lines
Would an API like WRITE_POINTER()/smp_store_pointer make sense, and
READ_POINTERfor 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.
Signature · 1 line
MST
Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
Quoted 12 lines · Michael S. Tsirkin
Quoted 5 lines · Peter Zijlstra
Quoted 2 lines · Michael S. Tsirkin
READ_ONCEis 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_ONCEand 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*.
Quoted 4 lines
Quoted 1 line
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.
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
Would an API like WRITE_POINTER()/smp_store_pointer make sense, and
READ_POINTERfor 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
Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
In reply to Michael S. TsirkinQuoted 12 lines · Michael S. Tsirkin
Quoted 5 lines · Peter Zijlstra
Quoted 2 lines · Michael S. Tsirkin
READ_ONCEis 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_ONCEand 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*.
Quoted 4 lines
Quoted 1 line
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.
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
Would an API like WRITE_POINTER()/smp_store_pointer make sense, and
READ_POINTERfor 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
Michael S. Tsirkin
maintainerRe: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
In reply to Peter ZijlstraQuoted 5 lines · Peter Zijlstra
Quoted 2 lines · Michael S. Tsirkin
READ_ONCEis 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*.
Quoted 1 line
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.
Quoted 5 lines
Quoted 2 lines
Would an API like WRITE_POINTER()/smp_store_pointer make sense, and
READ_POINTERfor 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.
Signature · 1 line
MST
Peter Zijlstra
maintainerRe: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
In reply to Michael S. TsirkinQuoted 2 lines · Michael S. Tsirkin
READ_ONCEis 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.
Quoted 2 lines
Would an API like WRITE_POINTER()/smp_store_pointer make sense, and
READ_POINTERfor symmetry?
No, the whole point of the exercise was to get away from the fact that dependent loads are special.
Michael S. Tsirkin
maintainerRe: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
In reply to Paul E. McKenneyQuoted 17 lines · Paul E. McKenney
Quoted 2 lines · Michael S. Tsirkin
Quoted 1 line · Paul E. McKenney
On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin wrote:
[ . . . ]
Quoted 11 lines
Quoted 10 lines
Quoted 5 lines
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 issmp_wmb(). My experience withsmp_read_barrier_depends()andrcu_dereference()leads me to believe that they are correct.
Quoted 7 lines
Quoted 2 lines
OK, but
smp_store_releaseis still not paired with anything since we rely onREAD_ONCEto include the implicit dpendendency barrier.Why wouldn't you consider the
smp_store_release()to be paired with the new improvedREAD_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?
Signature · 1 line
MST
Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
In reply to Michael S. TsirkinQuoted 2 lines · Michael S. Tsirkin
Quoted 1 line · Paul E. McKenney
On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin wrote:
[ . . . ]
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
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.
Quoted 7 lines
Quoted 4 lines
There are quite a few people who say that
smp_store_release()is easier for the tools to analyze than issmp_wmb(). My experience withsmp_read_barrier_depends()andrcu_dereference()leads me to believe that they are correct.OK, but
smp_store_releaseis still not paired with anything since we rely onREAD_ONCEto 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
Michael S. Tsirkin
maintainerRe: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
In reply to Paul E. McKenneyQuoted 26 lines · Paul E. McKenney
Quoted 25 lines · Michael S. Tsirkin
Quoted 12 lines · Peter Zijlstra
Quoted 2 lines · Michael S. Tsirkin
I don't see
WRITE_ONCEinserting any barriers, release or write.Correct, never claimed there was.
Just saying that:
obj = READ_ONCE(*foo); val = READ_ONCE(obj->val);Quoted 9 lines
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
Quoted 8 lines
Quoted 3 lines
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?
Quoted 6 lines
Quoted 5 lines
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.
Quoted 6 lines
There are quite a few people who say that
smp_store_release()is easier for the tools to analyze than issmp_wmb(). My experience withsmp_read_barrier_depends()andrcu_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.
Signature · 1 line
MST
Peter Zijlstra
maintainerRe: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
In reply to Michael S. TsirkinQuoted 23 lines · Michael S. Tsirkin
Quoted 12 lines · Peter Zijlstra
Quoted 2 lines · Michael S. Tsirkin
I don't see
WRITE_ONCEinserting any barriers, release or write.Correct, never claimed there was.
Just saying that:
obj = READ_ONCE(*foo); val = READ_ONCE(obj->val);Quoted 9 lines
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.
Quoted 5 lines
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.
Quoted 2 lines
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.
Quoted 2 lines
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.
Peter Zijlstra
maintainerRe: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
In reply to Michael S. TsirkinQuoted 2 lines · Michael S. Tsirkin
I don't see
WRITE_ONCEinserting 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.
Michael S. Tsirkin
maintainerRe: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
In reply to Paul E. McKenneyQuoted 10 lines · Paul E. McKenney
Because
READ_ONCE()now impliesread_barrier_depends(), theread_barrier_depends()innext_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.
Quoted 24 lines
--- drivers/vhost/vhost.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)drivers/vhost/vhost.c +1
-6@@ -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; }Signature · 1 line
2.5.2