Re: [PATCH v2 3/4] MCS Lock: Barrier corrections

[Will Deacon <will.deacon@arm.com>]

On Tue, Nov 05, 2013 at 05:42:36PM +0000, Tim Chen wrote:
> This patch corrects the way memory barriers are used in the MCS lock
> and removes ones that are not needed. Also add comments on all barriers.

Hmm, I see that you're fixing up the barriers, but I still don't completely
understand how what you have is correct. Hopefully you can help me out :)

> Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Reviewed-by: Tim Chen <tim.c.chen@linux.intel.com>
> Signed-off-by: Jason Low <jason.low2@hp.com>
> ---
>  include/linux/mcs_spinlock.h |   13 +++++++++++--
>  1 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/mcs_spinlock.h b/include/linux/mcs_spinlock.h
> index 96f14299..93d445d 100644
> --- a/include/linux/mcs_spinlock.h
> +++ b/include/linux/mcs_spinlock.h
> @@ -36,16 +36,19 @@ void mcs_spin_lock(struct mcs_spinlock **lock, struct mcs_spinlock *node)
>  	node->locked = 0;
>  	node->next   = NULL;
>  
> +	/* xchg() provides a memory barrier */
>  	prev = xchg(lock, node);
>  	if (likely(prev == NULL)) {
>  		/* Lock acquired */
>  		return;
>  	}
>  	ACCESS_ONCE(prev->next) = node;
> -	smp_wmb();
>  	/* Wait until the lock holder passes the lock down */
>  	while (!ACCESS_ONCE(node->locked))
>  		arch_mutex_cpu_relax();
> +
> +	/* Make sure subsequent operations happen after the lock is acquired */
> +	smp_rmb();

Ok, so this is an smp_rmb() because we assume that stores aren't speculated,
right? (i.e. the control dependency above is enough for stores to be ordered
with respect to taking the lock)...

>  }
>  
>  /*
> @@ -58,6 +61,7 @@ static void mcs_spin_unlock(struct mcs_spinlock **lock, struct mcs_spinlock *nod
>  
>  	if (likely(!next)) {
>  		/*
> +		 * cmpxchg() provides a memory barrier.
>  		 * Release the lock by setting it to NULL
>  		 */
>  		if (likely(cmpxchg(lock, node, NULL) == node))
> @@ -65,9 +69,14 @@ static void mcs_spin_unlock(struct mcs_spinlock **lock, struct mcs_spinlock *nod
>  		/* Wait until the next pointer is set */
>  		while (!(next = ACCESS_ONCE(node->next)))
>  			arch_mutex_cpu_relax();
> +	} else {
> +		/*
> +		 * Make sure all operations within the critical section
> +		 * happen before the lock is released.
> +		 */
> +		smp_wmb();

...but I don't see what prevents reads inside the critical section from
moving across the smp_wmb() here.

What am I missing?

Will

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>