lkml.org 
[lkml]   [2013]   [Nov]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 3/4] MCS Lock: Barrier corrections
On Tue, Nov 05, 2013 at 10:18:03PM +0100, Peter Zijlstra wrote:
> On Tue, Nov 05, 2013 at 11:21:57AM -0800, Tim Chen wrote:
> > On Tue, 2013-11-05 at 18:37 +0000, Will Deacon wrote:
> > > 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)...
>
> PaulMck completely confused me a few days ago with control dependencies
> etc.. Pretty much saying that C/C++ doesn't do those.

I remember that there was a subtlety here, but don't remember what it was...

And while I do remember reviewing this code, I don't find any evidence
that I gave my "Reviewed-by". Tim/Jason, if I fat-fingered this, please
forward that email back to me.

Thanx, Paul



\
 
 \ /
  Last update: 2013-11-06 16:21    [W:0.059 / U:0.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site