linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] slub: proper kmemleak tracking if CONFIG_SLUB_DEBUG disabled
@ 2013-10-08 22:58 Tim Bird
  2013-10-09 13:28 ` Catalin Marinas
  2013-10-09 19:07 ` Christoph Lameter
  0 siblings, 2 replies; 14+ messages in thread
From: Tim Bird @ 2013-10-08 22:58 UTC (permalink / raw)
  To: cl, catalin.marinas, frowand.list, bjorn.andersson, tim.bird, tbird20d
  Cc: linux-mm, linux-kernel, Roman Bobniev

From: Roman Bobniev <Roman.Bobniev@sonymobile.com>

Move all kmemleak calls into hook functions, and make it so
that all hooks (both inside and outside of #ifdef CONFIG_SLUB_DEBUG)
call the appropriate kmemleak routines.  This allows for kmemleak
to be configured independently of slub debug features.

It also fixes a bug where kmemleak was only partially enabled in some
configurations.

Signed-off-by: Roman Bobniev <Roman.Bobniev@sonymobile.com>
Signed-off-by: Tim Bird <tim.bird@sonymobile.com>
---
 mm/slub.c |   35 +++++++++++++++++++++++++++++++----
 1 file changed, 31 insertions(+), 4 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index c3eb3d3..0bb8591 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -933,6 +933,16 @@ static void trace(struct kmem_cache *s, struct page *page, void *object,
  * Hooks for other subsystems that check memory allocations. In a typical
  * production configuration these hooks all should produce no code at all.
  */
+static inline void kmalloc_large_node_hook(void *ptr, size_t size, gfp_t flags)
+{
+	kmemleak_alloc(ptr, size, 1, flags);
+}
+
+static inline void kfree_hook(const void *x)
+{
+	kmemleak_free(x);
+}
+
 static inline int slab_pre_alloc_hook(struct kmem_cache *s, gfp_t flags)
 {
 	flags &= gfp_allowed_mask;
@@ -1260,13 +1270,30 @@ static inline void inc_slabs_node(struct kmem_cache *s, int node,
 static inline void dec_slabs_node(struct kmem_cache *s, int node,
 							int objects) {}
 
+static inline void kmalloc_large_node_hook(void *ptr, size_t size, gfp_t flags)
+{
+	kmemleak_alloc(ptr, size, 1, flags);
+}
+
+static inline void kfree_hook(const void *x)
+{
+	kmemleak_free(x);
+}
+
 static inline int slab_pre_alloc_hook(struct kmem_cache *s, gfp_t flags)
 							{ return 0; }
 
 static inline void slab_post_alloc_hook(struct kmem_cache *s, gfp_t flags,
-		void *object) {}
+		void *object)
+{
+	kmemleak_alloc_recursive(object, s->object_size, 1, s->flags,
+		flags & gfp_allowed_mask);
+}
 
-static inline void slab_free_hook(struct kmem_cache *s, void *x) {}
+static inline void slab_free_hook(struct kmem_cache *s, void *x)
+{
+	kmemleak_free_recursive(x, s->flags);
+}
 
 #endif /* CONFIG_SLUB_DEBUG */
 
@@ -3272,7 +3299,7 @@ static void *kmalloc_large_node(size_t size, gfp_t flags, int node)
 	if (page)
 		ptr = page_address(page);
 
-	kmemleak_alloc(ptr, size, 1, flags);
+	kmalloc_large_node_hook(ptr, size, flags);
 	return ptr;
 }
 
@@ -3336,7 +3363,7 @@ void kfree(const void *x)
 	page = virt_to_head_page(x);
 	if (unlikely(!PageSlab(page))) {
 		BUG_ON(!PageCompound(page));
-		kmemleak_free(x);
+		kfree_hook(x);
 		__free_memcg_kmem_pages(page, compound_order(page));
 		return;
 	}
-- 
1.7.9.5

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] slub: proper kmemleak tracking if CONFIG_SLUB_DEBUG disabled
  2013-10-08 22:58 [PATCH] slub: proper kmemleak tracking if CONFIG_SLUB_DEBUG disabled Tim Bird
@ 2013-10-09 13:28 ` Catalin Marinas
  2013-10-09 19:07 ` Christoph Lameter
  1 sibling, 0 replies; 14+ messages in thread
From: Catalin Marinas @ 2013-10-09 13:28 UTC (permalink / raw)
  To: Tim Bird
  Cc: cl, frowand.list, bjorn.andersson, tbird20d, linux-mm,
	linux-kernel, Roman Bobniev

On Tue, Oct 08, 2013 at 11:58:57PM +0100, Tim Bird wrote:
> From: Roman Bobniev <Roman.Bobniev@sonymobile.com>
> 
> Move all kmemleak calls into hook functions, and make it so
> that all hooks (both inside and outside of #ifdef CONFIG_SLUB_DEBUG)
> call the appropriate kmemleak routines.  This allows for kmemleak
> to be configured independently of slub debug features.
> 
> It also fixes a bug where kmemleak was only partially enabled in some
> configurations.
> 
> Signed-off-by: Roman Bobniev <Roman.Bobniev@sonymobile.com>
> Signed-off-by: Tim Bird <tim.bird@sonymobile.com>

Looks ok to me.

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] slub: proper kmemleak tracking if CONFIG_SLUB_DEBUG disabled
  2013-10-08 22:58 [PATCH] slub: proper kmemleak tracking if CONFIG_SLUB_DEBUG disabled Tim Bird
  2013-10-09 13:28 ` Catalin Marinas
@ 2013-10-09 19:07 ` Christoph Lameter
  2013-10-23 11:52   ` Bobniev, Roman
  1 sibling, 1 reply; 14+ messages in thread
From: Christoph Lameter @ 2013-10-09 19:07 UTC (permalink / raw)
  To: Tim Bird
  Cc: catalin.marinas, frowand.list, bjorn.andersson, tbird20d,
	linux-mm, linux-kernel, Roman Bobniev, Pekka Enberg

On Tue, 8 Oct 2013, Tim Bird wrote:

> It also fixes a bug where kmemleak was only partially enabled in some
> configurations.

Acked-by: Christoph Lameter <cl@linux.com>

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH] slub: proper kmemleak tracking if CONFIG_SLUB_DEBUG disabled
  2013-10-09 19:07 ` Christoph Lameter
@ 2013-10-23 11:52   ` Bobniev, Roman
  2013-10-24 17:20     ` Pekka Enberg
  0 siblings, 1 reply; 14+ messages in thread
From: Bobniev, Roman @ 2013-10-23 11:52 UTC (permalink / raw)
  To: linux-mm
  Cc: catalin.marinas, frowand.list, "Andersson, Björn",
	tbird20d, linux-kernel, Pekka Enberg, Bird, Tim, cl, akpm

> On Tue, 8 Oct 2013, Tim Bird wrote:
> 
> > It also fixes a bug where kmemleak was only partially enabled in some
> > configurations.
> 
> Acked-by: Christoph Lameter <cl@linux.com>

Could you help me, who the maintainer is that
puts this patch in a tree and pushes it to mainline?
Do we wait on some additional Ack from someone?

With best regards,
Roman.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] slub: proper kmemleak tracking if CONFIG_SLUB_DEBUG disabled
  2013-10-23 11:52   ` Bobniev, Roman
@ 2013-10-24 17:20     ` Pekka Enberg
  0 siblings, 0 replies; 14+ messages in thread
From: Pekka Enberg @ 2013-10-24 17:20 UTC (permalink / raw)
  To: Bobniev, Roman
  Cc: linux-mm, catalin.marinas, frowand.list, Andersson, Björn,
	tbird20d, linux-kernel, Bird, Tim, cl, akpm

On Wed, Oct 23, 2013 at 2:52 PM, Bobniev, Roman
<Roman.Bobniev@sonymobile.com> wrote:
>> On Tue, 8 Oct 2013, Tim Bird wrote:
>>
>> > It also fixes a bug where kmemleak was only partially enabled in some
>> > configurations.
>>
>> Acked-by: Christoph Lameter <cl@linux.com>
>
> Could you help me, who the maintainer is that
> puts this patch in a tree and pushes it to mainline?
> Do we wait on some additional Ack from someone?

That would be me - sorry for the delay!

Applied, thanks!

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH] slub: proper kmemleak tracking if CONFIG_SLUB_DEBUG disabled
@ 2013-10-08 22:37 Tim Bird
  0 siblings, 0 replies; 14+ messages in thread
From: Tim Bird @ 2013-10-08 22:37 UTC (permalink / raw)
  To: cl, catalin.marinas, bjorn.andersson, frowand.list, tim.bird, tbird20d
  Cc: linux-mm, linux-kernel, Roman Bobniev

From: Roman Bobniev <Roman.Bobniev@sonymobile.com>

Move more kmemleak calls into hook functions, and make it so
that all hooks (both inside and outside of #ifdef CONFIG_SLUB_DEBUG_ON)
call the appropriate kmemleak routines.  This allows for
kmemleak to be configured independently of slub debug features.

It also fixes a bug where kmemleak was only partially enabled
in some configurations.

Signed-off-by: Roman Bobniev <Roman.Bobniev@sonymobile.com>
Signed-off-by: Tim Bird <tim.bird@sonymobile.com>
---
 mm/slub.c | 40 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 36 insertions(+), 4 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index c3eb3d3..95e170e 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -933,6 +933,11 @@ static void trace(struct kmem_cache *s, struct page *page, void *object,
  * Hooks for other subsystems that check memory allocations. In a typical
  * production configuration these hooks all should produce no code at all.
  */
+static inline post_alloc_hook(void *ptr, size_t size, gfp_t flags)
+{
+	kmemleak_alloc(ptr, size, 1, flags);
+}
+
 static inline int slab_pre_alloc_hook(struct kmem_cache *s, gfp_t flags)
 {
 	flags &= gfp_allowed_mask;
@@ -973,6 +978,11 @@ static inline void slab_free_hook(struct kmem_cache *s, void *x)
 		debug_check_no_obj_freed(x, s->object_size);
 }
 
+static inline void free_hook(void *x)
+{
+	kmemleak_free(x);
+}
+
 /*
  * Tracking of fully allocated slabs for debugging purposes.
  *
@@ -1260,13 +1270,35 @@ static inline void inc_slabs_node(struct kmem_cache *s, int node,
 static inline void dec_slabs_node(struct kmem_cache *s, int node,
 							int objects) {}
 
+/*
+ * Define the hook functions as empty of most debug code.
+ * However, leave the kmemleak calls so they can be configured
+ * independently of CONFIG_SLUB_DEBUG_ON.
+ */
+static inline post_alloc_hook(void *ptr, size_t size, gfp_t flags)
+{
+	kmemleak_alloc(ptr, size, 1, flags);
+}
+
 static inline int slab_pre_alloc_hook(struct kmem_cache *s, gfp_t flags)
 							{ return 0; }
 
 static inline void slab_post_alloc_hook(struct kmem_cache *s, gfp_t flags,
-		void *object) {}
+		void *object)
+{
+	kmemleak_alloc_recursive(object, s->object_size, 1, s->flags,
+		flags & gfp_allowed_mask);
+}
 
-static inline void slab_free_hook(struct kmem_cache *s, void *x) {}
+static inline void slab_free_hook(struct kmem_cache *s, void *x)
+{
+	kmemleak_free_recursive(x, s->flags);
+}
+
+static inline void free_hook(void *x)
+{
+	kmemleak_free(x);
+}
 
 #endif /* CONFIG_SLUB_DEBUG */
 
@@ -3272,7 +3304,7 @@ static void *kmalloc_large_node(size_t size, gfp_t flags, int node)
 	if (page)
 		ptr = page_address(page);
 
-	kmemleak_alloc(ptr, size, 1, flags);
+	post_alloc_hook(ptr, size, flags);
 	return ptr;
 }
 
@@ -3336,7 +3368,7 @@ void kfree(const void *x)
 	page = virt_to_head_page(x);
 	if (unlikely(!PageSlab(page))) {
 		BUG_ON(!PageCompound(page));
-		kmemleak_free(x);
+		free_hook(x);
 		__free_memcg_kmem_pages(page, compound_order(page));
 		return;
 	}
-- 
1.8.2.2

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] slub: Proper kmemleak tracking if CONFIG_SLUB_DEBUG disabled
  2013-10-02 15:57     ` Christoph Lameter
@ 2013-10-02 16:25       ` Catalin Marinas
  0 siblings, 0 replies; 14+ messages in thread
From: Catalin Marinas @ 2013-10-02 16:25 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Bird, Tim, Frank Rowand, Pekka Enberg, Matt Mackall, linux-mm,
	Linux Kernel list, Bobniev, Roman, Andersson, Björn

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

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] slub: Proper kmemleak tracking if CONFIG_SLUB_DEBUG disabled
  2013-10-02 15:54     ` Catalin Marinas
@ 2013-10-02 16:24       ` Christoph Lameter
  0 siblings, 0 replies; 14+ messages in thread
From: Christoph Lameter @ 2013-10-02 16:24 UTC (permalink / raw)
  To: Catalin Marinas
  Cc: Bird, Tim, Frank Rowand, Pekka Enberg, Matt Mackall, linux-mm,
	Linux Kernel list, Bobniev, Roman, Andersson, Björn

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.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH] slub: Proper kmemleak tracking if CONFIG_SLUB_DEBUG disabled
  2013-10-02 15:33   ` Bird, Tim
  2013-10-02 15:54     ` Catalin Marinas
@ 2013-10-02 15:57     ` Christoph Lameter
  2013-10-02 16:25       ` Catalin Marinas
  1 sibling, 1 reply; 14+ messages in thread
From: Christoph Lameter @ 2013-10-02 15:57 UTC (permalink / raw)
  To: Bird, Tim
  Cc: Frank Rowand, Pekka Enberg, Matt Mackall, linux-mm,
	Linux Kernel list, Catalin Marinas, Bobniev, Roman,
	"Andersson, Björn"

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.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] slub: Proper kmemleak tracking if CONFIG_SLUB_DEBUG disabled
  2013-10-02 15:33   ` Bird, Tim
@ 2013-10-02 15:54     ` Catalin Marinas
  2013-10-02 16:24       ` Christoph Lameter
  2013-10-02 15:57     ` Christoph Lameter
  1 sibling, 1 reply; 14+ messages in thread
From: Catalin Marinas @ 2013-10-02 15:54 UTC (permalink / raw)
  To: Bird, Tim
  Cc: Christoph Lameter, Frank Rowand, Pekka Enberg, Matt Mackall,
	linux-mm, Linux Kernel list, Bobniev, Roman, Andersson,
	Björn

On Wed, Oct 02, 2013 at 04:33:47PM +0100, Bird, Tim wrote:
> On Wednesday, October 02, 2013 7:41 AM, Christoph Lameter [cl@linux.com] 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

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH] slub: Proper kmemleak tracking if CONFIG_SLUB_DEBUG disabled
  2013-10-02 14:41 ` Christoph Lameter
@ 2013-10-02 15:33   ` Bird, Tim
  2013-10-02 15:54     ` Catalin Marinas
  2013-10-02 15:57     ` Christoph Lameter
  0 siblings, 2 replies; 14+ messages in thread
From: Bird, Tim @ 2013-10-02 15:33 UTC (permalink / raw)
  To: Christoph Lameter, Frank Rowand
  Cc: Pekka Enberg, Matt Mackall, linux-mm, Linux Kernel list,
	Catalin Marinas, Bobniev, Roman, "Andersson, Björn"

On Wednesday, October 02, 2013 7:41 AM, Christoph Lameter [cl@linux.com] 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


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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] slub: Proper kmemleak tracking if CONFIG_SLUB_DEBUG disabled
  2013-09-27 20:38 [PATCH] slub: Proper " Frank Rowand
  2013-09-30  9:04 ` Catalin Marinas
@ 2013-10-02 14:41 ` Christoph Lameter
  2013-10-02 15:33   ` Bird, Tim
  1 sibling, 1 reply; 14+ messages in thread
From: Christoph Lameter @ 2013-10-02 14:41 UTC (permalink / raw)
  To: Frank Rowand
  Cc: Pekka Enberg, Matt Mackall, linux-mm, Linux Kernel list,
	Catalin Marinas, Bobniev, Roman, Bird, Tim, "Andersson,
	Björn"

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.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] slub: Proper kmemleak tracking if CONFIG_SLUB_DEBUG disabled
  2013-09-27 20:38 [PATCH] slub: Proper " Frank Rowand
@ 2013-09-30  9:04 ` Catalin Marinas
  2013-10-02 14:41 ` Christoph Lameter
  1 sibling, 0 replies; 14+ messages in thread
From: Catalin Marinas @ 2013-09-30  9:04 UTC (permalink / raw)
  To: Frank Rowand
  Cc: Christoph Lameter, Pekka Enberg, Matt Mackall, linux-mm,
	Linux Kernel list, Bobniev, Roman, Bird, Tim, Andersson,
	Björn

On Fri, Sep 27, 2013 at 09:38:27PM +0100, Frank Rowand wrote:
> From: Roman Bobniev <roman.bobniev@sonymobile.com>
> 
> 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 <roman.bobniev@sonymobile.com>
> Signed-off-by: Frank Rowand <frank.rowand@sonymobile.com>

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH] slub: Proper kmemleak tracking if CONFIG_SLUB_DEBUG disabled
@ 2013-09-27 20:38 Frank Rowand
  2013-09-30  9:04 ` Catalin Marinas
  2013-10-02 14:41 ` Christoph Lameter
  0 siblings, 2 replies; 14+ messages in thread
From: Frank Rowand @ 2013-09-27 20:38 UTC (permalink / raw)
  To: Christoph Lameter, Pekka Enberg, Matt Mackall, linux-mm,
	Linux Kernel list, Catalin Marinas, Bobniev, Roman
  Cc: Bird, Tim, "Andersson, Björn"

From: Roman Bobniev <roman.bobniev@sonymobile.com>

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 <roman.bobniev@sonymobile.com>
Signed-off-by: Frank Rowand <frank.rowand@sonymobile.com>
---
 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:

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2013-10-24 17:21 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-10-08 22:58 [PATCH] slub: proper kmemleak tracking if CONFIG_SLUB_DEBUG disabled Tim Bird
2013-10-09 13:28 ` Catalin Marinas
2013-10-09 19:07 ` Christoph Lameter
2013-10-23 11:52   ` Bobniev, Roman
2013-10-24 17:20     ` Pekka Enberg
  -- strict thread matches above, loose matches on Subject: below --
2013-10-08 22:37 Tim Bird
2013-09-27 20:38 [PATCH] slub: Proper " Frank Rowand
2013-09-30  9:04 ` Catalin Marinas
2013-10-02 14:41 ` Christoph Lameter
2013-10-02 15:33   ` Bird, Tim
2013-10-02 15:54     ` Catalin Marinas
2013-10-02 16:24       ` Christoph Lameter
2013-10-02 15:57     ` Christoph Lameter
2013-10-02 16:25       ` Catalin Marinas

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).