[PATCH] Replaces two GOTO statements with one IF_ELSE statement in /fs/open.c

[PATCH] Replaces two GOTO statements with one IF_ELSE statement in /fs/open.c

[Telemaque Ndizihiwe]

This Patch replaces two GOTO statements and their corresponding LABELs
with one IF_ELSE statement in /fs/open.c, 2.6.12 kernel.
The patch keeps the same implementation of sys_open system call, it only 
makes the code smaller and easy to read.

Signed-off-by: Telemaque Ndizihiwe <telendiz@eircom.net>


--- linux-2.6.12/fs/open.c.orig	2005-06-20 15:15:52.000000000 +0100
+++ linux-2.6.12/fs/open.c	2005-06-20 15:38:47.580923552 +0100
@@ -945,19 +945,16 @@ asmlinkage long sys_open(const char __us
  		if (fd >= 0) {
  			struct file *f = filp_open(tmp, flags, mode);
  			error = PTR_ERR(f);
-			if (IS_ERR(f))
-				goto out_error;
-			fd_install(fd, f);
+			if (IS_ERR(f)) {
+				put_unused_fd(fd);
+				fd = error;
+			} else {
+				fd_install(fd, f);
+			}
  		}
-out:
  		putname(tmp);
  	}
  	return fd;
-
-out_error:
-	put_unused_fd(fd);
-	fd = error;
-	goto out;
  }
  EXPORT_SYMBOL_GPL(sys_open);

Re: [PATCH] Replaces two GOTO statements with one IF_ELSE statement in /fs/open.c

[Jeff Garzik]

Telemaque Ndizihiwe wrote:
> This Patch replaces two GOTO statements and their corresponding LABELs
> with one IF_ELSE statement in /fs/open.c, 2.6.12 kernel.
> The patch keeps the same implementation of sys_open system call, it only 
> makes the code smaller and easy to read.
> 
> Signed-off-by: Telemaque Ndizihiwe <telendiz@eircom.net>


If you don't like goto, don't read kernel code!

	Jeff

Re: [PATCH] Replaces two GOTO statements with one IF_ELSE statement in /fs/open.c

[Christoph Lameter]

On Mon, 20 Jun 2005, Jeff Garzik wrote:

> If you don't like goto, don't read kernel code!

But his patch also cleans up a code quit a bit.

Re: [PATCH] Replaces two GOTO statements with one IF_ELSE statement in /fs/open.c

[Joel Schopp]

Christoph Lameter wrote:

> On Mon, 20 Jun 2005, Jeff Garzik wrote:
> 
> 
>>If you don't like goto, don't read kernel code!
> 
> 
> But his patch also cleans up a code quit a bit.

As a wider question, what is the practice for accepting patches without 
functional changes that simply clean up code and make it look better?

BTW, I also agree that the gotos in this case keep the normal code flow 
cleaner, while making the exceptions isolated well.

Re: [PATCH] Replaces two GOTO statements with one IF_ELSE statement in /fs/open.c

[Andrew Morton]

Christoph Lameter <christoph@lameter.com> wrote:
>
> On Mon, 20 Jun 2005, Jeff Garzik wrote:
> 
> > If you don't like goto, don't read kernel code!
> 
> But his patch also cleans up a code quit a bit.

Yes, it is cleaner that way.

The old trick to make the error-handling code out-of-line shouldn't be
needed nowadays - IS_ERR uses unlikely(), which is supposed to handle that
stuff.

We can go a bit further and remove local variable `error'.

Code size is the same before and after, and the assembly very similar.

diff -puN fs/open.c~sys_open-cleanup fs/open.c
--- 25/fs/open.c~sys_open-cleanup	Mon Jun 20 13:31:54 2005
+++ 25-akpm/fs/open.c	Mon Jun 20 13:33:37 2005
@@ -934,7 +934,7 @@ EXPORT_SYMBOL(fd_install);
 asmlinkage long sys_open(const char __user * filename, int flags, int mode)
 {
 	char * tmp;
-	int fd, error;
+	int fd;
 
 	if (force_o_largefile())
 		flags |= O_LARGEFILE;
@@ -945,20 +945,16 @@ asmlinkage long sys_open(const char __us
 		fd = get_unused_fd();
 		if (fd >= 0) {
 			struct file *f = filp_open(tmp, flags, mode);
-			error = PTR_ERR(f);
-			if (IS_ERR(f))
-				goto out_error;
-			fd_install(fd, f);
+			if (IS_ERR(f)) {
+				put_unused_fd(fd);
+				fd = PTR_ERR(f);
+			} else {
+				fd_install(fd, f);
+			}
 		}
-out:
 		putname(tmp);
 	}
 	return fd;
-
-out_error:
-	put_unused_fd(fd);
-	fd = error;
-	goto out;
 }
 EXPORT_SYMBOL_GPL(sys_open);
 
_

Re: [PATCH] Replaces two GOTO statements with one IF_ELSE statement in /fs/open.c

[Martin Waitz]

[-- Attachment #1: Type: text/plain, Size: 550 bytes --]

hoi :)

On Mon, Jun 20, 2005 at 01:38:00PM -0700, Andrew Morton wrote:
> Yes, it is cleaner that way.

Well, I don't think so...

> The old trick to make the error-handling code out-of-line shouldn't be
> needed nowadays - IS_ERR uses unlikely(), which is supposed to handle that
> stuff.

IMHO out-of-line error handling improves readability because you have a
clear boundary between real functionality and error handling.  If both
are mixed up you have to look longer at the code to understand the
control-flow.

-- 
Martin Waitz

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Replaces two GOTO statements with one IF_ELSE statement in /fs/open.c

[Linus Torvalds]

On Tue, 21 Jun 2005, Martin Waitz wrote:
> 
> On Mon, Jun 20, 2005 at 01:38:00PM -0700, Andrew Morton wrote:
> > Yes, it is cleaner that way.
> 
> Well, I don't think so...
> 
> > The old trick to make the error-handling code out-of-line shouldn't be
> > needed nowadays - IS_ERR uses unlikely(), which is supposed to handle that
> > stuff.
> 
> IMHO out-of-line error handling improves readability because you have a
> clear boundary between real functionality and error handling.  If both
> are mixed up you have to look longer at the code to understand the
> control-flow.

I personally tend to prefer out-of-line error handling when there are 
multiple error paths, or the function is even slightly more complex. 

In this case the in-line error handling seems warranted, though. Doing it 
out-of-line just doesn't buy you anything in this case.

		Linus

Re: [PATCH] Replaces two GOTO statements with one IF_ELSE statement in /fs/open.c

[Richard B. Johnson]

On Mon, 20 Jun 2005, Telemaque Ndizihiwe wrote:

>
> This Patch replaces two GOTO statements and their corresponding LABELs
> with one IF_ELSE statement in /fs/open.c, 2.6.12 kernel.
> The patch keeps the same implementation of sys_open system call, it only
> makes the code smaller and easy to read.
>

Did you check and benchmark the resulting code? The goto statements
are never because the kernel developers didn't know how to use
other constructs, you know.

The goto statements are so that the normal path continues
straight through the code while the exceptions take the jumps.


> Signed-off-by: Telemaque Ndizihiwe <telendiz@eircom.net>
>
>
> --- linux-2.6.12/fs/open.c.orig	2005-06-20 15:15:52.000000000 +0100
> +++ linux-2.6.12/fs/open.c	2005-06-20 15:38:47.580923552 +0100
> @@ -945,19 +945,16 @@ asmlinkage long sys_open(const char __us
>  		if (fd >= 0) {
>  			struct file *f = filp_open(tmp, flags, mode);
>  			error = PTR_ERR(f);
> -			if (IS_ERR(f))
> -				goto out_error;
> -			fd_install(fd, f);
> +			if (IS_ERR(f)) {
> +				put_unused_fd(fd);
> +				fd = error;
> +			} else {
> +				fd_install(fd, f);
> +			}
>  		}
> -out:
>  		putname(tmp);
>  	}
>  	return fd;
> -
> -out_error:
> -	put_unused_fd(fd);
> -	fd = error;
> -	goto out;
>  }
>  EXPORT_SYMBOL_GPL(sys_open);
>
> -
> 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.9 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] Replaces two GOTO statements with one IF_ELSE statement in /fs/open.c

[Mitchell Blank Jr]

Richard B. Johnson wrote:
> Did you check and benchmark the resulting code? The goto statements
> are never because the kernel developers didn't know how to use
> other constructs, you know.
> 
> The goto statements are so that the normal path continues
> straight through the code while the exceptions take the jumps.

Yes, but I think in this case it's OK.  IS_ERR() already includes an
"unlikely()" marking so gcc *should* be able to do the right thing.
The goto is probably just a remnant from before unlikely() existed.
There are a lot of them - cleaning them up is a good thing.

That said, it'd probably be good to look at the generated assembly in each
case to make sure it still looks good.  That'd be a lot more useful than
trying to benchmark a tiny micro-optimizaiton in open()  It's generally
nearly impossible to benchmark these sorts of things anyway since when
you're running a simple benchmark the whole function will be in Icache and
the branch predictor history so it'll run the same speed either way.

The advantage to keeping the fast-path straight through is that you consume
the minimum number of lines in the Icache and the branch will get predicted
correctly even without history (since the first-level branch prediction
heuristic is always "branches backward are taken, branches forward are not")

-Mitch

Re: [PATCH] Replaces two GOTO statements with one IF_ELSE statement in /fs/open.c

[Bill Davidsen]

Telemaque Ndizihiwe wrote:
> 
> This Patch replaces two GOTO statements and their corresponding LABELs
> with one IF_ELSE statement in /fs/open.c, 2.6.12 kernel.
> The patch keeps the same implementation of sys_open system call, it only 
> makes the code smaller and easy to read.

Full credit with making the code more readable and maintainable. I'd 
like to look at the code generated to see if you have introduced a 
slowdown, however. My personal preference is for readable code and let 
the compiler sort it out, but I know there are people who would trade 
ugly code to save a cycle even in seldom-used code paths.

You might like to see if "unlikely" halps the code, I won't have a 
recent compiler until tomorrow night, since I'm locked into AS3.0 (gcc 
3.2.3) today and tomorrow. Oops, I do have 3.3.2 on my lappie, but 
that's still old.

> 
> Signed-off-by: Telemaque Ndizihiwe <telendiz@eircom.net>
> 
> 
> --- linux-2.6.12/fs/open.c.orig    2005-06-20 15:15:52.000000000 +0100
> +++ linux-2.6.12/fs/open.c    2005-06-20 15:38:47.580923552 +0100
> @@ -945,19 +945,16 @@ asmlinkage long sys_open(const char __us
>          if (fd >= 0) {
>              struct file *f = filp_open(tmp, flags, mode);
>              error = PTR_ERR(f);
> -            if (IS_ERR(f))
> -                goto out_error;
> -            fd_install(fd, f);
> +            if (IS_ERR(f)) {
> +                put_unused_fd(fd);
> +                fd = error;
> +            } else {
> +                fd_install(fd, f);
> +            }
>          }
> -out:
>          putname(tmp);
>      }
>      return fd;
> -
> -out_error:
> -    put_unused_fd(fd);
> -    fd = error;
> -    goto out;
>  }
>  EXPORT_SYMBOL_GPL(sys_open);
> 

Re: [PATCH] Replaces two GOTO statements with one IF_ELSE statement in /fs/open.c

[Andi Kleen]

Andrew Morton <akpm@osdl.org> writes:
> 
> The old trick to make the error-handling code out-of-line shouldn't be
> needed nowadays - IS_ERR uses unlikely(), which is supposed to handle that
> stuff.

In fact it doesn't even work anymore with -freorder-blocks (which is
default on i386). Without unlikely gcc 3.3/3.4 will happily move the out of
line block back. I was told that newer gcc gives a bit more value
to user gotos, but it doesn't help on the older compilers.

-Andi (who also thinks there are too many gotos in the kernel and 
many should be cleaned up)