On Wed, May 11, 2016 at 09:23:57AM +0200, Michal Hocko wrote:
> On Tue 10-05-16 14:38:06, Peter Zijlstra wrote:
> > Also, looking at it again; I think we're forgetting to re-adjust the
> > BIAS for the cancelled writer.
>
> Hmm, __rwsem_down_write_failed_common does
>
> /* undo write bias from down_write operation, stop active locking */
> count = rwsem_atomic_update(-RWSEM_ACTIVE_WRITE_BIAS, sem);
>
> which should remove the bias AFAIU.
Right; at this point we're neutral wrt bias.
> Later we do
>
> if (waiting) {
> count = READ_ONCE(sem->count);
>
> /*
> * If there were already threads queued before us and there are
> * no active writers, the lock must be read owned; so we try to
> * wake any read locks that were queued ahead of us.
> */
> if (count > RWSEM_WAITING_BIAS)
> sem = __rwsem_do_wake(sem, RWSEM_WAKE_READERS);
>
> } else
> count = rwsem_atomic_update(RWSEM_WAITING_BIAS, sem);
>
> and that might set RWSEM_WAITING_BIAS but the current holder of the lock
> should handle that correctly and wake the waiting tasks IIUC. I will go
> and check the code closer. It is quite easy to get this subtle code
> wrong..
Subtle; yes.
So if you look at rwsem_try_write_lock() -- traditionally the only way
to exit this wait loop, you see it does:
if (count == RWSEM_WAITING_BIAS &&
cmpxchg_acquire(&sem->count, RWSEM_WAITING_BIAS,
RWSEM_ACTIVE_WRITE_BIAS) == RWSEM_WAITING_BIAS) {
if (!list_is_singular(&sem->wait_list))
rwsem_atomic_update(RWSEM_WAITING_BIAS, sem);
rwsem_set_owner(sem);
return true;
}
Which ends up clearing RWSEM_WAITING_BIAS is we were the only waiter --
or rather, it always clear WAITING, but then tests the list and re-sets
it if there's more than one waiters on.
Now, the signal break doesn't clear WAITING if we were the only waiter
on the list; which means any further down_read() will block (I didn't
look at what a subsequent down_write() would do).
So I think we needs something like this, to clear WAITING if we leave
the list empty.
Does that make sense?
diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index df4dcb883b50..7011dd1c286c 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -489,6 +489,8 @@ __rwsem_down_write_failed_common(struct rw_semaphore *sem,
int state)
do {
if (signal_pending_state(state, current)) {
raw_spin_lock_irq(&sem->wait_lock);
+ if (list_singular(&sem->wait_list))
+
rwsem_atomic_update(-RWSEM_WAITING_BIAS, sem);
ret = ERR_PTR(-EINTR);
goto out;
}
