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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 24 Jan 2016 16:03:50 +0800
From:	Ding Tianhong <dingtianhong@...wei.com>
To:	Waiman Long <waiman.long@....com>,
	Peter Zijlstra <peterz@...radead.org>
CC:	Jason Low <jason.low2@...com>, Ingo Molnar <mingo@...hat.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Davidlohr Bueso" <dave@...olabs.net>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	"Paul E. McKenney" <paulmck@...ibm.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Will Deacon <Will.Deacon@....com>,
	Tim Chen <tim.c.chen@...ux.intel.com>,
	Waiman Long <Waiman.Long@...com>
Subject: Re: [PATCH RFC] locking/mutexes: don't spin on owner when wait list
 is not NULL.

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

> .
>