Re: BUG or not? GFP_KERNEL with interrupts disabled.

On Thu, 27 Mar 2003, Linus Torvalds wrote:

On Thu, 27 Mar 2003, David S. Miller wrote:

quoted

Ok, so can we add a:

	if (irqs_disabled())
		BUG();

check to do_softirq()?

I'd suggest making it a counting warning (with a static counter per
local-bh-enable macro expansion) and adding it to local_bh_enable() -
otherwise it will only BUG()  when the (potentially rare) condition
happens - instead of always giving a nice backtrace of exact problem 
spots.

So, to return to my original question...  local_bh_count() > 0 when 
a BH is running or after local_bh_disable().  local_irq_count() > 0 in 
interrupt context, but not necessarily when interrupts are disabled.

This makes checks like the following (in alloc_skb) asymmetric:

    if (in_interrupt() && (gfp_mask & __GFP_WAIT)) {
        static int count = 0;
        if (++count < 5) {
            printk(KERN_ERR "alloc_skb called nonatomically "
                   "from interrupt %p\n", NET_CALLER(size));
            BUG();

In a driver I'm writing, this bug was hidden until I switched from using
write_lock_irqsave() to write_lock_bh().  Shouldn't this bug also be
announced if interrupts are disabled?  (I understand that disabling bh/irq
in the correct order will ensure that this bug is properly detected, but
it seems like a strange policy to rely on correct coding to catch a bug.)

-- 
Dan Eble [off-list ref]  _____  .
                           |  _  |/|
Applied Innovation Inc.    | |_| | |
http://www.aiinet.com/     |__/|_|_|

On Thu, 27 Mar 2003, Dan Eble wrote:

This makes checks like the following (in alloc_skb) asymmetric:

    if (in_interrupt() && (gfp_mask & __GFP_WAIT)) {
        static int count = 0;
        if (++count < 5) {
            printk(KERN_ERR "alloc_skb called nonatomically "
                   "from interrupt %p\n", NET_CALLER(size));
            BUG();

In a driver I'm writing, this bug was hidden until I switched from using
write_lock_irqsave() to write_lock_bh().  Shouldn't this bug also be
announced if interrupts are disabled?

Yeah. It should also probably use "in_atomic()" instead of 
"in_interrupt()", since that also finds people who have marked themselves 
non-preemptible.

So what the test SHOULD look like is this:

	if (gfp_mask & __GFP_WAIT) {
		if (in_atomic() || irqs_disabled()) {
			static int count = 0;
			...
		}
	}

which should catch all the cases we really care about.

		Linus

   From: Linus Torvalds [off-list ref]
   Date: Thu, 27 Mar 2003 11:08:26 -0800 (PST)

   So what the test SHOULD look like is this:
   
   	if (gfp_mask & __GFP_WAIT) {
   		if (in_atomic() || irqs_disabled()) {
   			static int count = 0;
   			...
   		}
   	}
   
   which should catch all the cases we really care about.

Let's codify this "in_atomic() || irqs_disabled()" test into a macro
that everyone can use to test sleepability, ok?

On Thu, 27 Mar 2003, David S. Miller wrote:

Let's codify this "in_atomic() || irqs_disabled()" test into a macro
that everyone can use to test sleepability, ok?

Well, I really don't want people to act dynamically differently depending 
on whether they can sleep or not. That makes static sanity-testing 
impossible. So I really think that the only really valid use of the above 
is on one single place: might_sleep().

Which right now doesn't do the "irqs_disabled()" test, but otherwise looks 
good. So the code should really just say

	if (gfp_mask & __GFP_WAIT)
		might_sleep();

and might_sleep() should be updated.

Anybody want to try that and see whether things break horribly?

		Linus

   From: Linus Torvalds [off-list ref]
   Date: Thu, 27 Mar 2003 11:22:55 -0800 (PST)

   	if (gfp_mask & __GFP_WAIT)
   		might_sleep();
   
   and might_sleep() should be updated.
   
   Anybody want to try that and see whether things break horribly?

I hadn't considered this, good idea.  I'm trying this out right now.

Someone should backport the might_sleep() stuff to 2.4.x, it's very
useful.

On Thu, 2003-03-27 at 14:39, David S. Miller wrote:

I hadn't considered this, good idea.  I'm trying this out right now.

I hope it works.  I have a sinking feeling we call it some places that
may have interrupts disabled...

Someone should backport the might_sleep() stuff to 2.4.x, it's very
useful.

Would be nice, but for the maximum effect we need kernel preemption
(which keeps track of atomicity via preempt_count).  I doubt we want to
go there for 2.4.

	Robert Love

   From: Robert Love [off-list ref]
   Date: 27 Mar 2003 14:52:11 -0500

   On Thu, 2003-03-27 at 14:39, David S. Miller wrote:
   
   > I hadn't considered this, good idea.  I'm trying this out right now.
   
   I hope it works.  I have a sinking feeling we call it some places that
   may have interrupts disabled...

Your sinking feeling was warranted.

Nearly every hw IRQ implementation invokes irq_exit() with
CPU interrupts off :-(  That has to be screwing with performance
as well.

   From: "David S. Miller" [off-list ref]
   Date: Thu, 27 Mar 2003 11:39:33 -0800 (PST)

      From: Linus Torvalds [off-list ref]
      Date: Thu, 27 Mar 2003 11:22:55 -0800 (PST)
   
      	if (gfp_mask & __GFP_WAIT)
      		might_sleep();
      
      and might_sleep() should be updated.
      
      Anybody want to try that and see whether things break horribly?
   
   I hadn't considered this, good idea.  I'm trying this out right now.
   
Ok, I'm running this now and it appears to work.

i386 will need similar changes to it's irq_exit() call sites.

One might_sleep still triggers, the cpufreq_register_notifier() call
during boot.  It takes a rwsem.  This will trigger on i386 too with
TSC and CPUFREQ both enabled.

Oh yeah, the fbcon cursor thing triggers too, but that's been
discussed to death in another thread and hopefully a fix will
be pushed upstream soon by the fbcon guys.

--- ./arch/sparc64/kernel/smp.c.~1~	Thu Mar 27 11:57:41 2003
+++ ./arch/sparc64/kernel/smp.c	Thu Mar 27 12:05:46 2003

@@ -1055,11 +1055,10 @@ void smp_percpu_timer_interrupt(struct p
 		clear_softint(tick_mask);
 	}
 
+	irq_enter();
 	do {
 		sparc64_do_profile(regs);
 		if (!--prof_counter(cpu)) {
-			irq_enter();
-
 			if (cpu == boot_cpu_id) {
 				kstat_cpu(cpu).irqs[0]++;
 				timer_tick_interrupt(regs);

@@ -1067,7 +1066,6 @@ void smp_percpu_timer_interrupt(struct p
 
 			update_process_times(user);
 
-			irq_exit();
 
 			prof_counter(cpu) = prof_multiplier(cpu);
 		}

@@ -1088,6 +1086,9 @@ void smp_percpu_timer_interrupt(struct p
 				     : /* no outputs */
 				     : "r" (pstate));
 	} while (time_after_eq(tick, compare));
+
+	local_irq_enable();
+	irq_exit();
 }
 
 static void __init smp_setup_percpu_timer(void)
--- ./arch/sparc64/kernel/irq.c.~1~	Thu Mar 27 11:57:41 2003
+++ ./arch/sparc64/kernel/irq.c	Thu Mar 27 12:42:13 2003

@@ -356,7 +356,7 @@ int request_irq(unsigned int irq, void (
 	}	
 	if (action == NULL)
 	    action = (struct irqaction *)kmalloc(sizeof(struct irqaction),
-						 GFP_KERNEL);
+						 GFP_ATOMIC);
 	
 	if (!action) { 
 		spin_unlock_irqrestore(&irq_action_lock, flags);

@@ -376,7 +376,7 @@ int request_irq(unsigned int irq, void (
 				goto free_and_ebusy;
 			}
 			if ((bucket->flags & IBF_MULTI) == 0) {
-				vector = kmalloc(sizeof(void *) * 4, GFP_KERNEL);
+				vector = kmalloc(sizeof(void *) * 4, GFP_ATOMIC);
 				if (vector == NULL)
 					goto free_and_enomem;
 

@@ -793,6 +793,7 @@ void handler_irq(int irq, struct pt_regs
 
 		bp->flags &= ~IBF_INPROGRESS;
 	}
+	local_irq_enable();
 	irq_exit();
 }
 

@@ -900,7 +901,7 @@ int request_fast_irq(unsigned int irq,
 	}
 	if (action == NULL)
 		action = (struct irqaction *)kmalloc(sizeof(struct irqaction),
-						     GFP_KERNEL);
+						     GFP_ATOMIC);
 	if (!action) {
 		spin_unlock_irqrestore(&irq_action_lock, flags);
 		return -ENOMEM;
--- ./arch/sparc64/kernel/traps.c.~1~	Thu Mar 27 12:13:23 2003
+++ ./arch/sparc64/kernel/traps.c	Thu Mar 27 12:15:53 2003

@@ -1575,6 +1575,9 @@ void show_trace_raw(struct thread_info *
 	struct reg_window *rw;
 	int count = 0;
 
+	if (tp == current_thread_info())
+		flushw_all();
+
 	fp = ksp + STACK_BIAS;
 	thread_base = (unsigned long) tp;
 	do {

@@ -1595,6 +1598,15 @@ void show_trace_task(struct task_struct 
 	if (tsk)
 		show_trace_raw(tsk->thread_info,
 			       tsk->thread_info->ksp);
+}
+
+void dump_stack(void)
+{
+	unsigned long ksp;
+
+	__asm__ __volatile__("mov	%%fp, %0"
+			     : "=r" (ksp));
+	show_trace_raw(current_thread_info(), ksp);
 }
 
 void die_if_kernel(char *str, struct pt_regs *regs)
--- ./kernel/sched.c.~1~	Thu Mar 27 11:27:01 2003
+++ ./kernel/sched.c	Thu Mar 27 11:27:41 2003

@@ -2554,7 +2554,7 @@ void __might_sleep(char *file, int line)
 #if defined(in_atomic)
 	static unsigned long prev_jiffy;	/* ratelimiting */
 
-	if (in_atomic()) {
+	if (in_atomic() || irqs_disabled()) {
 		if (time_before(jiffies, prev_jiffy + HZ))
 			return;
 		prev_jiffy = jiffies;
--- ./kernel/softirq.c.~1~	Thu Mar 27 11:28:20 2003
+++ ./kernel/softirq.c	Thu Mar 27 11:52:35 2003

@@ -60,6 +60,9 @@ asmlinkage void do_softirq()
 	if (in_interrupt())
 		return;
 
+	if (irqs_disabled())
+		BUG();
+
 	local_irq_save(flags);
 	cpu = smp_processor_id();
 
--- ./net/core/skbuff.c.~1~	Thu Mar 27 11:28:53 2003
+++ ./net/core/skbuff.c	Thu Mar 27 11:29:12 2003

@@ -170,15 +170,8 @@ struct sk_buff *alloc_skb(unsigned int s
 	struct sk_buff *skb;
 	u8 *data;
 
-	if (in_interrupt() && (gfp_mask & __GFP_WAIT)) {
-		static int count;
-		if (++count < 5) {
-			printk(KERN_ERR "alloc_skb called nonatomically "
-			       "from interrupt %p\n", NET_CALLER(size));
- 			BUG();
-		}
-		gfp_mask &= ~__GFP_WAIT;
-	}
+	if (gfp_mask & __GFP_WAIT)
+		might_sleep();
 
 	/* Get the HEAD */
 	skb = skb_head_from_pool();

   From: "David S. Miller" [off-list ref]
   Date: Thu, 27 Mar 2003 12:55:07 -0800 (PST)

Alexey has pointed out a bug in my changes.

   @@ -1088,6 +1086,9 @@ void smp_percpu_timer_interrupt(struct p
    				     : /* no outputs */
    				     : "r" (pstate));
    	} while (time_after_eq(tick, compare));
   +
   +	local_irq_enable();
   +	irq_exit();
    }
    
    static void __init smp_setup_percpu_timer(void)

Of course this is bogus.

The IRQ enable needs to occur in the irq_exit() branch right
before do_softirq() is invoked.