i386: Execute stack overflow warning on interrupt stack II
in [Kernel]
|
From: Thomas Gleixner on 5 May 2008 06:10 On Fri, 2 May 2008, Andi Kleen wrote: > +static void stack_overflow(void) > +{ > + printk("low stack detected by irq handler\n"); Needs a KERN_ERR > + /* Execute warning on interrupt stack */ > + if (unlikely(overflow)) > + call_on_stack2(stack_overflow, isp, 0, 0); > + > + call_on_stack2(desc->handle_irq, isp, irq, desc); arch/x86/kernel/irq_32.c:148: warning: passing argument 2 of �call_on_stack2� makes integer from pointer without a cast arch/x86/kernel/irq_32.c:150: warning: passing argument 2 of �call_on_stack2� makes integer from pointer without a cast arch/x86/kernel/irq_32.c:150: warning: passing argument 4 of �call_on_stack2� makes integer from pointer without a cast > } else > #endif > - desc->handle_irq(irq, desc); > + { > + /* AK: Slightly bogus here */ Bogus comment. This applies to both the !4KSTACKS and the overflow of the irq stack in the 4KSTACKS case. > + if (overflow) unlikely(overflow) ? > + stack_overflow(); > + desc->handle_irq(irq, desc); Thanks, tglx
From: Andi Kleen on 5 May 2008 06:30 Thomas Gleixner wrote: > On Fri, 2 May 2008, Andi Kleen wrote: >> +static void stack_overflow(void) >> +{ >> + printk("low stack detected by irq handler\n"); > > Needs a KERN_ERR Just moving code. If there is one added it should be in another patch. Besides if anything it's a KERN_WARN I guess. > >> + /* Execute warning on interrupt stack */ >> + if (unlikely(overflow)) >> + call_on_stack2(stack_overflow, isp, 0, 0); >> + >> + call_on_stack2(desc->handle_irq, isp, irq, desc); > > arch/x86/kernel/irq_32.c:148: warning: passing argument 2 of �call_on_stack2� makes integer from pointer without a cast > arch/x86/kernel/irq_32.c:150: warning: passing argument 2 of �call_on_stack2� makes integer from pointer without a cast > arch/x86/kernel/irq_32.c:150: warning: passing argument 4 of �call_on_stack2� makes integer from pointer without a cast Hmm, strange. I don't see that here CC arch/x86/kernel/irq_32.o CC arch/x86/kernel/time_32.o gcc version 4.1.2 20061115 (prerelease) (SUSE Linux) What compiler are you using? Or did you change anything? (I know you like to do that) >> } else >> #endif >> - desc->handle_irq(irq, desc); >> + { >> + /* AK: Slightly bogus here */ > > Bogus comment. This applies to both the !4KSTACKS and the overflow of > the irq stack in the 4KSTACKS case. The comment refers to that the check here doesn't check the process stack, but the interrupt stack. In fact if the interrupt stack is near overflow we should probably just reject the interrupt? Although that might cause hangs too. Or perhaps just enlarge it [that is now possible with i386 pda with some effort]. Anyways it is probably not an interesting case because nested interrupts are rare. >> + if (overflow) > > unlikely(overflow) ? Doesn't matter really. The whole branch is unlikely. -Andi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Thomas Gleixner on 5 May 2008 08:50 On Mon, 5 May 2008, Andi Kleen wrote: > Thomas Gleixner wrote: > > On Fri, 2 May 2008, Andi Kleen wrote: > >> +static void stack_overflow(void) > >> +{ > >> + printk("low stack detected by irq handler\n"); > > > > Needs a KERN_ERR > > Just moving code. If there is one added it should be in another patch. Err, you are not moving code. The printk is pretty different and adding the KERN_xx in the same go is nothing which makes the patch harder to understand. > Besides if anything it's a KERN_WARN I guess. KERN_WARN is fine, even if I consider a stack overflow as an error. > > > >> + /* Execute warning on interrupt stack */ > >> + if (unlikely(overflow)) > >> + call_on_stack2(stack_overflow, isp, 0, 0); > >> + > >> + call_on_stack2(desc->handle_irq, isp, irq, desc); > > > > arch/x86/kernel/irq_32.c:148: warning: passing argument 2 of �call_on_stack2� makes integer from pointer without a cast > > arch/x86/kernel/irq_32.c:150: warning: passing argument 2 of �call_on_stack2� makes integer from pointer without a cast > > arch/x86/kernel/irq_32.c:150: warning: passing argument 4 of �call_on_stack2� makes integer from pointer without a cast > > Hmm, strange. I don't see that here > > > CC arch/x86/kernel/irq_32.o > CC arch/x86/kernel/time_32.o > > gcc version 4.1.2 20061115 (prerelease) (SUSE Linux) > > What compiler are you using? Or did you change anything? (I know you > like to do that) I noticed on review and just compiled the unmodified patch with 4KSTACKS=y. > >> } else > >> #endif > >> - desc->handle_irq(irq, desc); > >> + { > >> + /* AK: Slightly bogus here */ > > > > Bogus comment. This applies to both the !4KSTACKS and the overflow of > > the irq stack in the 4KSTACKS case. > > The comment refers to that the check here doesn't check the process > stack, but the interrupt stack. In fact if the interrupt stack is near > overflow we should probably just reject the interrupt? Although that > might cause hangs too. Or perhaps just enlarge it [that is now possible > with i386 pda with some effort]. Anyways it is probably not an > interesting case because nested interrupts are rare. Err, it checks the process stack when 4KSTACKS=n > >> + if (overflow) > > > > unlikely(overflow) ? > > Doesn't matter really. The whole branch is unlikely. There is no branch with 4KSTACKS=n Thanks, tglx
From: Eric Sandeen on 5 May 2008 09:20 Thomas Gleixner wrote: > On Mon, 5 May 2008, Andi Kleen wrote: >> Thomas Gleixner wrote: >>> On Fri, 2 May 2008, Andi Kleen wrote: >>>> +static void stack_overflow(void) >>>> +{ >>>> + printk("low stack detected by irq handler\n"); >>> Needs a KERN_ERR >> Just moving code. If there is one added it should be in another patch. > > Err, you are not moving code. The printk is pretty different and > adding the KERN_xx in the same go is nothing which makes the patch > harder to understand. > >> Besides if anything it's a KERN_WARN I guess. > > KERN_WARN is fine, even if I consider a stack overflow as an error. But it hasn't overflowed yet. It's a warning about being *close* - not an error. -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Andi Kleen on 5 May 2008 09:40 > Err, it checks the process stack when 4KSTACKS=n Ok then please add the two changes if you feel strongly about that (to the latest version I sent). You always edit my patches anyways (usually driving me crazy when I have dependent patches because nothing applies anymore when all the variables got renamed like you often do) so I don't see any reason why you can't do that here. -Andi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
|
Next
|
Last
Pages: 1 2 Prev: Oops with strace_test Next: [patch 11/15] vfs: truncate: append-only checking cleanup |