Re: [RFC PATCH 1/2] lib, stackdepot: Add input prompt for STACKDEPOT option.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 31/8/21 9:25 pm, Vlastimil Babka wrote:

+CC rest of slab maintainers (please use get_maintainers.pl next time)

On 8/31/21 08:25, Imran Khan wrote:
So far CONFIG_STACKDEPOT option was being selected by
features that need STACKDEPOT support for their operations,
[...]

Signed-off-by: Imran Khan <imran.f.khan@xxxxxxxxxx>

Right so you're probably not aware, but switching slub to stackdepot was
just recently attempted by 788691464c29 ("mm/slub: use stackdepot to save
stack trace in objects"), but Linus reverted it in ae14c63a9f20. Enabling
stackdepot unconditionally was one of the reasons as there's memory overhead.

Indeed I was not aware of this earlier attempt. I checked tip of linux-next and saw presence of array in struct track and then went ahead with my attempt.
---
  lib/Kconfig | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/Kconfig b/lib/Kconfig
index 6a6ae5312fa0..7e4b54f48af7 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -665,8 +665,9 @@ config ARCH_STACKWALK
         bool
config STACKDEPOT
-	bool
+	def_bool n
  	select STACKTRACE
+	prompt "Enable stackdepot support"
config STACK_HASH_ORDER
  	int "stack depot hash size (12 => 4KB, 20 => 1024KB)"


I'm not a big fan of a solution that throws another prompt at the person
configuring the kernel, especially if there's nothing to help decide how to
answer it.

Incidentally just yesterday I was trying locally to resolve this in a
different way, where stackdepot would be enabled if both CONFIG_SLUB_DEBUG
and CONFIG_STACKTRACE is enabled. That could be enough, unless there are
systems that would like to have STACKTRACE enabled for other reasons,
SLUB_DEBUG also for other reasons but still not want to pay the memory
overhead of stackdepot. At that point it could be more useful to investigate
if it's possible to optimize stackdepot to not make memblock allocations
unconditionally on init, but normal buddy allocations only when actually used.

My main intention of making STACKDEPOT explicitly configurable was to use it as substitute of STACKTRACE as far as storing user info was concerned. But now I realize this approach was wrong mainly because STACKTRACE does not have any memory overhead of its own whereas STACKDEPOT has memory overhead of it's own. So if someone enables STACKDEPOT but never enables slub_debug=U, STACKDEPOT
memory will remain useless.
Probably the most acceptable way forward in this scenario would be optimize STACKDEPOT memory usage, as you have suggested.
I will do some more digging in this regard.

Thoughts on the below?

----8<----
commit d3582a7c54b26f6fd3a44f1327a26c383e6b951c
Author: Vlastimil Babka <vbabka@xxxxxxx>
Date:   Mon Aug 30 17:26:15 2021 +0200

     Rework stackdepot deps

diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
index 80b57e7f4947..8c25b27014ee 100644
--- a/arch/x86/Kconfig.debug
+++ b/arch/x86/Kconfig.debug
@@ -267,7 +267,6 @@ config UNWINDER_FRAME_POINTER
  config UNWINDER_GUESS
  	bool "Guess unwinder"
  	depends on EXPERT
-	depends on !STACKDEPOT
  	help
  	  This option enables the "guess" unwinder for unwinding kernel stack
  	  traces.  It scans the stack and reports every kernel text address it
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 7ff89690a976..4638c4ece8f2 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -47,7 +47,8 @@ config DRM_DEBUG_MM
  	bool "Insert extra checks and debug info into the DRM range managers"
  	default n
  	depends on DRM=y
-	depends on STACKTRACE_SUPPORT
+	depends on STACKDEPOT_SUPPORT
+	select STACKTRACE
  	select STACKDEPOT
  	help
  	  Enable allocation tracking of memory manager and leak detection on
diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug
index 72a38f28393f..70794c6bb84f 100644
--- a/drivers/gpu/drm/i915/Kconfig.debug
+++ b/drivers/gpu/drm/i915/Kconfig.debug
@@ -21,9 +21,11 @@ config DRM_I915_DEBUG
  	depends on DRM_I915
  	depends on EXPERT # only for developers
  	depends on !COMPILE_TEST # never built by robots
+	depends on STACKDEPOT_SUPPORT
  	select DEBUG_FS
  	select PREEMPT_COUNT
  	select I2C_CHARDEV
+	select STACKTRACE
  	select STACKDEPOT
  	select DRM_DP_AUX_CHARDEV
  	select X86_MSR # used by igt/pm_rpm
diff --git a/init/Kconfig b/init/Kconfig
index 55f9f7738ebb..f9a4ec802516 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1847,6 +1847,7 @@ config SLUB_DEBUG
  	default y
  	bool "Enable SLUB debugging support" if EXPERT
  	depends on SLUB && SYSFS
+	select STACKDEPOT if STACKDEPOT_SUPPORT && STACKTRACE
  	help
  	  SLUB has extensive debug support features. Disabling these can
  	  result in significant savings in code size. This also disables
diff --git a/lib/Kconfig b/lib/Kconfig
index 5c9c0687f76d..67c388a09b7a 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -661,9 +661,37 @@ config ARCH_HAS_COPY_MC
  config ARCH_STACKWALK
         bool
+# encompasses STACKDEPOT dependencies
+config STACKDEPOT_SUPPORT
+	def_bool y
+	depends on STACKTRACE_SUPPORT && !UNWINDER_GUESS
+
+# STACKDEPOT consumes megabytes of memory when compiled in, so we want to make
+# sure it's only enabled when needed. But we don't want to burden the user with
+# another choice. STACKTRACE is already configurable (but often selected by
+# other configs).
+#
+# Due to how kconfig dependency resolution  works, it's slightly complicated
+# as we cannot e.g. change STACKDEPOT to select STACKTRACE. Here are common
+# scenarios how to use STACKDEPOT with your config options:
+#
+# Scenario 1: hard dependency, will select also STACKTRACE, your config will
+# only be visible when STACKDEPOT_SUPPORT (and thus also STACKTRACE_SUPPORT) is
+# satisfied:
+#
+# 	depends on STACKDEPOT_SUPPORT
+#	select STACKTRACE
+#	select STACKDEPOT
+#
+# Scenario 2: soft dependency (e.g. for code with #ifdef CONFIG_STACKDEPOT
+# guards), will select STACKDEPOT only when STACKTRACE was configured by user
+# (or selected by some other config)
+#
+#	select STACKDEPOT if STACKDEPOT_SUPPORT && STACKTRACE
+#
  config STACKDEPOT
  	bool
-	select STACKTRACE
+	depends on STACKDEPOT_SUPPORT
config STACK_HASH_ORDER
  	int "stack depot hash size (12 => 4KB, 20 => 1024KB)"
diff --git a/mm/Kconfig.debug b/mm/Kconfig.debug
index 1e73717802f8..56a248814788 100644
--- a/mm/Kconfig.debug
+++ b/mm/Kconfig.debug
@@ -47,7 +47,7 @@ config DEBUG_PAGEALLOC_ENABLE_DEFAULT
config PAGE_OWNER
  	bool "Track page owner"
-	depends on DEBUG_KERNEL && STACKTRACE_SUPPORT
+	depends on DEBUG_KERNEL && STACKDEPOT_SUPPORT
  	select DEBUG_FS
  	select STACKTRACE
  	select STACKDEPOT

This looks good to me but I will wait for feedback from other maintainers.

Thanks again for review and feedback.

-- Imran




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux