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.
On Wed, 08 Feb 2012 22:16:23 -0600
Daniel Santos <[email protected]> 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 ;)
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
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
>
>