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... > 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? 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? -- 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
