BUG(): sched.c: Line 944

Linus,

The current in_atomic() check fails with kernel preemption enabled since we set preempt_count to PREEMPT_ACTIVE in preempt_schedule().

We need to additionally check whether PREEMPT_ACTIVE is set.

There is also still the issue that bugging out is a bit drastic and a hindrance to debugging; but I will tackle that later. For now, please apply this so we can at least boot with preemption enabled.

Patch is against 2.5.35-bk.

Robert Love

diff -urN linux-2.5.35/kernel/sched.c linux/kernel/sched.c
@@ -940,8 +940,7 @@
 	struct list_head *queue;
 	int idx;
 
-	if (unlikely(in_atomic()))
-		BUG();
+	BUG_ON(in_atomic() && preempt_count() != PREEMPT_ACTIVE);
 
 #if CONFIG_DEBUG_HIGHMEM
 	check_highmem_ptes();

@@ -959,7 +958,7 @@
 	 * if entering off of a kernel preemption go straight
 	 * to picking the next task.
 	 */
-	if (unlikely(preempt_count() & PREEMPT_ACTIVE))
+	if (unlikely(preempt_count() == PREEMPT_ACTIVE))
 		goto pick_next_task;
 
 	switch (prev->state) {

Would it not be a lot better to just mask off PREEMPT_ACTIVE() instead of checking for it explicitly.

The in_interrupt() etc stuff already effectively do this by masking off the HARDIRQ_MASK etc. I would prefer a patch to hardirq.h that just adds a #define to make preempt_count() not contain PREEMPT_ACTIVE - and make the PREEMPT_ACTIVE checks be a totally separate check (logic: it's not a count, so it shouldn't show up in preempt_count())

I certainly wouldn't mind the DEBUG/WARNING/FATAL infrastructure discussed earlier..

Linus

I liked this idea, and was working on implementing it when I ran into a few roadblocks. Your ideas are welcome.

First, "preempt_count()" is used as an l-value in a lot of places, i.e. look at all the "preempt_count() += foo" in the IRQ code. We cannot mask things out of it.

Thus, I then looked into doing a separate function for the raw value, say an "atomic_count()" ... the code just looked ugly mixing "atomic_count()" and "preempt_count()" for no apparent reason.

Third, PREEMPT_ACTIVE actually _is_ part of the count. It helps assure us a task is not preempted repeatedly. If we did not have it, we would have to bump preempt_count on preemption. So we still need it in the preempt_count().

Simplest solution is to:

	#define in_atomic() \
		(preempt_count() & ~PREEMPT_ACTIVE) != kernel_locked())

although I still dislike the masking just to make the schedule() code-path cleaner.

Oh, and there is another problem: printk() from schedule() implicitly calls wake_up(). My machine dies even with just a printk() and not a BUG()... I suspect there may be some SMP issue in that whole mess too, because setting oops_in_progress prior did not help.

Comments?

Robert Love

Ok. Let's just make the masking explicit in in_atomic() then, like you suggest:

I don't think this is a scheduler cleanliness issue: it's a consistency issue. If "in_interrupt()" and friends do not care about PREEMPT_ACTIVE, then neither should "in_atomic()". The fact that the scheduler test gets cleaned up is secondary - although it is obviously a result of being consistent.

Hmm.. It will call wake_up() because it will try to wake up any klogd. What's the problem? Calling wake_up() should be fine from there..

Linus

OK.

But, ugh, more fun: we preempt_disable() in do_exit(). Every exiting task hits the test. My syslog is huge.

At least for now, can we please revert the check to in_interrupt() ?

Robert Love

diff -urN linux-2.5.35/kernel/sched.c linux/kernel/sched.c
@@ -940,7 +940,7 @@
 	struct list_head *queue;
 	int idx;
 
-	if (unlikely(in_atomic()))
+	if (unlikely(in_interrupt()))
 		BUG();
 
 #if CONFIG_DEBUG_HIGHMEM

I really think the test is correct, and if we revert it now, we certainly won't be able to re-introduce it later when we're closer to 2.6.

So if the in_atomic() change is enough to fix everything but do_exit(), then how about just making do_exit() use PREEMPT_ACTIVE instead?

Linus

Nope. If PREEMPT_ACTIVE is set, schedule() assumes the task is being preempted and skips certain logic e.g. deactivate_task() (this is the same code that lets us safely preempt a TASK_ZOMBIE).

Result is death before init even executes.

Ugh...

Robert Love

Ahhah! I know. You just make lock_depth 0 when you exit, without actually taking the kernel lock. Which fools the logic into accepting a preempt-disable, since it thinks that the preempt disable is due to holding the kernel lock.

Add a big comment and you're all done.

Linus

I was this -> <- close to celebrating. Not so fast, smarty.

What about release_kernel_lock() ?

It sees task->lock_depth>=0 and calls spin_unlock() on a lock that it does not hold.

Robert Love

You forget - I'm not only a smarty, I'm sick and twisted too.

We have a simple rule:
- task->lock_depth = -1 means "no lock held"
- task->lock_depth >= 0 means "BKL really held"

... but what does "task->lock_depth < -1" mean?

Yup: "validly nonpreemptable".

So then you have:

	#define kernel_locked()	(current->lock_depth >= 0)

	#define in_atomic() (preempt_count() & ~PREEMPT_ACTIVE) != (current->lock_depth != -1))

and you're all set - just set lock_depth to -2 when you exit to show that you don't hold the BKL, but that you are validly not preemtable.

I get the award for having the most disgusting ideas.

Linus "but it works and is efficient" Torvalds

Yah, you do. Good job though.

I implemented exactly what you detailed, with one change: we need to check kernel_locked() before setting lock_depth because it is valid to exit() while holding the BKL. Aside from kernel code that does it intentionally, crashed code (e.g. modules) would have the same problem.

Anyhow, this works. Finally.

Except the printk() still causes my system to lockup, but that is another (tomorrow's) issue.

Patch is against current BK. Thanks for the help.

Robert Love

diff -urN linux-2.5.35/include/asm-i386/hardirq.h linux/include/asm-i386/hardirq.h
@@ -77,7 +77,8 @@
 #define irq_enter()		(preempt_count() += HARDIRQ_OFFSET)
 
 #if CONFIG_PREEMPT
-# define in_atomic()	(preempt_count() != kernel_locked())
+# define in_atomic() \
+	((preempt_count() & ~PREEMPT_ACTIVE) != (current->lock_depth != -1))
 # define IRQ_EXIT_OFFSET (HARDIRQ_OFFSET-1)
 #else
 # define in_atomic()	(preempt_count() != 0)

diff -urN linux-2.5.35/kernel/exit.c linux/kernel/exit.c
@@ -637,6 +637,21 @@
 	preempt_disable();
 	if (current->exit_signal == -1)
 		release_task(current);
+
+	/*
+	 * This little bit of genius comes from the twisted mind of Linus.
+	 * We need exit() to be atomic but we also want a debugging check
+	 * in schedule() to whine if we are atomic.  The wickedness is in
+	 * these rules:
+	 *	- task->lock_depth = -2 means "validly nonpreemptable"
+	 * 	- task->lock_depth = -1 means "BKL not held"
+         * 	- task->lock_depth >= 0 means "BKL held"
+	 * release_kernel_lock and kernel_locked() check >=0, and
+	 * in_atomic() checks != -1... the "fake BKL" will "cancel out"
+	 * the preempt_disable() above and everyone is happy.
+	 */
+	if (unlikely(!kernel_locked()))
+		tsk->lock_depth = -2;
 	schedule();
 	BUG();
 /*

diff -urN linux-2.5.35/kernel/sched.c linux/kernel/sched.c
@@ -940,8 +940,10 @@
 	struct list_head *queue;
 	int idx;
 
-	if (unlikely(in_atomic()))
-		BUG();
+	if (unlikely(in_atomic())) {
+		printk(KERN_ERR "error: scheduling while non-atomic!\n");
+		dump_stack();
+	}
 
 #if CONFIG_DEBUG_HIGHMEM
 	check_highmem_ptes();

@@ -959,7 +961,7 @@
 	 * if entering off of a kernel preemption go straight
 	 * to picking the next task.
 	 */
-	if (unlikely(preempt_count() & PREEMPT_ACTIVE))
+	if (unlikely(preempt_count() == PREEMPT_ACTIVE))
 		goto pick_next_task;
 
 	switch (prev->state) {

fsck, this is non-ending... obviously, this is insufficient:
preempt_count will equal two if the BKL was previously held and in_atomic() will trip.

This should work:

	if (likely(!kernel_locked())
		tsk->lock_depth = -2;
	else {
		/* compensate for BKL; we still cannot preempt */
		preempt_enable_no_resched();
	}

look sane?

Now, remind me why this is all worth it...

Robert Love

On 17 Sep 2002, Robert Love wrote:

[...]

having preemption support that 1) is correct 2) works?

We *must* use the schedule() check to debug preemption bugs, or we wont have usable preemption in 2.6, i dont really understand why your are not happy that we have such a great tool. In fact we should also add other debugging bits, like 'check for !0 preemption count in smp_processor_id()' , and the underflow checks that caught the IDE bug. These are all bits that help the elimination of preemption bugs which are also often SMP bugs, on plain UP boxes.

Ingo

Hm, sorry if I sound like I do not want something "so great". I do. I just do not ever want to compromise the existing code... I would much prefer to say "wow we cannot do this cleanly now, let's wait until we figure out a clean way".

Anyhow, one of us is confused. How can this in_atomic() test _ever_ catch a preemption bug? We cannot enter the scheduler off kernel preemption unless preempt_count==0. This is a test to catch bugs in other parts of the kernel, e.g. where code explicitly calls schedule() while holding a lock.

Robert Love

you are right, i was confusing this with the older check for disabled interrupt in preempt_schedule() [which i'd still find useful].

The smp_processor_id() test catches true preemption bugs. So does preempt_count() underflow detection.

i do agree with Alan - there can be nothing bad in trying to fix all that non-preempt-aware code right now, before it becomes too late.

Ingo

Robert,

I applied the modified patch you sent at 04:51 plus the above and had to change dump_stack() to show_stack(NULL) in sched.c due to

kernel/kernel.o: In function `schedule':
kernel/kernel.o(.text+0xf13): undefined reference to `dump_stack'

I used plain vanilla 2.5.35 to patch against.

I booted that so-patched kernel and it got much further than before, up to where syslogd was able to write some stuff to /var/log/messages.

There were many instances of "error: scheduling while non-atomic!", and the traces were similar. Here is a decoded one:

[steven@spc5 linux-2.5.35]$ ksymoops -K -L -O -v vmlinux -m System.map <non-atomic.txt ksymoops 2.4.4 on i686 2.5.35. Options used
-v vmlinux (specified)
-K (specified)
-L (specified)
-O (specified)
-m System.map (specified)

f7739f50 c0282f00 f7771960 c0337f20 c011c51b c1b0eec0 c1b71ba0 f7738000

       f7738000 f7738000 f78d4da0 c011d08b f78eb380 c012d67f f78d01c0 f78eb380
       f78eb560 f78d01c0 f78d01dc 40015000 bffffc48 c012d6c5 4213030c 00000004
Call Trace: [<c011c51b>] [<c011d08b>] [<c012d67f>] [<c012d6c5>] [<c010918f>]

Warning (Oops_read): Code line not seen, dumping what data is available

Trace; c011c51b <put_files_struct+bb/d0>
Trace; c011d08b <do_exit+35b/370>
Trace; c012d67f <do_munmap+11f/130>
Trace; c012d6c5 <sys_munmap+35/60>
Trace; c010918f <syscall_call+7/b>

Hope this helps,
Steven

That is what is happening to me... if you trace when you lockup, its due to the printk. My machines in these cases are only livelocked, I can still ping etc.

Ugh this looks like the exit path which should not be triggered.

Robert Love

I grabbed the 2.5.35-bk3 patch provided by Jeff Garzik so that I could test something even more current and applied your patches to that (one hunk was already there in -bk3).

I ran several of the stack dumps through ksymoops. Perhaps some of these might point to something interesting.

Steven

ksymoops 2.4.4 on i686 2.5.35. Options used
-v vmlinux (specified)
-K (specified)
-L (specified)
-O (specified)
-m System.map (specified)

f773df4c c01167a6 c02834a0 f77801c0 c0337f20 c011c4eb c1b0eec0 f7936c60

       f773c000 f773c000 f773c000 f77021a0 c011d05b 00000000 0000a9db f77021a0
       c0122340 f7744760 f78cd2e0 f7744760 f773c000 bffffd08 c0122522 4213030c
Call Trace: [<c01167a6>] [<c011c4eb>] [<c011d05b>] [<c0122340>] [<c0122522>]
   [<c010918f>]
f773df4c c01167a6 c02834a0 f77801c0 c0337f20 c011c4eb c1b0eec0 f7936ee0
       f773c000 f773c000 f773c000 f77021a0 c011d05b c022305b 00000003 bfffea00
       0000001c 00000000 0804b3c8 00000014 00000003 bfffea00 0000001c 4213030c
Call Trace: [<c01167a6>] [<c011c4eb>] [<c011d05b>] [<c022305b>] [<c010918f>]
f7739f4c c01167a6 c02834a0 f77801c0 c0337f20 c011c4eb c1b0eec0 f7936820
       f7738000 f7738000 f7738000 f772a800 c011d05b 00000000 f7738000 bffff7a4
       f7739fb0 0807e210 04000000 42029098 00000000 00000000 00000000 4213030c
Call Trace: [<c01167a6>] [<c011c4eb>] [<c011d05b>] [<c010918f>]
f7789f4c c01167a6 c02834a0 f7c5e760 c0337f20 c011c4eb c1b0eec0 c1b71da0
       f7788000 f7788000 f7788000 f7ea8d60 c011d05b 0000006e 00000000 00000005
       c0222fb0 00000005 f77fa1a0 fffffff7 00000005 bffffb28 c0142fe3 4213030c
Call Trace: [<c01167a6>] [<c011c4eb>] [<c011d05b>] [<c0222fb0>] [<c0142fe3>]
   [<c010918f>]

Warning (Oops_read): Code line not seen, dumping what data is available

Trace; c01167a6 <schedule+36/3d0>
Trace; c011c4eb <put_files_struct+bb/d0>
Trace; c011d05b <do_exit+35b/370>
Trace; c0122340 <process_timeout+0/10>
Trace; c0122522 <sys_nanosleep+122/1a0>
Trace; c010918f <syscall_call+7/b>
Trace; c01167a6 <schedule+36/3d0>
Trace; c011c4eb <put_files_struct+bb/d0>
Trace; c011d05b <do_exit+35b/370>
Trace; c022305b <sys_socketcall+13b/200>
Trace; c010918f <syscall_call+7/b>
Trace; c01167a6 <schedule+36/3d0>
Trace; c011c4eb <put_files_struct+bb/d0>
Trace; c011d05b <do_exit+35b/370>
Trace; c010918f <syscall_call+7/b>
Trace; c01167a6 <schedule+36/3d0>
Trace; c011c4eb <put_files_struct+bb/d0>
Trace; c011d05b <do_exit+35b/370>
Trace; c0222fb0 <sys_socketcall+90/200>
Trace; c0142fe3 <sys_write+33/40>
Trace; c010918f <syscall_call+7/b>

1 warning issued. Results may not be reliable.

I really think we need this to have a stable system. Alan still thinks preempt is unstable, and this helps counter some of that - by adding sanity checks that all the counters are doing the right thing.

Also, it's one more reason to support preemption in the first place. Having lower latency is all fine, but not everybody cares and clearly preemption adds its own overhead. Having the preemption infrastructure add its own set of help (ie helping find not only preempt-related bugs, but spinlock bugs too) makes preempt all the more useful.

In particular, with preempt we can add atomicity debugging to "put_user()" and friends, to verify that nobody tries to do user-mode accesses when they aren't allowed to - another use for "in_atomic()". But that's requires a functioning in_atomic() that isn't too limited to be generally used..

Linus

That said, the exit() crap does end up being a bit unreadable. How about just splitting up schedule() into the "normal schedule" and the "exit()" case. In particular, there are a few other things that are illegal for the non-exit case anyway, and that we migth as well check for. There are also potentially things we might want to do in the exit case that we don't want to do in the hot-path.

So we could just rename the current core "schedule()" functionality as "static void do_schedule()", and then have

	void schedule(void)
	{
		BUG_ON(in_atomic());
		BUG_ON(current->state & TASK_ZOMBIE);
		do_schedule();
	}

	void exit_schedule(void)
	{
		BUG_ON(!(current->state & TASK_ZOMBIE));
		do_schedule();
	}

which is simpler than playing games with preempt_count == -2, and also allows us more sanity checking than we have right now (the TASK_ZOMBIE thing is also a exit-specific thing, and there may well be others too).

Linus

i'd rather do it the other way around - ie. do a:

	if (unlikely(current->state == TASK_ZOMBIE)) {
		... exit checks ...
	} else {
		... normal checks ...
	}

this test is cheaper than a function call. I'd say the ZOMBIE check alone is not significant enough to introduce a function split in the hotpath. This check is also easier to remove later on.

TASK_ZOMBIE is only ever allowed to be set for exiting tasks. [and if TASK_ZOMBIE handling is broken then it will trigger the normal atomic tests so it's not like we are silently ignoring those errors.]

Ingo

Ehh.. On most architectures an unconditional direct function call is _faster_ than a conditional test that does not predict all that well. So the "test is faster than a function call" is not a very good optimization, I suspect.

In fact, we'd probably be better off trying to factor out more out of schedule() rather than making it even bigger. We've done that before: the timeout stuff was done that way, and meant that normal reschedules no longer need to care about timeouts. That made things simpler.

On the other hand, we do have other ways to test the preempt count inside the scheduler. In particular, we might just move the "in_atomic()" check a few lines downwards, at which point we've released the kernel lock and explicitly disabled preemption, so at that point the test should be even simpler with fewer conditionals..

Linus

indeed ...

Ingo

OK so do we want to do (a):

(moved down to after the preempt_disable() and release_kernel_lock())

if (likely(current->state != TASK_ZOMBIE)

	if (unlikely((preempt_count() & ~PREEMPT_ACTIVE) != 1))
		...

or go with (b) where we split schedule() into schedule(), exit_schedule(), and do_schedule().

Robert Love

i'd do (a). current->state is to be used anyway, and the default-untaken first branch should be cheap. Plus by moving things down the splitup of the function would create more code duplication than necessery i think.

Ingo

Note by moving it down, the only gain over keeping it at the top is not having to check for the BKL...

Anyhow, I would appreciate it if you could give this a try (with kernel preemption enabled)... any comments are appreciated.

(Note you need a 2.5.35-bk release to get the dump_stack(). Otherwise use show_trace(0).)

Robert Love

diff -urN linux-2.5.35/kernel/sched.c linux/kernel/sched.c
@@ -940,9 +940,6 @@
 	struct list_head *queue;
 	int idx;
 
-	if (unlikely(in_atomic()))
-		BUG();
-
 #if CONFIG_DEBUG_HIGHMEM
 	check_highmem_ptes();
 #endif

@@ -950,8 +947,20 @@
 	preempt_disable();
 	prev = current;
 	rq = this_rq();
-
 	release_kernel_lock(prev);
+
+	/*
+	 * Test if we are atomic.  Since do_exit() needs to call into
+	 * schedule() atomically, we ignore that for now.  Otherwise,
+	 * whine if we are scheduling when we should not be.
+	 */
+	if (likely(current->state != TASK_ZOMBIE)) {
+		if (unlikely((preempt_count() & ~PREEMPT_ACTIVE) != 1)) {
+			printk(KERN_ERR "scheduling while non-atomic!\n");
+			dump_stack();
+		}
+	}
+
 	prev->sleep_timestamp = jiffies;
 	spin_lock_irq(&rq->lock);
 

@@ -959,7 +968,7 @@
 	 * if entering off of a kernel preemption go straight
 	 * to picking the next task.
 	 */
-	if (unlikely(preempt_count() & PREEMPT_ACTIVE))
+	if (unlikely(preempt_count() == PREEMPT_ACTIVE))
 		goto pick_next_task;
 
 	switch (prev->state) {

I applied that patch to 2.5.35-bk3 and with PREEMPT enabled. And it booted without any of the usual complaints with preempt and the in_atomic check. But then, I ran

1) dbench 1 OK
2) dbench 2 OK
3) dbench 3 blam!

Running dbench 3 resulted in the dbench clients hanging and being unkillable with kill -9 in the D state.

steven    1046  0.0  0.0  1440  472 ?        D    13:46   0:00 ./dbench 3
steven    1047  0.0  0.0  1440  420 ?        D    13:46   0:00 ./dbench 3
steven    1048  0.0  0.0  1440  472 ?        D    13:46   0:00 ./dbench 3

I can ssh and enter my user password, but it hangs after that with no bash prompt. Other ssh sessions which I started previously are still responsive.

Test box is 2-way pIII, kernel SMP.

Steven

On Tue, 2002-09-17 at 15:54, Steven Cole wrote:

Thank you for the testing, Steven.

Hrm, I cannot reproduce this. I just successfully completed a `dbench 16'. Can you find where they are hanging? You can get a trace via sysrq. You can also see where they are in the kernel via the wchan field of ps: "ps -ewo user,pid,priority,%cpu,stat,command,wchan" is a favorite of mine.

Sure it does not happen with a stock kernel (no preempt)?

What if you replace the printk() and dump_stack() in schedule() with a no-op (but not something that will optimize away the conditional, i.e. try a cpu_relax()).

Oh, is the previous patch fully backed out? None of that do_exit muck anymore, right?

I too am SMP with kernel preemption, dual Athlon MP.

Regards,

Robert Love

I rebooted the box, but had to SYSRQ-S, SYSRQ-B, due to shutdown -r now hanging up. On reboot, the init scripts froze after "starting atd", but fortunately this was after sshd, so I ssh'd to the box several times, and did your ps command before hanging it with dbench 3 (dbench 1 and 2 were successful again). I have that ps output if you want it. I could NOT do that, or even ls, after the dbench 3 hang. When I shutdown, SYSRQ-S only partially synced (just one partition).

I'll try 2.5.35-bk3 (no other patches) without preempt shortly. I'll report any failures, but no news is good news.

I'll try that if I get the time. But I wasn't getting any dump info in dmesg for those boots.

Yep, plain 2.5.35, then patched with 2.5.35-bk3, then with your patch posted at 15:23. Nothing else.

Steven

Sorry, it hung so badly that it didn't respond to that.

I just began testing plain vanilla 2.5.35-bk3 without preempt, and the box has run up to 24 clients so far. So far so good.

I'll try that in a bit.

Steven

I fixed the hang. If you notice the problem, please do not laugh.

The attached patch, against 2.5.36, should work fine...

Robert Love

diff -urN linux-2.5.36/kernel/sched.c linux/kernel/sched.c
@@ -940,9 +940,6 @@
 	struct list_head *queue;
 	int idx;
 
-	if (unlikely(in_atomic()))
-		BUG();
-
 #if CONFIG_DEBUG_HIGHMEM
 	check_highmem_ptes();
 #endif

@@ -950,8 +947,20 @@
 	preempt_disable();
 	prev = current;
 	rq = this_rq();
-
 	release_kernel_lock(prev);
+
+	/*
+	 * Test if we are atomic.  Since do_exit() needs to call into
+	 * schedule() atomically, we ignore that for now.  Otherwise,
+	 * whine if we are scheduling when we should not be.
+	 */
+	if (likely(current->state != TASK_ZOMBIE)) {
+		if (unlikely((preempt_count() & ~PREEMPT_ACTIVE) != 1)) {
+			printk(KERN_ERR "scheduling while non-atomic!\n");
+			dump_stack();
+		}
+	}
+
 	prev->sleep_timestamp = jiffies;
 	spin_lock_irq(&rq->lock);
 

Thanks. That works fine here. Now that I can run with PREEMPT again, I'll leave it enabled. It does seem to improve interactive feel under heavy load, but without an easily identifiable metric for that you know what Rik would say.

Steven

ie. for the time being, something like:

@@ -940,8 +940,9 @@
 	struct list_head *queue;
 	int idx;
 
-	if (unlikely(in_atomic()))
-		BUG();
+	if (likely(current->state != TASK_ZOMBIE))
+		if (unlikely(in_atomic()))
+			BUG();
 
 #if CONFIG_DEBUG_HIGHMEM
 	check_highmem_ptes();