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

[Ingo Molnar]

* OGAWA Hirofumi <hirofumi@xxxxxxxxxxxxxxxxxx> wrote:

> "Fabio Comolli" <fabio.comolli@xxxxxxxxx> 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.

	Ingo

------------------->
Subject: [patch] sched: remove __resched_legal() and fix cond_resched_softirq()
From: Ingo Molnar <mingo@xxxxxxx>

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@xxxxxxx>
---
 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();
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/