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(-)
Index: b/mm/slub.c
===================================================================
--- a/mm/slub.c
+++ 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:
On Fri, Sep 27, 2013 at 09:38:27PM +0100, Frank Rowand wrote:
> 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]>
Acked-by: Catalin Marinas <[email protected]>
On Fri, 27 Sep 2013, Frank Rowand wrote:
> 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.
On Wednesday, October 02, 2013 7:41 AM, Christoph Lameter [[email protected]] wrote:
>
>On Fri, 27 Sep 2013, Frank Rowand wrote:
>
>> 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.
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
On Wed, Oct 02, 2013 at 04:33:47PM +0100, Bird, Tim wrote:
> On Wednesday, October 02, 2013 7:41 AM, Christoph Lameter [[email protected]] wrote:
> >
> >On Fri, 27 Sep 2013, Frank Rowand wrote:
> >
> >> 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.
>
> 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.
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?
--
Catalin
On Wed, 2 Oct 2013, Bird, Tim wrote:
> 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.
> 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.
> 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.
On Wed, 2 Oct 2013, Catalin Marinas wrote:
> 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.
On Wed, Oct 02, 2013 at 04:57:12PM +0100, Christoph Lameter wrote:
> On Wed, 2 Oct 2013, Bird, Tim wrote:
>
> > 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.
If we are to do this, we also need a DEBUG_KMEMLEAK dependency,
something like:
depends on (SLUB && SLUB_DEBUG) || !SLUB
or
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.
> > 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).
--
Catalin