Re: BUG or not? GFP_KERNEL with interrupts disabled.

Re: BUG or not? GFP_KERNEL with interrupts disabled.

[shmulik.hen]

Further more, holding a lock_irq doesn't mean bottom halves are disabled
too, it just means interrupts are disabled and no *new* softirq can be
queued. Consider the following situation:

In bond_release() we hold write_lock_irqsave(&bond->lock, flags) and then
do all the releasing stuff. If, for example, we need to call
dev_mc_upload() for the released slave, the following will happen

        spin_lock_bh(&dev->xmit_lock);
        __dev_mc_upload(dev);
        spin_unlock_bh(&dev->xmit_lock);

spin_unlock_bh() calls local_bh_enable() which checks local_bh_count. If
local_bh_count reaches zero (and it does), it directly executes
do_softirq(). The check for in_interrupt() in do_softirq() is false and
the softirqs that were queued begin to run and process the Tx and Rx
backlogs. dev_queue_xmit() is called on the bond device which calls, lets
say, bond_xmit_xor(). The first thing bond_xmit_xor() does is try to grab
read_lock_irqsave(&bond->lock, flags). Since this lock is already held by
bond_release(), and we're on the same cpu without any context switch,
we've got ourselves a deadlock. This actually happened to us and it took us
a while to figure the system halt, but we've got the kdb trace to prove
it.

Specifically for bonding, as stated by Dan below, it is indeed not
necessary to hold a lock_irq in every entry point in the driver. From our
experience in previous projects, we discovered that it is sufficient to
just grab a read_lock when accessing the slaves list in any softirq level
function (receive, transmit and timer), and hold a write_lock_bh() only
when changing the slaves list in ioctl calls like bond_enslave(),
bond_release(), bond_release_all() which all run at user context.

We have created a version that uses the above scheme that is being tested
by our QA group these days. Such a major change in the locking scheme
requires allot of testing to try and detect potential hidden bugs and
corner cases. We expect this will also increase the total throughput,
since interrupts won't be blocked each time a packet is being transmitted
or the miimon timer pops. We believe we will be able to post the patch
(+results) next week.


On Tue, 25 Mar 2003, Dan Eble wrote:
> 
> (kernel is ppc 2.4.21-pre4)
> 
> In bond_enslave() [drivers/net/bonding.c]:
> 
>         write_lock_irqsave(&bond->lock, flags);
>         ...
>         err = netdev_set_master(slave_dev, master_dev);
>         ...
>         write_unlock_irqrestore(&bond->lock, flags);
> 
> In netdev_set_master() [net/core/dev.c]:
> 
>         rtmsg_ifinfo(RTM_NEWLINK, slave, IFF_SLAVE);
> 
> In rtmsg_ifinfo() [net/core/rtnetlink.c]:
> 
>         skb = alloc_skb(size, GFP_KERNEL);
>         ...
>         netlink_broadcast(rtnl, skb, 0, RTMGRP_LINK, GFP_KERNEL);
> 
> Doesn't this admit the possibility of sleeping with interrupts disabled? 
> I found it because I'm working on a driver that uses a master-slave
> relationship like the bonding driver, and decided I didn't really need to
> disable interrupts, so I tried using write_lock_bh()  instead.  The
> result
> was an "alloc_skb called nonatomically from interrupt" message because
> write_lock_bh() increments the local BH count (which seems reasonable).
> 
> A bigger question: Why are the IRQ check and the BH check inconsistent?
> That is, local_bh_count() says "yes" if you are currently running in BH
> context OR have disabled BHs; however, local_irq_count() says "yes" if
> you
> are currently running in interrupt context, but it says nothing (as far
> as
> I have seen) about whether IRQs are enabled or disabled.  Is this (a) the
> Right Way, (b) something that's more trouble to fix than to be burned-by
> once and then avoid for the rest of your life, or (c) totally horked?
> 
> --
> Dan Eble <dane@aiinet.com>  _____  .
>                            |  _  |/|
> Applied Innovation Inc.    | |_| | |
> http://www.aiinet.com/     |__/|_|_|
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-net" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 

-- 
| Shmulik Hen                                    |
| Israel Design Center (Jerusalem)               |
| LAN Access Division                            |
| Intel Communications Group, Intel corp.        |

Re: BUG or not? GFP_KERNEL with interrupts disabled.

[David S. Miller]

   From: shmulik.hen@intel.com
   Date: Thu, 27 Mar 2003 15:32:02 +0200 (IST)

   Further more, holding a lock_irq doesn't mean bottom halves are disabled
   too, it just means interrupts are disabled and no *new* softirq can be
   queued. Consider the following situation:
   
I think local_bh_enable() should check irqs_disabled() and honour that.
What you are showing here, that BH's can run via local_bh_enable()
even when IRQs are disabled, is a BUG().

IRQ disabling is meant to be stronger than softint disabling.

Ingo/Linus?

Re: BUG or not? GFP_KERNEL with interrupts disabled.

[Trond Myklebust]

>>>>> " " == David S Miller <davem@redhat.com> writes:

     >    From: shmulik.hen@intel.com Date: Thu, 27 Mar 2003 15:32:02
     >    +0200 (IST)

     >    Further more, holding a lock_irq doesn't mean bottom halves
     >    are disabled too, it just means interrupts are disabled and
     >    no *new* softirq can be queued. Consider the following
     >    situation:
   
     > I think local_bh_enable() should check irqs_disabled() and
     > honour that.  What you are showing here, that BH's can run via
     > local_bh_enable() even when IRQs are disabled, is a BUG().

     > IRQ disabling is meant to be stronger than softint disabling.

In that case, you'll need to have things like spin_lock_irqrestore()
call local_bh_enable() in order to run the pending softirqs. Is that
worth the trouble?

Cheers,
  Trond

Re: BUG or not? GFP_KERNEL with interrupts disabled.

[David S. Miller]

   From: Trond Myklebust <trond.myklebust@fys.uio.no>
   Date: 27 Mar 2003 15:11:56 +0100

        > IRQ disabling is meant to be stronger than softint disabling.
   
   In that case, you'll need to have things like spin_lock_irqrestore()
   call local_bh_enable() in order to run the pending softirqs. Is that
   worth the trouble?

"trouble" is a weird word to use when the current behavior is
just wrong. :-)

My point is that it doesn't matter what the fix is, running
softints while hw IRQs are disabled must be fixed.

Re: BUG or not? GFP_KERNEL with interrupts disabled.

[Linus Torvalds]

On Thu, 27 Mar 2003, David S. Miller wrote:
> 
>    Further more, holding a lock_irq doesn't mean bottom halves are disabled
>    too, it just means interrupts are disabled and no *new* softirq can be
>    queued. Consider the following situation:
>    
> I think local_bh_enable() should check irqs_disabled() and honour that.
> What you are showing here, that BH's can run via local_bh_enable()
> even when IRQs are disabled, is a BUG().

I'd disagree.

I do agree that we should obviously not run bottom halves with interrupts 
disabled, but I think the _real_ bug is doing "local_bh_enable()" in the 
first place. It's a nesting bug: you must nest the "stronger" lock inside 
the weaker one, which means that the following is right:

	local_bh_disable()
		..
		local_irq_disable()
		...
		local_irq_enable()
		..
	local_bh_enable()

and this is WRONG:

	local_irq_disable() (or spinlock)
		..
		local_bh_disable()
		..
		local_bh_enable()	!BUG BUG BUG!
		..
	local_irq_enable()

So the bug is, in my opinion, not in BK handling, but in the caller.

I missed the start of this thread, so I don't know how hard this is to 
fix. But if you have a buggy sequence, the _simple_ fix may be to do 
somehting like this:

+++	local_bh_disable()
	local_irq_disable() (or spinlock)
		..
		local_bh_disable()
		..
		local_bh_enable()	! now it's a no-op and no longer a bug
		..
	local_irq_enable()
+++	local_bh_enable()

What's the code sequence?

		Linus

Re: BUG or not? GFP_KERNEL with interrupts disabled.

[David S. Miller]

   From: Linus Torvalds <torvalds@transmeta.com>
   Date: Thu, 27 Mar 2003 09:22:29 -0800 (PST)

   I do agree that we should obviously not run bottom halves with
   interrupts disabled
   
Ok, so can we add a:

	if (irqs_disabled())
		BUG();

check to do_softirq()?

I'll address the rest of your email in a bit.

Re: BUG or not? GFP_KERNEL with interrupts disabled.

[Linus Torvalds]

On Thu, 27 Mar 2003, David S. Miller wrote:
> 
>    I do agree that we should obviously not run bottom halves with
>    interrupts disabled
>    
> 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.

		Linus

Re: BUG or not? GFP_KERNEL with interrupts disabled.

[David S. Miller]

   From: Linus Torvalds <torvalds@transmeta.com>
   Date: Thu, 27 Mar 2003 10:04:52 -0800 (PST)

   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.

Ok, maybe it's time to move local_bh_enable() out of line, it's
getting large and it's expanded in hundreds of places.

udp_recvmsg: possible bug causing infinite hang?

[Chad N. Tindel]

I've encountered a hang condition during testing that only appeared when
we upgraded from Redhat EL 3 Update 2 to Redhat EL 3 Update 3.  After
looking at the differences, it appears to be caused by a change to
udp_recvmsg that also appears to have filtered back into the main kernel
tree, so it is possible that more people would be affected by this than
just redhat users.  Anyway, here is the scenario:

User space code sends a datagram on a blocking socket, and then calls
select() or poll() to wait for the reply.  When that pops with a non-error
condition (so we _know_ there is data to be read), recvfrom() is called.
Now, assume that somewhere along the way (it doesn't really matter where)
the UDP packet is corrupted.  Also, assume that no further inbound
datagrams are destined for this socket.  The new udp_recvmsg() will get
down to the bottom, and then will go to the try_again label, where it will
block forever in skb_recv_datagram() waiting for a datagram that will
never come.

The old code used to not have this try_again case, and so would always
just return immediately.

While this is a general problem for any program that uses UDP and relies
on the fact that select popped to insure that recvfrom won't hang, the
place where we always see it is in the DNS lookup portion of glibc.  The
send_dg() function is what actually hangs.

My questions are:

1.  Why was the code changed in this way?
2.  Is this a bug?  It seems so to me, because select (or poll)
specifically says there is data to read, and then it hangs when we try to
read it.  But, without the context of #1, it is hard to make this
determination.

Thanks,

Chad

Re: udp_recvmsg: possible bug causing infinite hang?

[David S. Miller]

On Sat, 9 Oct 2004 18:38:58 -0700 (PDT)
"Chad N. Tindel" <ctindel@falcon.csc.calpoly.edu> wrote:

> User space code sends a datagram on a blocking socket, and then calls
> select() or poll() to wait for the reply.

If you wish the socket not to block, mark the file descriptor
as non-blocking or pass in the appropriate MSG_* flag into
recvmsg().

Select() returning that a filedescriptor is readable does not
guarentee that a blocking socket will not block in the ead
call.

This behavior has been in the Linux kernel for an enourmous
amount of them, the change you are noticing going from EL 3 update
2 to update 3 is that previously we were returning -EAGAIN to
blocking sockets, instead we are properly blocking to wait for
another packet.

There is an enormous and long thread about this topic on the
linux-kernel list, please read there before we duplicate such
a long and tiring thread here.

Re: udp_recvmsg: possible bug causing infinite hang?

[Chad N. Tindel]

On Sat, Oct 09, 2004 at 06:45:03PM -0700, David S. Miller wrote:
> On Sat, 9 Oct 2004 18:38:58 -0700 (PDT)
> "Chad N. Tindel" <ctindel@falcon.csc.calpoly.edu> wrote:
> 
> > User space code sends a datagram on a blocking socket, and then calls
> > select() or poll() to wait for the reply.
> 
> If you wish the socket not to block, mark the file descriptor
> as non-blocking or pass in the appropriate MSG_* flag into
> recvmsg().
> 
> Select() returning that a filedescriptor is readable does not
> guarentee that a blocking socket will not block in the ead
> call.
> 
> This behavior has been in the Linux kernel for an enourmous
> amount of them, the change you are noticing going from EL 3 update
> 2 to update 3 is that previously we were returning -EAGAIN to
> blocking sockets, instead we are properly blocking to wait for
> another packet.
> 
> There is an enormous and long thread about this topic on the
> linux-kernel list, please read there before we duplicate such
> a long and tiring thread here.

OK, so what you're saying is that the C-library has a bug because it isn't 
using a non-blocking socket when doing DNS lookups?

Thanks,

Chad

Re: udp_recvmsg: possible bug causing infinite hang?

[Nagendra Singh Tomar]

On Sat, 9 Oct 2004, David S. Miller wrote:

> Select() returning that a filedescriptor is readable does not
> guarentee that a blocking socket will not block in the ead
> call.
> 

Does the POSIX standard _not_ give this implicit guarantee ? Whats the 
point in having a select call that says that the fd is readable, and when 
I go to read it the blocking read call blocks. Then either select returned 
a wrong positive or the read is buggy. 
	The kernel should mark the socket readable (and hence select returns 
"readable") only after it has added the incoming data to the socket receive 
queue or it has changed the state of the socket (in case of TCP connections 
getting closed), in case of peer closing the connection.
After that has been done, is the guarantee not there ?

Thanx,
tomar

-- You have moved the mouse. Windows must be restarted for the 
   changes to take effect.

Re: udp_recvmsg: possible bug causing infinite hang?

[Chad N. Tindel]

> Does the POSIX standard _not_ give this implicit guarantee ? Whats the 
> point in having a select call that says that the fd is readable, and when 
> I go to read it the blocking read call blocks. Then either select returned 
> a wrong positive or the read is buggy. 
> 	The kernel should mark the socket readable (and hence select returns 
> "readable") only after it has added the incoming data to the socket receive 
> queue or it has changed the state of the socket (in case of TCP connections 
> getting closed), in case of peer closing the connection.
> After that has been done, is the guarantee not there ?

What you're saying is correct.  However, if the UDP handler pulls the data
off of the receive queue and then finds that the datagram is corrupted, it
will go back into the read function to wait for another datagram.  Personally
I think that it should return some other kind of error (not EAGAIN) to indicate
somehow that we discarded a datagram due to a bad checksum.

Chad

Re: udp_recvmsg: possible bug causing infinite hang?

[Nagendra Singh Tomar]

On Tue, 12 Oct 2004, Chad N. Tindel wrote:

> > Does the POSIX standard _not_ give this implicit guarantee ? Whats the 
> > point in having a select call that says that the fd is readable, and when 
> > I go to read it the blocking read call blocks. Then either select returned 
> > a wrong positive or the read is buggy. 
> > 	The kernel should mark the socket readable (and hence select returns 
> > "readable") only after it has added the incoming data to the socket receive 
> > queue or it has changed the state of the socket (in case of TCP connections 
> > getting closed), in case of peer closing the connection.
> > After that has been done, is the guarantee not there ?
> 
> What you're saying is correct.  However, if the UDP handler pulls the data
> off of the receive queue and then finds that the datagram is corrupted, it
> will go back into the read function to wait for another datagram.  Personally
> I think that it should return some other kind of error (not EAGAIN) to indicate
> somehow that we discarded a datagram due to a bad checksum.

Now I wonder whether the skb should be added to the UDP socket receive 
queue, only after checksum verification (as is done for TCP). I understand 
that, doing it that way we lose the advantage of folding copy-to-user and 
checksumming. If the standard does not explicitly write anything about the 
guarantee of read/write passing after successful return from select, then 
its fine; but somehow that does not sound very rational. 
If anyone can point to the specific section in the POSIX standard that 
dictates this, it will be of great help.

Thanx,
tomar

> 
> Chad
> 

-- 



-- You have moved the mouse. Windows must be restarted for the 
   changes to take effect.

Re: udp_recvmsg: possible bug causing infinite hang?

[Henrik Nordstrom]

On Tue, 12 Oct 2004, Nagendra Singh Tomar wrote:

> checksumming. If the standard does not explicitly write anything about the
> guarantee of read/write passing after successful return from select, then
> its fine; but somehow that does not sound very rational.
> If anyone can point to the specific section in the POSIX standard that
> dictates this, it will be of great help.

>From SUSv3 on select():

       A descriptor shall be considered ready for reading when a
       call to an input function with O_NONBLOCK clear would not block,
       whether or not the function would transfer data successfully.
       (The function might return data, an end-of-file indication, or
       an error other than one indicating that it is blocked, and in
       each of these cases the descriptor shall be considered ready
       for reading.)

       A descriptor shall be considered ready for writing when a call
       to an output function with O_NONBLOCK clear would not block,
       whether or not the function would transfer data successfully.


so it seems to me the current Linux implementation is wrong in this 
regard.


Regards
Henrik

Re: BUG or not? GFP_KERNEL with interrupts disabled.

[Dan Eble]

On Thu, 27 Mar 2003, Linus Torvalds wrote:
> 
> On Thu, 27 Mar 2003, David S. Miller wrote:
> > 
> > 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 <dane@aiinet.com>  _____  .
                           |  _  |/|
Applied Innovation Inc.    | |_| | |
http://www.aiinet.com/     |__/|_|_|

Re: BUG or not? GFP_KERNEL with interrupts disabled.

[Linus Torvalds]

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

Re: BUG or not? GFP_KERNEL with interrupts disabled.

[David S. Miller]

   From: Linus Torvalds <torvalds@transmeta.com>
   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?

Re: BUG or not? GFP_KERNEL with interrupts disabled.

[Linus Torvalds]

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

Re: BUG or not? GFP_KERNEL with interrupts disabled.

[David S. Miller]

   From: Linus Torvalds <torvalds@transmeta.com>
   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.

Re: BUG or not? GFP_KERNEL with interrupts disabled.

[Robert Love]

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

Re: BUG or not? GFP_KERNEL with interrupts disabled.

[David S. Miller]

   From: Robert Love <rml@tech9.net>
   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.

Re: BUG or not? GFP_KERNEL with interrupts disabled.

[David S. Miller]

   From: "David S. Miller" <davem@redhat.com>
   Date: Thu, 27 Mar 2003 11:39:33 -0800 (PST)

      From: Linus Torvalds <torvalds@transmeta.com>
      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();

Re: BUG or not? GFP_KERNEL with interrupts disabled.

[David S. Miller]

   From: "David S. Miller" <davem@redhat.com>
   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.