Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752022Ab3FWBQZ (ORCPT ); Sat, 22 Jun 2013 21:16:25 -0400 Received: from mga09.intel.com ([134.134.136.24]:35990 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751775Ab3FWBQX (ORCPT ); Sat, 22 Jun 2013 21:16:23 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.87,921,1363158000"; d="scan'208";a="354166115" Message-ID: <51C64C5D.5090400@intel.com> Date: Sun, 23 Jun 2013 09:16:13 +0800 From: Alex Shi User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120912 Thunderbird/15.0.1 MIME-Version: 1.0 To: Peter Hurley CC: Tim Chen , Michel Lespinasse , Ingo Molnar , Andrew Morton , Andrea Arcangeli , Andi Kleen , Davidlohr Bueso , Matthew R Wilcox , Dave Hansen , Peter Zijlstra , Rik van Riel , linux-kernel@vger.kernel.org, linux-mm Subject: Re: [PATCH 1/2] rwsem: check the lock before cpmxchg in down_write_trylock and rwsem_do_wake References: <1371858695.22432.4.camel@schen9-DESK> <51C55082.5040500@hurleysoftware.com> In-Reply-To: <51C55082.5040500@hurleysoftware.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4526 Lines: 140 On 06/22/2013 03:21 PM, Peter Hurley wrote: > On 06/21/2013 07:51 PM, Tim Chen wrote: >> Doing cmpxchg will cause cache bouncing when checking >> sem->count. This could cause scalability issue >> in a large machine (e.g. a 80 cores box). >> >> A pre-read of sem->count can mitigate this. >> >> Signed-off-by: Alex Shi >> Signed-off-by: Tim Chen >> --- >> include/asm-generic/rwsem.h | 8 ++++---- >> lib/rwsem.c | 21 +++++++++++++-------- >> 2 files changed, 17 insertions(+), 12 deletions(-) >> >> diff --git a/include/asm-generic/rwsem.h b/include/asm-generic/rwsem.h >> index bb1e2cd..052d973 100644 >> --- a/include/asm-generic/rwsem.h >> +++ b/include/asm-generic/rwsem.h >> @@ -70,11 +70,11 @@ static inline void __down_write(struct >> rw_semaphore *sem) >> >> static inline int __down_write_trylock(struct rw_semaphore *sem) >> { >> - long tmp; >> + if (unlikely(&sem->count != RWSEM_UNLOCKED_VALUE)) > ^^^^^^^^^^^ > > This is probably not what you want. > this function logical is quite simple. check the sem->count before cmpxchg is no harm this logical. So could you like to tell us what should we want? > >> + return 0; >> >> - tmp = cmpxchg(&sem->count, RWSEM_UNLOCKED_VALUE, >> - RWSEM_ACTIVE_WRITE_BIAS); >> - return tmp == RWSEM_UNLOCKED_VALUE; >> + return cmpxchg(&sem->count, RWSEM_UNLOCKED_VALUE, >> + RWSEM_ACTIVE_WRITE_BIAS) == RWSEM_UNLOCKED_VALUE; >> } >> >> /* >> diff --git a/lib/rwsem.c b/lib/rwsem.c >> index 19c5fa9..2072af5 100644 >> --- a/lib/rwsem.c >> +++ b/lib/rwsem.c >> @@ -75,7 +75,7 @@ __rwsem_do_wake(struct rw_semaphore *sem, enum >> rwsem_wake_type wake_type) >> * will block as they will notice the queued writer. >> */ >> wake_up_process(waiter->task); >> - goto out; >> + return sem; > > Please put these flow control changes in a separate patch. I had sent the split patches to Tim&Davidlohr. They will send them out as a single patchset. > > >> } >> >> /* Writers might steal the lock before we grant it to the next >> reader. >> @@ -85,15 +85,21 @@ __rwsem_do_wake(struct rw_semaphore *sem, enum >> rwsem_wake_type wake_type) >> adjustment = 0; >> if (wake_type != RWSEM_WAKE_READ_OWNED) { >> adjustment = RWSEM_ACTIVE_READ_BIAS; >> - try_reader_grant: >> - oldcount = rwsem_atomic_update(adjustment, sem) - adjustment; >> - if (unlikely(oldcount < RWSEM_WAITING_BIAS)) { >> - /* A writer stole the lock. Undo our reader grant. */ >> + while (1) { >> + /* A writer stole the lock. */ >> + if (sem->count < RWSEM_WAITING_BIAS) >> + return sem; > > I'm all for structured looping instead of goto labels but this optimization > is only useful on the 1st iteration. IOW, on the second iteration you > already > know that you need to try for reclaiming the lock. > sorry. could you like to say more clear, what's the 1st or 2nd iteration or others? > >> + >> + oldcount = rwsem_atomic_update(adjustment, sem) >> + - adjustment; >> + if (likely(oldcount >= RWSEM_WAITING_BIAS)) >> + break; >> + >> + /* A writer stole the lock. Undo our reader grant. */ >> if (rwsem_atomic_update(-adjustment, sem) & >> RWSEM_ACTIVE_MASK) >> - goto out; >> + return sem; >> /* Last active locker left. Retry waking readers. */ >> - goto try_reader_grant; >> } >> } >> >> @@ -136,7 +142,6 @@ __rwsem_do_wake(struct rw_semaphore *sem, enum >> rwsem_wake_type wake_type) >> sem->wait_list.next = next; >> next->prev = &sem->wait_list; >> >> - out: >> return sem; >> } > > > Alex and Tim, > > Was there a v1 of this series; ie., is this v2 (or higher)? > > How are you validating lock correctness/behavior with this series? some benchmark tested against this patch, mainly aim7. plus by eyes, we didn't change the logical except check the lock value before do locking > > Regards, > Peter Hurley > -- Thanks Alex -- 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/