slub: Proper kmemleak tracking if CONFIG_SLUB_DEBUG disabled

PATCH In review Frank Rowand 4 participants 8 messages Sep 27, 2013 – Oct 2, 2013

Subsystems (2)

Memory management Slab allocator

lore

Share & tools

lore

Frank Rowand

author

2013-09-27 20:38 UTC #0 lore raw

PATCH 1/1 slub: Proper kmemleak tracking if CONFIG_SLUB_DEBUG disabled

From: Roman Bobniev <[email protected]>

When kmemleak checking is enabled and CONFIG_SLUB_DEBUG is disabled, the kmemleak code for small block allocation is disabled. This results in false kmemleak errors when memory is freed.

Move the kmemleak code for small block allocation out from under CONFIG_SLUB_DEBUG.

Signed-off-by Roman Bobniev <[email protected]>

Signed-off-by Frank Rowand <[email protected]>

---
mm/slub.c | 6 3 + 3 - 0 !

 1 file changed, 3 insertions(+), 3 deletions(-)

mm/slub.c +3 -3

Index: b/mm/slub.c
===================================================================
@@ -947,13 +947,10 @@ static inline void slab_post_alloc_hook(
 {
 	flags &= gfp_allowed_mask;
 	kmemcheck_slab_alloc(s, flags, object, slab_ksize(s));
-	kmemleak_alloc_recursive(object, s->object_size, 1, s->flags, flags);
 }
 
 static inline void slab_free_hook(struct kmem_cache *s, void *x)
 {
-	kmemleak_free_recursive(x, s->flags);
-
 	/*
 	 * Trouble is that we may no longer disable interupts in the fast path
 	 * So in order to make the debug calls that expect irqs to be

@@ -2418,6 +2415,8 @@ redo:
 		memset(object, 0, s->object_size);
 
 	slab_post_alloc_hook(s, gfpflags, object);
+	kmemleak_alloc_recursive(object, s->objsize, 1, s->flags,
+				 gfpflags & gfp_allowed_mask);
 
 	return object;
 }

@@ -2614,6 +2613,7 @@ static __always_inline void slab_free(st
 	struct kmem_cache_cpu *c;
 	unsigned long tid;
 
+	kmemleak_free_recursive(x, s->flags);
 	slab_free_hook(s, x);
 
 redo:

Catalin Marinas

Acked-by on v1 1/1

2013-09-30 09:04 UTC #1 lore raw

Quoted 7 lines · Frank Rowand

From: Roman Bobniev <[email protected]>
When kmemleak checking is enabled and CONFIG_SLUB_DEBUG is disabled, the kmemleak code for small block allocation is disabled. This results in false kmemleak errors when memory is freed.

Quoted 5 lines

Move the kmemleak code for small block allocation out from under CONFIG_SLUB_DEBUG.
Signed-off-by Roman Bobniev <[email protected]>
Signed-off-by Frank Rowand <[email protected]>

Acked-by Catalin Marinas <[email protected]>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Christoph Lameter

2013-10-02 14:41 UTC #2 lore raw

In reply to Frank Rowand

Quoted 2 lines · Frank Rowand

Move the kmemleak code for small block allocation out from under CONFIG_SLUB_DEBUG.

Well in that case it may be better to move the hooks as a whole out of the CONFIG_SLUB_DEBUG section. Do the #ifdeffering for each call from the hooks instead.

The point of the hook functions is to separate the hooks out of the functions so taht they do not accumulate in the main code.

The patch moves one hook back into the main code. Please keep the checks in the hooks.

Bird, Tim

2013-10-02 15:33 UTC #3 lore raw

Quoted 10 lines · On Wednesday, October 02, 2013 7:41 AM, Christoph Lameter [[email protected]] wrote:

Quoted 2 lines · Frank Rowand
Move the kmemleak code for small block allocation out from under CONFIG_SLUB_DEBUG.
Well in that case it may be better to move the hooks as a whole out of the CONFIG_SLUB_DEBUG section. Do the #ifdeffering for each call from the hooks instead.

Quoted 5 lines

The point of the hook functions is to separate the hooks out of the functions so taht they do not accumulate in the main code.
The patch moves one hook back into the main code. Please keep the checks in the hooks.

Thanks for the feedback. Roman's first patch, which we discussed internally before sending this one, did exactly that. I guess Roman gets to say "I told you so." :-) My bad for telling him to change it.

We'll refactor along the lines that you describe, and send another one.

The problem child is actually the unconditional call to kmemleak_alloc() in kmalloc_large_node() (in slub.c). The problem comes because that call is unconditional on CONFIG_SLUB_DEBUG but the kmemleak
calls in the hook routines are conditional on CONFIG_SLUB_DEBUG. So if you have CONFIG_SLUB_DEBUG=n but CONFIG_DEBUG_KMEMLEAK=y, you get the false reports.

Now, there are kmemleak calls in kmalloc_large_node() and kfree() that don't follow the "hook" pattern. Should these be moved to 'hook' routines, to keep all the checks in the hooks?

Personally, I like the idea of keeping bookeeping/tracing/debug stuff in hook routines. I also like de-coupling CONFIG_SLUB_DEBUG and CONFIG_DEBUG_KMEMLEAK, but maybe others have a different opinon. Unless someone speaks up, we'll move the the currently in-function kmemleak calls into hooks, and all of the kmemleak stuff out from under CONFIG_SLUB_DEBUG.
We'll have to see if the ifdefs get a little messy.
-- Tim

Catalin Marinas

2013-10-02 15:54 UTC #4 lore raw

Quoted 34 lines · Tim

Quoted 10 lines · On Wednesday, October 02, 2013 7:41 AM, Christoph Lameter [[email protected]] wrote:
Quoted 2 lines · Frank Rowand
Move the kmemleak code for small block allocation out from under CONFIG_SLUB_DEBUG.
Well in that case it may be better to move the hooks as a whole out of the CONFIG_SLUB_DEBUG section. Do the #ifdeffering for each call from the hooks instead.
Quoted 5 lines
The point of the hook functions is to separate the hooks out of the functions so taht they do not accumulate in the main code.
The patch moves one hook back into the main code. Please keep the checks in the hooks.
Thanks for the feedback. Roman's first patch, which we discussed internally before sending this one, did exactly that. I guess Roman gets to say "I told you so." :-) My bad for telling him to change it.
We'll refactor along the lines that you describe, and send another one.
The problem child is actually the unconditional call to kmemleak_alloc() in kmalloc_large_node() (in slub.c). The problem comes because that call is unconditional on CONFIG_SLUB_DEBUG but the kmemleak
calls in the hook routines are conditional on CONFIG_SLUB_DEBUG. So if you have CONFIG_SLUB_DEBUG=n but CONFIG_DEBUG_KMEMLEAK=y, you get the false reports.
Now, there are kmemleak calls in kmalloc_large_node() and kfree() that don't follow the "hook" pattern. Should these be moved to 'hook' routines, to keep all the checks in the hooks?

Quoted 6 lines

Personally, I like the idea of keeping bookeeping/tracing/debug stuff in hook routines. I also like de-coupling CONFIG_SLUB_DEBUG and CONFIG_DEBUG_KMEMLEAK, but maybe others have a different opinon. Unless someone speaks up, we'll move the the currently in-function kmemleak calls into hooks, and all of the kmemleak stuff out from under CONFIG_SLUB_DEBUG.
We'll have to see if the ifdefs get a little messy.

Kmemleak doesn't depend on SLUB_DEBUG (at least it didn't originally ;), so I don't think we should add an artificial dependency (or select). Can we have kmemleak_*() calls in both debug and !debug hooks?

Signature · 7 lines

Catalin

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Christoph Lameter

2013-10-02 15:57 UTC #6 lore raw

In reply to Bird, Tim

Quoted 6 lines · Tim

The problem child is actually the unconditional call to kmemleak_alloc() in kmalloc_large_node() (in slub.c). The problem comes because that call is unconditional on CONFIG_SLUB_DEBUG but the kmemleak
calls in the hook routines are conditional on CONFIG_SLUB_DEBUG. So if you have CONFIG_SLUB_DEBUG=n but CONFIG_DEBUG_KMEMLEAK=y, you get the false reports.

Right. You need to put the #ifdef CONFIG_SLUB_DEBUG around the hooks that need it in the function itself instead of disabling the whole function if CONFIG_SLUB_DEUBG is not set.

Quoted 3 lines

Now, there are kmemleak calls in kmalloc_large_node() and kfree() that don't follow the "hook" pattern. Should these be moved to 'hook' routines, to keep all the checks in the hooks?

That would be great.

Quoted 6 lines

Personally, I like the idea of keeping bookeeping/tracing/debug stuff in hook routines. I also like de-coupling CONFIG_SLUB_DEBUG and CONFIG_DEBUG_KMEMLEAK, but maybe others have a different opinon. Unless someone speaks up, we'll move the the currently in-function kmemleak calls into hooks, and all of the kmemleak stuff out from under CONFIG_SLUB_DEBUG.
We'll have to see if the ifdefs get a little messy.

Decouple of you want. CONFIG_SLUB_DEBUG may duplicate what you already do.

Christoph Lameter

2013-10-02 16:24 UTC #5 lore raw

In reply to Catalin Marinas

Quoted 3 lines · Catalin Marinas

Kmemleak doesn't depend on SLUB_DEBUG (at least it didn't originally ;), so I don't think we should add an artificial dependency (or select). Can we have kmemleak_*() calls in both debug and !debug hooks?

Yes if you move the hook calls out from under CONFIG_SLUB_DEBUG.

Catalin Marinas

2013-10-02 16:26 UTC #7 lore raw

In reply to Christoph Lameter

Quoted 9 lines · Christoph Lameter

Quoted 6 lines · Tim
The problem child is actually the unconditional call to kmemleak_alloc() in kmalloc_large_node() (in slub.c). The problem comes because that call is unconditional on CONFIG_SLUB_DEBUG but the kmemleak
calls in the hook routines are conditional on CONFIG_SLUB_DEBUG. So if you have CONFIG_SLUB_DEBUG=n but CONFIG_DEBUG_KMEMLEAK=y, you get the false reports.

Quoted 3 lines

Right. You need to put the #ifdef CONFIG_SLUB_DEBUG around the hooks that need it in the function itself instead of disabling the whole function if CONFIG_SLUB_DEUBG is not set.

If we are to do this, we also need a DEBUG_KMEMLEAK dependency, something like:

	depends on (SLUB && SLUB_DEBUG) || !SLUB

select SLUB_DEBUG if SLUB

Otherwise you get a lot of false positives.

But with any of the above, #ifdef'ing out kmemleak_* calls wouldn't make much difference since they would already be no-ops in kmemleak.h with !SLUB_DEBUG.

Quoted 8 lines

Quoted 6 lines
Personally, I like the idea of keeping bookeeping/tracing/debug stuff in hook routines. I also like de-coupling CONFIG_SLUB_DEBUG and CONFIG_DEBUG_KMEMLEAK, but maybe others have a different opinon. Unless someone speaks up, we'll move the the currently in-function kmemleak calls into hooks, and all of the kmemleak stuff out from under CONFIG_SLUB_DEBUG.
We'll have to see if the ifdefs get a little messy.
Decouple of you want. CONFIG_SLUB_DEBUG may duplicate what you already do.

I would prefer the decoupling but I'm fine either way (as long as the dependencies are in place).

Signature · 7 lines

Catalin

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>