From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933042AbcBAOhc (ORCPT ); Mon, 1 Feb 2016 09:37:32 -0500 Received: from bombadil.infradead.org ([198.137.202.9]:32810 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932634AbcBAOha (ORCPT ); Mon, 1 Feb 2016 09:37:30 -0500 Date: Mon, 1 Feb 2016 15:37:24 +0100 From: Peter Zijlstra To: Will Deacon , Paul McKenney Cc: linux-kernel@vger.kernel.org, Davidlohr Bueso , Ingo Molnar , parri.andrea@gmail.com Subject: [RFC][PATCH] locking/mcs: Fix ordering for mcs_spin_lock() Message-ID: <20160201143724.GW6357@twins.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.5.21 (2012-12-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Given the below patch; we've now got an unconditional full global barrier in, does this make the MCS spinlock RCsc ? The 'problem' is that this barrier can happen before we actually acquire the lock. That is, if we hit arch_mcs_spin_lock_contended() _that_ will be the acquire barrier and we end up with a SYNC in between unlock and lock -- ie. not an smp_mb__after_unlock_lock() equivalent. --- Subject: locking/mcs: Fix ordering for mcs_spin_lock() From: Peter Zijlstra Date: Mon Feb 1 15:11:28 CET 2016 Similar to commit b4b29f94856a ("locking/osq: Fix ordering of node initialisation in osq_lock") the use of xchg_acquire() is fundamentally broken with MCS like constructs. Furthermore, it turns out we rely on the global transitivity of this operation because the unlock path observes the pointer with a READ_ONCE(), not an smp_load_acquire(). This is non-critical because the MCS code isn't actually used and mostly serves as documentation, a stepping stone to the more complex things we've build on top of the idea. Cc: Will Deacon Cc: "Paul E. McKenney" Reported-by: Andrea Parri Fixes: 3552a07a9c4a ("locking/mcs: Use acquire/release semantics") Signed-off-by: Peter Zijlstra (Intel) --- kernel/locking/mcs_spinlock.h | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) --- a/kernel/locking/mcs_spinlock.h +++ b/kernel/locking/mcs_spinlock.h @@ -67,7 +67,13 @@ void mcs_spin_lock(struct mcs_spinlock * node->locked = 0; node->next = NULL; - prev = xchg_acquire(lock, node); + /* + * We rely on the full barrier with global transitivity implied by the + * below xchg() to order the initialization stores above against any + * observation of @node. And to provide the ACQUIRE ordering associated + * with a LOCK primitive. + */ + prev = xchg(lock, node); if (likely(prev == NULL)) { /* * Lock acquired, don't need to set node->locked to 1. Threads