Re: [PATCH 2/5] x86: ret_from_fork - get rid of jump back

[Ingo Molnar <mingo@elte.hu>]

* 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)