Re: [PATCH] net-next: Fix __inet_inherit_port() to correctly increment bsockets and num_owners

Re: [PATCH] net-next: Fix __inet_inherit_port() to correctly increment bsockets and num_owners

[Nagendra Tomar]

Ok, here is the patch against net-next.

Thanks,
Tomar


---
inet sockets corresponding to passive connections are added to the bind hash
using ___inet_inherit_port(). These sockets are later removed from the bind 
hash using __inet_put_port(). These two functions are not exactly symmetrical. 
__inet_put_port() decrements hashinfo->bsockets and tb->num_owners, whereas 
___inet_inherit_port() does not increment them. This results in both of these 
going to -ve values.

This patch fixes this by calling inet_bind_hash() from ___inet_inherit_port(),
which does the right thing.

'bsockets' and 'num_owners' were introduced by commit a9d8f9110d7e953c 
(inet: Allowing more than 64k connections and heavily optimize bind(0))

Signed-off-by: Nagendra Singh Tomar <tomer_iisc@yahoo.com>

---
--- linux-2.6.37-rc3/net/ipv4/inet_hashtables.c.orig	2010-11-26 13:28:51.034811940 -0500
+++ linux-2.6.37-rc3/net/ipv4/inet_hashtables.c	2010-11-26 14:41:41.006268035 -0500
@@ -133,8 +133,7 @@ int __inet_inherit_port(struct sock *sk,
 			}
 		}
 	}
-	sk_add_bind_node(child, &tb->owners);
-	inet_csk(child)->icsk_bind_hash = tb;
+	inet_bind_hash(child, tb, port);
 	spin_unlock(&head->lock);
 
 	return 0;
--- 	





      

[PATCH] net-next: Fix __inet_inherit_port() to correctly increment bsockets and num_owners

[Nagendra Tomar]

inet sockets corresponding to passive connections are added to the bind hash
using ___inet_inherit_port(). These sockets are later removed from the bind 
hash using __inet_put_port(). These two functions are not exactly symmetrical. 
__inet_put_port() decrements hashinfo->bsockets and tb->num_owners, whereas 
___inet_inherit_port() does not increment them. This results in both of these 
going to -ve values.

This patch fixes this by calling inet_bind_hash() from ___inet_inherit_port(),
which does the right thing.

'bsockets' and 'num_owners' were introduced by commit a9d8f9110d7e953c 
(inet: Allowing more than 64k connections and heavily optimize bind(0))

Signed-off-by: Nagendra Singh Tomar <tomer_iisc@yahoo.com>

---
--- linux-2.6.37-rc3/net/ipv4/inet_hashtables.c.orig	2010-11-26 13:28:51.034811940 -0500
+++ linux-2.6.37-rc3/net/ipv4/inet_hashtables.c	2010-11-26 14:41:41.006268035 -0500
@@ -133,8 +133,7 @@ int __inet_inherit_port(struct sock *sk,
 			}
 		}
 	}
-	sk_add_bind_node(child, &tb->owners);
-	inet_csk(child)->icsk_bind_hash = tb;
+	inet_bind_hash(child, tb, port);
 	spin_unlock(&head->lock);
 
 	return 0;
--- 	



      

Re: [PATCH] net-next: Fix __inet_inherit_port() to correctly increment bsockets and num_owners

[Eric Dumazet]

Le vendredi 26 novembre 2010 à 16:26 -0800, Nagendra Tomar a écrit :
> inet sockets corresponding to passive connections are added to the bind hash
> using ___inet_inherit_port(). These sockets are later removed from the bind 
> hash using __inet_put_port(). These two functions are not exactly symmetrical. 
> __inet_put_port() decrements hashinfo->bsockets and tb->num_owners, whereas 
> ___inet_inherit_port() does not increment them. This results in both of these 
> going to -ve values.
> 
> This patch fixes this by calling inet_bind_hash() from ___inet_inherit_port(),
> which does the right thing.
> 
> 'bsockets' and 'num_owners' were introduced by commit a9d8f9110d7e953c 
> (inet: Allowing more than 64k connections and heavily optimize bind(0))
> 
> Signed-off-by: Nagendra Singh Tomar <tomer_iisc@yahoo.com>
> 
> ---
> --- linux-2.6.37-rc3/net/ipv4/inet_hashtables.c.orig	2010-11-26 13:28:51.034811940 -0500
> +++ linux-2.6.37-rc3/net/ipv4/inet_hashtables.c	2010-11-26 14:41:41.006268035 -0500
> @@ -133,8 +133,7 @@ int __inet_inherit_port(struct sock *sk,
>  			}
>  		}
>  	}
> -	sk_add_bind_node(child, &tb->owners);
> -	inet_csk(child)->icsk_bind_hash = tb;
> +	inet_bind_hash(child, tb, port);
>  	spin_unlock(&head->lock);
>  
>  	return 0;

Acked-by: Eric Dumazet <eric.dumazet@gmail.com>

Thanks !

Re: [PATCH] net-next: Fix __inet_inherit_port() to correctly increment bsockets and num_owners

[Evgeniy Polyakov]

Hi.

On Fri, Nov 26, 2010 at 04:26:27PM -0800, Nagendra Tomar (tomer_iisc@yahoo.com) wrote:
> inet sockets corresponding to passive connections are added to the bind hash
> using ___inet_inherit_port(). These sockets are later removed from the bind 
> hash using __inet_put_port(). These two functions are not exactly symmetrical. 
> __inet_put_port() decrements hashinfo->bsockets and tb->num_owners, whereas 
> ___inet_inherit_port() does not increment them. This results in both of these 
> going to -ve values.
> 
> This patch fixes this by calling inet_bind_hash() from ___inet_inherit_port(),
> which does the right thing.
> 
> 'bsockets' and 'num_owners' were introduced by commit a9d8f9110d7e953c 
> (inet: Allowing more than 64k connections and heavily optimize bind(0))

Yup, things changed from that simple patch a lot.
Thanks for fixing it up.
Ack.

-- 
	Evgeniy Polyakov

Re: [PATCH] net-next: Fix __inet_inherit_port() to correctly increment bsockets and num_owners

[David Miller]

From: Evgeniy Polyakov <zbr@ioremap.net>
Date: Mon, 29 Nov 2010 02:00:41 +0300

> Hi.
> 
> On Fri, Nov 26, 2010 at 04:26:27PM -0800, Nagendra Tomar (tomer_iisc@yahoo.com) wrote:
>> inet sockets corresponding to passive connections are added to the bind hash
>> using ___inet_inherit_port(). These sockets are later removed from the bind 
>> hash using __inet_put_port(). These two functions are not exactly symmetrical. 
>> __inet_put_port() decrements hashinfo->bsockets and tb->num_owners, whereas 
>> ___inet_inherit_port() does not increment them. This results in both of these 
>> going to -ve values.
>> 
>> This patch fixes this by calling inet_bind_hash() from ___inet_inherit_port(),
>> which does the right thing.
>> 
>> 'bsockets' and 'num_owners' were introduced by commit a9d8f9110d7e953c 
>> (inet: Allowing more than 64k connections and heavily optimize bind(0))
> 
> Yup, things changed from that simple patch a lot.
> Thanks for fixing it up.
> Ack.

I've decided to apply this to net-2.6, thanks everyone.

Re: [PATCH] net-next: Fix __inet_inherit_port() to correctly increment bsockets and num_owners

[Jarek Poplawski]

On 2010-11-29 00:00, Evgeniy Polyakov wrote:
> Hi.
> 
> On Fri, Nov 26, 2010 at 04:26:27PM -0800, Nagendra Tomar (tomer_iisc@yahoo.com) wrote:
>> inet sockets corresponding to passive connections are added to the bind hash
>> using ___inet_inherit_port(). These sockets are later removed from the bind 
>> hash using __inet_put_port(). These two functions are not exactly symmetrical. 
>> __inet_put_port() decrements hashinfo->bsockets and tb->num_owners, whereas 
>> ___inet_inherit_port() does not increment them. This results in both of these 
>> going to -ve values.
>>
>> This patch fixes this by calling inet_bind_hash() from ___inet_inherit_port(),
>> which does the right thing.
>>
>> 'bsockets' and 'num_owners' were introduced by commit a9d8f9110d7e953c 
>> (inet: Allowing more than 64k connections and heavily optimize bind(0))
> 
> Yup, things changed from that simple patch a lot.
> Thanks for fixing it up.
> Ack.

Probably I miss something, but since bsockets is increased by each
passive connection now, it seems it will trigger "hash table is full"
too early?

Jarek P.

Re: [PATCH] net-next: Fix __inet_inherit_port() to correctly increment bsockets and num_owners

[Eric Dumazet]

Le lundi 29 novembre 2010 à 12:39 +0000, Jarek Poplawski a écrit :

> Probably I miss something, but since bsockets is increased by each
> passive connection now, it seems it will trigger "hash table is full"
> too early?
> 

bsockets is the number of bound sockets.

It must be increased for each passive connection, since they are
bound :)

The value of bsockets is used to speedup bind() syscall, if few sockets
are bound. If not, no optimization takes place.

Re: [PATCH] net-next: Fix __inet_inherit_port() to correctly increment bsockets and num_owners

[Evgeniy Polyakov]

On Mon, Nov 29, 2010 at 12:39:09PM +0000, Jarek Poplawski (jarkao2@gmail.com) wrote:
> >> inet sockets corresponding to passive connections are added to the bind hash
> >> using ___inet_inherit_port(). These sockets are later removed from the bind 
> >> hash using __inet_put_port(). These two functions are not exactly symmetrical. 
> >> __inet_put_port() decrements hashinfo->bsockets and tb->num_owners, whereas 
> >> ___inet_inherit_port() does not increment them. This results in both of these 
> >> going to -ve values.
> >>
> >> This patch fixes this by calling inet_bind_hash() from ___inet_inherit_port(),
> >> which does the right thing.
> >>
> >> 'bsockets' and 'num_owners' were introduced by commit a9d8f9110d7e953c 
> >> (inet: Allowing more than 64k connections and heavily optimize bind(0))
> > 
> > Yup, things changed from that simple patch a lot.
> > Thanks for fixing it up.
> > Ack.
> 
> Probably I miss something, but since bsockets is increased by each
> passive connection now, it seems it will trigger "hash table is full"
> too early?

Why would it? bsockets and num_owners are supposed to be increased for each
new socket added into the table, and are used as a hint to find a bucket with
the smallest number of sockets in it.

Hash table insertion did not change, only bucket selection algorithm got
a hint.

-- 
	Evgeniy Polyakov

Re: [PATCH] net-next: Fix __inet_inherit_port() to correctly increment bsockets and num_owners

[Jarek Poplawski]

On Mon, Nov 29, 2010 at 03:51:02PM +0300, Evgeniy Polyakov wrote:
> On Mon, Nov 29, 2010 at 12:39:09PM +0000, Jarek Poplawski (jarkao2@gmail.com) wrote:
> > >> inet sockets corresponding to passive connections are added to the bind hash
> > >> using ___inet_inherit_port(). These sockets are later removed from the bind 
> > >> hash using __inet_put_port(). These two functions are not exactly symmetrical. 
> > >> __inet_put_port() decrements hashinfo->bsockets and tb->num_owners, whereas 
> > >> ___inet_inherit_port() does not increment them. This results in both of these 
> > >> going to -ve values.
> > >>
> > >> This patch fixes this by calling inet_bind_hash() from ___inet_inherit_port(),
> > >> which does the right thing.
> > >>
> > >> 'bsockets' and 'num_owners' were introduced by commit a9d8f9110d7e953c 
> > >> (inet: Allowing more than 64k connections and heavily optimize bind(0))
> > > 
> > > Yup, things changed from that simple patch a lot.
> > > Thanks for fixing it up.
> > > Ack.
> > 
> > Probably I miss something, but since bsockets is increased by each
> > passive connection now, it seems it will trigger "hash table is full"
> > too early?
> 
> Why would it? bsockets and num_owners are supposed to be increased for each
> new socket added into the table, and are used as a hint to find a bucket with
> the smallest number of sockets in it.
> 
> Hash table insertion did not change, only bucket selection algorithm got
> a hint.

Evgeniy & Eric,

But it's compared to the numer of available port numbers in
inet_csk_get_port():

     "if (atomic_read(&hashinfo->bsockets) > (high - low) + 1)"

Can't you have bsockets higher than this with only one port used?

Jarek P.

Re: [PATCH] net-next: Fix __inet_inherit_port() to correctly increment bsockets and num_owners

[Eric Dumazet]

Le lundi 29 novembre 2010 à 13:12 +0000, Jarek Poplawski a écrit :

> Evgeniy & Eric,
> 
> But it's compared to the numer of available port numbers in
> inet_csk_get_port():
> 
>      "if (atomic_read(&hashinfo->bsockets) > (high - low) + 1)"
> 
> Can't you have bsockets higher than this with only one port used?
> 

Because we store tuples, not only port information.

You can have many sockets bound on a single port.

This 'optimization' was only meaningful on a machine you make only
active connections, if you ask me...

Problem is, as soon as some passive connections are done, bsockets count
becomes wrong. The patch fixes this, thats all.

Re: [PATCH] net-next: Fix __inet_inherit_port() to correctly increment bsockets and num_owners

[Jarek Poplawski]

On Mon, Nov 29, 2010 at 02:17:46PM +0100, Eric Dumazet wrote:
> Le lundi 29 novembre 2010 ?? 13:12 +0000, Jarek Poplawski a écrit :
> 
> > Evgeniy & Eric,

Eric & Evgeniy ;-)

> > But it's compared to the numer of available port numbers in
> > inet_csk_get_port():
> > 
> >      "if (atomic_read(&hashinfo->bsockets) > (high - low) + 1)"
> > 
> > Can't you have bsockets higher than this with only one port used?
> > 
> 
> Because we store tuples, not only port information.
> 
> You can have many sockets bound on a single port.
> 
> This 'optimization' was only meaningful on a machine you make only
> active connections, if you ask me...

That's what I've missed...

> Problem is, as soon as some passive connections are done, bsockets count
> becomes wrong. The patch fixes this, thats all.

Sure, the numbers are correct now. I wondered about this optimization
only.

Thanks for explanations,
Jarek P.