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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 06 Nov 2013 10:22:13 -0800
From:	Tim Chen <tim.c.chen@...ux.intel.com>
To:	paulmck@...ux.vnet.ibm.com
Cc:	Peter Zijlstra <peterz@...radead.org>,
	Will Deacon <will.deacon@....com>, Ingo Molnar <mingo@...e.hu>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	linux-mm <linux-mm@...ck.org>,
	"linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Waiman Long <waiman.long@...com>,
	Andrea Arcangeli <aarcange@...hat.com>,
	Alex Shi <alex.shi@...aro.org>,
	Andi Kleen <andi@...stfloor.org>,
	Michel Lespinasse <walken@...gle.com>,
	Davidlohr Bueso <davidlohr.bueso@...com>,
	Matthew R Wilcox <matthew.r.wilcox@...el.com>,
	Dave Hansen <dave.hansen@...el.com>,
	Rik van Riel <riel@...hat.com>,
	Peter Hurley <peter@...leysoftware.com>,
	Raghavendra K T <raghavendra.kt@...ux.vnet.ibm.com>,
	George Spelvin <linux@...izon.com>,
	"H. Peter Anvin" <hpa@...or.com>, Arnd Bergmann <arnd@...db.de>,
	Aswin Chandramouleeswaran <aswin@...com>,
	Scott J Norton <scott.norton@...com>
Subject: Re: [PATCH v2 3/4] MCS Lock: Barrier corrections

On Wed, 2013-11-06 at 06:45 -0800, Paul E. McKenney wrote:
> 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@...ux.vnet.ibm.com>
> > > > > Reviewed-by: Tim Chen <tim.c.chen@...ux.intel.com>
> > > > > Signed-off-by: Jason Low <jason.low2@...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.

Yes Paul, you didn't explicitly gave the Reviewed-by. 
I put it in there because you have given valuable
comments on the potential critical section bleeding when 
reviewing initial version of the code.

I'll take it out now till you have explicitly given it.
Appreciate if you can provide your feedback on the current
version of code.

Thanks.

Tim

> 
> 							Thanx, Paul
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/