optimize check in port-allocation code

Hi,

In the 2.6.11 code, I found this:
static int tcp_v6_get_port(struct sock *sk, unsigned short snum) also in:
static int tcp_v4_get_port(struct sock *sk, unsigned short snum) {
...

        if (snum == 0) {
                int low = sysctl_local_port_range[0];
                int high = sysctl_local_port_range[1];

                spin_lock(&tcp_portalloc_lock);
		rover = tcp_port_rover;
                do {    rover++;
                        if ((rover < low) || (rover > high))
                                rover = low;

That 'rover < low' check is redundant?
ints are bigger then the maximum portnumber (65535) so when rover++ gets too high, the check for 'rover > high' will truncate it to low (in the next line) waaay before the int itself wraps. It is needed because tcp_port_rover might be < low before the function starts, in that case the check for <low can be taken out of the loop.

Patch:

diff -uNr net/ipv4/tcp_ipv4.c.org net/ipv4/tcp_ipv4.c
@@ -222,10 +222,13 @@
                int rover;
 
                spin_lock(&tcp_portalloc_lock);
-               rover = tcp_port_rover;
+               if (tcp_port_rover < low)
+                       rover = low;
+               else
+                       rover = tcp_port_rover;
                do {
                        rover++;
-                       if (rover < low || rover > high)
+                       if (rover > high)
                                rover = low;
                        head = &tcp_bhash[tcp_bhashfn(rover)];
                        spin_lock(&head->lock);

diff -uNr net/ipv6/tcp_ipv6.c.org net/ipv6/tcp_ipv6.c
@@ -139,9 +139,12 @@
                int rover;
 
                spin_lock(&tcp_portalloc_lock);
-               rover = tcp_port_rover;
+               if (tcp_port_rover < low)
+                       rover = low;
+               else
+                       rover = tcp_port_rover;
                do {    rover++;
-                       if ((rover < low) || (rover > high))
+                       if (rover > high)
                                rover = low;
                        head = &tcp_bhash[tcp_bhashfn(rover)];
                        spin_lock(&head->lock);

(please cc me, i'm not (yet) on the list)

Folkert van Heusden

Auto te koop, zie: http://www.vanheusden.com/daihatsu.php
Op zoek naar een IT of Finance baan? Mail me voor de mogelijkheden.
--------------------------------------------------------------------
UNIX admin? Then give MultiTail (http://vanheusden.com/multitail/)
a try, it brings monitoring logfiles to a different level! See
http://vanheusden.com/multitail/features.html for a feature-list.
--------------------------------------------------------------------
Phone: +31-6-41278122, PGP-key: 1F28D8AE
Get your PGP/GPG key signed at www.biglumber.com!

It still has spaces where there should be tabs, even though you compressed the patch and used attachment.

The diff itself must already be corrupt before you attach it.

Please double check that the patch you submit actually apply, this will save me a lot of wasted time.

Ok, I've redone them and tested if they patch cleanly, they now do. See them attached.

While looking at the code I also saw something else:

@@ -222,11 +222,11 @@
 		int rover;
 
 		spin_lock(&tcp_portalloc_lock);
-		rover = tcp_port_rover;
+		if (tcp_port_rover < low)
+			rover = low;
+		else
+			rover = tcp_port_rover;
 		do {
-			rover++;
-			if (rover < low || rover > high)
-				rover = low;
 			head = &tcp_bhash[tcp_bhashfn(rover)];
 			spin_lock(&head->lock);
 			tb_for_each(tb, node, &head->chain)

@@ -235,6 +235,10 @@
 			break;
 		next:
 			spin_unlock(&head->lock);
+
+			rover++;
+			if (rover > high)
+				rover = low;
 		} while (--remaining > 0);
 		tcp_port_rover = rover;
 		spin_unlock(&tcp_portalloc_lock);

as you can see I moved (here! not in the included patches as I wanted to discuss this first!) the 'rover++' and its check to the end of the while loop. I did this because if they are at the top of the wile-loop the 'low' port number is never used as the first statement was a rover++!

Folkert van Heusden

Auto te koop, zie: http://www.vanheusden.com/daihatsu.php
Op zoek naar een IT of Finance baan? Mail me voor de mogelijkheden.
--------------------------------------------------------------------
 UNIX admin? Then give MultiTail (http://vanheusden.com/multitail/) 
 a try, it brings monitoring logfiles to a different level! See     
 http://vanheusden.com/multitail/features.html for a feature-list.  
--------------------------------------------------------------------
Phone: +31-6-41278122, PGP-key: 1F28D8AE
Get your PGP/GPG key signed at www.biglumber.com!

Both patches applied, thanks a lot.

What about moving that rover++ to the end of the loop? Was my analysis correct?

Folkert van Heusden

Auto te koop, zie: http://www.vanheusden.com/daihatsu.php
Op zoek naar een IT of Finance baan? Mail me voor de mogelijkheden.
--------------------------------------------------------------------
 UNIX admin? Then give MultiTail (http://vanheusden.com/multitail/) 
 a try, it brings monitoring logfiles to a different level! See     
 http://vanheusden.com/multitail/features.html for a feature-list.  
--------------------------------------------------------------------
Phone: +31-6-41278122, PGP-key: 1F28D8AE
Get your PGP/GPG key signed at www.biglumber.com!