答复: [外部邮件] Re: [PATCH][v2] mm/mempool: Untangle CONFIG_SLUB_DEBUG_ON abuse and switch to static key

[Li,Rongqing]

> On 6/2/26 08:21, lirongqing wrote:
> > From: Li RongQing <lirongqing@xxxxxxxxx>
> >
> > The mempool subsystem historically wrapped its debugging logic inside
> > an merely defines compile-time defaults for SLUB and caused two flaws:
> >
> > 1. On production kernels where CONFIG_SLUB_DEBUG=y but
> >    CONFIG_SLUB_DEBUG_ON=n, mempool debugging was completely
> compiled out
> >    at compile time.
> > 2. On kernels with CONFIG_SLUB_DEBUG_ON=y, mempool debugging stayed
> active
> >    even if a user explicitly disabled slub debugging at boot time.
> >
> > Clean up this mess by removing the #ifdef and switching to a runtime
> > static key (mempool_debug_enabled), allowing mempool debugging to be
> > toggled cleanly via its own boot parameter.
> >
> > Suggested-by: Vlastimil Babka (SUSE) <vbabka@xxxxxxxxxx>
> > Signed-off-by: Li RongQing <lirongqing@xxxxxxxxx>
> > Cc: Vlastimil Babka <vbabka@xxxxxxxxxx>
> > Cc: Harry Yoo <harry@xxxxxxxxxx>
> > Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> > Cc: Hao Li <hao.li@xxxxxxxxx>
> > Cc: Christoph Lameter <cl@xxxxxxxxxx>
> > Cc: David Rientjes <rientjes@xxxxxxxxxx>
> > Cc: Roman Gushchin <roman.gushchin@xxxxxxxxx>
> > Cc: Matthew Wilcox <willy@xxxxxxxxxxxxx>
> > Cc: Usama Arif <usama.arif@xxxxxxxxx>
> > ---
> > Diff with v1:
> > 	Rewrite commit message, change early_param to __setup
> >
> >  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 642659b..89b5994 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -3980,6 +3980,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..71e4b54
> 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 1;
> > +}
> > +__setup("mempool_debug", mempool_debug_setup);
> > +
> >  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;
> 
> With static keys it's better to have the check in the caller of check_element so
> we skip over that call if it's disabled, and not do a call that immediately
> returns. Since there's a single caller, there's no need for an extra inline
> wrapper.
> 

Ok, I will change it in next version 
Thanks 

[Li,Rongqing] 


> > +
> >  	/* 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;
> 
> Ditto.
> 
> > +
> >  	/* 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)