Skip to contentSkip to conversation

Eliminate some unnecessary gotos in tcp v4 hash handling

PATCH In review Andi Kleen 2 participants 3 messages Apr 5, 2007 – Apr 21, 2007

Subsystems (3)
lore
Share & tools
lore

Andi Kleen

author
#0 lore raw

PATCH 1/1 Eliminate some unnecessary gotos in tcp v4 hash handling

The compiler eliminates them anyways and this makes the code easier to read and shorter.

net/ipv4/tcp_ipv4.c +5 -14
Index: linux-2.6.21-rc3-net/net/ipv4/tcp_ipv4.c
===================================================================
@@ -2052,8 +2052,7 @@ static void *established_get_first(struc
 			if (sk->sk_family != st->family) {
 				continue;
 			}
-			rc = sk;
-			goto out;
+			return sk;
 		}
 		st->state = TCP_SEQ_STATE_TIME_WAIT;
 		inet_twsk_for_each(tw, node,
@@ -2061,13 +2060,11 @@ static void *established_get_first(struc
 			if (tw->tw_family != st->family) {
 				continue;
 			}
-			rc = tw;
-			goto out;
+			return tw;
 		}
 		read_unlock(&tcp_hashinfo.ehash[st->bucket].lock);
 		st->state = TCP_SEQ_STATE_ESTABLISHED;
 	}
-out:
 	return rc;
 }
 
@@ -2088,10 +2085,8 @@ get_tw:
 		while (tw && tw->tw_family != st->family) {
 			tw = tw_next(tw);
 		}
-		if (tw) {
-			cur = tw;
-			goto out;
-		}
+		if (tw)
+			return tw; 
 		read_unlock(&tcp_hashinfo.ehash[st->bucket].lock);
 		st->state = TCP_SEQ_STATE_ESTABLISHED;
 
@@ -2111,16 +2106,12 @@ get_tw:
 
 	sk_for_each_from(sk, node) {
 		if (sk->sk_family == st->family)
-			goto found;
+			return sk; 
 	}
 
 	st->state = TCP_SEQ_STATE_TIME_WAIT;
 	tw = tw_head(&tcp_hashinfo.ehash[st->bucket].twchain);
 	goto get_tw;
-found:
-	cur = sk;
-out:
-	return cur;
 }
 
 static void *established_get_idx(struct seq_file *seq, loff_t pos)
maintainer
#1 lore raw

From: Andi Kleen <[email protected]>
Date: Thu, 5 Apr 2007 12:48:44 +0200

Quoted 4 lines

The compiler eliminates them anyways and this makes the code easier to read and shorter.

It depends upon who you ask :-)

For someone trying to verify "all code paths lead to foo and therefore release resources and drop locks" it's always better to use a goto to some common unwind handler at the end of the function, regardless of what the compiler does with it.

When you're doing such an audit, return statements in the middle of the function are red flags and make the audit more difficult. :-)

But in these cases it's just straight returns and there are no resources to release so I'll apply this patch, thanks!

maintainer
#2 lore raw
In reply to Andi Kleen

From: Andi Kleen <[email protected]>
Date: Thu, 5 Apr 2007 12:48:44 +0200

Quoted 5 lines

The compiler eliminates them anyways and this makes the code easier to read and shorter.

Actually I won't apply this patch, ever, -ENOCOMPILE.

Andi, please spare me with these patches if you won't even bother trying to compile them. It doesn't matter how easy to read the code is if it doesn't build. :-/

I found this build failure just by reviewing your patch for correctness, I didn't even need to give it to the compiler to see that you left the "cur = NULL; goto out;" case in there yet removed the "out" label.

Don't bother posting the fixed version, I'll ignore it. You just basically proved how pointless this patch is, and how little you yourself even care about it.