[v2, 5/5] riscv: vector: allow kernel-mode Vector with preemption
Conor Dooley
conor at kernel.org
Mon Jul 24 09:26:07 PDT 2023
On Mon, Jul 24, 2023 at 11:45:47PM +0800, Andy Chiu wrote:
> On Mon, Jul 24, 2023 at 8:19 PM Conor Dooley <conor.dooley at microchip.com> wrote:
> > On Fri, Jul 21, 2023 at 11:28:55AM +0000, Andy Chiu wrote:
> > > Add kernel_vstate to keep track of kernel-mode Vector registers when
> > > trap introduced context switch happens. Also, provide trap_pt_regs to
> > > let context save/restore routine reference status.VS at which the trap
> > > takes place. The thread flag TIF_RISCV_V_KERNEL_MODE indicates whether
> > > a task is running in kernel-mode Vector with preemption 'ON'. So context
> > > switch routines know and would save V-regs to kernel_vstate and restore
> > > V-regs immediately from kernel_vstate if the bit is set.
> > >
> > > Apart from a task's preemption status, the capability of
> > > running preemptive kernel-mode Vector is jointly controlled by the
> > > RISCV_V_VSTATE_CTRL_PREEMPTIBLE mask in the task's
> > > thread.vstate_ctrl. This bit is masked whenever a trap takes place in
> > > kernel mode while executing preemptive Vector code.
> > >
> > > Also, provide a config CONFIG_RISCV_ISA_V_PREEMPTIVE to give users an
> > > option to disable preemptible kernel-mode Vector at build time. Users
> > > with constraint memory may want to disable this config as preemptible
> > > kernel-mode Vector needs extra space for tracking per thread's
> > > kernel-mode V context. Or, users might as well want to disable it if all
> > > kernel-mode Vector code is time sensitive and cannot tolerate context
> > > swicth overhead.
> > >
> > > Signed-off-by: Andy Chiu <andy.chiu at sifive.com>
> > > ---
> > > Changelog v2:
> > > - fix build fail when compiling without RISCV_ISA_V (Conor)
> > > - 's/TIF_RISCV_V_KMV/TIF_RISCV_V_KERNEL_MODE' and add comment (Conor)
> > > - merge Kconfig patch into this oine (Conor).
> > > - 's/CONFIG_RISCV_ISA_V_PREEMPTIVE_KMV/CONFIG_RISCV_ISA_V_PREEMPTIVE/'
> > > (Conor)
> > > - fix some typos (Conor)
> > > - enclose assembly with RISCV_ISA_V_PREEMPTIVE.
> > > - change riscv_v_vstate_ctrl_config_kmv() to
> > > kernel_vector_allow_preemption() for better understanding. (Conor)
> > > - 's/riscv_v_kmv_preempitble/kernel_vector_preemptible/'
> > > ---
> > > arch/riscv/Kconfig | 10 +++++
> > > arch/riscv/include/asm/processor.h | 2 +
> > > arch/riscv/include/asm/simd.h | 4 +-
> > > arch/riscv/include/asm/thread_info.h | 4 ++
> > > arch/riscv/include/asm/vector.h | 27 +++++++++++--
> > > arch/riscv/kernel/asm-offsets.c | 2 +
> > > arch/riscv/kernel/entry.S | 45 ++++++++++++++++++++++
> > > arch/riscv/kernel/kernel_mode_vector.c | 53 ++++++++++++++++++++++++--
> > > arch/riscv/kernel/process.c | 8 +++-
> > > arch/riscv/kernel/vector.c | 3 +-
> > > 10 files changed, 148 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > index 4c07b9189c86..0622951b15dd 100644
> > > --- a/arch/riscv/Kconfig
> > > +++ b/arch/riscv/Kconfig
> > > @@ -507,6 +507,16 @@ config RISCV_ISA_V_DEFAULT_ENABLE
> > >
> > > If you don't know what to do here, say Y.
> > >
> > > +config RISCV_ISA_V_PREEMPTIVE
> > > + bool "Run kernel-mode Vector with kernel preemption"
> > > + depends on PREEMPTION
> > > + depends on RISCV_ISA_V
> > > + default y
> > > + help
> > > + Ordinarily the kernel disables preemption before running in-kernel
> > > + Vector code. This config frees the kernel from disabling preemption
> > > + by adding memory on demand for tracking kernel's V-context.
> > > +
> > > config TOOLCHAIN_HAS_ZBB
> > > bool
> > > default y
> > > diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
> > > index c950a8d9edef..497c0dd30b2a 100644
> > > --- a/arch/riscv/include/asm/processor.h
> > > +++ b/arch/riscv/include/asm/processor.h
> > > @@ -42,6 +42,8 @@ struct thread_struct {
> > > unsigned long bad_cause;
> > > unsigned long vstate_ctrl;
> > > struct __riscv_v_ext_state vstate;
> > > + struct pt_regs *trap_pt_regs;
> > > + struct __riscv_v_ext_state kernel_vstate;
> > > };
> > >
> > > /* Whitelist the fstate from the task_struct for hardened usercopy */
> > > diff --git a/arch/riscv/include/asm/simd.h b/arch/riscv/include/asm/simd.h
> > > index ef70af78005d..a54a0ce58f4d 100644
> > > --- a/arch/riscv/include/asm/simd.h
> > > +++ b/arch/riscv/include/asm/simd.h
> > > @@ -12,6 +12,7 @@
> > > #include <linux/percpu.h>
> > > #include <linux/preempt.h>
> > > #include <linux/types.h>
> > > +#include <linux/thread_info.h>
> > >
> > > #ifdef CONFIG_RISCV_ISA_V
> > >
> > > @@ -35,7 +36,8 @@ static __must_check inline bool may_use_simd(void)
> > > * where it is set.
> > > */
> > > return !in_irq() && !irqs_disabled() && !in_nmi() &&
> > > - !this_cpu_read(vector_context_busy);
> > > + !this_cpu_read(vector_context_busy) &&
> > > + !test_thread_flag(TIF_RISCV_V_KERNEL_MODE);
> > > }
> > >
> > > #else /* ! CONFIG_RISCV_ISA_V */
> > > diff --git a/arch/riscv/include/asm/thread_info.h b/arch/riscv/include/asm/thread_info.h
> > > index b182f2d03e25..8797d520e8ef 100644
> > > --- a/arch/riscv/include/asm/thread_info.h
> > > +++ b/arch/riscv/include/asm/thread_info.h
> > > @@ -94,6 +94,7 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src);
> > > #define TIF_UPROBE 10 /* uprobe breakpoint or singlestep */
> > > #define TIF_32BIT 11 /* compat-mode 32bit process */
> > > #define TIF_RISCV_V_DEFER_RESTORE 12 /* restore Vector before returing to user */
> > > +#define TIF_RISCV_V_KERNEL_MODE 13 /* kernel-mode Vector run with preemption-on */
> > >
> > > #define _TIF_NOTIFY_RESUME (1 << TIF_NOTIFY_RESUME)
> > > #define _TIF_SIGPENDING (1 << TIF_SIGPENDING)
> > > @@ -101,9 +102,12 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src);
> > > #define _TIF_NOTIFY_SIGNAL (1 << TIF_NOTIFY_SIGNAL)
> > > #define _TIF_UPROBE (1 << TIF_UPROBE)
> > > #define _TIF_RISCV_V_DEFER_RESTORE (1 << TIF_RISCV_V_DEFER_RESTORE)
> > > +#define _TIF_RISCV_V_KERNEL_MODE (1 << TIF_RISCV_V_KERNEL_MODE)
> > >
> > > #define _TIF_WORK_MASK \
> > > (_TIF_NOTIFY_RESUME | _TIF_SIGPENDING | _TIF_NEED_RESCHED | \
> > > _TIF_NOTIFY_SIGNAL | _TIF_UPROBE)
> > >
> > > +#define RISCV_V_VSTATE_CTRL_PREEMPTIBLE 0x20
> > > +
> > > #endif /* _ASM_RISCV_THREAD_INFO_H */
> > > diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h
> > > index 3b783b317112..c2776851d50d 100644
> > > --- a/arch/riscv/include/asm/vector.h
> > > +++ b/arch/riscv/include/asm/vector.h
> > > @@ -195,9 +195,24 @@ static inline void __switch_to_vector(struct task_struct *prev,
> > > {
> > > struct pt_regs *regs;
> > >
> > > - regs = task_pt_regs(prev);
> > > - riscv_v_vstate_save(&prev->thread.vstate, regs);
> > > - riscv_v_vstate_set_restore(next, task_pt_regs(next));
> > > + if (IS_ENABLED(CONFIG_RISCV_ISA_V_PREEMPTIVE) &&
> > > + test_tsk_thread_flag(prev, TIF_RISCV_V_KERNEL_MODE)) {
> > > + regs = prev->thread.trap_pt_regs;
> > > + WARN_ON(!regs);
> >
> > In what cases could these WARN_ON()s be triggered?
>
> It probably happens when a kernel thread calls schedule() in the
> middle of preemptible kernel mode Vector code. Because the kernel sets
> trap_pt_regs only at trap entries. For example
>
> // assume preemption = "ON" and memory allocation
> // for kernel_vstate.datap success
> kernel_vector_begin();
> // some vector code
> ...
> schedule();
> ...
> kernel_vector_end();
>
> It is possible to support making scheduler calls in preemptible kernel
> mode Vector though. We just need to save nothing (all V regs are
> caller-save) and set an appropriate status.VS for the "next" process.
I'm struggling to theorycraft where this can go wrong, because my
knowledge in this area is limited. If the only way it can go wrong is by
calling schedule() in a "protected" section like this, that seems
"okay". Are there not non-trap induced context switches that we need to
worry about?
> > > + riscv_v_vstate_save(&prev->thread.kernel_vstate, regs);
> > > + } else {
> > > + regs = task_pt_regs(prev);
> > > + riscv_v_vstate_save(&prev->thread.vstate, regs);
> > > + }
> > > +
> > > + if (IS_ENABLED(CONFIG_RISCV_ISA_V_PREEMPTIVE) &&
> > > + test_tsk_thread_flag(next, TIF_RISCV_V_KERNEL_MODE)) {
> > > + regs = next->thread.trap_pt_regs;
> > > + WARN_ON(!regs);
> > > + riscv_v_vstate_restore(&next->thread.kernel_vstate, regs);
> > > + } else {
> > > + riscv_v_vstate_set_restore(next, task_pt_regs(next));
> > > + }
> > > }
> >
> >
> > > /*
> > > * kernel_vector_begin(): obtain the CPU vector registers for use by the calling
> > > * context
> > > @@ -70,11 +109,14 @@ void kernel_vector_begin(void)
> > >
> > > riscv_v_vstate_save(¤t->thread.vstate, task_pt_regs(current));
> > >
> > > - get_cpu_vector_context();
> > > + if (!preemptible() || !kernel_vector_preemptible()) {
> > > + get_cpu_vector_context();
> > > + } else {
> > > + if (riscv_v_start_kernel_context())
> > > + get_cpu_vector_context();
> >
> > What happens here if riscv_v_start_kernel_context() fails w/ -ENOMEM?
>
> Here we would fallback to starting kernel-mode Vector with preemption
> disabled, by calling get_cpu_vector_context(). This makes calling
> kernel_vector_begin() end up with 2 possible consequences, if the
> caller runs in a preemptible context. One, which is the success path
> of riscv_v_start_kernel_context(), will not alter the preemption
> status but may increase memory usage if the context does not exist
> yet.
>
> However, if, on the other path, riscv_v_start_kernel_context() fails
> with -ENOMEM, then the kernel-mode Vector code will be executed with
> preemption "off".
>
> Another way of solving this ambiguity is to add another function to
> enable kernel mode Vector with preemption, and let the user check if
> the allocation fails. So users who really want to run their Vector
> code with preemption shall make this call. Otherwise, kernel mode
> Vector runs with preemption off. However, I don't really want to add
> it because I'd like to make the "upgrade" transparent to the caller.
>
> >
> > > + }
> > >
> > > riscv_v_enable();
> > > -
> > > - return 0;
> > > }
> > > EXPORT_SYMBOL_GPL(kernel_vector_begin);
> > >
> > > @@ -96,6 +138,9 @@ void kernel_vector_end(void)
> > >
> > > riscv_v_disable();
> > >
> > > - put_cpu_vector_context();
> > > + if (!test_thread_flag(TIF_RISCV_V_KERNEL_MODE))
> > > + put_cpu_vector_context();
> > > + else
> > > + riscv_v_stop_kernel_context();
> > > }
> >
> > Probably just missing something here, but how come we don't need to call
> > put_cpu_vector_context() here. I'm just a little confused, since, in
> > kernel_vector_begin, get_cpu_vector_context() is called.
>
> If "TIF_RISCV_V_KERNEL_MODE" is set, then we are running kernel-mode
> Vector with preemption "ON". In such cases we don't need to call
> put_cpu_vector_context(), which is the epilogue of kernel-mode Vector
> with preemption "OFF". Instead, we should call
> riscv_v_stop_kernel_context() to end the session.
I think, for these last two comments, I screwed up. I misread
if (riscv_v_start_kernel_context())
as
if (!riscv_v_start_kernel_context())
which is the source of my confusion about this being imbalanced.
Thanks for your explanations however!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-riscv/attachments/20230724/f0a24fc1/attachment.sig>
More information about the linux-riscv
mailing list