Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1404856 > unrolled thread

[PATCH 3/4] x86: Rewrite switch_to() code

Started byBrian Gerst <brgerst@gmail.com>
First post2016-05-21 18:10 +0200
Last post2016-05-23 13:50 +0200
Articles 15 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3/4] x86: Rewrite switch_to() code Brian Gerst <brgerst@gmail.com> - 2016-05-21 18:10 +0200
    Re: [PATCH 3/4] x86: Rewrite switch_to() code Andy Lutomirski <luto@amacapital.net> - 2016-05-22 20:10 +0200
      Re: [PATCH 3/4] x86: Rewrite switch_to() code Brian Gerst <brgerst@gmail.com> - 2016-05-22 21:40 +0200
        Re: [PATCH 3/4] x86: Rewrite switch_to() code Andy Lutomirski <luto@amacapital.net> - 2016-05-22 23:10 +0200
      Re: [PATCH 3/4] x86: Rewrite switch_to() code Josh Poimboeuf <jpoimboe@redhat.com> - 2016-05-23 04:40 +0200
        Re: [PATCH 3/4] x86: Rewrite switch_to() code Andy Lutomirski <luto@amacapital.net> - 2016-05-23 06:50 +0200
          Re: [PATCH 3/4] x86: Rewrite switch_to() code Josh Poimboeuf <jpoimboe@redhat.com> - 2016-05-23 13:50 +0200
            Re: [PATCH 3/4] x86: Rewrite switch_to() code Brian Gerst <brgerst@gmail.com> - 2016-05-23 13:50 +0200
              Re: [PATCH 3/4] x86: Rewrite switch_to() code Josh Poimboeuf <jpoimboe@redhat.com> - 2016-05-23 14:10 +0200
        Re: [PATCH 3/4] x86: Rewrite switch_to() code Brian Gerst <brgerst@gmail.com> - 2016-05-23 13:20 +0200
          Re: [PATCH 3/4] x86: Rewrite switch_to() code Josh Poimboeuf <jpoimboe@redhat.com> - 2016-05-23 13:50 +0200
            Re: [PATCH 3/4] x86: Rewrite switch_to() code Josh Poimboeuf <jpoimboe@redhat.com> - 2016-05-23 18:50 +0200
              Re: [PATCH 3/4] x86: Rewrite switch_to() code Andy Lutomirski <luto@amacapital.net> - 2016-05-23 19:10 +0200
                Re: [PATCH 3/4] x86: Rewrite switch_to() code Josh Poimboeuf <jpoimboe@redhat.com> - 2016-05-23 20:50 +0200
          Re: [PATCH 3/4] x86: Rewrite switch_to() code Josh Poimboeuf <jpoimboe@redhat.com> - 2016-05-23 13:50 +0200

#1404856 — [PATCH 3/4] x86: Rewrite switch_to() code

FromBrian Gerst <brgerst@gmail.com>
Date2016-05-21 18:10 +0200
Subject[PATCH 3/4] x86: Rewrite switch_to() code
Message-ID<rBhDs-5HD-7@gated-at.bofh.it>
Move the low-level context switch code to an out-of-line asm stub instead of
using complex inline asm.  This allows constructing a new stack frame for the
child process to make it seamlessly flow to ret_from_fork without an extra
test and branch in __switch_to().  It also improves code generation for
__schedule() by using the C calling convention instead of clobbering all
registers.

Signed-off-by: Brian Gerst <brgerst@gmail.com>
---
 arch/x86/entry/entry_32.S          |  38 ++++++++++
 arch/x86/entry/entry_64.S          |  42 +++++++++++-
 arch/x86/include/asm/processor.h   |   3 -
 arch/x86/include/asm/switch_to.h   | 137 ++++++-------------------------------
 arch/x86/include/asm/thread_info.h |   2 -
 arch/x86/kernel/asm-offsets.c      |   6 ++
 arch/x86/kernel/asm-offsets_32.c   |   5 ++
 arch/x86/kernel/asm-offsets_64.c   |   5 ++
 arch/x86/kernel/process_32.c       |   8 ++-
 arch/x86/kernel/process_64.c       |   7 +-
 arch/x86/kernel/smpboot.c          |   1 -
 11 files changed, 124 insertions(+), 130 deletions(-)

diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
index ee6fea0..05e5340 100644
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -204,6 +204,44 @@
 	POP_GS_EX
 .endm
 
+/*
+ * %eax: prev task
+ * %edx: next task
+ */
+ENTRY(__switch_to_asm)
+	/*
+	 * Save callee-saved registers
+	 * This must match the order in struct fork_frame
+	 * Frame pointer must be last for get_wchan
+	 */
+	pushl	%ebx
+	pushl	%edi
+	pushl	%esi
+	pushl	%ebp
+
+	/* switch stack */
+	movl	%esp, TASK_threadsp(%eax)
+	movl	TASK_threadsp(%edx), %esp
+
+#ifdef CONFIG_CC_STACKPROTECTOR
+	movl	TASK_stack_canary(%edx), %ebx
+	movl	%ebx, PER_CPU_VAR(stack_canary)+stack_canary_offset
+#endif
+
+	/* restore callee-saved registers */
+	popl	%ebp
+	popl	%esi
+	popl	%edi
+	popl	%ebx
+
+	jmp	__switch_to
+END(__switch_to_asm)
+
+/*
+ * A newly forked process directly context switches into this address.
+ *
+ * eax: prev task we switched from
+ */
 ENTRY(ret_from_fork)
 	pushl	%eax
 	call	schedule_tail
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index ab9f8c8..0542ad1 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -365,13 +365,49 @@ END(ptregs_\func)
 #include <asm/syscalls_64.h>
 
 /*
+ * %rdi: prev task
+ * %rsi: next task
+ */
+ENTRY(__switch_to_asm)
+	/*
+	 * Save callee-saved registers
+	 * This must match the order in struct fork_frame
+	 * Frame pointer must be last for get_wchan
+	 */
+	pushq	%rbx
+	pushq	%r12
+	pushq	%r13
+	pushq	%r14
+	pushq	%r15
+	pushq	%rbp
+
+	/* switch stack */
+	movq	%rsp, TASK_threadsp(%rdi)
+	movq	TASK_threadsp(%rsi), %rsp
+
+#ifdef CONFIG_CC_STACKPROTECTOR
+	movq	TASK_stack_canary(%rsi), %rbx
+	movq	%rbx, PER_CPU_VAR(irq_stack_union)+stack_canary_offset
+#endif
+
+	/* restore callee-saved registers */
+	popq	%rbp
+	popq	%r15
+	popq	%r14
+	popq	%r13
+	popq	%r12
+	popq	%rbx
+
+	jmp	__switch_to
+END(__switch_to_asm)
+
+/*
  * A newly forked process directly context switches into this address.
  *
- * rdi: prev task we switched from
+ * rax: prev task we switched from
  */
 ENTRY(ret_from_fork)
-	LOCK ; btr $TIF_FORK, TI_flags(%r8)
-
+	movq	%rax, %rdi
 	call	schedule_tail			/* rdi: 'prev' task parameter */
 
 	testb	$3, CS(%rsp)			/* from kernel_thread? */
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 62c6cc3..d3c2598 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -384,9 +384,6 @@ struct thread_struct {
 	unsigned short		fsindex;
 	unsigned short		gsindex;
 #endif
-#ifdef CONFIG_X86_32
-	unsigned long		ip;
-#endif
 #ifdef CONFIG_X86_64
 	unsigned long		fsbase;
 	unsigned long		gsbase;
diff --git a/arch/x86/include/asm/switch_to.h b/arch/x86/include/asm/switch_to.h
index 8f321a1..b6c9e0c 100644
--- a/arch/x86/include/asm/switch_to.h
+++ b/arch/x86/include/asm/switch_to.h
@@ -2,130 +2,35 @@
 #define _ASM_X86_SWITCH_TO_H
 
 struct task_struct; /* one of the stranger aspects of C forward declarations */
+
+struct task_struct *__switch_to_asm(struct task_struct *prev,
+				    struct task_struct *next);
+
 __visible struct task_struct *__switch_to(struct task_struct *prev,
-					   struct task_struct *next);
+					  struct task_struct *next);
 struct tss_struct;
 void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
 		      struct tss_struct *tss);
 
-#ifdef CONFIG_X86_32
-
-#ifdef CONFIG_CC_STACKPROTECTOR
-#define __switch_canary							\
-	"movl %P[task_canary](%[next]), %%ebx\n\t"			\
-	"movl %%ebx, "__percpu_arg([stack_canary])"\n\t"
-#define __switch_canary_oparam						\
-	, [stack_canary] "=m" (stack_canary.canary)
-#define __switch_canary_iparam						\
-	, [task_canary] "i" (offsetof(struct task_struct, stack_canary))
-#else	/* CC_STACKPROTECTOR */
-#define __switch_canary
-#define __switch_canary_oparam
-#define __switch_canary_iparam
-#endif	/* CC_STACKPROTECTOR */
+struct fork_frame {
+	unsigned long bp;
+#ifdef CONFIG_X86_64
+	unsigned long r15;
+	unsigned long r14;
+	unsigned long r13;
+	unsigned long r12;
+#else
+	unsigned long si;
+	unsigned long di;
+#endif
+	unsigned long bx;
+	unsigned long ret_addr;
+	struct pt_regs regs;
+};
 
-/*
- * Saving eflags is important. It switches not only IOPL between tasks,
- * it also protects other tasks from NT leaking through sysenter etc.
- */
 #define switch_to(prev, next, last)					\
 do {									\
-	/*								\
-	 * Context-switching clobbers all registers, so we clobber	\
-	 * them explicitly, via unused output variables.		\
-	 * (EAX and EBP is not listed because EBP is saved/restored	\
-	 * explicitly for wchan access and EAX is the return value of	\
-	 * __switch_to())						\
-	 */								\
-	unsigned long ebx, ecx, edx, esi, edi;				\
-									\
-	asm volatile("pushl %%ebp\n\t"		/* save    EBP   */	\
-		     "movl %%esp,%[prev_sp]\n\t"	/* save    ESP   */ \
-		     "movl %[next_sp],%%esp\n\t"	/* restore ESP   */ \
-		     "movl $1f,%[prev_ip]\n\t"	/* save    EIP   */	\
-		     "pushl %[next_ip]\n\t"	/* restore EIP   */	\
-		     __switch_canary					\
-		     "jmp __switch_to\n"	/* regparm call  */	\
-		     "1:\t"						\
-		     "popl %%ebp\n\t"		/* restore EBP   */	\
-									\
-		     /* output parameters */				\
-		     : [prev_sp] "=m" (prev->thread.sp),		\
-		       [prev_ip] "=m" (prev->thread.ip),		\
-		       "=a" (last),					\
-									\
-		       /* clobbered output registers: */		\
-		       "=b" (ebx), "=c" (ecx), "=d" (edx),		\
-		       "=S" (esi), "=D" (edi)				\
-		       							\
-		       __switch_canary_oparam				\
-									\
-		       /* input parameters: */				\
-		     : [next_sp]  "m" (next->thread.sp),		\
-		       [next_ip]  "m" (next->thread.ip),		\
-		       							\
-		       /* regparm parameters for __switch_to(): */	\
-		       [prev]     "a" (prev),				\
-		       [next]     "d" (next)				\
-									\
-		       __switch_canary_iparam				\
-									\
-		     : /* reloaded segment registers */			\
-			"memory");					\
+	((last) = __switch_to_asm((prev), (next)));			\
 } while (0)
 
-#else /* CONFIG_X86_32 */
-
-/* frame pointer must be last for get_wchan */
-#define SAVE_CONTEXT    "pushq %%rbp ; movq %%rsi,%%rbp\n\t"
-#define RESTORE_CONTEXT "movq %%rbp,%%rsi ; popq %%rbp\t"
-
-#define __EXTRA_CLOBBER  \
-	, "rcx", "rbx", "rdx", "r8", "r9", "r10", "r11", \
-	  "r12", "r13", "r14", "r15", "flags"
-
-#ifdef CONFIG_CC_STACKPROTECTOR
-#define __switch_canary							  \
-	"movq %P[task_canary](%%rsi),%%r8\n\t"				  \
-	"movq %%r8,"__percpu_arg([gs_canary])"\n\t"
-#define __switch_canary_oparam						  \
-	, [gs_canary] "=m" (irq_stack_union.stack_canary)
-#define __switch_canary_iparam						  \
-	, [task_canary] "i" (offsetof(struct task_struct, stack_canary))
-#else	/* CC_STACKPROTECTOR */
-#define __switch_canary
-#define __switch_canary_oparam
-#define __switch_canary_iparam
-#endif	/* CC_STACKPROTECTOR */
-
-/*
- * There is no need to save or restore flags, because flags are always
- * clean in kernel mode, with the possible exception of IOPL.  Kernel IOPL
- * has no effect.
- */
-#define switch_to(prev, next, last) \
-	asm volatile(SAVE_CONTEXT					  \
-	     "movq %%rsp,%P[threadrsp](%[prev])\n\t" /* save RSP */	  \
-	     "movq %P[threadrsp](%[next]),%%rsp\n\t" /* restore RSP */	  \
-	     "call __switch_to\n\t"					  \
-	     "movq "__percpu_arg([current_task])",%%rsi\n\t"		  \
-	     __switch_canary						  \
-	     "movq %P[thread_info](%%rsi),%%r8\n\t"			  \
-	     "movq %%rax,%%rdi\n\t" 					  \
-	     "testl  %[_tif_fork],%P[ti_flags](%%r8)\n\t"		  \
-	     "jnz   ret_from_fork\n\t"					  \
-	     RESTORE_CONTEXT						  \
-	     : "=a" (last)					  	  \
-	       __switch_canary_oparam					  \
-	     : [next] "S" (next), [prev] "D" (prev),			  \
-	       [threadrsp] "i" (offsetof(struct task_struct, thread.sp)), \
-	       [ti_flags] "i" (offsetof(struct thread_info, flags)),	  \
-	       [_tif_fork] "i" (_TIF_FORK),			  	  \
-	       [thread_info] "i" (offsetof(struct task_struct, stack)),   \
-	       [current_task] "m" (current_task)			  \
-	       __switch_canary_iparam					  \
-	     : "memory", "cc" __EXTRA_CLOBBER)
-
-#endif /* CONFIG_X86_32 */
-
 #endif /* _ASM_X86_SWITCH_TO_H */
diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index 30c133a..20d56ec 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -99,7 +99,6 @@ struct thread_info {
 #define TIF_UPROBE		12	/* breakpointed or singlestepping */
 #define TIF_NOTSC		16	/* TSC is not accessible in userland */
 #define TIF_IA32		17	/* IA32 compatibility process */
-#define TIF_FORK		18	/* ret_from_fork */
 #define TIF_NOHZ		19	/* in adaptive nohz mode */
 #define TIF_MEMDIE		20	/* is terminating due to OOM killer */
 #define TIF_POLLING_NRFLAG	21	/* idle is polling for TIF_NEED_RESCHED */
@@ -123,7 +122,6 @@ struct thread_info {
 #define _TIF_UPROBE		(1 << TIF_UPROBE)
 #define _TIF_NOTSC		(1 << TIF_NOTSC)
 #define _TIF_IA32		(1 << TIF_IA32)
-#define _TIF_FORK		(1 << TIF_FORK)
 #define _TIF_NOHZ		(1 << TIF_NOHZ)
 #define _TIF_POLLING_NRFLAG	(1 << TIF_POLLING_NRFLAG)
 #define _TIF_IO_BITMAP		(1 << TIF_IO_BITMAP)
diff --git a/arch/x86/kernel/asm-offsets.c b/arch/x86/kernel/asm-offsets.c
index 674134e..ec41c79 100644
--- a/arch/x86/kernel/asm-offsets.c
+++ b/arch/x86/kernel/asm-offsets.c
@@ -29,6 +29,12 @@
 
 void common(void) {
 	BLANK();
+	OFFSET(TASK_threadsp, task_struct, thread.sp);
+#ifdef CONFIG_CC_STACKPROTECTOR
+	OFFSET(TASK_stack_canary, task_struct, stack_canary);
+#endif
+
+	BLANK();
 	OFFSET(TI_flags, thread_info, flags);
 	OFFSET(TI_status, thread_info, status);
 	OFFSET(TI_addr_limit, thread_info, addr_limit);
diff --git a/arch/x86/kernel/asm-offsets_32.c b/arch/x86/kernel/asm-offsets_32.c
index ecdc1d2..880aa09 100644
--- a/arch/x86/kernel/asm-offsets_32.c
+++ b/arch/x86/kernel/asm-offsets_32.c
@@ -57,6 +57,11 @@ void foo(void)
 	/* Size of SYSENTER_stack */
 	DEFINE(SIZEOF_SYSENTER_stack, sizeof(((struct tss_struct *)0)->SYSENTER_stack));
 
+#ifdef CONFIG_CC_STACKPROTECTOR
+	BLANK();
+	OFFSET(stack_canary_offset, stack_canary, canary);
+#endif
+
 #if defined(CONFIG_LGUEST) || defined(CONFIG_LGUEST_GUEST) || defined(CONFIG_LGUEST_MODULE)
 	BLANK();
 	OFFSET(LGUEST_DATA_irq_enabled, lguest_data, irq_enabled);
diff --git a/arch/x86/kernel/asm-offsets_64.c b/arch/x86/kernel/asm-offsets_64.c
index d875f97..210927e 100644
--- a/arch/x86/kernel/asm-offsets_64.c
+++ b/arch/x86/kernel/asm-offsets_64.c
@@ -56,6 +56,11 @@ int main(void)
 	OFFSET(TSS_sp0, tss_struct, x86_tss.sp0);
 	BLANK();
 
+#ifdef CONFIG_CC_STACKPROTECTOR
+	DEFINE(stack_canary_offset, offsetof(union irq_stack_union, stack_canary));
+	BLANK();
+#endif
+
 	DEFINE(__NR_syscall_max, sizeof(syscalls_64) - 1);
 	DEFINE(NR_syscalls, sizeof(syscalls_64));
 
diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
index 9f95091..0ba6fdf 100644
--- a/arch/x86/kernel/process_32.c
+++ b/arch/x86/kernel/process_32.c
@@ -133,17 +133,19 @@ int copy_thread_tls(unsigned long clone_flags, unsigned long sp,
 	unsigned long arg, struct task_struct *p, unsigned long tls)
 {
 	struct pt_regs *childregs = task_pt_regs(p);
+	struct fork_frame *frame = container_of(childregs, struct fork_frame, regs);
 	struct task_struct *tsk;
 	int err;
 
-	p->thread.sp = (unsigned long) childregs;
+	frame->bp = 0;
+	p->thread.sp = (unsigned long) frame;
 	p->thread.sp0 = (unsigned long) (childregs+1);
 	memset(p->thread.ptrace_bps, 0, sizeof(p->thread.ptrace_bps));
 
 	if (unlikely(p->flags & PF_KTHREAD)) {
 		/* kernel thread */
 		memset(childregs, 0, sizeof(struct pt_regs));
-		p->thread.ip = (unsigned long) ret_from_kernel_thread;
+		frame->ret_addr = (unsigned long) ret_from_kernel_thread;
 		task_user_gs(p) = __KERNEL_STACK_CANARY;
 		childregs->ds = __USER_DS;
 		childregs->es = __USER_DS;
@@ -161,7 +163,7 @@ int copy_thread_tls(unsigned long clone_flags, unsigned long sp,
 	if (sp)
 		childregs->sp = sp;
 
-	p->thread.ip = (unsigned long) ret_from_fork;
+	frame->ret_addr = (unsigned long) ret_from_fork;
 	task_user_gs(p) = get_user_gs(current_pt_regs());
 
 	p->thread.io_bitmap_ptr = NULL;
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 6e789ca..9fab915 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -142,11 +142,14 @@ int copy_thread_tls(unsigned long clone_flags, unsigned long sp,
 	int err;
 	struct pt_regs *childregs;
 	struct task_struct *me = current;
+	struct fork_frame *frame;
 
 	p->thread.sp0 = (unsigned long)task_stack_page(p) + THREAD_SIZE;
 	childregs = task_pt_regs(p);
-	p->thread.sp = (unsigned long) childregs;
-	set_tsk_thread_flag(p, TIF_FORK);
+	frame = container_of(childregs, struct fork_frame, regs);
+	frame->bp = 0;
+	frame->ret_addr = (unsigned long) ret_from_fork;
+	p->thread.sp = (unsigned long) frame;
 	p->thread.io_bitmap_ptr = NULL;
 
 	savesegment(gs, p->thread.gsindex);
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index fafe8b9..8feb392 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -928,7 +928,6 @@ void common_cpu_up(unsigned int cpu, struct task_struct *idle)
 	per_cpu(cpu_current_top_of_stack, cpu) =
 		(unsigned long)task_stack_page(idle) + THREAD_SIZE;
 #else
-	clear_tsk_thread_flag(idle, TIF_FORK);
 	initial_gs = per_cpu_offset(cpu);
 #endif
 }
-- 
2.5.5

[toc] | [next] | [standalone]

#1405029

FromAndy Lutomirski <luto@amacapital.net>
Date2016-05-22 20:10 +0200
Message-ID<rBFZ8-3Y4-25@gated-at.bofh.it>
In reply to#1404856
cc: Josh Poimboeuf: do you care about the exact stack layout of the
bottom of the stack of an inactive task?

On May 21, 2016 9:05 AM, "Brian Gerst" <brgerst@gmail.com> wrote:
>
> Move the low-level context switch code to an out-of-line asm stub instead of
> using complex inline asm.  This allows constructing a new stack frame for the
> child process to make it seamlessly flow to ret_from_fork without an extra
> test and branch in __switch_to().  It also improves code generation for
> __schedule() by using the C calling convention instead of clobbering all
> registers.

I like the concept a lot.

>
> Signed-off-by: Brian Gerst <brgerst@gmail.com>
> ---
>  arch/x86/entry/entry_32.S          |  38 ++++++++++
>  arch/x86/entry/entry_64.S          |  42 +++++++++++-
>  arch/x86/include/asm/processor.h   |   3 -
>  arch/x86/include/asm/switch_to.h   | 137 ++++++-------------------------------
>  arch/x86/include/asm/thread_info.h |   2 -
>  arch/x86/kernel/asm-offsets.c      |   6 ++
>  arch/x86/kernel/asm-offsets_32.c   |   5 ++
>  arch/x86/kernel/asm-offsets_64.c   |   5 ++
>  arch/x86/kernel/process_32.c       |   8 ++-
>  arch/x86/kernel/process_64.c       |   7 +-
>  arch/x86/kernel/smpboot.c          |   1 -
>  11 files changed, 124 insertions(+), 130 deletions(-)
>
> diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
> index ee6fea0..05e5340 100644
> --- a/arch/x86/entry/entry_32.S
> +++ b/arch/x86/entry/entry_32.S
> @@ -204,6 +204,44 @@
>         POP_GS_EX
>  .endm
>
> +/*
> + * %eax: prev task
> + * %edx: next task
> + */
> +ENTRY(__switch_to_asm)
> +       /*
> +        * Save callee-saved registers
> +        * This must match the order in struct fork_frame
> +        * Frame pointer must be last for get_wchan
> +        */
> +       pushl   %ebx
> +       pushl   %edi
> +       pushl   %esi
> +       pushl   %ebp
> +
> +       /* switch stack */
> +       movl    %esp, TASK_threadsp(%eax)
> +       movl    TASK_threadsp(%edx), %esp
> +
> +#ifdef CONFIG_CC_STACKPROTECTOR
> +       movl    TASK_stack_canary(%edx), %ebx
> +       movl    %ebx, PER_CPU_VAR(stack_canary)+stack_canary_offset
> +#endif
> +
> +       /* restore callee-saved registers */
> +       popl    %ebp
> +       popl    %esi
> +       popl    %edi
> +       popl    %ebx

This is highly, highly magical.  eax and edx are prev and next, and:

> +
> +       jmp     __switch_to

leaves prev in eax.  This works, but it might be worth a comment.

> +END(__switch_to_asm)

>  /*
> + * %rdi: prev task
> + * %rsi: next task
> + */
> +ENTRY(__switch_to_asm)
> +       /*
> +        * Save callee-saved registers
> +        * This must match the order in struct fork_frame
> +        * Frame pointer must be last for get_wchan
> +        */
> +       pushq   %rbx
> +       pushq   %r12
> +       pushq   %r13
> +       pushq   %r14
> +       pushq   %r15
> +       pushq   %rbp
> +
> +       /* switch stack */
> +       movq    %rsp, TASK_threadsp(%rdi)
> +       movq    TASK_threadsp(%rsi), %rsp
> +
> +#ifdef CONFIG_CC_STACKPROTECTOR
> +       movq    TASK_stack_canary(%rsi), %rbx
> +       movq    %rbx, PER_CPU_VAR(irq_stack_union)+stack_canary_offset
> +#endif
> +
> +       /* restore callee-saved registers */
> +       popq    %rbp
> +       popq    %r15
> +       popq    %r14
> +       popq    %r13
> +       popq    %r12
> +       popq    %rbx
> +
> +       jmp     __switch_to

Ditto with the magic here.

> +struct fork_frame {
> +       unsigned long bp;
> +#ifdef CONFIG_X86_64
> +       unsigned long r15;
> +       unsigned long r14;
> +       unsigned long r13;
> +       unsigned long r12;
> +#else
> +       unsigned long si;
> +       unsigned long di;
> +#endif
> +       unsigned long bx;
> +       unsigned long ret_addr;
> +       struct pt_regs regs;
> +};

This, like the old implementation, is very much geared to the current
implementation of fork.  Can you split it up:

struct inactive_task_frame {
    unsigned long bp;
    ...
    unsigned long ret_addr;
};

/* fork works by setting up the child stack so that switch_to will
land at ret_from_fork with sp pointing at pt_regs */
struct fork_frame {
    struct inactive_task_frame switch_frame;
    struct pt_regs regs;
};

Then, if and when someone wants to fork into a different type of
context, they can reuse this.  Also, a future improved unwinder can
use inactive_task_frame directly to kick off its unwind.

--Andy

[toc] | [prev] | [next] | [standalone]

#1405036

FromBrian Gerst <brgerst@gmail.com>
Date2016-05-22 21:40 +0200
Message-ID<rBHod-4Fo-1@gated-at.bofh.it>
In reply to#1405029
On Sun, May 22, 2016 at 1:59 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> cc: Josh Poimboeuf: do you care about the exact stack layout of the
> bottom of the stack of an inactive task?
>
> On May 21, 2016 9:05 AM, "Brian Gerst" <brgerst@gmail.com> wrote:
>>
>> Move the low-level context switch code to an out-of-line asm stub instead of
>> using complex inline asm.  This allows constructing a new stack frame for the
>> child process to make it seamlessly flow to ret_from_fork without an extra
>> test and branch in __switch_to().  It also improves code generation for
>> __schedule() by using the C calling convention instead of clobbering all
>> registers.
>
> I like the concept a lot.
>
>>
>> Signed-off-by: Brian Gerst <brgerst@gmail.com>
>> ---
>>  arch/x86/entry/entry_32.S          |  38 ++++++++++
>>  arch/x86/entry/entry_64.S          |  42 +++++++++++-
>>  arch/x86/include/asm/processor.h   |   3 -
>>  arch/x86/include/asm/switch_to.h   | 137 ++++++-------------------------------
>>  arch/x86/include/asm/thread_info.h |   2 -
>>  arch/x86/kernel/asm-offsets.c      |   6 ++
>>  arch/x86/kernel/asm-offsets_32.c   |   5 ++
>>  arch/x86/kernel/asm-offsets_64.c   |   5 ++
>>  arch/x86/kernel/process_32.c       |   8 ++-
>>  arch/x86/kernel/process_64.c       |   7 +-
>>  arch/x86/kernel/smpboot.c          |   1 -
>>  11 files changed, 124 insertions(+), 130 deletions(-)
>>
>> diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
>> index ee6fea0..05e5340 100644
>> --- a/arch/x86/entry/entry_32.S
>> +++ b/arch/x86/entry/entry_32.S
>> @@ -204,6 +204,44 @@
>>         POP_GS_EX
>>  .endm
>>
>> +/*
>> + * %eax: prev task
>> + * %edx: next task
>> + */
>> +ENTRY(__switch_to_asm)
>> +       /*
>> +        * Save callee-saved registers
>> +        * This must match the order in struct fork_frame
>> +        * Frame pointer must be last for get_wchan
>> +        */
>> +       pushl   %ebx
>> +       pushl   %edi
>> +       pushl   %esi
>> +       pushl   %ebp
>> +
>> +       /* switch stack */
>> +       movl    %esp, TASK_threadsp(%eax)
>> +       movl    TASK_threadsp(%edx), %esp
>> +
>> +#ifdef CONFIG_CC_STACKPROTECTOR
>> +       movl    TASK_stack_canary(%edx), %ebx
>> +       movl    %ebx, PER_CPU_VAR(stack_canary)+stack_canary_offset
>> +#endif
>> +
>> +       /* restore callee-saved registers */
>> +       popl    %ebp
>> +       popl    %esi
>> +       popl    %edi
>> +       popl    %ebx
>
> This is highly, highly magical.  eax and edx are prev and next, and:

What is so magical about the standard C calling convention (regarm(3)
in the 32-bit case)?  This just passes them right though to
__switch_to().

>> +
>> +       jmp     __switch_to
>
> leaves prev in eax.  This works, but it might be worth a comment.

Not quite, __switch_to() returns 'last', not 'prev'.  The previous
task when this is called is not the same task when the thread  wakes
up.

>> +END(__switch_to_asm)
>
>>  /*
>> + * %rdi: prev task
>> + * %rsi: next task
>> + */
>> +ENTRY(__switch_to_asm)
>> +       /*
>> +        * Save callee-saved registers
>> +        * This must match the order in struct fork_frame
>> +        * Frame pointer must be last for get_wchan
>> +        */
>> +       pushq   %rbx
>> +       pushq   %r12
>> +       pushq   %r13
>> +       pushq   %r14
>> +       pushq   %r15
>> +       pushq   %rbp
>> +
>> +       /* switch stack */
>> +       movq    %rsp, TASK_threadsp(%rdi)
>> +       movq    TASK_threadsp(%rsi), %rsp
>> +
>> +#ifdef CONFIG_CC_STACKPROTECTOR
>> +       movq    TASK_stack_canary(%rsi), %rbx
>> +       movq    %rbx, PER_CPU_VAR(irq_stack_union)+stack_canary_offset
>> +#endif
>> +
>> +       /* restore callee-saved registers */
>> +       popq    %rbp
>> +       popq    %r15
>> +       popq    %r14
>> +       popq    %r13
>> +       popq    %r12
>> +       popq    %rbx
>> +
>> +       jmp     __switch_to
>
> Ditto with the magic here.
>
>> +struct fork_frame {
>> +       unsigned long bp;
>> +#ifdef CONFIG_X86_64
>> +       unsigned long r15;
>> +       unsigned long r14;
>> +       unsigned long r13;
>> +       unsigned long r12;
>> +#else
>> +       unsigned long si;
>> +       unsigned long di;
>> +#endif
>> +       unsigned long bx;
>> +       unsigned long ret_addr;
>> +       struct pt_regs regs;
>> +};
>
> This, like the old implementation, is very much geared to the current
> implementation of fork.  Can you split it up:
>
> struct inactive_task_frame {
>     unsigned long bp;
>     ...
>     unsigned long ret_addr;
> };
>
> /* fork works by setting up the child stack so that switch_to will
> land at ret_from_fork with sp pointing at pt_regs */
> struct fork_frame {
>     struct inactive_task_frame switch_frame;
>     struct pt_regs regs;
> };
>
> Then, if and when someone wants to fork into a different type of
> context, they can reuse this.  Also, a future improved unwinder can
> use inactive_task_frame directly to kick off its unwind.

Sounds reasonable.

--
Brian Gerst

[toc] | [prev] | [next] | [standalone]

#1405046

FromAndy Lutomirski <luto@amacapital.net>
Date2016-05-22 23:10 +0200
Message-ID<rBINj-5B7-3@gated-at.bofh.it>
In reply to#1405036
On Sun, May 22, 2016 at 12:31 PM, Brian Gerst <brgerst@gmail.com> wrote:
> On Sun, May 22, 2016 at 1:59 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>> cc: Josh Poimboeuf: do you care about the exact stack layout of the
>> bottom of the stack of an inactive task?
>>
>> On May 21, 2016 9:05 AM, "Brian Gerst" <brgerst@gmail.com> wrote:
>>>
>>> Move the low-level context switch code to an out-of-line asm stub instead of
>>> using complex inline asm.  This allows constructing a new stack frame for the
>>> child process to make it seamlessly flow to ret_from_fork without an extra
>>> test and branch in __switch_to().  It also improves code generation for
>>> __schedule() by using the C calling convention instead of clobbering all
>>> registers.
>>
>> I like the concept a lot.
>>
>>>
>>> Signed-off-by: Brian Gerst <brgerst@gmail.com>
>>> ---
>>>  arch/x86/entry/entry_32.S          |  38 ++++++++++
>>>  arch/x86/entry/entry_64.S          |  42 +++++++++++-
>>>  arch/x86/include/asm/processor.h   |   3 -
>>>  arch/x86/include/asm/switch_to.h   | 137 ++++++-------------------------------
>>>  arch/x86/include/asm/thread_info.h |   2 -
>>>  arch/x86/kernel/asm-offsets.c      |   6 ++
>>>  arch/x86/kernel/asm-offsets_32.c   |   5 ++
>>>  arch/x86/kernel/asm-offsets_64.c   |   5 ++
>>>  arch/x86/kernel/process_32.c       |   8 ++-
>>>  arch/x86/kernel/process_64.c       |   7 +-
>>>  arch/x86/kernel/smpboot.c          |   1 -
>>>  11 files changed, 124 insertions(+), 130 deletions(-)
>>>
>>> diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
>>> index ee6fea0..05e5340 100644
>>> --- a/arch/x86/entry/entry_32.S
>>> +++ b/arch/x86/entry/entry_32.S
>>> @@ -204,6 +204,44 @@
>>>         POP_GS_EX
>>>  .endm
>>>
>>> +/*
>>> + * %eax: prev task
>>> + * %edx: next task
>>> + */
>>> +ENTRY(__switch_to_asm)
>>> +       /*
>>> +        * Save callee-saved registers
>>> +        * This must match the order in struct fork_frame
>>> +        * Frame pointer must be last for get_wchan
>>> +        */
>>> +       pushl   %ebx
>>> +       pushl   %edi
>>> +       pushl   %esi
>>> +       pushl   %ebp
>>> +
>>> +       /* switch stack */
>>> +       movl    %esp, TASK_threadsp(%eax)
>>> +       movl    TASK_threadsp(%edx), %esp
>>> +
>>> +#ifdef CONFIG_CC_STACKPROTECTOR
>>> +       movl    TASK_stack_canary(%edx), %ebx
>>> +       movl    %ebx, PER_CPU_VAR(stack_canary)+stack_canary_offset
>>> +#endif
>>> +
>>> +       /* restore callee-saved registers */
>>> +       popl    %ebp
>>> +       popl    %esi
>>> +       popl    %edi
>>> +       popl    %ebx
>>
>> This is highly, highly magical.  eax and edx are prev and next, and:
>
> What is so magical about the standard C calling convention (regarm(3)
> in the 32-bit case)?  This just passes them right though to
> __switch_to().
>

I guess it's not highly magical, just a bit different from what I
expected.  I guess it's okay.

--Andy

>>> +
>>> +       jmp     __switch_to
>>
>> leaves prev in eax.  This works, but it might be worth a comment.
>
> Not quite, __switch_to() returns 'last', not 'prev'.  The previous
> task when this is called is not the same task when the thread  wakes
> up.

Right.

I wish switch_to were a normal function instead of a silly macro.

--Andy

[toc] | [prev] | [next] | [standalone]

#1405075

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2016-05-23 04:40 +0200
Message-ID<rBNWG-9l-7@gated-at.bofh.it>
In reply to#1405029
On Sun, May 22, 2016 at 10:59:38AM -0700, Andy Lutomirski wrote:
> cc: Josh Poimboeuf: do you care about the exact stack layout of the
> bottom of the stack of an inactive task?

So there's one minor issue with this patch, relating to unwinding the
stack of a newly forked task.  For detecting reliable stacks, the
unwinder needs to unwind all the way to the syscall pt_regs to make sure
the stack is sane.  But for newly forked tasks, that won't be possible
here because the unwinding will stop at the fork_frame instead.

So from an unwinder standpoint it might be nice for copy_thread_tls() to
place a frame pointer on the stack next to the ret_from_fork return
address, so that it would resemble an actual stack frame.  The frame
pointer could probably just be hard-coded to zero.  And then the first
bp in fork_frame would need to be a pointer to it instead of zero.  That
would make it nicely resemble the stack of any other task.

Alternatively I could teach the unwinder that if the unwinding starts at
the fork_frame offset from the end of the stack page, and the saved rbp
is zero, it can assume that it's a newly forked task.  But that seems a
little more brittle to me, as it requires the unwinder to understand
more of the internal workings of the fork code.

But overall I think this patch is a really nice cleanup, and other than
the above minor issue it should be fine with my reliable unwinder, since
rbp is still at the top of the stack.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1405092

FromAndy Lutomirski <luto@amacapital.net>
Date2016-05-23 06:50 +0200
Message-ID<rBPYu-1mf-23@gated-at.bofh.it>
In reply to#1405075
On Sun, May 22, 2016 at 7:34 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Sun, May 22, 2016 at 10:59:38AM -0700, Andy Lutomirski wrote:
>> cc: Josh Poimboeuf: do you care about the exact stack layout of the
>> bottom of the stack of an inactive task?
>
> So there's one minor issue with this patch, relating to unwinding the
> stack of a newly forked task.  For detecting reliable stacks, the
> unwinder needs to unwind all the way to the syscall pt_regs to make sure
> the stack is sane.  But for newly forked tasks, that won't be possible
> here because the unwinding will stop at the fork_frame instead.
>
> So from an unwinder standpoint it might be nice for copy_thread_tls() to
> place a frame pointer on the stack next to the ret_from_fork return
> address, so that it would resemble an actual stack frame.  The frame
> pointer could probably just be hard-coded to zero.  And then the first
> bp in fork_frame would need to be a pointer to it instead of zero.  That
> would make it nicely resemble the stack of any other task.
>
> Alternatively I could teach the unwinder that if the unwinding starts at
> the fork_frame offset from the end of the stack page, and the saved rbp
> is zero, it can assume that it's a newly forked task.  But that seems a
> little more brittle to me, as it requires the unwinder to understand
> more of the internal workings of the fork code.
>
> But overall I think this patch is a really nice cleanup, and other than
> the above minor issue it should be fine with my reliable unwinder, since
> rbp is still at the top of the stack.

Is this a regression or is there some reason that it works right
without the patch?

In any event, whatever we settle on for general pt_regs unwinding
should work for this, too.

>
> --
> Josh



-- 
Andy Lutomirski
AMA Capital Management, LLC

[toc] | [prev] | [next] | [standalone]

#1405290

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2016-05-23 13:50 +0200
Message-ID<rBWwV-5gE-13@gated-at.bofh.it>
In reply to#1405092
On Sun, May 22, 2016 at 09:47:22PM -0700, Andy Lutomirski wrote:
> On Sun, May 22, 2016 at 7:34 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > On Sun, May 22, 2016 at 10:59:38AM -0700, Andy Lutomirski wrote:
> >> cc: Josh Poimboeuf: do you care about the exact stack layout of the
> >> bottom of the stack of an inactive task?
> >
> > So there's one minor issue with this patch, relating to unwinding the
> > stack of a newly forked task.  For detecting reliable stacks, the
> > unwinder needs to unwind all the way to the syscall pt_regs to make sure
> > the stack is sane.  But for newly forked tasks, that won't be possible
> > here because the unwinding will stop at the fork_frame instead.
> >
> > So from an unwinder standpoint it might be nice for copy_thread_tls() to
> > place a frame pointer on the stack next to the ret_from_fork return
> > address, so that it would resemble an actual stack frame.  The frame
> > pointer could probably just be hard-coded to zero.  And then the first
> > bp in fork_frame would need to be a pointer to it instead of zero.  That
> > would make it nicely resemble the stack of any other task.
> >
> > Alternatively I could teach the unwinder that if the unwinding starts at
> > the fork_frame offset from the end of the stack page, and the saved rbp
> > is zero, it can assume that it's a newly forked task.  But that seems a
> > little more brittle to me, as it requires the unwinder to understand
> > more of the internal workings of the fork code.
> >
> > But overall I think this patch is a really nice cleanup, and other than
> > the above minor issue it should be fine with my reliable unwinder, since
> > rbp is still at the top of the stack.
> 
> Is this a regression or is there some reason that it works right
> without the patch?

Without the patch, it uses TIF_FORK to determine the stack is empty.

> In any event, whatever we settle on for general pt_regs unwinding
> should work for this, too.

Yeah, agreed.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1405293

FromBrian Gerst <brgerst@gmail.com>
Date2016-05-23 13:50 +0200
Message-ID<rBWwW-5gE-21@gated-at.bofh.it>
In reply to#1405290
On Mon, May 23, 2016 at 7:40 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Sun, May 22, 2016 at 09:47:22PM -0700, Andy Lutomirski wrote:
>> On Sun, May 22, 2016 at 7:34 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> > On Sun, May 22, 2016 at 10:59:38AM -0700, Andy Lutomirski wrote:
>> >> cc: Josh Poimboeuf: do you care about the exact stack layout of the
>> >> bottom of the stack of an inactive task?
>> >
>> > So there's one minor issue with this patch, relating to unwinding the
>> > stack of a newly forked task.  For detecting reliable stacks, the
>> > unwinder needs to unwind all the way to the syscall pt_regs to make sure
>> > the stack is sane.  But for newly forked tasks, that won't be possible
>> > here because the unwinding will stop at the fork_frame instead.
>> >
>> > So from an unwinder standpoint it might be nice for copy_thread_tls() to
>> > place a frame pointer on the stack next to the ret_from_fork return
>> > address, so that it would resemble an actual stack frame.  The frame
>> > pointer could probably just be hard-coded to zero.  And then the first
>> > bp in fork_frame would need to be a pointer to it instead of zero.  That
>> > would make it nicely resemble the stack of any other task.
>> >
>> > Alternatively I could teach the unwinder that if the unwinding starts at
>> > the fork_frame offset from the end of the stack page, and the saved rbp
>> > is zero, it can assume that it's a newly forked task.  But that seems a
>> > little more brittle to me, as it requires the unwinder to understand
>> > more of the internal workings of the fork code.
>> >
>> > But overall I think this patch is a really nice cleanup, and other than
>> > the above minor issue it should be fine with my reliable unwinder, since
>> > rbp is still at the top of the stack.
>>
>> Is this a regression or is there some reason that it works right
>> without the patch?
>
> Without the patch, it uses TIF_FORK to determine the stack is empty.

Where is this code?  I don't see it in the mainline kernel.

--
Brian Gerst

[toc] | [prev] | [next] | [standalone]

#1405307

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2016-05-23 14:10 +0200
Message-ID<rBWQi-5Ct-11@gated-at.bofh.it>
In reply to#1405293
On Mon, May 23, 2016 at 07:49:37AM -0400, Brian Gerst wrote:
> On Mon, May 23, 2016 at 7:40 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > On Sun, May 22, 2016 at 09:47:22PM -0700, Andy Lutomirski wrote:
> >> On Sun, May 22, 2016 at 7:34 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >> > On Sun, May 22, 2016 at 10:59:38AM -0700, Andy Lutomirski wrote:
> >> >> cc: Josh Poimboeuf: do you care about the exact stack layout of the
> >> >> bottom of the stack of an inactive task?
> >> >
> >> > So there's one minor issue with this patch, relating to unwinding the
> >> > stack of a newly forked task.  For detecting reliable stacks, the
> >> > unwinder needs to unwind all the way to the syscall pt_regs to make sure
> >> > the stack is sane.  But for newly forked tasks, that won't be possible
> >> > here because the unwinding will stop at the fork_frame instead.
> >> >
> >> > So from an unwinder standpoint it might be nice for copy_thread_tls() to
> >> > place a frame pointer on the stack next to the ret_from_fork return
> >> > address, so that it would resemble an actual stack frame.  The frame
> >> > pointer could probably just be hard-coded to zero.  And then the first
> >> > bp in fork_frame would need to be a pointer to it instead of zero.  That
> >> > would make it nicely resemble the stack of any other task.
> >> >
> >> > Alternatively I could teach the unwinder that if the unwinding starts at
> >> > the fork_frame offset from the end of the stack page, and the saved rbp
> >> > is zero, it can assume that it's a newly forked task.  But that seems a
> >> > little more brittle to me, as it requires the unwinder to understand
> >> > more of the internal workings of the fork code.
> >> >
> >> > But overall I think this patch is a really nice cleanup, and other than
> >> > the above minor issue it should be fine with my reliable unwinder, since
> >> > rbp is still at the top of the stack.
> >>
> >> Is this a regression or is there some reason that it works right
> >> without the patch?
> >
> > Without the patch, it uses TIF_FORK to determine the stack is empty.
> 
> Where is this code?  I don't see it in the mainline kernel.

Yeah, it hasn't been merged.  Here's the last version:

  https://lkml.kernel.org/r/4d34d452bf8f85c7d6d5f93db1d3eeb4cba335c7.1461875890.git.jpoimboe@redhat.com

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1405266

FromBrian Gerst <brgerst@gmail.com>
Date2016-05-23 13:20 +0200
Message-ID<rBW3U-56T-17@gated-at.bofh.it>
In reply to#1405075
On Sun, May 22, 2016 at 10:34 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Sun, May 22, 2016 at 10:59:38AM -0700, Andy Lutomirski wrote:
>> cc: Josh Poimboeuf: do you care about the exact stack layout of the
>> bottom of the stack of an inactive task?
>
> So there's one minor issue with this patch, relating to unwinding the
> stack of a newly forked task.  For detecting reliable stacks, the
> unwinder needs to unwind all the way to the syscall pt_regs to make sure
> the stack is sane.  But for newly forked tasks, that won't be possible
> here because the unwinding will stop at the fork_frame instead.
>
> So from an unwinder standpoint it might be nice for copy_thread_tls() to
> place a frame pointer on the stack next to the ret_from_fork return
> address, so that it would resemble an actual stack frame.  The frame
> pointer could probably just be hard-coded to zero.  And then the first
> bp in fork_frame would need to be a pointer to it instead of zero.  That
> would make it nicely resemble the stack of any other task.
>
> Alternatively I could teach the unwinder that if the unwinding starts at
> the fork_frame offset from the end of the stack page, and the saved rbp
> is zero, it can assume that it's a newly forked task.  But that seems a
> little more brittle to me, as it requires the unwinder to understand
> more of the internal workings of the fork code.
>
> But overall I think this patch is a really nice cleanup, and other than
> the above minor issue it should be fine with my reliable unwinder, since
> rbp is still at the top of the stack.

Ok, how about if it pushed RBP first, then we teach get_wchan() to add
the fixed offset from thread.sp to get bp?  that way it don't have to
push it twice.

--
Brian Gerst

[toc] | [prev] | [next] | [standalone]

#1405288

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2016-05-23 13:50 +0200
Message-ID<rBWwV-5gE-1@gated-at.bofh.it>
In reply to#1405266
On Mon, May 23, 2016 at 06:47:22AM -0500, Josh Poimboeuf wrote:
> On Mon, May 23, 2016 at 07:14:14AM -0400, Brian Gerst wrote:
> > On Sun, May 22, 2016 at 10:34 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > On Sun, May 22, 2016 at 10:59:38AM -0700, Andy Lutomirski wrote:
> > >> cc: Josh Poimboeuf: do you care about the exact stack layout of the
> > >> bottom of the stack of an inactive task?
> > >
> > > So there's one minor issue with this patch, relating to unwinding the
> > > stack of a newly forked task.  For detecting reliable stacks, the
> > > unwinder needs to unwind all the way to the syscall pt_regs to make sure
> > > the stack is sane.  But for newly forked tasks, that won't be possible
> > > here because the unwinding will stop at the fork_frame instead.
> > >
> > > So from an unwinder standpoint it might be nice for copy_thread_tls() to
> > > place a frame pointer on the stack next to the ret_from_fork return
> > > address, so that it would resemble an actual stack frame.  The frame
> > > pointer could probably just be hard-coded to zero.  And then the first
> > > bp in fork_frame would need to be a pointer to it instead of zero.  That
> > > would make it nicely resemble the stack of any other task.
> > >
> > > Alternatively I could teach the unwinder that if the unwinding starts at
> > > the fork_frame offset from the end of the stack page, and the saved rbp
> > > is zero, it can assume that it's a newly forked task.  But that seems a
> > > little more brittle to me, as it requires the unwinder to understand
> > > more of the internal workings of the fork code.
> > >
> > > But overall I think this patch is a really nice cleanup, and other than
> > > the above minor issue it should be fine with my reliable unwinder, since
> > > rbp is still at the top of the stack.
> > 
> > Ok, how about if it pushed RBP first, then we teach get_wchan() to add
> > the fixed offset from thread.sp to get bp?  that way it don't have to
> > push it twice.
> 
> In theory I like the idea, and it would work: the unwinder could just
> use the inactive_task_frame struct (as Andy suggested) to find the frame
> pointer.
> 
> But I suspect it would break all existing unwinders, both in-tree and
> out-of-tree.  The only out-of-tree one I know of is crash, not sure if
> there are more out there.

I should mention it would only affect those unwinders which know how to
do sleeping kernel tasks.  So generic tools like gdb wouldn't be
affected.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1405509

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2016-05-23 18:50 +0200
Message-ID<rC1dg-8iS-13@gated-at.bofh.it>
In reply to#1405288
On Mon, May 23, 2016 at 06:49:03AM -0500, Josh Poimboeuf wrote:
> On Mon, May 23, 2016 at 06:47:22AM -0500, Josh Poimboeuf wrote:
> > On Mon, May 23, 2016 at 07:14:14AM -0400, Brian Gerst wrote:
> > > On Sun, May 22, 2016 at 10:34 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > > On Sun, May 22, 2016 at 10:59:38AM -0700, Andy Lutomirski wrote:
> > > >> cc: Josh Poimboeuf: do you care about the exact stack layout of the
> > > >> bottom of the stack of an inactive task?
> > > >
> > > > So there's one minor issue with this patch, relating to unwinding the
> > > > stack of a newly forked task.  For detecting reliable stacks, the
> > > > unwinder needs to unwind all the way to the syscall pt_regs to make sure
> > > > the stack is sane.  But for newly forked tasks, that won't be possible
> > > > here because the unwinding will stop at the fork_frame instead.
> > > >
> > > > So from an unwinder standpoint it might be nice for copy_thread_tls() to
> > > > place a frame pointer on the stack next to the ret_from_fork return
> > > > address, so that it would resemble an actual stack frame.  The frame
> > > > pointer could probably just be hard-coded to zero.  And then the first
> > > > bp in fork_frame would need to be a pointer to it instead of zero.  That
> > > > would make it nicely resemble the stack of any other task.
> > > >
> > > > Alternatively I could teach the unwinder that if the unwinding starts at
> > > > the fork_frame offset from the end of the stack page, and the saved rbp
> > > > is zero, it can assume that it's a newly forked task.  But that seems a
> > > > little more brittle to me, as it requires the unwinder to understand
> > > > more of the internal workings of the fork code.
> > > >
> > > > But overall I think this patch is a really nice cleanup, and other than
> > > > the above minor issue it should be fine with my reliable unwinder, since
> > > > rbp is still at the top of the stack.
> > > 
> > > Ok, how about if it pushed RBP first, then we teach get_wchan() to add
> > > the fixed offset from thread.sp to get bp?  that way it don't have to
> > > push it twice.
> > 
> > In theory I like the idea, and it would work: the unwinder could just
> > use the inactive_task_frame struct (as Andy suggested) to find the frame
> > pointer.
> > 
> > But I suspect it would break all existing unwinders, both in-tree and
> > out-of-tree.  The only out-of-tree one I know of is crash, not sure if
> > there are more out there.
> 
> I should mention it would only affect those unwinders which know how to
> do sleeping kernel tasks.  So generic tools like gdb wouldn't be
> affected.

[continuing my conversation with myself...]

To clarify, I still think we should do it.  The stack format of a
sleeping task isn't exactly an ABI, and I wouldn't expect many tools to
rely on it.  I can help with the fixing of in-tree unwinders if needed.

Or I could even do the moving of the frame pointer as a separate patch
on top of this one, since it might cause breakage elsewhere.

Adding Dave Anderson (crash maintainer) to cc, as an FYI.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1405521

FromAndy Lutomirski <luto@amacapital.net>
Date2016-05-23 19:10 +0200
Message-ID<rC1wC-d5-17@gated-at.bofh.it>
In reply to#1405509
On Mon, May 23, 2016 at 9:46 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Mon, May 23, 2016 at 06:49:03AM -0500, Josh Poimboeuf wrote:
>> On Mon, May 23, 2016 at 06:47:22AM -0500, Josh Poimboeuf wrote:
>> > On Mon, May 23, 2016 at 07:14:14AM -0400, Brian Gerst wrote:
>> > > On Sun, May 22, 2016 at 10:34 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> > > > On Sun, May 22, 2016 at 10:59:38AM -0700, Andy Lutomirski wrote:
>> > > >> cc: Josh Poimboeuf: do you care about the exact stack layout of the
>> > > >> bottom of the stack of an inactive task?
>> > > >
>> > > > So there's one minor issue with this patch, relating to unwinding the
>> > > > stack of a newly forked task.  For detecting reliable stacks, the
>> > > > unwinder needs to unwind all the way to the syscall pt_regs to make sure
>> > > > the stack is sane.  But for newly forked tasks, that won't be possible
>> > > > here because the unwinding will stop at the fork_frame instead.
>> > > >
>> > > > So from an unwinder standpoint it might be nice for copy_thread_tls() to
>> > > > place a frame pointer on the stack next to the ret_from_fork return
>> > > > address, so that it would resemble an actual stack frame.  The frame
>> > > > pointer could probably just be hard-coded to zero.  And then the first
>> > > > bp in fork_frame would need to be a pointer to it instead of zero.  That
>> > > > would make it nicely resemble the stack of any other task.
>> > > >
>> > > > Alternatively I could teach the unwinder that if the unwinding starts at
>> > > > the fork_frame offset from the end of the stack page, and the saved rbp
>> > > > is zero, it can assume that it's a newly forked task.  But that seems a
>> > > > little more brittle to me, as it requires the unwinder to understand
>> > > > more of the internal workings of the fork code.
>> > > >
>> > > > But overall I think this patch is a really nice cleanup, and other than
>> > > > the above minor issue it should be fine with my reliable unwinder, since
>> > > > rbp is still at the top of the stack.
>> > >
>> > > Ok, how about if it pushed RBP first, then we teach get_wchan() to add
>> > > the fixed offset from thread.sp to get bp?  that way it don't have to
>> > > push it twice.
>> >
>> > In theory I like the idea, and it would work: the unwinder could just
>> > use the inactive_task_frame struct (as Andy suggested) to find the frame
>> > pointer.
>> >
>> > But I suspect it would break all existing unwinders, both in-tree and
>> > out-of-tree.  The only out-of-tree one I know of is crash, not sure if
>> > there are more out there.
>>
>> I should mention it would only affect those unwinders which know how to
>> do sleeping kernel tasks.  So generic tools like gdb wouldn't be
>> affected.
>
> [continuing my conversation with myself...]
>
> To clarify, I still think we should do it.  The stack format of a
> sleeping task isn't exactly an ABI, and I wouldn't expect many tools to
> rely on it.  I can help with the fixing of in-tree unwinders if needed.
>
> Or I could even do the moving of the frame pointer as a separate patch
> on top of this one, since it might cause breakage elsewhere.

Do you have any understanding of why there are so many unwinder
implementations?  Your reliable unwinder seems to be yet another copy
of more or less the same code.

I'd like to see a single, high-quality unwinder implemented as a state
machine, along the lines of:

struct unwind_state state;
unwind_start_inactive_task(&state, ...); or
unwind_start_pt_regs(&state, regs); or whatever.
unwind_next_frame(&state);

where, after unwind_next_frame, state encodes whatever registers are
known (at least bp and ip, but all the GPRs would be nice and are
probably mandatory for DWARF) and an indication of whether this is a
real frame or a guessed frame (the things that currently show up as
'?').

--Andy

[toc] | [prev] | [next] | [standalone]

#1405577

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2016-05-23 20:50 +0200
Message-ID<rC35o-13f-31@gated-at.bofh.it>
In reply to#1405521
On Mon, May 23, 2016 at 10:03:54AM -0700, Andy Lutomirski wrote:
> On Mon, May 23, 2016 at 9:46 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > On Mon, May 23, 2016 at 06:49:03AM -0500, Josh Poimboeuf wrote:
> >> On Mon, May 23, 2016 at 06:47:22AM -0500, Josh Poimboeuf wrote:
> >> > On Mon, May 23, 2016 at 07:14:14AM -0400, Brian Gerst wrote:
> >> > > On Sun, May 22, 2016 at 10:34 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >> > > > On Sun, May 22, 2016 at 10:59:38AM -0700, Andy Lutomirski wrote:
> >> > > >> cc: Josh Poimboeuf: do you care about the exact stack layout of the
> >> > > >> bottom of the stack of an inactive task?
> >> > > >
> >> > > > So there's one minor issue with this patch, relating to unwinding the
> >> > > > stack of a newly forked task.  For detecting reliable stacks, the
> >> > > > unwinder needs to unwind all the way to the syscall pt_regs to make sure
> >> > > > the stack is sane.  But for newly forked tasks, that won't be possible
> >> > > > here because the unwinding will stop at the fork_frame instead.
> >> > > >
> >> > > > So from an unwinder standpoint it might be nice for copy_thread_tls() to
> >> > > > place a frame pointer on the stack next to the ret_from_fork return
> >> > > > address, so that it would resemble an actual stack frame.  The frame
> >> > > > pointer could probably just be hard-coded to zero.  And then the first
> >> > > > bp in fork_frame would need to be a pointer to it instead of zero.  That
> >> > > > would make it nicely resemble the stack of any other task.
> >> > > >
> >> > > > Alternatively I could teach the unwinder that if the unwinding starts at
> >> > > > the fork_frame offset from the end of the stack page, and the saved rbp
> >> > > > is zero, it can assume that it's a newly forked task.  But that seems a
> >> > > > little more brittle to me, as it requires the unwinder to understand
> >> > > > more of the internal workings of the fork code.
> >> > > >
> >> > > > But overall I think this patch is a really nice cleanup, and other than
> >> > > > the above minor issue it should be fine with my reliable unwinder, since
> >> > > > rbp is still at the top of the stack.
> >> > >
> >> > > Ok, how about if it pushed RBP first, then we teach get_wchan() to add
> >> > > the fixed offset from thread.sp to get bp?  that way it don't have to
> >> > > push it twice.
> >> >
> >> > In theory I like the idea, and it would work: the unwinder could just
> >> > use the inactive_task_frame struct (as Andy suggested) to find the frame
> >> > pointer.
> >> >
> >> > But I suspect it would break all existing unwinders, both in-tree and
> >> > out-of-tree.  The only out-of-tree one I know of is crash, not sure if
> >> > there are more out there.
> >>
> >> I should mention it would only affect those unwinders which know how to
> >> do sleeping kernel tasks.  So generic tools like gdb wouldn't be
> >> affected.
> >
> > [continuing my conversation with myself...]
> >
> > To clarify, I still think we should do it.  The stack format of a
> > sleeping task isn't exactly an ABI, and I wouldn't expect many tools to
> > rely on it.  I can help with the fixing of in-tree unwinders if needed.
> >
> > Or I could even do the moving of the frame pointer as a separate patch
> > on top of this one, since it might cause breakage elsewhere.
> 
> Do you have any understanding of why there are so many unwinder
> implementations?  Your reliable unwinder seems to be yet another copy
> of more or less the same code.

Yeah, there are way too many instantations of stacktrace_ops and there's
definitely a lot of room for consolidation and simplification.  There
are different requirements needed by all the different codes relying on
dump_trace():

- starting with a given pt_regs
- starting with a given task
- whether to skip sched code functions
- whether to skip the random '?' ktext addresses found on the stack
- whether frame pointers are enabled
- whether the stack is reliable
- output to an arch-independent "struct stack_trace" array

So everybody implements their own callbacks for dump_trace().  It's kind
of a big mess.

> I'd like to see a single, high-quality unwinder implemented as a state
> machine, along the lines of:
> 
> struct unwind_state state;
> unwind_start_inactive_task(&state, ...); or
> unwind_start_pt_regs(&state, regs); or whatever.
> unwind_next_frame(&state);
> 
> where, after unwind_next_frame, state encodes whatever registers are
> known (at least bp and ip, but all the GPRs would be nice and are
> probably mandatory for DWARF) and an indication of whether this is a
> real frame or a guessed frame (the things that currently show up as
> '?').

I like the idea of a state machine.  I'll probably end up doing
something like that before introducing the DWARF unwinder.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1405296

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2016-05-23 13:50 +0200
Message-ID<rBWwV-5gE-3@gated-at.bofh.it>
In reply to#1405266
On Mon, May 23, 2016 at 07:14:14AM -0400, Brian Gerst wrote:
> On Sun, May 22, 2016 at 10:34 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > On Sun, May 22, 2016 at 10:59:38AM -0700, Andy Lutomirski wrote:
> >> cc: Josh Poimboeuf: do you care about the exact stack layout of the
> >> bottom of the stack of an inactive task?
> >
> > So there's one minor issue with this patch, relating to unwinding the
> > stack of a newly forked task.  For detecting reliable stacks, the
> > unwinder needs to unwind all the way to the syscall pt_regs to make sure
> > the stack is sane.  But for newly forked tasks, that won't be possible
> > here because the unwinding will stop at the fork_frame instead.
> >
> > So from an unwinder standpoint it might be nice for copy_thread_tls() to
> > place a frame pointer on the stack next to the ret_from_fork return
> > address, so that it would resemble an actual stack frame.  The frame
> > pointer could probably just be hard-coded to zero.  And then the first
> > bp in fork_frame would need to be a pointer to it instead of zero.  That
> > would make it nicely resemble the stack of any other task.
> >
> > Alternatively I could teach the unwinder that if the unwinding starts at
> > the fork_frame offset from the end of the stack page, and the saved rbp
> > is zero, it can assume that it's a newly forked task.  But that seems a
> > little more brittle to me, as it requires the unwinder to understand
> > more of the internal workings of the fork code.
> >
> > But overall I think this patch is a really nice cleanup, and other than
> > the above minor issue it should be fine with my reliable unwinder, since
> > rbp is still at the top of the stack.
> 
> Ok, how about if it pushed RBP first, then we teach get_wchan() to add
> the fixed offset from thread.sp to get bp?  that way it don't have to
> push it twice.

In theory I like the idea, and it would work: the unwinder could just
use the inactive_task_frame struct (as Andy suggested) to find the frame
pointer.

But I suspect it would break all existing unwinders, both in-tree and
out-of-tree.  The only out-of-tree one I know of is crash, not sure if
there are more out there.

-- 
Josh

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web