Messages in this thread | | | Date | Mon, 30 Sep 2013 11:51:47 -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 09/28/2013 12:34 AM, Jason Low wrote: >> Also, below is what the mcs_spin_lock() and mcs_spin_unlock() >> functions would look like after applying the proposed changes. >> >> static noinline >> void mcs_spin_lock(struct mcs_spin_node **lock, struct mcs_spin_node *node) >> { >> struct mcs_spin_node *prev; >> >> /* Init node */ >> node->locked = 0; >> node->next = NULL; >> >> prev = xchg(lock, node); >> if (likely(prev == NULL)) { >> /* Lock acquired. No need to set node->locked since it >> won't be used */ >> return; >> } >> ACCESS_ONCE(prev->next) = node; >> /* Wait until the lock holder passes the lock down */ >> while (!ACCESS_ONCE(node->locked)) >> arch_mutex_cpu_relax(); >> smp_mb();
I wonder if a memory barrier is really needed here.
>> } >> >> static void mcs_spin_unlock(struct mcs_spin_node **lock, struct >> mcs_spin_node *node) >> { >> struct mcs_spin_node *next = ACCESS_ONCE(node->next); >> >> if (likely(!next)) { >> /* >> * Release the lock by setting it to NULL >> */ >> if (cmpxchg(lock, node, NULL) == node) >> return; >> /* Wait until the next pointer is set */ >> while (!(next = ACCESS_ONCE(node->next))) >> arch_mutex_cpu_relax(); >> } >> smp_wmb(); >> ACCESS_ONCE(next->locked) = 1; >> }
Instead, I think what we need may be:
if (likely(!next)) { .... } else smp_mb(); ACCESS_ONCE(next->locked) = 1;
That will ensure a memory barrier in the unlock path.
Regards, Longman
| |