[PATCH] mm: slub: SLUB_DEBUG=n: use the same alloc/free hooks as for SLUB_DEBUG=y

[PATCH] mm: slub: SLUB_DEBUG=n: use the same alloc/free hooks as for SLUB_DEBUG=y

[Andrey Ryabinin]

There are two versions of alloc/free hooks now - one for CONFIG_SLUB_DEBUG=y
and another one for CONFIG_SLUB_DEBUG=n.

I see no reason why calls to other debugging subsystems (LOCKDEP,
DEBUG_ATOMIC_SLEEP, KMEMCHECK and FAILSLAB) are hidden under SLUB_DEBUG.
All this features should work regardless of SLUB_DEBUG config, as all of
them already have own Kconfig options.

This also fixes failslab for CONFIG_SLUB_DEBUG=n configuration.
It simply not worked before because should_failslab() call was in hook
hidden under "#ifdef CONFIG_SLUB_DEBUG #else".

Note: There is one concealed change in allocation path for SLUB_DEBUG=n and all
other debugging features disabled. might_sleep_if() call can generate some code
even if DEBUG_ATOMIC_SLEEP=n. For PREEMPT_VOLUNTARY=y migth_sleep() inserts
_cond_resched() call, but I think it should be ok.

Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com>
---
 mm/slub.c | 97 ++++++++++++++++++++++++---------------------------------------
 1 file changed, 36 insertions(+), 61 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index b2b0473..8f477c3 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -945,60 +945,6 @@ 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;
-	lockdep_trace_alloc(flags);
-	might_sleep_if(flags & __GFP_WAIT);
-
-	return should_failslab(s->object_size, flags, s->flags);
-}
-
-static inline void slab_post_alloc_hook(struct kmem_cache *s,
-					gfp_t flags, void *object)
-{
-	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 interrupts in the fast path
-	 * So in order to make the debug calls that expect irqs to be
-	 * disabled we need to disable interrupts temporarily.
-	 */
-#if defined(CONFIG_KMEMCHECK) || defined(CONFIG_LOCKDEP)
-	{
-		unsigned long flags;
-
-		local_irq_save(flags);
-		kmemcheck_slab_free(s, x, s->object_size);
-		debug_check_no_locks_freed(x, s->object_size);
-		local_irq_restore(flags);
-	}
-#endif
-	if (!(s->flags & SLAB_DEBUG_OBJECTS))
-		debug_check_no_obj_freed(x, s->object_size);
-}
-
-/*
  * Tracking of fully allocated slabs for debugging purposes.
  */
 static void add_full(struct kmem_cache *s,
@@ -1282,6 +1228,12 @@ 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) {}
 
+#endif /* CONFIG_SLUB_DEBUG */
+
+/*
+ * 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);
@@ -1293,22 +1245,45 @@ static inline void kfree_hook(const void *x)
 }
 
 static inline int slab_pre_alloc_hook(struct kmem_cache *s, gfp_t flags)
-							{ return 0; }
+{
+	flags &= gfp_allowed_mask;
+	lockdep_trace_alloc(flags);
+	might_sleep_if(flags & __GFP_WAIT);
+
+	return should_failslab(s->object_size, flags, s->flags);
+}
 
-static inline void slab_post_alloc_hook(struct kmem_cache *s, gfp_t flags,
-		void *object)
+static inline void slab_post_alloc_hook(struct kmem_cache *s,
+					gfp_t flags, void *object)
 {
-	kmemleak_alloc_recursive(object, s->object_size, 1, s->flags,
-		flags & gfp_allowed_mask);
+	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 interrupts in the fast path
+	 * So in order to make the debug calls that expect irqs to be
+	 * disabled we need to disable interrupts temporarily.
+	 */
+#if defined(CONFIG_KMEMCHECK) || defined(CONFIG_LOCKDEP)
+	{
+		unsigned long flags;
+
+		local_irq_save(flags);
+		kmemcheck_slab_free(s, x, s->object_size);
+		debug_check_no_locks_freed(x, s->object_size);
+		local_irq_restore(flags);
+	}
+#endif
+	if (!(s->flags & SLAB_DEBUG_OBJECTS))
+		debug_check_no_obj_freed(x, s->object_size);
 }
 
-#endif /* CONFIG_SLUB_DEBUG */
-
 /*
  * Slab allocation and freeing
  */
-- 
1.8.5.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>

Re: [PATCH] mm: slub: SLUB_DEBUG=n: use the same alloc/free hooks as for SLUB_DEBUG=y

[Christoph Lameter]

On Thu, 19 Jun 2014, Andrey Ryabinin wrote:

> I see no reason why calls to other debugging subsystems (LOCKDEP,
> DEBUG_ATOMIC_SLEEP, KMEMCHECK and FAILSLAB) are hidden under SLUB_DEBUG.
> All this features should work regardless of SLUB_DEBUG config, as all of
> them already have own Kconfig options.

The reason for hiding this under SLUB_DEBUG was to have some way to
guarantee that no instrumentations is added if one does not want it.

SLUB_DEBUG is on by default and builds in a general
debugging framework that can be enabled at runtime in
production kernels.

If someone disabled SLUB_DEBUG then that has been done with the intend to
get a minimal configuration.


--
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>

Re: [PATCH] mm: slub: SLUB_DEBUG=n: use the same alloc/free hooks as for SLUB_DEBUG=y

[Andrew Morton]

On Thu, 19 Jun 2014 15:56:56 -0500 (CDT) Christoph Lameter <cl@gentwo.org> wrote:

> On Thu, 19 Jun 2014, Andrey Ryabinin wrote:
> 
> > I see no reason why calls to other debugging subsystems (LOCKDEP,
> > DEBUG_ATOMIC_SLEEP, KMEMCHECK and FAILSLAB) are hidden under SLUB_DEBUG.
> > All this features should work regardless of SLUB_DEBUG config, as all of
> > them already have own Kconfig options.
> 
> The reason for hiding this under SLUB_DEBUG was to have some way to
> guarantee that no instrumentations is added if one does not want it.
> 
> SLUB_DEBUG is on by default and builds in a general
> debugging framework that can be enabled at runtime in
> production kernels.
> 
> If someone disabled SLUB_DEBUG then that has been done with the intend to
> get a minimal configuration.
> 

(Is that a nack?)

The intent seems to have been implemented strangely.  Perhaps it would
be clearer and more conventional to express all this using Kconfig
logic.

Anyway, if we plan to leave the code as-is then can we please get a
comment in there so the next person is not similarly confused?

--
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>

Re: [PATCH] mm: slub: SLUB_DEBUG=n: use the same alloc/free hooks as for SLUB_DEBUG=y

[Andrey Ryabinin]

2014-06-20 1:06 GMT+04:00 Andrew Morton <akpm@linux-foundation.org>:
> On Thu, 19 Jun 2014 15:56:56 -0500 (CDT) Christoph Lameter <cl@gentwo.org> wrote:
>
>> On Thu, 19 Jun 2014, Andrey Ryabinin wrote:
>>
>> > I see no reason why calls to other debugging subsystems (LOCKDEP,
>> > DEBUG_ATOMIC_SLEEP, KMEMCHECK and FAILSLAB) are hidden under SLUB_DEBUG.
>> > All this features should work regardless of SLUB_DEBUG config, as all of
>> > them already have own Kconfig options.
>>
>> The reason for hiding this under SLUB_DEBUG was to have some way to
>> guarantee that no instrumentations is added if one does not want it.
>>
>> SLUB_DEBUG is on by default and builds in a general
>> debugging framework that can be enabled at runtime in
>> production kernels.
>>
>> If someone disabled SLUB_DEBUG then that has been done with the intend to
>> get a minimal configuration.
>>
>
> (Is that a nack?)
>
> The intent seems to have been implemented strangely.  Perhaps it would
> be clearer and more conventional to express all this using Kconfig
> logic.
>

Seems I forgot to mention in commit message that the main intent of
this patch is not to fix some weird configurations, which nobody uses,
but a simple cleanup. Just look at diffstat (36 insertions vs 61
deletions). And someone who is going to add more debug hooks in future
will have to do it only in one place.

> Anyway, if we plan to leave the code as-is then can we please get a
> comment in there so the next person is not similarly confused?
>



-- 
Best regards,
Andrey Ryabinin

--
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>

Re: [PATCH] mm: slub: SLUB_DEBUG=n: use the same alloc/free hooks as for SLUB_DEBUG=y

[Christoph Lameter]

On Thu, 19 Jun 2014, Andrew Morton wrote:

> (Is that a nack?)

Not sure.

> The intent seems to have been implemented strangely.  Perhaps it would
> be clearer and more conventional to express all this using Kconfig
> logic.

Well it really does not work right since SLUB_DEBUG=y is the default
config and this behavior would be a bit surprising.

> Anyway, if we plan to leave the code as-is then can we please get a
> comment in there so the next person is not similarly confused?

Ok. Lets apply the patch.

Gosh. I think we need some way to figure out if code is being added to the
critical paths. I had no idea about that latest issue where might_sleep
suddenly became a call to cond_resched() until I saw the bug report.



--
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>