Skip to contentSkip to conversation

block/IDE/interrupt lockup

PATCH Manfred Spraul 4 participants 5 messages Mar 30, 2002

lore
Share & tools
lore

Manfred Spraul

author
#0 lore raw

Review

Quoted 3 lines
(quoted diff) +1 -1
-	spin_unlock_irq(&io_request_lock);
+	spin_unlock_irqrestore(&io_request_lock, flags);
 	rq = kmem_cache_alloc(request_cachep, SLAB_KERNEL);

Great patch.
kmem_cache_alloc with SLAB_KERNEL can sleep, i.e. you've just converted an obvious bug into a rare, difficult to find bug. What about trying to fix it?

I agree that this won't happen during boot, but what about a hotplug PCI ide controller?

--
Manfred

#1 lore raw
Quoted 9 lines · Manfred Spraul

Review

Quoted 4 lines
(quoted diff) +1 -1
-     spin_unlock_irq(&io_request_lock);
+     spin_unlock_irqrestore(&io_request_lock, flags);
      rq = kmem_cache_alloc(request_cachep, SLAB_KERNEL);

Great patch.
kmem_cache_alloc with SLAB_KERNEL can sleep, i.e. you've just converted an obvious bug into a rare, difficult to find bug. What about trying to fix it?

Gimme a break, Manfred. The patch fixes the new bug. Which was hardly obvious. The longstanding (as in years-old) bug was pointed out to the maintainer.

It may not even be a bug. Certainly I don't think it's worth my time to fiddle with it. But you're at liberty to.

Quoted 2 lines

I agree that this won't happen during boot, but what about a hotplug PCI ide controller?

The kernel calls request_irq() inside cli() in lots of places. That's the same bug: "if you called cli(), how come you're allowing kmalloc to clear it?".

In 2.4, this is a design wart. In 2.5, it will go BUG() if the page allocator performs I/O.

-

#2 lore raw
Quoted 3 lines

The kernel calls request_irq() inside cli() in lots of places. That's the same bug: "if you called cli(), how come you're allowing kmalloc to clear it?".

Those places should if possible be fixed. I take patches. If we can get 2.4 to BUG() on those kmalloc violations and clean them up it sounds like progress

#3 lore raw
In reply to Andrew Morton
Quoted 8 lines · Alan Cox
Quoted 4 lines

The kernel calls request_irq() inside cli() in lots of places. That's the same bug: "if you called cli(), how come you're allowing kmalloc to clear it?".

Those places should if possible be fixed. I take patches. If we can get 2.4 to BUG() on those kmalloc violations and clean them up it sounds like progress

What I'd like is a debugging function `can_sleep()'. This is good for documentary purposes, and will catch bugs.

So kmalloc() would gain:

	if (gfp_flags & __GFP_WAIT)
		can_sleep();

can_sleep() would do the following:

- If CONFIG_PREEMPT, check the locking depth (minus BKL depth), whine if non-zero.

- If inside cli(), whine.

- If inside __cli(), also whine (not really a bug, but a design error).

- whining will include generation of a backtrace.

I suspect a 2.4 version would generate too many bug reports :) It would have to implement its own lock depth accounting if we want the sleep-inside-spinlock checking.

There's some arch-dependent stuff in there. I'll do a 2.5 patch. I suspect it'll generate showers of stuff. We can feed fixes back into 2.4.

-

#4 lore raw
Quoted 7 lines · Andrew Morton

What I'd like is a debugging function `can_sleep()'. This is good for documentary purposes, and will catch bugs.

So kmalloc() would gain:

	if (gfp_flags & __GFP_WAIT)
		can_sleep();

can_sleep_if(gfp_flags & __GFP_WAIT) would be better. can_sleep_if() is
do { } while(0)
for no debugging, for debugging it is
if (unlikely(condition)) {
whine(__stringify(condition))
}

One line instead of two, no references to variables when debugging is off, automatically adds unlikely.