From mboxrd@z Thu Jan 1 00:00:00 1970 From: Waiman Long Subject: Re: [PATCH v10 06/19] qspinlock: prolong the stay in the pending bit path Date: Fri, 09 May 2014 20:58:47 -0400 Message-ID: <536D79C7.2000701@hp.com> References: <1399474907-22206-1-git-send-email-Waiman.Long@hp.com> <1399474907-22206-7-git-send-email-Waiman.Long@hp.com> <20140508185854.GN2844@laptop.programming.kicks-ass.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20140508185854.GN2844@laptop.programming.kicks-ass.net> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: virtualization-bounces@lists.linux-foundation.org Errors-To: virtualization-bounces@lists.linux-foundation.org To: Peter Zijlstra Cc: linux-arch@vger.kernel.org, Rik van Riel , Raghavendra K T , Oleg Nesterov , Gleb Natapov , kvm@vger.kernel.org, Konrad Rzeszutek Wilk , Scott J Norton , x86@kernel.org, Paolo Bonzini , linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, Ingo Molnar , Chegu Vinod , David Vrabel , "H. Peter Anvin" , xen-devel@lists.xenproject.org, Thomas Gleixner , "Paul E. McKenney" , Linus Torvalds , Boris Ostrovsky List-Id: linux-arch.vger.kernel.org On 05/08/2014 02:58 PM, Peter Zijlstra wrote: > On Wed, May 07, 2014 at 11:01:34AM -0400, Waiman Long wrote: >> @@ -221,11 +222,37 @@ static inline int trylock_pending(struct qspinlock *lock, u32 *pval) >> */ >> for (;;) { >> /* >> - * If we observe any contention; queue. >> + * If we observe that the queue is not empty, >> + * return and be queued. >> */ >> - if (val& ~_Q_LOCKED_MASK) >> + if (val& _Q_TAIL_MASK) >> return 0; >> >> + if (val == (_Q_LOCKED_VAL|_Q_PENDING_VAL)) { >> + /* >> + * If both the lock and pending bits are set, we wait >> + * a while to see if that either bit will be cleared. >> + * If that is no change, we return and be queued. >> + */ >> + if (!retry) >> + return 0; >> + retry--; >> + cpu_relax(); >> + cpu_relax(); >> + *pval = val = atomic_read(&lock->val); >> + continue; >> + } else if (val == _Q_PENDING_VAL) { >> + /* >> + * Pending bit is set, but not the lock bit. >> + * Assuming that the pending bit holder is going to >> + * set the lock bit and clear the pending bit soon, >> + * it is better to wait than to exit at this point. >> + */ >> + cpu_relax(); >> + *pval = val = atomic_read(&lock->val); >> + continue; >> + } > Didn't I give a much saner alternative to this mess last time? I don't recall you have any suggestion last time. Anyway, if you think the code is too messy, I think I can give up the first if statement which is more an optimistic spinning kind of code for short critical section. The 2nd if statement is still need to improve chance of using this code path due to timing reason. I will rerun my performance test to make sure it won't have too much performance impact. -Longman From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from g4t3425.houston.hp.com ([15.201.208.53]:49630 "EHLO g4t3425.houston.hp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932328AbaEJA7M (ORCPT ); Fri, 9 May 2014 20:59:12 -0400 Message-ID: <536D79C7.2000701@hp.com> Date: Fri, 09 May 2014 20:58:47 -0400 From: Waiman Long MIME-Version: 1.0 Subject: Re: [PATCH v10 06/19] qspinlock: prolong the stay in the pending bit path References: <1399474907-22206-1-git-send-email-Waiman.Long@hp.com> <1399474907-22206-7-git-send-email-Waiman.Long@hp.com> <20140508185854.GN2844@laptop.programming.kicks-ass.net> In-Reply-To: <20140508185854.GN2844@laptop.programming.kicks-ass.net> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-arch-owner@vger.kernel.org List-ID: To: Peter Zijlstra Cc: Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , linux-arch@vger.kernel.org, x86@kernel.org, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, xen-devel@lists.xenproject.org, kvm@vger.kernel.org, Paolo Bonzini , Konrad Rzeszutek Wilk , Boris Ostrovsky , "Paul E. McKenney" , Rik van Riel , Linus Torvalds , Raghavendra K T , David Vrabel , Oleg Nesterov , Gleb Natapov , Scott J Norton , Chegu Vinod Message-ID: <20140510005847.z3_Xg2Hgo1FHMxI94b5UFDVs_cyjJeVbAu1s4TpXuxM@z> On 05/08/2014 02:58 PM, Peter Zijlstra wrote: > On Wed, May 07, 2014 at 11:01:34AM -0400, Waiman Long wrote: >> @@ -221,11 +222,37 @@ static inline int trylock_pending(struct qspinlock *lock, u32 *pval) >> */ >> for (;;) { >> /* >> - * If we observe any contention; queue. >> + * If we observe that the queue is not empty, >> + * return and be queued. >> */ >> - if (val& ~_Q_LOCKED_MASK) >> + if (val& _Q_TAIL_MASK) >> return 0; >> >> + if (val == (_Q_LOCKED_VAL|_Q_PENDING_VAL)) { >> + /* >> + * If both the lock and pending bits are set, we wait >> + * a while to see if that either bit will be cleared. >> + * If that is no change, we return and be queued. >> + */ >> + if (!retry) >> + return 0; >> + retry--; >> + cpu_relax(); >> + cpu_relax(); >> + *pval = val = atomic_read(&lock->val); >> + continue; >> + } else if (val == _Q_PENDING_VAL) { >> + /* >> + * Pending bit is set, but not the lock bit. >> + * Assuming that the pending bit holder is going to >> + * set the lock bit and clear the pending bit soon, >> + * it is better to wait than to exit at this point. >> + */ >> + cpu_relax(); >> + *pval = val = atomic_read(&lock->val); >> + continue; >> + } > Didn't I give a much saner alternative to this mess last time? I don't recall you have any suggestion last time. Anyway, if you think the code is too messy, I think I can give up the first if statement which is more an optimistic spinning kind of code for short critical section. The 2nd if statement is still need to improve chance of using this code path due to timing reason. I will rerun my performance test to make sure it won't have too much performance impact. -Longman