lkml.org 
[lkml]   [2016]   [Aug]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC PATCH-queue/locking/rfc 2/2] locking/mutex: Enable optimistic spinning of woken waiter
On 08/30/2016 11:08 AM, Peter Zijlstra wrote:
> On Fri, Aug 26, 2016 at 07:35:09PM -0400, Waiman Long wrote:
>
>> @@ -624,13 +649,24 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
>> /* didn't get the lock, go to sleep: */
>> spin_unlock_mutex(&lock->wait_lock, flags);
>> schedule_preempt_disabled();
>>
>> + /*
>> + * Both __mutex_trylock() and __mutex_waiter_is_first()
>> + * can be done without the protection of wait_lock.
>> + */
> True, but it took me a little while to figure out why
> __mutex_waiter_is_first() is safe without the lock :-)

Yes, if you are the first waiter, the condition will not be changed even
when new waiter is being added to the tail of the list.

>
>> + acquired = __mutex_trylock(lock);
>>
>> + if (!acquired&& __mutex_waiter_is_first(lock,&waiter)) {
>> __mutex_set_flag(lock, MUTEX_FLAG_HANDOFF);
>> + /*
>> + * Wait until the lock is handed off or the owner
>> + * sleeps.
>> + */
>> + acquired = mutex_optimistic_spin(lock, ww_ctx,
>> + use_ww_ctx, true);
>> + }
> That said; I think there's a few problems with this. Since we now poke
> at the loop termination conditions outside of the wait_lock, it becomes
> important where we do the task->state vs wakeup bits.
>
> Specifically, since we still have state==RUNNING here, its possible
> we'll fail to acquire the lock _and_ miss the wakeup from
> mutex_unlock(). Leaving us stuck forever more.
>
> Also, we should do the __mutex_trylock _after_ we set the handoff,
> otherwise its possible we get the lock handed (miss the wakeup as per
> the above) and fail to notice, again going back to sleep forever more.
>

Yes, you are right. I am less familiar with the intricacy of the
sleep-wakeup interaction.

> @@ -638,7 +636,8 @@ __mutex_lock_common(struct mutex *lock,
>
> lock_contended(&lock->dep_map, ip);
>
> - for (acquired = false; !acquired; ) {
> + set_task_state(task, state);
> + for (;;) {
> /*
> * got a signal? (This code gets eliminated in the
> * TASK_UNINTERRUPTIBLE case.)
> @@ -654,30 +653,23 @@ __mutex_lock_common(struct mutex *lock,
> goto err;
> }
>
> - __set_task_state(task, state);
> -
> - /* didn't get the lock, go to sleep: */
> spin_unlock_mutex(&lock->wait_lock, flags);
> schedule_preempt_disabled();
>
> - /*
> - * Both __mutex_trylock() and __mutex_waiter_is_first()
> - * can be done without the protection of wait_lock.
> - */
> - acquired = __mutex_trylock(lock, true);
> + set_task_state(task, state);
>
> - if (!acquired&& __mutex_waiter_is_first(lock,&waiter)) {
> + if (__mutex_waiter_is_first(lock,&waiter)) {
> __mutex_set_flag(lock, MUTEX_FLAG_HANDOFF);
> - /*
> - * Wait until the lock is handed off or the owner
> - * sleeps.
> - */
> - acquired = mutex_optimistic_spin(lock, ww_ctx,
> - use_ww_ctx, true);
> + if (mutex_optimistic_spin(lock, ww_ctx, use_ww_ctx, true))
> + break;
> }
>
> + if (__mutex_trylock(lock, true))
> + break;
> +

I think the set _task_state() can be moved to just before
__mutex_trylock(). In this way, we can save a smp_mb() if we can get the
lock in the optspin loop. Other than that, I am fine with the other changes.

Cheers,
Longman

\
 
 \ /
  Last update: 2016-09-17 09:58    [W:2.767 / U:0.304 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site