On Mon, Oct 17, 2016 at 03:24:08PM +0200, Peter Zijlstra wrote:
> On Mon, Oct 17, 2016 at 12:44:49PM +0200, Peter Zijlstra wrote:
> > 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.
> 
> 
> Humm,.. we might actually rely on this order, since the MB implied by
> set_task_state() is the only thing that separates the store of
> __mutex_set_flag() from the load of __mutex_trylock(), and those should
> be ordered I think.
> 

But __mutex_set_flag() and __mutex_trylock() actually touch the same
atomic word? So we don't need extra things to order them?

Regards,
Boqun

> Argh, completely messed up my brain. I'll not touch it and think on this
> again tomorrow.

Attachment: signature.asc
Description: PGP signature

Reply via email to