kernel BUG at sched.c:944! only with CONFIG_PREEMPT=y]

Yes. Sorry, didn't catch this reply until now.
I backed out Changeset 1.606 which Linus did to kernel/sched.c did this:

(quoted diff) +1 -1

-       if (unlikely(in_interrupt()))
+       if (unlikely(in_atomic()))

and 2.5.34-mm2 was able to boot with CONFIG_PREEMPT=y.

As I said in a response to myself on lkml, I know this isn't a fix, it just shows there is a problem somewhere with preempt.

While this sounds like a great debugging check, it is not useful in general since we surely have some bad code that calls schedule() with locks held. Further, since the atomic accounting only includes locks if CONFIG_PREEMPT is set, you only see this with kernel preemption enabled.

it *is* a great debugging check, at zero added cost. Scheduling from an atomic region *is* a critical bug that can and will cause problems in 99% of the cases. Rather fix the asserts that got triggered instead of backing out useful debugging checks ...

Quoted 4 lines · Ingo Molnar

it *is* a great debugging check, at zero added cost. Scheduling from an atomic region *is* a critical bug that can and will cause problems in 99% of the cases. Rather fix the asserts that got triggered instead of backing out useful debugging checks ...

There are a lot of shitty drivers that this is going to catch. Yes, that is great... but we cannot BUG(). There really are a LOT of them. In the least, we need to show_trace().

Anyhow, this currently will catch _all_ kernel preemptions because the PREEMPT_ACTIVE flag is set.

We need to do something like the attached (untested), at the very least...

I would prefer to make this a kernel debugging check, however. Or, make using kernel preemption the default. Using the "free" abilities of kernel preemption is great, but not at the expense of its users.

Robert Love

diff -urN linux-2.5.34/kernel/sched.c linux/kernel/sched.c

kernel/sched.c +4 -2

@@ -940,8 +940,10 @@
 	struct list_head *queue;
 	int idx;
 
-	if (unlikely(in_atomic()))
-		BUG();
+	if (unlikely(in_atomic() && preempt_count() != PREEMPT_ACTIVE)) {
+		printk(KERN_ERROR "schedule() called while non-atomic!\n");
+		show_stack(NULL);
+	}
 
 #if CONFIG_DEBUG_HIGHMEM
 	check_highmem_ptes();

Quoted 4 lines

it *is* a great debugging check, at zero added cost. Scheduling from an atomic region *is* a critical bug that can and will cause problems in 99% of the cases. Rather fix the asserts that got triggered instead of backing out useful debugging checks ...

There are a lot of shitty drivers that this is going to catch. [...]

[...] Yes, that is great... but we cannot BUG(). There really are a LOT of them. In the least, we need to show_trace().

of course. And your point in making it in_interrupt() had what purpose - hiding that tons of code breaks preemption? [and tons of code breaks on SMP.] Your patch was removing precisely the tool that can be used to improve SMP quality on UP boxes as well.

yes. And we also need kallsyms and kksymoops in the kernel, so that people can send in meaningful traces.

On Fri, 2002-09-13 at 03:36, Robert Love wrote:
Actually, looking at this again, we probably want to still BUG() if in_interrupt() but _not_ if in_atomic().

Quoted 4 lines · Robert Love

While this sounds like a great debugging check, it is not useful in general since we surely have some bad code that calls schedule() with locks held. Further, since the atomic accounting only includes locks if CONFIG_PREEMPT is set, you only see this with kernel preemption enabled.

it *is* a great debugging check, at zero added cost. Scheduling from an atomic region *is* a critical bug that can and will cause problems in 99% of the cases. Rather fix the asserts that got triggered instead of backing out useful debugging checks ...