* mm/slab.c: remove effectively dead code from kmem_cache_create
@ 2012-02-09 4:16 Daniel Santos
2012-02-09 22:39 ` Andrew Morton
0 siblings, 1 reply; 4+ messages in thread
From: Daniel Santos @ 2012-02-09 4:16 UTC (permalink / raw)
To: torvalds; +Cc: linux-kernel
[-- 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--
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: mm/slab.c: remove effectively dead code from kmem_cache_create
2012-02-09 4:16 mm/slab.c: remove effectively dead code from kmem_cache_create Daniel Santos
@ 2012-02-09 22:39 ` Andrew Morton
2012-02-10 13:06 ` Pekka Enberg
0 siblings, 1 reply; 4+ messages in thread
From: Andrew Morton @ 2012-02-09 22:39 UTC (permalink / raw)
To: daniel.santos; +Cc: Daniel Santos, torvalds, linux-kernel, Pekka Enberg
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 ;)
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: mm/slab.c: remove effectively dead code from kmem_cache_create
2012-02-09 22:39 ` Andrew Morton
@ 2012-02-10 13:06 ` Pekka Enberg
2012-02-10 19:58 ` Daniel Santos
0 siblings, 1 reply; 4+ messages in thread
From: Pekka Enberg @ 2012-02-10 13:06 UTC (permalink / raw)
To: Andrew Morton; +Cc: daniel.santos, Daniel Santos, torvalds, linux-kernel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: mm/slab.c: remove effectively dead code from kmem_cache_create
2012-02-10 13:06 ` Pekka Enberg
@ 2012-02-10 19:58 ` Daniel Santos
0 siblings, 0 replies; 4+ messages in thread
From: Daniel Santos @ 2012-02-10 19:58 UTC (permalink / raw)
To: Pekka Enberg; +Cc: Andrew Morton, daniel.santos, torvalds, linux-kernel
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
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-02-10 20:04 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-02-09 4:16 mm/slab.c: remove effectively dead code from kmem_cache_create Daniel Santos
2012-02-09 22:39 ` Andrew Morton
2012-02-10 13:06 ` Pekka Enberg
2012-02-10 19:58 ` Daniel Santos
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).