lkml.org 
[lkml]   [2018]   [Nov]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC] locking/rwsem: Avoid issuing wakeup before setting the reader waiter to nil
From
Date
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


\
 
 \ /
  Last update: 2018-11-29 18:02    [W:0.141 / U:3.604 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site