Re: [PATCH 2/4] locking/rwsem: Drop superfluous waiter refcount

[Davidlohr Bueso <dave@stgolabs.net>]

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