lkml.org 
[lkml]   [2008]   [Nov]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [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)


\
 
 \ /
  Last update: 2008-11-27 14:43    [W:0.097 / U:0.060 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site