PATCH: __bprm_mm_init(): remove uneeded goto

PATCH: __bprm_mm_init(): remove uneeded goto

[Luiz Fernando N. Capitulino]

It is only really used if insert_vm_struct() fails, we can inline it
and drop some (uneeded) lines of code.

Signed-off-by: Luiz Fernando N. Capitulino <lcapitulino@mandriva.com.br>

---
 fs/exec.c |   16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

Index: linux-2.6/fs/exec.c
===================================================================
--- linux-2.6.orig/fs/exec.c
+++ linux-2.6/fs/exec.c
@@ -232,13 +232,13 @@ static void flush_arg_page(struct linux_
 
 static int __bprm_mm_init(struct linux_binprm *bprm)
 {
-	int err = -ENOMEM;
+	int err;
 	struct vm_area_struct *vma = NULL;
 	struct mm_struct *mm = bprm->mm;
 
 	bprm->vma = vma = kmem_cache_zalloc(vm_area_cachep, GFP_KERNEL);
 	if (!vma)
-		goto err;
+		return -ENOMEM;
 
 	down_write(&mm->mmap_sem);
 	vma->vm_mm = mm;
@@ -257,7 +257,9 @@ static int __bprm_mm_init(struct linux_b
 	err = insert_vm_struct(mm, vma);
 	if (err) {
 		up_write(&mm->mmap_sem);
-		goto err;
+		kmem_cache_free(vm_area_cachep, vma);
+		bprm->vma = NULL;
+		return err;
 	}
 
 	mm->stack_vm = mm->total_vm = 1;
@@ -266,14 +268,6 @@ static int __bprm_mm_init(struct linux_b
 	bprm->p = vma->vm_end - sizeof(void *);
 
 	return 0;
-
-err:
-	if (vma) {
-		bprm->vma = NULL;
-		kmem_cache_free(vm_area_cachep, vma);
-	}
-
-	return err;
 }
 
 static bool valid_arg_len(struct linux_binprm *bprm, long len)


-- 
Luiz Fernando N. Capitulino

Re: PATCH: __bprm_mm_init(): remove uneeded goto

[Andrew Morton]

On Tue, 4 Nov 2008 14:03:14 -0200
"Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br> wrote:

> 
> It is only really used if insert_vm_struct() fails, we can inline it
> and drop some (uneeded) lines of code.
> 
> Signed-off-by: Luiz Fernando N. Capitulino <lcapitulino@mandriva.com.br>
> 
> ---
>  fs/exec.c |   16 +++++-----------
>  1 file changed, 5 insertions(+), 11 deletions(-)
> 
> Index: linux-2.6/fs/exec.c
> ===================================================================
> --- linux-2.6.orig/fs/exec.c
> +++ linux-2.6/fs/exec.c
> @@ -232,13 +232,13 @@ static void flush_arg_page(struct linux_
>  
>  static int __bprm_mm_init(struct linux_binprm *bprm)
>  {
> -	int err = -ENOMEM;
> +	int err;
>  	struct vm_area_struct *vma = NULL;
>  	struct mm_struct *mm = bprm->mm;
>  
>  	bprm->vma = vma = kmem_cache_zalloc(vm_area_cachep, GFP_KERNEL);
>  	if (!vma)
> -		goto err;
> +		return -ENOMEM;
>  
>  	down_write(&mm->mmap_sem);
>  	vma->vm_mm = mm;
> @@ -257,7 +257,9 @@ static int __bprm_mm_init(struct linux_b
>  	err = insert_vm_struct(mm, vma);
>  	if (err) {
>  		up_write(&mm->mmap_sem);
> -		goto err;
> +		kmem_cache_free(vm_area_cachep, vma);
> +		bprm->vma = NULL;
> +		return err;
>  	}
>  
>  	mm->stack_vm = mm->total_vm = 1;
> @@ -266,14 +268,6 @@ static int __bprm_mm_init(struct linux_b
>  	bprm->p = vma->vm_end - sizeof(void *);
>  
>  	return 0;
> -
> -err:
> -	if (vma) {
> -		bprm->vma = NULL;
> -		kmem_cache_free(vm_area_cachep, vma);
> -	}
> -
> -	return err;
>  }
>  
>  static bool valid_arg_len(struct linux_binprm *bprm, long len)

eek, that made the code worse.

Please avoid multiple `return' statements in functions.  The first one
you have there is OK - it occurs before any resources have been
allocated and it's right at the start of the function, etc.

But the second `return' is a no-no.  Doing this is a fairly common
source of locking errors and resource leaks as the code evolves.  And
what frequently happens is that someone changes the code to allocate
some new resource or to take some new lock and then they end up putting
an unlock or a free ahead of each and every `return' statement in the
function, which is daft.

It would be better to do this:

--- a/fs/exec.c~__bprm_mm_init-remove-uneeded-goto
+++ a/fs/exec.c
@@ -233,13 +233,13 @@ static void flush_arg_page(struct linux_
 
 static int __bprm_mm_init(struct linux_binprm *bprm)
 {
-	int err = -ENOMEM;
+	int err;
 	struct vm_area_struct *vma = NULL;
 	struct mm_struct *mm = bprm->mm;
 
 	bprm->vma = vma = kmem_cache_zalloc(vm_area_cachep, GFP_KERNEL);
 	if (!vma)
-		goto err;
+		return -ENOMEM;
 
 	down_write(&mm->mmap_sem);
 	vma->vm_mm = mm;
@@ -258,6 +258,8 @@ static int __bprm_mm_init(struct linux_b
 	err = insert_vm_struct(mm, vma);
 	if (err) {
 		up_write(&mm->mmap_sem);
+		kmem_cache_free(vm_area_cachep, vma);
+		bprm->vma = NULL;
 		goto err;
 	}
 
@@ -267,13 +269,7 @@ static int __bprm_mm_init(struct linux_b
 	bprm->p = vma->vm_end - sizeof(void *);
 
 	return 0;
-
 err:
-	if (vma) {
-		bprm->vma = NULL;
-		kmem_cache_free(vm_area_cachep, vma);
-	}
-
 	return err;
 }
 
_


But that's still not very good, because if someone later adds some new
lock-taking or resource-allocating to this function, how does their
error-handling path avoid duplicating the existing unlock and free?

So a better approach is this:

--- a/fs/exec.c~__bprm_mm_init-remove-uneeded-goto
+++ a/fs/exec.c
@@ -233,13 +233,13 @@ static void flush_arg_page(struct linux_
 
 static int __bprm_mm_init(struct linux_binprm *bprm)
 {
-	int err = -ENOMEM;
+	int err;
 	struct vm_area_struct *vma = NULL;
 	struct mm_struct *mm = bprm->mm;
 
 	bprm->vma = vma = kmem_cache_zalloc(vm_area_cachep, GFP_KERNEL);
 	if (!vma)
-		goto err;
+		return -ENOMEM;
 
 	down_write(&mm->mmap_sem);
 	vma->vm_mm = mm;
@@ -256,10 +256,8 @@ static int __bprm_mm_init(struct linux_b
 	vma->vm_flags = VM_STACK_FLAGS;
 	vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
 	err = insert_vm_struct(mm, vma);
-	if (err) {
-		up_write(&mm->mmap_sem);
+	if (err)
 		goto err;
-	}
 
 	mm->stack_vm = mm->total_vm = 1;
 	up_write(&mm->mmap_sem);
@@ -267,13 +265,10 @@ static int __bprm_mm_init(struct linux_b
 	bprm->p = vma->vm_end - sizeof(void *);
 
 	return 0;
-
 err:
-	if (vma) {
-		bprm->vma = NULL;
-		kmem_cache_free(vm_area_cachep, vma);
-	}
-
+	up_write(&mm->mmap_sem);
+	bprm->vma = NULL;
+	kmem_cache_free(vm_area_cachep, vma);
 	return err;
 }
 
_

Now, if someone later adds more resource-allocating or lock-taking to
this function they can use `goto err' on the error path.  Or they can
add a new err_unlocked: after the up_write() or whatever.

The above code now uses the most common pattern for a kernel
function.  One we've learned from hard experience!

Re: PATCH: __bprm_mm_init(): remove uneeded goto

[Luiz Fernando N. Capitulino]

Em Tue, 4 Nov 2008 10:57:07 -0800
Andrew Morton <akpm@linux-foundation.org> escreveu:

| The above code now uses the most common pattern for a kernel
| function.  One we've learned from hard experience!

 Wow, I have no words to thank you enough for this full explanation!

-- 
Luiz Fernando N. Capitulino

Re: PATCH: __bprm_mm_init(): remove uneeded goto

[Andrew Morton]

On Tue, 4 Nov 2008 17:14:14 -0200
"Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br> wrote:

> Em Tue, 4 Nov 2008 10:57:07 -0800
> Andrew Morton <akpm@linux-foundation.org> escreveu:
> 
> | The above code now uses the most common pattern for a kernel
> | function.  One we've learned from hard experience!
> 
>  Wow, I have no words to thank you enough for this full explanation!

How about "don't be so anal"?

I have more!

The code as we have it now looks like this:

foo()
{
	if (!(mem = kmalloc(...)))
		return -ENOMEM;

	down(sem);
	err = something();
	if (err)
		goto err;
	...
	return 0;
err:
	up(sem);
	kfree(mem);
	return err;
}

it is legitimate (and arguably better) to do:

foo()
{
	if (!(mem = kmalloc(...))) {
		err = -ENOMEM;
		goto err;
	}

	down(sem);
	err = something();
	if (err)
		goto err_locked;
	...
	return 0;
err_locked:
	up(sem);
	kfree(mem);
err:
	return err;
}

so we now have a single `return' point and we've maximised
maintainability.  But that's a fairly minor detail, and we often leave
those initial `return's in place.




Secondly, there are instruction-cache concerns.

This code:

foo()
{
	if (!(mem = kmalloc(...)))
		return -ENOMEM;

	down(sem);
	err = something();
	if (err) {
		up(sem);
		kfree(mem);
		goto err;
	}
	...
	return 0;
}

might cause the instructions for the `up' and the `kfree' to be laid
out in the middle of the function fastpath.  This will, on average,
cause the function to consume additional instruction cache lines.

Doing this:

foo()
{
	if (!(mem = kmalloc(...)))
		return -ENOMEM;

	down(sem);
	err = something();
	if (err)
		goto err;
	...
	return 0;
err:
	up(sem);
	kfree(mem);
	return err;
}

will, we hope, help the compiler to move the rarely-executed error-path
instructions out of line, thus maybe reducing the function's average
icache footprint.  The fastpath now spans a smaller address range.


We used to do this trick a *lot* in the kernel (back in the 2.2 days?)
for this performance reason.  Nowdays gcc is a lot more complex and we
hope that it can sometimes work these things out for itself and we hope
that `unlikely' might cause the compiler to move the unlikely code out
of line.  But I don't know how successful the compiler is at doing
this, and it'll be dependent upon the gcc version, the wind direction,
etc.

As long as it doesn't muck up the code readability, I expect that it's
still beneficial to provide this layout hint to the compiler.  A bit of
poking around in the .s files would be instructive..

Re: PATCH: __bprm_mm_init(): remove uneeded goto

[Arjan van de Ven]

On Tue, 4 Nov 2008 11:47:03 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:

> We used to do this trick a *lot* in the kernel (back in the 2.2 days?)
> for this performance reason.  Nowdays gcc is a lot more complex and we
> hope that it can sometimes work these things out for itself and we
> hope that `unlikely' might cause the compiler to move the unlikely
> code out of line.  But I don't know how successful the compiler is at
> doing this, and it'll be dependent upon the gcc version, the wind
> direction, etc.
> 

as far as I know, gcc tends to consider NULL pointer checks as unlikely
by default.

(but we can always tell it so if we think we know better..)

-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org