Re: [patch] sched: remove __resched_legal() and fix cond_resched_softirq()

[OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>]

Ingo Molnar <mingo@elte.hu> writes:

> * OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> wrote:
>
>> "Fabio Comolli" <fabio.comolli@gmail.com> writes:
>> 
>> > Just found this in syslog. It was during normal activity, about 6
>> > minutes after resume-from-ram. I never saw this before.
>> 
>> It seems someone missed to check PREEMPT_ACTIVE in __resched_legal().
>
> but PREEMPT_ACTIVE is 0x10000000, not 0x20000000.
>
>> Could you please test the following patch?
>
> no. cond_resched() is always legal in the !PREEMPT case.
>
> i found another bug and realized that the whole __resched_legal() 
> approach is fundamentally wrong! The patch below fixes this.

Hmm.. but the path seems,

-> cond_resched()
  -> if (__resched_legal())		/* preempt_count == 0 */
    -> __cond_resched()			/* preempt_count == 0x10000000 */
      -> schedule()
        [...]
        -> cond_resched()
          -> if (__resched_legal())	/* preempt_count == 0x10000000 */
            -> __cond_resched()		/* preempt_count == 0x20000000 */
              -> schedule()             /* warning */

Where is it prevented? Or warning is just wrong?

> ------------------->
> Subject: [patch] sched: remove __resched_legal() and fix cond_resched_softirq()
> From: Ingo Molnar <mingo@elte.hu>
>
> remove the __resched_legal() check: it is conceptually broken. The 
> biggest problem it had is that it can mask buggy cond_resched() calls. A 
> cond_resched() call is only legal if we are not in an atomic context. 
> But __resched_legal() hid this fact. Same goes for cond_resched_locked() 
> and cond_resched_softirq().
>
> furthermore, the __legal_resched(0) call was buggy in 
> cond_resched_softirq() and caused unnecessary long softirq latencies!
>
> the fix is to preserve the only valid inhibitor to voluntary preemption: 
> if the system is still booting. None of the other behavior of 
> __resched_legal() made any sense.
>
> the effect of this fix should be more real bugs exposed, and shorter 
> softirq latencies.
>
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> ---
>  kernel/sched.c |   17 +++--------------
>  1 file changed, 3 insertions(+), 14 deletions(-)
>
> Index: linux/kernel/sched.c
> ===================================================================
> --- linux.orig/kernel/sched.c
> +++ linux/kernel/sched.c
> @@ -4617,17 +4617,6 @@ asmlinkage long sys_sched_yield(void)
>  	return 0;
>  }
>  
> -static inline int __resched_legal(int expected_preempt_count)
> -{
> -#ifdef CONFIG_PREEMPT
> -	if (unlikely(preempt_count() != expected_preempt_count))
> -		return 0;
> -#endif
> -	if (unlikely(system_state != SYSTEM_RUNNING))
> -		return 0;
> -	return 1;
> -}
> -
>  static void __cond_resched(void)
>  {
>  #ifdef CONFIG_DEBUG_SPINLOCK_SLEEP
> @@ -4647,7 +4636,7 @@ static void __cond_resched(void)
>  
>  int __sched cond_resched(void)
>  {
> -	if (need_resched() && __resched_legal(0)) {
> +	if (need_resched() && system_state == SYSTEM_RUNNING) {
>  		__cond_resched();
>  		return 1;
>  	}
> @@ -4673,7 +4662,7 @@ int cond_resched_lock(spinlock_t *lock)
>  		ret = 1;
>  		spin_lock(lock);
>  	}
> -	if (need_resched() && __resched_legal(1)) {
> +	if (need_resched() && system_state == SYSTEM_RUNNING) {
>  		spin_release(&lock->dep_map, 1, _THIS_IP_);
>  		_raw_spin_unlock(lock);
>  		preempt_enable_no_resched();
> @@ -4689,7 +4678,7 @@ int __sched cond_resched_softirq(void)
>  {
>  	BUG_ON(!in_softirq());
>  
> -	if (need_resched() && __resched_legal(0)) {
> +	if (need_resched() && system_state == SYSTEM_RUNNING) {
>  		raw_local_irq_disable();
>  		_local_bh_enable();
>  		raw_local_irq_enable();

-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>