| Message ID | 20190827135548.21457-4-npiggin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powerpc/64: syscalls in C | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch next (0e4523c0b4f64eaf7abe59e143e6bdf8f972acff) |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 163 lines checked |
Le 27/08/2019 à 15:55, Nicholas Piggin a écrit : > powerpc has an optimisation where interrupts avoid saving the > non-volatile (or callee saved) registers to the interrupt stack frame if > they are not required. > > Two problems with this are that an interrupt does not always know > whether it will need non-volatiles; and if it does need them, they can > only be saved from the entry-scoped asm code (because we don't control > what the C compiler does with these registers). > > system calls are the most difficult: some system calls always require > all registers (e.g., fork, to copy regs into the child). Sometimes > registers are only required under certain conditions (e.g., tracing, > signal delivery). These cases require ugly logic in the call chains > (e.g., ppc_fork), and require a lot of logic to be implemented in asm. Do you really find it ugly to just call function nvgprs() before calling sys_fork() ? I guess there are things a lot uglier. > > So remove the optimisation for system calls, and always save NVGPRs on > entry. Modern high performance CPUs are not so sensitive, because the > stores are dense in cache and can be hidden by other expensive work in > the syscall path -- the null syscall selftests benchmark on POWER9 is > not slowed (124.40ns before and 123.64ns after, i.e., within the noise). I did the test on PPC32: On an 885, null_syscall reports 2227ns (132MHz) If saving non-volatile regs, it goes to 2419, ie +8.6% On an 8321, null_syscall reports 1021ns (333MHz) If saving non-volatile regs, it goes to 1100, ie +7.7% So unless going to C compensates this degradation, I guess it is not worth it on PPC32. > > Other interrupts retain the NVGPR optimisation for now. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > Changes since v1: > - Improve changelog > - Fix clone3 spu table entry (Segher) > > arch/powerpc/kernel/entry_64.S | 72 +++++------------------- > arch/powerpc/kernel/syscalls/syscall.tbl | 22 +++++--- > 2 files changed, 28 insertions(+), 66 deletions(-) > > diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S > index 6467bdab8d40..5a3e0b5c9ad1 100644 > --- a/arch/powerpc/kernel/entry_64.S > +++ b/arch/powerpc/kernel/entry_64.S > @@ -98,13 +98,14 @@ END_BTB_FLUSH_SECTION > std r11,_XER(r1) > std r11,_CTR(r1) > std r9,GPR13(r1) > + SAVE_NVGPRS(r1) > mflr r10 > /* > * This clears CR0.SO (bit 28), which is the error indication on > * return from this system call. > */ > rldimi r2,r11,28,(63-28) > - li r11,0xc01 > + li r11,0xc00 > std r10,_LINK(r1) > std r11,_TRAP(r1) > std r3,ORIG_GPR3(r1) > @@ -323,7 +324,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR) > > /* Traced system call support */ > .Lsyscall_dotrace: > - bl save_nvgprs > addi r3,r1,STACK_FRAME_OVERHEAD > bl do_syscall_trace_enter > > @@ -408,7 +408,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR) > mtmsrd r10,1 > #endif /* CONFIG_PPC_BOOK3E */ > > - bl save_nvgprs > addi r3,r1,STACK_FRAME_OVERHEAD > bl do_syscall_trace_leave > b ret_from_except > @@ -442,62 +441,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR) > _ASM_NOKPROBE_SYMBOL(system_call_common); > _ASM_NOKPROBE_SYMBOL(system_call_exit); > > -/* Save non-volatile GPRs, if not already saved. */ > -_GLOBAL(save_nvgprs) > - ld r11,_TRAP(r1) > - andi. r0,r11,1 > - beqlr- > - SAVE_NVGPRS(r1) > - clrrdi r0,r11,1 > - std r0,_TRAP(r1) > - blr > -_ASM_NOKPROBE_SYMBOL(save_nvgprs); I see it is added back somewhere below. Why don't you leave it where it is ? > - > - > -/* > - * The sigsuspend and rt_sigsuspend system calls can call do_signal > - * and thus put the process into the stopped state where we might > - * want to examine its user state with ptrace. Therefore we need > - * to save all the nonvolatile registers (r14 - r31) before calling > - * the C code. Similarly, fork, vfork and clone need the full > - * register state on the stack so that it can be copied to the child. > - */ > - > -_GLOBAL(ppc_fork) > - bl save_nvgprs > - bl sys_fork > - b .Lsyscall_exit > - > -_GLOBAL(ppc_vfork) > - bl save_nvgprs > - bl sys_vfork > - b .Lsyscall_exit > - > -_GLOBAL(ppc_clone) > - bl save_nvgprs > - bl sys_clone > - b .Lsyscall_exit > - > -_GLOBAL(ppc_clone3) > - bl save_nvgprs > - bl sys_clone3 > - b .Lsyscall_exit > - > -_GLOBAL(ppc32_swapcontext) > - bl save_nvgprs > - bl compat_sys_swapcontext > - b .Lsyscall_exit > - > -_GLOBAL(ppc64_swapcontext) > - bl save_nvgprs > - bl sys_swapcontext > - b .Lsyscall_exit > - > -_GLOBAL(ppc_switch_endian) > - bl save_nvgprs > - bl sys_switch_endian > - b .Lsyscall_exit > - > _GLOBAL(ret_from_fork) > bl schedule_tail > REST_NVGPRS(r1) > @@ -516,6 +459,17 @@ _GLOBAL(ret_from_kernel_thread) > li r3,0 > b .Lsyscall_exit > > +/* Save non-volatile GPRs, if not already saved. */ > +_GLOBAL(save_nvgprs) > + ld r11,_TRAP(r1) > + andi. r0,r11,1 > + beqlr- > + SAVE_NVGPRS(r1) > + clrrdi r0,r11,1 > + std r0,_TRAP(r1) > + blr > +_ASM_NOKPROBE_SYMBOL(save_nvgprs); > + Moved here. > #ifdef CONFIG_PPC_BOOK3S_64 > > #define FLUSH_COUNT_CACHE \ Christophe
Christophe Leroy's on August 28, 2019 7:02 pm: > > > Le 27/08/2019 à 15:55, Nicholas Piggin a écrit : >> powerpc has an optimisation where interrupts avoid saving the >> non-volatile (or callee saved) registers to the interrupt stack frame if >> they are not required. >> >> Two problems with this are that an interrupt does not always know >> whether it will need non-volatiles; and if it does need them, they can >> only be saved from the entry-scoped asm code (because we don't control >> what the C compiler does with these registers). >> >> system calls are the most difficult: some system calls always require >> all registers (e.g., fork, to copy regs into the child). Sometimes >> registers are only required under certain conditions (e.g., tracing, >> signal delivery). These cases require ugly logic in the call chains >> (e.g., ppc_fork), and require a lot of logic to be implemented in asm. > > Do you really find it ugly to just call function nvgprs() before calling > sys_fork() ? I guess there are things a lot uglier. That's not the ugly part, the ugly part is trashing the link register and then branching directly to where it was supposed to return, which is bad for any CPU which has a return predictor so we try to eliminate it from the ppc64 kernel. >> So remove the optimisation for system calls, and always save NVGPRs on >> entry. Modern high performance CPUs are not so sensitive, because the >> stores are dense in cache and can be hidden by other expensive work in >> the syscall path -- the null syscall selftests benchmark on POWER9 is >> not slowed (124.40ns before and 123.64ns after, i.e., within the noise). > > I did the test on PPC32: > > On an 885, null_syscall reports 2227ns (132MHz) > If saving non-volatile regs, it goes to 2419, ie +8.6% > > On an 8321, null_syscall reports 1021ns (333MHz) > If saving non-volatile regs, it goes to 1100, ie +7.7% > > So unless going to C compensates this degradation, I guess it is not > worth it on PPC32. Yeah that's unfortunate. It is a good optimization for small cores. I doubt going to C would help for PPC32, probably be even slower. >> >> -/* Save non-volatile GPRs, if not already saved. */ >> -_GLOBAL(save_nvgprs) >> - ld r11,_TRAP(r1) >> - andi. r0,r11,1 >> - beqlr- >> - SAVE_NVGPRS(r1) >> - clrrdi r0,r11,1 >> - std r0,_TRAP(r1) >> - blr >> -_ASM_NOKPROBE_SYMBOL(save_nvgprs); > > I see it is added back somewhere below. Why don't you leave it where it is ? No longer used by syscalls so I it out from between other syscall related code to improve icache. Thanks, Nick
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S index 6467bdab8d40..5a3e0b5c9ad1 100644 --- a/arch/powerpc/kernel/entry_64.S +++ b/arch/powerpc/kernel/entry_64.S @@ -98,13 +98,14 @@ END_BTB_FLUSH_SECTION std r11,_XER(r1) std r11,_CTR(r1) std r9,GPR13(r1) + SAVE_NVGPRS(r1) mflr r10 /* * This clears CR0.SO (bit 28), which is the error indication on * return from this system call. */ rldimi r2,r11,28,(63-28) - li r11,0xc01 + li r11,0xc00 std r10,_LINK(r1) std r11,_TRAP(r1) std r3,ORIG_GPR3(r1) @@ -323,7 +324,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR) /* Traced system call support */ .Lsyscall_dotrace: - bl save_nvgprs addi r3,r1,STACK_FRAME_OVERHEAD bl do_syscall_trace_enter @@ -408,7 +408,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR) mtmsrd r10,1 #endif /* CONFIG_PPC_BOOK3E */ - bl save_nvgprs addi r3,r1,STACK_FRAME_OVERHEAD bl do_syscall_trace_leave b ret_from_except @@ -442,62 +441,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR) _ASM_NOKPROBE_SYMBOL(system_call_common); _ASM_NOKPROBE_SYMBOL(system_call_exit); -/* Save non-volatile GPRs, if not already saved. */ -_GLOBAL(save_nvgprs) - ld r11,_TRAP(r1) - andi. r0,r11,1 - beqlr- - SAVE_NVGPRS(r1) - clrrdi r0,r11,1 - std r0,_TRAP(r1) - blr -_ASM_NOKPROBE_SYMBOL(save_nvgprs); - - -/* - * The sigsuspend and rt_sigsuspend system calls can call do_signal - * and thus put the process into the stopped state where we might - * want to examine its user state with ptrace. Therefore we need - * to save all the nonvolatile registers (r14 - r31) before calling - * the C code. Similarly, fork, vfork and clone need the full - * register state on the stack so that it can be copied to the child. - */ - -_GLOBAL(ppc_fork) - bl save_nvgprs - bl sys_fork - b .Lsyscall_exit - -_GLOBAL(ppc_vfork) - bl save_nvgprs - bl sys_vfork - b .Lsyscall_exit - -_GLOBAL(ppc_clone) - bl save_nvgprs - bl sys_clone - b .Lsyscall_exit - -_GLOBAL(ppc_clone3) - bl save_nvgprs - bl sys_clone3 - b .Lsyscall_exit - -_GLOBAL(ppc32_swapcontext) - bl save_nvgprs - bl compat_sys_swapcontext - b .Lsyscall_exit - -_GLOBAL(ppc64_swapcontext) - bl save_nvgprs - bl sys_swapcontext - b .Lsyscall_exit - -_GLOBAL(ppc_switch_endian) - bl save_nvgprs - bl sys_switch_endian - b .Lsyscall_exit - _GLOBAL(ret_from_fork) bl schedule_tail REST_NVGPRS(r1) @@ -516,6 +459,17 @@ _GLOBAL(ret_from_kernel_thread) li r3,0 b .Lsyscall_exit +/* Save non-volatile GPRs, if not already saved. */ +_GLOBAL(save_nvgprs) + ld r11,_TRAP(r1) + andi. r0,r11,1 + beqlr- + SAVE_NVGPRS(r1) + clrrdi r0,r11,1 + std r0,_TRAP(r1) + blr +_ASM_NOKPROBE_SYMBOL(save_nvgprs); + #ifdef CONFIG_PPC_BOOK3S_64 #define FLUSH_COUNT_CACHE \ diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl index 43f736ed47f2..d899bcb5343e 100644 --- a/arch/powerpc/kernel/syscalls/syscall.tbl +++ b/arch/powerpc/kernel/syscalls/syscall.tbl @@ -9,7 +9,9 @@ # 0 nospu restart_syscall sys_restart_syscall 1 nospu exit sys_exit -2 nospu fork ppc_fork +2 32 fork ppc_fork sys_fork +2 64 fork sys_fork +2 spu fork sys_ni_syscall 3 common read sys_read 4 common write sys_write 5 common open sys_open compat_sys_open @@ -158,7 +160,9 @@ 119 32 sigreturn sys_sigreturn compat_sys_sigreturn 119 64 sigreturn sys_ni_syscall 119 spu sigreturn sys_ni_syscall -120 nospu clone ppc_clone +120 32 clone ppc_clone sys_clone +120 64 clone sys_clone +120 spu clone sys_ni_syscall 121 common setdomainname sys_setdomainname 122 common uname sys_newuname 123 common modify_ldt sys_ni_syscall @@ -240,7 +244,9 @@ 186 spu sendfile sys_sendfile64 187 common getpmsg sys_ni_syscall 188 common putpmsg sys_ni_syscall -189 nospu vfork ppc_vfork +189 32 vfork ppc_vfork sys_vfork +189 64 vfork sys_vfork +189 spu vfork sys_ni_syscall 190 common ugetrlimit sys_getrlimit compat_sys_getrlimit 191 common readahead sys_readahead compat_sys_readahead 192 32 mmap2 sys_mmap2 compat_sys_mmap2 @@ -316,8 +322,8 @@ 248 32 clock_nanosleep sys_clock_nanosleep_time32 248 64 clock_nanosleep sys_clock_nanosleep 248 spu clock_nanosleep sys_clock_nanosleep -249 32 swapcontext ppc_swapcontext ppc32_swapcontext -249 64 swapcontext ppc64_swapcontext +249 32 swapcontext ppc_swapcontext compat_sys_swapcontext +249 64 swapcontext sys_swapcontext 249 spu swapcontext sys_ni_syscall 250 common tgkill sys_tgkill 251 32 utimes sys_utimes_time32 @@ -456,7 +462,7 @@ 361 common bpf sys_bpf 362 nospu execveat sys_execveat compat_sys_execveat 363 32 switch_endian sys_ni_syscall -363 64 switch_endian ppc_switch_endian +363 64 switch_endian sys_switch_endian 363 spu switch_endian sys_ni_syscall 364 common userfaultfd sys_userfaultfd 365 common membarrier sys_membarrier @@ -516,4 +522,6 @@ 432 common fsmount sys_fsmount 433 common fspick sys_fspick 434 common pidfd_open sys_pidfd_open -435 nospu clone3 ppc_clone3 +435 32 clone3 ppc_clone3 sys_clone3 +435 64 clone3 sys_clone3 +435 spu clone3 sys_ni_syscall
powerpc has an optimisation where interrupts avoid saving the non-volatile (or callee saved) registers to the interrupt stack frame if they are not required. Two problems with this are that an interrupt does not always know whether it will need non-volatiles; and if it does need them, they can only be saved from the entry-scoped asm code (because we don't control what the C compiler does with these registers). system calls are the most difficult: some system calls always require all registers (e.g., fork, to copy regs into the child). Sometimes registers are only required under certain conditions (e.g., tracing, signal delivery). These cases require ugly logic in the call chains (e.g., ppc_fork), and require a lot of logic to be implemented in asm. So remove the optimisation for system calls, and always save NVGPRs on entry. Modern high performance CPUs are not so sensitive, because the stores are dense in cache and can be hidden by other expensive work in the syscall path -- the null syscall selftests benchmark on POWER9 is not slowed (124.40ns before and 123.64ns after, i.e., within the noise). Other interrupts retain the NVGPR optimisation for now. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- Changes since v1: - Improve changelog - Fix clone3 spu table entry (Segher) arch/powerpc/kernel/entry_64.S | 72 +++++------------------- arch/powerpc/kernel/syscalls/syscall.tbl | 22 +++++--- 2 files changed, 28 insertions(+), 66 deletions(-)