Messages in this thread | | | Subject | Re: [PATCH 4/6] locking/rwsem: Avoid deceiving lock spinners | From | Tim Chen <> | Date | Wed, 28 Jan 2015 09:01:33 -0800 |
| |
On Tue, 2015-01-27 at 19:54 -0800, Davidlohr Bueso wrote: > On Tue, 2015-01-27 at 09:23 -0800, Jason Low wrote: > > On Sun, 2015-01-25 at 23:36 -0800, Davidlohr Bueso wrote: > > > When readers hold the semaphore, the ->owner is nil. As such, > > > and unlike mutexes, '!owner' does not necessarily imply that > > > the lock is free. This will cause writer spinners to potentially > > > spin excessively as they've been mislead to thinking they have > > > a chance of acquiring the lock, instead of blocking. > > > > > > This patch therefore replaces this bogus check to solely rely on > > > the counter to know if the lock is available. Because we don't > > > hold the wait lock, we can obviously do this in an unqueued > > > manner. > > > > > > Signed-off-by: Davidlohr Bueso <dbueso@suse.de> > > > --- > > > kernel/locking/rwsem-xadd.c | 8 ++++++-- > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c > > > index 5e425d8..18a50da 100644 > > > --- a/kernel/locking/rwsem-xadd.c > > > +++ b/kernel/locking/rwsem-xadd.c > > > @@ -335,6 +335,8 @@ static inline bool owner_running(struct rw_semaphore *sem, > > > static noinline > > > bool rwsem_spin_on_owner(struct rw_semaphore *sem, struct task_struct *owner) > > > { > > > + long count; > > > + > > > rcu_read_lock(); > > > while (owner_running(sem, owner)) { > > > if (need_resched()) > > > @@ -347,9 +349,11 @@ bool rwsem_spin_on_owner(struct rw_semaphore *sem, struct task_struct *owner) > > > /* > > > * We break out the loop above on need_resched() or when the > > > * owner changed, which is a sign for heavy contention. Return > > > - * success only when sem->owner is NULL. > > > + * success only when the lock is available in order to attempt > > > + * another trylock. > > > */ > > > - return sem->owner == NULL; > > > + count = READ_ONCE(sem->count); > > > + return count == 0 || count == RWSEM_WAITING_BIAS; > > > > If we clear the owner field right before unlocking, would this cause > > some situations where we spin until the owner is cleared (about to > > release the lock), and then the spinner return false from > > rwsem_spin_on_owner? > > I'm not sure I understand your concern ;) could you rephrase that? > > So I think you're referring to the window between when we 1) clear the > ->owner and 2) update the ->counter in the unlocking paths. That would > lead the function to break out of the loop ("owner changed") and return > a bogus "sem is locked, thus taken by a new owner now, continue > spinning" reason for it (counter !=0 yet, for example). > > And that's perfectly fine, really. We've never held a strict > owner-counter dependency, and the owner pointer is completely > unreliable. So all this would end up doing is causing us to perform an > extra iteration per race. This is a pretty good tradeoff for what the > patch addresses.
I agree. The counter is a more accurate and immediate indicator of whether the lock is available, which is what we want to find out here.
Tim
| |