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