Messages in this thread | | | Date | Wed, 02 Oct 2013 15:32:19 -0400 | From | Waiman Long <> | Subject | Re: [PATCH v6 5/6] MCS Lock: Restructure the MCS lock defines and locking code into its own file |
| |
On 10/02/2013 02:43 PM, Tim Chen wrote: > On Tue, 2013-10-01 at 21:25 -0400, Waiman Long wrote: >> >> If the lock and unlock functions are done right, there should be no >> overlap of critical section. So it is job of the lock/unlock functions >> to make sure that critical section code won't leak out. There should be >> some kind of memory barrier at the beginning of the lock function and >> the end of the unlock function. >> >> The critical section also likely to have branches. The CPU may >> speculatively execute code on the 2 branches, but one of them will be >> discarded once the branch condition is known. Also >> arch_mutex_cpu_relax() is a compiler barrier by itself. So we may not >> need a barrier() after all. The while statement is a branch instruction, >> any code after that can only be speculatively executed and cannot be >> committed until the branch is done. > But the condition code may be checked after speculative execution? > The condition may not be true during speculative execution and only > turns true when we check the condition, and take that branch? > > The thing that bothers me is without memory barrier after the while > statement, we could speculatively execute before affirming the lock is > in acquired state. Then when we check the lock, the lock is set > to acquired state in the mean time. > We could be loading some memory entry *before* > the node->locked has been set true. I think a smp_rmb (if not a > smp_mb) should be set after the while statement.
Yes, I think a smp_rmb() make sense here to correspond to the smp_wmb() in the unlock path.
BTW, you need to move the node->locked = 0; statement before xchg() if you haven't done so.
-Longman
| |