On Tue, Jan 26, 2016 at 12:10 PM, Paul E. McKenney <paulmck at linux.vnet.ibm.com> wrote:> On Tue, Jan 26, 2016 at 11:44:46AM -0800, Linus Torvalds wrote: >> >> > struct foo *x = READ_ONCE(*ptr); >> > smp_read_barrier_depends(); >> > x->bar = 5; >> >> This case is complete BS. Stop perpetuating it. I already removed a >> number of bogus cases of it, and I removed the incorrect documentation >> that had this crap. > > If I understand your objection correctly, you want the above pattern > expressed either like this: > > struct foo *x = rcu_dereference(*ptr); > x->bar = 5; > > Or like this: > > struct foo *x = lockless_dereference(*ptr); > x->bar = 5; > > Or am I missing your point?You are entirely missing the point. You might as well just write it as struct foo x = READ_ONCE(*ptr); x->bar = 5; because that "smp_read_barrier_depends()" does NOTHING wrt the second write. So what I am saying is simple: anybody who writes that "smp_read_barrier_depends()" in there is just ttoally and completely WRONG, and the fact that Peter wrote it out after I removed several instances of that bloody f*cking idiocy is disturbing. Don't do it. It's BS. It's wrong. Don't make excuses for it. Linus
On Tue, Jan 26, 2016 at 2:15 PM, Linus Torvalds <torvalds at linux-foundation.org> wrote:> > You might as well just write it as > > struct foo x = READ_ONCE(*ptr); > x->bar = 5; > > because that "smp_read_barrier_depends()" does NOTHING wrt the second write.Just to clarify: on alpha it adds a memory barrier, but that memory barrier is useless. On non-alpha, it is a no-op, and obviously does nothing simply because it generates no code. So if anybody believes that the "smp_read_barrier_depends()" does something, they are *wrong*. And if anybody sends out an email with that smp_read_barrier_depends() in an example, they are actively just confusing other people, which is even worse than just being wrong. Which is why I jumped in. So stop perpetuating the myth that smp_read_barrier_depends() does something here. It does not. It's a bug, and it has become this "mind virus" for some people that seem to believe that it does something. I had to remove this crap once from the kernel already, see commit 105ff3cbf225 ("atomic: remove all traces of READ_ONCE_CTRL() and atomic*_read_ctrl()"). I don't want to ever see that broken construct again. And I want to make sure that everybody is educated about how broken it was. I'm extremely unhappy that it came up again. If it turns out that some architecture does actually need a barrier between a read and a dependent write, then that will mean that (a) we'll have to make up a _new_ barrier, because "smp_read_barrier_depends()" is not that barrier. We'll presumably then have to make that new barrier part of "rcu_derefence()" and friends. (b) we will have found an architecture with even worse memory ordering semantics than alpha, and we'll have to stop castigating alpha for being the worst memory ordering ever. but I sincerely hope that we'll never find that kind of broken architecture. Linus
On Tue, Jan 26, 2016 at 02:33:40PM -0800, Linus Torvalds wrote:> On Tue, Jan 26, 2016 at 2:15 PM, Linus Torvalds > <torvalds at linux-foundation.org> wrote: > > > > You might as well just write it as > > > > struct foo x = READ_ONCE(*ptr); > > x->bar = 5; > > > > because that "smp_read_barrier_depends()" does NOTHING wrt the second write. > > Just to clarify: on alpha it adds a memory barrier, but that memory > barrier is useless.No trailing data-dependent read, so agreed, no smp_read_barrier_depends() needed. That said, I believe that we should encourage rcu_dereference*() or lockless_dereference() instead of READ_ONCE() for documentation reasons, though.> On non-alpha, it is a no-op, and obviously does nothing simply because > it generates no code. > > So if anybody believes that the "smp_read_barrier_depends()" does > something, they are *wrong*.The other problem with smp_read_barrier_depends() is that it is often a pain figuring out which prior load it is supposed to apply to. Hence my preference for rcu_dereference*() and lockless_dereference().> And if anybody sends out an email with that smp_read_barrier_depends() > in an example, they are actively just confusing other people, which is > even worse than just being wrong. Which is why I jumped in. > > So stop perpetuating the myth that smp_read_barrier_depends() does > something here. It does not. It's a bug, and it has become this "mind > virus" for some people that seem to believe that it does something.It looks like I should add words to memory-barriers.txt de-emphasizing smp_read_barrier_depends(). I will take a look at that.> I had to remove this crap once from the kernel already, see commit > 105ff3cbf225 ("atomic: remove all traces of READ_ONCE_CTRL() and > atomic*_read_ctrl()"). > > I don't want to ever see that broken construct again. And I want to > make sure that everybody is educated about how broken it was. I'm > extremely unhappy that it came up again.Well, if it makes you feel better, that was control dependencies and this was data dependencies. So it was not -exactly- the same. ;-) (Sorry, couldn't resist...)> If it turns out that some architecture does actually need a barrier > between a read and a dependent write, then that will mean that > > (a) we'll have to make up a _new_ barrier, because > "smp_read_barrier_depends()" is not that barrier. We'll presumably > then have to make that new barrier part of "rcu_derefence()" and > friends.Agreed. We can worry about whether or not we replace the current smp_read_barrier_depends() with that new barrier when and if such hardware appears.> (b) we will have found an architecture with even worse memory > ordering semantics than alpha, and we'll have to stop castigating > alpha for being the worst memory ordering ever.;-) ;-) ;-)> but I sincerely hope that we'll never find that kind of broken architecture.Apparently at least some hardware vendors are reading memory-barriers.txt, so perhaps the odds of that kind of breakage have reduced. Thanx, Paul
On Tue, Jan 26, 2016 at 02:33:40PM -0800, Linus Torvalds wrote:> If it turns out that some architecture does actually need a barrier > between a read and a dependent write, then that will mean that > > (a) we'll have to make up a _new_ barrier, because > "smp_read_barrier_depends()" is not that barrier. We'll presumably > then have to make that new barrier part of "rcu_derefence()" and > friends. > > (b) we will have found an architecture with even worse memory > ordering semantics than alpha, and we'll have to stop castigating > alpha for being the worst memory ordering ever. > > but I sincerely hope that we'll never find that kind of broken architecture.So for a moment it looked like MIPS wanted to equal or surpass Alpha in this respect. And Paul made the point that smp_read_barrier_depends() really should be smp_aquire_barrier_depends() in that we rely on both dependent reads and writes to be ordered against the initial pointer load. Now, as you've made abundantly clear, Alpha does this, although it needs the little extra help in the dependent read department. The 'problem' is that someone seemed to have used our Documentation/memory-barriers.txt as a specification for what hardware is permitted and we require. And in that light Paul noted that read_barrier_depends really should be considered an acquire_barrier_depends and order both dependent reads and writes against the (prior) read (if nothing else already does). Now clearly, any sane architecture doesn't need anything like this, but again our document doesn't seem to judge. That is, from reading the document one can get the impression is a perfectly fine thing to do. Nowhere does our disdain for this thing show.