Re: [PATCH v5 4/4] MCS Lock: Barrier corrections

[Waiman Long]

On 11/11/2013 04:17 PM, Tim Chen wrote:
> > You could then augment that with [cmp]xchg_{acquire,release} as
> > appropriate.
> >
> > > +/*
> > >    * In order to acquire the lock, the caller should declare a local node and
> > >    * pass a reference of the node to this function in addition to the lock.
> > >    * If the lock has already been acquired, then this will proceed to spin
> > > @@ -37,15 +62,19 @@ void mcs_spin_lock(struct mcs_spinlock **lock, struct mcs_spinlock *node)
> > >   	node->locked = 0;
> > >   	node->next   = NULL;
> > >
> > > -	prev = xchg(lock, node);
> > > +	/* xchg() provides a memory barrier */
> > > +	prev = xchg_acquire(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))
> > > +	/*
> > > +	 * Wait until the lock holder passes the lock down.
> > > +	 * Using smp_load_acquire() provides a memory barrier that
> > > +	 * ensures subsequent operations happen after the lock is acquired.
> > > +	 */
> > > +	while (!(smp_load_acquire(&node->locked)))
> > >   		arch_mutex_cpu_relax();
> An alternate implementation is
> 	while (!ACCESS_ONCE(node->locked))
> 		arch_mutex_cpu_relax();
> 	smp_load_acquire(&node->locked);
>
> Leaving the smp_load_acquire at the end to provide appropriate barrier.
> Will that be acceptable?
>
> Tim

I second Tim's opinion. It will be help to have a smp_mb_load_acquire() 
function that provide a memory barrier with load-acquire semantic. I 
don't think we need one for store-release as that will not be in a loop.

Peter, what do you think about adding that to your patch?

-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/