Re: [PATCH -v4 6/8] locking/mutex: Restructure wait loop

[Will Deacon <will.deacon@arm.com>]

Hi Peter,

I'm struggling to get my head around the handoff code after this change...

On Fri, Oct 07, 2016 at 04:52:49PM +0200, Peter Zijlstra wrote:
> --- a/kernel/locking/mutex.c
> +++ b/kernel/locking/mutex.c
> @@ -631,13 +631,21 @@ __mutex_lock_common(struct mutex *lock,
>  
>  	lock_contended(&lock->dep_map, ip);
>  
> +	set_task_state(task, state);
>  	for (;;) {
> +		/*
> +		 * Once we hold wait_lock, we're serialized against
> +		 * mutex_unlock() handing the lock off to us, do a trylock
> +		 * before testing the error conditions to make sure we pick up
> +		 * the handoff.
> +		 */
>  		if (__mutex_trylock(lock, first))
> -			break;
> +			goto acquired;
>  
>  		/*
> -		 * got a signal? (This code gets eliminated in the
> -		 * TASK_UNINTERRUPTIBLE case.)
> +		 * Check for signals and wound conditions while holding
> +		 * wait_lock. This ensures the lock cancellation is ordered
> +		 * against mutex_unlock() and wake-ups do not go missing.
>  		 */
>  		if (unlikely(signal_pending_state(state, task))) {
>  			ret = -EINTR;
> @@ -650,16 +658,27 @@ __mutex_lock_common(struct mutex *lock,
>  				goto err;
>  		}
>  
> -		__set_task_state(task, state);
>  		spin_unlock_mutex(&lock->wait_lock, flags);
>  		schedule_preempt_disabled();
> -		spin_lock_mutex(&lock->wait_lock, flags);
>  
>  		if (!first && __mutex_waiter_is_first(lock, &waiter)) {
>  			first = true;
>  			__mutex_set_flag(lock, MUTEX_FLAG_HANDOFF);
>  		}
> +
> +		set_task_state(task, state);

With this change, we no longer hold the lock wit_hen we set the task
state, and it's ordered strictly *after* setting the HANDOFF flag.
Doesn't that mean that the unlock code can see the HANDOFF flag, issue
the wakeup, but then we come in and overwrite the task state?

I'm struggling to work out whether that's an issue, but it certainly
feels odd and is a change from the previous behaviour.

Will