Re: [PATCH] powerpc/irq: Run softirqs off the top of the irq stack
On Mon, 2013-09-23 at 09:47 -0700, Linus Torvalds wrote: > On Sun, Sep 22, 2013 at 9:35 PM, Benjamin Herrenschmidt > wrote: > > > > This is the "band aid" discussed so far for the stack overflow > > problem for powerpc. > > I don't think it's a "band-aid" in any way, except perhaps in the > sense that there are certainly other things we can also do in this > series (ie I liked Frederic's cleanups etc). Ah yes, I thought of it as a band-aid in the sense that a better approach would be to switch to the irq stack earlier like x86_64 does but that would be a lot more invasive. Definitely something I would look into if I was to tackle changing how we do per-cpu and the PACA though. Cheers, Ben. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] powerpc/irq: Run softirqs off the top of the irq stack
On Sun, Sep 22, 2013 at 9:35 PM, Benjamin Herrenschmidt wrote: > > This is the "band aid" discussed so far for the stack overflow > problem for powerpc. I don't think it's a "band-aid" in any way, except perhaps in the sense that there are certainly other things we can also do in this series (ie I liked Frederic's cleanups etc). Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] powerpc/irq: Run softirqs off the top of the irq stack
On Mon, 2013-09-23 at 17:56 +1000, Stephen Rothwell wrote: > Hi Ben, > > On Mon, 23 Sep 2013 14:35:58 +1000 Benjamin Herrenschmidt > wrote: > > > > diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c > > index c69440c..0c9646f 100644 > > --- a/arch/powerpc/kernel/irq.c > > +++ b/arch/powerpc/kernel/irq.c > > @@ -443,46 +443,7 @@ void migrate_irqs(void) > > > > static inline void handle_one_irq(unsigned int irq) > > { > > - struct thread_info *curtp, *irqtp; > > - unsigned long saved_sp_limit; > > - struct irq_desc *desc; > > - > > - desc = irq_to_desc(irq); > > - if (!desc) > > - return; > > - > > - /* Switch to the irq stack to handle this */ > > - curtp = current_thread_info(); > > - irqtp = hardirq_ctx[smp_processor_id()]; > > - > > - if (curtp == irqtp) { > > - /* We're already on the irq stack, just handle it */ > > - desc->handle_irq(irq, desc); > > - return; > > - } > > - > > - saved_sp_limit = current->thread.ksp_limit; > > - > > - irqtp->task = curtp->task; > > - irqtp->flags = 0; > > - > > - /* Copy the softirq bits in preempt_count so that the > > -* softirq checks work in the hardirq context. */ > > - irqtp->preempt_count = (irqtp->preempt_count & ~SOFTIRQ_MASK) | > > - (curtp->preempt_count & SOFTIRQ_MASK); > > > > - current->thread.ksp_limit = (unsigned long)irqtp + > > - _ALIGN_UP(sizeof(struct thread_info), 16); > > - > > - call_handle_irq(irq, desc, irqtp, desc->handle_irq); > > - current->thread.ksp_limit = saved_sp_limit; > > - irqtp->task = NULL; > > - > > - /* Set any flag that may have been set on the > > -* alternate stack > > -*/ > > - if (irqtp->flags) > > - set_bits(irqtp->flags, >flags); > > } > > This function ends up as a single blank line ... > > > @@ -519,18 +480,64 @@ void do_IRQ(struct pt_regs *regs) > > */ > > irq = ppc_md.get_irq(); > > > > - /* We can hard enable interrupts now */ > > + /* We can hard enable interrupts now to allow perf interrupts */ > > may_hard_irq_enable(); > > > > /* And finally process it */ > > - if (irq != NO_IRQ) > > - handle_one_irq(irq); > > then you remove the only call, so why not just remove the function > completely? Because I'm an idiot ? :-) I moved bits and pieces to do_IRQ and forgot to remove the remainder (and gcc didn't warn :-) Cheers, Ben. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] powerpc/irq: Run softirqs off the top of the irq stack
Hi Ben, On Mon, 23 Sep 2013 14:35:58 +1000 Benjamin Herrenschmidt wrote: > > diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c > index c69440c..0c9646f 100644 > --- a/arch/powerpc/kernel/irq.c > +++ b/arch/powerpc/kernel/irq.c > @@ -443,46 +443,7 @@ void migrate_irqs(void) > > static inline void handle_one_irq(unsigned int irq) > { > - struct thread_info *curtp, *irqtp; > - unsigned long saved_sp_limit; > - struct irq_desc *desc; > - > - desc = irq_to_desc(irq); > - if (!desc) > - return; > - > - /* Switch to the irq stack to handle this */ > - curtp = current_thread_info(); > - irqtp = hardirq_ctx[smp_processor_id()]; > - > - if (curtp == irqtp) { > - /* We're already on the irq stack, just handle it */ > - desc->handle_irq(irq, desc); > - return; > - } > - > - saved_sp_limit = current->thread.ksp_limit; > - > - irqtp->task = curtp->task; > - irqtp->flags = 0; > - > - /* Copy the softirq bits in preempt_count so that the > - * softirq checks work in the hardirq context. */ > - irqtp->preempt_count = (irqtp->preempt_count & ~SOFTIRQ_MASK) | > -(curtp->preempt_count & SOFTIRQ_MASK); > > - current->thread.ksp_limit = (unsigned long)irqtp + > - _ALIGN_UP(sizeof(struct thread_info), 16); > - > - call_handle_irq(irq, desc, irqtp, desc->handle_irq); > - current->thread.ksp_limit = saved_sp_limit; > - irqtp->task = NULL; > - > - /* Set any flag that may have been set on the > - * alternate stack > - */ > - if (irqtp->flags) > - set_bits(irqtp->flags, >flags); > } This function ends up as a single blank line ... > @@ -519,18 +480,64 @@ void do_IRQ(struct pt_regs *regs) >*/ > irq = ppc_md.get_irq(); > > - /* We can hard enable interrupts now */ > + /* We can hard enable interrupts now to allow perf interrupts */ > may_hard_irq_enable(); > > /* And finally process it */ > - if (irq != NO_IRQ) > - handle_one_irq(irq); then you remove the only call, so why not just remove the function completely? -- Cheers, Stephen Rothwells...@canb.auug.org.au pgp7reOVBzIjg.pgp Description: PGP signature
Re: [PATCH] powerpc/irq: Run softirqs off the top of the irq stack
Hi Ben, On Mon, 23 Sep 2013 14:35:58 +1000 Benjamin Herrenschmidt b...@kernel.crashing.org wrote: diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c index c69440c..0c9646f 100644 --- a/arch/powerpc/kernel/irq.c +++ b/arch/powerpc/kernel/irq.c @@ -443,46 +443,7 @@ void migrate_irqs(void) static inline void handle_one_irq(unsigned int irq) { - struct thread_info *curtp, *irqtp; - unsigned long saved_sp_limit; - struct irq_desc *desc; - - desc = irq_to_desc(irq); - if (!desc) - return; - - /* Switch to the irq stack to handle this */ - curtp = current_thread_info(); - irqtp = hardirq_ctx[smp_processor_id()]; - - if (curtp == irqtp) { - /* We're already on the irq stack, just handle it */ - desc-handle_irq(irq, desc); - return; - } - - saved_sp_limit = current-thread.ksp_limit; - - irqtp-task = curtp-task; - irqtp-flags = 0; - - /* Copy the softirq bits in preempt_count so that the - * softirq checks work in the hardirq context. */ - irqtp-preempt_count = (irqtp-preempt_count ~SOFTIRQ_MASK) | -(curtp-preempt_count SOFTIRQ_MASK); - current-thread.ksp_limit = (unsigned long)irqtp + - _ALIGN_UP(sizeof(struct thread_info), 16); - - call_handle_irq(irq, desc, irqtp, desc-handle_irq); - current-thread.ksp_limit = saved_sp_limit; - irqtp-task = NULL; - - /* Set any flag that may have been set on the - * alternate stack - */ - if (irqtp-flags) - set_bits(irqtp-flags, curtp-flags); } This function ends up as a single blank line ... @@ -519,18 +480,64 @@ void do_IRQ(struct pt_regs *regs) */ irq = ppc_md.get_irq(); - /* We can hard enable interrupts now */ + /* We can hard enable interrupts now to allow perf interrupts */ may_hard_irq_enable(); /* And finally process it */ - if (irq != NO_IRQ) - handle_one_irq(irq); then you remove the only call, so why not just remove the function completely? -- Cheers, Stephen Rothwells...@canb.auug.org.au pgp7reOVBzIjg.pgp Description: PGP signature
Re: [PATCH] powerpc/irq: Run softirqs off the top of the irq stack
On Mon, 2013-09-23 at 17:56 +1000, Stephen Rothwell wrote: Hi Ben, On Mon, 23 Sep 2013 14:35:58 +1000 Benjamin Herrenschmidt b...@kernel.crashing.org wrote: diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c index c69440c..0c9646f 100644 --- a/arch/powerpc/kernel/irq.c +++ b/arch/powerpc/kernel/irq.c @@ -443,46 +443,7 @@ void migrate_irqs(void) static inline void handle_one_irq(unsigned int irq) { - struct thread_info *curtp, *irqtp; - unsigned long saved_sp_limit; - struct irq_desc *desc; - - desc = irq_to_desc(irq); - if (!desc) - return; - - /* Switch to the irq stack to handle this */ - curtp = current_thread_info(); - irqtp = hardirq_ctx[smp_processor_id()]; - - if (curtp == irqtp) { - /* We're already on the irq stack, just handle it */ - desc-handle_irq(irq, desc); - return; - } - - saved_sp_limit = current-thread.ksp_limit; - - irqtp-task = curtp-task; - irqtp-flags = 0; - - /* Copy the softirq bits in preempt_count so that the -* softirq checks work in the hardirq context. */ - irqtp-preempt_count = (irqtp-preempt_count ~SOFTIRQ_MASK) | - (curtp-preempt_count SOFTIRQ_MASK); - current-thread.ksp_limit = (unsigned long)irqtp + - _ALIGN_UP(sizeof(struct thread_info), 16); - - call_handle_irq(irq, desc, irqtp, desc-handle_irq); - current-thread.ksp_limit = saved_sp_limit; - irqtp-task = NULL; - - /* Set any flag that may have been set on the -* alternate stack -*/ - if (irqtp-flags) - set_bits(irqtp-flags, curtp-flags); } This function ends up as a single blank line ... @@ -519,18 +480,64 @@ void do_IRQ(struct pt_regs *regs) */ irq = ppc_md.get_irq(); - /* We can hard enable interrupts now */ + /* We can hard enable interrupts now to allow perf interrupts */ may_hard_irq_enable(); /* And finally process it */ - if (irq != NO_IRQ) - handle_one_irq(irq); then you remove the only call, so why not just remove the function completely? Because I'm an idiot ? :-) I moved bits and pieces to do_IRQ and forgot to remove the remainder (and gcc didn't warn :-) Cheers, Ben. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] powerpc/irq: Run softirqs off the top of the irq stack
On Sun, Sep 22, 2013 at 9:35 PM, Benjamin Herrenschmidt b...@kernel.crashing.org wrote: This is the band aid discussed so far for the stack overflow problem for powerpc. I don't think it's a band-aid in any way, except perhaps in the sense that there are certainly other things we can also do in this series (ie I liked Frederic's cleanups etc). Linus -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] powerpc/irq: Run softirqs off the top of the irq stack
On Mon, 2013-09-23 at 09:47 -0700, Linus Torvalds wrote: On Sun, Sep 22, 2013 at 9:35 PM, Benjamin Herrenschmidt b...@kernel.crashing.org wrote: This is the band aid discussed so far for the stack overflow problem for powerpc. I don't think it's a band-aid in any way, except perhaps in the sense that there are certainly other things we can also do in this series (ie I liked Frederic's cleanups etc). Ah yes, I thought of it as a band-aid in the sense that a better approach would be to switch to the irq stack earlier like x86_64 does but that would be a lot more invasive. Definitely something I would look into if I was to tackle changing how we do per-cpu and the PACA though. Cheers, Ben. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] powerpc/irq: Run softirqs off the top of the irq stack
Nowadays, irq_exit() calls __do_softirq() pretty much directly instead of calling do_softirq() which switches to the decicated softirq stack. This has lead to observed stack overflows on powerpc since we call irq_enter() and irq_exit() outside of the scope that switches to the irq stack. This fixes it by moving the stack switching up a level, making irq_enter() and irq_exit() run off the irq stack. Signed-off-by: Benjamin Herrenschmidt --- This is the "band aid" discussed so far for the stack overflow problem for powerpc. I assume sparc and i386 at least need something similar (I had a quick look at ARM and it doesn't seem to have irq stacks at all). Unless objection in the next day or so, I intend to send that to Linus and to -stable ASAP. arch/powerpc/include/asm/irq.h | 4 +- arch/powerpc/kernel/irq.c | 99 ++ arch/powerpc/kernel/misc_32.S | 9 ++-- arch/powerpc/kernel/misc_64.S | 10 ++--- 4 files changed, 62 insertions(+), 60 deletions(-) diff --git a/arch/powerpc/include/asm/irq.h b/arch/powerpc/include/asm/irq.h index 0e40843..41f13ce 100644 --- a/arch/powerpc/include/asm/irq.h +++ b/arch/powerpc/include/asm/irq.h @@ -69,9 +69,9 @@ extern struct thread_info *softirq_ctx[NR_CPUS]; extern void irq_ctx_init(void); extern void call_do_softirq(struct thread_info *tp); -extern int call_handle_irq(int irq, void *p1, - struct thread_info *tp, void *func); +extern void call_do_irq(struct pt_regs *regs, struct thread_info *tp); extern void do_IRQ(struct pt_regs *regs); +extern void __do_irq(struct pt_regs *regs); int irq_choose_cpu(const struct cpumask *mask); diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c index c69440c..0c9646f 100644 --- a/arch/powerpc/kernel/irq.c +++ b/arch/powerpc/kernel/irq.c @@ -443,46 +443,7 @@ void migrate_irqs(void) static inline void handle_one_irq(unsigned int irq) { - struct thread_info *curtp, *irqtp; - unsigned long saved_sp_limit; - struct irq_desc *desc; - - desc = irq_to_desc(irq); - if (!desc) - return; - - /* Switch to the irq stack to handle this */ - curtp = current_thread_info(); - irqtp = hardirq_ctx[smp_processor_id()]; - - if (curtp == irqtp) { - /* We're already on the irq stack, just handle it */ - desc->handle_irq(irq, desc); - return; - } - - saved_sp_limit = current->thread.ksp_limit; - - irqtp->task = curtp->task; - irqtp->flags = 0; - - /* Copy the softirq bits in preempt_count so that the -* softirq checks work in the hardirq context. */ - irqtp->preempt_count = (irqtp->preempt_count & ~SOFTIRQ_MASK) | - (curtp->preempt_count & SOFTIRQ_MASK); - current->thread.ksp_limit = (unsigned long)irqtp + - _ALIGN_UP(sizeof(struct thread_info), 16); - - call_handle_irq(irq, desc, irqtp, desc->handle_irq); - current->thread.ksp_limit = saved_sp_limit; - irqtp->task = NULL; - - /* Set any flag that may have been set on the -* alternate stack -*/ - if (irqtp->flags) - set_bits(irqtp->flags, >flags); } static inline void check_stack_overflow(void) @@ -501,9 +462,9 @@ static inline void check_stack_overflow(void) #endif } -void do_IRQ(struct pt_regs *regs) +void __do_irq(struct pt_regs *regs) { - struct pt_regs *old_regs = set_irq_regs(regs); + struct irq_desc *desc; unsigned int irq; irq_enter(); @@ -519,18 +480,64 @@ void do_IRQ(struct pt_regs *regs) */ irq = ppc_md.get_irq(); - /* We can hard enable interrupts now */ + /* We can hard enable interrupts now to allow perf interrupts */ may_hard_irq_enable(); /* And finally process it */ - if (irq != NO_IRQ) - handle_one_irq(irq); - else + if (unlikely(irq == NO_IRQ)) __get_cpu_var(irq_stat).spurious_irqs++; + else { + desc = irq_to_desc(irq); + if (likely(desc)) + desc->handle_irq(irq, desc); + } trace_irq_exit(regs); irq_exit(); +} + +void do_IRQ(struct pt_regs *regs) +{ + struct pt_regs *old_regs = set_irq_regs(regs); + struct thread_info *curtp, *irqtp; + unsigned long saved_sp_limit; + + /* Switch to the irq stack to handle this */ + curtp = current_thread_info(); + irqtp = hardirq_ctx[raw_smp_processor_id()]; + + /* Already there ? */ + if (unlikely(curtp == irqtp)) { + __do_irq(regs); + set_irq_regs(old_regs); + return; + } + + /* Adjust the stack limit */ + saved_sp_limit = current->thread.ksp_limit; + current->thread.ksp_limit = (unsigned long)irqtp + + _ALIGN_UP(sizeof(struct thread_info), 16); + + +
[PATCH] powerpc/irq: Run softirqs off the top of the irq stack
Nowadays, irq_exit() calls __do_softirq() pretty much directly instead of calling do_softirq() which switches to the decicated softirq stack. This has lead to observed stack overflows on powerpc since we call irq_enter() and irq_exit() outside of the scope that switches to the irq stack. This fixes it by moving the stack switching up a level, making irq_enter() and irq_exit() run off the irq stack. Signed-off-by: Benjamin Herrenschmidt b...@kernel.crashing.org --- This is the band aid discussed so far for the stack overflow problem for powerpc. I assume sparc and i386 at least need something similar (I had a quick look at ARM and it doesn't seem to have irq stacks at all). Unless objection in the next day or so, I intend to send that to Linus and to -stable ASAP. arch/powerpc/include/asm/irq.h | 4 +- arch/powerpc/kernel/irq.c | 99 ++ arch/powerpc/kernel/misc_32.S | 9 ++-- arch/powerpc/kernel/misc_64.S | 10 ++--- 4 files changed, 62 insertions(+), 60 deletions(-) diff --git a/arch/powerpc/include/asm/irq.h b/arch/powerpc/include/asm/irq.h index 0e40843..41f13ce 100644 --- a/arch/powerpc/include/asm/irq.h +++ b/arch/powerpc/include/asm/irq.h @@ -69,9 +69,9 @@ extern struct thread_info *softirq_ctx[NR_CPUS]; extern void irq_ctx_init(void); extern void call_do_softirq(struct thread_info *tp); -extern int call_handle_irq(int irq, void *p1, - struct thread_info *tp, void *func); +extern void call_do_irq(struct pt_regs *regs, struct thread_info *tp); extern void do_IRQ(struct pt_regs *regs); +extern void __do_irq(struct pt_regs *regs); int irq_choose_cpu(const struct cpumask *mask); diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c index c69440c..0c9646f 100644 --- a/arch/powerpc/kernel/irq.c +++ b/arch/powerpc/kernel/irq.c @@ -443,46 +443,7 @@ void migrate_irqs(void) static inline void handle_one_irq(unsigned int irq) { - struct thread_info *curtp, *irqtp; - unsigned long saved_sp_limit; - struct irq_desc *desc; - - desc = irq_to_desc(irq); - if (!desc) - return; - - /* Switch to the irq stack to handle this */ - curtp = current_thread_info(); - irqtp = hardirq_ctx[smp_processor_id()]; - - if (curtp == irqtp) { - /* We're already on the irq stack, just handle it */ - desc-handle_irq(irq, desc); - return; - } - - saved_sp_limit = current-thread.ksp_limit; - - irqtp-task = curtp-task; - irqtp-flags = 0; - - /* Copy the softirq bits in preempt_count so that the -* softirq checks work in the hardirq context. */ - irqtp-preempt_count = (irqtp-preempt_count ~SOFTIRQ_MASK) | - (curtp-preempt_count SOFTIRQ_MASK); - current-thread.ksp_limit = (unsigned long)irqtp + - _ALIGN_UP(sizeof(struct thread_info), 16); - - call_handle_irq(irq, desc, irqtp, desc-handle_irq); - current-thread.ksp_limit = saved_sp_limit; - irqtp-task = NULL; - - /* Set any flag that may have been set on the -* alternate stack -*/ - if (irqtp-flags) - set_bits(irqtp-flags, curtp-flags); } static inline void check_stack_overflow(void) @@ -501,9 +462,9 @@ static inline void check_stack_overflow(void) #endif } -void do_IRQ(struct pt_regs *regs) +void __do_irq(struct pt_regs *regs) { - struct pt_regs *old_regs = set_irq_regs(regs); + struct irq_desc *desc; unsigned int irq; irq_enter(); @@ -519,18 +480,64 @@ void do_IRQ(struct pt_regs *regs) */ irq = ppc_md.get_irq(); - /* We can hard enable interrupts now */ + /* We can hard enable interrupts now to allow perf interrupts */ may_hard_irq_enable(); /* And finally process it */ - if (irq != NO_IRQ) - handle_one_irq(irq); - else + if (unlikely(irq == NO_IRQ)) __get_cpu_var(irq_stat).spurious_irqs++; + else { + desc = irq_to_desc(irq); + if (likely(desc)) + desc-handle_irq(irq, desc); + } trace_irq_exit(regs); irq_exit(); +} + +void do_IRQ(struct pt_regs *regs) +{ + struct pt_regs *old_regs = set_irq_regs(regs); + struct thread_info *curtp, *irqtp; + unsigned long saved_sp_limit; + + /* Switch to the irq stack to handle this */ + curtp = current_thread_info(); + irqtp = hardirq_ctx[raw_smp_processor_id()]; + + /* Already there ? */ + if (unlikely(curtp == irqtp)) { + __do_irq(regs); + set_irq_regs(old_regs); + return; + } + + /* Adjust the stack limit */ + saved_sp_limit = current-thread.ksp_limit; + current-thread.ksp_limit = (unsigned long)irqtp + + _ALIGN_UP(sizeof(struct thread_info),