linux-kernel - Re: [RFC PATCH-queue/locking/rfc 2/2] locking/mutex: Enable optimistic spinning of woken waiter

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]

Message-ID: <57C60F7B.9040804@hpe.com>
Date:   Tue, 30 Aug 2016 18:58:03 -0400
From:   Waiman Long <waiman.long@....com>
To:     Peter Zijlstra <peterz@...radead.org>
CC:     Ingo Molnar <mingo@...hat.com>, <linux-kernel@...r.kernel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Ding Tianhong <dingtianhong@...wei.com>,
        Jason Low <jason.low2@....com>,
        Davidlohr Bueso <dave@...olabs.net>,
        "Paul E. McKenney" <paulmck@...ibm.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Will Deacon <Will.Deacon@....com>,
        Tim Chen <tim.c.chen@...ux.intel.com>,
        Imre Deak <imre.deak@...el.com>
Subject: Re: [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