Re: [RFC] locking/rwsem: Avoid issuing wakeup before setting the reader waiter to nil

[Waiman Long <longman@redhat.com>]

On 11/29/2018 11:06 AM, Peter Zijlstra wrote:
> On Thu, Nov 29, 2018 at 10:21:58AM -0500, Waiman Long wrote:
>
>>>> diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
>>>> index 09b1800..50d9af6 100644
>>>> --- a/kernel/locking/rwsem-xadd.c
>>>> +++ b/kernel/locking/rwsem-xadd.c
>>>> @@ -198,15 +198,22 @@ static void __rwsem_mark_wake(struct rw_semaphore *sem,
>>>>  		woken++;
>>>>  		tsk = waiter->task;
>>>>  
>>>> -		wake_q_add(wake_q, tsk);
>>>> +		get_task_struct(tsk);
>>>>  		list_del(&waiter->list);
>>>>  		/*
>>>> -		 * Ensure that the last operation is setting the reader
>>>> +		 * Ensure calling get_task_struct() before setting the reader
>>>>  		 * waiter to nil such that rwsem_down_read_failed() cannot
>>>>  		 * race with do_exit() by always holding a reference count
>>>>  		 * to the task to wakeup.
>>>>  		 */
>>>>  		smp_store_release(&waiter->task, NULL);
>>>> +		/*
>>>> +		 * Ensure issuing the wakeup (either by us or someone else)
>>>> +		 * after setting the reader waiter to nil.
>>>> +		 */
>>>> +		wake_q_add(wake_q, tsk);
>>>> +		/* wake_q_add() already take the task ref */
>>>> +		put_task_struct(tsk);
>>>>  	}
>>>>  
>>>>  	adjustment = woken * RWSEM_ACTIVE_READ_BIAS - adjustment;
>> I doubt putting wake_q_add() after clearing waiter->task can really fix
> Why; at that point we know the wakeup will happen after, which is all we
> require.
>
>

Thread 1                                  Thread 2      Thread 3

    rwsem_down_read_failed()
 raw_spin_lock_irq(&sem->wait_lock);
 list_add_tail(&waiter.list, &wait_list);
 raw_spin_unlock_irq(&sem->wait_lock);
                                                        __rwsem_mark_wake();
                                                         wake_q_add();
                                          wake_up_q();
                                                         waiter->task =
NULL; --+
 while (true)
{                                                                 |
 
set_current_state(TASK_UNINTERRUPTIBLE);                                     
|
  if (!waiter.task) //
false                                                    |
     
break;                                                                    |
 
schedule();                                                                  
|
 }                                                                       
<-----+
                                                        wake_up_q(&wake_q);

OK, I got confused by the thread racing chart shown in the patch. It
will be clearer if the clearing of waiter->task is moved down as shown.
Otherwise, moving the clearing of waiter->task before wake_q_add() won't
make a difference. So the patch can be a possible fix.

Still we are talking about 3 threads racing with each other. The
clearing of wake_q.next in wake_up_q() is not atomic and it is hard to
predict the racing result of the concurrent wake_q operations between
threads 2 and 3. The essence of my tentative patch is to prevent the
concurrent wake_q operations in the first place.

Cheers,
Longman



The second wake_q_add() above will fail to add the task to the second
wake_q because it is still in the first wake_q. So the second
wake_up_q() will not wake up the task because it is not in its wake_q.

Cheers,
Longman