Re: [PATCH v4 2/2] x86/kconfig: make it easier to switch to the new ORC unwinder

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Tue, Jul 25, 2017 at 11:10:59AM +0200, Ingo Molnar wrote:
> 
> * Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> > A couple of Kconfig changes which make it much easier to switch to the
> > new CONFIG_ORC_UNWINDER:
> > 
> > 1) Remove x86 dependencies on CONFIG_FRAME_POINTER for lockdep,
> >    latencytop, and fault injection.  x86 has a 'guess' unwinder which
> >    just scans the stack for kernel text addresses.  It's not 100%
> >    accurate but in many cases it's good enough.  This allows those users
> >    who don't want the text overhead of the frame pointer or ORC
> >    unwinders to still use these features.  More importantly, this also
> >    makes it much more straightforward to disable frame pointers.
> > 
> > 2) Make CONFIG_ORC_UNWINDER depend on !CONFIG_FRAME_POINTER.  While it
> >    would be possible to have both enabled, it doesn't really make sense
> >    to do so.  So enforce a sane configuration to prevent the user from
> >    making a dumb mistake.
> > 
> > With these changes, when you disable CONFIG_FRAME_POINTER, "make
> > oldconfig" will ask if you want to enable CONFIG_ORC_UNWINDER.
> 
> Yeah, so I think this is still suboptimal: the frame pointer and the Orc unwinders 
> are configured in different places, and the user won't know about the various 
> unwinder options unless stumbling across them by accidentally disabling frame 
> pointers ...
> 
> Also, the Kconfig help text for frame pointers is now actively misleading:
> 
>   CONFIG_FRAME_POINTER:
> 
>   If you say Y here the resulting kernel image will be slightly
>   larger and slower, but it gives very useful debugging information
>   in case of kernel bugs. (precise oopses/stacktraces/warnings)
> 
> Please, as I suggested it before, make it a multiple choice option: frame, Orc, or 
> the guess unwinder.
> 
> I'd only offer the 'guess' unwinder if EXPERT is selected, because it's a really 
> sub-optimal selection all things considered.
> 
> Once things are tested and it's all rosy we can change the default x86 unwinder to 
> Orc and organize the naming of the config variables to:
> 
> 	CONFIG_UNWINDER_ORC
> 	CONFIG_UNWINDER_FRAME_POINTER
> 	CONFIG_UNWINDER_GUESS

How about the below patch?  It goes on top of the other two.

----

From: Josh Poimboeuf <jpoimboe@redhat.com>
Subject: [PATCH] x86/kconfig: consolidate unwinders into multiple choice selection

There are three mutually exclusive unwinders.  Make that more obvious by
combining them into a multiple-choice selection:

  CONFIG_FRAME_POINTER_UNWINDER
  CONFIG_ORC_UNWINDER
  CONFIG_GUESS_UNWINDER (if CONFIG_EXPERT=y)

Frame pointers are still the default (for now).

The old CONFIG_FRAME_POINTER option is still used in some
arch-independent places, so keep it around, but make it invisible to the
user on x86.  It's now selected by CONFIG_FRAME_POINTER_UNWINDER.

Suggested-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/Kconfig              |  3 +--
 arch/x86/Kconfig.debug        | 47 ++++++++++++++++++++++++++++++++++++-------
 arch/x86/configs/tiny.config  |  2 ++
 arch/x86/include/asm/unwind.h |  4 ++--
 4 files changed, 45 insertions(+), 11 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 21c75a652bb9..1e789ecefc02 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -73,7 +73,6 @@ config X86
 	select ARCH_USE_QUEUED_RWLOCKS
 	select ARCH_USE_QUEUED_SPINLOCKS
 	select ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH
-	select ARCH_WANT_FRAME_POINTERS
 	select ARCH_WANTS_DYNAMIC_TASK_STRUCT
 	select ARCH_WANTS_THP_SWAP		if X86_64
 	select BUILDTIME_EXTABLE_SORT
@@ -168,7 +167,7 @@ config X86
 	select HAVE_PERF_REGS
 	select HAVE_PERF_USER_STACK_DUMP
 	select HAVE_REGS_AND_STACK_ACCESS_API
-	select HAVE_RELIABLE_STACKTRACE		if X86_64 && FRAME_POINTER && STACK_VALIDATION
+	select HAVE_RELIABLE_STACKTRACE		if X86_64 && FRAME_POINTER_UNWINDER && STACK_VALIDATION
 	select HAVE_STACK_VALIDATION		if X86_64
 	select HAVE_SYSCALL_TRACEPOINTS
 	select HAVE_UNSTABLE_SCHED_CLOCK
diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
index e6176007b838..71a48a30fc84 100644
--- a/arch/x86/Kconfig.debug
+++ b/arch/x86/Kconfig.debug
@@ -356,9 +356,32 @@ config PUNIT_ATOM_DEBUG
 	  The current power state can be read from
 	  /sys/kernel/debug/punit_atom/dev_power_state
 
+choice
+	prompt "Choose kernel unwinder"
+	default FRAME_POINTER_UNWINDER
+	---help---
+	  This determines which method will be used for unwinding kernel stack
+	  traces for panics, oopses, bugs, warnings, perf, /proc/<pid>/stack,
+	  livepatch, lockdep, and more.
+
+config FRAME_POINTER_UNWINDER
+	bool "Frame pointer unwinder"
+	select FRAME_POINTER
+	---help---
+	  This option enables the frame pointer unwinder for unwinding kernel
+	  stack traces.
+
+	  The unwinder itself is fast and it uses less RAM than the ORC
+	  unwinder, but the kernel text size will grow by ~3% and the kernel's
+	  overall performance will degrade by roughly 5-10%.
+
+	  This option is recommended if you want to use the livepatch
+	  consistency model, as this is currently the only way to get a
+	  reliable stack trace (CONFIG_HAVE_RELIABLE_STACKTRACE).
+
 config ORC_UNWINDER
 	bool "ORC unwinder"
-	depends on X86_64 && !FRAME_POINTER
+	depends on X86_64
 	select STACK_VALIDATION
 	---help---
 	  This option enables the ORC (Oops Rewind Capability) unwinder for
@@ -372,12 +395,22 @@ config ORC_UNWINDER
 	  Enabling this option will increase the kernel's runtime memory usage
 	  by roughly 2-4MB, depending on your kernel config.
 
-config FRAME_POINTER_UNWINDER
-	def_bool y
-	depends on !ORC_UNWINDER && FRAME_POINTER
-
 config GUESS_UNWINDER
-	def_bool y
-	depends on !ORC_UNWINDER && !FRAME_POINTER
+	bool "Guess unwinder"
+	depends on EXPERT
+	---help---
+	  This option enables the "guess" unwinder for unwinding kernel stack
+	  traces.  It scans the stack and reports every kernel text address it
+	  finds.  Some of the addresses it reports may be incorrect.
+
+	  While this option often produces false positives, it can still be
+	  useful in many cases.  Unlike the other unwinders, it has no runtime
+	  overhead.
+
+endchoice
+
+config FRAME_POINTER
+	depends on !ORC_UNWINDER && !GUESS_UNWINDER
+	bool
 
 endmenu
diff --git a/arch/x86/configs/tiny.config b/arch/x86/configs/tiny.config
index 4b429df40d7a..550cd5012b73 100644
--- a/arch/x86/configs/tiny.config
+++ b/arch/x86/configs/tiny.config
@@ -1,3 +1,5 @@
 CONFIG_NOHIGHMEM=y
 # CONFIG_HIGHMEM4G is not set
 # CONFIG_HIGHMEM64G is not set
+CONFIG_GUESS_UNWINDER=y
+# CONFIG_FRAME_POINTER_UNWINDER is not set
diff --git a/arch/x86/include/asm/unwind.h b/arch/x86/include/asm/unwind.h
index 25b8d31a007d..e9f793e2df7a 100644
--- a/arch/x86/include/asm/unwind.h
+++ b/arch/x86/include/asm/unwind.h
@@ -16,7 +16,7 @@ struct unwind_state {
 	bool signal, full_regs;
 	unsigned long sp, bp, ip;
 	struct pt_regs *regs;
-#elif defined(CONFIG_FRAME_POINTER)
+#elif defined(CONFIG_FRAME_POINTER_UNWINDER)
 	bool got_irq;
 	unsigned long *bp, *orig_sp, ip;
 	struct pt_regs *regs;
@@ -50,7 +50,7 @@ void unwind_start(struct unwind_state *state, struct task_struct *task,
 	__unwind_start(state, task, regs, first_frame);
 }
 
-#if defined(CONFIG_ORC_UNWINDER) || defined(CONFIG_FRAME_POINTER)
+#if defined(CONFIG_ORC_UNWINDER) || defined(CONFIG_FRAME_POINTER_UNWINDER)
 static inline struct pt_regs *unwind_get_entry_regs(struct unwind_state *state)
 {
 	if (unwind_done(state))
-- 
2.13.3