Discussion:
[PATCH] remove redundant NULL pointer checks prior to calling kfree() in fs/nfsd/
(too old to reply)
linux-os
2005-03-25 23:01:02 UTC
Permalink
(please keep me on CC)
checking for NULL before calling kfree() is redundant and needlessly
enlarges the kernel image, let's get rid of those checks.
Hardly. ORing a value with itself and jumping on condition is
real cheap compared with pushing a value into the stack and
calling a function. This is NOT a good change if you want
performance. You really should reconsider this activity. It
is quite counter-productive.
--- linux-2.6.12-rc1-mm3-orig/fs/nfsd/export.c 2005-03-21 23:12:41.000000000 +0100
+++ linux-2.6.12-rc1-mm3/fs/nfsd/export.c 2005-03-25 22:48:11.000000000 +0100
@@ -189,8 +189,7 @@ static int expkey_parse(struct cache_det
if (dom)
auth_domain_put(dom);
- if (buf)
- kfree(buf);
+ kfree(buf);
return err;
}
@@ -426,8 +425,7 @@ static int svc_export_parse(struct cache
path_release(&nd);
if (dom)
auth_domain_put(dom);
- if (buf)
- kfree(buf);
+ kfree(buf);
return err;
}
--- linux-2.6.12-rc1-mm3-orig/fs/nfsd/nfs4xdr.c 2005-03-25 15:28:59.000000000 +0100
+++ linux-2.6.12-rc1-mm3/fs/nfsd/nfs4xdr.c 2005-03-25 22:49:53.000000000 +0100
@@ -151,8 +151,7 @@ u32 *read_buf(struct nfsd4_compoundargs
if (nbytes <= sizeof(argp->tmp))
p = argp->tmp;
else {
- if (argp->tmpp)
- kfree(argp->tmpp);
+ kfree(argp->tmpp);
p = argp->tmpp = kmalloc(nbytes, GFP_KERNEL);
if (!p)
return NULL;
@@ -2474,10 +2473,8 @@ void nfsd4_release_compoundargs(struct n
kfree(args->ops);
args->ops = args->iops;
}
- if (args->tmpp) {
- kfree(args->tmpp);
- args->tmpp = NULL;
- }
+ kfree(args->tmpp);
+ args->tmpp = NULL;
while (args->to_free) {
struct tmpbuf *tb = args->to_free;
args->to_free = tb->next;
--- linux-2.6.12-rc1-mm3-orig/fs/nfsd/nfscache.c 2005-03-21 23:12:41.000000000 +0100
+++ linux-2.6.12-rc1-mm3/fs/nfsd/nfscache.c 2005-03-25 22:50:14.000000000 +0100
@@ -93,8 +93,7 @@ nfsd_cache_shutdown(void)
cache_disabled = 1;
- if (hash_list)
- kfree (hash_list);
+ kfree(hash_list);
hash_list = NULL;
}
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Cheers,
Dick Johnson
Penguin : Linux version 2.6.11 on an i686 machine (5537.79 BogoMips).
Notice : All mail here is now cached for review by Dictator Bush.
98.36% of all statistics are fiction.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/



-------------------------------------------------------------------------------
Achtung: diese Newsgruppe ist eine unidirektional gegatete Mailingliste.
Antworten nur per Mail an die im Reply-To-Header angegebene Adresse.
Fragen zum Gateway -> ***@inka.de.
-------------------------------------------------------------------------------
Jesper Juhl
2005-03-25 23:12:43 UTC
Permalink
Post by linux-os
(please keep me on CC)
checking for NULL before calling kfree() is redundant and needlessly
enlarges the kernel image, let's get rid of those checks.
Hardly. ORing a value with itself and jumping on condition is
real cheap compared with pushing a value into the stack and
calling a function. This is NOT a good change if you want
performance. You really should reconsider this activity. It
is quite counter-productive.
Let's have a look - at fs/nfsd/export.o for example.

Size first.
Without my patch :
$ ls -l fs/nfsd/export.o
-rw-r--r-- 1 juhl users 97144 2005-03-25 23:58 fs/nfsd/export.o
With my patch :
$ ls -l fs/nfsd/export.o
-rw-r--r-- 1 juhl users 97092 2005-03-25 23:59 fs/nfsd/export.o

That's our first bennefit - 52 bytes saved - not much, but still some.

Now let's take a look at the code gcc generates for one of the functions -
expkey_parse for example. Here's a diff -u of an objdump -D of export.o
cut down to just the bit for expkey_parse :

--- func-without-patch 2005-03-26 00:02:47.000000000 +0100
+++ func-with-patch 2005-03-26 00:03:26.000000000 +0100
@@ -8,7 +8,7 @@
13b: 81 ec d0 00 00 00 sub $0xd0,%esp
141: 89 95 40 ff ff ff mov %edx,0xffffff40(%ebp)
147: 80 7c 0a ff 0a cmpb $0xa,0xffffffff(%edx,%ecx,1)
- 14c: 0f 85 2b 02 00 00 jne 37d <expkey_parse+0x24d>
+ 14c: 0f 85 27 02 00 00 jne 379 <expkey_parse+0x249>
152: c6 44 0a ff 00 movb $0x0,0xffffffff(%edx,%ecx,1)
157: a1 64 00 00 00 mov 0x64,%eax
15c: ba d0 00 00 00 mov $0xd0,%edx
@@ -17,7 +17,7 @@
168: 89 c3 mov %eax,%ebx
16a: c7 85 30 ff ff ff f4 movl $0xfffffff4,0xffffff30(%ebp)
171: ff ff ff
- 174: 0f 84 fd 01 00 00 je 377 <expkey_parse+0x247>
+ 174: 0f 84 f2 01 00 00 je 36c <expkey_parse+0x23c>
17a: 89 c2 mov %eax,%edx
17c: 8d 85 40 ff ff ff lea 0xffffff40(%ebp),%eax
182: b9 00 10 00 00 mov $0x1000,%ecx
@@ -52,7 +52,7 @@
208: 80 38 00 cmpb $0x0,(%eax)
20b: 0f 85 46 01 00 00 jne 357 <expkey_parse+0x227>
211: f6 05 00 00 00 00 04 testb $0x4,0x0
- 218: 0f 85 6a 01 00 00 jne 388 <expkey_parse+0x258>
+ 218: 0f 85 66 01 00 00 jne 384 <expkey_parse+0x254>
21e: 83 ff 02 cmp $0x2,%edi
221: 0f 8f 30 01 00 00 jg 357 <expkey_parse+0x227>
227: 8d 85 40 ff ff ff lea 0xffffff40(%ebp),%eax
@@ -134,22 +134,20 @@
35f: 74 0b je 36c <expkey_parse+0x23c>
361: 8b 85 34 ff ff ff mov 0xffffff34(%ebp),%eax
367: e8 fc ff ff ff call 368 <expkey_parse+0x238>
- 36c: 85 db test %ebx,%ebx
- 36e: 74 07 je 377 <expkey_parse+0x247>
- 370: 89 d8 mov %ebx,%eax
- 372: e8 fc ff ff ff call 373 <expkey_parse+0x243>
- 377: 8b 85 30 ff ff ff mov 0xffffff30(%ebp),%eax
- 37d: 81 c4 d0 00 00 00 add $0xd0,%esp
- 383: 5b pop %ebx
- 384: 5e pop %esi
- 385: 5f pop %edi
- 386: c9 leave
- 387: c3 ret
- 388: 89 7c 24 04 mov %edi,0x4(%esp)
- 38c: c7 04 24 03 00 00 00 movl $0x3,(%esp)
- 393: e8 fc ff ff ff call 394 <expkey_parse+0x264>
- 398: e9 81 fe ff ff jmp 21e <expkey_parse+0xee>
- 39d: 8d 76 00 lea 0x0(%esi),%esi
+ 36c: 89 d8 mov %ebx,%eax
+ 36e: e8 fc ff ff ff call 36f <expkey_parse+0x23f>
+ 373: 8b 85 30 ff ff ff mov 0xffffff30(%ebp),%eax
+ 379: 81 c4 d0 00 00 00 add $0xd0,%esp
+ 37f: 5b pop %ebx
+ 380: 5e pop %esi
+ 381: 5f pop %edi
+ 382: c9 leave
+ 383: c3 ret
+ 384: 89 7c 24 04 mov %edi,0x4(%esp)
+ 388: c7 04 24 03 00 00 00 movl $0x3,(%esp)
+ 38f: e8 fc ff ff ff call 390 <expkey_parse+0x260>
+ 394: e9 85 fe ff ff jmp 21e <expkey_parse+0xee>
+ 399: 8d b4 26 00 00 00 00 lea 0x0(%esi),%esi
3a0: 89 5c 24 04 mov %ebx,0x4(%esp)
3a4: c7 04 24 16 00 00 00 movl $0x16,(%esp)
3ab: e8 fc ff ff ff call 3ac <expkey_parse+0x27c>

This is not too bad, but I've seen a lot worse, see this one for a gross
example : http://www.ussg.iu.edu/hypermail/linux/kernel/0503.2/1050.html
--
Jesper Juhl

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/



-------------------------------------------------------------------------------
Achtung: diese Newsgruppe ist eine unidirektional gegatete Mailingliste.
Antworten nur per Mail an die im Reply-To-Header angegebene Adresse.
Fragen zum Gateway -> ***@inka.de.
-------------------------------------------------------------------------------
Arjan van de Ven
2005-03-26 08:42:20 UTC
Permalink
Post by linux-os
(please keep me on CC)
checking for NULL before calling kfree() is redundant and needlessly
enlarges the kernel image, let's get rid of those checks.
Hardly. ORing a value with itself and jumping on condition is
real cheap compared with pushing a value into the stack
which century are you from?
"jumping on condition" can easily be 100+ cycles, depending on how
effective the branch predictor is. Pushing a value onto the stack otoh
is half a cycle.

Your argument was right probably in 1994, when cpus didn't do
speculation and out of order execution...

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/



-------------------------------------------------------------------------------
Achtung: diese Newsgruppe ist eine unidirektional gegatete Mailingliste.
Antworten nur per Mail an die im Reply-To-Header angegebene Adresse.
Fragen zum Gateway -> ***@inka.de.
-------------------------------------------------------------------------------
Denis Vlasenko
2005-03-27 12:48:43 UTC
Permalink
Post by Arjan van de Ven
Post by linux-os
(please keep me on CC)
checking for NULL before calling kfree() is redundant and needlessly
enlarges the kernel image, let's get rid of those checks.
Hardly. ORing a value with itself and jumping on condition is
real cheap compared with pushing a value into the stack
which century are you from?
"jumping on condition" can easily be 100+ cycles, depending on how
effective the branch predictor is. Pushing a value onto the stack otoh
is half a cycle.
linux-os is right because kfree does NULL check with exactly
the same code sequence, test and branch:

# objdump -d mm/slab.o
...
000012ef <kfree>:
12ef: 55 push %ebp
12f0: 89 e5 mov %esp,%ebp
12f2: 57 push %edi
12f3: 56 push %esi
12f4: 53 push %ebx
12f5: 51 push %ecx
12f6: 8b 7d 08 mov 0x8(%ebp),%edi
12f9: 85 ff test %edi,%edi
12fb: 74 46 je 1343 <kfree+0x54>
...
...
...
1343: 8d 65 f4 lea 0xfffffff4(%ebp),%esp
1346: 5b pop %ebx
1347: 5e pop %esi
1348: 5f pop %edi
1349: 5d pop %ebp
134a: c3 ret

So kfree(p) indeed will spend time for doing a call,
for test-and-branch, *and* finally for ret,
while if(p) kfree(p) will do test-and-branch first
and won't do call/ret if p==NULL.

However, if p is not NULL, if(p) kfree(p) does:
1) test-and-branch (not taken)
2) call
3) another test-and-branch (not taken)!

I conclude that if(p) kfree(p) makes sense only if:
a) p is more often NULL than not, and
b) it's in the hot path (you don't want to save on code size)

Since (a) is not typical, I think Jesper's cleanups are ok.
--
vda

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/



-------------------------------------------------------------------------------
Achtung: diese Newsgruppe ist eine unidirektional gegatete Mailingliste.
Antworten nur per Mail an die im Reply-To-Header angegebene Adresse.
Fragen zum Gateway -> ***@inka.de.
-------------------------------------------------------------------------------
Arjan van de Ven
2005-03-27 14:33:54 UTC
Permalink
Post by Denis Vlasenko
Post by Arjan van de Ven
Post by linux-os
(please keep me on CC)
checking for NULL before calling kfree() is redundant and needlessly
enlarges the kernel image, let's get rid of those checks.
Hardly. ORing a value with itself and jumping on condition is
real cheap compared with pushing a value into the stack
which century are you from?
"jumping on condition" can easily be 100+ cycles, depending on how
effective the branch predictor is. Pushing a value onto the stack otoh
is half a cycle.
linux-os is right because kfree does NULL check with exactly
I know it does. The thing is that you have *two* chances to get a branch
mispredict now.

Now if kfree did NOT do the if but move it always to the caller, then
you have somewhat different dynamics (since you then always if the
conditional jump once no matter) but that is not the case.
Post by Denis Vlasenko
a) p is more often NULL than not, and
b) it's in the hot path (you don't want to save on code size)
Since (a) is not typical, I think Jesper's cleanups are ok.
note that to gain from teh branch predictor "more often than not"
probably needs to be in the 20:1 ratio to actually gain.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/



-------------------------------------------------------------------------------
Achtung: diese Newsgruppe ist eine unidirektional gegatete Mailingliste.
Antworten nur per Mail an die im Reply-To-Header angegebene Adresse.
Fragen zum Gateway -> ***@inka.de.
-------------------------------------------------------------------------------
Pekka Enberg
2005-03-27 14:02:24 UTC
Permalink
On Fri, 25 Mar 2005 17:34:29 -0500 (EST), linux-os
You really should reconsider this activity. It is quite counter-productive.
No it's not. NULL is checked twice without Jesper's cleanups. If
kfree() calls are really that performance sensitive, just make it
inline and GCC will take care of it.

Pekka
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/



-------------------------------------------------------------------------------
Achtung: diese Newsgruppe ist eine unidirektional gegatete Mailingliste.
Antworten nur per Mail an die im Reply-To-Header angegebene Adresse.
Fragen zum Gateway -> ***@inka.de.
-------------------------------------------------------------------------------
Loading...