(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.
Signed-off-by: Jesper Juhl <[email protected]>
--- 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
out:
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;
}
On Fri, 25 Mar 2005, Jesper Juhl wrote:
> (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.
> Signed-off-by: Jesper Juhl <[email protected]>
>
> --- 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
> out:
> 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
> the body of a message to [email protected]
> 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.
On Fri, 25 Mar 2005, linux-os wrote:
> On Fri, 25 Mar 2005, Jesper Juhl wrote:
>
> > (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
On Fri, 2005-03-25 at 17:34 -0500, linux-os wrote:
> On Fri, 25 Mar 2005, Jesper Juhl wrote:
>
> > (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...
On Saturday 26 March 2005 10:34, Arjan van de Ven wrote:
> On Fri, 2005-03-25 at 17:34 -0500, linux-os wrote:
> > On Fri, 25 Mar 2005, Jesper Juhl wrote:
> >
> > > (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
On Fri, 25 Mar 2005 17:34:29 -0500 (EST), linux-os
<[email protected]> wrote:
> 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
On Sun, 2005-03-27 at 15:45 +0300, Denis Vlasenko wrote:
> On Saturday 26 March 2005 10:34, Arjan van de Ven wrote:
> > On Fri, 2005-03-25 at 17:34 -0500, linux-os wrote:
> > > On Fri, 25 Mar 2005, Jesper Juhl wrote:
> > >
> > > > (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:
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.
> 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.
note that to gain from teh branch predictor "more often than not"
probably needs to be in the 20:1 ratio to actually gain.