start_kernel(): bug: interrupts were enabled early

On latest git, I'm seeing "start_kernel(): bug: interrupts were enabled early" messages on ARM (sample log below).

This appears to be caused by:

start_kernel -> radix_tree_init -> kmem_cache_create (slub) ->
down_write -> __down_write (lib/rwsem-spinlock.c) -> spin_unlock_irq

radix_tree_init was moved earlier by:

commit 773e3eb7b81e5ba13b5155dfb3bb75b8ce37f8f9
Author: Yinghai Lu <[email protected]>
Date: Wed Feb 10 01:20:33 2010 -0800

init: Move radix_tree_init() early

Prepare for using radix trees in early_irq_init().

Signed-off-by Yinghai Lu <[email protected]>

LKML-Reference: <[email protected]>

Signed-off-by H. Peter Anvin <[email protected]>

Quoted 14 lines · On Fri, 26 Mar 2010 01:11:00 +0530 Rabin Vincent <[email protected]> wrote:

Quoted 14 lines

On latest git, I'm seeing "start_kernel(): bug: interrupts were enabled early" messages on ARM (sample log below).

This appears to be caused by:

start_kernel -> radix_tree_init -> kmem_cache_create (slub) ->
down_write -> __down_write (lib/rwsem-spinlock.c) -> spin_unlock_irq

radix_tree_init was moved earlier by:

commit 773e3eb7b81e5ba13b5155dfb3bb75b8ce37f8f9
Author: Yinghai Lu <[email protected]>
Date: Wed Feb 10 01:20:33 2010 -0800

Quoted 8 lines

Quoted 8 lines

init: Move radix_tree_init() early

Prepare for using radix trees in early_irq_init().

Signed-off-by Yinghai Lu <[email protected]>

LKML-Reference: <[email protected]>

Signed-off-by H. Peter Anvin <[email protected]>

That's going to be hard to fix.

Once upon a time, enabling interrupts too early in boot would kill powerpc boxes stone dead. From the lack of noise I assume that this is not happening in current kernels for some reason.

We have two checks in start_kernel():

	if (!irqs_disabled()) {
		printk(KERN_WARNING "start_kernel(): bug: interrupts were "
				"enabled *very* early, fixing it\n");
		local_irq_disable();
	}
	rcu_init();
	radix_tree_init();
	/* init some links before init_ISA_irqs() */
	early_irq_init();
	init_IRQ();
	prio_tree_init();
	init_timers();
	hrtimers_init();
	softirq_init();
	timekeeping_init();
	time_init();
	profile_init();
	if (!irqs_disabled())
		printk(KERN_CRIT "start_kernel(): bug: interrupts were "
				 "enabled early\n");

perhaps the second one isn't needed? Perhaps no architecture requires that local interrupts be disabled across the above initialisations?

Quoted 7 lines

Quoted 5 lines

Quoted 5 lines

This appears to be caused by:

start_kernel -> radix_tree_init -> kmem_cache_create (slub) ->
down_write -> __down_write (lib/rwsem-spinlock.c) -> spin_unlock_irq

That's going to be hard to fix.

spin_unlock_irq from arm is different from other archs?

Quoted 5 lines · Yinghai Lu

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

This appears to be caused by:

start_kernel -> radix_tree_init -> kmem_cache_create (slub) ->
down_write -> __down_write (lib/rwsem-spinlock.c) -> spin_unlock_irq

Quoted 3 lines

Quoted 2 lines

That's going to be hard to fix.

spin_unlock_irq from arm is different from other archs?

Not all arches use lib/rwsem-spinlock.c. In particular, x86 doesn't when X86_XADD is set.

What I note is that lib/rwsem-spinlock.c seems to be rather inconsistent in its use of spin_lock_irqsave/spin_lock_irqrestore versus spin_lock_irq/spin_unlock_irq... in fact, __down_read is the *only* place where we use the latter as opposed to the former.

Is that a bug? If so, it would certainly explain this behavior.

Quoted 5 lines · H. Peter Anvin

What I note is that lib/rwsem-spinlock.c seems to be rather inconsistent in its use of spin_lock_irqsave/spin_lock_irqrestore versus spin_lock_irq/spin_unlock_irq... in fact, __down_read is the *only* place where we use the latter as opposed to the former.

Quoted 1 line

Is that a bug? If so, it would certainly explain this behavior.

It's based on down_read() and down_write() not being callable from interrupt context, or with interrupts disabled (since they can sleep). up_read(), up_write(), down_read_trylock(), down_write_trylock(), downgrade_write() can all be called from interrupt context since they cannot sleep.

Do not run the checks while we are in a single threaded context?

I thought we had some dynamic code patching thingamy that could change those when we go to smp mode?

Quoted 4 lines · Christoph Lameter

Do not run the checks while we are in a single threaded context?

I thought we had some dynamic code patching thingamy that could change those when we go to smp mode?

You have to remember that on embedded architectures, such as ARM, where XIP is supported we can't change the text segment at run time - which means dynamic code patching won't work.

However, the kernel should still work in such situations.

Quoted 10 lines · Russell King

Quoted 4 lines · Christoph Lameter

Do not run the checks while we are in a single threaded context?

I thought we had some dynamic code patching thingamy that could change those when we go to smp mode?

You have to remember that on embedded architectures, such as ARM, where XIP is supported we can't change the text segment at run time - which means dynamic code patching won't work.

Quoted 2 lines

However, the kernel should still work in such situations.

The question still remains what the incremental cost is of doing irqsave/irqrestore.

The question still remains what the incremental cost is of doing irqsave/irqrestore.

Quoted 3 lines · H. Peter Anvin

The question still remains what the incremental cost is of doing irqsave/irqrestore.

The only other option is to have local_irq_enable() check a global (system_state ?) before enabling. Almost as gross ...

But these things are all utterly gross. The bottom line is that radix_tree_init() is manifestly unsuited to being called with local interrupts disabled. 773e3eb7b81e5ba13b5155dfb3bb75b8ce37f8f9 was just a wrong patch.

Quoted 5 lines · Andrew Morton

But these things are all utterly gross. The bottom line is that radix_tree_init() is manifestly unsuited to being called with local interrupts disabled. 773e3eb7b81e5ba13b5155dfb3bb75b8ce37f8f9 was just a wrong patch.

Except that powerpc (and now it seems x86) both want to use radix trees for interrupt handling... At least on powerpc, we trick and use a linear search until the radix trees are initialized, which we do later during boot, but that somewhat sucks.

I believe sherry picking things like not calling radix_tree_init() is going to fix one case today, until we have another one, and another one, and etc...

I suspect we're better off fixing the root of the problem in down/up.

Not by adding overhead to every single down_read()/down_write() just to fix a once-off startup problem - that's taking laziness way too far.

Quoted 4 lines · Andrew Morton

Not by adding overhead to every single down_read()/down_write() just to fix a once-off startup problem - that's taking laziness way too far.

How much overhead is this on non-x86 architectures (keep in mind x86 doesn't use this?)

Just a few instructions, I guess. But we can do it with zero.

And from a design POV, pretending that down_read()/down_write() can be called with interrupts disabled is daft - they cannot! Why muck up the
usual code paths with this startup-specific hack?

Can we provide a kmem_cache_create_early()? One that takes no locks and gets cleaned up with the other __init stuff?

Can we provide a kmem_cache_create_early()? One that takes no locks and gets cleaned up with the other __init stuff?

Quoted 6 lines · Andrew Morton

Just a few instructions, I guess. But we can do it with zero.

And from a design POV, pretending that down_read()/down_write() can be called with interrupts disabled is daft - they cannot! Why muck up the
usual code paths with this startup-specific hack?

Because we the problem of when interrupts are enabled for the first time is a nasty one, and having entire layer of things not usable at the right level of init because somewhere something might do an irq enable due to calling generic code that down's a semaphore is a PITA.

Seriously, Andrew, I don't see a clean solution... Something -somewhere- will have to be ugly.

Allocation is a pretty basic service that a lot of stuff expect especially when booting.

We went through that discussion before when we moved the SLAB init earlier during boot, because it makes no sense to have tons of code to have to figure out what allocator to call depending on what phase of the moon it's called from (especially when said code can also be called later during boot, say for hotplug reasons).

So we moved sl*b init earlier, thus we ought to be able to also kmem_cache_alloc() earlier. We -fixed- that problem already afaik.

Quoted 4 lines · Andrew Morton

Not by adding overhead to every single down_read()/down_write() just to fix a once-off startup problem - that's taking laziness way too far.

How much overhead is this on non-x86 architectures (keep in mind x86 doesn't use this?)

Do not run the checks while we are in a single threaded context?

I thought we had some dynamic code patching thingamy that could change those when we go to smp mode?

Quoted 5 lines · Christoph Lameter

Do not run the checks while we are in a single threaded context?

I thought we had some dynamic code patching thingamy that could change those when we go to smp mode?

The problem isn't about checks. Those -will- enable IRQs before it's safe to do so, the checks are perfectly right to point it out, it -is- a bug waiting to happen on some random HW.

Quoted 23 lines · Andrew Morton

We have two checks in start_kernel():

	if (!irqs_disabled()) {
		printk(KERN_WARNING "start_kernel(): bug: interrupts were "
				"enabled *very* early, fixing it\n");
		local_irq_disable();
	}
	rcu_init();
	radix_tree_init();
	/* init some links before init_ISA_irqs() */
	early_irq_init();
	init_IRQ();
	prio_tree_init();
	init_timers();
	hrtimers_init();
	softirq_init();
	timekeeping_init();
	time_init();
	profile_init();
	if (!irqs_disabled())
		printk(KERN_CRIT "start_kernel(): bug: interrupts were "
				 "enabled early\n");

Quoted 2 lines

perhaps the second one isn't needed? Perhaps no architecture requires that local interrupts be disabled across the above initialisations?

spin_unlock_irq from arm is different from other archs?

We use the standard generic kernel implementation. Is x86 different? ;)

We use the standard generic kernel implementation. Is x86 different? ;)

Quoted 1 line · Russell King

We use the standard generic kernel implementation. Is x86 different? ;)

The optimised fast paths used on x86 rwsems don't disable interrupts.

Any reason not to use the same technique for all the archs

Quoted 4 lines · David Howells

Quoted 1 line · Russell King

We use the standard generic kernel implementation. Is x86 different? ;)

The optimised fast paths used on x86 rwsems don't disable interrupts.

Any reason not to use the same technique for all the archs - plus the trick used in arch/armkernel/entry-armv.S:__kuser_cmpxchg for those archs which don't have atomic instructions or ll/sc?

If the problem here is _only_ semaphores, and the above might make semaphores faster anyway, perhaps it's a solution.

spin_unlock_irq from arm is different from other archs?

Quoted 1 line · On Wed, 31 Mar 2010 13:47:23 -0700 Yinghai Lu <[email protected]> wrote:

spin_unlock_irq from arm is different from other archs?

No, spin_unlock_irq() unconditionally enables interrupts on all architectures.

Quoted 7 lines · Andrew Morton

Quoted 1 line · On Wed, 31 Mar 2010 13:47:23 -0700 Yinghai Lu <[email protected]> wrote:

spin_unlock_irq from arm is different from other archs?

No, spin_unlock_irq() unconditionally enables interrupts on all architectures.

So I found checkin 60ba96e546da45d9e22bb04b84971a25684e4d46 in the bk-historic git tree:

[PATCH] rwsem: Make rwsems use interrupt disabling spinlocks

The attached patch makes read/write semaphores use interrupt disabling spinlocks in the slow path, thus rendering the up functions and trylock functions available for use in interrupt context. This matches the regular semaphore behaviour.

I've assumed that the normal down functions must be called with interrupts enabled (since they might schedule), and used the irq-disabling spinlock variants that don't save the flags.

Signed-off-by David Howells <[email protected]>

Tested-by Badari Pulavarty <[email protected]>

Signed-off-by Linus Torvalds <[email protected]>

What we have here is a case of this assumption being violated, because the lock is taken with interrupts disabled on a path where contention cannot happen (because the code is single-threaded at this point), but the lock is taken due to reuse of generic code.

The obvious way to fix this would be to use spin_lock_irqsave..spin_lock_irqrestore in __down_read as well as in the other locations; I don't have a good feel for what the cost of doing so would be, though. On x86 it's fairly expensive simply because the only way to save the state is to push it on the stack, which the compiler doesn't deal well with, but this code isn't used on x86.

Well, it's all a bit nasty. kmem_cache_create() does a lot of stuff, including calling into the page allocator with GFP_KERNEL - expecting kmem_cache_create() to preserve local_irq_disable() is a bit optimistic.

radix_tree_init() calls hotcpu_notifier() which also does mutex_lock(&cpu_add_remove_lock);

The easiest fix is to reposition the interrutps-are-now-enabled point in start_kernel(). But I have a feeling that some versions of early_irq_init() won't like that.

The obvious way to fix this would be to use spin_lock_irqsave..spin_lock_irqrestore in __down_read as well as in the other locations; I don't have a good feel for what the cost of doing so would be, though. On x86 it's fairly expensive simply because the only way to save the state is to push it on the stack, which the compiler doesn't deal well with, but this code isn't used on x86.

Quoted 7 lines · H. Peter Anvin

The obvious way to fix this would be to use spin_lock_irqsave..spin_lock_irqrestore in __down_read as well as in the other locations; I don't have a good feel for what the cost of doing so would be, though. On x86 it's fairly expensive simply because the only way to save the state is to push it on the stack, which the compiler doesn't deal well with, but this code isn't used on x86.

I think that's what we should just do, with a good comment both in the code and the changelog. I'm not entirely happy with it, because obviously it's conceptually kind of dubious to take a lock with interrupts disabled in the first place, but this is not a new issue per se.

The whole bootup code is special, and we already make similar guarantees about memory allocators and friends - just because it's too dang painful to have some special code that does GFP_ATOMIC for early bootup when the same code is often shared and used at run-time too.

So we've accepted that people can do GFP_KERNEL allocations and we won't care about them if we're in the boot phase (and suspend/resume), and we have that whole 'gfp_allowed_mask' thing for that.

I think this probably falls under exactly the same heading of "not pretty, but let's not blow up".

So making the slow-path do the spin_[un]lock_irq{save,restore}() versions sounds like the right thing. It won't be a performance issue: it _is_ the slow-path, and we're already doing the expensive part (the spinlock itself and the irq thing).

So ACK on the idea. Who wants to write the trivial patch and test it? Preferably somebody who sees the problem in the first place - x86 should not be impacted, since the irq-disabling slow-path should never be hit without contention anyway (and contention cannot/mustnot happen for this case).

Linus

Quoted 5 lines

So making the slow-path do the spin_[un]lock_irq{save,restore}() versions sounds like the right thing. It won't be a performance issue: it _is_ the slow-path, and we're already doing the expensive part (the spinlock itself and the irq thing).

It's actually on the fastpath for lib/rwsem-spinlock.c.

Ahh, yes. In this case, that doesn't likely change anything. The save/restore versions of the irq-safe locks shouldn't be appreciably more expensive than the non-saving ones. And architectures that really care should have done their own per-arch optimized version anyway.

Maybe we should even document that - so that nobody else makes the mistake x86-64 did of thinking that the "generic spinlock" version of the rwsem's is anything but a hacky and bad fallback case.

Quoted 4 lines · Linus Torvalds

Ahh, yes. In this case, that doesn't likely change anything. The save/restore versions of the irq-safe locks shouldn't be appreciably more expensive than the non-saving ones. And architectures that really care should have done their own per-arch optimized version anyway.

That depends on the CPU. Some CPUs have quite expensive interrupt disablement instructions. FRV does for instance; but fortunately, on the FRV, I can use some of the excessive quantities of conditional registers to pretend that I disable interrupts, and only actually do so if an interrupt actually happens.

Quoted 3 lines

Maybe we should even document that - so that nobody else makes the mistake x86-64 did of thinking that the "generic spinlock" version of the rwsem's is anything but a hacky and bad fallback case.

In some cases, it's actually the best way. On a UP machine, for instance, where they reduce to nothing or where your only atomic instruction is an XCHG equivalent.

Quoted 7 lines · H. Peter Anvin

The obvious way to fix this would be to use spin_lock_irqsave..spin_lock_irqrestore in __down_read as well as in the other locations; I don't have a good feel for what the cost of doing so would be, though. On x86 it's fairly expensive simply because the only way to save the state is to push it on the stack, which the compiler doesn't deal well with, but this code isn't used on x86.

So making the slow-path do the spin_[un]lock_irq{save,restore}() versions sounds like the right thing. It won't be a performance issue: it _is_ the slow-path, and we're already doing the expensive part (the spinlock itself and the irq thing).

So ACK on the idea. Who wants to write the trivial patch and test it?

Quoted 9 lines · Linus Torvalds

Quoted 7 lines · H. Peter Anvin

The obvious way to fix this would be to use spin_lock_irqsave..spin_lock_irqrestore in __down_read as well as in the other locations; I don't have a good feel for what the cost of doing so would be, though. On x86 it's fairly expensive simply because the only way to save the state is to push it on the stack, which the compiler doesn't deal well with, but this code isn't used on x86.

[...]

Quoted 5 lines

So making the slow-path do the spin_[un]lock_irq{save,restore}() versions sounds like the right thing. It won't be a performance issue: it _is_ the slow-path, and we're already doing the expensive part (the spinlock itself and the irq thing).

Quoted 1 line

So ACK on the idea. Who wants to write the trivial patch and test it?

OK, I'll bite since I was seeing boot-time hangs on ARM (TI OMAP3) due to this. Patch below.

Kevin

From: Kevin Hilman <[email protected]>
Date: Wed, 7 Apr 2010 11:52:46 -0700
Subject: [PATCH] rwsem generic spinlock: use IRQ save/restore spinlocks

rwsems can be used with IRQs disabled, particularily in early boot before IRQs are enabled. Currently the spin_unlock_irq() usage in the slow-patch will unconditionally enable interrupts and cause problems since interrupts are not yet initialized or enabled.

This patch uses save/restore versions of IRQ spinlocks in the slowpath to ensure interrupts are not unintentionally disabled.

Signed-off-by Kevin Hilman <[email protected]>

---
lib/rwsem-spinlock.c | 14 ++++++++------

1 files changed, 8 insertions(+), 6 deletions(-)

lib/rwsem-spinlock.c +8 -6

index ccf95bf..ffc9fc7 100644
@@ -143,13 +143,14 @@ void __sched __down_read(struct rw_semaphore *sem)
 {
 	struct rwsem_waiter waiter;
 	struct task_struct *tsk;
+	unsigned long flags;
 
-	spin_lock_irq(&sem->wait_lock);
+	spin_lock_irqsave(&sem->wait_lock, flags);
 
 	if (sem->activity >= 0 && list_empty(&sem->wait_list)) {
 		/* granted */
 		sem->activity++;
-		spin_unlock_irq(&sem->wait_lock);
+		spin_unlock_irqrestore(&sem->wait_lock, flags);
 		goto out;
 	}
 

@@ -164,7 +165,7 @@ void __sched __down_read(struct rw_semaphore *sem)
 	list_add_tail(&waiter.list, &sem->wait_list);
 
 	/* we don't need to touch the semaphore struct anymore */
-	spin_unlock_irq(&sem->wait_lock);
+	spin_unlock_irqrestore(&sem->wait_lock, flags);
 
 	/* wait to be given the lock */
 	for (;;) {

@@ -209,13 +210,14 @@ void __sched __down_write_nested(struct rw_semaphore *sem, int subclass)
 {
 	struct rwsem_waiter waiter;
 	struct task_struct *tsk;
+	unsigned long flags;
 
-	spin_lock_irq(&sem->wait_lock);
+	spin_lock_irqsave(&sem->wait_lock, flags);
 
 	if (sem->activity == 0 && list_empty(&sem->wait_list)) {
 		/* granted */
 		sem->activity = -1;
-		spin_unlock_irq(&sem->wait_lock);
+		spin_unlock_irqrestore(&sem->wait_lock, flags);
 		goto out;
 	}
 

@@ -230,7 +232,7 @@ void __sched __down_write_nested(struct rw_semaphore *sem, int subclass)
 	list_add_tail(&waiter.list, &sem->wait_list);
 
 	/* we don't need to touch the semaphore struct anymore */
-	spin_unlock_irq(&sem->wait_lock);
+	spin_unlock_irqrestore(&sem->wait_lock, flags);
 
 	/* wait to be given the lock */
 	for (;;) {

Signature · 7 lines

1.7.0.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Quoted 1 line

perhaps the second one isn't needed? Perhaps no architecture

requires

Quoted 1 line

that local interrupts be disabled across the above initialisations?

spin_unlock_irq from arm is different from other archs?

Quoted 5 lines · Yinghai Lu

Quoted 1 line

perhaps the second one isn't needed? Perhaps no architecture

requires

Quoted 1 line

that local interrupts be disabled across the above initialisations?

spin_unlock_irq from arm is different from other archs?

No, it's not, it will enable IRQs and thats illegal to do so early during boot. We've been over that one again and again, the problem is that people want to keep using that instead of irqsave/restore because it's a nano-optimisation on x86... oh well...