Re: [PATCH v3] locking/rwsem: reduce spinlock contention in wakeup after up_read/up_write

[Waiman Long <waiman.long@hp.com>]

On 04/30/2015 10:12 AM, Davidlohr Bueso wrote:
> On Wed, 2015-04-29 at 15:58 -0400, Waiman Long wrote:
>> A write lock can also be acquired by a spinning writer in
>> rwsem_try_write_lock_unqueued() where wait_lock isn't used. With
>> multiple down_read's, it is possible that the first exiting reader wakes
>> up a writer who acquires the write lock while the other readers are
>> waiting for acquiring the wait_lock.
> Except that readers that do the wakeup do not call __rwsem_do_wake() if
> there is an active writer:
>
> 	/* If there are no active locks, wake the front queued process(es).
> 	 *
> 	 * If there are no writers and we are first in the queue,
> 	 * wake our own waiter to join the existing active readers !
> 	 */
> 	if (count == RWSEM_WAITING_BIAS ||
> 	    (count>  RWSEM_WAITING_BIAS&&
> 	adjustment != -RWSEM_ACTIVE_READ_BIAS))
> 		sem = __rwsem_do_wake(sem, RWSEM_WAKE_ANY);
>
> And for the reader part, your rwsem_has_active_writer() check, while
> avoiding the counter atomic update, would break current semantics in
> that we still do the next reader grant -- also note the unlikely()
> predictor ;) And this is done with the counter, so by using the owner,
> you would have a race between the cmpxchg and rwsem_set_owner(). Yes,
> its a small window (specially after commit 7a215f89), but there
> nonetheless and could cause even more bogus wakeups. Again, I simply do
> not like mixing these two -- we get away with it with the optimistic
> spinning because we just iterate again, but this is not the case.

I think I know what you are talking about. However, I don't think it is 
really a race condition. In up_write():

         rwsem_clear_owner(sem);
         __up_write(sem);

The owner field is cleared before the lock holding writer decrement the 
count. So there is simply no way a reader will infer from the count that 
there is no writer in the rwsem while the owner field is still set 
unless it has just been stolen by another writer. In this case, 
__rwsem_do_wake() will have to back out from the reader grant anyway. It 
is possible that the second writer release the lock just before 
__rwsem_do_wake() is called, perhaps an interrupt happen. In this case, 
2nd writer will be waiting for the wait_lock in rwsem_wake() to do the 
wakeup duty. So there should be no missed wakeup.

If you are still worrying about this, I can put the owner check in 
rwsem_wake() function after taking the wait_lock instead. That should 
remove any concern that you have.

> Ultimately, is this really an issue? Do you have numbers that could
> justify such a change? I suspect all the benchmark results you posted in
> the patch are from reducing the spinlock contention, not from this.

This is true. This change has no statistically significant contribution 
(just a tiny bit, perhaps) for improving benchmark results.

Cheers,
Longman