On Thu, Oct 07, 2021 at 11:58AM +0200, Vlastimil Babka wrote:
[...]
> - Add a CONFIG_STACKDEPOT_ALWAYS_INIT flag to keep using the current
> well-defined point of allocation as part of mem_init(). Make CONFIG_KASAN
> select this flag.
> - Other users have to call stack_depot_init() as part of their own init when
> it's determined that stack depot will actually be used. This may depend on
> both config and runtime conditions. Convert current users which are
> page_owner and several in the DRM subsystem. Same will be done for SLUB
> later.
> - Because the init might now be called after the boot-time memblock allocation
> has given all memory to the buddy allocator, change stack_depot_init() to
> allocate stack_table with kvmalloc() when memblock is no longer available.
> Also handle allocation failure by disabling stackdepot (could have
> theoretically happened even with memblock allocation previously), and don't
> unnecessarily align the memblock allocation to its own size anymore.
...
> Hi, I'd appreciate review of the DRM parts - namely that I've got correctly
> that stack_depot_init() is called from the proper init functions and iff
> stack_depot_save() is going to be used later. Thanks!
For ease of review between stackdepot and DRM changes, I thought it'd be
nice to split into 2 patches, but not sure it'll work, because you're
changing the semantics of the normal STACKDEPOT.
One option would be to flip it around, and instead have
STACKDEPOT_LAZY_INIT, but that seems counter-intuitive if the majority
of STACKDEPOT users are LAZY_INIT users.
On the other hand, the lazy initialization mode you're introducing
requires an explicit stack_depot_init() call somewhere and isn't as
straightforward as before.
Not sure what is best. My intuition tells me STACKDEPOT_LAZY_INIT would
be safer as it's a deliberate opt-in to the lazy initialization
behaviour.
Preferences?
[...]
> --- a/drivers/gpu/drm/drm_mm.c
> +++ b/drivers/gpu/drm/drm_mm.c
> @@ -980,6 +980,10 @@ void drm_mm_init(struct drm_mm *mm, u64 start, u64 size)
> add_hole(&mm->head_node);
>
> mm->scan_active = 0;
> +
> +#ifdef CONFIG_DRM_DEBUG_MM
> + stack_depot_init();
> +#endif
DRM_DEBUG_MM implies STACKDEPOT. Not sure what is more readable to drm
maintainers, but perhaps it'd be nicer to avoid the #ifdef here, and
instead just keep the no-op version of stack_depot_init() in
<linux/stackdepot.h>. I don't have a strong preference.
> }
> EXPORT_SYMBOL(drm_mm_init);
>
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 0d85f3c5c526..806c32ab410b 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -68,6 +68,9 @@ static noinline depot_stack_handle_t __save_depot_stack(void)
> static void init_intel_runtime_pm_wakeref(struct intel_runtime_pm *rpm)
> {
> spin_lock_init(&rpm->debug.lock);
> +
> + if (rpm->available)
> + stack_depot_init();
> }
>
> static noinline depot_stack_handle_t
> diff --git a/include/linux/stackdepot.h b/include/linux/stackdepot.h
> index c34b55a6e554..60ba99a43745 100644
> --- a/include/linux/stackdepot.h
> +++ b/include/linux/stackdepot.h
> @@ -15,6 +15,16 @@
>
> typedef u32 depot_stack_handle_t;
>
> +/*
> + * Every user of stack depot has to call this during its own init when it's
> + * decided that it will be calling stack_depot_save() later.
> + *
> + * The alternative is to select STACKDEPOT_ALWAYS_INIT to have stack depot
> + * enabled as part of mm_init(), for subsystems where it's known at compile time
> + * that stack depot will be used.
> + */
> +int stack_depot_init(void);
> +
> depot_stack_handle_t __stack_depot_save(unsigned long *entries,
> unsigned int nr_entries,
> gfp_t gfp_flags, bool can_alloc);
> @@ -30,13 +40,4 @@ int stack_depot_snprint(depot_stack_handle_t handle, char *buf, size_t size,
>
> void stack_depot_print(depot_stack_handle_t stack);
>
> -#ifdef CONFIG_STACKDEPOT
> -int stack_depot_init(void);
> -#else
> -static inline int stack_depot_init(void)
> -{
> - return 0;
> -}
> -#endif /* CONFIG_STACKDEPOT */
> -
Could we avoid the IS_ENABLED() in init/main.c by adding a wrapper here:
+#ifdef CONFIG_STACKDEPOT_ALWAYS_INIT
+static inline int stack_depot_early_init(void) { return stack_depot_init(); }
+#else
+static inline int stack_depot_early_init(void) { return 0; }
+#endif /* CONFIG_STACKDEPOT_ALWAYS_INIT */
> #endif
> diff --git a/init/main.c b/init/main.c
> index ee4d3e1b3eb9..b6a5833d98f5 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -844,7 +844,8 @@ static void __init mm_init(void)
> init_mem_debugging_and_hardening();
> kfence_alloc_pool();
> report_meminit();
> - stack_depot_init();
> + if (IS_ENABLED(CONFIG_STACKDEPOT_ALWAYS_INIT))
> + stack_depot_init();
I'd push the decision of when to call this into <linux/stackdepot.h> via
wrapper stack_depot_early_init().
> mem_init();
> mem_init_print_info();
> /* page_owner must be initialized after buddy is ready */
> diff --git a/lib/Kconfig b/lib/Kconfig
> index 5e7165e6a346..df6bcf0a4cc3 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -671,6 +671,9 @@ config STACKDEPOT
> bool
> select STACKTRACE
>
> +config STACKDEPOT_ALWAYS_INIT
> + bool
It looks like every users of STACKDEPOT_ALWAYS_INIT will also select
STACKDEPOT, so we could just make this:
+config STACKDEPOT_ALWAYS_INIT
+ bool
+ select STACKDEPOT
And remove the redundant 'select STACKDEPOT' in Kconfig.kasan.
> config STACK_HASH_ORDER
> int "stack depot hash size (12 => 4KB, 20 => 1024KB)"
> range 12 20
> diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
> index cdc842d090db..695deb603c66 100644
> --- a/lib/Kconfig.kasan
> +++ b/lib/Kconfig.kasan
> @@ -39,6 +39,7 @@ menuconfig KASAN
> HAVE_ARCH_KASAN_HW_TAGS
> depends on (SLUB && SYSFS) || (SLAB && !DEBUG_SLAB)
> select STACKDEPOT
> + select STACKDEPOT_ALWAYS_INIT
[...]
>
> -int __init stack_depot_init(void)
> +/*
> + * __ref because of memblock_alloc(), which will not be actually called after
> + * the __init code is gone
The reason is that after __init code is gone, slab_is_available() will
be true (might be worth adding to the comment).
> + */
> +__ref int stack_depot_init(void)
> {
> - if (!stack_depot_disable) {
> + mutex_lock(&stack_depot_init_mutex);
> + if (!stack_depot_disable && stack_table == NULL) {
> size_t size = (STACK_HASH_SIZE * sizeof(struct stack_record *));
Thanks,
-- Marco
