[PATCH] mips: irq: add statckoverflow detection

[PATCH] mips: irq: add statckoverflow detection

[jiang.adam]

From: Adam Jiang <jiang.adam@gmail.com>

Add stackoverflow detection to mips arch

Signed-off-by: Adam Jiang <jiang.adam@gmail.com>
---
 arch/mips/Kconfig.debug |    7 +++++++
 arch/mips/kernel/irq.c  |   19 +++++++++++++++++++
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/arch/mips/Kconfig.debug b/arch/mips/Kconfig.debug
index 43dc279..f1a00a2 100644
--- a/arch/mips/Kconfig.debug
+++ b/arch/mips/Kconfig.debug
@@ -67,6 +67,13 @@ config CMDLINE_OVERRIDE
 
 	  Normally, you will choose 'N' here.
 
+config DEBUG_STACKOVERFLOW
+	bool "Check for stack overflows"
+	depends on DEBUG_KERNEL
+	help
+	  This option will cause messages to be printed if free stack space
+	  drops below a certain limit.
+
 config DEBUG_STACK_USAGE
 	bool "Enable stack utilization instrumentation"
 	depends on DEBUG_KERNEL
diff --git a/arch/mips/kernel/irq.c b/arch/mips/kernel/irq.c
index c6345f5..6334037 100644
--- a/arch/mips/kernel/irq.c
+++ b/arch/mips/kernel/irq.c
@@ -151,6 +151,22 @@ void __init init_IRQ(void)
 #endif
 }
 
+static inline void check_stack_overflow(void)
+{
+#ifdef CONFIG_DEBUG_STACKOVERFLOW
+	long sp;
+
+	asm volatile("move %0, $sp" : "=r" (sp));
+	sp = sp & (THREAD_SIZE-1);
+
+	/* check for stack overflow: is there less then 2KB free? */
+	if (unlikely(sp < (sizeof(struct thread_info) + 2048))) {
+		printk("do_IRQ: stack overflow: %ld\n",
+		       sp - sizeof(struct thread_info));
+		dump_stack();
+	}
+#endif
+}
 /*
  * do_IRQ handles all normal device IRQ's (the special
  * SMP cross-CPU interrupts have their own specific
@@ -159,6 +175,9 @@ void __init init_IRQ(void)
 void __irq_entry do_IRQ(unsigned int irq)
 {
 	irq_enter();
+
+	check_stack_overflow();
+
 	__DO_IRQ_SMTC_HOOK(irq);
 	generic_handle_irq(irq);
 	irq_exit();
-- 
1.7.1

Re: [PATCH] mips: irq: add statckoverflow detection

[Sergei Shtylyov]

Hello.

jiang.adam@gmail.com wrote:

> From: Adam Jiang <jiang.adam@gmail.com>

> Add stackoverflow detection to mips arch

> Signed-off-by: Adam Jiang <jiang.adam@gmail.com>
[...]
> diff --git a/arch/mips/kernel/irq.c b/arch/mips/kernel/irq.c
> index c6345f5..6334037 100644
> --- a/arch/mips/kernel/irq.c
> +++ b/arch/mips/kernel/irq.c
> @@ -151,6 +151,22 @@ void __init init_IRQ(void)
>  #endif
>  }
>  
> +static inline void check_stack_overflow(void)
> +{
> +#ifdef CONFIG_DEBUG_STACKOVERFLOW

    #ifdef within function is considered bad style. Better do it this way:

#ifdef CONFIG_DEBUG_STACKOVERFLOW
static inline void check_stack_overflow(void)
{
[...]
}
#else
static inline void check_stack_overflow(void) {}
#endif

> +	long sp;
> +
> +	asm volatile("move %0, $sp" : "=r" (sp));
> +	sp = sp & (THREAD_SIZE-1);
> +
> +	/* check for stack overflow: is there less then 2KB free? */

    Hm, 2KB seems pretty large margin...

WBR, Sergei

Re: [PATCH] mips: irq: add statckoverflow detection

[David Daney]

It looks like this patch only checks when processing an interrupt, which 
doesn't seem like it would give much coverage.

Am I missing something?

David Daney


On 08/20/2010 11:31 PM, jiang.adam@gmail.com wrote:
> From: Adam Jiang<jiang.adam@gmail.com>
>
> Add stackoverflow detection to mips arch
>
> Signed-off-by: Adam Jiang<jiang.adam@gmail.com>
> ---
>   arch/mips/Kconfig.debug |    7 +++++++
>   arch/mips/kernel/irq.c  |   19 +++++++++++++++++++
>   2 files changed, 26 insertions(+), 0 deletions(-)
>
> diff --git a/arch/mips/Kconfig.debug b/arch/mips/Kconfig.debug
> index 43dc279..f1a00a2 100644
> --- a/arch/mips/Kconfig.debug
> +++ b/arch/mips/Kconfig.debug
> @@ -67,6 +67,13 @@ config CMDLINE_OVERRIDE
>
>   	  Normally, you will choose 'N' here.
>
> +config DEBUG_STACKOVERFLOW
> +	bool "Check for stack overflows"
> +	depends on DEBUG_KERNEL
> +	help
> +	  This option will cause messages to be printed if free stack space
> +	  drops below a certain limit.
> +
>   config DEBUG_STACK_USAGE
>   	bool "Enable stack utilization instrumentation"
>   	depends on DEBUG_KERNEL
> diff --git a/arch/mips/kernel/irq.c b/arch/mips/kernel/irq.c
> index c6345f5..6334037 100644
> --- a/arch/mips/kernel/irq.c
> +++ b/arch/mips/kernel/irq.c
> @@ -151,6 +151,22 @@ void __init init_IRQ(void)
>   #endif
>   }
>
> +static inline void check_stack_overflow(void)
> +{
> +#ifdef CONFIG_DEBUG_STACKOVERFLOW
> +	long sp;
> +
> +	asm volatile("move %0, $sp" : "=r" (sp));
> +	sp = sp&  (THREAD_SIZE-1);
> +
> +	/* check for stack overflow: is there less then 2KB free? */
> +	if (unlikely(sp<  (sizeof(struct thread_info) + 2048))) {
> +		printk("do_IRQ: stack overflow: %ld\n",
> +		       sp - sizeof(struct thread_info));
> +		dump_stack();
> +	}
> +#endif
> +}
>   /*
>    * do_IRQ handles all normal device IRQ's (the special
>    * SMP cross-CPU interrupts have their own specific
> @@ -159,6 +175,9 @@ void __init init_IRQ(void)
>   void __irq_entry do_IRQ(unsigned int irq)
>   {
>   	irq_enter();
> +
> +	check_stack_overflow();
> +
>   	__DO_IRQ_SMTC_HOOK(irq);
>   	generic_handle_irq(irq);
>   	irq_exit();

Re: [PATCH] mips: irq: add statckoverflow detection

[David Daney]

On 08/25/2010 06:12 PM, Adam Jiang wrote:
> 2010/8/26 David Daney<ddaney@caviumnetworks.com>:
>> It looks like this patch only checks when processing an interrupt, which
>> doesn't seem like it would give much coverage.
>
> Yes, it is. This is only for detection on the situation which may be
> caused by nested interruption. No more coverage to any other cases.
> Because to do that is much difficult and unpredicted.
>

Well, since the default is to run interrupt handlers with interrupts 
disabled, it seems like it is of little use as you will almost never see 
nested interrupts.

A solution that shows overflow in all situations would be of more use. 
Something that could use GCC's -stack-check or related machinery might 
be interesting.

David Daney


> I will revise the bad code style Sergei blamed.
>
> /Adam
>
>>
>> Am I missing something?
>>
>> David Daney
>>
>>
>> On 08/20/2010 11:31 PM, jiang.adam@gmail.com wrote:
>>>
>>> From: Adam Jiang<jiang.adam@gmail.com>
>>>
>>> Add stackoverflow detection to mips arch
>>>
>>> Signed-off-by: Adam Jiang<jiang.adam@gmail.com>
>>> ---
>>>   arch/mips/Kconfig.debug |    7 +++++++
>>>   arch/mips/kernel/irq.c  |   19 +++++++++++++++++++
>>>   2 files changed, 26 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch/mips/Kconfig.debug b/arch/mips/Kconfig.debug
>>> index 43dc279..f1a00a2 100644
>>> --- a/arch/mips/Kconfig.debug
>>> +++ b/arch/mips/Kconfig.debug
>>> @@ -67,6 +67,13 @@ config CMDLINE_OVERRIDE
>>>
>>>           Normally, you will choose 'N' here.
>>>
>>> +config DEBUG_STACKOVERFLOW
>>> +       bool "Check for stack overflows"
>>> +       depends on DEBUG_KERNEL
>>> +       help
>>> +         This option will cause messages to be printed if free stack
>>> space
>>> +         drops below a certain limit.
>>> +
>>>   config DEBUG_STACK_USAGE
>>>         bool "Enable stack utilization instrumentation"
>>>         depends on DEBUG_KERNEL
>>> diff --git a/arch/mips/kernel/irq.c b/arch/mips/kernel/irq.c
>>> index c6345f5..6334037 100644
>>> --- a/arch/mips/kernel/irq.c
>>> +++ b/arch/mips/kernel/irq.c
>>> @@ -151,6 +151,22 @@ void __init init_IRQ(void)
>>>   #endif
>>>   }
>>>
>>> +static inline void check_stack_overflow(void)
>>> +{
>>> +#ifdef CONFIG_DEBUG_STACKOVERFLOW
>>> +       long sp;
>>> +
>>> +       asm volatile("move %0, $sp" : "=r" (sp));
>>> +       sp = sp&    (THREAD_SIZE-1);
>>> +
>>> +       /* check for stack overflow: is there less then 2KB free? */
>>> +       if (unlikely(sp<    (sizeof(struct thread_info) + 2048))) {
>>> +               printk("do_IRQ: stack overflow: %ld\n",
>>> +                      sp - sizeof(struct thread_info));
>>> +               dump_stack();
>>> +       }
>>> +#endif
>>> +}
>>>   /*
>>>    * do_IRQ handles all normal device IRQ's (the special
>>>    * SMP cross-CPU interrupts have their own specific
>>> @@ -159,6 +175,9 @@ void __init init_IRQ(void)
>>>   void __irq_entry do_IRQ(unsigned int irq)
>>>   {
>>>         irq_enter();
>>> +
>>> +       check_stack_overflow();
>>> +
>>>         __DO_IRQ_SMTC_HOOK(irq);
>>>         generic_handle_irq(irq);
>>>         irq_exit();
>>
>>

Re: [PATCH] mips: irq: add statckoverflow detection

[Adam Jiang]

On Thu, Aug 26, 2010 at 09:21:44AM -0700, David Daney wrote:
> On 08/25/2010 06:12 PM, Adam Jiang wrote:
> >2010/8/26 David Daney<ddaney@caviumnetworks.com>:
> >>It looks like this patch only checks when processing an interrupt, which
> >>doesn't seem like it would give much coverage.
> >
> >Yes, it is. This is only for detection on the situation which may be
> >caused by nested interruption. No more coverage to any other cases.
> >Because to do that is much difficult and unpredicted.
> >
> 
> Well, since the default is to run interrupt handlers with interrupts
> disabled, it seems like it is of little use as you will almost never
> see nested interrupts.
> 
> A solution that shows overflow in all situations would be of more
> use. Something that could use GCC's -stack-check or related
> machinery might be interesting.
> 

Yes, you may be right. However, some toolchain has no -stack-check
option available. At least, it seems my mispel gcc missed this option.

Actually, the issue has been discussed about one month before in
mips-linux list. You can see the thread here

http://www.spinics.net/lists/mips/msg38198.html

I tried to port the same functionality from x86 to mips. This may be
another option for checking stackoverflow but not an essential solution.
But yes, you are right. Nested interruption never happen unless some
really stupid thing. Unfortunately, I have seen such things in a BSP
code. I think it is necessary to provide a small function to check it.
How do you say?

Best regards,
/Adam

> David Daney
> 
> 
> >I will revise the bad code style Sergei blamed.
> >
> >/Adam
> >
> >>
> >>Am I missing something?
> >>
> >>David Daney
> >>
> >>
> >>On 08/20/2010 11:31 PM, jiang.adam@gmail.com wrote:
> >>>
> >>>From: Adam Jiang<jiang.adam@gmail.com>
> >>>
> >>>Add stackoverflow detection to mips arch
> >>>
> >>>Signed-off-by: Adam Jiang<jiang.adam@gmail.com>
> >>>---
> >>>  arch/mips/Kconfig.debug |    7 +++++++
> >>>  arch/mips/kernel/irq.c  |   19 +++++++++++++++++++
> >>>  2 files changed, 26 insertions(+), 0 deletions(-)
> >>>
> >>>diff --git a/arch/mips/Kconfig.debug b/arch/mips/Kconfig.debug
> >>>index 43dc279..f1a00a2 100644
> >>>--- a/arch/mips/Kconfig.debug
> >>>+++ b/arch/mips/Kconfig.debug
> >>>@@ -67,6 +67,13 @@ config CMDLINE_OVERRIDE
> >>>
> >>>          Normally, you will choose 'N' here.
> >>>
> >>>+config DEBUG_STACKOVERFLOW
> >>>+       bool "Check for stack overflows"
> >>>+       depends on DEBUG_KERNEL
> >>>+       help
> >>>+         This option will cause messages to be printed if free stack
> >>>space
> >>>+         drops below a certain limit.
> >>>+
> >>>  config DEBUG_STACK_USAGE
> >>>        bool "Enable stack utilization instrumentation"
> >>>        depends on DEBUG_KERNEL
> >>>diff --git a/arch/mips/kernel/irq.c b/arch/mips/kernel/irq.c
> >>>index c6345f5..6334037 100644
> >>>--- a/arch/mips/kernel/irq.c
> >>>+++ b/arch/mips/kernel/irq.c
> >>>@@ -151,6 +151,22 @@ void __init init_IRQ(void)
> >>>  #endif
> >>>  }
> >>>
> >>>+static inline void check_stack_overflow(void)
> >>>+{
> >>>+#ifdef CONFIG_DEBUG_STACKOVERFLOW
> >>>+       long sp;
> >>>+
> >>>+       asm volatile("move %0, $sp" : "=r" (sp));
> >>>+       sp = sp&    (THREAD_SIZE-1);
> >>>+
> >>>+       /* check for stack overflow: is there less then 2KB free? */
> >>>+       if (unlikely(sp<    (sizeof(struct thread_info) + 2048))) {
> >>>+               printk("do_IRQ: stack overflow: %ld\n",
> >>>+                      sp - sizeof(struct thread_info));
> >>>+               dump_stack();
> >>>+       }
> >>>+#endif
> >>>+}
> >>>  /*
> >>>   * do_IRQ handles all normal device IRQ's (the special
> >>>   * SMP cross-CPU interrupts have their own specific
> >>>@@ -159,6 +175,9 @@ void __init init_IRQ(void)
> >>>  void __irq_entry do_IRQ(unsigned int irq)
> >>>  {
> >>>        irq_enter();
> >>>+
> >>>+       check_stack_overflow();
> >>>+
> >>>        __DO_IRQ_SMTC_HOOK(irq);
> >>>        generic_handle_irq(irq);
> >>>        irq_exit();
> >>
> >>
>