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 3:29 PM, Paul E. McKenney <paulmck at linux.vnet.ibm.com> wrote:> > 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.I agree that that is likely the right thing to do in pretty much all situations. In theory, there might be performance situations where we'd want to actively avoid the smp_read_barrier_depends() inherent in those, but considering that it's only a performance issue on alpha, and we probably have all of two or three people using Linux on alpha, it's a pretty theoretical performance worry. Linus
On Tue, Jan 26, 2016 at 03:45:23PM -0800, Linus Torvalds wrote:> On Tue, Jan 26, 2016 at 3:29 PM, Paul E. McKenney > <paulmck at linux.vnet.ibm.com> wrote: > > > > 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. > > I agree that that is likely the right thing to do in pretty much all situations. > > In theory, there might be performance situations where we'd want to > actively avoid the smp_read_barrier_depends() inherent in those, but > considering that it's only a performance issue on alpha, and we > probably have all of two or three people using Linux on alpha, it's a > pretty theoretical performance worry.Agreed! Thanx, Paul
On Tue, Jan 26, 2016 at 03:29:21PM -0800, Paul E. McKenney wrote:> 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(). >Because semantically speaking, rcu_derefence*() and lockless_dereference() are CONSUME(i.e. data/address dependent read->read and read->write pairs are ordered), whereas smp_read_barrier_depends() only guarantees read->read pairs with data dependency are ordered, right? If so, maybe we need to call it out in memory-barriers.txt, for example: diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt index 904ee42..6b262c2 100644 --- a/Documentation/memory-barriers.txt +++ b/Documentation/memory-barriers.txt @@ -1703,8 +1703,8 @@ There are some more advanced barrier functions: (*) lockless_dereference(); - This can be thought of as a pointer-fetch wrapper around the - smp_read_barrier_depends() data-dependency barrier. + This is a load, and any load or store that has a data dependency on the + value returned by this load won't be reordered before this load. This is also similar to rcu_dereference(), but in cases where object lifetime is handled by some mechanism other than RCU, for Regards, Boqun> > 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 >-------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 473 bytes Desc: not available URL: <http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20160127/e2ee2923/attachment.sig>
On Wed, Jan 27, 2016 at 10:04:47AM +0800, Boqun Feng wrote:> On Tue, Jan 26, 2016 at 03:29:21PM -0800, Paul E. McKenney wrote: > > 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(). > > > > Because semantically speaking, rcu_derefence*() and > lockless_dereference() are CONSUME(i.e. data/address dependent > read->read and read->write pairs are ordered), whereas > smp_read_barrier_depends() only guarantees read->read pairs with data > dependency are ordered, right? > > If so, maybe we need to call it out in memory-barriers.txt, for example: > > diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt > index 904ee42..6b262c2 100644 > --- a/Documentation/memory-barriers.txt > +++ b/Documentation/memory-barriers.txt > @@ -1703,8 +1703,8 @@ There are some more advanced barrier functions: > > > (*) lockless_dereference(); > - This can be thought of as a pointer-fetch wrapper around the > - smp_read_barrier_depends() data-dependency barrier. > + This is a load, and any load or store that has a data dependency on the > + value returned by this load won't be reordered before this load.This is a good start, but more is needed to warn people off of smp_read_barrier_depends(). But yes, better explanation would be good. Thanx, Paul> This is also similar to rcu_dereference(), but in cases where > object lifetime is handled by some mechanism other than RCU, for > > > Regards, > Boqun > > > > 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 > >