On Mon, Oct 27, 2014 at 05:28:01PM +0000, Al Viro wrote: > On Mon, Oct 27, 2014 at 08:59:01AM -0700, Paul E. McKenney wrote: > > > Indeed, life is hard here. Keep in mind that lock acquisition is not > > guaranteed to prevent prior operations from being reordered into the > > critical section, possibly as follows: > > > > CPU1: > > grab lock > > if (!global) > > global = p; > > /* Assume all of CPU2's accesses happen here. */ > > p->foo = 1; > > A bit of context: p is a local pointer to struct file here and alloc is > opening it... OK, good to know. ;-) > > This clearly allows CPU2 to execute as follows: > > > > CPU2: > > p = global; /* gets p */ > > if (p) /* non-NULL */ > > q = p->foo; /* might not be 1 */ > > > > Not only that, on DEC Alpha, even if CPU1's accesses are ordered, CPU2's > > accesses can be misordered. You need rcu_dereference() or the combination > > of ACCESS_ONCE() and smp_read_barrier_depends() to avoid this issue. > > As always, see http://www.openvms.compaq.com/wizard/wiz_2637.html for > > more info. > > > > So no, there is no guarantee. I am assuming that the lock grabbed by > > CPU1 guards all assignments to "global", otherwise the code needs further > > help. I am further assuming that the memory pointed to by CPU1's "p" > > is inaccessible to any other CPU, as in CPU1 just allocated the memory. > > Otherwise, the assignment "p->foo = 1" is questionable. And finally, > > I am assuming that p->foo stays constant once it has been made > > accessible to readers. > > > > But the following should work: > > > > CPU1: > > p->foo = 1; /* Assumes p is local. */ > > smp_mb__before_spinlock(); > > grab lock > > if (!global) /* Assumes lock protects all assignments to global. */ > > global = p; > > > > CPU2: > > p = rcu_dereference(global); > > if (p) > > q = p->foo; /* Assumes p->foo constant once visible to readers. */ > > /* Also assumes p and q are local. */ > > > > If the assumptions called out in the comments do not hold, you at least > > need ACCESS_ONCE(), and perhaps even more synchronization. For more info > > on ACCESS_ONCE(), Jon's LWN article is at http://lwn.net/Articles/508991/. > > First of all, this "p->foo = 1" is a shorthand for initialization of > struct file done by opening a file. What you are saying is that it > can leak past the point where we stick a pointer to freshly opened > file into a place where another CPU can see it, but not past the > barrier in dropping the lock, right? Exactly! I should also add that smp_mb__before_spinlock() implies smp_wmb(), but nothing more. But that is OK because smp_wmb() suffices in this case. > And you are suggesting rcu_dereference() as a way to bring the required > barriers in. Ouch... The names are really bad, but there's another > fun issue - rcu_dereference brings in sparse noise. Wouldn't direct use > of smp_read_barrier_depends() be cleaner, anyway? Code making direct use of smp_read_barrier_depends() is harder to read, in my experience, but good point on the sparse noise. Maybe a new lockless_dereference() primitive? Maybe something like the following? (Untested, probably does not even build.) #define lockless_dereference(p) \ ({ \ typeof(*p) *_________p1 = ACCESS_ONCE(p); \ smp_read_barrier_depends(); /* Dependency order vs. p above. */ \ _________p1; \ }) Thanx, Paul -- To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
