Messages in this thread Patch in this message | | | Date | Thu, 27 Nov 2008 14:41:21 +0100 | From | Ingo Molnar <> | Subject | Re: [PATCH 2/5] x86: ret_from_fork - get rid of jump back |
| |
* Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> [Andi Kleen - Wed, Nov 26, 2008 at 09:04:33PM +0100] > | gorcunov@gmail.com writes: > | > --- a/arch/x86/kernel/entry_64.S > | > +++ b/arch/x86/kernel/entry_64.S > | > @@ -379,7 +379,10 @@ ENTRY(ret_from_fork) > | > GET_THREAD_INFO(%rcx) > | > testl $(_TIF_SYSCALL_TRACE|_TIF_SYSCALL_AUDIT),TI_flags(%rcx) > | > CFI_REMEMBER_STATE > | > - jnz rff_trace > | > + jz rff_action > | > + movq %rsp,%rdi > | > + call syscall_trace_leave > | > + GET_THREAD_INFO(%rcx) > | > | The uncommon path is supposed to be out of line. I don't think > | the CPU will like that. > | > | -Andi > | > | > rff_action: > | > | -- > | ak@linux.intel.com > | > > Aha! Thanks Andi for review.
Unfortunately that review got you off the right track ...
The principle you applied was good: entry_64.S is murky, and we want to simplify the current overcomplicated assembly code flow spaghetti there.
Note that if you take a closer look at rff_trace/rff_action ret_from_fork code here, you'll realize that it does all the wrong things: for example it checks the TIF flag - while later on jumping back to the ret-from-syscall path - duplicating the check needlessly.
But it gets worse than that: checking for _TIF_SYSCALL_TRACE is completely unnecessary here because we clear that flag for every freshly forked task. So the whole "tracing" code here, for which there is this "out of line jump optimization" is in reality completely dead code in the ptrace case...
Could you test something like the patch attached below, which cleans up this code and applies the code reduction and speedup? Warning: completely untested! Please check that things like strace -f and gdb attaching to forked tasks still works fine. (it should by all means)
Thanks,
Ingo
--- arch/x86/kernel/entry_64.S | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-)
Index: linux/arch/x86/kernel/entry_64.S =================================================================== --- linux.orig/arch/x86/kernel/entry_64.S +++ linux/arch/x86/kernel/entry_64.S @@ -366,34 +366,35 @@ ENTRY(save_paranoid) END(save_paranoid) /* - * A newly forked process directly context switches into this. + * A newly forked process directly context switches into this address. + * + * rdi: prev task we switched from */ -/* rdi: prev */ ENTRY(ret_from_fork) DEFAULT_FRAME + push kernel_eflags(%rip) CFI_ADJUST_CFA_OFFSET 8 - popf # reset kernel eflags + popf # reset kernel eflags CFI_ADJUST_CFA_OFFSET -8 - call schedule_tail + + call schedule_tail # rdi: 'prev' task parameter + GET_THREAD_INFO(%rcx) - testl $(_TIF_SYSCALL_TRACE|_TIF_SYSCALL_AUDIT),TI_flags(%rcx) + CFI_REMEMBER_STATE - jnz rff_trace -rff_action: RESTORE_REST - testl $3,CS-ARGOFFSET(%rsp) # from kernel_thread? + + testl $3, CS-ARGOFFSET(%rsp) # from kernel_thread? je int_ret_from_sys_call - testl $_TIF_IA32,TI_flags(%rcx) + + testl $_TIF_IA32, TI_flags(%rcx) # 32-bit compat task needs IRET jnz int_ret_from_sys_call + RESTORE_TOP_OF_STACK %rdi, -ARGOFFSET - jmp ret_from_sys_call + jmp ret_from_sys_call # go to the SYSRET fastpath + CFI_RESTORE_STATE -rff_trace: - movq %rsp,%rdi - call syscall_trace_leave - GET_THREAD_INFO(%rcx) - jmp rff_action CFI_ENDPROC END(ret_from_fork)
| |