From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751415AbcEII1q (ORCPT ); Mon, 9 May 2016 04:27:46 -0400 Received: from bombadil.infradead.org ([198.137.202.9]:43746 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750919AbcEII1p (ORCPT ); Mon, 9 May 2016 04:27:45 -0400 Date: Mon, 9 May 2016 10:27:41 +0200 From: Peter Zijlstra To: Waiman Long Cc: Ingo Molnar , linux-kernel@vger.kernel.org, Davidlohr Bueso , Jason Low , Dave Chinner , Scott J Norton , Douglas Hatch Subject: Re: [PATCH v2] locking/rwsem: Add reader-owned state to the owner field Message-ID: <20160509082741.GF3430@twins.programming.kicks-ass.net> References: <1462580424-40333-1-git-send-email-Waiman.Long@hpe.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1462580424-40333-1-git-send-email-Waiman.Long@hpe.com> 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 On Fri, May 06, 2016 at 08:20:24PM -0400, Waiman Long wrote: > @@ -391,9 +386,11 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem) > * When there's no owner, we might have preempted between the > * owner acquiring the lock and setting the owner field. If > * we're an RT task that will live-lock because we won't let > + * the owner complete. We also quit if the lock is owned by > + * readers. Maybe also note why we quit on readers. > */ > + if (rwsem_is_reader_owned(owner) || > + (!owner && (need_resched() || rt_task(current)))) > break; > > /* > diff --git a/kernel/locking/rwsem.h b/kernel/locking/rwsem.h > index 870ed9a..d7fea18 100644 > --- a/kernel/locking/rwsem.h > +++ b/kernel/locking/rwsem.h > @@ -1,3 +1,20 @@ > +/* > + * The owner field of the rw_semaphore structure will be set to > + * RWSEM_READ_OWNED when a reader grabs the lock. A writer will clear > + * the owner field when it unlocks. A reader, on the other hand, will > + * not touch the owner field when it unlocks. > + * > + * In essence, the owner field now has the following 3 states: > + * 1) 0 > + * - lock is free or the owner hasn't set the field yet > + * 2) RWSEM_READER_OWNED > + * - lock is currently or previously owned by readers (lock is free > + * or not set by owner yet) > + * 3) Other non-zero value > + * - a writer owns the lock > + */ > +#define RWSEM_READER_OWNED 1UL #define RWSEM_READER_OWNED ((struct task_struct *)1UL) > + > #ifdef CONFIG_RWSEM_SPIN_ON_OWNER > static inline void rwsem_set_owner(struct rw_semaphore *sem) > { > @@ -9,6 +26,26 @@ static inline void rwsem_clear_owner(struct rw_semaphore *sem) > sem->owner = NULL; > } > > +static inline void rwsem_set_reader_owned(struct rw_semaphore *sem) > +{ > + /* > + * We check the owner value first to make sure that we will only > + * do a write to the rwsem cacheline when it is really necessary > + * to minimize cacheline contention. > + */ > + if (sem->owner != (struct task_struct *)RWSEM_READER_OWNED) > + sem->owner = (struct task_struct *)RWSEM_READER_OWNED; How much if anything did this optimization matter? > +} > + > +static inline bool rwsem_is_writer_owned(struct task_struct *owner) > +{ > + return (unsigned long)owner > RWSEM_READER_OWNED; > +} Tad too clever that; what does GCC generate if you write the obvious: return owner && owner != RWSEM_READER_OWNER; > + > +static inline bool rwsem_is_reader_owned(struct task_struct *owner) > +{ > + return owner == (struct task_struct *)RWSEM_READER_OWNED; > +} So I don't particularly like these names; they read like they take a rwsem as argument, but they don't. Would something like: rwsem_owner_is_{reader,writer}() make more sense? > #else > static inline void rwsem_set_owner(struct rw_semaphore *sem) > {