[PATCH] tcp: bind() use stronger condition for bind_conflict

Subsystems: networking [general], networking [tcp], the rest

STALE5261d

6 messages, 4 authors, 2012-03-29 · open the first message on its own page

[PATCH] tcp: bind() use stronger condition for bind_conflict

From: Alexandru Copot <hidden>
Date: 2012-03-27 21:12:14

We must try harder to get unique (addr, port) pairs when
doing port autoselection for sockets with SO_REUSEADDR
option set.

We achieve this by adding a relaxation parameter to
inet_csk_bind_conflict. When 'relax' parameter is off
we return a conflict whenever the current searched
pair (addr, port) is not unique.

This tries to address the problems reported in patch:
	8d238b25b1ec22a73b1c2206f111df2faaff8285
	Revert "tcp: bind() fix when many ports are bound"

Signed-off-by: Alexandru Copot <redacted>
Signed-off-by: Daniel Baluta <redacted>
---
 include/net/inet6_connection_sock.h |    2 +-
 include/net/inet_connection_sock.h  |    4 ++--
 net/ipv4/inet_connection_sock.c     |   17 +++++++++++++----
 net/ipv6/inet6_connection_sock.c    |    3 ++-
 4 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/include/net/inet6_connection_sock.h b/include/net/inet6_connection_sock.h
index 3207e58..9d29ae2 100644
--- a/include/net/inet6_connection_sock.h
+++ b/include/net/inet6_connection_sock.h
@@ -23,7 +23,7 @@ struct sock;
 struct sockaddr;
 
 extern int inet6_csk_bind_conflict(const struct sock *sk,
-				   const struct inet_bind_bucket *tb);
+				   const struct inet_bind_bucket *tb, int relax);
 
 extern struct dst_entry* inet6_csk_route_req(struct sock *sk,
 					     const struct request_sock *req);
diff --git a/include/net/inet_connection_sock.h b/include/net/inet_connection_sock.h
index dbf9aab..f05a032 100644
--- a/include/net/inet_connection_sock.h
+++ b/include/net/inet_connection_sock.h
@@ -60,7 +60,7 @@ struct inet_connection_sock_af_ops {
 #endif
 	void	    (*addr2sockaddr)(struct sock *sk, struct sockaddr *);
 	int	    (*bind_conflict)(const struct sock *sk,
-				     const struct inet_bind_bucket *tb);
+				     const struct inet_bind_bucket *tb, int relax);
 };
 
 /** inet_connection_sock - INET connection oriented sock
@@ -245,7 +245,7 @@ extern struct request_sock *inet_csk_search_req(const struct sock *sk,
 						const __be32 raddr,
 						const __be32 laddr);
 extern int inet_csk_bind_conflict(const struct sock *sk,
-				  const struct inet_bind_bucket *tb);
+				  const struct inet_bind_bucket *tb, int relax);
 extern int inet_csk_get_port(struct sock *sk, unsigned short snum);
 
 extern struct dst_entry* inet_csk_route_req(struct sock *sk,
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index 19d66ce..bf50e77 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -53,7 +53,7 @@ void inet_get_local_port_range(int *low, int *high)
 EXPORT_SYMBOL(inet_get_local_port_range);
 
 int inet_csk_bind_conflict(const struct sock *sk,
-			   const struct inet_bind_bucket *tb)
+			   const struct inet_bind_bucket *tb, int relax)
 {
 	struct sock *sk2;
 	struct hlist_node *node;
@@ -79,6 +79,13 @@ int inet_csk_bind_conflict(const struct sock *sk,
 				    sk2_rcv_saddr == sk_rcv_saddr(sk))
 					break;
 			}
+			if (!relax && reuse && sk2->sk_reuse &&
+					sk2->sk_state != TCP_LISTEN) {
+				const __be32 sk2_rcv_saddr = sk_rcv_saddr(sk2);
+				if (!sk2_rcv_saddr || !sk_rcv_saddr(sk) ||
+						sk2_rcv_saddr == sk_rcv_saddr(sk))
+					break;
+			}
 		}
 	}
 	return node != NULL;
@@ -122,12 +129,13 @@ again:
 					    (tb->num_owners < smallest_size || smallest_size == -1)) {
 						smallest_size = tb->num_owners;
 						smallest_rover = rover;
-						if (atomic_read(&hashinfo->bsockets) > (high - low) + 1) {
+						if (atomic_read(&hashinfo->bsockets) > (high - low) + 1 &&
+							!inet_csk(sk)->icsk_af_ops->bind_conflict(sk, tb, 0)) {
 							snum = smallest_rover;
 							goto tb_found;
 						}
 					}
-					if (!inet_csk(sk)->icsk_af_ops->bind_conflict(sk, tb)) {
+					if (!inet_csk(sk)->icsk_af_ops->bind_conflict(sk, tb, 0)) {
 						snum = rover;
 						goto tb_found;
 					}
@@ -178,12 +186,13 @@ tb_found:
 			goto success;
 		} else {
 			ret = 1;
-			if (inet_csk(sk)->icsk_af_ops->bind_conflict(sk, tb)) {
+			if (inet_csk(sk)->icsk_af_ops->bind_conflict(sk, tb, 1)) {
 				if (sk->sk_reuse && sk->sk_state != TCP_LISTEN &&
 				    smallest_size != -1 && --attempts >= 0) {
 					spin_unlock(&head->lock);
 					goto again;
 				}
+
 				goto fail_unlock;
 			}
 		}
diff --git a/net/ipv6/inet6_connection_sock.c b/net/ipv6/inet6_connection_sock.c
index 02dd203..dfc8bc3 100644
--- a/net/ipv6/inet6_connection_sock.c
+++ b/net/ipv6/inet6_connection_sock.c
@@ -28,7 +28,8 @@
 #include <net/inet6_connection_sock.h>
 
 int inet6_csk_bind_conflict(const struct sock *sk,
-			    const struct inet_bind_bucket *tb)
+			    const struct inet_bind_bucket *tb,
+				int relax)
 {
 	const struct sock *sk2;
 	const struct hlist_node *node;
-- 
1.7.9.4

Re: [PATCH] tcp: bind() use stronger condition for bind_conflict

From: Flavio Leitner <hidden>
Date: 2012-03-28 13:52:59

On Wed, 28 Mar 2012 00:11:52 +0300
Alexandru Copot [off-list ref] wrote:
quoted hunk
We must try harder to get unique (addr, port) pairs when
doing port autoselection for sockets with SO_REUSEADDR
option set.

We achieve this by adding a relaxation parameter to
inet_csk_bind_conflict. When 'relax' parameter is off
we return a conflict whenever the current searched
pair (addr, port) is not unique.

This tries to address the problems reported in patch:
	8d238b25b1ec22a73b1c2206f111df2faaff8285
	Revert "tcp: bind() fix when many ports are bound"

Signed-off-by: Alexandru Copot <redacted>
Signed-off-by: Daniel Baluta <redacted>
---
 include/net/inet6_connection_sock.h |    2 +-
 include/net/inet_connection_sock.h  |    4 ++--
 net/ipv4/inet_connection_sock.c     |   17 +++++++++++++----
 net/ipv6/inet6_connection_sock.c    |    3 ++-
 4 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/include/net/inet6_connection_sock.h b/include/net/inet6_connection_sock.h
index 3207e58..9d29ae2 100644
--- a/include/net/inet6_connection_sock.h
+++ b/include/net/inet6_connection_sock.h
@@ -23,7 +23,7 @@ struct sock;
 struct sockaddr;
 
 extern int inet6_csk_bind_conflict(const struct sock *sk,
-				   const struct inet_bind_bucket *tb);
+				   const struct inet_bind_bucket *tb, int relax);
 
 extern struct dst_entry* inet6_csk_route_req(struct sock *sk,
 					     const struct request_sock *req);
diff --git a/include/net/inet_connection_sock.h b/include/net/inet_connection_sock.h
index dbf9aab..f05a032 100644
--- a/include/net/inet_connection_sock.h
+++ b/include/net/inet_connection_sock.h
@@ -60,7 +60,7 @@ struct inet_connection_sock_af_ops {
 #endif
 	void	    (*addr2sockaddr)(struct sock *sk, struct sockaddr *);
 	int	    (*bind_conflict)(const struct sock *sk,
-				     const struct inet_bind_bucket *tb);
+				     const struct inet_bind_bucket *tb, int relax);
 };
 
 /** inet_connection_sock - INET connection oriented sock
@@ -245,7 +245,7 @@ extern struct request_sock *inet_csk_search_req(const struct sock *sk,
 						const __be32 raddr,
 						const __be32 laddr);
 extern int inet_csk_bind_conflict(const struct sock *sk,
-				  const struct inet_bind_bucket *tb);
+				  const struct inet_bind_bucket *tb, int relax);
 extern int inet_csk_get_port(struct sock *sk, unsigned short snum);
 
 extern struct dst_entry* inet_csk_route_req(struct sock *sk,
diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
index 19d66ce..bf50e77 100644
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -53,7 +53,7 @@ void inet_get_local_port_range(int *low, int *high)
 EXPORT_SYMBOL(inet_get_local_port_range);
 
 int inet_csk_bind_conflict(const struct sock *sk,
-			   const struct inet_bind_bucket *tb)
+			   const struct inet_bind_bucket *tb, int relax)
 {
 	struct sock *sk2;
 	struct hlist_node *node;
@@ -79,6 +79,13 @@ int inet_csk_bind_conflict(const struct sock *sk,
 				    sk2_rcv_saddr == sk_rcv_saddr(sk))
 					break;
 			}
+			if (!relax && reuse && sk2->sk_reuse &&
+					sk2->sk_state != TCP_LISTEN) {
+				const __be32 sk2_rcv_saddr = sk_rcv_saddr(sk2);
+				if (!sk2_rcv_saddr || !sk_rcv_saddr(sk) ||
+						sk2_rcv_saddr == sk_rcv_saddr(sk))
I am still checking the patch, but the above is out of coding style.
perhaps:
+				if (!sk2_rcv_saddr || !sk_rcv_saddr(sk) ||
+				    sk2_rcv_saddr == sk_rcv_saddr(sk))

quoted hunk
+					break;
+			}
 		}
 	}
 	return node != NULL;
@@ -122,12 +129,13 @@ again:
 					    (tb->num_owners < smallest_size || smallest_size == -1)) {
 						smallest_size = tb->num_owners;
 						smallest_rover = rover;
-						if (atomic_read(&hashinfo->bsockets) > (high - low) + 1) {
+						if (atomic_read(&hashinfo->bsockets) > (high - low) + 1 &&
+							!inet_csk(sk)->icsk_af_ops->bind_conflict(sk, tb, 0)) {
Same above.
quoted hunk
 							snum = smallest_rover;
 							goto tb_found;
 						}
 					}
-					if (!inet_csk(sk)->icsk_af_ops->bind_conflict(sk, tb)) {
+					if (!inet_csk(sk)->icsk_af_ops->bind_conflict(sk, tb, 0)) {
 						snum = rover;
 						goto tb_found;
 					}
@@ -178,12 +186,13 @@ tb_found:
 			goto success;
 		} else {
 			ret = 1;
-			if (inet_csk(sk)->icsk_af_ops->bind_conflict(sk, tb)) {
+			if (inet_csk(sk)->icsk_af_ops->bind_conflict(sk, tb, 1)) {
 				if (sk->sk_reuse && sk->sk_state != TCP_LISTEN &&
 				    smallest_size != -1 && --attempts >= 0) {
 					spin_unlock(&head->lock);
 					goto again;
 				}
+
 				goto fail_unlock;
 			}
 		}
diff --git a/net/ipv6/inet6_connection_sock.c b/net/ipv6/inet6_connection_sock.c
index 02dd203..dfc8bc3 100644
--- a/net/ipv6/inet6_connection_sock.c
+++ b/net/ipv6/inet6_connection_sock.c
@@ -28,7 +28,8 @@
 #include <net/inet6_connection_sock.h>
 
 int inet6_csk_bind_conflict(const struct sock *sk,
-			    const struct inet_bind_bucket *tb)
+			    const struct inet_bind_bucket *tb,
+				int relax)
Same here
Maybe you can wait for more feedbacks before spin another patch 
version just to fix that.


fbl
 {
 	const struct sock *sk2;
 	const struct hlist_node *node;

Re: [PATCH] tcp: bind() use stronger condition for bind_conflict

From: Eric Dumazet <hidden>
Date: 2012-03-28 14:48:41

On Wed, 2012-03-28 at 10:52 -0300, Flavio Leitner wrote:
On Wed, 28 Mar 2012 00:11:52 +0300
Alexandru Copot [off-list ref] wrote:
quoted
diff --git a/net/ipv6/inet6_connection_sock.c b/net/ipv6/inet6_connection_sock.c
index 02dd203..dfc8bc3 100644
--- a/net/ipv6/inet6_connection_sock.c
+++ b/net/ipv6/inet6_connection_sock.c
@@ -28,7 +28,8 @@
 #include <net/inet6_connection_sock.h>
 
 int inet6_csk_bind_conflict(const struct sock *sk,
-			    const struct inet_bind_bucket *tb)
+			    const struct inet_bind_bucket *tb,
+				int relax)
Same here
Maybe you can wait for more feedbacks before spin another patch 
version just to fix that.


Also 'int relax' should be 'bool relax'... 1/0 -> true/false

Anyway net-next is not open yet...

Re: [PATCH] tcp: bind() use stronger condition for bind_conflict

From: Daniel Baluta <hidden>
Date: 2012-03-28 20:08:03

On Wed, Mar 28, 2012 at 5:48 PM, Eric Dumazet [off-list ref] wrote:
On Wed, 2012-03-28 at 10:52 -0300, Flavio Leitner wrote:
quoted
On Wed, 28 Mar 2012 00:11:52 +0300
Alexandru Copot [off-list ref] wrote:
quoted
quoted
diff --git a/net/ipv6/inet6_connection_sock.c b/net/ipv6/inet6_connection_sock.c
index 02dd203..dfc8bc3 100644
--- a/net/ipv6/inet6_connection_sock.c
+++ b/net/ipv6/inet6_connection_sock.c
@@ -28,7 +28,8 @@
 #include <net/inet6_connection_sock.h>

 int inet6_csk_bind_conflict(const struct sock *sk,
-                       const struct inet_bind_bucket *tb)
+                       const struct inet_bind_bucket *tb,
+                           int relax)
Same here
Maybe you can wait for more feedbacks before spin another patch
version just to fix that.


Also 'int relax' should be 'bool relax'... 1/0 -> true/false

Anyway net-next is not open yet...
Eric, Flavio, thanks a lot for your comments. We will resubmit this
when net-next opens.

Please let us know if you have any other observations.

thanks,
Daniel.

Re: [PATCH] tcp: bind() use stronger condition for bind_conflict

From: Eric Dumazet <hidden>
Date: 2012-03-28 20:12:18

On Wed, 2012-03-28 at 23:08 +0300, Daniel Baluta wrote:
Eric, Flavio, thanks a lot for your comments. We will resubmit this
when net-next opens.

Please let us know if you have any other observations.
Some performance data would be welcomed, in the case many sockets are
already bound...

Thanks

Re: [PATCH] tcp: bind() use stronger condition for bind_conflict

From: Alexandru Copot <hidden>
Date: 2012-03-29 08:22:29

On Wed, Mar 28, 2012 at 11:12 PM, Eric Dumazet [off-list ref] wrote:
Some performance data would be welcomed, in the case many sockets are
already bound...

I've done some tests running a program that creates and binds(0) 90000 sockets.
The total running time, on average, is:
*  without this patch: 0.352 s
*  with the patch:       0.355 s

Also, recording this program with perf shows a small increase of 0.6%
for inet_csk_get_port
relative to inet_bind, after applying the patch.

So the performance is almost the same, even for such a large number of sockets.

Alex Copot
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help