[PATCH] optimize check in port-allocation code

[PATCH] optimize check in port-allocation code

[folkert]

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

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
--- net/ipv4/tcp_ipv4.c.org     2005-03-04 22:39:37.340950747 +0100
+++ net/ipv4/tcp_ipv4.c 2005-03-04 22:40:35.570059217 +0100
@@ -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
--- net/ipv6/tcp_ipv6.c.org     2005-03-04 22:41:44.043007791 +0100
+++ net/ipv6/tcp_ipv6.c 2005-03-04 22:42:17.604728073 +0100
@@ -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);


Signed-off-by: Folkert van Heusden <folkert@vanheusden.com>
(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!

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

Re: [PATCH] optimize check in port-allocation code

[David S. Miller]

On Mon, 25 Apr 2005 08:12:13 +0200
folkert@vanheusden.com wrote:

> I'll resend it as an attachment (see attachment).

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.

Re: [PATCH] optimize check in port-allocation code

[folkert]

[-- Attachment #1.1: Type: text/plain, Size: 2111 bytes --]

> > I'll resend it as an attachment (see attachment).
> 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:

--- tcp_ipv4.c.org	2005-04-28 23:13:32.000000000 +0200
+++ tcp_ipv4.c	2005-04-28 23:19:18.000000000 +0200
@@ -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!

[-- Attachment #1.2: tcp_ipv4.c.diff.gz --]
[-- Type: application/octet-stream, Size: 260 bytes --]

[-- Attachment #1.3: tcp_ipv6.c.diff.gz --]
[-- Type: application/octet-stream, Size: 261 bytes --]

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

Re: [PATCH] optimize check in port-allocation code

[David S. Miller]

On Thu, 28 Apr 2005 23:24:53 +0200
folkert@vanheusden.com wrote:

> > > I'll resend it as an attachment (see attachment).
> > 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.

Both patches applied, thanks a lot.

Re: [PATCH] optimize check in port-allocation code

[folkert]

> > > > I'll resend it as an attachment (see attachment).
> > > 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.
> 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!