Messages in this thread | | | Date | Mon, 9 May 2016 11:51:18 -0700 | From | Davidlohr Bueso <> | Subject | Re: [PATCH 2/4] locking/rwsem: Drop superfluous waiter refcount |
| |
On Mon, 09 May 2016, Peter Zijlstra wrote:
>> >So I think you're wrong here; imagine this: >> > >> > >> > rwsem_down_read_failed() rwsem_wake() >> > get_task_struct(); >> > raw_spin_lock_irq(&wait_lock); >> > list_add_tail(&waiter.list, &wait_list); >> > raw_spin_unlock_irq(&wait_lock); >> > raw_spin_lock_irqsave(&wait_lock) >> > __rwsem_do_wake() >> > while (true) { >> > set_task_state(tsk, TASK_UNINTERRUPTIBLE); >> > waiter->task = NULL >> > if (!waiter.task) // true >> > break; >> > >> > __set_task_state(tsk, TASK_RUNNING); >> > >> > do_exit(); >> > wake_up_process(tsk); /* BOOM */ >> >> I may be missing something, but rwsem_down_read_failed() will not return until >> after the wakeup is done by the rwsem_wake() thread. > >The above never gets to schedule(), and even if it did, a spurious >wakeup could've happened, no?
Ah indeed, you are most certainly correct. For some reason I was always considering schedule() in the picture. Hmm I'll have to think about this some more, but given the small chance of a waiter actually seeing the nil task at the first iteration I'm wondering if we could just invert the code and call schedule() before the task check. Saving the refcounts will serve _all_ reader waiters otoh, but this would obviously need numbers...
Thanks, Davidlohr
| |