Why do_swap() sets cpu.current before context switch?


Yasushi SHOJI
 

Hi Katsuhiro and Andy,

On Tue, Sep 14, 2021 at 7:52 PM Katsuhiro Suzuki
<katsuhiro@katsuster.net> wrote:
On 2021/09/09 10:41, Yasushi SHOJI wrote:
On Thu, Sep 9, 2021 at 12:01 AM Katsuhiro Suzuki <katsuhiro@katsuster.net> wrote:
On 2021/09/08 16:22, Yasushi SHOJI wrote:
On Wed, Sep 8, 2021 at 10:23 AM Katsuhiro Suzuki <katsuhiro@katsuster.net> wrote:
Why do you `use _current_cpu` at all? `_arch_switch()` or
`arch_switch()` on the main branch takes
both new and old thread handles.
Because to keep consistency for another context switching (by preemption) and
other interrupts.
Only _current_cpu.current is available when an interrupt occurred.
The reason why we set _current to the new thread is that we can't set it after
we switch to the new thread. The newly switched thread will just start
running from
the point it left off. Otherwise, we have to make sure that each and
every arch must
set _current to the new thread in `arch_switch()`.
Hmm... It seems that in CONFIG_USE_SWITCH=n case (ex. aarch32(*)) _current_cpu.current
is updated by software interrupt handler.
(*) arch/arm/core/aarch32/swap_helper.S

So I wonder that why CONFIG_USE_SWITCH=y changed strategy to update current thread.
Ah, OK. I didn't answer your question at all. And I don't have the answer.

Andy, would you mind telling us if you have any comments about the
rationale behind this?

--
yashi


Katsuhiro Suzuki
 

Hello,

On 2021/09/09 10:41, Yasushi SHOJI wrote:
Hi,
On Thu, Sep 9, 2021 at 12:01 AM Katsuhiro Suzuki
<katsuhiro@katsuster.net> wrote:
On 2021/09/08 16:22, Yasushi SHOJI wrote:
On Wed, Sep 8, 2021 at 10:23 AM Katsuhiro Suzuki <katsuhiro@katsuster.net> wrote:
Why do you `use _current_cpu` at all? `_arch_switch()` or
`arch_switch()` on the main branch takes
both new and old thread handles.
Because to keep consistency for another context switching (by preemption) and
other interrupts.
Only _current_cpu.current is available when an interrupt occurred.
The reason why we set _current to the new thread is that we can't set it after
we switch to the new thread. The newly switched thread will just start
running from
the point it left off. Otherwise, we have to make sure that each and
every arch must
set _current to the new thread in `arch_switch()`.
Hmm... It seems that in CONFIG_USE_SWITCH=n case (ex. aarch32(*)) _current_cpu.current
is updated by software interrupt handler.
(*) arch/arm/core/aarch32/swap_helper.S

So I wonder that why CONFIG_USE_SWITCH=y changed strategy to update current thread.


At an IRQ, you know the `_current` is the currently executing thread and you can
get a new thread from ready_q, if needed.
At explicit switch, you are given both old and new threads.
So in both cases, we _can_ implement the switch.
Right and agree with you. If this is intentional behavior and we encourage
CONFIG_USE_SWITCH=y case to separate from old code, I want to follow it.
I don't have any claim to current context switching specification, just want to
know "why changed?".


I agree that we need to do similar things for arch_switch and irq, and
love to use
exact same code for both, but it might be better to have separate code for
each situation. Or, at least use .macro to construct parts of it.
ps. Unfortunately, unlike the Linux kernel, we don't have any way to
get the thread struct
from a stack pointer, IIRC.
Yeah, this is Zephyr. Not Linux :)


just my 2 cents,
Best Regards,
Katsuhiro Suzuki


Katsuhiro Suzuki
 

Hello,

On 2021/09/09 1:14, Andy Ross wrote:
On 9/8/2021 8:08 AM, Katsuhiro Suzuki wrote:
RISC-V's FPU arch has the flag to permit/forbid using FPU. In Zephyr, this flag is set to
forbid side if thread was declared as not using FPU. And CPU raise interrupt when using
any FPU instruction at FPU forbidden state.
Can't you just check that flag in the context switch code?  It seems like that would be faster on average (most DSP workloads try very hard to avoid doing synchronous context switches to avoid the need to spill all that state), and have much better latency guarantees (taking an exception is REALLY expensive!).
I can check it directly. I think this is future work to achieve more faster switching
than now. At this point, I want to implement most conservative one..


Andy
Best Regards,
Katsuhiro Suzuki


Yasushi SHOJI
 

Hi,

On Thu, Sep 9, 2021 at 12:01 AM Katsuhiro Suzuki
<katsuhiro@katsuster.net> wrote:
On 2021/09/08 16:22, Yasushi SHOJI wrote:
On Wed, Sep 8, 2021 at 10:23 AM Katsuhiro Suzuki <katsuhiro@katsuster.net> wrote:
Why do you `use _current_cpu` at all? `_arch_switch()` or
`arch_switch()` on the main branch takes
both new and old thread handles.
Because to keep consistency for another context switching (by preemption) and
other interrupts.
Only _current_cpu.current is available when an interrupt occurred.
The reason why we set _current to the new thread is that we can't set it after
we switch to the new thread. The newly switched thread will just start
running from
the point it left off. Otherwise, we have to make sure that each and
every arch must
set _current to the new thread in `arch_switch()`.

At an IRQ, you know the `_current` is the currently executing thread and you can
get a new thread from ready_q, if needed.
At explicit switch, you are given both old and new threads.
So in both cases, we _can_ implement the switch.

I agree that we need to do similar things for arch_switch and irq, and
love to use
exact same code for both, but it might be better to have separate code for
each situation. Or, at least use .macro to construct parts of it.

ps. Unfortunately, unlike the Linux kernel, we don't have any way to
get the thread struct
from a stack pointer, IIRC.

just my 2 cents,
--
yashi


Andy Ross
 

On 9/8/2021 8:08 AM, Katsuhiro Suzuki wrote:
RISC-V's FPU arch has the flag to permit/forbid using FPU. In Zephyr, this flag is set to
forbid side if thread was declared as not using FPU. And CPU raise interrupt when using
any FPU instruction at FPU forbidden state.

Can't you just check that flag in the context switch code?  It seems like that would be faster on average (most DSP workloads try very hard to avoid doing synchronous context switches to avoid the need to spill all that state), and have much better latency guarantees (taking an exception is REALLY expensive!).

Andy


Katsuhiro Suzuki
 

Hello,

On 2021/09/08 11:35, andy-intel@plausible.org wrote:
Katsuhiro Suzuki wrote:
Newer switching sets NEW thread handle into _current_cpu.current
BEFORE calling arch_switch().  This implementation will face a
problem in RISC-V environment if thread calls do_swap() explicitly
and switch to thread B (use FPU) from thread A (not use FPU)
because...
So... this is the very middle of a context switch.  The expectation of the kernel is that no exception/interrupt handlers are going to fire, because (by definition) if they do they will see corrupt/inconsistent thread state.  Or rather, if an architecture wants to allow that, it needs to be prepared to do the work.
I mean, the information is available to you: you know what the new thread is, because you're handed its switch handle which you created yourself.  You likewise know the old thread, because you're passed the address of its switch_handle field from which you can extract a pointer to the enclosing thread struct.  You know you're in the middle of a context switch, because arch_switch() was called.  You COULD write an FPU exception handler to detect this state and do the right thing.  I don't necessarily think this would be a good design, but it's possible.
Thanks for your advice.

Current RISC-V implementation (CONFIG_USE_SWITCH=n) is using 'ecall' (this is
SW interrupt instruction) to execute context switching explicitly.

And jump into interrupt handler that is used from not only SW interrupt but also
other interrupts. In interrupt handler, kernel saves FPU registers of current thread
that is pointing old thread.

I agree with you that I can add special path for explicit context switching.
If such path is needed, I will add that.


Can you explain why you need to take an FPU exception in the middle of a context switch?  That seems a little questionable to me.  Why are you hitting lazy-saved FPU registers in a situation where it would be faster to just check the mask state to see if they are populated or not?  (Forgive me: I don't know the RISC-V FPU architecture, but I assume that's the situation you're in: the FPU registers may or may not be in use and using them if they aren't will trap.)
In my understanding, RISC-V has not supported lazy-saved FPU yet. Always need to save
FPU registers at the beginning of interrupt handler.

RISC-V's FPU arch has the flag to permit/forbid using FPU. In Zephyr, this flag is set to
forbid side if thread was declared as not using FPU. And CPU raise interrupt when using
any FPU instruction at FPU forbidden state.


Andy
Best Regards,
Katsuhiro Suzuki



Katsuhiro Suzuki
 

Hello,

Thank you for reply.


On 2021/09/08 16:22, Yasushi SHOJI wrote:
Hi,
On Wed, Sep 8, 2021 at 10:23 AM Katsuhiro Suzuki
<katsuhiro@katsuster.net> wrote:
I have question about newer version of context switching (CONFIG_USE_SWITCH=y).
Does RISC-V support USE_SWITCH?
Currently, No. I'm trying to add a support for CONFIG_USE_SWITCH=y case.


Newer switching sets NEW thread handle into _current_cpu.current BEFORE calling arch_switch().
This implementation will face a problem in RISC-V environment if thread calls do_swap() explicitly and switch to thread B (use FPU) from thread A (not use FPU) because...
AFAICS, even with v1.11, we are setting `_current` to `new_thread` in
`_Swap()` before calling `_arch_switch()`.
What version are you referring to as "older"?
Sorry for confusing. Older means CONFIG_USE_SWITCH=n.
Does not means Zephyr's version.


Why do you `use _current_cpu` at all? `_arch_switch()` or
`arch_switch()` on the main branch takes
both new and old thread handles.
Because to keep consistency for another context switching (by preemption) and
other interrupts.
Only _current_cpu.current is available when an interrupt occurred.


Best,
Best Regards,
Katsuhiro Suzuki


Yasushi SHOJI
 

Hi,

On Wed, Sep 8, 2021 at 10:23 AM Katsuhiro Suzuki
<katsuhiro@katsuster.net> wrote:
I have question about newer version of context switching (CONFIG_USE_SWITCH=y).
Does RISC-V support USE_SWITCH?

Newer switching sets NEW thread handle into _current_cpu.current BEFORE calling arch_switch().
This implementation will face a problem in RISC-V environment if thread calls do_swap() explicitly and switch to thread B (use FPU) from thread A (not use FPU) because...
AFAICS, even with v1.11, we are setting `_current` to `new_thread` in
`_Swap()` before calling `_arch_switch()`.
What version are you referring to as "older"?

Why do you `use _current_cpu` at all? `_arch_switch()` or
`arch_switch()` on the main branch takes
both new and old thread handles.

Best,
--
yashi


Andy Ross
 

Katsuhiro Suzuki wrote:
> Newer switching sets NEW thread handle into _current_cpu.current
> BEFORE calling arch_switch().  This implementation will face a
> problem in RISC-V environment if thread calls do_swap() explicitly
> and switch to thread B (use FPU) from thread A (not use FPU)
> because...

So... this is the very middle of a context switch.  The expectation of the kernel is that no exception/interrupt handlers are going to fire, because (by definition) if they do they will see corrupt/inconsistent thread state.  Or rather, if an architecture wants to allow that, it needs to be prepared to do the work.

I mean, the information is available to you: you know what the new thread is, because you're handed its switch handle which you created yourself.  You likewise know the old thread, because you're passed the address of its switch_handle field from which you can extract a pointer to the enclosing thread struct.  You know you're in the middle of a context switch, because arch_switch() was called.  You COULD write an FPU exception handler to detect this state and do the right thing.  I don't necessarily think this would be a good design, but it's possible.

Can you explain why you need to take an FPU exception in the middle of a context switch?  That seems a little questionable to me.  Why are you hitting lazy-saved FPU registers in a situation where it would be faster to just check the mask state to see if they are populated or not?  (Forgive me: I don't know the RISC-V FPU architecture, but I assume that's the situation you're in: the FPU registers may or may not be in use and using them if they aren't will trap.)

Andy


Katsuhiro Suzuki
 

Hello kernel guys,

I have question about newer version of context switching (CONFIG_USE_SWITCH=y).

Newer switching sets NEW thread handle into _current_cpu.current BEFORE calling arch_switch().
This implementation will face a problem in RISC-V environment if thread calls do_swap() explicitly and switch to thread B (use FPU) from thread A (not use FPU) because...

- If _current_cpu.current has FPU flag, interrupt handler will save all FPU regs to stack
- At older switching
- _current_cpu.current is pointing thread A (not use FPU)
- The handler will skip saving
- But at newer switching
- _current_cpu.current is thread B (use FPU)
- The handler try to save FPU regs using FPU instructions
- But we haven't switching thread yet and thread A is prohibited to use FPU
- The handler will face illegal instruction exception (and going to hang...)

I don't know why newer switching sets thread B into _current_cpu.current such timing.
Does anyone know the reason about this implementation?

Best Regards,
Katsuhiro Suzuki