| Message ID | 20231221070449.1809020-1-songshuaishuai@tinylab.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e1b76bc00ed1835060c5ff89d19b77fcb9918b92 |
| Headers | show |
| Series | riscv: vector: Check SR_SD before saving vstate | expand |
> -----Original Message----- > From: Song Shuai <songshuaishuai@tinylab.org> > Sent: Thursday, December 21, 2023 3:05 PM > To: paul.walmsley@sifive.com; palmer@dabbelt.com; > aou@eecs.berkeley.edu; andy.chiu@sifive.com; greentime.hu@sifive.com; > conor.dooley@microchip.com; guoren@kernel.org; > songshuaishuai@tinylab.org; bjorn@rivosinc.com; Wang, Xiao W > <xiao.w.wang@intel.com>; heiko@sntech.de; ruinland.tsai@sifive.com > Cc: linux-riscv@lists.infradead.org; linux-kernel@vger.kernel.org > Subject: [PATCH] riscv: vector: Check SR_SD before saving vstate > > The SD bit summarizes the dirty states of FS, VS, or XS fields, > providing a "fast check" before saving fstate or vstate. > > Let __switch_to_vector() check SD bit as __switch_to_fpu() does. It looks a duplication of status check since the __switch_to_*() internally will check the ext specific status bit. Can we just remove SR_SD check for the fpu() case? BRs, Xiao > > Fixes: 3a2df6323def ("riscv: Add task switch support for vector") > Signed-off-by: Song Shuai <songshuaishuai@tinylab.org> > --- > arch/riscv/include/asm/vector.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h > index 87aaef656257..d30fa56f67c6 100644 > --- a/arch/riscv/include/asm/vector.h > +++ b/arch/riscv/include/asm/vector.h > @@ -190,7 +190,8 @@ static inline void __switch_to_vector(struct > task_struct *prev, > struct pt_regs *regs; > > regs = task_pt_regs(prev); > - riscv_v_vstate_save(prev, regs); > + if (unlikely(regs->status & SR_SD)) > + riscv_v_vstate_save(prev, regs); > riscv_v_vstate_restore(next, task_pt_regs(next)); > } > > -- > 2.20.1
On Thu, Dec 21, 2023 at 3:37 PM Wang, Xiao W <xiao.w.wang@intel.com> wrote: > > > > > -----Original Message----- > > From: Song Shuai <songshuaishuai@tinylab.org> > > Sent: Thursday, December 21, 2023 3:05 PM > > To: paul.walmsley@sifive.com; palmer@dabbelt.com; > > aou@eecs.berkeley.edu; andy.chiu@sifive.com; greentime.hu@sifive.com; > > conor.dooley@microchip.com; guoren@kernel.org; > > songshuaishuai@tinylab.org; bjorn@rivosinc.com; Wang, Xiao W > > <xiao.w.wang@intel.com>; heiko@sntech.de; ruinland.tsai@sifive.com > > Cc: linux-riscv@lists.infradead.org; linux-kernel@vger.kernel.org > > Subject: [PATCH] riscv: vector: Check SR_SD before saving vstate > > > > The SD bit summarizes the dirty states of FS, VS, or XS fields, > > providing a "fast check" before saving fstate or vstate. > > > > Let __switch_to_vector() check SD bit as __switch_to_fpu() does. > > It looks a duplication of status check since the __switch_to_*() internally will check the ext specific status bit. > Can we just remove SR_SD check for the fpu() case? Hi, I came to the same question when adding the Vector context switch. I think the better solution is to skip saving both fpu and vector if the SD is clear. However, this may make code less maintainable because fpu and vector code are scattered and we must mix code together by doing that. Also, we will have to duplicate has_fpu and has_vector check because of the branch dependency e.g. if (likely((regs->status & SR_SD))) { if (has_fpu()) fstate_save() if (has_vector()) vstate_save() } if (has_fpu()) <--- duplicated check (nop) fstate_restore() if (has_vector()) <--- same vstate_restore() So, it really is "Is it worth to trade extra nop with SR_SD that potentially skip one SR_*S check" Besides, with user space libraries start embracing Vector, I don't expect SR_SD would be in "cleared" state very often. Though I haven't done any real-world experiment yet. > > BRs, > Xiao > > > > > Fixes: 3a2df6323def ("riscv: Add task switch support for vector") > > Signed-off-by: Song Shuai <songshuaishuai@tinylab.org> > > --- > > arch/riscv/include/asm/vector.h | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h > > index 87aaef656257..d30fa56f67c6 100644 > > --- a/arch/riscv/include/asm/vector.h > > +++ b/arch/riscv/include/asm/vector.h > > @@ -190,7 +190,8 @@ static inline void __switch_to_vector(struct > > task_struct *prev, > > struct pt_regs *regs; > > > > regs = task_pt_regs(prev); > > - riscv_v_vstate_save(prev, regs); > > + if (unlikely(regs->status & SR_SD)) > > + riscv_v_vstate_save(prev, regs); > > riscv_v_vstate_restore(next, task_pt_regs(next)); > > } > > > > -- > > 2.20.1 > Thanks, Andy
Hello: This patch was applied to riscv/linux.git (for-next) by Palmer Dabbelt <palmer@rivosinc.com>: On Thu, 21 Dec 2023 15:04:49 +0800 you wrote: > The SD bit summarizes the dirty states of FS, VS, or XS fields, > providing a "fast check" before saving fstate or vstate. > > Let __switch_to_vector() check SD bit as __switch_to_fpu() does. > > Fixes: 3a2df6323def ("riscv: Add task switch support for vector") > Signed-off-by: Song Shuai <songshuaishuai@tinylab.org> > > [...] Here is the summary with links: - riscv: vector: Check SR_SD before saving vstate https://git.kernel.org/riscv/c/e1b76bc00ed1 You are awesome, thank you!
Hi Palmer, On Thu, Jan 11, 2024 at 10:50 PM <patchwork-bot+linux-riscv@kernel.org> wrote: > > Hello: > > This patch was applied to riscv/linux.git (for-next) IIUC the conclusion for this thread is not to check SD bit for either vector or fpu. The patch for this was sent together with the kernel-mode vector series and has been reviewed-by both Song and Guo. > by Palmer Dabbelt <palmer@rivosinc.com>: > > On Thu, 21 Dec 2023 15:04:49 +0800 you wrote: > > The SD bit summarizes the dirty states of FS, VS, or XS fields, > > providing a "fast check" before saving fstate or vstate. > > > > Let __switch_to_vector() check SD bit as __switch_to_fpu() does. > > > > Fixes: 3a2df6323def ("riscv: Add task switch support for vector") > > Signed-off-by: Song Shuai <songshuaishuai@tinylab.org> > > > > [...] > > Here is the summary with links: > - riscv: vector: Check SR_SD before saving vstate > https://git.kernel.org/riscv/c/e1b76bc00ed1 > > You are awesome, thank you! > -- > Deet-doot-dot, I am a bot. > https://korg.docs.kernel.org/patchwork/pwbot.html > > Please let me know if I missed anything. Thanks, Andy
On Thu, 11 Jan 2024 07:16:06 PST (-0800), andy.chiu@sifive.com wrote: > Hi Palmer, > > On Thu, Jan 11, 2024 at 10:50 PM <patchwork-bot+linux-riscv@kernel.org> wrote: >> >> Hello: >> >> This patch was applied to riscv/linux.git (for-next) > > IIUC the conclusion for this thread is not to check SD bit for either > vector or fpu. The patch for this was sent together with the > kernel-mode vector series and has been reviewed-by both Song and Guo. > >> by Palmer Dabbelt <palmer@rivosinc.com>: >> >> On Thu, 21 Dec 2023 15:04:49 +0800 you wrote: >> > The SD bit summarizes the dirty states of FS, VS, or XS fields, >> > providing a "fast check" before saving fstate or vstate. >> > >> > Let __switch_to_vector() check SD bit as __switch_to_fpu() does. >> > >> > Fixes: 3a2df6323def ("riscv: Add task switch support for vector") >> > Signed-off-by: Song Shuai <songshuaishuai@tinylab.org> >> > >> > [...] >> >> Here is the summary with links: >> - riscv: vector: Check SR_SD before saving vstate >> https://git.kernel.org/riscv/c/e1b76bc00ed1 >> >> You are awesome, thank you! >> -- >> Deet-doot-dot, I am a bot. >> https://korg.docs.kernel.org/patchwork/pwbot.html >> >> > > Please let me know if I missed anything. Sorry, I must have misunderstood. I'm dropping it.
diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h index 87aaef656257..d30fa56f67c6 100644 --- a/arch/riscv/include/asm/vector.h +++ b/arch/riscv/include/asm/vector.h @@ -190,7 +190,8 @@ static inline void __switch_to_vector(struct task_struct *prev, struct pt_regs *regs; regs = task_pt_regs(prev); - riscv_v_vstate_save(prev, regs); + if (unlikely(regs->status & SR_SD)) + riscv_v_vstate_save(prev, regs); riscv_v_vstate_restore(next, task_pt_regs(next)); }
The SD bit summarizes the dirty states of FS, VS, or XS fields, providing a "fast check" before saving fstate or vstate. Let __switch_to_vector() check SD bit as __switch_to_fpu() does. Fixes: 3a2df6323def ("riscv: Add task switch support for vector") Signed-off-by: Song Shuai <songshuaishuai@tinylab.org> --- arch/riscv/include/asm/vector.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)