Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754452Ab3I0SJX (ORCPT ); Fri, 27 Sep 2013 14:09:23 -0400 Received: from mga02.intel.com ([134.134.136.20]:7814 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754098Ab3I0SJT (ORCPT ); Fri, 27 Sep 2013 14:09:19 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.90,994,1371106800"; d="scan'208";a="410528296" Subject: Re: [PATCH v6 5/6] MCS Lock: Restructure the MCS lock defines and locking code into its own file From: Tim Chen To: paulmck@linux.vnet.ibm.com, Waiman Long Cc: Ingo Molnar , Andrew Morton , Andrea Arcangeli , Alex Shi , Andi Kleen , Michel Lespinasse , Davidlohr Bueso , Matthew R Wilcox , Dave Hansen , Peter Zijlstra , Rik van Riel , Peter Hurley , linux-kernel@vger.kernel.org, linux-mm In-Reply-To: <20130927152953.GA4464@linux.vnet.ibm.com> References: <1380147049.3467.67.camel@schen9-DESK> <20130927152953.GA4464@linux.vnet.ibm.com> Content-Type: text/plain; charset="UTF-8" Date: Fri, 27 Sep 2013 11:09:08 -0700 Message-ID: <1380305348.3467.109.camel@schen9-DESK> Mime-Version: 1.0 X-Mailer: Evolution 2.32.3 (2.32.3-1.fc14) Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3242 Lines: 95 On Fri, 2013-09-27 at 08:29 -0700, Paul E. McKenney wrote: > On Wed, Sep 25, 2013 at 03:10:49PM -0700, Tim Chen wrote: > > We will need the MCS lock code for doing optimistic spinning for rwsem. > > Extracting the MCS code from mutex.c and put into its own file allow us > > to reuse this code easily for rwsem. > > > > Signed-off-by: Tim Chen > > Signed-off-by: Davidlohr Bueso > > --- > > include/linux/mcslock.h | 58 +++++++++++++++++++++++++++++++++++++++++++++++ > > kernel/mutex.c | 58 +++++----------------------------------------- > > 2 files changed, 65 insertions(+), 51 deletions(-) > > create mode 100644 include/linux/mcslock.h > > > > diff --git a/include/linux/mcslock.h b/include/linux/mcslock.h > > new file mode 100644 > > index 0000000..20fd3f0 > > --- /dev/null > > +++ b/include/linux/mcslock.h > > @@ -0,0 +1,58 @@ > > +/* > > + * MCS lock defines > > + * > > + * This file contains the main data structure and API definitions of MCS lock. > > + */ > > +#ifndef __LINUX_MCSLOCK_H > > +#define __LINUX_MCSLOCK_H > > + > > +struct mcs_spin_node { > > + struct mcs_spin_node *next; > > + int locked; /* 1 if lock acquired */ > > +}; > > + > > +/* > > + * We don't inline mcs_spin_lock() so that perf can correctly account for the > > + * time spent in this lock function. > > + */ > > +static noinline > > +void mcs_spin_lock(struct mcs_spin_node **lock, struct mcs_spin_node *node) > > +{ > > + struct mcs_spin_node *prev; > > + > > + /* Init node */ > > + node->locked = 0; > > + node->next = NULL; > > + > > + prev = xchg(lock, node); > > + if (likely(prev == NULL)) { > > + /* Lock acquired */ > > + node->locked = 1; > > + 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(); > > +} > > + > > +static void mcs_spin_unlock(struct mcs_spin_node **lock, struct mcs_spin_node *node) > > +{ > > + struct mcs_spin_node *next = ACCESS_ONCE(node->next); > > + > > + if (likely(!next)) { > > + /* > > + * Release the lock by setting it to NULL > > + */ > > + if (cmpxchg(lock, node, NULL) == node) > > + return; > > + /* Wait until the next pointer is set */ > > + while (!(next = ACCESS_ONCE(node->next))) > > + arch_mutex_cpu_relax(); > > + } > > + ACCESS_ONCE(next->locked) = 1; > > + smp_wmb(); > > Shouldn't the memory barrier precede the "ACCESS_ONCE(next->locked) = 1;"? > Maybe in an "else" clause of the prior "if" statement, given that the > cmpxchg() does it otherwise. > > Otherwise, in the case where the "if" conditionn is false, the critical > section could bleed out past the unlock. Yes, I agree with you that the smp_wmb should be moved before ACCESS_ONCE to prevent critical section from bleeding. Copying Waiman who is the original author of the mcs code to see if he has any comments on things we may have missed. Tim -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/