Re: [PATCH] sched: Move the sleeping while atomic checks early in cond_resched()

[Frederic Weisbecker <fweisbec@gmail.com>]

On Fri, Jul 10, 2009 at 07:43:30PM +0200, Peter Zijlstra wrote:
> On Fri, 2009-07-10 at 19:14 +0200, Frederic Weisbecker wrote:
> > might_sleep() is called lately in cond_resched(), after the
> > need_resched()/preempt enabled/system running tests are checked.
> > 
> > It's better to check the sleeps while atomic earlier and not depend
> > on some environment datas that reduce the chances to detect a
> > problem.
> > 
> > Changes in v2:
> > - call __might_sleep() directly instead of might_sleep() which may call
> >   cond_resched()
> > - turn cond_resched() into a macro so that the file:line couple reported
> >   refers to the caller of cond_resched() and not __cond_resched() itself.
> > - drop the obsolete CONFIG_PREEMPT_BKL related zones
> > 
> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> > Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > 
> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> > ---
> >  include/linux/sched.h |   22 +++++++---------------
> >  kernel/sched.c        |    5 ++---
> >  2 files changed, 9 insertions(+), 18 deletions(-)
> > 
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index 0cb0d8d..737f569 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -2276,23 +2276,15 @@ static inline int need_resched(void)
> >   * cond_resched_softirq() will enable bhs before scheduling.
> >   */
> >  extern int _cond_resched(void);
> > -#ifdef CONFIG_PREEMPT_BKL
> > -static inline int cond_resched(void)
> > -{
> > -	return 0;
> > -}
> > -#else
> > -static inline int cond_resched(void)
> > -{
> > -	return _cond_resched();
> > -}
> > -#endif
> > +#define cond_resched() ({			\
> > +	__might_sleep(__FILE__, __LINE__);	\
> > +	_cond_resched();			\
> > +})
> 
> I don't think this will compile for !CONFIG_DEBUG_SPINLOCK_SLEEP.


Ahh, right.

 
> >  extern int cond_resched_lock(spinlock_t * lock);
> >  extern int cond_resched_softirq(void);
> > -static inline int cond_resched_bkl(void)
> > -{
> > -	return _cond_resched();
> > -}
> > +
> > +#define cond_resched_bkl()	cond_resched();
> 
> We might as well convert the one user of this ;-)


Ok :)


> >  /*
> >   * Does a critical section need to be broken due to another
> > diff --git a/kernel/sched.c b/kernel/sched.c
> > index 87ecac1..649ec92 100644
> > --- a/kernel/sched.c
> > +++ b/kernel/sched.c
> > @@ -6605,9 +6605,6 @@ SYSCALL_DEFINE0(sched_yield)
> >  
> >  static void __cond_resched(void)
> >  {
> > -#ifdef CONFIG_DEBUG_SPINLOCK_SLEEP
> > -	__might_sleep(__FILE__, __LINE__);
> > -#endif
> >  	/*
> >  	 * The BKS might be reacquired before we have dropped
> >  	 * PREEMPT_ACTIVE, which could trigger a second
> > @@ -6644,6 +6641,7 @@ int cond_resched_lock(spinlock_t *lock)
> >  
> >  	if (spin_needbreak(lock) || resched) {
> >  		spin_unlock(lock);
> > +		__might_sleep(__FILE__, __LINE__);
> >  		if (resched && need_resched())
> >  			__cond_resched();
> >  		else
> > @@ -6661,6 +6659,7 @@ int __sched cond_resched_softirq(void)
> >  
> >  	if (need_resched() && system_state == SYSTEM_RUNNING) {
> >  		local_bh_enable();
> > +		__might_sleep(__FILE__, __LINE__);
> >  		__cond_resched();
> >  		local_bh_disable();
> >  		return 1;
> 
> Right, how about renaming these to _cond_resched_{lock,softirq}, and
> added a __might_sleep() definition for !DEBUG_SPINLOCK_SLEEP and add
> macro wrappers to sched.c for these two as well?

I did that first but thought that might_sleep() would fail in a spinlock
held or softirq context, right?