Re: [PATCH RFC] locking/mutexes: don't spin on owner when wait list is not NULL.

[Ding Tianhong]

On 2016/1/22 21:59, Waiman Long wrote:
> On 01/22/2016 06:06 AM, Peter Zijlstra wrote:
>> On Fri, Jan 22, 2016 at 11:56:52AM +0100, Peter Zijlstra wrote:
>>> On Fri, Jan 22, 2016 at 11:53:12AM +0100, Peter Zijlstra wrote:
>>>
>>>> There might be other details, but this is the one that stood out.
>>> I think this also does the wrong thing for use_ww_ctx.
>> Something like so?
> 
> I think that should work. My only minor concern is that putting the waiter spinner at the end of the OSQ will take it longer to get the lock, but that shouldn't be a big issue.
> 
>> ---
>>   kernel/locking/mutex.c | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
>> index 0551c219c40e..070a0ac34aa7 100644
>> --- a/kernel/locking/mutex.c
>> +++ b/kernel/locking/mutex.c
>> @@ -512,6 +512,7 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
>>       struct task_struct *task = current;
>>       struct mutex_waiter waiter;
>>       unsigned long flags;
>> +    bool acquired;
>>       int ret;
>>
>>       preempt_disable();
>> @@ -543,6 +544,7 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
>>       lock_contended(&lock->dep_map, ip);
>>
>>       for (;;) {
>> +        acquired = false;
>>           /*
>>            * Lets try to take the lock again - this is needed even if
>>            * we get here for the first time (shortly after failing to
>> @@ -577,7 +579,16 @@ __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();
>> +
>> +        if (mutex_is_locked(lock))
>> +            acquired = mutex_optimistic_spin(lock, ww_ctx, use_ww_ctx);
>> +
>>           spin_lock_mutex(&lock->wait_lock, flags);
>> +
>> +        if (acquired) {
>> +            atomic_set(&lock->count, -1);
>> +            break;
>> +        }
>>       }
>>       __set_task_state(task, TASK_RUNNING);
>>
>> @@ -587,6 +598,9 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
>>           atomic_set(&lock->count, 0);
>>       debug_mutex_free_waiter(&waiter);
>>
>> +    if (acquired)
>> +        goto unlock;
>> +
>>   skip_wait:
>>       /* got the lock - cleanup and rejoice! */
>>       lock_acquired(&lock->dep_map, ip);
>> @@ -597,6 +611,7 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
>>           ww_mutex_set_context_slowpath(ww, ww_ctx);
>>       }
>>
>> +unlock:
>>       spin_unlock_mutex(&lock->wait_lock, flags);
>>       preempt_enable();
>>       return 0;
> 
> Cheers,
> Longman
> 

looks good to me, I will try this solution and report the result, thanks everyone.

Ding

> .
>