Re: [PATCH v2] arm64: Introduce IRQ stack

[Jungseok Lee]

On Sep 21, 2015, at 6:25 PM, Catalin Marinas wrote:
> On Sat, Sep 19, 2015 at 05:44:37PM +0900, Jungseok Lee wrote:
>> On Sep 19, 2015, at 12:31 AM, Catalin Marinas wrote:
>>> On Fri, Sep 18, 2015 at 04:03:02PM +0100, Catalin Marinas wrote:
 On Fri, Sep 18, 2015 at 09:57:56PM +0900, Jungseok Lee wrote:
> On Sep 18, 2015, at 1:21 AM, Catalin Marinas wrote:
>> So, without any better suggestion for current_thread_info(), I'm giving
>> up the idea of using SPSel == 0 in the kernel. I'll look at your patch
>> in more detail. BTW, I don't think we need the any count for the irq
>> stack as we don't re-enter the same IRQ stack.
> [...]
> BTW, in this context, it is only meaningful to decide whether a current 
> interrupt
> is re-enterrant or not. Its actual value is not important, but I could 
> not figure
> out a better implementation than this one yet. Any suggestions are 
> welcome!
> [...]
>>> Another thought (it seems that x86 does something similar): we know the
>>> IRQ stack is not re-entered until interrupts are enabled in
>>> __do_softirq. If we enable __ARCH_HAS_DO_SOFTIRQ, we can implement an
>>> arm64-specific do_softirq_own_stack() which increments a counter before
>>> calling __do_softirq. The difference from your patch is that
>>> irq_stack_entry only reads such counter, doesn't need to write it.
>>> 
>>> Yet another idea is to reserve some space in the lower address part of
>>> the stack with a "stack type" information. It still requires another
>>> read, so I think the x86 approach is probably better.
>> 
>> I've realized both hardirq and softirq should be handled on a separate stack
>> in order to reduce kernel stack size, which is a principal objective of this
>> patch.
> 
> The objective is to reduce the kernel thread stack size (THREAD_SIZE).
> This can get pretty deep on some syscalls and together with IRQs (hard
> or soft), we run out of stack.
> 
> So, for now, just stick to reducing THREAD_SIZE by moving the IRQs off
> this stack. If we later find that hardirqs + softirqs can't fit on the
> same _IRQ_ stack, we could either increase it or allocate separate stack
> for softirqs. These are static anyway, allocated during boot. But I
> wouldn't get distracted with separate hard and soft IRQ stacks for now,
> I doubt we would see any issues (when a softirq runs, the IRQ stack is
> pretty much empty, apart from the pt_regs).

Completely agreed.

> 
>> (If I'm not missing something) It is not possible to get a big win
>> with implementing do_softirq_own_stack() since hardirq is handled using a 
>> task
>> stack. This prevents a size of kernel stack from being decreased.
> 
> What I meant is that hard and soft IRQs both run on the IRQ stack (not
> the thread stack). But instead of incrementing a counter every time you
> take a hard IRQ, just increment it in do_softirq_own_stack() with a
> simple read+check in elX_irq. The "own_stack" is not the most
> appropriate name because we still have the same IRQ stack but I'm not
> really bothered about this.

Personally I'm favor of James's top-bit comparison idea since there is no
{load|store} operation in irq_stack_exit macro by utilizing one of callee-
saved registers.

I will do re-spin soon with the changes based on feedbacks in this thread
for clarification. It would be better to trace a change history of this patch.

>> However, it would be meaningful to separate hard IRQ stack and soft IRQ one
>> as the next step.
> 
> Only if we see IRQ stack overflowing, otherwise I don't think it's worth
> the effort.

Okay.

Thanks for comments!

Best Regards
Jungseok Lee--
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 v2] arm64: Introduce IRQ stack

[Catalin Marinas]

On Sat, Sep 19, 2015 at 05:44:37PM +0900, Jungseok Lee wrote:
> On Sep 19, 2015, at 12:31 AM, Catalin Marinas wrote:
> > On Fri, Sep 18, 2015 at 04:03:02PM +0100, Catalin Marinas wrote:
> >> On Fri, Sep 18, 2015 at 09:57:56PM +0900, Jungseok Lee wrote:
> >>> On Sep 18, 2015, at 1:21 AM, Catalin Marinas wrote:
>  So, without any better suggestion for current_thread_info(), I'm giving
>  up the idea of using SPSel == 0 in the kernel. I'll look at your patch
>  in more detail. BTW, I don't think we need the any count for the irq
>  stack as we don't re-enter the same IRQ stack.
[...]
> >>> BTW, in this context, it is only meaningful to decide whether a current 
> >>> interrupt
> >>> is re-enterrant or not. Its actual value is not important, but I could 
> >>> not figure
> >>> out a better implementation than this one yet. Any suggestions are 
> >>> welcome!
[...]
> > Another thought (it seems that x86 does something similar): we know the
> > IRQ stack is not re-entered until interrupts are enabled in
> > __do_softirq. If we enable __ARCH_HAS_DO_SOFTIRQ, we can implement an
> > arm64-specific do_softirq_own_stack() which increments a counter before
> > calling __do_softirq. The difference from your patch is that
> > irq_stack_entry only reads such counter, doesn't need to write it.
> > 
> > Yet another idea is to reserve some space in the lower address part of
> > the stack with a "stack type" information. It still requires another
> > read, so I think the x86 approach is probably better.
> 
> I've realized both hardirq and softirq should be handled on a separate stack
> in order to reduce kernel stack size, which is a principal objective of this
> patch.

The objective is to reduce the kernel thread stack size (THREAD_SIZE).
This can get pretty deep on some syscalls and together with IRQs (hard
or soft), we run out of stack.

So, for now, just stick to reducing THREAD_SIZE by moving the IRQs off
this stack. If we later find that hardirqs + softirqs can't fit on the
same _IRQ_ stack, we could either increase it or allocate separate stack
for softirqs. These are static anyway, allocated during boot. But I
wouldn't get distracted with separate hard and soft IRQ stacks for now,
I doubt we would see any issues (when a softirq runs, the IRQ stack is
pretty much empty, apart from the pt_regs).

> (If I'm not missing something) It is not possible to get a big win
> with implementing do_softirq_own_stack() since hardirq is handled using a task
> stack. This prevents a size of kernel stack from being decreased.

What I meant is that hard and soft IRQs both run on the IRQ stack (not
the thread stack). But instead of incrementing a counter every time you
take a hard IRQ, just increment it in do_softirq_own_stack() with a
simple read+check in elX_irq. The "own_stack" is not the most
appropriate name because we still have the same IRQ stack but I'm not
really bothered about this.

> However, it would be meaningful to separate hard IRQ stack and soft IRQ one
> as the next step.

Only if we see IRQ stack overflowing, otherwise I don't think it's worth
the effort.

-- 
Catalin
--
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 v2] arm64: Introduce IRQ stack

[Jungseok Lee]

On Sep 21, 2015, at 6:25 PM, Catalin Marinas wrote:
> On Sat, Sep 19, 2015 at 05:44:37PM +0900, Jungseok Lee wrote:
>> On Sep 19, 2015, at 12:31 AM, Catalin Marinas wrote:
>>> On Fri, Sep 18, 2015 at 04:03:02PM +0100, Catalin Marinas wrote:
 On Fri, Sep 18, 2015 at 09:57:56PM +0900, Jungseok Lee wrote:
> On Sep 18, 2015, at 1:21 AM, Catalin Marinas wrote:
>> So, without any better suggestion for current_thread_info(), I'm giving
>> up the idea of using SPSel == 0 in the kernel. I'll look at your patch
>> in more detail. BTW, I don't think we need the any count for the irq
>> stack as we don't re-enter the same IRQ stack.
> [...]
> BTW, in this context, it is only meaningful to decide whether a current 
> interrupt
> is re-enterrant or not. Its actual value is not important, but I could 
> not figure
> out a better implementation than this one yet. Any suggestions are 
> welcome!
> [...]
>>> Another thought (it seems that x86 does something similar): we know the
>>> IRQ stack is not re-entered until interrupts are enabled in
>>> __do_softirq. If we enable __ARCH_HAS_DO_SOFTIRQ, we can implement an
>>> arm64-specific do_softirq_own_stack() which increments a counter before
>>> calling __do_softirq. The difference from your patch is that
>>> irq_stack_entry only reads such counter, doesn't need to write it.
>>> 
>>> Yet another idea is to reserve some space in the lower address part of
>>> the stack with a "stack type" information. It still requires another
>>> read, so I think the x86 approach is probably better.
>> 
>> I've realized both hardirq and softirq should be handled on a separate stack
>> in order to reduce kernel stack size, which is a principal objective of this
>> patch.
> 
> The objective is to reduce the kernel thread stack size (THREAD_SIZE).
> This can get pretty deep on some syscalls and together with IRQs (hard
> or soft), we run out of stack.
> 
> So, for now, just stick to reducing THREAD_SIZE by moving the IRQs off
> this stack. If we later find that hardirqs + softirqs can't fit on the
> same _IRQ_ stack, we could either increase it or allocate separate stack
> for softirqs. These are static anyway, allocated during boot. But I
> wouldn't get distracted with separate hard and soft IRQ stacks for now,
> I doubt we would see any issues (when a softirq runs, the IRQ stack is
> pretty much empty, apart from the pt_regs).

Completely agreed.

> 
>> (If I'm not missing something) It is not possible to get a big win
>> with implementing do_softirq_own_stack() since hardirq is handled using a 
>> task
>> stack. This prevents a size of kernel stack from being decreased.
> 
> What I meant is that hard and soft IRQs both run on the IRQ stack (not
> the thread stack). But instead of incrementing a counter every time you
> take a hard IRQ, just increment it in do_softirq_own_stack() with a
> simple read+check in elX_irq. The "own_stack" is not the most
> appropriate name because we still have the same IRQ stack but I'm not
> really bothered about this.

Personally I'm favor of James's top-bit comparison idea since there is no
{load|store} operation in irq_stack_exit macro by utilizing one of callee-
saved registers.

I will do re-spin soon with the changes based on feedbacks in this thread
for clarification. It would be better to trace a change history of this patch.

>> However, it would be meaningful to separate hard IRQ stack and soft IRQ one
>> as the next step.
> 
> Only if we see IRQ stack overflowing, otherwise I don't think it's worth
> the effort.

Okay.

Thanks for comments!

Best Regards
Jungseok Lee--
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 v2] arm64: Introduce IRQ stack

[Catalin Marinas]

On Sat, Sep 19, 2015 at 05:44:37PM +0900, Jungseok Lee wrote:
> On Sep 19, 2015, at 12:31 AM, Catalin Marinas wrote:
> > On Fri, Sep 18, 2015 at 04:03:02PM +0100, Catalin Marinas wrote:
> >> On Fri, Sep 18, 2015 at 09:57:56PM +0900, Jungseok Lee wrote:
> >>> On Sep 18, 2015, at 1:21 AM, Catalin Marinas wrote:
>  So, without any better suggestion for current_thread_info(), I'm giving
>  up the idea of using SPSel == 0 in the kernel. I'll look at your patch
>  in more detail. BTW, I don't think we need the any count for the irq
>  stack as we don't re-enter the same IRQ stack.
[...]
> >>> BTW, in this context, it is only meaningful to decide whether a current 
> >>> interrupt
> >>> is re-enterrant or not. Its actual value is not important, but I could 
> >>> not figure
> >>> out a better implementation than this one yet. Any suggestions are 
> >>> welcome!
[...]
> > Another thought (it seems that x86 does something similar): we know the
> > IRQ stack is not re-entered until interrupts are enabled in
> > __do_softirq. If we enable __ARCH_HAS_DO_SOFTIRQ, we can implement an
> > arm64-specific do_softirq_own_stack() which increments a counter before
> > calling __do_softirq. The difference from your patch is that
> > irq_stack_entry only reads such counter, doesn't need to write it.
> > 
> > Yet another idea is to reserve some space in the lower address part of
> > the stack with a "stack type" information. It still requires another
> > read, so I think the x86 approach is probably better.
> 
> I've realized both hardirq and softirq should be handled on a separate stack
> in order to reduce kernel stack size, which is a principal objective of this
> patch.

The objective is to reduce the kernel thread stack size (THREAD_SIZE).
This can get pretty deep on some syscalls and together with IRQs (hard
or soft), we run out of stack.

So, for now, just stick to reducing THREAD_SIZE by moving the IRQs off
this stack. If we later find that hardirqs + softirqs can't fit on the
same _IRQ_ stack, we could either increase it or allocate separate stack
for softirqs. These are static anyway, allocated during boot. But I
wouldn't get distracted with separate hard and soft IRQ stacks for now,
I doubt we would see any issues (when a softirq runs, the IRQ stack is
pretty much empty, apart from the pt_regs).

> (If I'm not missing something) It is not possible to get a big win
> with implementing do_softirq_own_stack() since hardirq is handled using a task
> stack. This prevents a size of kernel stack from being decreased.

What I meant is that hard and soft IRQs both run on the IRQ stack (not
the thread stack). But instead of incrementing a counter every time you
take a hard IRQ, just increment it in do_softirq_own_stack() with a
simple read+check in elX_irq. The "own_stack" is not the most
appropriate name because we still have the same IRQ stack but I'm not
really bothered about this.

> However, it would be meaningful to separate hard IRQ stack and soft IRQ one
> as the next step.

Only if we see IRQ stack overflowing, otherwise I don't think it's worth
the effort.

-- 
Catalin
--
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 v2] arm64: Introduce IRQ stack

[Jungseok Lee]

On Sep 19, 2015, at 12:31 AM, Catalin Marinas wrote:
> On Fri, Sep 18, 2015 at 04:03:02PM +0100, Catalin Marinas wrote:
>> On Fri, Sep 18, 2015 at 09:57:56PM +0900, Jungseok Lee wrote:
>>> On Sep 18, 2015, at 1:21 AM, Catalin Marinas wrote:
 So, without any better suggestion for current_thread_info(), I'm giving
 up the idea of using SPSel == 0 in the kernel. I'll look at your patch
 in more detail. BTW, I don't think we need the any count for the irq
 stack as we don't re-enter the same IRQ stack.
>>> 
>>> Another interrupt could come in since IRQ is enabled when handling softirq
>>> according to the following information which are self-evident.
>>> 
>>> (Am I missing something?)
>> 
>> No. I had the wrong impression that we switch to the softirqd stack for
>> softirqs but you are right, if we run them on the same stack the IRQs
>> are enabled and they can be re-entered before we return from this
>> exception handler.
>> 
>> I've seen other architectures implementing a dedicated softirq stack but
>> for now we should just re-use the current IRQ stack.
>> 
>>> In my first approach using SPSel = 0, current_thread_info function was 
>>> inefficient
>>> in order to handle this case correctly.
>> 
>> I agree. And we don't have any other scratch register left as we use
>> tpidr_el1 for per-cpu areas.
>> 
>>> BTW, in this context, it is only meaningful to decide whether a current 
>>> interrupt
>>> is re-enterrant or not. Its actual value is not important, but I could not 
>>> figure
>>> out a better implementation than this one yet. Any suggestions are welcome!
>> 
>> James' idea of checking the lower SP bits instead of a count may work.
> 
> Another thought (it seems that x86 does something similar): we know the
> IRQ stack is not re-entered until interrupts are enabled in
> __do_softirq. If we enable __ARCH_HAS_DO_SOFTIRQ, we can implement an
> arm64-specific do_softirq_own_stack() which increments a counter before
> calling __do_softirq. The difference from your patch is that
> irq_stack_entry only reads such counter, doesn't need to write it.
> 
> Yet another idea is to reserve some space in the lower address part of
> the stack with a "stack type" information. It still requires another
> read, so I think the x86 approach is probably better.

I've realized both hardirq and softirq should be handled on a separate stack
in order to reduce kernel stack size, which is a principal objective of this
patch. (If I'm not missing something) It is not possible to get a big win
with implementing do_softirq_own_stack() since hardirq is handled using a task
stack. This prevents a size of kernel stack from being decreased.

However, it would be meaningful to separate hard IRQ stack and soft IRQ one
as the next step.

Best Regards
Jungseok Lee--
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 v2] arm64: Introduce IRQ stack

[Jungseok Lee]

On Sep 18, 2015, at 10:46 PM, James Morse wrote:
> Hi Jungseok Lee,

Hi James Morse,

> I gave this a go on a Juno board, while generating usb/network interrupts:
> 
> Tested-by: James Morse 

Thanks a lot!

> On 13/09/15 15:42, Jungseok Lee wrote:
>> Currently, kernel context and interrupts are handled using a single
>> kernel stack navigated by sp_el1. This forces many systems to use
>> 16KB stack, not 8KB one. Low memory platforms naturally suffer from
>> memory pressure accompanied by performance degradation.
>> 
>> This patch addresses the issue as introducing a separate percpu IRQ
>> stack to handle both hard and soft interrupts with two ground rules:
>> 
>>  - Utilize sp_el0 in EL1 context, which is not used currently
>>  - Do not complicate current_thread_info calculation
>> 
>> It is a core concept to trace struct thread_info using sp_el0 instead
>> of sp_el1. This approach helps arm64 align with other architectures
>> regarding object_is_on_stack() without additional complexity.
> 
> I think you are missing a 'mov , sp; msr sp_el0, ' in
> kernel/sleep.S:cpu_resume():175. This code finds the saved stack pointer
> from 'sleep_save_sp', and is called when the cpu wakes up from suspend.

Make sense.

> It didn't show up in testing, because the wake-up is always under the idle
> task, which evidently doesn't call current_thread_info() after wake-up.

I've never seen any issues under suspend/resume scenario yet, but it is
logically reasonable to update sp_el0 in resume context.

> 
>> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
>> index 4306c93..c156540 100644
>> --- a/arch/arm64/kernel/entry.S
>> +++ b/arch/arm64/kernel/entry.S
>> @@ -88,7 +88,7 @@
>> 
>>  .if \el == 0
>>  mrs x21, sp_el0
>> -get_thread_info tsk // Ensure MDSCR_EL1.SS is clear,
>> +get_thread_info \el, tsk// Ensure MDSCR_EL1.SS is clear,
>>  ldr x19, [tsk, #TI_FLAGS]   // since we can unmask debug
>>  disable_step_tsk x19, x20   // exceptions when scheduling.
>>  .else
>> @@ -105,6 +105,8 @@
>>  .if \el == 0
>>  mvn x21, xzr
>>  str x21, [sp, #S_SYSCALLNO]
>> +mov x25, sp
>> +msr sp_el0, x25
>>  .endif
>> 
>>  /*
>> @@ -163,9 +165,45 @@ alternative_endif
>>  eret// return to kernel
>>  .endm
>> 
>> -.macro  get_thread_info, rd
>> +.macro  get_thread_info, el, rd
>> +.if \el == 0
> 
> Why does \el matter here?
> If \el==0, we interrupted an el0 thread, and set sp_el0 in kernel_entry()
> to the el1 stack.
> If \el==1, we interrupted an el1 thread, didn't overwrite its sp_el0, so
> sp_el0 & ~(THREAD_SIZE-1) will give us the struct thread_info of the
> interrupted task.
> 
> So either way, sp_el0 is correct…

You're right.

For the next version, I've written this macro with a single line as directly
accessing thread_info via sp_el0.

> 
>>  mov \rd, sp
>> -and \rd, \rd, #~(THREAD_SIZE - 1)   // top of stack
>> +.else
>> +mrs \rd, sp_el0
>> +.endif
>> +and \rd, \rd, #~(THREAD_SIZE - 1)   // bottom of thread stack
>> +.endm
>> +
>> +.macro  get_irq_stack
>> +adr_l   x21, irq_stacks
>> +mrs x22, tpidr_el1
>> +add x21, x21, x22
>> +.endm
>> +
>> +.macro  irq_stack_entry
>> +get_irq_stack
>> +ldr w23, [x21, #IRQ_COUNT]
>> +cbnzw23, 1f // check irq recursion
>> +mov x23, sp
>> +str x23, [x21, #IRQ_THREAD_SP]
>> +ldr x23, [x21, #IRQ_STACK]
>> +mov sp, x23
>> +mov x23, xzr
>> +1:  add w23, w23, #1
>> +str w23, [x21, #IRQ_COUNT]
> 
> A (largely untested) example for the 'compare the high-order bits' way of
> doing this:
> 
>   .macro  irq_stack_entry
>   get_irq_stack
>   ldr x22, [x21, #IRQ_STACK]
>   and x23, x22, #~(THREAD_SIZE -1)
>   mov x24, sp
>   and x24, x24, #~(THREAD_SIZE -1)
>   cmp x23, x24// irq_recursion?
>   mov x24, sp
>   cselx23, x24, x22, eq
>   mov sp, x23
>   .endm
> 
>   /* preserve x24 between irq_stack_entry/irq_stack_exit */
> 
>   .macro  irq_stack_exit
>   mov sp, x24
>   .endm
> 
> This would let you remove IRQ_COUNT and IRQ_THREAD_SP, and avoid the two
> stores and a conditional-branch in irq_stack_entry/irq_stack_exit.
> 
> Thoughts?

Great idea. Thanks to this change, about 20 lines can be removed.
It works well on my board till now.

> 
>> +.endm
>> +
>> +.macro  irq_stack_exit
>> +get_irq_stack
>> +ldr w23, [x21, #IRQ_COUNT]
>> +sub w23, w23, #1
>> +cbnzw23, 1f // check irq recursion
>> +mov x23, sp
>> +str x23, [x21, #IRQ_STACK]
>> +ldr x23, [x21, #IRQ_THREAD_SP]
>> +mov sp, x23
>> +mov x23, xzr
>> +1:  str w23, 

Re: [PATCH v2] arm64: Introduce IRQ stack

[Jungseok Lee]

On Sep 19, 2015, at 12:31 AM, Catalin Marinas wrote:
> On Fri, Sep 18, 2015 at 04:03:02PM +0100, Catalin Marinas wrote:
>> On Fri, Sep 18, 2015 at 09:57:56PM +0900, Jungseok Lee wrote:
>>> On Sep 18, 2015, at 1:21 AM, Catalin Marinas wrote:
 So, without any better suggestion for current_thread_info(), I'm giving
 up the idea of using SPSel == 0 in the kernel. I'll look at your patch
 in more detail. BTW, I don't think we need the any count for the irq
 stack as we don't re-enter the same IRQ stack.
>>> 
>>> Another interrupt could come in since IRQ is enabled when handling softirq
>>> according to the following information which are self-evident.
>>> 
>>> (Am I missing something?)
>> 
>> No. I had the wrong impression that we switch to the softirqd stack for
>> softirqs but you are right, if we run them on the same stack the IRQs
>> are enabled and they can be re-entered before we return from this
>> exception handler.
>> 
>> I've seen other architectures implementing a dedicated softirq stack but
>> for now we should just re-use the current IRQ stack.
>> 
>>> In my first approach using SPSel = 0, current_thread_info function was 
>>> inefficient
>>> in order to handle this case correctly.
>> 
>> I agree. And we don't have any other scratch register left as we use
>> tpidr_el1 for per-cpu areas.
>> 
>>> BTW, in this context, it is only meaningful to decide whether a current 
>>> interrupt
>>> is re-enterrant or not. Its actual value is not important, but I could not 
>>> figure
>>> out a better implementation than this one yet. Any suggestions are welcome!
>> 
>> James' idea of checking the lower SP bits instead of a count may work.
> 
> Another thought (it seems that x86 does something similar): we know the
> IRQ stack is not re-entered until interrupts are enabled in
> __do_softirq. If we enable __ARCH_HAS_DO_SOFTIRQ, we can implement an
> arm64-specific do_softirq_own_stack() which increments a counter before
> calling __do_softirq. The difference from your patch is that
> irq_stack_entry only reads such counter, doesn't need to write it.
> 
> Yet another idea is to reserve some space in the lower address part of
> the stack with a "stack type" information. It still requires another
> read, so I think the x86 approach is probably better.

I've realized both hardirq and softirq should be handled on a separate stack
in order to reduce kernel stack size, which is a principal objective of this
patch. (If I'm not missing something) It is not possible to get a big win
with implementing do_softirq_own_stack() since hardirq is handled using a task
stack. This prevents a size of kernel stack from being decreased.

However, it would be meaningful to separate hard IRQ stack and soft IRQ one
as the next step.

Best Regards
Jungseok Lee--
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 v2] arm64: Introduce IRQ stack

[Jungseok Lee]

On Sep 18, 2015, at 10:46 PM, James Morse wrote:
> Hi Jungseok Lee,

Hi James Morse,

> I gave this a go on a Juno board, while generating usb/network interrupts:
> 
> Tested-by: James Morse 

Thanks a lot!

> On 13/09/15 15:42, Jungseok Lee wrote:
>> Currently, kernel context and interrupts are handled using a single
>> kernel stack navigated by sp_el1. This forces many systems to use
>> 16KB stack, not 8KB one. Low memory platforms naturally suffer from
>> memory pressure accompanied by performance degradation.
>> 
>> This patch addresses the issue as introducing a separate percpu IRQ
>> stack to handle both hard and soft interrupts with two ground rules:
>> 
>>  - Utilize sp_el0 in EL1 context, which is not used currently
>>  - Do not complicate current_thread_info calculation
>> 
>> It is a core concept to trace struct thread_info using sp_el0 instead
>> of sp_el1. This approach helps arm64 align with other architectures
>> regarding object_is_on_stack() without additional complexity.
> 
> I think you are missing a 'mov , sp; msr sp_el0, ' in
> kernel/sleep.S:cpu_resume():175. This code finds the saved stack pointer
> from 'sleep_save_sp', and is called when the cpu wakes up from suspend.

Make sense.

> It didn't show up in testing, because the wake-up is always under the idle
> task, which evidently doesn't call current_thread_info() after wake-up.

I've never seen any issues under suspend/resume scenario yet, but it is
logically reasonable to update sp_el0 in resume context.

> 
>> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
>> index 4306c93..c156540 100644
>> --- a/arch/arm64/kernel/entry.S
>> +++ b/arch/arm64/kernel/entry.S
>> @@ -88,7 +88,7 @@
>> 
>>  .if \el == 0
>>  mrs x21, sp_el0
>> -get_thread_info tsk // Ensure MDSCR_EL1.SS is clear,
>> +get_thread_info \el, tsk// Ensure MDSCR_EL1.SS is clear,
>>  ldr x19, [tsk, #TI_FLAGS]   // since we can unmask debug
>>  disable_step_tsk x19, x20   // exceptions when scheduling.
>>  .else
>> @@ -105,6 +105,8 @@
>>  .if \el == 0
>>  mvn x21, xzr
>>  str x21, [sp, #S_SYSCALLNO]
>> +mov x25, sp
>> +msr sp_el0, x25
>>  .endif
>> 
>>  /*
>> @@ -163,9 +165,45 @@ alternative_endif
>>  eret// return to kernel
>>  .endm
>> 
>> -.macro  get_thread_info, rd
>> +.macro  get_thread_info, el, rd
>> +.if \el == 0
> 
> Why does \el matter here?
> If \el==0, we interrupted an el0 thread, and set sp_el0 in kernel_entry()
> to the el1 stack.
> If \el==1, we interrupted an el1 thread, didn't overwrite its sp_el0, so
> sp_el0 & ~(THREAD_SIZE-1) will give us the struct thread_info of the
> interrupted task.
> 
> So either way, sp_el0 is correct…

You're right.

For the next version, I've written this macro with a single line as directly
accessing thread_info via sp_el0.

> 
>>  mov \rd, sp
>> -and \rd, \rd, #~(THREAD_SIZE - 1)   // top of stack
>> +.else
>> +mrs \rd, sp_el0
>> +.endif
>> +and \rd, \rd, #~(THREAD_SIZE - 1)   // bottom of thread stack
>> +.endm
>> +
>> +.macro  get_irq_stack
>> +adr_l   x21, irq_stacks
>> +mrs x22, tpidr_el1
>> +add x21, x21, x22
>> +.endm
>> +
>> +.macro  irq_stack_entry
>> +get_irq_stack
>> +ldr w23, [x21, #IRQ_COUNT]
>> +cbnzw23, 1f // check irq recursion
>> +mov x23, sp
>> +str x23, [x21, #IRQ_THREAD_SP]
>> +ldr x23, [x21, #IRQ_STACK]
>> +mov sp, x23
>> +mov x23, xzr
>> +1:  add w23, w23, #1
>> +str w23, [x21, #IRQ_COUNT]
> 
> A (largely untested) example for the 'compare the high-order bits' way of
> doing this:
> 
>   .macro  irq_stack_entry
>   get_irq_stack
>   ldr x22, [x21, #IRQ_STACK]
>   and x23, x22, #~(THREAD_SIZE -1)
>   mov x24, sp
>   and x24, x24, #~(THREAD_SIZE -1)
>   cmp x23, x24// irq_recursion?
>   mov x24, sp
>   cselx23, x24, x22, eq
>   mov sp, x23
>   .endm
> 
>   /* preserve x24 between irq_stack_entry/irq_stack_exit */
> 
>   .macro  irq_stack_exit
>   mov sp, x24
>   .endm
> 
> This would let you remove IRQ_COUNT and IRQ_THREAD_SP, and avoid the two
> stores and a conditional-branch in irq_stack_entry/irq_stack_exit.
> 
> Thoughts?

Great idea. Thanks to this change, about 20 lines can be removed.
It works well on my board till now.

> 
>> +.endm
>> +
>> +.macro  irq_stack_exit
>> +get_irq_stack
>> +ldr w23, [x21, #IRQ_COUNT]
>> +sub w23, w23, #1
>> +cbnzw23, 1f // check irq recursion
>> +mov x23, sp
>> +str x23, [x21, #IRQ_STACK]
>> +ldr x23, [x21, #IRQ_THREAD_SP]
>> +mov sp, x23
>> +mov x23, xzr
>> 

Re: [PATCH v2] arm64: Introduce IRQ stack

[Catalin Marinas]

On Fri, Sep 18, 2015 at 04:03:02PM +0100, Catalin Marinas wrote:
> On Fri, Sep 18, 2015 at 09:57:56PM +0900, Jungseok Lee wrote:
> > On Sep 18, 2015, at 1:21 AM, Catalin Marinas wrote:
> > > So, without any better suggestion for current_thread_info(), I'm giving
> > > up the idea of using SPSel == 0 in the kernel. I'll look at your patch
> > > in more detail. BTW, I don't think we need the any count for the irq
> > > stack as we don't re-enter the same IRQ stack.
> > 
> > Another interrupt could come in since IRQ is enabled when handling softirq
> > according to the following information which are self-evident.
> > 
> > (Am I missing something?)
> 
> No. I had the wrong impression that we switch to the softirqd stack for
> softirqs but you are right, if we run them on the same stack the IRQs
> are enabled and they can be re-entered before we return from this
> exception handler.
> 
> I've seen other architectures implementing a dedicated softirq stack but
> for now we should just re-use the current IRQ stack.
> 
> > In my first approach using SPSel = 0, current_thread_info function was 
> > inefficient
> > in order to handle this case correctly.
> 
> I agree. And we don't have any other scratch register left as we use
> tpidr_el1 for per-cpu areas.
> 
> > BTW, in this context, it is only meaningful to decide whether a current 
> > interrupt
> > is re-enterrant or not. Its actual value is not important, but I could not 
> > figure
> > out a better implementation than this one yet. Any suggestions are welcome!
> 
> James' idea of checking the lower SP bits instead of a count may work.

Another thought (it seems that x86 does something similar): we know the
IRQ stack is not re-entered until interrupts are enabled in
__do_softirq. If we enable __ARCH_HAS_DO_SOFTIRQ, we can implement an
arm64-specific do_softirq_own_stack() which increments a counter before
calling __do_softirq. The difference from your patch is that
irq_stack_entry only reads such counter, doesn't need to write it.

Yet another idea is to reserve some space in the lower address part of
the stack with a "stack type" information. It still requires another
read, so I think the x86 approach is probably better.

-- 
Catalin
--
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 v2] arm64: Introduce IRQ stack

[Catalin Marinas]

On Fri, Sep 18, 2015 at 09:57:56PM +0900, Jungseok Lee wrote:
> On Sep 18, 2015, at 1:21 AM, Catalin Marinas wrote:
> > So, without any better suggestion for current_thread_info(), I'm giving
> > up the idea of using SPSel == 0 in the kernel. I'll look at your patch
> > in more detail. BTW, I don't think we need the any count for the irq
> > stack as we don't re-enter the same IRQ stack.
> 
> Another interrupt could come in since IRQ is enabled when handling softirq
> according to the following information which are self-evident.
> 
> (Am I missing something?)

No. I had the wrong impression that we switch to the softirqd stack for
softirqs but you are right, if we run them on the same stack the IRQs
are enabled and they can be re-entered before we return from this
exception handler.

I've seen other architectures implementing a dedicated softirq stack but
for now we should just re-use the current IRQ stack.

> In my first approach using SPSel = 0, current_thread_info function was 
> inefficient
> in order to handle this case correctly.

I agree. And we don't have any other scratch register left as we use
tpidr_el1 for per-cpu areas.

> BTW, in this context, it is only meaningful to decide whether a current 
> interrupt
> is re-enterrant or not. Its actual value is not important, but I could not 
> figure
> out a better implementation than this one yet. Any suggestions are welcome!

James' idea of checking the lower SP bits instead of a count may work.

-- 
Catalin
--
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 v2] arm64: Introduce IRQ stack

[James Morse]

Hi Jungseok Lee,

I gave this a go on a Juno board, while generating usb/network interrupts:

Tested-by: James Morse 


On 13/09/15 15:42, Jungseok Lee wrote:
> Currently, kernel context and interrupts are handled using a single
> kernel stack navigated by sp_el1. This forces many systems to use
> 16KB stack, not 8KB one. Low memory platforms naturally suffer from
> memory pressure accompanied by performance degradation.
> 
> This patch addresses the issue as introducing a separate percpu IRQ
> stack to handle both hard and soft interrupts with two ground rules:
> 
>   - Utilize sp_el0 in EL1 context, which is not used currently
>   - Do not complicate current_thread_info calculation
> 
> It is a core concept to trace struct thread_info using sp_el0 instead
> of sp_el1. This approach helps arm64 align with other architectures
> regarding object_is_on_stack() without additional complexity.

I think you are missing a 'mov , sp; msr sp_el0, ' in
kernel/sleep.S:cpu_resume():175. This code finds the saved stack pointer
from 'sleep_save_sp', and is called when the cpu wakes up from suspend.

It didn't show up in testing, because the wake-up is always under the idle
task, which evidently doesn't call current_thread_info() after wake-up.


> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 4306c93..c156540 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -88,7 +88,7 @@
>  
>   .if \el == 0
>   mrs x21, sp_el0
> - get_thread_info tsk // Ensure MDSCR_EL1.SS is clear,
> + get_thread_info \el, tsk// Ensure MDSCR_EL1.SS is clear,
>   ldr x19, [tsk, #TI_FLAGS]   // since we can unmask debug
>   disable_step_tsk x19, x20   // exceptions when scheduling.
>   .else
> @@ -105,6 +105,8 @@
>   .if \el == 0
>   mvn x21, xzr
>   str x21, [sp, #S_SYSCALLNO]
> + mov x25, sp
> + msr sp_el0, x25
>   .endif
>  
>   /*
> @@ -163,9 +165,45 @@ alternative_endif
>   eret// return to kernel
>   .endm
>  
> - .macro  get_thread_info, rd
> + .macro  get_thread_info, el, rd
> + .if \el == 0

Why does \el matter here?
If \el==0, we interrupted an el0 thread, and set sp_el0 in kernel_entry()
to the el1 stack.
If \el==1, we interrupted an el1 thread, didn't overwrite its sp_el0, so
sp_el0 & ~(THREAD_SIZE-1) will give us the struct thread_info of the
interrupted task.

So either way, sp_el0 is correct...


>   mov \rd, sp
> - and \rd, \rd, #~(THREAD_SIZE - 1)   // top of stack
> + .else
> + mrs \rd, sp_el0
> + .endif
> + and \rd, \rd, #~(THREAD_SIZE - 1)   // bottom of thread stack
> + .endm
> +
> + .macro  get_irq_stack
> + adr_l   x21, irq_stacks
> + mrs x22, tpidr_el1
> + add x21, x21, x22
> + .endm
> +
> + .macro  irq_stack_entry
> + get_irq_stack
> + ldr w23, [x21, #IRQ_COUNT]
> + cbnzw23, 1f // check irq recursion
> + mov x23, sp
> + str x23, [x21, #IRQ_THREAD_SP]
> + ldr x23, [x21, #IRQ_STACK]
> + mov sp, x23
> + mov x23, xzr
> +1:   add w23, w23, #1
> + str w23, [x21, #IRQ_COUNT]

A (largely untested) example for the 'compare the high-order bits' way of
doing this:

.macro  irq_stack_entry
get_irq_stack
ldr x22, [x21, #IRQ_STACK]
and x23, x22, #~(THREAD_SIZE -1)
mov x24, sp
and x24, x24, #~(THREAD_SIZE -1)
cmp x23, x24// irq_recursion?
mov x24, sp
cselx23, x24, x22, eq
mov sp, x23
.endm

/* preserve x24 between irq_stack_entry/irq_stack_exit */

.macro  irq_stack_exit
mov sp, x24
.endm

This would let you remove IRQ_COUNT and IRQ_THREAD_SP, and avoid the two
stores and a conditional-branch in irq_stack_entry/irq_stack_exit.

Thoughts?


> + .endm
> +
> + .macro  irq_stack_exit
> + get_irq_stack
> + ldr w23, [x21, #IRQ_COUNT]
> + sub w23, w23, #1
> + cbnzw23, 1f // check irq recursion
> + mov x23, sp
> + str x23, [x21, #IRQ_STACK]
> + ldr x23, [x21, #IRQ_THREAD_SP]
> + mov sp, x23
> + mov x23, xzr
> +1:   str w23, [x21, #IRQ_COUNT]
>   .endm
>  
>  /*
> @@ -183,10 +221,11 @@ tsk .reqx28 // current thread_info
>   * Interrupt handling.
>   */
>   .macro  irq_handler
> - adrpx1, handle_arch_irq
> - ldr x1, [x1, #:lo12:handle_arch_irq]
> + ldr_l   x1, handle_arch_irq
>   mov x0, sp
> + irq_stack_entry
>   blr x1
> + irq_stack_exit
>   .endm
>  
>   .text
> @@ -361,7 +400,7 @@ el1_irq:
>   irq_handler
>  
>  #ifdef CONFIG_PREEMPT
> - get_thread_info tsk
> + 

Re: [PATCH v2] arm64: Introduce IRQ stack

[James Morse]

On 18/09/15 13:57, Jungseok Lee wrote:
> On Sep 18, 2015, at 1:21 AM, Catalin Marinas wrote:
>> in more detail. BTW, I don't think we need the any count for the irq
>> stack as we don't re-enter the same IRQ stack.
> 
> Another interrupt could come in since IRQ is enabled when handling softirq
> according to the following information which are self-evident.
> 
> (Am I missing something?)
> 
> 1) kernel/softirq.c
> 
> asmlinkage __visible void __do_softirq(void)
> {
> unsigned long end = jiffies + MAX_SOFTIRQ_TIME;
> unsigned long old_flags = current->flags;
> int max_restart = MAX_SOFTIRQ_RESTART;
> struct softirq_action *h;
> bool in_hardirq;
> __u32 pending;
> int softirq_bit;
> 
> /*  
>  * Mask out PF_MEMALLOC s current task context is borrowed for the
>  * softirq. A softirq handled such as network RX might set PF_MEMALLOC
>  * again if the socket is related to swap
>  */
> current->flags &= ~PF_MEMALLOC;
> 
> pending = local_softirq_pending();
> account_irq_enter_time(current);
> 
> __local_bh_disable_ip(_RET_IP_, SOFTIRQ_OFFSET);
> in_hardirq = lockdep_softirq_start();
> 
> restart:
> /* Reset the pending bitmask before enabling irqs */
> set_softirq_pending(0);
> 
> local_irq_enable();

This call into __do_softirq() should be prevented by
kernel/softirq.c:irq_exit():
> preempt_count_sub(HARDIRQ_OFFSET);
> if (!in_interrupt() && local_softirq_pending())
>  invoke_softirq();

in_interrupt(), pulls preempt_count out of thread_info, and masks it with
(SOFTIRQ_MASK | HARDIRQ_MASK | NMI_MASK). This value is zero due to the
preempt_count_sub() immediately before.

preempt_count_add(HARDIRQ_OFFSET) is called in __irq_enter(), so its not
unreasonable that it decrements it here - but it is falling to zero,
causing softirqs to be handled during interrupt handling.


Despite the '!in_interrupt()', it looks like this is entirely intentional,
from invoke_softirq():
/*
 * We can safely execute softirq on the current stack if
 * it is the irq stack, because it should be near empty
 * at this stage.
 */


x86 has an additional preempt_count_{add,sub}(HARDIRQ_OFFSET) in
ist_{enter,exit}(), which would prevent this. They call this for
double_faults and debug-exceptions.

It looks like we need to 'preempt_count_add(HARDIRQ_OFFSET)' in el1_irq()
to prevent the fall-to-zero, to prevent recursive use of the irq stack -
alternatively I have a smaller set of asm for irq_stack_entry() which keeps
the status quo.



James



--
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 v2] arm64: Introduce IRQ stack

[Jungseok Lee]

On Sep 18, 2015, at 2:07 AM, Catalin Marinas wrote:
> On Thu, Sep 17, 2015 at 09:36:04PM +0900, Jungseok Lee wrote:
>> On Sep 17, 2015, at 7:33 PM, James Morse wrote:
>>> On 16/09/15 12:25, Will Deacon wrote:
 On Sun, Sep 13, 2015 at 03:42:17PM +0100, Jungseok Lee wrote:
> diff --git a/arch/arm64/include/asm/thread_info.h 
> b/arch/arm64/include/asm/thread_info.h
> index dcd06d1..44839c0 100644
> --- a/arch/arm64/include/asm/thread_info.h
> +++ b/arch/arm64/include/asm/thread_info.h
> @@ -73,8 +73,11 @@ static inline struct thread_info 
> *current_thread_info(void) __attribute_const__;
> 
> static inline struct thread_info *current_thread_info(void)
> {
> - return (struct thread_info *)
> - (current_stack_pointer & ~(THREAD_SIZE - 1));
> + unsigned long sp_el0;
> +
> + asm volatile("mrs %0, sp_el0" : "=r" (sp_el0));
> +
> + return (struct thread_info *)(sp_el0 & ~(THREAD_SIZE - 1));
 
 This looks like it will generate worse code than our current 
 implementation,
 thanks to the asm volatile. Maybe just add something like a global
 current_stack_pointer_el0?
> [...]
>> --- a/arch/arm64/include/asm/thread_info.h
>> +++ b/arch/arm64/include/asm/thread_info.h
>> @@ -77,7 +77,7 @@ static inline struct thread_info *current_thread_info(void)
>> 
>>  asm volatile("mrs %0, sp_el0" : "=r" (sp_el0));
>> 
>> -return (struct thread_info *)(sp_el0 & ~(THREAD_SIZE - 1));
>> +return (struct thread_info *)sp_el0;
>> }
> 
> This makes sense, since we just use sp_el0 as a scratch register, store
> the current thread_info address directly. But, as James mentioned, I
> don't think you need asm volatile, just asm (it has a small impact in my
> tests).

I will squash this change into the original one without volatile.

Best Regards
Jungseok Lee--
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 v2] arm64: Introduce IRQ stack

[Jungseok Lee]

On Sep 18, 2015, at 1:21 AM, Catalin Marinas wrote:
> On Thu, Sep 17, 2015 at 10:22:26PM +0900, Jungseok Lee wrote:
>> On Sep 17, 2015, at 10:17 PM, Jungseok Lee wrote:
>>> On Sep 17, 2015, at 8:17 PM, Catalin Marinas wrote:
 On Sun, Sep 13, 2015 at 02:42:17PM +, Jungseok Lee wrote:
> Currently, kernel context and interrupts are handled using a single
> kernel stack navigated by sp_el1. This forces many systems to use
> 16KB stack, not 8KB one. Low memory platforms naturally suffer from
> memory pressure accompanied by performance degradation.
> 
> This patch addresses the issue as introducing a separate percpu IRQ
> stack to handle both hard and soft interrupts with two ground rules:
> 
> - Utilize sp_el0 in EL1 context, which is not used currently
> - Do not complicate current_thread_info calculation
> 
> It is a core concept to trace struct thread_info using sp_el0 instead
> of sp_el1. This approach helps arm64 align with other architectures
> regarding object_is_on_stack() without additional complexity.
 
 I'm still trying to understand how this patch works. I initially thought
 that we would set SPSel = 0 while in kernel thread mode to make use of
 SP_EL0 but I can't find any such code. Do you still use SP_EL1 all the
 time and SP_EL0 just for temporary saving the thread stack?
>>> 
>>> Exactly.
>>> 
>>> My first approach was to set SPSel = 0 and implement EL1t Sync and IRQ.
>>> This idea originally comes from your comment [1]. A kernel thread could
>>> be handled easily and neatly, but it complicated current_thread_info
>>> calculation due to a user process.
>>> 
>>> Let's assume that a kernel thread uses SP_EL0 by default. When an interrupt
>>> comes in, a core jumps to EL1t IRQ. In case of a user process, a CPU goes
>>> into EL1h IRQ when an interrupt raises. To handle this scenario correctly,
>>> SPSel or spsr_el1 should be referenced. This reaches to quite big overhead
>>> in current_thread_info function.
>> 
>> This statement is described incorrectly. In case of user process, a CPU goes
>> into EL0 IRQ. Under this context, another interrupt could come in. At this
>> time, a core jumps to EL1h IRQ.
> 
> I don't I entirely follow you here.

My description was not clear enough. It's my fault.

> First of all, we don't allow re-entrant IRQs, they are disabled during
> handling (there are patches for NMI via IRQ priorities but these would
> be a special case on a different code path; for the current code, let's
> just assume that IRQs are not re-entrant).
> 
> Second, SPSel is automatically set to 1 when taking an exception. So we
> are guaranteed that the kernel entry code always switches to SP_EL1
> (EL1h mode).
> 
> My initial thought was to populate SP_EL1 per CPU as a handler stack and
> never change it afterwards. The entry code may continue to use SP_EL1 if
> in interrupt or switch to SP_EL0 and SPSel = 0 if in thread context.
> What I didn't realise is that SP_EL0 cannot be accessed directly when
> SPSel == 0, only as SP. This indeed complicates current_thread_info
> slightly.
> 
> I did some tests with using SPSel in current_thread_info() to read SP or
> SP_EL0 and it doesn't look good, it increased the .text section by 132KB
> (I may have been able to optimise it a bit but it is still quite large).
> With your approach to always use sp_el0, I get about 4KB larger .text
> section.
> 
> So, without any better suggestion for current_thread_info(), I'm giving
> up the idea of using SPSel == 0 in the kernel. I'll look at your patch
> in more detail. BTW, I don't think we need the any count for the irq
> stack as we don't re-enter the same IRQ stack.

Another interrupt could come in since IRQ is enabled when handling softirq
according to the following information which are self-evident.

(Am I missing something?)

1) kernel/softirq.c

asmlinkage __visible void __do_softirq(void)
{
unsigned long end = jiffies + MAX_SOFTIRQ_TIME;
unsigned long old_flags = current->flags;
int max_restart = MAX_SOFTIRQ_RESTART;
struct softirq_action *h;
bool in_hardirq;
__u32 pending;
int softirq_bit;

/*  
 * Mask out PF_MEMALLOC s current task context is borrowed for the
 * softirq. A softirq handled such as network RX might set PF_MEMALLOC
 * again if the socket is related to swap
 */
current->flags &= ~PF_MEMALLOC;

pending = local_softirq_pending();
account_irq_enter_time(current);

__local_bh_disable_ip(_RET_IP_, SOFTIRQ_OFFSET);
in_hardirq = lockdep_softirq_start();

restart:
/* Reset the pending bitmask before enabling irqs */
set_softirq_pending(0);

local_irq_enable();

2) Stack tracer on ftrace

DepthSize   Location(49 entries)
-   
  0) 4456  16   arch_counter_read+0xc/0x24
  1) 4440  16   

Re: [PATCH v2] arm64: Introduce IRQ stack

[Catalin Marinas]

On Fri, Sep 18, 2015 at 09:57:56PM +0900, Jungseok Lee wrote:
> On Sep 18, 2015, at 1:21 AM, Catalin Marinas wrote:
> > So, without any better suggestion for current_thread_info(), I'm giving
> > up the idea of using SPSel == 0 in the kernel. I'll look at your patch
> > in more detail. BTW, I don't think we need the any count for the irq
> > stack as we don't re-enter the same IRQ stack.
> 
> Another interrupt could come in since IRQ is enabled when handling softirq
> according to the following information which are self-evident.
> 
> (Am I missing something?)

No. I had the wrong impression that we switch to the softirqd stack for
softirqs but you are right, if we run them on the same stack the IRQs
are enabled and they can be re-entered before we return from this
exception handler.

I've seen other architectures implementing a dedicated softirq stack but
for now we should just re-use the current IRQ stack.

> In my first approach using SPSel = 0, current_thread_info function was 
> inefficient
> in order to handle this case correctly.

I agree. And we don't have any other scratch register left as we use
tpidr_el1 for per-cpu areas.

> BTW, in this context, it is only meaningful to decide whether a current 
> interrupt
> is re-enterrant or not. Its actual value is not important, but I could not 
> figure
> out a better implementation than this one yet. Any suggestions are welcome!

James' idea of checking the lower SP bits instead of a count may work.

-- 
Catalin
--
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 v2] arm64: Introduce IRQ stack

[James Morse]

Hi Jungseok Lee,

I gave this a go on a Juno board, while generating usb/network interrupts:

Tested-by: James Morse 


On 13/09/15 15:42, Jungseok Lee wrote:
> Currently, kernel context and interrupts are handled using a single
> kernel stack navigated by sp_el1. This forces many systems to use
> 16KB stack, not 8KB one. Low memory platforms naturally suffer from
> memory pressure accompanied by performance degradation.
> 
> This patch addresses the issue as introducing a separate percpu IRQ
> stack to handle both hard and soft interrupts with two ground rules:
> 
>   - Utilize sp_el0 in EL1 context, which is not used currently
>   - Do not complicate current_thread_info calculation
> 
> It is a core concept to trace struct thread_info using sp_el0 instead
> of sp_el1. This approach helps arm64 align with other architectures
> regarding object_is_on_stack() without additional complexity.

I think you are missing a 'mov , sp; msr sp_el0, ' in
kernel/sleep.S:cpu_resume():175. This code finds the saved stack pointer
from 'sleep_save_sp', and is called when the cpu wakes up from suspend.

It didn't show up in testing, because the wake-up is always under the idle
task, which evidently doesn't call current_thread_info() after wake-up.


> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 4306c93..c156540 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -88,7 +88,7 @@
>  
>   .if \el == 0
>   mrs x21, sp_el0
> - get_thread_info tsk // Ensure MDSCR_EL1.SS is clear,
> + get_thread_info \el, tsk// Ensure MDSCR_EL1.SS is clear,
>   ldr x19, [tsk, #TI_FLAGS]   // since we can unmask debug
>   disable_step_tsk x19, x20   // exceptions when scheduling.
>   .else
> @@ -105,6 +105,8 @@
>   .if \el == 0
>   mvn x21, xzr
>   str x21, [sp, #S_SYSCALLNO]
> + mov x25, sp
> + msr sp_el0, x25
>   .endif
>  
>   /*
> @@ -163,9 +165,45 @@ alternative_endif
>   eret// return to kernel
>   .endm
>  
> - .macro  get_thread_info, rd
> + .macro  get_thread_info, el, rd
> + .if \el == 0

Why does \el matter here?
If \el==0, we interrupted an el0 thread, and set sp_el0 in kernel_entry()
to the el1 stack.
If \el==1, we interrupted an el1 thread, didn't overwrite its sp_el0, so
sp_el0 & ~(THREAD_SIZE-1) will give us the struct thread_info of the
interrupted task.

So either way, sp_el0 is correct...


>   mov \rd, sp
> - and \rd, \rd, #~(THREAD_SIZE - 1)   // top of stack
> + .else
> + mrs \rd, sp_el0
> + .endif
> + and \rd, \rd, #~(THREAD_SIZE - 1)   // bottom of thread stack
> + .endm
> +
> + .macro  get_irq_stack
> + adr_l   x21, irq_stacks
> + mrs x22, tpidr_el1
> + add x21, x21, x22
> + .endm
> +
> + .macro  irq_stack_entry
> + get_irq_stack
> + ldr w23, [x21, #IRQ_COUNT]
> + cbnzw23, 1f // check irq recursion
> + mov x23, sp
> + str x23, [x21, #IRQ_THREAD_SP]
> + ldr x23, [x21, #IRQ_STACK]
> + mov sp, x23
> + mov x23, xzr
> +1:   add w23, w23, #1
> + str w23, [x21, #IRQ_COUNT]

A (largely untested) example for the 'compare the high-order bits' way of
doing this:

.macro  irq_stack_entry
get_irq_stack
ldr x22, [x21, #IRQ_STACK]
and x23, x22, #~(THREAD_SIZE -1)
mov x24, sp
and x24, x24, #~(THREAD_SIZE -1)
cmp x23, x24// irq_recursion?
mov x24, sp
cselx23, x24, x22, eq
mov sp, x23
.endm

/* preserve x24 between irq_stack_entry/irq_stack_exit */

.macro  irq_stack_exit
mov sp, x24
.endm

This would let you remove IRQ_COUNT and IRQ_THREAD_SP, and avoid the two
stores and a conditional-branch in irq_stack_entry/irq_stack_exit.

Thoughts?


> + .endm
> +
> + .macro  irq_stack_exit
> + get_irq_stack
> + ldr w23, [x21, #IRQ_COUNT]
> + sub w23, w23, #1
> + cbnzw23, 1f // check irq recursion
> + mov x23, sp
> + str x23, [x21, #IRQ_STACK]
> + ldr x23, [x21, #IRQ_THREAD_SP]
> + mov sp, x23
> + mov x23, xzr
> +1:   str w23, [x21, #IRQ_COUNT]
>   .endm
>  
>  /*
> @@ -183,10 +221,11 @@ tsk .reqx28 // current thread_info
>   * Interrupt handling.
>   */
>   .macro  irq_handler
> - adrpx1, handle_arch_irq
> - ldr x1, [x1, #:lo12:handle_arch_irq]
> + ldr_l   x1, handle_arch_irq
>   mov x0, sp
> + irq_stack_entry
>   blr x1
> + irq_stack_exit
>   .endm
>  
>   .text
> @@ -361,7 +400,7 @@ el1_irq:
>   irq_handler
>  
>  #ifdef CONFIG_PREEMPT
> - 

Re: [PATCH v2] arm64: Introduce IRQ stack

[Jungseok Lee]

On Sep 18, 2015, at 1:21 AM, Catalin Marinas wrote:
> On Thu, Sep 17, 2015 at 10:22:26PM +0900, Jungseok Lee wrote:
>> On Sep 17, 2015, at 10:17 PM, Jungseok Lee wrote:
>>> On Sep 17, 2015, at 8:17 PM, Catalin Marinas wrote:
 On Sun, Sep 13, 2015 at 02:42:17PM +, Jungseok Lee wrote:
> Currently, kernel context and interrupts are handled using a single
> kernel stack navigated by sp_el1. This forces many systems to use
> 16KB stack, not 8KB one. Low memory platforms naturally suffer from
> memory pressure accompanied by performance degradation.
> 
> This patch addresses the issue as introducing a separate percpu IRQ
> stack to handle both hard and soft interrupts with two ground rules:
> 
> - Utilize sp_el0 in EL1 context, which is not used currently
> - Do not complicate current_thread_info calculation
> 
> It is a core concept to trace struct thread_info using sp_el0 instead
> of sp_el1. This approach helps arm64 align with other architectures
> regarding object_is_on_stack() without additional complexity.
 
 I'm still trying to understand how this patch works. I initially thought
 that we would set SPSel = 0 while in kernel thread mode to make use of
 SP_EL0 but I can't find any such code. Do you still use SP_EL1 all the
 time and SP_EL0 just for temporary saving the thread stack?
>>> 
>>> Exactly.
>>> 
>>> My first approach was to set SPSel = 0 and implement EL1t Sync and IRQ.
>>> This idea originally comes from your comment [1]. A kernel thread could
>>> be handled easily and neatly, but it complicated current_thread_info
>>> calculation due to a user process.
>>> 
>>> Let's assume that a kernel thread uses SP_EL0 by default. When an interrupt
>>> comes in, a core jumps to EL1t IRQ. In case of a user process, a CPU goes
>>> into EL1h IRQ when an interrupt raises. To handle this scenario correctly,
>>> SPSel or spsr_el1 should be referenced. This reaches to quite big overhead
>>> in current_thread_info function.
>> 
>> This statement is described incorrectly. In case of user process, a CPU goes
>> into EL0 IRQ. Under this context, another interrupt could come in. At this
>> time, a core jumps to EL1h IRQ.
> 
> I don't I entirely follow you here.

My description was not clear enough. It's my fault.

> First of all, we don't allow re-entrant IRQs, they are disabled during
> handling (there are patches for NMI via IRQ priorities but these would
> be a special case on a different code path; for the current code, let's
> just assume that IRQs are not re-entrant).
> 
> Second, SPSel is automatically set to 1 when taking an exception. So we
> are guaranteed that the kernel entry code always switches to SP_EL1
> (EL1h mode).
> 
> My initial thought was to populate SP_EL1 per CPU as a handler stack and
> never change it afterwards. The entry code may continue to use SP_EL1 if
> in interrupt or switch to SP_EL0 and SPSel = 0 if in thread context.
> What I didn't realise is that SP_EL0 cannot be accessed directly when
> SPSel == 0, only as SP. This indeed complicates current_thread_info
> slightly.
> 
> I did some tests with using SPSel in current_thread_info() to read SP or
> SP_EL0 and it doesn't look good, it increased the .text section by 132KB
> (I may have been able to optimise it a bit but it is still quite large).
> With your approach to always use sp_el0, I get about 4KB larger .text
> section.
> 
> So, without any better suggestion for current_thread_info(), I'm giving
> up the idea of using SPSel == 0 in the kernel. I'll look at your patch
> in more detail. BTW, I don't think we need the any count for the irq
> stack as we don't re-enter the same IRQ stack.

Another interrupt could come in since IRQ is enabled when handling softirq
according to the following information which are self-evident.

(Am I missing something?)

1) kernel/softirq.c

asmlinkage __visible void __do_softirq(void)
{
unsigned long end = jiffies + MAX_SOFTIRQ_TIME;
unsigned long old_flags = current->flags;
int max_restart = MAX_SOFTIRQ_RESTART;
struct softirq_action *h;
bool in_hardirq;
__u32 pending;
int softirq_bit;

/*  
 * Mask out PF_MEMALLOC s current task context is borrowed for the
 * softirq. A softirq handled such as network RX might set PF_MEMALLOC
 * again if the socket is related to swap
 */
current->flags &= ~PF_MEMALLOC;

pending = local_softirq_pending();
account_irq_enter_time(current);

__local_bh_disable_ip(_RET_IP_, SOFTIRQ_OFFSET);
in_hardirq = lockdep_softirq_start();

restart:
/* Reset the pending bitmask before enabling irqs */
set_softirq_pending(0);

local_irq_enable();

2) Stack tracer on ftrace

DepthSize   Location(49 entries)
-   
  0) 4456  16   arch_counter_read+0xc/0x24
  1) 4440  16   

Re: [PATCH v2] arm64: Introduce IRQ stack

[Jungseok Lee]

On Sep 18, 2015, at 2:07 AM, Catalin Marinas wrote:
> On Thu, Sep 17, 2015 at 09:36:04PM +0900, Jungseok Lee wrote:
>> On Sep 17, 2015, at 7:33 PM, James Morse wrote:
>>> On 16/09/15 12:25, Will Deacon wrote:
 On Sun, Sep 13, 2015 at 03:42:17PM +0100, Jungseok Lee wrote:
> diff --git a/arch/arm64/include/asm/thread_info.h 
> b/arch/arm64/include/asm/thread_info.h
> index dcd06d1..44839c0 100644
> --- a/arch/arm64/include/asm/thread_info.h
> +++ b/arch/arm64/include/asm/thread_info.h
> @@ -73,8 +73,11 @@ static inline struct thread_info 
> *current_thread_info(void) __attribute_const__;
> 
> static inline struct thread_info *current_thread_info(void)
> {
> - return (struct thread_info *)
> - (current_stack_pointer & ~(THREAD_SIZE - 1));
> + unsigned long sp_el0;
> +
> + asm volatile("mrs %0, sp_el0" : "=r" (sp_el0));
> +
> + return (struct thread_info *)(sp_el0 & ~(THREAD_SIZE - 1));
 
 This looks like it will generate worse code than our current 
 implementation,
 thanks to the asm volatile. Maybe just add something like a global
 current_stack_pointer_el0?
> [...]
>> --- a/arch/arm64/include/asm/thread_info.h
>> +++ b/arch/arm64/include/asm/thread_info.h
>> @@ -77,7 +77,7 @@ static inline struct thread_info *current_thread_info(void)
>> 
>>  asm volatile("mrs %0, sp_el0" : "=r" (sp_el0));
>> 
>> -return (struct thread_info *)(sp_el0 & ~(THREAD_SIZE - 1));
>> +return (struct thread_info *)sp_el0;
>> }
> 
> This makes sense, since we just use sp_el0 as a scratch register, store
> the current thread_info address directly. But, as James mentioned, I
> don't think you need asm volatile, just asm (it has a small impact in my
> tests).

I will squash this change into the original one without volatile.

Best Regards
Jungseok Lee--
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 v2] arm64: Introduce IRQ stack

[James Morse]

On 18/09/15 13:57, Jungseok Lee wrote:
> On Sep 18, 2015, at 1:21 AM, Catalin Marinas wrote:
>> in more detail. BTW, I don't think we need the any count for the irq
>> stack as we don't re-enter the same IRQ stack.
> 
> Another interrupt could come in since IRQ is enabled when handling softirq
> according to the following information which are self-evident.
> 
> (Am I missing something?)
> 
> 1) kernel/softirq.c
> 
> asmlinkage __visible void __do_softirq(void)
> {
> unsigned long end = jiffies + MAX_SOFTIRQ_TIME;
> unsigned long old_flags = current->flags;
> int max_restart = MAX_SOFTIRQ_RESTART;
> struct softirq_action *h;
> bool in_hardirq;
> __u32 pending;
> int softirq_bit;
> 
> /*  
>  * Mask out PF_MEMALLOC s current task context is borrowed for the
>  * softirq. A softirq handled such as network RX might set PF_MEMALLOC
>  * again if the socket is related to swap
>  */
> current->flags &= ~PF_MEMALLOC;
> 
> pending = local_softirq_pending();
> account_irq_enter_time(current);
> 
> __local_bh_disable_ip(_RET_IP_, SOFTIRQ_OFFSET);
> in_hardirq = lockdep_softirq_start();
> 
> restart:
> /* Reset the pending bitmask before enabling irqs */
> set_softirq_pending(0);
> 
> local_irq_enable();

This call into __do_softirq() should be prevented by
kernel/softirq.c:irq_exit():
> preempt_count_sub(HARDIRQ_OFFSET);
> if (!in_interrupt() && local_softirq_pending())
>  invoke_softirq();

in_interrupt(), pulls preempt_count out of thread_info, and masks it with
(SOFTIRQ_MASK | HARDIRQ_MASK | NMI_MASK). This value is zero due to the
preempt_count_sub() immediately before.

preempt_count_add(HARDIRQ_OFFSET) is called in __irq_enter(), so its not
unreasonable that it decrements it here - but it is falling to zero,
causing softirqs to be handled during interrupt handling.


Despite the '!in_interrupt()', it looks like this is entirely intentional,
from invoke_softirq():
/*
 * We can safely execute softirq on the current stack if
 * it is the irq stack, because it should be near empty
 * at this stage.
 */


x86 has an additional preempt_count_{add,sub}(HARDIRQ_OFFSET) in
ist_{enter,exit}(), which would prevent this. They call this for
double_faults and debug-exceptions.

It looks like we need to 'preempt_count_add(HARDIRQ_OFFSET)' in el1_irq()
to prevent the fall-to-zero, to prevent recursive use of the irq stack -
alternatively I have a smaller set of asm for irq_stack_entry() which keeps
the status quo.



James



--
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 v2] arm64: Introduce IRQ stack

[Catalin Marinas]

On Fri, Sep 18, 2015 at 04:03:02PM +0100, Catalin Marinas wrote:
> On Fri, Sep 18, 2015 at 09:57:56PM +0900, Jungseok Lee wrote:
> > On Sep 18, 2015, at 1:21 AM, Catalin Marinas wrote:
> > > So, without any better suggestion for current_thread_info(), I'm giving
> > > up the idea of using SPSel == 0 in the kernel. I'll look at your patch
> > > in more detail. BTW, I don't think we need the any count for the irq
> > > stack as we don't re-enter the same IRQ stack.
> > 
> > Another interrupt could come in since IRQ is enabled when handling softirq
> > according to the following information which are self-evident.
> > 
> > (Am I missing something?)
> 
> No. I had the wrong impression that we switch to the softirqd stack for
> softirqs but you are right, if we run them on the same stack the IRQs
> are enabled and they can be re-entered before we return from this
> exception handler.
> 
> I've seen other architectures implementing a dedicated softirq stack but
> for now we should just re-use the current IRQ stack.
> 
> > In my first approach using SPSel = 0, current_thread_info function was 
> > inefficient
> > in order to handle this case correctly.
> 
> I agree. And we don't have any other scratch register left as we use
> tpidr_el1 for per-cpu areas.
> 
> > BTW, in this context, it is only meaningful to decide whether a current 
> > interrupt
> > is re-enterrant or not. Its actual value is not important, but I could not 
> > figure
> > out a better implementation than this one yet. Any suggestions are welcome!
> 
> James' idea of checking the lower SP bits instead of a count may work.

Another thought (it seems that x86 does something similar): we know the
IRQ stack is not re-entered until interrupts are enabled in
__do_softirq. If we enable __ARCH_HAS_DO_SOFTIRQ, we can implement an
arm64-specific do_softirq_own_stack() which increments a counter before
calling __do_softirq. The difference from your patch is that
irq_stack_entry only reads such counter, doesn't need to write it.

Yet another idea is to reserve some space in the lower address part of
the stack with a "stack type" information. It still requires another
read, so I think the x86 approach is probably better.

-- 
Catalin
--
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 v2] arm64: Introduce IRQ stack

[Catalin Marinas]

On Thu, Sep 17, 2015 at 09:36:04PM +0900, Jungseok Lee wrote:
> On Sep 17, 2015, at 7:33 PM, James Morse wrote:
> > On 16/09/15 12:25, Will Deacon wrote:
> >> On Sun, Sep 13, 2015 at 03:42:17PM +0100, Jungseok Lee wrote:
> >>> diff --git a/arch/arm64/include/asm/thread_info.h 
> >>> b/arch/arm64/include/asm/thread_info.h
> >>> index dcd06d1..44839c0 100644
> >>> --- a/arch/arm64/include/asm/thread_info.h
> >>> +++ b/arch/arm64/include/asm/thread_info.h
> >>> @@ -73,8 +73,11 @@ static inline struct thread_info 
> >>> *current_thread_info(void) __attribute_const__;
> >>> 
> >>> static inline struct thread_info *current_thread_info(void)
> >>> {
> >>> - return (struct thread_info *)
> >>> - (current_stack_pointer & ~(THREAD_SIZE - 1));
> >>> + unsigned long sp_el0;
> >>> +
> >>> + asm volatile("mrs %0, sp_el0" : "=r" (sp_el0));
> >>> +
> >>> + return (struct thread_info *)(sp_el0 & ~(THREAD_SIZE - 1));
> >> 
> >> This looks like it will generate worse code than our current 
> >> implementation,
> >> thanks to the asm volatile. Maybe just add something like a global
> >> current_stack_pointer_el0?
[...]
> --- a/arch/arm64/include/asm/thread_info.h
> +++ b/arch/arm64/include/asm/thread_info.h
> @@ -77,7 +77,7 @@ static inline struct thread_info *current_thread_info(void)
>  
>   asm volatile("mrs %0, sp_el0" : "=r" (sp_el0));
>  
> - return (struct thread_info *)(sp_el0 & ~(THREAD_SIZE - 1));
> + return (struct thread_info *)sp_el0;
>  }

This makes sense, since we just use sp_el0 as a scratch register, store
the current thread_info address directly. But, as James mentioned, I
don't think you need asm volatile, just asm (it has a small impact in my
tests).

-- 
Catalin
--
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 v2] arm64: Introduce IRQ stack

[Catalin Marinas]

On Thu, Sep 17, 2015 at 10:22:26PM +0900, Jungseok Lee wrote:
> On Sep 17, 2015, at 10:17 PM, Jungseok Lee wrote:
> > On Sep 17, 2015, at 8:17 PM, Catalin Marinas wrote:
> >> On Sun, Sep 13, 2015 at 02:42:17PM +, Jungseok Lee wrote:
> >>> Currently, kernel context and interrupts are handled using a single
> >>> kernel stack navigated by sp_el1. This forces many systems to use
> >>> 16KB stack, not 8KB one. Low memory platforms naturally suffer from
> >>> memory pressure accompanied by performance degradation.
> >>> 
> >>> This patch addresses the issue as introducing a separate percpu IRQ
> >>> stack to handle both hard and soft interrupts with two ground rules:
> >>> 
> >>> - Utilize sp_el0 in EL1 context, which is not used currently
> >>> - Do not complicate current_thread_info calculation
> >>> 
> >>> It is a core concept to trace struct thread_info using sp_el0 instead
> >>> of sp_el1. This approach helps arm64 align with other architectures
> >>> regarding object_is_on_stack() without additional complexity.
> >> 
> >> I'm still trying to understand how this patch works. I initially thought
> >> that we would set SPSel = 0 while in kernel thread mode to make use of
> >> SP_EL0 but I can't find any such code. Do you still use SP_EL1 all the
> >> time and SP_EL0 just for temporary saving the thread stack?
> > 
> > Exactly.
> > 
> > My first approach was to set SPSel = 0 and implement EL1t Sync and IRQ.
> > This idea originally comes from your comment [1]. A kernel thread could
> > be handled easily and neatly, but it complicated current_thread_info
> > calculation due to a user process.
> > 
> > Let's assume that a kernel thread uses SP_EL0 by default. When an interrupt
> > comes in, a core jumps to EL1t IRQ. In case of a user process, a CPU goes
> > into EL1h IRQ when an interrupt raises. To handle this scenario correctly,
> > SPSel or spsr_el1 should be referenced. This reaches to quite big overhead
> > in current_thread_info function.
> 
> This statement is described incorrectly. In case of user process, a CPU goes
> into EL0 IRQ. Under this context, another interrupt could come in. At this
> time, a core jumps to EL1h IRQ.

I don't I entirely follow you here.

First of all, we don't allow re-entrant IRQs, they are disabled during
handling (there are patches for NMI via IRQ priorities but these would
be a special case on a different code path; for the current code, let's
just assume that IRQs are not re-entrant).

Second, SPSel is automatically set to 1 when taking an exception. So we
are guaranteed that the kernel entry code always switches to SP_EL1
(EL1h mode).

My initial thought was to populate SP_EL1 per CPU as a handler stack and
never change it afterwards. The entry code may continue to use SP_EL1 if
in interrupt or switch to SP_EL0 and SPSel = 0 if in thread context.
What I didn't realise is that SP_EL0 cannot be accessed directly when
SPSel == 0, only as SP. This indeed complicates current_thread_info
slightly.

I did some tests with using SPSel in current_thread_info() to read SP or
SP_EL0 and it doesn't look good, it increased the .text section by 132KB
(I may have been able to optimise it a bit but it is still quite large).
With your approach to always use sp_el0, I get about 4KB larger .text
section.

So, without any better suggestion for current_thread_info(), I'm giving
up the idea of using SPSel == 0 in the kernel. I'll look at your patch
in more detail. BTW, I don't think we need the any count for the irq
stack as we don't re-enter the same IRQ stack.

-- 
Catalin
--
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 v2] arm64: Introduce IRQ stack

[Jungseok Lee]

On Sep 17, 2015, at 10:17 PM, Jungseok Lee wrote:
> On Sep 17, 2015, at 8:17 PM, Catalin Marinas wrote:
> 
> Hi Catalin,
> 
>> On Sun, Sep 13, 2015 at 02:42:17PM +, Jungseok Lee wrote:
>>> Currently, kernel context and interrupts are handled using a single
>>> kernel stack navigated by sp_el1. This forces many systems to use
>>> 16KB stack, not 8KB one. Low memory platforms naturally suffer from
>>> memory pressure accompanied by performance degradation.
>>> 
>>> This patch addresses the issue as introducing a separate percpu IRQ
>>> stack to handle both hard and soft interrupts with two ground rules:
>>> 
>>> - Utilize sp_el0 in EL1 context, which is not used currently
>>> - Do not complicate current_thread_info calculation
>>> 
>>> It is a core concept to trace struct thread_info using sp_el0 instead
>>> of sp_el1. This approach helps arm64 align with other architectures
>>> regarding object_is_on_stack() without additional complexity.
>> 
>> I'm still trying to understand how this patch works. I initially thought
>> that we would set SPSel = 0 while in kernel thread mode to make use of
>> SP_EL0 but I can't find any such code. Do you still use SP_EL1 all the
>> time and SP_EL0 just for temporary saving the thread stack?
> 
> Exactly.
> 
> My first approach was to set SPSel = 0 and implement EL1t Sync and IRQ.
> This idea originally comes from your comment [1]. A kernel thread could
> be handled easily and neatly, but it complicated current_thread_info
> calculation due to a user process.
> 
> Let's assume that a kernel thread uses SP_EL0 by default. When an interrupt
> comes in, a core jumps to EL1t IRQ. In case of a user process, a CPU goes
> into EL1h IRQ when an interrupt raises. To handle this scenario correctly,
> SPSel or spsr_el1 should be referenced. This reaches to quite big overhead
> in current_thread_info function.

This statement is described incorrectly. In case of user process, a CPU goes
into EL0 IRQ. Under this context, another interrupt could come in. At this
time, a core jumps to EL1h IRQ.

My original intention is to describe this situation.

Sorry for confusion.

Best Regards
Jungseok Lee
--
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 v2] arm64: Introduce IRQ stack

[Jungseok Lee]

On Sep 17, 2015, at 8:17 PM, Catalin Marinas wrote:

Hi Catalin,

> On Sun, Sep 13, 2015 at 02:42:17PM +, Jungseok Lee wrote:
>> Currently, kernel context and interrupts are handled using a single
>> kernel stack navigated by sp_el1. This forces many systems to use
>> 16KB stack, not 8KB one. Low memory platforms naturally suffer from
>> memory pressure accompanied by performance degradation.
>> 
>> This patch addresses the issue as introducing a separate percpu IRQ
>> stack to handle both hard and soft interrupts with two ground rules:
>> 
>>  - Utilize sp_el0 in EL1 context, which is not used currently
>>  - Do not complicate current_thread_info calculation
>> 
>> It is a core concept to trace struct thread_info using sp_el0 instead
>> of sp_el1. This approach helps arm64 align with other architectures
>> regarding object_is_on_stack() without additional complexity.
> 
> I'm still trying to understand how this patch works. I initially thought
> that we would set SPSel = 0 while in kernel thread mode to make use of
> SP_EL0 but I can't find any such code. Do you still use SP_EL1 all the
> time and SP_EL0 just for temporary saving the thread stack?

Exactly.

My first approach was to set SPSel = 0 and implement EL1t Sync and IRQ.
This idea originally comes from your comment [1]. A kernel thread could
be handled easily and neatly, but it complicated current_thread_info
calculation due to a user process.

Let's assume that a kernel thread uses SP_EL0 by default. When an interrupt
comes in, a core jumps to EL1t IRQ. In case of a user process, a CPU goes
into EL1h IRQ when an interrupt raises. To handle this scenario correctly,
SPSel or spsr_el1 should be referenced. This reaches to quite big overhead
in current_thread_info function.

I always keep my mind on simplicity of the function. Thus, I've decided to
give up the approach.

[1] https://lkml.org/lkml/2015/5/25/454

Best Regards
Jungseok Lee
--
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 v2] arm64: Introduce IRQ stack

[Jungseok Lee]

On Sep 17, 2015, at 7:33 PM, James Morse wrote:

Hi James and Will,

> Hi Will,
> 
> On 16/09/15 12:25, Will Deacon wrote:
>> On Sun, Sep 13, 2015 at 03:42:17PM +0100, Jungseok Lee wrote:
>>> diff --git a/arch/arm64/include/asm/thread_info.h 
>>> b/arch/arm64/include/asm/thread_info.h
>>> index dcd06d1..44839c0 100644
>>> --- a/arch/arm64/include/asm/thread_info.h
>>> +++ b/arch/arm64/include/asm/thread_info.h
>>> @@ -73,8 +73,11 @@ static inline struct thread_info 
>>> *current_thread_info(void) __attribute_const__;
>>> 
>>> static inline struct thread_info *current_thread_info(void)
>>> {
>>> -   return (struct thread_info *)
>>> -   (current_stack_pointer & ~(THREAD_SIZE - 1));
>>> +   unsigned long sp_el0;
>>> +
>>> +   asm volatile("mrs %0, sp_el0" : "=r" (sp_el0));
>>> +
>>> +   return (struct thread_info *)(sp_el0 & ~(THREAD_SIZE - 1));
>> 
>> This looks like it will generate worse code than our current implementation,
>> thanks to the asm volatile. Maybe just add something like a global
>> current_stack_pointer_el0?
> 
> Like current_stack_pointer does?:
>> register unsigned long current_stack_pointer_el0 asm ("sp_el0");
> 
> Unfortunately the compiler won't accept this, as it doesn't like the
> register name, it also won't accept instructions in this asm string.
> 
> Dropping the 'volatile' has the desired affect[0]. This would only cause a
> problem over a call to cpu_switch_to(), which writes to sp_el0, but also
> save/restores the callee-saved registers, so they will always be consistent.
> 
> 
> James
> 
> 
> 
> 
> [0] A fictitious example printk:
>> printk("%p%p%u%p", get_fs(), current_thread_info(),
>>   smp_processor_id(), current);
> 
> With this patch compiles to:
> 5f8:   d5384101mrs x1, sp_el0
> 5fc:   d5384100mrs x0, sp_el0
> 600:   d5384103mrs x3, sp_el0
> 604:   d5384104mrs x4, sp_el0
> 608:   9272c484and x4, x4, #0xc000
> 60c:   9272c463and x3, x3, #0xc000
> 610:   9272c421and x1, x1, #0xc000
> 614:   aa0403e2mov x2, x4
> 618:   9000adrpx0, 0 
> 61c:   f9400884ldr x4, [x4,#16]
> 620:   9100add x0, x0, #0x0
> 624:   b9401c63ldr w3, [x3,#28]
> 628:   f9400421ldr x1, [x1,#8]
> 62c:   9400bl  0 
> 
> Removing the volatile:
> 5e4:   d5384102mrs x2, sp_el0
> 5e8:   f9400844ldr x4, [x2,#16]
> 5ec:   9100add x0, x0, #0x0
> 5f0:   b9401c43ldr w3, [x2,#28]
> 5f4:   f9400441ldr x1, [x2,#8]
> 5f8:   9400bl  0 
> 
> 


As Will pointed out, if "worse" means "bigger text size", the change generates
worse codes than current implementation. A data based on System.map is as 
follows.

GCC version: aarch64-linux-gnu-gcc (Linaro GCC 2014.11) 4.9.3 20141031 
(prerelease)

[1] 4.3-rc1 
ffc8 T _text
ffc0007f1524 R _etext

[2] 4.3-rc1 + this patch
ffc8 T _text
ffc0007f8504 R _etext

[3] 4.3-rc1 + this patch + the following hunk
ffc8 T _text
ffc0007ef514 R _etext

diff --git a/arch/arm64/include/asm/thread_info.h 
b/arch/arm64/include/asm/thread_info.h
index 44839c0..4ab08a1 100644
--- a/arch/arm64/include/asm/thread_info.h
+++ b/arch/arm64/include/asm/thread_info.h
@@ -77,7 +77,7 @@ static inline struct thread_info *current_thread_info(void)
 
asm volatile("mrs %0, sp_el0" : "=r" (sp_el0));
 
-   return (struct thread_info *)(sp_el0 & ~(THREAD_SIZE - 1));
+   return (struct thread_info *)sp_el0;
 }
 
 #define thread_saved_pc(tsk)   \
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index c156540..314ac81 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -88,7 +88,8 @@
 
.if \el == 0
mrs x21, sp_el0
-   get_thread_info \el, tsk// Ensure MDSCR_EL1.SS is clear,
+   mov tsk, sp
+   and tsk, tsk, #~(THREAD_SIZE - 1)   // Ensure MDSCR_EL1.SS is clear,
ldr x19, [tsk, #TI_FLAGS]   // since we can unmask debug
disable_step_tsk x19, x20   // exceptions when scheduling.
.else
@@ -105,8 +106,7 @@
.if \el == 0
mvn x21, xzr
str x21, [sp, #S_SYSCALLNO]
-   mov x25, sp
-   msr sp_el0, x25
+   msr sp_el0, tsk
.endif
 
/*
@@ -165,13 +165,8 @@ alternative_endif
eret// return to kernel
.endm
 
-   .macro  get_thread_info, el, rd
-   .if \el == 0
-   mov \rd, sp
-   .else
+   .macro  get_thread_info, rd
mrs \rd, sp_el0
-   .endif
-   and \rd, \rd, #~(THREAD_SIZE - 1)   // bottom of thread stack
.endm
 
.macro  get_irq_stack
@@ -400,7 +395,7 @@ el1_irq:
irq_handler
 
 #ifdef CONFIG_PREEMPT
-   get_thread_info 

Re: [PATCH v2] arm64: Introduce IRQ stack

[Catalin Marinas]

On Sun, Sep 13, 2015 at 02:42:17PM +, Jungseok Lee wrote:
> Currently, kernel context and interrupts are handled using a single
> kernel stack navigated by sp_el1. This forces many systems to use
> 16KB stack, not 8KB one. Low memory platforms naturally suffer from
> memory pressure accompanied by performance degradation.
> 
> This patch addresses the issue as introducing a separate percpu IRQ
> stack to handle both hard and soft interrupts with two ground rules:
> 
>   - Utilize sp_el0 in EL1 context, which is not used currently
>   - Do not complicate current_thread_info calculation
> 
> It is a core concept to trace struct thread_info using sp_el0 instead
> of sp_el1. This approach helps arm64 align with other architectures
> regarding object_is_on_stack() without additional complexity.

I'm still trying to understand how this patch works. I initially thought
that we would set SPSel = 0 while in kernel thread mode to make use of
SP_EL0 but I can't find any such code. Do you still use SP_EL1 all the
time and SP_EL0 just for temporary saving the thread stack?

-- 
Catalin
--
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 v2] arm64: Introduce IRQ stack

[Catalin Marinas]

On Thu, Sep 17, 2015 at 11:33:44AM +0100, James Morse wrote:
> Hi Will,
> 
> On 16/09/15 12:25, Will Deacon wrote:
> > On Sun, Sep 13, 2015 at 03:42:17PM +0100, Jungseok Lee wrote:
> >> diff --git a/arch/arm64/include/asm/thread_info.h 
> >> b/arch/arm64/include/asm/thread_info.h
> >> index dcd06d1..44839c0 100644
> >> --- a/arch/arm64/include/asm/thread_info.h
> >> +++ b/arch/arm64/include/asm/thread_info.h
> >> @@ -73,8 +73,11 @@ static inline struct thread_info 
> >> *current_thread_info(void) __attribute_const__;
> >>  
> >>  static inline struct thread_info *current_thread_info(void)
> >>  {
> >> -  return (struct thread_info *)
> >> -  (current_stack_pointer & ~(THREAD_SIZE - 1));
> >> +  unsigned long sp_el0;
> >> +
> >> +  asm volatile("mrs %0, sp_el0" : "=r" (sp_el0));
> >> +
> >> +  return (struct thread_info *)(sp_el0 & ~(THREAD_SIZE - 1));
> > 
> > This looks like it will generate worse code than our current implementation,
> > thanks to the asm volatile. Maybe just add something like a global
> > current_stack_pointer_el0?
> 
> Like current_stack_pointer does?:
> > register unsigned long current_stack_pointer_el0 asm ("sp_el0");
> 
> Unfortunately the compiler won't accept this, as it doesn't like the
> register name, it also won't accept instructions in this asm string.

But once we do SPSel = 0, can we not just use the SP register here?

(I haven't read the rest of the patch yet)

-- 
Catalin
--
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 v2] arm64: Introduce IRQ stack

[James Morse]

Hi Will,

On 16/09/15 12:25, Will Deacon wrote:
> On Sun, Sep 13, 2015 at 03:42:17PM +0100, Jungseok Lee wrote:
>> diff --git a/arch/arm64/include/asm/thread_info.h 
>> b/arch/arm64/include/asm/thread_info.h
>> index dcd06d1..44839c0 100644
>> --- a/arch/arm64/include/asm/thread_info.h
>> +++ b/arch/arm64/include/asm/thread_info.h
>> @@ -73,8 +73,11 @@ static inline struct thread_info 
>> *current_thread_info(void) __attribute_const__;
>>  
>>  static inline struct thread_info *current_thread_info(void)
>>  {
>> -return (struct thread_info *)
>> -(current_stack_pointer & ~(THREAD_SIZE - 1));
>> +unsigned long sp_el0;
>> +
>> +asm volatile("mrs %0, sp_el0" : "=r" (sp_el0));
>> +
>> +return (struct thread_info *)(sp_el0 & ~(THREAD_SIZE - 1));
> 
> This looks like it will generate worse code than our current implementation,
> thanks to the asm volatile. Maybe just add something like a global
> current_stack_pointer_el0?

Like current_stack_pointer does?:
> register unsigned long current_stack_pointer_el0 asm ("sp_el0");

Unfortunately the compiler won't accept this, as it doesn't like the
register name, it also won't accept instructions in this asm string.

Dropping the 'volatile' has the desired affect[0]. This would only cause a
problem over a call to cpu_switch_to(), which writes to sp_el0, but also
save/restores the callee-saved registers, so they will always be consistent.


James




[0] A fictitious example printk:
> printk("%p%p%u%p", get_fs(), current_thread_info(),
>smp_processor_id(), current);

With this patch compiles to:
 5f8:   d5384101mrs x1, sp_el0
 5fc:   d5384100mrs x0, sp_el0
 600:   d5384103mrs x3, sp_el0
 604:   d5384104mrs x4, sp_el0
 608:   9272c484and x4, x4, #0xc000
 60c:   9272c463and x3, x3, #0xc000
 610:   9272c421and x1, x1, #0xc000
 614:   aa0403e2mov x2, x4
 618:   9000adrpx0, 0 
 61c:   f9400884ldr x4, [x4,#16]
 620:   9100add x0, x0, #0x0
 624:   b9401c63ldr w3, [x3,#28]
 628:   f9400421ldr x1, [x1,#8]
 62c:   9400bl  0 

Removing the volatile:
 5e4:   d5384102mrs x2, sp_el0
 5e8:   f9400844ldr x4, [x2,#16]
 5ec:   9100add x0, x0, #0x0
 5f0:   b9401c43ldr w3, [x2,#28]
 5f4:   f9400441ldr x1, [x2,#8]
 5f8:   9400bl  0 


--
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 v2] arm64: Introduce IRQ stack

[James Morse]

Hi Will,

On 16/09/15 12:25, Will Deacon wrote:
> On Sun, Sep 13, 2015 at 03:42:17PM +0100, Jungseok Lee wrote:
>> diff --git a/arch/arm64/include/asm/thread_info.h 
>> b/arch/arm64/include/asm/thread_info.h
>> index dcd06d1..44839c0 100644
>> --- a/arch/arm64/include/asm/thread_info.h
>> +++ b/arch/arm64/include/asm/thread_info.h
>> @@ -73,8 +73,11 @@ static inline struct thread_info 
>> *current_thread_info(void) __attribute_const__;
>>  
>>  static inline struct thread_info *current_thread_info(void)
>>  {
>> -return (struct thread_info *)
>> -(current_stack_pointer & ~(THREAD_SIZE - 1));
>> +unsigned long sp_el0;
>> +
>> +asm volatile("mrs %0, sp_el0" : "=r" (sp_el0));
>> +
>> +return (struct thread_info *)(sp_el0 & ~(THREAD_SIZE - 1));
> 
> This looks like it will generate worse code than our current implementation,
> thanks to the asm volatile. Maybe just add something like a global
> current_stack_pointer_el0?

Like current_stack_pointer does?:
> register unsigned long current_stack_pointer_el0 asm ("sp_el0");

Unfortunately the compiler won't accept this, as it doesn't like the
register name, it also won't accept instructions in this asm string.

Dropping the 'volatile' has the desired affect[0]. This would only cause a
problem over a call to cpu_switch_to(), which writes to sp_el0, but also
save/restores the callee-saved registers, so they will always be consistent.


James




[0] A fictitious example printk:
> printk("%p%p%u%p", get_fs(), current_thread_info(),
>smp_processor_id(), current);

With this patch compiles to:
 5f8:   d5384101mrs x1, sp_el0
 5fc:   d5384100mrs x0, sp_el0
 600:   d5384103mrs x3, sp_el0
 604:   d5384104mrs x4, sp_el0
 608:   9272c484and x4, x4, #0xc000
 60c:   9272c463and x3, x3, #0xc000
 610:   9272c421and x1, x1, #0xc000
 614:   aa0403e2mov x2, x4
 618:   9000adrpx0, 0 
 61c:   f9400884ldr x4, [x4,#16]
 620:   9100add x0, x0, #0x0
 624:   b9401c63ldr w3, [x3,#28]
 628:   f9400421ldr x1, [x1,#8]
 62c:   9400bl  0 

Removing the volatile:
 5e4:   d5384102mrs x2, sp_el0
 5e8:   f9400844ldr x4, [x2,#16]
 5ec:   9100add x0, x0, #0x0
 5f0:   b9401c43ldr w3, [x2,#28]
 5f4:   f9400441ldr x1, [x2,#8]
 5f8:   9400bl  0 


--
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 v2] arm64: Introduce IRQ stack

[Catalin Marinas]

On Thu, Sep 17, 2015 at 11:33:44AM +0100, James Morse wrote:
> Hi Will,
> 
> On 16/09/15 12:25, Will Deacon wrote:
> > On Sun, Sep 13, 2015 at 03:42:17PM +0100, Jungseok Lee wrote:
> >> diff --git a/arch/arm64/include/asm/thread_info.h 
> >> b/arch/arm64/include/asm/thread_info.h
> >> index dcd06d1..44839c0 100644
> >> --- a/arch/arm64/include/asm/thread_info.h
> >> +++ b/arch/arm64/include/asm/thread_info.h
> >> @@ -73,8 +73,11 @@ static inline struct thread_info 
> >> *current_thread_info(void) __attribute_const__;
> >>  
> >>  static inline struct thread_info *current_thread_info(void)
> >>  {
> >> -  return (struct thread_info *)
> >> -  (current_stack_pointer & ~(THREAD_SIZE - 1));
> >> +  unsigned long sp_el0;
> >> +
> >> +  asm volatile("mrs %0, sp_el0" : "=r" (sp_el0));
> >> +
> >> +  return (struct thread_info *)(sp_el0 & ~(THREAD_SIZE - 1));
> > 
> > This looks like it will generate worse code than our current implementation,
> > thanks to the asm volatile. Maybe just add something like a global
> > current_stack_pointer_el0?
> 
> Like current_stack_pointer does?:
> > register unsigned long current_stack_pointer_el0 asm ("sp_el0");
> 
> Unfortunately the compiler won't accept this, as it doesn't like the
> register name, it also won't accept instructions in this asm string.

But once we do SPSel = 0, can we not just use the SP register here?

(I haven't read the rest of the patch yet)

-- 
Catalin
--
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 v2] arm64: Introduce IRQ stack

[Catalin Marinas]

On Sun, Sep 13, 2015 at 02:42:17PM +, Jungseok Lee wrote:
> Currently, kernel context and interrupts are handled using a single
> kernel stack navigated by sp_el1. This forces many systems to use
> 16KB stack, not 8KB one. Low memory platforms naturally suffer from
> memory pressure accompanied by performance degradation.
> 
> This patch addresses the issue as introducing a separate percpu IRQ
> stack to handle both hard and soft interrupts with two ground rules:
> 
>   - Utilize sp_el0 in EL1 context, which is not used currently
>   - Do not complicate current_thread_info calculation
> 
> It is a core concept to trace struct thread_info using sp_el0 instead
> of sp_el1. This approach helps arm64 align with other architectures
> regarding object_is_on_stack() without additional complexity.

I'm still trying to understand how this patch works. I initially thought
that we would set SPSel = 0 while in kernel thread mode to make use of
SP_EL0 but I can't find any such code. Do you still use SP_EL1 all the
time and SP_EL0 just for temporary saving the thread stack?

-- 
Catalin
--
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 v2] arm64: Introduce IRQ stack

[Jungseok Lee]

On Sep 17, 2015, at 7:33 PM, James Morse wrote:

Hi James and Will,

> Hi Will,
> 
> On 16/09/15 12:25, Will Deacon wrote:
>> On Sun, Sep 13, 2015 at 03:42:17PM +0100, Jungseok Lee wrote:
>>> diff --git a/arch/arm64/include/asm/thread_info.h 
>>> b/arch/arm64/include/asm/thread_info.h
>>> index dcd06d1..44839c0 100644
>>> --- a/arch/arm64/include/asm/thread_info.h
>>> +++ b/arch/arm64/include/asm/thread_info.h
>>> @@ -73,8 +73,11 @@ static inline struct thread_info 
>>> *current_thread_info(void) __attribute_const__;
>>> 
>>> static inline struct thread_info *current_thread_info(void)
>>> {
>>> -   return (struct thread_info *)
>>> -   (current_stack_pointer & ~(THREAD_SIZE - 1));
>>> +   unsigned long sp_el0;
>>> +
>>> +   asm volatile("mrs %0, sp_el0" : "=r" (sp_el0));
>>> +
>>> +   return (struct thread_info *)(sp_el0 & ~(THREAD_SIZE - 1));
>> 
>> This looks like it will generate worse code than our current implementation,
>> thanks to the asm volatile. Maybe just add something like a global
>> current_stack_pointer_el0?
> 
> Like current_stack_pointer does?:
>> register unsigned long current_stack_pointer_el0 asm ("sp_el0");
> 
> Unfortunately the compiler won't accept this, as it doesn't like the
> register name, it also won't accept instructions in this asm string.
> 
> Dropping the 'volatile' has the desired affect[0]. This would only cause a
> problem over a call to cpu_switch_to(), which writes to sp_el0, but also
> save/restores the callee-saved registers, so they will always be consistent.
> 
> 
> James
> 
> 
> 
> 
> [0] A fictitious example printk:
>> printk("%p%p%u%p", get_fs(), current_thread_info(),
>>   smp_processor_id(), current);
> 
> With this patch compiles to:
> 5f8:   d5384101mrs x1, sp_el0
> 5fc:   d5384100mrs x0, sp_el0
> 600:   d5384103mrs x3, sp_el0
> 604:   d5384104mrs x4, sp_el0
> 608:   9272c484and x4, x4, #0xc000
> 60c:   9272c463and x3, x3, #0xc000
> 610:   9272c421and x1, x1, #0xc000
> 614:   aa0403e2mov x2, x4
> 618:   9000adrpx0, 0 
> 61c:   f9400884ldr x4, [x4,#16]
> 620:   9100add x0, x0, #0x0
> 624:   b9401c63ldr w3, [x3,#28]
> 628:   f9400421ldr x1, [x1,#8]
> 62c:   9400bl  0 
> 
> Removing the volatile:
> 5e4:   d5384102mrs x2, sp_el0
> 5e8:   f9400844ldr x4, [x2,#16]
> 5ec:   9100add x0, x0, #0x0
> 5f0:   b9401c43ldr w3, [x2,#28]
> 5f4:   f9400441ldr x1, [x2,#8]
> 5f8:   9400bl  0 
> 
> 


As Will pointed out, if "worse" means "bigger text size", the change generates
worse codes than current implementation. A data based on System.map is as 
follows.

GCC version: aarch64-linux-gnu-gcc (Linaro GCC 2014.11) 4.9.3 20141031 
(prerelease)

[1] 4.3-rc1 
ffc8 T _text
ffc0007f1524 R _etext

[2] 4.3-rc1 + this patch
ffc8 T _text
ffc0007f8504 R _etext

[3] 4.3-rc1 + this patch + the following hunk
ffc8 T _text
ffc0007ef514 R _etext

diff --git a/arch/arm64/include/asm/thread_info.h 
b/arch/arm64/include/asm/thread_info.h
index 44839c0..4ab08a1 100644
--- a/arch/arm64/include/asm/thread_info.h
+++ b/arch/arm64/include/asm/thread_info.h
@@ -77,7 +77,7 @@ static inline struct thread_info *current_thread_info(void)
 
asm volatile("mrs %0, sp_el0" : "=r" (sp_el0));
 
-   return (struct thread_info *)(sp_el0 & ~(THREAD_SIZE - 1));
+   return (struct thread_info *)sp_el0;
 }
 
 #define thread_saved_pc(tsk)   \
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index c156540..314ac81 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -88,7 +88,8 @@
 
.if \el == 0
mrs x21, sp_el0
-   get_thread_info \el, tsk// Ensure MDSCR_EL1.SS is clear,
+   mov tsk, sp
+   and tsk, tsk, #~(THREAD_SIZE - 1)   // Ensure MDSCR_EL1.SS is clear,
ldr x19, [tsk, #TI_FLAGS]   // since we can unmask debug
disable_step_tsk x19, x20   // exceptions when scheduling.
.else
@@ -105,8 +106,7 @@
.if \el == 0
mvn x21, xzr
str x21, [sp, #S_SYSCALLNO]
-   mov x25, sp
-   msr sp_el0, x25
+   msr sp_el0, tsk
.endif
 
/*
@@ -165,13 +165,8 @@ alternative_endif
eret// return to kernel
.endm
 
-   .macro  get_thread_info, el, rd
-   .if \el == 0
-   mov \rd, sp
-   .else
+   .macro  get_thread_info, rd
mrs \rd, sp_el0
-   .endif
-   and \rd, \rd, #~(THREAD_SIZE - 1)   // bottom of thread stack
.endm
 
.macro  get_irq_stack
@@ -400,7 +395,7 @@ el1_irq:
irq_handler
 
 #ifdef CONFIG_PREEMPT
-   get_thread_info 

Re: [PATCH v2] arm64: Introduce IRQ stack

[Catalin Marinas]

On Thu, Sep 17, 2015 at 10:22:26PM +0900, Jungseok Lee wrote:
> On Sep 17, 2015, at 10:17 PM, Jungseok Lee wrote:
> > On Sep 17, 2015, at 8:17 PM, Catalin Marinas wrote:
> >> On Sun, Sep 13, 2015 at 02:42:17PM +, Jungseok Lee wrote:
> >>> Currently, kernel context and interrupts are handled using a single
> >>> kernel stack navigated by sp_el1. This forces many systems to use
> >>> 16KB stack, not 8KB one. Low memory platforms naturally suffer from
> >>> memory pressure accompanied by performance degradation.
> >>> 
> >>> This patch addresses the issue as introducing a separate percpu IRQ
> >>> stack to handle both hard and soft interrupts with two ground rules:
> >>> 
> >>> - Utilize sp_el0 in EL1 context, which is not used currently
> >>> - Do not complicate current_thread_info calculation
> >>> 
> >>> It is a core concept to trace struct thread_info using sp_el0 instead
> >>> of sp_el1. This approach helps arm64 align with other architectures
> >>> regarding object_is_on_stack() without additional complexity.
> >> 
> >> I'm still trying to understand how this patch works. I initially thought
> >> that we would set SPSel = 0 while in kernel thread mode to make use of
> >> SP_EL0 but I can't find any such code. Do you still use SP_EL1 all the
> >> time and SP_EL0 just for temporary saving the thread stack?
> > 
> > Exactly.
> > 
> > My first approach was to set SPSel = 0 and implement EL1t Sync and IRQ.
> > This idea originally comes from your comment [1]. A kernel thread could
> > be handled easily and neatly, but it complicated current_thread_info
> > calculation due to a user process.
> > 
> > Let's assume that a kernel thread uses SP_EL0 by default. When an interrupt
> > comes in, a core jumps to EL1t IRQ. In case of a user process, a CPU goes
> > into EL1h IRQ when an interrupt raises. To handle this scenario correctly,
> > SPSel or spsr_el1 should be referenced. This reaches to quite big overhead
> > in current_thread_info function.
> 
> This statement is described incorrectly. In case of user process, a CPU goes
> into EL0 IRQ. Under this context, another interrupt could come in. At this
> time, a core jumps to EL1h IRQ.

I don't I entirely follow you here.

First of all, we don't allow re-entrant IRQs, they are disabled during
handling (there are patches for NMI via IRQ priorities but these would
be a special case on a different code path; for the current code, let's
just assume that IRQs are not re-entrant).

Second, SPSel is automatically set to 1 when taking an exception. So we
are guaranteed that the kernel entry code always switches to SP_EL1
(EL1h mode).

My initial thought was to populate SP_EL1 per CPU as a handler stack and
never change it afterwards. The entry code may continue to use SP_EL1 if
in interrupt or switch to SP_EL0 and SPSel = 0 if in thread context.
What I didn't realise is that SP_EL0 cannot be accessed directly when
SPSel == 0, only as SP. This indeed complicates current_thread_info
slightly.

I did some tests with using SPSel in current_thread_info() to read SP or
SP_EL0 and it doesn't look good, it increased the .text section by 132KB
(I may have been able to optimise it a bit but it is still quite large).
With your approach to always use sp_el0, I get about 4KB larger .text
section.

So, without any better suggestion for current_thread_info(), I'm giving
up the idea of using SPSel == 0 in the kernel. I'll look at your patch
in more detail. BTW, I don't think we need the any count for the irq
stack as we don't re-enter the same IRQ stack.

-- 
Catalin
--
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 v2] arm64: Introduce IRQ stack

[Jungseok Lee]

On Sep 17, 2015, at 10:17 PM, Jungseok Lee wrote:
> On Sep 17, 2015, at 8:17 PM, Catalin Marinas wrote:
> 
> Hi Catalin,
> 
>> On Sun, Sep 13, 2015 at 02:42:17PM +, Jungseok Lee wrote:
>>> Currently, kernel context and interrupts are handled using a single
>>> kernel stack navigated by sp_el1. This forces many systems to use
>>> 16KB stack, not 8KB one. Low memory platforms naturally suffer from
>>> memory pressure accompanied by performance degradation.
>>> 
>>> This patch addresses the issue as introducing a separate percpu IRQ
>>> stack to handle both hard and soft interrupts with two ground rules:
>>> 
>>> - Utilize sp_el0 in EL1 context, which is not used currently
>>> - Do not complicate current_thread_info calculation
>>> 
>>> It is a core concept to trace struct thread_info using sp_el0 instead
>>> of sp_el1. This approach helps arm64 align with other architectures
>>> regarding object_is_on_stack() without additional complexity.
>> 
>> I'm still trying to understand how this patch works. I initially thought
>> that we would set SPSel = 0 while in kernel thread mode to make use of
>> SP_EL0 but I can't find any such code. Do you still use SP_EL1 all the
>> time and SP_EL0 just for temporary saving the thread stack?
> 
> Exactly.
> 
> My first approach was to set SPSel = 0 and implement EL1t Sync and IRQ.
> This idea originally comes from your comment [1]. A kernel thread could
> be handled easily and neatly, but it complicated current_thread_info
> calculation due to a user process.
> 
> Let's assume that a kernel thread uses SP_EL0 by default. When an interrupt
> comes in, a core jumps to EL1t IRQ. In case of a user process, a CPU goes
> into EL1h IRQ when an interrupt raises. To handle this scenario correctly,
> SPSel or spsr_el1 should be referenced. This reaches to quite big overhead
> in current_thread_info function.

This statement is described incorrectly. In case of user process, a CPU goes
into EL0 IRQ. Under this context, another interrupt could come in. At this
time, a core jumps to EL1h IRQ.

My original intention is to describe this situation.

Sorry for confusion.

Best Regards
Jungseok Lee
--
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 v2] arm64: Introduce IRQ stack

[Jungseok Lee]

On Sep 17, 2015, at 8:17 PM, Catalin Marinas wrote:

Hi Catalin,

> On Sun, Sep 13, 2015 at 02:42:17PM +, Jungseok Lee wrote:
>> Currently, kernel context and interrupts are handled using a single
>> kernel stack navigated by sp_el1. This forces many systems to use
>> 16KB stack, not 8KB one. Low memory platforms naturally suffer from
>> memory pressure accompanied by performance degradation.
>> 
>> This patch addresses the issue as introducing a separate percpu IRQ
>> stack to handle both hard and soft interrupts with two ground rules:
>> 
>>  - Utilize sp_el0 in EL1 context, which is not used currently
>>  - Do not complicate current_thread_info calculation
>> 
>> It is a core concept to trace struct thread_info using sp_el0 instead
>> of sp_el1. This approach helps arm64 align with other architectures
>> regarding object_is_on_stack() without additional complexity.
> 
> I'm still trying to understand how this patch works. I initially thought
> that we would set SPSel = 0 while in kernel thread mode to make use of
> SP_EL0 but I can't find any such code. Do you still use SP_EL1 all the
> time and SP_EL0 just for temporary saving the thread stack?

Exactly.

My first approach was to set SPSel = 0 and implement EL1t Sync and IRQ.
This idea originally comes from your comment [1]. A kernel thread could
be handled easily and neatly, but it complicated current_thread_info
calculation due to a user process.

Let's assume that a kernel thread uses SP_EL0 by default. When an interrupt
comes in, a core jumps to EL1t IRQ. In case of a user process, a CPU goes
into EL1h IRQ when an interrupt raises. To handle this scenario correctly,
SPSel or spsr_el1 should be referenced. This reaches to quite big overhead
in current_thread_info function.

I always keep my mind on simplicity of the function. Thus, I've decided to
give up the approach.

[1] https://lkml.org/lkml/2015/5/25/454

Best Regards
Jungseok Lee
--
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 v2] arm64: Introduce IRQ stack

[Catalin Marinas]

On Thu, Sep 17, 2015 at 09:36:04PM +0900, Jungseok Lee wrote:
> On Sep 17, 2015, at 7:33 PM, James Morse wrote:
> > On 16/09/15 12:25, Will Deacon wrote:
> >> On Sun, Sep 13, 2015 at 03:42:17PM +0100, Jungseok Lee wrote:
> >>> diff --git a/arch/arm64/include/asm/thread_info.h 
> >>> b/arch/arm64/include/asm/thread_info.h
> >>> index dcd06d1..44839c0 100644
> >>> --- a/arch/arm64/include/asm/thread_info.h
> >>> +++ b/arch/arm64/include/asm/thread_info.h
> >>> @@ -73,8 +73,11 @@ static inline struct thread_info 
> >>> *current_thread_info(void) __attribute_const__;
> >>> 
> >>> static inline struct thread_info *current_thread_info(void)
> >>> {
> >>> - return (struct thread_info *)
> >>> - (current_stack_pointer & ~(THREAD_SIZE - 1));
> >>> + unsigned long sp_el0;
> >>> +
> >>> + asm volatile("mrs %0, sp_el0" : "=r" (sp_el0));
> >>> +
> >>> + return (struct thread_info *)(sp_el0 & ~(THREAD_SIZE - 1));
> >> 
> >> This looks like it will generate worse code than our current 
> >> implementation,
> >> thanks to the asm volatile. Maybe just add something like a global
> >> current_stack_pointer_el0?
[...]
> --- a/arch/arm64/include/asm/thread_info.h
> +++ b/arch/arm64/include/asm/thread_info.h
> @@ -77,7 +77,7 @@ static inline struct thread_info *current_thread_info(void)
>  
>   asm volatile("mrs %0, sp_el0" : "=r" (sp_el0));
>  
> - return (struct thread_info *)(sp_el0 & ~(THREAD_SIZE - 1));
> + return (struct thread_info *)sp_el0;
>  }

This makes sense, since we just use sp_el0 as a scratch register, store
the current thread_info address directly. But, as James mentioned, I
don't think you need asm volatile, just asm (it has a small impact in my
tests).

-- 
Catalin
--
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 v2] arm64: Introduce IRQ stack

[Will Deacon]

On Sun, Sep 13, 2015 at 03:42:17PM +0100, Jungseok Lee wrote:
> Currently, kernel context and interrupts are handled using a single
> kernel stack navigated by sp_el1. This forces many systems to use
> 16KB stack, not 8KB one. Low memory platforms naturally suffer from
> memory pressure accompanied by performance degradation.
> 
> This patch addresses the issue as introducing a separate percpu IRQ
> stack to handle both hard and soft interrupts with two ground rules:
> 
>   - Utilize sp_el0 in EL1 context, which is not used currently
>   - Do not complicate current_thread_info calculation
> 
> It is a core concept to trace struct thread_info using sp_el0 instead
> of sp_el1. This approach helps arm64 align with other architectures
> regarding object_is_on_stack() without additional complexity.
> 
> Cc: James Morse 
> Signed-off-by: Jungseok Lee 
> ---
> Changes since v1:
> - Rebased on top of v4.3-rc1
> - Removed Kconfig about IRQ stack, per James
> - Used PERCPU for IRQ stack, per James
> - Tried to allocate IRQ stack when CPU is about to start up, per James
> - Moved sp_el0 update into kernel_entry macro, per James
> - Dropped S_SP removal patch, per Mark and James
> 
>  arch/arm64/include/asm/irq.h |  8 +++
>  arch/arm64/include/asm/thread_info.h |  7 +-
>  arch/arm64/kernel/asm-offsets.c  |  5 ++
>  arch/arm64/kernel/entry.S| 54 ++--
>  arch/arm64/kernel/head.S |  3 +
>  arch/arm64/kernel/irq.c  | 21 ++
>  arch/arm64/kernel/smp.c  |  6 ++
>  7 files changed, 95 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h
> index bbb251b..67975a2 100644
> --- a/arch/arm64/include/asm/irq.h
> +++ b/arch/arm64/include/asm/irq.h
> @@ -5,11 +5,19 @@
>  
>  #include 
>  
> +struct irq_stack {
> + void *stack;
> + unsigned long thread_sp;
> + unsigned int count;
> +};
> +
>  struct pt_regs;
>  
>  extern void migrate_irqs(void);
>  extern void set_handle_irq(void (*handle_irq)(struct pt_regs *));
>  
> +extern int alloc_irq_stack(unsigned int cpu);
> +
>  static inline void acpi_irq_init(void)
>  {
>   /*
> diff --git a/arch/arm64/include/asm/thread_info.h 
> b/arch/arm64/include/asm/thread_info.h
> index dcd06d1..44839c0 100644
> --- a/arch/arm64/include/asm/thread_info.h
> +++ b/arch/arm64/include/asm/thread_info.h
> @@ -73,8 +73,11 @@ static inline struct thread_info 
> *current_thread_info(void) __attribute_const__;
>  
>  static inline struct thread_info *current_thread_info(void)
>  {
> - return (struct thread_info *)
> - (current_stack_pointer & ~(THREAD_SIZE - 1));
> + unsigned long sp_el0;
> +
> + asm volatile("mrs %0, sp_el0" : "=r" (sp_el0));
> +
> + return (struct thread_info *)(sp_el0 & ~(THREAD_SIZE - 1));

This looks like it will generate worse code than our current implementation,
thanks to the asm volatile. Maybe just add something like a global
current_stack_pointer_el0?

Will
--
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 v2] arm64: Introduce IRQ stack

[Will Deacon]

On Sun, Sep 13, 2015 at 03:42:17PM +0100, Jungseok Lee wrote:
> Currently, kernel context and interrupts are handled using a single
> kernel stack navigated by sp_el1. This forces many systems to use
> 16KB stack, not 8KB one. Low memory platforms naturally suffer from
> memory pressure accompanied by performance degradation.
> 
> This patch addresses the issue as introducing a separate percpu IRQ
> stack to handle both hard and soft interrupts with two ground rules:
> 
>   - Utilize sp_el0 in EL1 context, which is not used currently
>   - Do not complicate current_thread_info calculation
> 
> It is a core concept to trace struct thread_info using sp_el0 instead
> of sp_el1. This approach helps arm64 align with other architectures
> regarding object_is_on_stack() without additional complexity.
> 
> Cc: James Morse 
> Signed-off-by: Jungseok Lee 
> ---
> Changes since v1:
> - Rebased on top of v4.3-rc1
> - Removed Kconfig about IRQ stack, per James
> - Used PERCPU for IRQ stack, per James
> - Tried to allocate IRQ stack when CPU is about to start up, per James
> - Moved sp_el0 update into kernel_entry macro, per James
> - Dropped S_SP removal patch, per Mark and James
> 
>  arch/arm64/include/asm/irq.h |  8 +++
>  arch/arm64/include/asm/thread_info.h |  7 +-
>  arch/arm64/kernel/asm-offsets.c  |  5 ++
>  arch/arm64/kernel/entry.S| 54 ++--
>  arch/arm64/kernel/head.S |  3 +
>  arch/arm64/kernel/irq.c  | 21 ++
>  arch/arm64/kernel/smp.c  |  6 ++
>  7 files changed, 95 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h
> index bbb251b..67975a2 100644
> --- a/arch/arm64/include/asm/irq.h
> +++ b/arch/arm64/include/asm/irq.h
> @@ -5,11 +5,19 @@
>  
>  #include 
>  
> +struct irq_stack {
> + void *stack;
> + unsigned long thread_sp;
> + unsigned int count;
> +};
> +
>  struct pt_regs;
>  
>  extern void migrate_irqs(void);
>  extern void set_handle_irq(void (*handle_irq)(struct pt_regs *));
>  
> +extern int alloc_irq_stack(unsigned int cpu);
> +
>  static inline void acpi_irq_init(void)
>  {
>   /*
> diff --git a/arch/arm64/include/asm/thread_info.h 
> b/arch/arm64/include/asm/thread_info.h
> index dcd06d1..44839c0 100644
> --- a/arch/arm64/include/asm/thread_info.h
> +++ b/arch/arm64/include/asm/thread_info.h
> @@ -73,8 +73,11 @@ static inline struct thread_info 
> *current_thread_info(void) __attribute_const__;
>  
>  static inline struct thread_info *current_thread_info(void)
>  {
> - return (struct thread_info *)
> - (current_stack_pointer & ~(THREAD_SIZE - 1));
> + unsigned long sp_el0;
> +
> + asm volatile("mrs %0, sp_el0" : "=r" (sp_el0));
> +
> + return (struct thread_info *)(sp_el0 & ~(THREAD_SIZE - 1));

This looks like it will generate worse code than our current implementation,
thanks to the asm volatile. Maybe just add something like a global
current_stack_pointer_el0?

Will
--
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/