[PATCH] mm/slub: disable user tracing for kmemleak caches

[PATCH] mm/slub: disable user tracing for kmemleak caches

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

If kmemleak is enabled, it uses a kmem cache for its own objects.
These objects are used to hold information kmemleak uses, including
a stack trace. If slub_debug is also turned on, each of them has
*another* stack trace, so the overhead adds up, and on my tests (on
ARCH=um, admittedly) 2/3rds of the allocations end up being doing
the stack tracing.

Turn off SLAB_STORE_USER if SLAB_NOLEAKTRACE was given, to avoid
storing the essentially same data twice.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
Perhaps instead it should go the other way around, and kmemleak
could even use/access the stack trace that's already in there ...
But I don't really care too much, I can just turn off slub debug
for the kmemleak caches via the command line anyway :-)

---
 mm/slub.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/mm/slub.c b/mm/slub.c
index 34dcc09e2ec9..625a32a6645b 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1446,7 +1446,16 @@ slab_flags_t kmem_cache_flags(unsigned int object_size,
 		}
 	}
 
-	return flags | slub_debug;
+	flags |= slub_debug;
+
+	/*
+	 * If the slab cache is for debugging (e.g. kmemleak) then
+	 * don't store user (stack trace) information.
+	 */
+	if (flags & SLAB_NOLEAKTRACE)
+		flags &= ~SLAB_STORE_USER;
+
+	return flags;
 }
 #else /* !CONFIG_SLUB_DEBUG */
 static inline void setup_object_debug(struct kmem_cache *s,
-- 
2.26.2

Re: [PATCH] mm/slub: disable user tracing for kmemleak caches

[Vlastimil Babka]

On 1/13/21 5:09 PM, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> If kmemleak is enabled, it uses a kmem cache for its own objects.
> These objects are used to hold information kmemleak uses, including
> a stack trace. If slub_debug is also turned on, each of them has
> *another* stack trace, so the overhead adds up, and on my tests (on
> ARCH=um, admittedly) 2/3rds of the allocations end up being doing
> the stack tracing.
> 
> Turn off SLAB_STORE_USER if SLAB_NOLEAKTRACE was given, to avoid
> storing the essentially same data twice.
> 
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>

How about stripping away SLAB_STORE_USER only if it's added from the global
slub_debug variable? In case somebody lists one of the kmemleak caches
explicitly in "slub_debug=..." instead of just booting with "slub_debug", we
should honor that.

> ---
> Perhaps instead it should go the other way around, and kmemleak
> could even use/access the stack trace that's already in there ...
> But I don't really care too much, I can just turn off slub debug
> for the kmemleak caches via the command line anyway :-)
> 
> ---
>  mm/slub.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 34dcc09e2ec9..625a32a6645b 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1446,7 +1446,16 @@ slab_flags_t kmem_cache_flags(unsigned int object_size,
>  		}
>  	}
>  
> -	return flags | slub_debug;
> +	flags |= slub_debug;
> +
> +	/*
> +	 * If the slab cache is for debugging (e.g. kmemleak) then
> +	 * don't store user (stack trace) information.
> +	 */
> +	if (flags & SLAB_NOLEAKTRACE)
> +		flags &= ~SLAB_STORE_USER;
> +
> +	return flags;
>  }
>  #else /* !CONFIG_SLUB_DEBUG */
>  static inline void setup_object_debug(struct kmem_cache *s,
> 

Re: [PATCH] mm/slub: disable user tracing for kmemleak caches

[Johannes Berg]

On Wed, 2021-01-13 at 17:59 +0100, Vlastimil Babka wrote:
> On 1/13/21 5:09 PM, Johannes Berg wrote:
> > From: Johannes Berg <johannes.berg@intel.com>
> > 
> > If kmemleak is enabled, it uses a kmem cache for its own objects.
> > These objects are used to hold information kmemleak uses, including
> > a stack trace. If slub_debug is also turned on, each of them has
> > *another* stack trace, so the overhead adds up, and on my tests (on
> > ARCH=um, admittedly) 2/3rds of the allocations end up being doing
> > the stack tracing.
> > 
> > Turn off SLAB_STORE_USER if SLAB_NOLEAKTRACE was given, to avoid
> > storing the essentially same data twice.
> > 
> > Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> 
> How about stripping away SLAB_STORE_USER only if it's added from the global
> slub_debug variable? In case somebody lists one of the kmemleak caches
> explicitly in "slub_debug=..." instead of just booting with "slub_debug", we
> should honor that.

Good point, that makes a lot of sense.

TBH, I mostly sent this to see if anyone would think it acceptable. I've
now disabled slub debugging completely for the kmemleak caches by
command line, and as expected that improves things further. I'm _hoping_
of course that kmemleak itself doesn't contain egregious bugs, but seems
like a fair bet for now :)

So what do you/people think? Should we disable this? Disable all?
Subject to the above constraint, either way.

Thanks,
johannes