Re: [PATCH v6 5/6] MCS Lock: Restructure the MCS lock defines andlocking code into its own file
From: Waiman Long
Date: Wed Oct 02 2013 - 15:32:40 EST
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/