| Message ID | 20240313103334.4036554-1-xiao.w.wang@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 74362d66a4164f8574c930912a9de8fb844a66e8 |
| Headers | show |
| Series | riscv: uaccess: Allow the last potential unrolled copy | expand |
Hi Xiao, On 13/03/2024 11:33, Xiao Wang wrote: > When the dst buffer pointer points to the last accessible aligned addr, we > could still run another iteration of unrolled copy. > > Signed-off-by: Xiao Wang <xiao.w.wang@intel.com> > --- > arch/riscv/lib/uaccess.S | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/riscv/lib/uaccess.S b/arch/riscv/lib/uaccess.S > index 2e665f8f8fcc..1399d797d81b 100644 > --- a/arch/riscv/lib/uaccess.S > +++ b/arch/riscv/lib/uaccess.S > @@ -103,7 +103,7 @@ SYM_FUNC_START(fallback_scalar_usercopy) > fixup REG_S t4, 7*SZREG(a0), 10f > addi a0, a0, 8*SZREG > addi a1, a1, 8*SZREG > - bltu a0, t0, 2b > + bleu a0, t0, 2b > > addi t0, t0, 8*SZREG /* revert to original value */ > j .Lbyte_copy_tail I agree it is still safe to continue for another word_copy here. Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com> Thanks, Alex
On 03/05/2024 13:16, Alexandre Ghiti wrote: > Hi Xiao, > > On 13/03/2024 11:33, Xiao Wang wrote: >> When the dst buffer pointer points to the last accessible aligned >> addr, we >> could still run another iteration of unrolled copy. >> >> Signed-off-by: Xiao Wang <xiao.w.wang@intel.com> >> --- >> arch/riscv/lib/uaccess.S | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/riscv/lib/uaccess.S b/arch/riscv/lib/uaccess.S >> index 2e665f8f8fcc..1399d797d81b 100644 >> --- a/arch/riscv/lib/uaccess.S >> +++ b/arch/riscv/lib/uaccess.S >> @@ -103,7 +103,7 @@ SYM_FUNC_START(fallback_scalar_usercopy) >> fixup REG_S t4, 7*SZREG(a0), 10f >> addi a0, a0, 8*SZREG >> addi a1, a1, 8*SZREG >> - bltu a0, t0, 2b >> + bleu a0, t0, 2b >> addi t0, t0, 8*SZREG /* revert to original value */ >> j .Lbyte_copy_tail > > > I agree it is still safe to continue for another word_copy here. > > Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com> Out of interest, has anyone checked if causing a schedule event during this code breaks like the last time we had issues with the upstream testing? I did propose saving the state of the user-access flag in the task struct but we mostly solved it by making sleeping functions stay away from the address calculation. This of course may have been done already or need to be done if three's long areas where the user-access flags can be disabled (generally only a few drivers did this, so we may not have come across the problem)
Hi Ben, On 03/05/2024 14:19, Ben Dooks wrote: > On 03/05/2024 13:16, Alexandre Ghiti wrote: >> Hi Xiao, >> >> On 13/03/2024 11:33, Xiao Wang wrote: >>> When the dst buffer pointer points to the last accessible aligned >>> addr, we >>> could still run another iteration of unrolled copy. >>> >>> Signed-off-by: Xiao Wang <xiao.w.wang@intel.com> >>> --- >>> arch/riscv/lib/uaccess.S | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/arch/riscv/lib/uaccess.S b/arch/riscv/lib/uaccess.S >>> index 2e665f8f8fcc..1399d797d81b 100644 >>> --- a/arch/riscv/lib/uaccess.S >>> +++ b/arch/riscv/lib/uaccess.S >>> @@ -103,7 +103,7 @@ SYM_FUNC_START(fallback_scalar_usercopy) >>> fixup REG_S t4, 7*SZREG(a0), 10f >>> addi a0, a0, 8*SZREG >>> addi a1, a1, 8*SZREG >>> - bltu a0, t0, 2b >>> + bleu a0, t0, 2b >>> addi t0, t0, 8*SZREG /* revert to original value */ >>> j .Lbyte_copy_tail >> >> >> I agree it is still safe to continue for another word_copy here. >> >> Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com> > > Out of interest, has anyone checked if causing a schedule event during > this code breaks like the last time we had issues with the upstream > testing? I vaguely remember something, do you have a link to that discussion by chance? > > I did propose saving the state of the user-access flag in the task > struct Makes sense, I just took a quick look and SR_SUM is cleared as soon as we enter handle_exception() and it does not seem to be restored. Weird it works, unless I missed something! > but we mostly solved it by making sleeping functions stay > away from the address calculation. This of course may have been done > already or need to be done if three's long areas where the user-access > flags can be disabled (generally only a few drivers did this, so we > may not have come across the problem) > I don't understand what you mean here, would you mind expanding a bit? Thanks, Alex
On 03/05/2024 14:02, Alexandre Ghiti wrote: > Hi Ben, > > On 03/05/2024 14:19, Ben Dooks wrote: >> On 03/05/2024 13:16, Alexandre Ghiti wrote: >>> Hi Xiao, >>> >>> On 13/03/2024 11:33, Xiao Wang wrote: >>>> When the dst buffer pointer points to the last accessible aligned >>>> addr, we >>>> could still run another iteration of unrolled copy. >>>> >>>> Signed-off-by: Xiao Wang <xiao.w.wang@intel.com> >>>> --- >>>> arch/riscv/lib/uaccess.S | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/arch/riscv/lib/uaccess.S b/arch/riscv/lib/uaccess.S >>>> index 2e665f8f8fcc..1399d797d81b 100644 >>>> --- a/arch/riscv/lib/uaccess.S >>>> +++ b/arch/riscv/lib/uaccess.S >>>> @@ -103,7 +103,7 @@ SYM_FUNC_START(fallback_scalar_usercopy) >>>> fixup REG_S t4, 7*SZREG(a0), 10f >>>> addi a0, a0, 8*SZREG >>>> addi a1, a1, 8*SZREG >>>> - bltu a0, t0, 2b >>>> + bleu a0, t0, 2b >>>> addi t0, t0, 8*SZREG /* revert to original value */ >>>> j .Lbyte_copy_tail >>> >>> >>> I agree it is still safe to continue for another word_copy here. >>> >>> Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com> >> >> Out of interest, has anyone checked if causing a schedule event during >> this code breaks like the last time we had issues with the upstream >> testing? > > > I vaguely remember something, do you have a link to that discussion by > chance? > > >> >> I did propose saving the state of the user-access flag in the task >> struct > > > Makes sense, I just took a quick look and SR_SUM is cleared as soon as > we enter handle_exception() and it does not seem to be restored. Weird > it works, unless I missed something! > > >> but we mostly solved it by making sleeping functions stay >> away from the address calculation. This of course may have been done >> already or need to be done if three's long areas where the user-access >> flags can be disabled (generally only a few drivers did this, so we >> may not have come across the problem) >> > I don't understand what you mean here, would you mind expanding a bit? > I think this was all gone through in the original post where we initially suggested saving SR_SUM and then moved as much out of the critical SR_SUM area by changing how the macros worked https://lore.kernel.org/linux-riscv/20210318151010.100966-1-ben.dooks@codethink.co.uk/ https://lore.kernel.org/linux-riscv/20210329095749.998940-1-ben.dooks@codethink.co.uk/
Hi Alex, > -----Original Message----- > From: ben.dooks@codethink.co.uk <ben.dooks@codethink.co.uk> > Sent: Friday, May 3, 2024 10:30 PM > To: Alexandre Ghiti <alex@ghiti.fr>; Wang, Xiao W <xiao.w.wang@intel.com>; > paul.walmsley@sifive.com; palmer@dabbelt.com; aou@eecs.berkeley.edu > Cc: jerry.shih@sifive.com; nick.knight@sifive.com; > ajones@ventanamicro.com; bjorn@rivosinc.com; andy.chiu@sifive.com; > viro@zeniv.linux.org.uk; cleger@rivosinc.com; alexghiti@rivosinc.com; Li, > Haicheng <haicheng.li@intel.com>; akira.tsukamoto@gmail.com; linux- > riscv@lists.infradead.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH] riscv: uaccess: Allow the last potential unrolled copy > > On 03/05/2024 14:02, Alexandre Ghiti wrote: > > Hi Ben, > > > > On 03/05/2024 14:19, Ben Dooks wrote: > >> On 03/05/2024 13:16, Alexandre Ghiti wrote: > >>> Hi Xiao, > >>> > >>> On 13/03/2024 11:33, Xiao Wang wrote: > >>>> When the dst buffer pointer points to the last accessible aligned > >>>> addr, we > >>>> could still run another iteration of unrolled copy. > >>>> > >>>> Signed-off-by: Xiao Wang <xiao.w.wang@intel.com> > >>>> --- > >>>> arch/riscv/lib/uaccess.S | 2 +- > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>> > >>>> diff --git a/arch/riscv/lib/uaccess.S b/arch/riscv/lib/uaccess.S > >>>> index 2e665f8f8fcc..1399d797d81b 100644 > >>>> --- a/arch/riscv/lib/uaccess.S > >>>> +++ b/arch/riscv/lib/uaccess.S > >>>> @@ -103,7 +103,7 @@ SYM_FUNC_START(fallback_scalar_usercopy) > >>>> fixup REG_S t4, 7*SZREG(a0), 10f > >>>> addi a0, a0, 8*SZREG > >>>> addi a1, a1, 8*SZREG > >>>> - bltu a0, t0, 2b > >>>> + bleu a0, t0, 2b > >>>> addi t0, t0, 8*SZREG /* revert to original value */ > >>>> j .Lbyte_copy_tail > >>> > >>> > >>> I agree it is still safe to continue for another word_copy here. > >>> > >>> Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com> > >> > >> Out of interest, has anyone checked if causing a schedule event during > >> this code breaks like the last time we had issues with the upstream > >> testing? > > > > > > I vaguely remember something, do you have a link to that discussion by > > chance? > > > > > >> > >> I did propose saving the state of the user-access flag in the task > >> struct > > > > > > Makes sense, I just took a quick look and SR_SUM is cleared as soon as > > we enter handle_exception() and it does not seem to be restored. Weird > > it works, unless I missed something! I think it's already saved/restored via pt_regs.status, using the PT_STATUS() macro in entry.S. BRs, Xiao > > > > > >> but we mostly solved it by making sleeping functions stay > >> away from the address calculation. This of course may have been done > >> already or need to be done if three's long areas where the user-access > >> flags can be disabled (generally only a few drivers did this, so we > >> may not have come across the problem) > >> > > I don't understand what you mean here, would you mind expanding a bit? > > > > I think this was all gone through in the original post where > we initially suggested saving SR_SUM and then moved as much out > of the critical SR_SUM area by changing how the macros worked > > https://lore.kernel.org/linux-riscv/20210318151010.100966-1- > ben.dooks@codethink.co.uk/ > > https://lore.kernel.org/linux-riscv/20210329095749.998940-1- > ben.dooks@codethink.co.uk/ > -- > Ben Dooks http://www.codethink.co.uk/ > Senior Engineer Codethink - Providing Genius > > https://www.codethink.co.uk/privacy.html
Hello: This patch was applied to riscv/linux.git (for-next) by Palmer Dabbelt <palmer@rivosinc.com>: On Wed, 13 Mar 2024 18:33:34 +0800 you wrote: > When the dst buffer pointer points to the last accessible aligned addr, we > could still run another iteration of unrolled copy. > > Signed-off-by: Xiao Wang <xiao.w.wang@intel.com> > --- > arch/riscv/lib/uaccess.S | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Here is the summary with links: - riscv: uaccess: Allow the last potential unrolled copy https://git.kernel.org/riscv/c/74362d66a416 You are awesome, thank you!
diff --git a/arch/riscv/lib/uaccess.S b/arch/riscv/lib/uaccess.S index 2e665f8f8fcc..1399d797d81b 100644 --- a/arch/riscv/lib/uaccess.S +++ b/arch/riscv/lib/uaccess.S @@ -103,7 +103,7 @@ SYM_FUNC_START(fallback_scalar_usercopy) fixup REG_S t4, 7*SZREG(a0), 10f addi a0, a0, 8*SZREG addi a1, a1, 8*SZREG - bltu a0, t0, 2b + bleu a0, t0, 2b addi t0, t0, 8*SZREG /* revert to original value */ j .Lbyte_copy_tail
When the dst buffer pointer points to the last accessible aligned addr, we could still run another iteration of unrolled copy. Signed-off-by: Xiao Wang <xiao.w.wang@intel.com> --- arch/riscv/lib/uaccess.S | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)