答复: [外部邮件] Re: [PATCH] mm/mempool: use static key for boot-time debug enablement

["Li,Rongqing(ACG CCN)" <lirongqing@baidu.com>]

> > From: Li RongQing <lirongqing@baidu.com>
> >
> > Replace the #ifdef CONFIG_SLUB_DEBUG_ON conditional compilation with a
> > static key (mempool_debug_enabled). This allows enabling mempool
> > debugging at boot time via:
> >
> >     mempool_debug
> >
> > Instead of requiring CONFIG_SLUB_DEBUG_ON at compile time. Benefits:
> >
> > - Debugging can be enabled without rebuilding the kernel
> > - Uses standard kernel static_key mechanism with minimal overhead
> >
> > Suggested-by: Vlastimil Babka (SUSE) <vbabka@kernel.org>
> > Signed-off-by: Li RongQing <lirongqing@baidu.com>
> > Cc: Vlastimil Babka <vbabka@kernel.org>
> > Cc: Harry Yoo <harry@kernel.org>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Hao Li <hao.li@linux.dev>
> > Cc: Christoph Lameter <cl@gentwo.org>
> > Cc: David Rientjes <rientjes@google.com>
> > Cc: Roman Gushchin <roman.gushchin@linux.dev>
> > ---
> >  Documentation/admin-guide/kernel-parameters.txt |  5 ++++
> >  mm/mempool.c                                    | 32
> ++++++++++++++++++-------
> >  2 files changed, 28 insertions(+), 9 deletions(-)
> >
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt
> > b/Documentation/admin-guide/kernel-parameters.txt
> > index 35ed9dc..5a070e6 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -3998,6 +3998,11 @@ Kernel parameters
> >  			Note that even when enabled, there are a few cases where
> >  			the feature is not effective.
> >
> > +	mempool_debug	[MM]
> > +			Enable mempool debugging. This enables element
> > +			poison checking when freeing elements back to the
> > +			pool. Useful for debugging mempool corruption.
> > +
> >  	memtest=	[KNL,X86,ARM,M68K,PPC,RISCV,EARLY] Enable memtest
> >  			Format: <integer>
> >  			default : 0 <disable>
> > diff --git a/mm/mempool.c b/mm/mempool.c index db23e0e..4f429a1
> 100644
> > --- a/mm/mempool.c
> > +++ b/mm/mempool.c
> > @@ -16,11 +16,28 @@
> >  #include <linux/export.h>
> >  #include <linux/mempool.h>
> >  #include <linux/writeback.h>
> > +#include <linux/static_key.h>
> > +#include <linux/init.h>
> >  #include "slab.h"
> >
> >  static DECLARE_FAULT_ATTR(fail_mempool_alloc);
> >  static DECLARE_FAULT_ATTR(fail_mempool_alloc_bulk);
> >
> > +/*
> > + * Debugging support for mempool using static key.
> > + *
> > + * This allows enabling mempool debug at boot time via:
> > + *   mempool_debug
> > + */
> > +static DEFINE_STATIC_KEY_FALSE(mempool_debug_enabled);
> > +
> > +static int __init mempool_debug_setup(char *str) {
> > +	static_branch_enable(&mempool_debug_enabled);
> > +	return 0;
> > +}
> > +early_param("mempool_debug", mempool_debug_setup);
> > +
> 
> Can static_branch_enable() in mempool_debug_setup() run before
> jump_label_init() has set static_key_initialized?
> 
> Looking at start_kernel() in init/main.c:
> 
> 	setup_arch(&command_line);
> 	mm_core_init_early();
> 	/* Static keys and static calls are needed by LSMs */
> 	jump_label_init();
> 	...
> 	/* parameters may set static keys */
> 	parse_early_param();
> 
> This will trigger the warning in include/linux/jump_label.h has:
> 
> 	#define STATIC_KEY_CHECK_USE(key) WARN(!static_key_initialized, \
> 	    "%s(): static key '%pS' used before call to jump_label_init()", \
> 	    __func__, (key))
> 
> 
> mm/dmapool.c registers an equivalent debug toggle via __setup() rather than
> early_param():
> 
> 	static int __init dmapool_debug_setup(char *str)
> 	{
> 		static_branch_enable(&dmapool_debug_enabled);
> 		return 1;
> 	}
> 	__setup("dmapool_debug", dmapool_debug_setup);
> 
> I think you can reuse that.

Thanks for your review!

While this boot-time ordering used to be a generic issue, it seems many
architectures have already aligned or fixed this internally. For instance,

commit ca829e05d3d4 ("powerpc/64: Init jump labels before parse_early_param()")
and commit 6070970db9fe ("m68k: Initialize jump labels early during setup_arch()")
explicitly relocated jump_label_init() before the early parameter parsing.

Furthermore, leveraging early_param() to directly manage static keys is still
actively used and accepted in the current core kernel. Some examples include:

  - early_param("randomize_kstack_offset", early_randomize_kstack_offset);
  - early_param("threadirqs", setup_forced_irqthreads);

The primary reason for using early_param() here instead of __setup() is that
mempool allocations can happen extremely early during the boot phase. Moving
this to a later stage like __setup() would mean missing the tracking for the
most critical early-stage memory pools, which defeats the purpose of boot-time
debugging.

Therefore, I think using early_param() here is the most robust option to
ensure full coverage of mempool allocations.

What do you think?

Thanks

-Li


> 
> >  static int __init mempool_faul_inject_init(void)  {
> >  	int error;
> > @@ -37,7 +54,6 @@ static int __init mempool_faul_inject_init(void)  }
> > late_initcall(mempool_faul_inject_init);
> >
> > -#ifdef CONFIG_SLUB_DEBUG_ON
> >  static void poison_error(struct mempool *pool, void *element, size_t size,
> >  			 size_t byte)
> >  {
> > @@ -73,6 +89,9 @@ static void __check_element(struct mempool *pool,
> > void *element, size_t size)
> >
> >  static void check_element(struct mempool *pool, void *element)  {
> > +	if (!static_branch_unlikely(&mempool_debug_enabled))
> > +		return;
> > +
> >  	/* Skip checking: KASAN might save its metadata in the element. */
> >  	if (kasan_enabled())
> >  		return;
> > @@ -112,6 +131,9 @@ static void __poison_element(void *element, size_t
> > size)
> >
> >  static void poison_element(struct mempool *pool, void *element)  {
> > +	if (!static_branch_unlikely(&mempool_debug_enabled))
> > +		return;
> > +
> 
> Before this change, building with CONFIG_SLUB_DEBUG_ON=y compiled in
> check_element() and poison_element() unconditionally, so the poisoning and
> corruption checks ran on every mempool free/alloc.
> After this change those checks are gated on the mempool_debug boot parameter
> even when CONFIG_SLUB_DEBUG_ON=y.
> 
> Existing users who relied on CONFIG_SLUB_DEBUG_ON=y giving them mempool
> poison checking will silently lose it on upgrade unless they also add
> "mempool_debug" to the command line.
> 
> Would it be worth defaulting the static key to true under
> CONFIG_SLUB_DEBUG_ON=y, for example:
> 
> 	#ifdef CONFIG_SLUB_DEBUG_ON
> 	static DEFINE_STATIC_KEY_TRUE(mempool_debug_enabled);
> 	#else
> 	static DEFINE_STATIC_KEY_FALSE(mempool_debug_enabled);
> 	#endif
> 
> so the previous default behaviour is preserved.
> 
> 
> >  	/* Skip poisoning: KASAN might save its metadata in the element. */
> >  	if (kasan_enabled())
> >  		return;
> > @@ -140,14 +162,6 @@ static void poison_element(struct mempool *pool,
> > void *element)  #endif
> >  	}
> >  }
> > -#else /* CONFIG_SLUB_DEBUG_ON */
> > -static inline void check_element(struct mempool *pool, void *element)
> > -{ -} -static inline void poison_element(struct mempool *pool, void
> > *element) -{ -} -#endif /* CONFIG_SLUB_DEBUG_ON */
> >
> >  static __always_inline bool kasan_poison_element(struct mempool *pool,
> >  		void *element)
> > --
> > 2.9.4
> >
> >