Re: [GIT PULL] locking changes for v4.4
On Wed, Nov 4, 2015 at 5:01 PM, Peter Zijlstra wrote: > On Wed, Nov 04, 2015 at 03:51:01PM +0100, Dmitry Vyukov wrote: >> To clarify, yes, documentation and tooling was my main motivation. > > Right; I don't object to having _ctrl() methods purely for documentation > purposes, I keep finding places we rely on them. Having them stand out > better might be useful. > >> It is usually helpful to see acquire/release, rmb/wmb pairs, and so it >> is useful to know that something below is ordered wrt this load by >> means of a control dependency (which effectively becomes an acquire, >> and there must be a pairing release somewhere). > > You need at least a trailing smp_rmb() before you cover the ACQUIRE > semantics -- or have no trailing reads at all of course. Yes, I know, but is the best we can do. Episodic false negatives are OK, while false positives are unacceptable. So we considered READ_ONCE_CTRL as acquire. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] locking changes for v4.4
On Wed, Nov 04, 2015 at 03:51:01PM +0100, Dmitry Vyukov wrote: > To clarify, yes, documentation and tooling was my main motivation. Right; I don't object to having _ctrl() methods purely for documentation purposes, I keep finding places we rely on them. Having them stand out better might be useful. > It is usually helpful to see acquire/release, rmb/wmb pairs, and so it > is useful to know that something below is ordered wrt this load by > means of a control dependency (which effectively becomes an acquire, > and there must be a pairing release somewhere). You need at least a trailing smp_rmb() before you cover the ACQUIRE semantics -- or have no trailing reads at all of course. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] locking changes for v4.4
* Paul E. McKenney wrote: > On Wed, Nov 04, 2015 at 12:48:50PM +0100, Ingo Molnar wrote: > > > > * Linus Torvalds wrote: > > > > > On Tue, Nov 3, 2015 at 3:58 PM, Linus Torvalds > > > wrote: > > > > > > > > I think I'll pull this, but then just make a separate commit to remove > > > > Thanks! > > > > > > all the bogus games with "control" dependencies that seem to have no > > > > basis is > > > > reality. > > > > > > So the attached is what I committed in my tree. [...] > > > > Acked-by: Ingo Molnar > > > > > + dependency into nonexistence. Careful use of READ_ONCE() or > > > + atomic{,64}_read() can help to preserve your control dependency. > > > + Please see the Compiler Barrier section for more information. > > > > So technically it's the "COMPILER BARRIER" section, but this is a > > pre-existing > > formulation and the document doesn't use such references consistently so I > > guess > > it doesn't matter much. > > I will take a pass through and fix these up. I don't see this as urgent, > so will add it to my patches for the next merge window. Sounds good to me! Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] locking changes for v4.4
On Wed, Nov 4, 2015 at 5:16 AM, Paul E. McKenney wrote: > On Tue, Nov 03, 2015 at 05:30:29PM -0800, Linus Torvalds wrote: >> On Tue, Nov 3, 2015 at 3:58 PM, Linus Torvalds >> wrote: >> > >> > I think I'll pull this, but then just make a separate commit to remove >> > all the bogus games with "control" dependencies that seem to have no >> > basis is reality. >> >> So the attached is what I committed in my tree. It took much longer to >> try to write the rationale than it took to actually remove the >> atomic_read_ctrl() functions, and even so I'm not sure how good that >> commit message is. But at least it tries to explain what's going on. >> >> Note the final part of the rationale: >> >> I may have to eat my words at some point, but in the absense of clear >> proof that alpha actually needs this, or indeed even an explanation of >> how alpha could _possibly_ need it, I do not believe these functions are >> called for. >> >> And if it turns out that alpha really _does_ need a barrier for this >> case, that barrier still should not be "smp_read_barrier_depends()". >> We'd have to make up some new speciality barrier just for alpha, along >> with the documentation for why it really is necessary. > > For whatever it is worth, the patch looks good to me. The reasons I > could imagine why we might want to mark control dependencies are things > like documentation and tooling, but given that we currently only have a > very small number of them, it is hard to argue that this is of immediate > concern, if it is ever of concern. To clarify, yes, documentation and tooling was my main motivation. It is usually helpful to see acquire/release, rmb/wmb pairs, and so it is useful to know that something below is ordered wrt this load by means of a control dependency (which effectively becomes an acquire, and there must be a pairing release somewhere). As for the tooling, as you may know, we are working on KernelThreadSanitizer, which is dynamic happens-before-based race detector (dynamically tracks acquire/release pairs to establish synchronizes-with relation). As shown by memory_order_consume tracking dependencies in compiler is practically impossible, and so the tool needs some hint for these. But we don't need to sort it out right now. And I agree we need a clear idea as to why we are doing this. And we definitely must not hide it under the alpha excuse. And our main headache for now is "benign" data races anyway (missing READ/WRITE_ONCE). -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] locking changes for v4.4
On Wed, Nov 04, 2015 at 12:37:01PM +0100, Peter Zijlstra wrote: > On Tue, Nov 03, 2015 at 05:30:29PM -0800, Linus Torvalds wrote: > > From 105ff3cbf225036b75a6a46c96d1ddce8e7bdc66 Mon Sep 17 00:00:00 2001 > > From: Linus Torvalds > > Date: Tue, 3 Nov 2015 17:22:17 -0800 > > Subject: [PATCH] atomic: remove all traces of READ_ONCE_CTRL() and > > atomic*_read_ctrl() > > > So I do not see how these "x_ctrl()" functions can currently be necessary. > > > Cc: Peter Zijlstra > > Cc: Paul E McKenney > > Cc: Dmitry Vyukov > > Cc: Will Deacon > > Cc: Ingo Molnar > > Signed-off-by: Linus Torvalds > > Hooray! > > Now if only we could convince the C/C++ people that write speculation is > a bad idea :-) Sigh... Working on it... Thanx, Paul > Acked-by: Peter Zijlstra (Intel) > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] locking changes for v4.4
On Wed, Nov 04, 2015 at 12:48:50PM +0100, Ingo Molnar wrote: > > * Linus Torvalds wrote: > > > On Tue, Nov 3, 2015 at 3:58 PM, Linus Torvalds > > wrote: > > > > > > I think I'll pull this, but then just make a separate commit to remove > > Thanks! > > > > all the bogus games with "control" dependencies that seem to have no > > > basis is > > > reality. > > > > So the attached is what I committed in my tree. [...] > > Acked-by: Ingo Molnar > > > + dependency into nonexistence. Careful use of READ_ONCE() or > > + atomic{,64}_read() can help to preserve your control dependency. > > + Please see the Compiler Barrier section for more information. > > So technically it's the "COMPILER BARRIER" section, but this is a > pre-existing > formulation and the document doesn't use such references consistently so I > guess > it doesn't matter much. I will take a pass through and fix these up. I don't see this as urgent, so will add it to my patches for the next merge window. Thanx, Paul -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] locking changes for v4.4
* Linus Torvalds wrote: > On Tue, Nov 3, 2015 at 3:58 PM, Linus Torvalds > wrote: > > > > I think I'll pull this, but then just make a separate commit to remove Thanks! > > all the bogus games with "control" dependencies that seem to have no basis > > is > > reality. > > So the attached is what I committed in my tree. [...] Acked-by: Ingo Molnar > + dependency into nonexistence. Careful use of READ_ONCE() or > + atomic{,64}_read() can help to preserve your control dependency. > + Please see the Compiler Barrier section for more information. So technically it's the "COMPILER BARRIER" section, but this is a pre-existing formulation and the document doesn't use such references consistently so I guess it doesn't matter much. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] locking changes for v4.4
On Tue, Nov 03, 2015 at 05:30:29PM -0800, Linus Torvalds wrote: > From 105ff3cbf225036b75a6a46c96d1ddce8e7bdc66 Mon Sep 17 00:00:00 2001 > From: Linus Torvalds > Date: Tue, 3 Nov 2015 17:22:17 -0800 > Subject: [PATCH] atomic: remove all traces of READ_ONCE_CTRL() and > atomic*_read_ctrl() > So I do not see how these "x_ctrl()" functions can currently be necessary. > Cc: Peter Zijlstra > Cc: Paul E McKenney > Cc: Dmitry Vyukov > Cc: Will Deacon > Cc: Ingo Molnar > Signed-off-by: Linus Torvalds Hooray! Now if only we could convince the C/C++ people that write speculation is a bad idea :-) Acked-by: Peter Zijlstra (Intel) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] locking changes for v4.4
On Tue, Nov 03, 2015 at 05:30:29PM -0800, Linus Torvalds wrote: > On Tue, Nov 3, 2015 at 3:58 PM, Linus Torvalds > wrote: > > > > I think I'll pull this, but then just make a separate commit to remove > > all the bogus games with "control" dependencies that seem to have no > > basis is reality. > > So the attached is what I committed in my tree. It took much longer to > try to write the rationale than it took to actually remove the > atomic_read_ctrl() functions, and even so I'm not sure how good that > commit message is. But at least it tries to explain what's going on. > > Note the final part of the rationale: > > I may have to eat my words at some point, but in the absense of clear > proof that alpha actually needs this, or indeed even an explanation of > how alpha could _possibly_ need it, I do not believe these functions are > called for. > > And if it turns out that alpha really _does_ need a barrier for this > case, that barrier still should not be "smp_read_barrier_depends()". > We'd have to make up some new speciality barrier just for alpha, along > with the documentation for why it really is necessary. For whatever it is worth, the patch looks good to me. The reasons I could imagine why we might want to mark control dependencies are things like documentation and tooling, but given that we currently only have a very small number of them, it is hard to argue that this is of immediate concern, if it is ever of concern. Thanx, Paul -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] locking changes for v4.4
On Tue, Nov 3, 2015 at 3:58 PM, Linus Torvalds wrote: > > I think I'll pull this, but then just make a separate commit to remove > all the bogus games with "control" dependencies that seem to have no > basis is reality. So the attached is what I committed in my tree. It took much longer to try to write the rationale than it took to actually remove the atomic_read_ctrl() functions, and even so I'm not sure how good that commit message is. But at least it tries to explain what's going on. Note the final part of the rationale: I may have to eat my words at some point, but in the absense of clear proof that alpha actually needs this, or indeed even an explanation of how alpha could _possibly_ need it, I do not believe these functions are called for. And if it turns out that alpha really _does_ need a barrier for this case, that barrier still should not be "smp_read_barrier_depends()". We'd have to make up some new speciality barrier just for alpha, along with the documentation for why it really is necessary. so it's possible that we'll have to re-introduce these things, even if I am obviously doubtful. However, if we do that, I think we need much more explanations for why they would be necessary, and we'd want to make another "smp_read_to_write_ctrl_barrier()" or something that makes this particular ordering explicit. Because I don't think "smp_read_barrier_depends()" is that barrier, even if it might have a very similar implementation. >From everything I have seen in the alpha architecture manual, it is really just read-to-dependent-read that is special (and in fact alpha there is "consistent": it doesn't matter whether there's a data dependency or a control dependency between the two reads). While "read-to-dependent-write" actually seems to be documented to be ordered, and act like other architectures (and again, it doesn't matter whether it's a control or data dependency between the read and the write). That said, I sure did *not* enjoy reading that crazy memory ordering documentation again. People who say that x86 memory ordering isn't clearly defined have clearly not read the alpha manual. Gods, I hope I will never have to try to read that ever again... Added Dmitry Vyukov to the cc, since he seems to be one of the people who wanted to use the atomic_read_ctrl() thing. So at least for now, it's just something we assume is always valid for READ_ONCE() and for atomic*_read(): doing a dependent write is just "ordered" with the read it depends on (whether that's a control dependency or a data one). Linus From 105ff3cbf225036b75a6a46c96d1ddce8e7bdc66 Mon Sep 17 00:00:00 2001 From: Linus Torvalds Date: Tue, 3 Nov 2015 17:22:17 -0800 Subject: [PATCH] atomic: remove all traces of READ_ONCE_CTRL() and atomic*_read_ctrl() This seems to be a mis-reading of how alpha memory ordering works, and is not backed up by the alpha architecture manual. The helper functions don't do anything special on any other architectures, and the arguments that support them being safe on other architectures also argue that they are safe on alpha. Basically, the "control dependency" is between a previous read and a subsequent write that is dependent on the value read. Even if the subsequent write is actually done speculatively, there is no way that such a speculative write could be made visible to other cpu's until it has been committed, which requires validating the speculation. Note that most weakely ordered architectures (very much including alpha) do not guarantee any ordering relationship between two loads that depend on each other on a control dependency: read A if (val == 1) read B because the conditional may be predicted, and the "read B" may be speculatively moved up to before reading the value A. So we require the user to insert a smp_rmb() between the two accesses to be correct: read A; if (A == 1) smp_rmb() read B Alpha is further special in that it can break that ordering even if the *address* of B depends on the read of A, because the cacheline that is read later may be stale unless you have a memory barrier in between the pointer read and the read of the value behind a pointer: read ptr read offset(ptr) whereas all other weakly ordered architectures guarantee that the data dependency (as opposed to just a control dependency) will order the two accesses. As a result, alpha needs a "smp_read_barrier_depends()" in between those two reads for them to be ordered. The coontrol dependency that "READ_ONCE_CTRL()" and "atomic_read_ctrl()" had was a control dependency to a subsequent *write*, however, and nobody can finalize such a subsequent write without having actually done the read. And were you to write such a value to a "stale" cacheline (the way the unordered reads came to be), that would seem to lose the write entirely. So the things that make alpha able to re-order reads even more aggressively
Re: [GIT PULL] locking changes for v4.4
On Tue, Nov 3, 2015 at 3:54 PM, Linus Torvalds wrote: > On Tue, Nov 3, 2015 at 1:16 AM, Ingo Molnar wrote: >> >> - More gradual enhancements to atomic ops: new atomic*_read_ctrl() ops, >> synchronize atomic_{read,set}() ordering requirements between >> architectures, >> add atomic_long_t bitops. (Peter Zijlstra) > > From another thread: those new "atomic*_read_ctrl()" operations are > complete voodoo programming, and should never ever be used. Sadly, that commit seems to be in the middle of the series. I think I'll pull this, but then just make a separate commit to remove all the bogus games with "control" dependencies that seem to have no basis is reality. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [GIT PULL] locking changes for v4.4
On Tue, Nov 3, 2015 at 1:16 AM, Ingo Molnar wrote: > > - More gradual enhancements to atomic ops: new atomic*_read_ctrl() ops, > synchronize atomic_{read,set}() ordering requirements between > architectures, > add atomic_long_t bitops. (Peter Zijlstra) >From another thread: those new "atomic*_read_ctrl()" operations are complete voodoo programming, and should never ever be used. Those helpers seem to be based entirely on a mis-reading of alpha memory ordering that is actually not possible in a universe where causality exists. It's not a new disease - we already have READ_ONCE_CTRL(), but it is currently only actually used in one single place (but mentioned many times in documentation that is looking less and less like reality). But this pull request seems to make that thing be institutional, and spreads this mis-understanding out, and tries to "document" it as some kind of actual truth. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[GIT PULL] locking changes for v4.4
Linus, Please pull the latest locking-core-for-linus git tree from: git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git locking-core-for-linus # HEAD: 6e490b0106a2118ee4c37c37847454a5c2dc6e32 ARM, locking/atomics: Implement _relaxed variants of atomic[64]_{inc,dec} The main changes in this cycle were: - More gradual enhancements to atomic ops: new atomic*_read_ctrl() ops, synchronize atomic_{read,set}() ordering requirements between architectures, add atomic_long_t bitops. (Peter Zijlstra) - Add _{relaxed|acquire|release}() variants for inc/dec atomics and use them in various locking primitives: mutex, rtmutex, mcs, rwsem. This enables weakly ordered architectures (such as arm64) to make use of more locking related optimizations. (Davidlohr Bueso) - Implement atomic[64]_{inc,dec}_relaxed() on ARM. (Will Deacon) - Futex kernel data cache footprint micro-optimization. (Rasmus Villemoes) - pvqspinlock runtime overhead micro-optimization. (Waiman Long) - misc smaller fixlets. Thanks, Ingo --> Boqun Feng (1): locking/atomics, cmpxchg: Privatize the inclusion of asm/cmpxchg.h Davidlohr Bueso (7): locking/qrwlock: Rename ->lock to ->wait_lock locking/osq: Relax atomic semantics locking/asm-generic: Add _{relaxed|acquire|release}() variants for inc/dec atomics locking/mutex: Use acquire/release semantics locking/rtmutex: Use acquire/release semantics locking/mcs: Use acquire/release semantics locking/rwsem: Use acquire/release semantics Peter Zijlstra (3): atomic: Add atomic_long_t bitops atomic, arch: Audit atomic_{read,set}() atomic: Implement atomic_read_ctrl() Rasmus Villemoes (1): futex: Force hot variables into a single cache line Stephen Boyd (1): locking/Documentation/lockstat: Fix typo - lokcing -> locking Waiman Long (1): locking/pvqspinlock: Kick the PV CPU unconditionally when _Q_SLOW_VAL Will Deacon (1): ARM, locking/atomics: Implement _relaxed variants of atomic[64]_{inc,dec} Documentation/atomic_ops.txt | 4 ++ Documentation/locking/lockstat.txt | 2 +- Documentation/memory-barriers.txt | 17 ++--- arch/alpha/include/asm/atomic.h| 8 +-- arch/arc/include/asm/atomic.h | 8 +-- arch/arm/include/asm/atomic.h | 12 ++-- arch/arm64/include/asm/atomic.h| 2 +- arch/avr32/include/asm/atomic.h| 4 +- arch/frv/include/asm/atomic.h | 4 +- arch/h8300/include/asm/atomic.h| 4 +- arch/hexagon/include/asm/atomic.h | 2 +- arch/ia64/include/asm/atomic.h | 8 +-- arch/m32r/include/asm/atomic.h | 4 +- arch/m68k/include/asm/atomic.h | 4 +- arch/metag/include/asm/atomic_lnkget.h | 2 +- arch/metag/include/asm/atomic_lock1.h | 2 +- arch/mips/include/asm/atomic.h | 8 +-- arch/mn10300/include/asm/atomic.h | 4 +- arch/parisc/include/asm/atomic.h | 2 +- arch/sh/include/asm/atomic.h | 4 +- arch/sparc/include/asm/atomic_64.h | 8 +-- arch/tile/include/asm/atomic.h | 2 +- arch/tile/include/asm/atomic_64.h | 6 +- arch/x86/include/asm/atomic.h | 4 +- arch/x86/include/asm/atomic64_64.h | 4 +- arch/xtensa/include/asm/atomic.h | 4 +- drivers/net/ethernet/sfc/mcdi.c| 2 +- drivers/phy/phy-rcar-gen2.c| 3 +- drivers/staging/speakup/selection.c| 2 +- include/asm-generic/atomic-long.h | 56 +--- include/asm-generic/atomic.h | 4 +- include/asm-generic/mutex-dec.h| 8 +-- include/asm-generic/mutex-xchg.h | 10 +-- include/asm-generic/qrwlock_types.h| 4 +- include/asm-generic/rwsem.h| 21 -- include/linux/atomic.h | 118 - kernel/futex.c | 13 +++- kernel/locking/mcs_spinlock.h | 4 +- kernel/locking/mutex.c | 9 +-- kernel/locking/osq_lock.c | 11 ++- kernel/locking/qrwlock.c | 8 +-- kernel/locking/qspinlock_paravirt.h| 6 +- kernel/locking/rtmutex.c | 30 ++--- kernel/locking/rwsem-xadd.c| 5 +- 44 files changed, 304 insertions(+), 143 deletions(-) diff --git a/Documentation/atomic_ops.txt b/Documentation/atomic_ops.txt index b19fc34efdb1..c9d1cacb4395 100644 --- a/Documentation/atomic_ops.txt +++ b/Documentation/atomic_ops.txt @@ -542,6 +542,10 @@ The routines xchg() and cmpxchg() must provide the same exact memory-barrier semantics as the atomic and bit operations returning values. +Note: If someone wants to use xchg(), cmpxchg() and their variants, +linux/atomic.h should be included rather than asm/cmpxchg.h, unless +the code is in arch/* and can take care of itself. + Spinlocks and rwlocks have memory barrier expectations as well. The r