On Tue, Jan 26, 2016 at 9:22 AM, Peter Zijlstra <peterz at infradead.org> wrote:> > This is distinct from:That may be distinct, but:> 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. It's called "smp_READ_barrier_depends()" for a reason. Alpha is the only one that needs it, and alpha needs it only for dependent READS. It's not called smp_read_write_barrier_depends(). It's not called "smp_mb_depends()". It's a weaker form of "smp_rmb()", nothing else. So alpha does have an implied dependency chain from a read to a subsequent dependent write, and does not need any extra barriers. Alpha does *not* have a dependency chain from a read to a subsequent read, which is why we need that horrible crappy smp_read_barrier_depends(). But it's the only reason. This is the alpha reference manual wrt read-to-write dependency: 5.6.1.7 Definition of Dependence Constraint The depends relation (DP) is defined as follows. Given u and v issued by processor Pi, where u is a read or an instruction fetch and v is a write, u precedes v in DP order (written u DP v, that is, v depends on u) in either of the following situations: ? u determines the execution of v, the location accessed by v, or the value written by v. ? u determines the execution or address or value of another memory access z that precedes v or might precede v (that is, would precede v in some execution path depending on the value read by u) by processor issue constraint (see Section 5.6.1.3). Note that the dependence barrier honors not only control flow, but address and data values too. This is a different syntax than we use, but 'u' is the READ_ONCE, and 'v' is the write. Any data, address or conditional dependency between the two implies an ordering. So no, "smp_read_barrier_depends()" is *ONLY* about two reads, where the second read is data-dependent on the first. Nothing else. So if you _ever_ see a "smp_read_barrier_depends()" that isn't about a barrier between two reads, then that is a bug. The above code is crap. It's exactly as much crap as a = READ_ONCE(x); smp_rmb(); WRITE_ONCE(b, y); because a "rmb()" simply doesn't have anything to do with read-vs-subsequent-write ordering. Linus
On Tue, Jan 26, 2016 at 11:44:46AM -0800, Linus Torvalds wrote:> On Tue, Jan 26, 2016 at 9:22 AM, Peter Zijlstra <peterz at infradead.org> wrote: > > > > This is distinct from: > > That may be distinct, but: > > > 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?> It's called "smp_READ_barrier_depends()" for a reason. > > Alpha is the only one that needs it, and alpha needs it only for > dependent READS. > > It's not called smp_read_write_barrier_depends(). It's not called > "smp_mb_depends()". It's a weaker form of "smp_rmb()", nothing else. > > So alpha does have an implied dependency chain from a read to a > subsequent dependent write, and does not need any extra barriers. > > Alpha does *not* have a dependency chain from a read to a subsequent > read, which is why we need that horrible crappy > smp_read_barrier_depends(). But it's the only reason. > > This is the alpha reference manual wrt read-to-write dependency: > > 5.6.1.7 Definition of Dependence Constraint > > The depends relation (DP) is defined as follows. Given u and v > issued by processor Pi, where u > is a read or an instruction fetch and v is a write, u precedes v > in DP order (written u DP v, that > is, v depends on u) in either of the following situations: > > ? u determines the execution of v, the location accessed by v, or > the value written by v. > ? u determines the execution or address or value of another > memory access z that precedes > > v or might precede v (that is, would precede v in some execution > path depending > on the value read by u) by processor issue constraint (see Section 5.6.1.3). > > Note that the dependence barrier honors not only control flow, but > address and data values too. This is a different syntax than we use, > but 'u' is the READ_ONCE, and 'v' is the write. Any data, address or > conditional dependency between the two implies an ordering. > > So no, "smp_read_barrier_depends()" is *ONLY* about two reads, where > the second read is data-dependent on the first. Nothing else. > > So if you _ever_ see a "smp_read_barrier_depends()" that isn't about a > barrier between two reads, then that is a bug.And the smp_read_barrier_depends() in both rcu_dereference() and in lockless_dereference() is ordering the read-to-read case and the underlying hardware is ordering the read-to-write case on weakly ordered hardware. Or, again, am I missing your point? Thanx, Paul> The above code is crap. It's exactly as much crap as > > a = READ_ONCE(x); > smp_rmb(); > WRITE_ONCE(b, y); > > because a "rmb()" simply doesn't have anything to do with > read-vs-subsequent-write ordering. > > Linus >
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