On Thu, Oct 13, 2016 at 04:17:21PM +0100, Will Deacon wrote:
> 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.
Right, so I think the code is fine, since in that case the
__mutex_trylock() must see the handoff and we'll break the loop and
(re)set the state to RUNNING.
But you're right in that its slightly odd. I'll reorder them and put the
set_task_state() above the !first thing.
