Re: [PATCH -v11][RFC] mutex: implement adaptive spinning

[Nick Piggin <npiggin@suse.de>]

On Wed, Jan 14, 2009 at 06:22:36PM +0100, Peter Zijlstra wrote:
> On Wed, 2009-01-14 at 18:18 +0100, Nick Piggin wrote:
> 
> > > @@ -173,21 +237,21 @@ __mutex_lock_common(struct mutex *lock, 
> > >  			spin_unlock_mutex(&lock->wait_lock, flags);
> > >  
> > >  			debug_mutex_free_waiter(&waiter);
> > > +			preempt_enable();
> > >  			return -EINTR;
> > >  		}
> > >  		__set_task_state(task, state);
> > >  
> > >  		/* didnt get the lock, go to sleep: */
> > >  		spin_unlock_mutex(&lock->wait_lock, flags);
> > > -		schedule();
> > > +		__schedule();
> > 
> > Why does this need to do a preempt-disabled schedule? After we schedule
> > away, the next task can do arbitrary things or reschedule itself, so if
> > we have not anticipated such a condition here, then I can't see what
> > __schedule protects. At least a comment is in order?
> 
> From:
> http://programming.kicks-ass.net/kernel-patches/mutex-adaptive-spin/mutex-preempt.patch
> 
> Subject: mutex: preemption fixes
> From: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Date: Wed Jan 14 15:36:26 CET 2009
> 
> The problem is that dropping the spinlock right before schedule is a voluntary
> preemption point and can cause a schedule, right after which we schedule again.
> 
> Fix this inefficiency by keeping preemption disabled until we schedule, do this
> by explicitly disabling preemption and providing a schedule() variant that
> assumes preemption is already disabled.
> 
> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> 
> > Pity to add the call overhead to schedule just for this case.
> 
> Good point, seeing any way around that?

Hmm, well this is rather a slow path, I would say. I'd prefer not to
modify schedule in this way (if we just get scheduled back on after
being switched away, the subsequent call to schedule is going to be
cache hot and not do too much work).

preempt_enable_noresched maybe if you really care, would close up the
window even more. But is it really worthwhile? We'd want to see numbers
(when in doubt, keep it  simpler).
 

> >  BTW. __schedule shouldn't need to be asmlinkage?
> 
> TBH I've no clue, probably not, Ingo?