On Wed 11-05-16 09:23:57, Michal Hocko wrote:
> On Tue 10-05-16 14:38:06, Peter Zijlstra wrote:
[...]
> > However, at the time I was thinking that if we have:
> >
> > reader (owner)
> > writer (pending)
> > reader (blocked on writer)
> >
> > and writer would get cancelled, the up_read() would do a wakeup and kick
> > the blocked reader.
> >
> > But yes, immediately kicking further pending waiters might be better.
>
> OK, that makes sense. We shouldn't be waiting for the first reader to
> do up_read.
I am still trying to wrap my head around the wake up logic (I managed to
forget all the juicy details). I still do not see the correctness
issue with the current code so the following should be "just an
optimization". But as I've said I might be missing something here.
Does the following look correct/reasonable? This is absolutely untested
and more for a discussion:
---
diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index df4dcb883b50..726620c97366 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -516,7 +516,27 @@ EXPORT_SYMBOL(rwsem_down_write_failed);
__visible struct rw_semaphore * __sched
rwsem_down_write_failed_killable(struct rw_semaphore *sem)
{
- return __rwsem_down_write_failed_common(sem, TASK_KILLABLE);
+ struct rw_semaphore * ret;
+
+ ret = __rwsem_down_write_failed_common(sem, TASK_KILLABLE);
+ if (IS_ERR(ret)) {
+ long count;
+
+ raw_spin_lock_irq(&sem->wait_lock);
+ 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 so they
+ * do not have to wait for up_read to wake up.
+ */
+ if (count > RWSEM_WAITING_BIAS)
+ sem = __rwsem_do_wake(sem, RWSEM_WAKE_READERS);
+ raw_spin_unlock_irq(&sem->wait_lock);
+ }
+
+ return ret;
}
EXPORT_SYMBOL(rwsem_down_write_failed_killable);
--
Michal Hocko
SUSE Labs
