[PATCH] remove redundant NULL pointer checks prior to calling kfree() in fs/nfsd/

[PATCH] remove redundant NULL pointer checks prior to calling kfree() in fs/nfsd/

[Jesper Juhl]

(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 <juhl-lkml@dif.dk>

--- 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;
 }
 

Re: [PATCH] remove redundant NULL pointer checks prior to calling kfree() in fs/nfsd/

[linux-os]

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 <juhl-lkml@dif.dk>
>
> --- 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 majordomo@vger.kernel.org
> 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.

Re: [PATCH] remove redundant NULL pointer checks prior to calling kfree() in fs/nfsd/

[Jesper Juhl]

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
 

Re: [PATCH] remove redundant NULL pointer checks prior to calling kfree() in fs/nfsd/

[Arjan van de Ven]

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...

Re: [PATCH] remove redundant NULL pointer checks prior to calling kfree() in fs/nfsd/

[Denis Vlasenko]

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

Re: [PATCH] remove redundant NULL pointer checks prior to calling kfree() in fs/nfsd/

[Pekka Enberg]

On Fri, 25 Mar 2005 17:34:29 -0500 (EST), linux-os
<linux-os@analogic.com> 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

Re: [PATCH] remove redundant NULL pointer checks prior to calling kfree() in fs/nfsd/

[Arjan van de Ven]

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.