Date: Thu, 13 Oct 2016 16:17:21 +0100
From: Will Deacon <will.deacon@arm.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
        Waiman Long <waiman.long@hpe.com>, Jason Low <jason.low2@hpe.com>,
        Ding Tianhong <dingtianhong@huawei.com>,
        Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>,
        Imre Deak <imre.deak@intel.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Davidlohr Bueso <dave@stgolabs.net>,
        Tim Chen <tim.c.chen@linux.intel.com>, Terry Rudd <terry.rudd@hpe.com>,
        "Paul E. McKenney" <paulmck@us.ibm.com>, Jason Low <jason.low2@hp.com>,
        Chris Wilson <chris@chris-wilson.co.uk>,
        Daniel Vetter <daniel.vetter@ffwll.ch>
Subject: Re: [PATCH -v4 6/8] locking/mutex: Restructure wait loop
Message-ID: <20161013151720.GB13138@arm.com>
References: <20161007145243.361481786@infradead.org>
 <20161007150211.271490994@infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20161007150211.271490994@infradead.org>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1890
Lines: 57

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