mm/slab.c: remove effectively dead code from kmem_cache_create

mm/slab.c: remove effectively dead code from kmem_cache_create

[Daniel Santos]

[-- Attachment #1: Type: text/plain, Size: 3333 bytes --]

I was examining slab.c when I noticed that there is code that will never
be executed, but that the compiler probably wouldn't determine as such. 
It turns out to be the case.  The below instructions (from a "disas /m 
kmem_cache_create" in gdb) will never be executed (or will have no
effect) since CONFIG_DEBUG_SLAB is not set and line 2301 (BUG_ON(flags &
~CREATE_MASK);) will oops us if we're using the flags in question.

2329            /*
2330             * Redzoning and user store require word alignment or
possibly larger.
2331             * Note this will be overridden by architecture or
caller mandated
2332             * alignment if either is greater than BYTES_PER_WORD.
2333             */
2334            if (flags & SLAB_STORE_USER)
2335                    ralign = BYTES_PER_WORD;
   0x00000000000038ae <+350>:   testq  $0x10000,0x20(%rsp)
   0x00000000000038b7 <+359>:   mov    $0x8,%eax
   0x00000000000038bc <+364>:   cmovne %rax,%r13

2336
2337            if (flags & SLAB_RED_ZONE) {
   0x00000000000038c0 <+368>:   testq  $0x400,0x20(%rsp)
   0x00000000000038c9 <+377>:   jne    0x3ba8 <kmem_cache_create+1112>

2338                    ralign = REDZONE_ALIGN;
   0x0000000000003bae <+1118>:  mov    $0x8,%r13d

2339                    /* If redzoning, ensure that the second redzone
is suitably
2340                     * aligned, by adjusting the object size
accordingly. */
2341                    size += REDZONE_ALIGN - 1;
   0x0000000000003ba8 <+1112>:  addq   $0x7,0x18(%rsp)

2342                    size &= ~(REDZONE_ALIGN - 1);
   0x0000000000003bb4 <+1124>:  andq   $0xfffffffffffffff8,0x18(%rsp)
   0x0000000000003bba <+1130>:  jmpq   0x38d7 <kmem_cache_create+391>
   0x0000000000003bbf <+1135>:  nop

2343            }
2344
2345            /* 2) arch mandated alignment */
2346            if (ralign < ARCH_SLAB_MINALIGN) {
   0x00000000000038d7 <+391>:   cmp    0x28(%rsp),%r13
   0x00000000000038e8 <+408>:   cmovb  0x28(%rsp),%r13

2347                    ralign = ARCH_SLAB_MINALIGN;
   0x00000000000038cf <+383>:   cmp    $0x7,%r13
   0x00000000000038d3 <+387>:   cmovbe %rax,%r13

2348            }
2349            /* 3) caller mandated alignment */
2350            if (ralign < align) {
2351                    ralign = align;
2352            }
2353            /* disable debug if necessary */
2354            if (ralign > __alignof__(unsigned long long))
2355                    flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
   0x00000000000038dc <+396>:   mov    0x20(%rsp),%rax
   0x00000000000038ee <+414>:   and    $0xfffffffffffefbff,%rax
   0x00000000000038f4 <+420>:   cmp    $0x9,%r13
   0x00000000000038f8 <+424>:   cmovb  0x20(%rsp),%rax
   0x0000000000003907 <+439>:   mov    %rax,0x20(%rsp)

2356            /*
2357             * 4) Store it.
2358             */
2359            align = ralign;

There's another little block that I can't illustrate since
CONFIG_PAGE_POISONING doesn't get enabled on my arch, but I've added it
into the patch as well.

Of note, in situations like this where I have a pre-process macro (i.e.,
DEBUG) that's defined to either zero or non-zero, my personal coding
style is to just use it directly in the the if() and let the optomizer
compile it out (as opposed to a #if/#endif block) but I was trying to
copy the coding style already in use.




[-- Attachment #2: 0001-compile-out-effectively-dead-code-from-kmem_cache_cr.patch --]
[-- Type: text/x-patch, Size: 2175 bytes --]

>From 9dfd9dec7a1d67265df88d75e55734d4ac049441 Mon Sep 17 00:00:00 2001
From: Daniel Santos <daniel.santos@pobox.com>
Date: Wed, 8 Feb 2012 21:26:49 -0600
Subject: compile out effectively dead code from kmem_cache_create
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="------------1.7.3.4"

This is a multi-part message in MIME format.
--------------1.7.3.4
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit

---
 mm/slab.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)


--------------1.7.3.4
Content-Type: text/x-patch; name="0001-compile-out-effectively-dead-code-from-kmem_cache_cr.patch"
Content-Transfer-Encoding: 8bit
Content-Disposition: attachment; filename="0001-compile-out-effectively-dead-code-from-kmem_cache_cr.patch"

diff --git a/mm/slab.c b/mm/slab.c
index f0bd785..1840a4a 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -2326,6 +2326,7 @@ kmem_cache_create (const char *name, size_t size, size_t align,
 		ralign = BYTES_PER_WORD;
 	}
 
+#if DEBUG
 	/*
 	 * Redzoning and user store require word alignment or possibly larger.
 	 * Note this will be overridden by architecture or caller mandated
@@ -2341,6 +2342,7 @@ kmem_cache_create (const char *name, size_t size, size_t align,
 		size += REDZONE_ALIGN - 1;
 		size &= ~(REDZONE_ALIGN - 1);
 	}
+#endif
 
 	/* 2) arch mandated alignment */
 	if (ralign < ARCH_SLAB_MINALIGN) {
@@ -2350,9 +2352,13 @@ kmem_cache_create (const char *name, size_t size, size_t align,
 	if (ralign < align) {
 		ralign = align;
 	}
+
+#if DEBUG
 	/* disable debug if necessary */
 	if (ralign > __alignof__(unsigned long long))
 		flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
+#endif
+
 	/*
 	 * 4) Store it.
 	 */
@@ -2442,7 +2448,7 @@ kmem_cache_create (const char *name, size_t size, size_t align,
 		slab_size =
 		    cachep->num * sizeof(kmem_bufctl_t) + sizeof(struct slab);
 
-#ifdef CONFIG_PAGE_POISONING
+#if DEBUG && defined(CONFIG_PAGE_POISONING)
 		/* If we're going to use the generic kernel_map_pages()
 		 * poisoning, then it's going to smash the contents of
 		 * the redzone and userword anyhow, so switch them off.

--------------1.7.3.4--

Re: mm/slab.c: remove effectively dead code from kmem_cache_create

[Andrew Morton]

On Wed, 08 Feb 2012 22:16:23 -0600
Daniel Santos <danielfsantos@att.net> wrote:

> I was examining slab.c when I noticed that there is code that will never
> be executed, but that the compiler probably wouldn't determine as such. 

Please cc the maintainer (Pekka) on slab patches.

Please include a Signed-off-by: on patches, as discussed in
Documentation/SubmittingPatches.

--- a/mm/slab.c~mm-slabc-remove-effectively-dead-code-from-kmem_cache_create
+++ a/mm/slab.c
@@ -2326,6 +2326,7 @@ kmem_cache_create (const char *name, siz
 		ralign = BYTES_PER_WORD;
 	}
 
+#if DEBUG
 	/*
 	 * Redzoning and user store require word alignment or possibly larger.
 	 * Note this will be overridden by architecture or caller mandated
@@ -2341,6 +2342,7 @@ kmem_cache_create (const char *name, siz
 		size += REDZONE_ALIGN - 1;
 		size &= ~(REDZONE_ALIGN - 1);
 	}
+#endif
 
 	/* 2) arch mandated alignment */
 	if (ralign < ARCH_SLAB_MINALIGN) {
@@ -2350,9 +2352,13 @@ kmem_cache_create (const char *name, siz
 	if (ralign < align) {
 		ralign = align;
 	}
+
+#if DEBUG
 	/* disable debug if necessary */
 	if (ralign > __alignof__(unsigned long long))
 		flags &= ~(SLAB_RED_ZONE | SLAB_STORE_USER);
+#endif
+
 	/*
 	 * 4) Store it.
 	 */
@@ -2442,7 +2448,7 @@ kmem_cache_create (const char *name, siz
 		slab_size =
 		    cachep->num * sizeof(kmem_bufctl_t) + sizeof(struct slab);
 
-#ifdef CONFIG_PAGE_POISONING
+#if DEBUG && defined(CONFIG_PAGE_POISONING)
 		/* If we're going to use the generic kernel_map_pages()
 		 * poisoning, then it's going to smash the contents of
 		 * the redzone and userword anyhow, so switch them off.
_


kmem_cache_create() is called extremely rarely, so the performance
benefit here is negligible.

We could presumably avoid two of those ifdefs by defining SLAB_RED_ZONE
and SLAB_STORE_USER to be zero if !defined(DEBUG).  Personally I think
that's a bit too subtle and would prefer the explicit ifdefs.

In my x86_64 allnoconfig build the patch reduces slab.o's text size
from 12859 bytes to 12812.  I'll let Pekka decide if that's worth it ;)

Re: mm/slab.c: remove effectively dead code from kmem_cache_create

[Pekka Enberg]

On Thu, 2012-02-09 at 14:39 -0800, Andrew Morton wrote:
> kmem_cache_create() is called extremely rarely, so the performance
> benefit here is negligible.
> 
> We could presumably avoid two of those ifdefs by defining SLAB_RED_ZONE
> and SLAB_STORE_USER to be zero if !defined(DEBUG).  Personally I think
> that's a bit too subtle and would prefer the explicit ifdefs.
> 
> In my x86_64 allnoconfig build the patch reduces slab.o's text size
> from 12859 bytes to 12812.  I'll let Pekka decide if that's worth it ;)

The text savings are worth it but I'd really prefer to see
include/linux/slab.h patched to define debugging flags as zero for
non-CONFIG_SLAB_DEBUG and let GCC eliminate the dead code for us.

			Pekka

Re: mm/slab.c: remove effectively dead code from kmem_cache_create

[Daniel Santos]

I can do that, but it will change the behavior slightly.  Currently, if
I write a module and call kmem_cache_create with SLAB_POISON or
SLAB_RED_ZONE when CONFIG_DEBUG_SLAB isn't set (mm/slab.c:2301)
BUG_ON(flags & ~CREATE_MASK) will oops me.  If we zero the flags when
CONFIG_DEBUG_SLAB isn't set in slab.h, it will silently ignore these
scenarios.  I'm new to Linux kernel programming, so I'm not yet familiar
with its general policies.  Let me know if this is acceptable behavior.

Also, I need to make sure that doing so has no other side effects elsewhere.

Daniel

On 02/10/2012 07:06 AM, Pekka Enberg wrote:
> On Thu, 2012-02-09 at 14:39 -0800, Andrew Morton wrote:
>> kmem_cache_create() is called extremely rarely, so the performance
>> benefit here is negligible.
>>
>> We could presumably avoid two of those ifdefs by defining SLAB_RED_ZONE
>> and SLAB_STORE_USER to be zero if !defined(DEBUG).  Personally I think
>> that's a bit too subtle and would prefer the explicit ifdefs.
>>
>> In my x86_64 allnoconfig build the patch reduces slab.o's text size
>> from 12859 bytes to 12812.  I'll let Pekka decide if that's worth it ;)
> The text savings are worth it but I'd really prefer to see
> include/linux/slab.h patched to define debugging flags as zero for
> non-CONFIG_SLAB_DEBUG and let GCC eliminate the dead code for us.
>
> 			Pekka
>
>