2016-12-02 13:39:38

by Matt Redfearn

[permalink] [raw]
Subject: [PATCH 0/5] MIPS: Add per-cpu IRQ stack


This series adds a separate stack for each CPU wihin the system to use
when handling IRQs. Previously IRQs were handled on the kernel stack of
the current task. If that task was deep down a call stack at the point
of the interrupt, and handling the interrupt required a deep IRQ stack,
then there was a likelihood of stack overflow. Since the kernel stack
is in normal unmapped memory, overflowing it can lead to silent
corruption of other kernel data, with weird and wonderful results.

Before this patch series, ftracing the maximum stack size of a v4.9-rc6
kernel running on a Ci40 board gave:
4996

And with this series:
4084

Handling interrupts on a separate stack reduces the maximum kernel stack
usage in this configuration by ~900 bytes.

Since do_IRQ is now invoked on a separate stack, we select
HAVE_IRQ_EXIT_ON_IRQ_STACK so that softirqs will also be executed on the
irq stack rather than attempting to switch with do_softirq_own_stack().

This series has been tested on MIPS Boston, Malta and SEAD3 platforms,
Pistachio on the Creator Ci40 board and Cavium Octeon III.



Matt Redfearn (5):
MIPS: Introduce irq_stack
MIPS: Stack unwinding while on IRQ stack
MIPS: Only change $28 to thread_info if coming from user mode
MIPS: Switch to the irq_stack in interrupts
MIPS: Select HAVE_IRQ_EXIT_ON_IRQ_STACK

arch/mips/Kconfig | 1 +
arch/mips/include/asm/irq.h | 12 ++++++
arch/mips/include/asm/stackframe.h | 10 +++++
arch/mips/kernel/asm-offsets.c | 1 +
arch/mips/kernel/genex.S | 81 +++++++++++++++++++++++++++++++++++---
arch/mips/kernel/irq.c | 11 ++++++
arch/mips/kernel/process.c | 15 ++++++-
7 files changed, 125 insertions(+), 6 deletions(-)

--
2.7.4


2016-12-02 13:39:41

by Matt Redfearn

[permalink] [raw]
Subject: [PATCH 2/5] MIPS: Stack unwinding while on IRQ stack

Within unwind stack, check if the stack pointer being unwound is within
the CPU's irq_stack and if so use that page rather than the task's stack
page.

Signed-off-by: Matt Redfearn <[email protected]>
---

arch/mips/kernel/process.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
index 9514e5f2209f..4fdbf7078071 100644
--- a/arch/mips/kernel/process.c
+++ b/arch/mips/kernel/process.c
@@ -33,6 +33,7 @@
#include <asm/dsemul.h>
#include <asm/dsp.h>
#include <asm/fpu.h>
+#include <asm/irq.h>
#include <asm/msa.h>
#include <asm/pgtable.h>
#include <asm/mipsregs.h>
@@ -511,7 +512,19 @@ EXPORT_SYMBOL(unwind_stack_by_address);
unsigned long unwind_stack(struct task_struct *task, unsigned long *sp,
unsigned long pc, unsigned long *ra)
{
- unsigned long stack_page = (unsigned long)task_stack_page(task);
+ unsigned long stack_page = 0;
+ int cpu;
+
+ for_each_possible_cpu(cpu) {
+ if (on_irq_stack(cpu, *sp)) {
+ stack_page = (unsigned long)irq_stack[cpu];
+ break;
+ }
+ }
+
+ if (!stack_page)
+ stack_page = (unsigned long)task_stack_page(task);
+
return unwind_stack_by_address(stack_page, sp, pc, ra);
}
#endif
--
2.7.4

2016-12-02 13:39:56

by Matt Redfearn

[permalink] [raw]
Subject: [PATCH 5/5] MIPS: Select HAVE_IRQ_EXIT_ON_IRQ_STACK

Since do_IRQ is now invoked on a separate IRQ stack, we select
HAVE_IRQ_EXIT_ON_IRQ_STACK so that softirq's may be invoked directly
from irq_exit(), rather than requiring do_softirq_own_stack.

Signed-off-by: Matt Redfearn <[email protected]>
---

arch/mips/Kconfig | 1 +
1 file changed, 1 insertion(+)

diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index b3c5bde43d34..80832aa8e4fb 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -9,6 +9,7 @@ config MIPS
select HAVE_CONTEXT_TRACKING
select HAVE_GENERIC_DMA_COHERENT
select HAVE_IDE
+ select HAVE_IRQ_EXIT_ON_IRQ_STACK
select HAVE_OPROFILE
select HAVE_PERF_EVENTS
select PERF_USE_VMALLOC
--
2.7.4

2016-12-02 13:39:42

by Matt Redfearn

[permalink] [raw]
Subject: [PATCH 4/5] MIPS: Switch to the irq_stack in interrupts

When enterring interrupt context via handle_int or except_vec_vi, switch
to the irq_stack of the current CPU if it is not already in use.

The current stack pointer is masked with the thread size and compared to
the base or the irq stack. If it does not match then the stack pointer
is set to the top of that stack, otherwise this is a nested irq being
handled on the irq stack so the stack pointer should be left as it was.

The in-use stack pointer is placed in the callee saved register s1. It
will be saved to the stack when plat_irq_dispatch is invoked and can be
restored once control returns here.

Signed-off-by: Matt Redfearn <[email protected]>
---

arch/mips/kernel/genex.S | 81 +++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 76 insertions(+), 5 deletions(-)

diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
index dc0b29612891..0a7ba4b2f687 100644
--- a/arch/mips/kernel/genex.S
+++ b/arch/mips/kernel/genex.S
@@ -187,9 +187,44 @@ NESTED(handle_int, PT_SIZE, sp)

LONG_L s0, TI_REGS($28)
LONG_S sp, TI_REGS($28)
- PTR_LA ra, ret_from_irq
- PTR_LA v0, plat_irq_dispatch
- jr v0
+
+ /*
+ * SAVE_ALL ensures we are using a valid kernel stack for the thread.
+ * Check if we are already using the IRQ stack.
+ */
+ move s1, sp # Preserve the sp
+
+ /* Get IRQ stack for this CPU */
+ ASM_CPUID_MFC0 k0, ASM_SMP_CPUID_REG
+#if defined(CONFIG_32BIT) || defined(KBUILD_64BIT_SYM32)
+ lui k1, %hi(irq_stack)
+#else
+ lui k1, %highest(irq_stack)
+ daddiu k1, %higher(irq_stack)
+ dsll k1, 16
+ daddiu k1, %hi(irq_stack)
+ dsll k1, 16
+#endif
+ LONG_SRL k0, SMP_CPUID_PTRSHIFT
+ LONG_ADDU k1, k0
+ LONG_L t0, %lo(irq_stack)(k1)
+
+ # Check if already on IRQ stack
+ PTR_LI t1, ~(_THREAD_SIZE-1)
+ and t1, t1, sp
+ beq t0, t1, 2f
+
+ /* Switch to IRQ stack */
+ li t1, _IRQ_STACK_SIZE
+ PTR_ADD sp, t0, t1
+
+2:
+ jal plat_irq_dispatch
+
+ /* Restore sp */
+ move sp, s1
+
+ j ret_from_irq
#ifdef CONFIG_CPU_MICROMIPS
nop
#endif
@@ -262,8 +297,44 @@ NESTED(except_vec_vi_handler, 0, sp)

LONG_L s0, TI_REGS($28)
LONG_S sp, TI_REGS($28)
- PTR_LA ra, ret_from_irq
- jr v0
+
+ /*
+ * SAVE_ALL ensures we are using a valid kernel stack for the thread.
+ * Check if we are already using the IRQ stack.
+ */
+ move s1, sp # Preserve the sp
+
+ /* Get IRQ stack for this CPU */
+ ASM_CPUID_MFC0 k0, ASM_SMP_CPUID_REG
+#if defined(CONFIG_32BIT) || defined(KBUILD_64BIT_SYM32)
+ lui k1, %hi(irq_stack)
+#else
+ lui k1, %highest(irq_stack)
+ daddiu k1, %higher(irq_stack)
+ dsll k1, 16
+ daddiu k1, %hi(irq_stack)
+ dsll k1, 16
+#endif
+ LONG_SRL k0, SMP_CPUID_PTRSHIFT
+ LONG_ADDU k1, k0
+ LONG_L t0, %lo(irq_stack)(k1)
+
+ # Check if already on IRQ stack
+ PTR_LI t1, ~(_THREAD_SIZE-1)
+ and t1, t1, sp
+ beq t0, t1, 2f
+
+ /* Switch to IRQ stack */
+ li t1, _IRQ_STACK_SIZE
+ PTR_ADD sp, t0, t1
+
+2:
+ jal plat_irq_dispatch
+
+ /* Restore sp */
+ move sp, s1
+
+ j ret_from_irq
END(except_vec_vi_handler)

/*
--
2.7.4

2016-12-02 13:40:16

by Matt Redfearn

[permalink] [raw]
Subject: [PATCH 3/5] MIPS: Only change $28 to thread_info if coming from user mode

The SAVE_SOME macro is used to save the execution context on all
exceptions.
If an exception occurs while executing user code, the stack is switched
to the kernel's stack for the current task, and register $28 is switched
to point to the current_thread_info, which is at the bottom of the stack
region.
If the exception occurs while executing kernel code, the stack is left,
and this change ensures that register $28 is not updated. This is the
correct behaviour when the kernel can be executing on the separate irq
stack, because the thread_info will not be at the base of it.

With this change, register $28 is only switched to it's kernel
conventional usage of the currrent thread info pointer at the point at
which execution enters kernel space. Doing it on every exception was
redundant, but OK without an IRQ stack, but will be erroneous once that
is introduced.

Signed-off-by: Matt Redfearn <[email protected]>
---

arch/mips/include/asm/stackframe.h | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/arch/mips/include/asm/stackframe.h b/arch/mips/include/asm/stackframe.h
index eebf39549606..5782fa3d63be 100644
--- a/arch/mips/include/asm/stackframe.h
+++ b/arch/mips/include/asm/stackframe.h
@@ -216,12 +216,22 @@
LONG_S $25, PT_R25(sp)
LONG_S $28, PT_R28(sp)
LONG_S $31, PT_R31(sp)
+
+ /* Set thread_info if we're coming from user mode */
+ .set reorder
+ mfc0 k0, CP0_STATUS
+ sll k0, 3 /* extract cu0 bit */
+ .set noreorder
+ bltz k0, 9f
+ nop
+
ori $28, sp, _THREAD_MASK
xori $28, _THREAD_MASK
#ifdef CONFIG_CPU_CAVIUM_OCTEON
.set mips64
pref 0, 0($28) /* Prefetch the current pointer */
#endif
+9:
.set pop
.endm

--
2.7.4

2016-12-02 13:40:37

by Matt Redfearn

[permalink] [raw]
Subject: [PATCH 1/5] MIPS: Introduce irq_stack

Allocate a per-cpu irq stack for use within interrupt handlers.

Also add a utility function on_irq_stack to determine if a given stack
pointer is within the irq stack for that cpu.

Signed-off-by: Matt Redfearn <[email protected]>

---

arch/mips/include/asm/irq.h | 12 ++++++++++++
arch/mips/kernel/asm-offsets.c | 1 +
arch/mips/kernel/irq.c | 11 +++++++++++
3 files changed, 24 insertions(+)

diff --git a/arch/mips/include/asm/irq.h b/arch/mips/include/asm/irq.h
index 6bf10e796553..956db6e201d1 100644
--- a/arch/mips/include/asm/irq.h
+++ b/arch/mips/include/asm/irq.h
@@ -17,6 +17,18 @@

#include <irq.h>

+#define IRQ_STACK_SIZE THREAD_SIZE
+
+extern void *irq_stack[NR_CPUS];
+
+static inline bool on_irq_stack(int cpu, unsigned long sp)
+{
+ unsigned long low = (unsigned long)irq_stack[cpu];
+ unsigned long high = low + IRQ_STACK_SIZE;
+
+ return (low <= sp && sp <= high);
+}
+
#ifdef CONFIG_I8259
static inline int irq_canonicalize(int irq)
{
diff --git a/arch/mips/kernel/asm-offsets.c b/arch/mips/kernel/asm-offsets.c
index fae2f9447792..4be2763f835d 100644
--- a/arch/mips/kernel/asm-offsets.c
+++ b/arch/mips/kernel/asm-offsets.c
@@ -102,6 +102,7 @@ void output_thread_info_defines(void)
OFFSET(TI_REGS, thread_info, regs);
DEFINE(_THREAD_SIZE, THREAD_SIZE);
DEFINE(_THREAD_MASK, THREAD_MASK);
+ DEFINE(_IRQ_STACK_SIZE, IRQ_STACK_SIZE);
BLANK();
}

diff --git a/arch/mips/kernel/irq.c b/arch/mips/kernel/irq.c
index f25f7eab7307..2b0a371b42af 100644
--- a/arch/mips/kernel/irq.c
+++ b/arch/mips/kernel/irq.c
@@ -25,6 +25,8 @@
#include <linux/atomic.h>
#include <asm/uaccess.h>

+void *irq_stack[NR_CPUS];
+
/*
* 'what should we do if we get a hw irq event on an illegal vector'.
* each architecture has to answer this themselves.
@@ -58,6 +60,15 @@ void __init init_IRQ(void)
clear_c0_status(ST0_IM);

arch_init_irq();
+
+ for_each_possible_cpu(i) {
+ int irq_pages = IRQ_STACK_SIZE / PAGE_SIZE;
+ void *s = (void *)__get_free_pages(GFP_KERNEL, irq_pages);
+
+ irq_stack[i] = s;
+ pr_debug("CPU%d IRQ stack at 0x%p - 0x%p\n", i,
+ irq_stack[i], irq_stack[i] + IRQ_STACK_SIZE);
+ }
}

#ifdef CONFIG_DEBUG_STACKOVERFLOW
--
2.7.4

2016-12-05 16:20:44

by Maciej W. Rozycki

[permalink] [raw]
Subject: Re: [PATCH 3/5] MIPS: Only change $28 to thread_info if coming from user mode

On Fri, 2 Dec 2016, Matt Redfearn wrote:

> diff --git a/arch/mips/include/asm/stackframe.h b/arch/mips/include/asm/stackframe.h
> index eebf39549606..5782fa3d63be 100644
> --- a/arch/mips/include/asm/stackframe.h
> +++ b/arch/mips/include/asm/stackframe.h
> @@ -216,12 +216,22 @@
> LONG_S $25, PT_R25(sp)
> LONG_S $28, PT_R28(sp)
> LONG_S $31, PT_R31(sp)
> +
> + /* Set thread_info if we're coming from user mode */
> + .set reorder
> + mfc0 k0, CP0_STATUS
> + sll k0, 3 /* extract cu0 bit */
> + .set noreorder
> + bltz k0, 9f
> + nop

This code is already `.set reorder', although a badly applied CONFIG_EVA
change made things slightly less obvious. So why do you need this `.set
reorder' in the first place, and then why do you switch code that follows
to `.set noreorder'?

Overall I think all <asm/stackframe.h> code should be using the (default)
`.set reorder' mode, perhaps forced explicitly in case these macros are
pasted into `.set noreorder' code, to make it easier to avoid subtle data
dependency bugs, and also to make R6 porting easier. Except maybe for the
RFE sequence, for readability's sake, although even there GAS will do the
right thing. Surely the BLTZ/MOVE piece does not have to be `.set
noreorder' as GAS will schedule that delay slot automatically if allowed
to.

Maciej

2016-12-05 16:53:31

by Matt Redfearn

[permalink] [raw]
Subject: Re: [PATCH 3/5] MIPS: Only change $28 to thread_info if coming from user mode


Hi Maciej,

On 05/12/16 16:20, Maciej W. Rozycki wrote:
> On Fri, 2 Dec 2016, Matt Redfearn wrote:
>
>> diff --git a/arch/mips/include/asm/stackframe.h b/arch/mips/include/asm/stackframe.h
>> index eebf39549606..5782fa3d63be 100644
>> --- a/arch/mips/include/asm/stackframe.h
>> +++ b/arch/mips/include/asm/stackframe.h
>> @@ -216,12 +216,22 @@
>> LONG_S $25, PT_R25(sp)
>> LONG_S $28, PT_R28(sp)
>> LONG_S $31, PT_R31(sp)
>> +
>> + /* Set thread_info if we're coming from user mode */
>> + .set reorder
>> + mfc0 k0, CP0_STATUS
>> + sll k0, 3 /* extract cu0 bit */
>> + .set noreorder
>> + bltz k0, 9f
>> + nop
> This code is already `.set reorder', although a badly applied CONFIG_EVA
> change made things slightly less obvious. So why do you need this `.set
> reorder' in the first place, and then why do you switch code that follows
> to `.set noreorder'?

Ah yes, I missed the .set reorder above the EVA ifdef and just included
the .set reorder as the similar snippet here:
http://lxr.free-electrons.com/source/arch/mips/include/asm/stackframe.h#L149

I'll revise that in V2.

Thanks,
Matt

>
> Overall I think all <asm/stackframe.h> code should be using the (default)
> `.set reorder' mode, perhaps forced explicitly in case these macros are
> pasted into `.set noreorder' code, to make it easier to avoid subtle data
> dependency bugs, and also to make R6 porting easier. Except maybe for the
> RFE sequence, for readability's sake, although even there GAS will do the
> right thing. Surely the BLTZ/MOVE piece does not have to be `.set
> noreorder' as GAS will schedule that delay slot automatically if allowed
> to.
>
> Maciej

2016-12-05 17:22:55

by Maciej W. Rozycki

[permalink] [raw]
Subject: Re: [PATCH 3/5] MIPS: Only change $28 to thread_info if coming from user mode

On Mon, 5 Dec 2016, Matt Redfearn wrote:

> Ah yes, I missed the .set reorder above the EVA ifdef and just included the
> .set reorder as the similar snippet here:
> http://lxr.free-electrons.com/source/arch/mips/include/asm/stackframe.h#L149

That's a global `.set reorder' for the whole of SAVE_SOME, just as I
suggested for all the macros here.

Then a temporary `.set noreorder' override follows just for the manually
scheduled BLTZ/MOVE sequence, where someone has later inserted a whole
CONFIG_EVA block. As I noted this temporary override is unnecessary.
Incidentally it does not harm the CONFIG_EVA block either, because EVA
processors have MFC0 interlocked.

NB I think the MFC0 macro ought to be called REG_MFC0, to avoid confusion
and for consistency with the rest of such macros.

Maciej

2016-12-05 17:27:29

by Paul Burton

[permalink] [raw]
Subject: Re: [PATCH 3/5] MIPS: Only change $28 to thread_info if coming from user mode

Hi Maciej,

> Overall I think all <asm/stackframe.h> code should be using the (default)
> `.set reorder' mode, perhaps forced explicitly in case these macros are
> pasted into `.set noreorder' code, to make it easier to avoid subtle data
> dependency bugs, and also to make R6 porting easier. Except maybe for the
> RFE sequence, for readability's sake, although even there GAS will do the
> right thing. Surely the BLTZ/MOVE piece does not have to be `.set
> noreorder' as GAS will schedule that delay slot automatically if allowed
> to.

Agreed we ought to use .set reorder (or rather, not use .set noreorder)
wherever possible but FYI one thing I've only noticed recently is that we
don't actually get any reordering anyway, presumably because we don't provide
any -O flags when building asm source. I haven't yet done the legwork or
figuring out whether we've ever had optimisation & if so what changed...

Thanks,
Paul


Attachments:
signature.asc (833.00 B)
This is a digitally signed message part.

2016-12-05 17:58:30

by Maciej W. Rozycki

[permalink] [raw]
Subject: Re: [PATCH 3/5] MIPS: Only change $28 to thread_info if coming from user mode

On Mon, 5 Dec 2016, Paul Burton wrote:

> Agreed we ought to use .set reorder (or rather, not use .set noreorder)
> wherever possible but FYI one thing I've only noticed recently is that we
> don't actually get any reordering anyway, presumably because we don't provide
> any -O flags when building asm source. I haven't yet done the legwork or
> figuring out whether we've ever had optimisation & if so what changed...

Reordering or `-O2' is the default for GAS, you actually have to pass a
different `-O*' option explicitly to GAS to disable it. NB it's *not* the
same as passing one of the `-O*' options to GCC, you'd need e.g. `-Wa,-O'.

So perhaps one of our Makefiles or a particular configuration of the GCC
driver does the wrong thing?

Maciej

2016-12-06 22:10:06

by Jason A. Donenfeld

[permalink] [raw]
Subject: Re: [PATCH 0/5] MIPS: Add per-cpu IRQ stack

Hi Matt,

Thanks for submitting this. A happy OpenWRT/WireGuard user has
reported to me that this patch set frees ~1300 bytes of stack on a
small MIPS router. This kind of savings should allow me to reintroduce
my crypto operations to be on the stack, rather than the conditional
MIPS kmallocing, which will be a performance win.

By the way, it looks like x86 and SPARC have separately allocated hard
IRQ and soft IRQ stacks. ARM has only one for both, similar to this
MIPS patchset.

Jason

On Fri, Dec 2, 2016 at 2:39 PM, Matt Redfearn <[email protected]> wrote:
>
> This series adds a separate stack for each CPU wihin the system to use
> when handling IRQs. Previously IRQs were handled on the kernel stack of
> the current task. If that task was deep down a call stack at the point
> of the interrupt, and handling the interrupt required a deep IRQ stack,
> then there was a likelihood of stack overflow. Since the kernel stack
> is in normal unmapped memory, overflowing it can lead to silent
> corruption of other kernel data, with weird and wonderful results.
>
> Before this patch series, ftracing the maximum stack size of a v4.9-rc6
> kernel running on a Ci40 board gave:
> 4996
>
> And with this series:
> 4084
>
> Handling interrupts on a separate stack reduces the maximum kernel stack
> usage in this configuration by ~900 bytes.
>
> Since do_IRQ is now invoked on a separate stack, we select
> HAVE_IRQ_EXIT_ON_IRQ_STACK so that softirqs will also be executed on the
> irq stack rather than attempting to switch with do_softirq_own_stack().
>
> This series has been tested on MIPS Boston, Malta and SEAD3 platforms,
> Pistachio on the Creator Ci40 board and Cavium Octeon III.
>
>
>
> Matt Redfearn (5):
> MIPS: Introduce irq_stack
> MIPS: Stack unwinding while on IRQ stack
> MIPS: Only change $28 to thread_info if coming from user mode
> MIPS: Switch to the irq_stack in interrupts
> MIPS: Select HAVE_IRQ_EXIT_ON_IRQ_STACK
>
> arch/mips/Kconfig | 1 +
> arch/mips/include/asm/irq.h | 12 ++++++
> arch/mips/include/asm/stackframe.h | 10 +++++
> arch/mips/kernel/asm-offsets.c | 1 +
> arch/mips/kernel/genex.S | 81 +++++++++++++++++++++++++++++++++++---
> arch/mips/kernel/irq.c | 11 ++++++
> arch/mips/kernel/process.c | 15 ++++++-
> 7 files changed, 125 insertions(+), 6 deletions(-)
>
> --
> 2.7.4
>

2016-12-06 22:13:36

by Jason A. Donenfeld

[permalink] [raw]
Subject: Re: [PATCH 1/5] MIPS: Introduce irq_stack

On Fri, Dec 2, 2016 at 2:39 PM, Matt Redfearn <[email protected]> wrote:
> +void *irq_stack[NR_CPUS];

I'm curious why you implemented it this way rather than using
DEFINE_PER_CPU and the related percpu helper functions.

2016-12-07 09:25:57

by Matt Redfearn

[permalink] [raw]
Subject: Re: [PATCH 1/5] MIPS: Introduce irq_stack

Hi Jason,


On 06/12/16 22:13, Jason A. Donenfeld wrote:
> On Fri, Dec 2, 2016 at 2:39 PM, Matt Redfearn <[email protected]> wrote:
>> +void *irq_stack[NR_CPUS];
> I'm curious why you implemented it this way rather than using
> DEFINE_PER_CPU and the related percpu helper functions.
Because in the IRQ entry point in assembler we have to look up the
address of this CPU's IRQ stack. Doing so with a simple array can be
done with fewer instructions than a per-cpu variable. The kernel stack
pointer for each CPU is held in a similar array.

MIPS does not have a particularly optimized per-cpu implementation with
the per-cpu offset being held somewhere easily accessible, so right now
it has be looked up from the __per_cpu_offset array, and then applied to
the per-cpu pointer. Obviously doing the first lookup is analogous to
what I am doing, and the subsequent application to the per-cpu pointer
would be additional instructions.

Thanks,
Matt

2016-12-07 09:30:27

by Matt Redfearn

[permalink] [raw]
Subject: Re: [PATCH 0/5] MIPS: Add per-cpu IRQ stack

Hi Jason,


On 06/12/16 22:09, Jason A. Donenfeld wrote:
> Hi Matt,
>
> Thanks for submitting this. A happy OpenWRT/WireGuard user has
> reported to me that this patch set frees ~1300 bytes of stack on a
> small MIPS router. This kind of savings should allow me to reintroduce
> my crypto operations to be on the stack, rather than the conditional
> MIPS kmallocing, which will be a performance win.

Excellent.

>
> By the way, it looks like x86 and SPARC have separately allocated hard
> IRQ and soft IRQ stacks. ARM has only one for both, similar to this
> MIPS patchset.

The separate stack for soft-IRQs could be added as well, but of course
there is a tradeoff against the additional memory consumed by it. I
believe that most soft IRQs will now be handled on the new irq stack,
because HAVE_IRQ_EXIT_ON_IRQ_STACK causes them to be invoked within
irq_exit, while still executing on the irq stack

Thanks,
Matt

>
> Jason
>
> On Fri, Dec 2, 2016 at 2:39 PM, Matt Redfearn <[email protected]> wrote:
>> This series adds a separate stack for each CPU wihin the system to use
>> when handling IRQs. Previously IRQs were handled on the kernel stack of
>> the current task. If that task was deep down a call stack at the point
>> of the interrupt, and handling the interrupt required a deep IRQ stack,
>> then there was a likelihood of stack overflow. Since the kernel stack
>> is in normal unmapped memory, overflowing it can lead to silent
>> corruption of other kernel data, with weird and wonderful results.
>>
>> Before this patch series, ftracing the maximum stack size of a v4.9-rc6
>> kernel running on a Ci40 board gave:
>> 4996
>>
>> And with this series:
>> 4084
>>
>> Handling interrupts on a separate stack reduces the maximum kernel stack
>> usage in this configuration by ~900 bytes.
>>
>> Since do_IRQ is now invoked on a separate stack, we select
>> HAVE_IRQ_EXIT_ON_IRQ_STACK so that softirqs will also be executed on the
>> irq stack rather than attempting to switch with do_softirq_own_stack().
>>
>> This series has been tested on MIPS Boston, Malta and SEAD3 platforms,
>> Pistachio on the Creator Ci40 board and Cavium Octeon III.
>>
>>
>>
>> Matt Redfearn (5):
>> MIPS: Introduce irq_stack
>> MIPS: Stack unwinding while on IRQ stack
>> MIPS: Only change $28 to thread_info if coming from user mode
>> MIPS: Switch to the irq_stack in interrupts
>> MIPS: Select HAVE_IRQ_EXIT_ON_IRQ_STACK
>>
>> arch/mips/Kconfig | 1 +
>> arch/mips/include/asm/irq.h | 12 ++++++
>> arch/mips/include/asm/stackframe.h | 10 +++++
>> arch/mips/kernel/asm-offsets.c | 1 +
>> arch/mips/kernel/genex.S | 81 +++++++++++++++++++++++++++++++++++---
>> arch/mips/kernel/irq.c | 11 ++++++
>> arch/mips/kernel/process.c | 15 ++++++-
>> 7 files changed, 125 insertions(+), 6 deletions(-)
>>
>> --
>> 2.7.4
>>