Patchwork [net-next,v2] tcp: bind() use stronger condition for bind_conflict

login
register
mail settings
Submitter Alexandru Copot
Date April 6, 2012, 7:47 a.m.
Message ID <1333698444-24144-1-git-send-email-alex.mihai.c@gmail.com>
Download mbox | patch
Permalink /patch/151099/
State Superseded
Delegated to: David Miller
Headers show

Comments

Alexandru Copot - April 6, 2012, 7:47 a.m.
From: Alex Copot <alex.mihai.c@gmail.com>

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"

Tests where ran for creating and binding(0) many sockets
on 100 IPs. The results are, on average:

	* 60000 sockets, 600 ports / IP:
		* 0.210 s, 620 (IP, port) duplicates without patch
		* 0.219 s, no duplicates with patch
	* 100000 sockets, 1000 ports / IP:
		* 0.371 s, 1720 duplicates without patch
		* 0.373 s, no duplicates with patch
	* 200000 sockets, 2000 ports / IP:
		* 0.766 s, 6900 duplicates without patch
		* 0.768 s, no duplicates with patch
	* 500000 sockets, 5000 ports / IP:
		* 2.227 s, 41500 duplicates without patch
		* 2.284 s, no duplicates with patch

Signed-off-by: Alex Copot <alex.mihai.c@gmail.com>
Signed-off-by: Daniel Baluta <dbaluta@ixiacom.com>

---
Changes from v1:
 - Fixed coding style
 - Replaced int flag with bool
 - Added test results
---
 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    |    2 +-
 4 files changed, 17 insertions(+), 8 deletions(-)
Daniel Baluta - April 13, 2012, 5:36 a.m.
On Fri, Apr 6, 2012 at 10:47 AM, Alexandru Copot <alex.mihai.c@gmail.com> wrote:
> From: Alex Copot <alex.mihai.c@gmail.com>
>
> 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"
>
> Tests where ran for creating and binding(0) many sockets
> on 100 IPs. The results are, on average:
>
>        * 60000 sockets, 600 ports / IP:
>                * 0.210 s, 620 (IP, port) duplicates without patch
>                * 0.219 s, no duplicates with patch
>        * 100000 sockets, 1000 ports / IP:
>                * 0.371 s, 1720 duplicates without patch
>                * 0.373 s, no duplicates with patch
>        * 200000 sockets, 2000 ports / IP:
>                * 0.766 s, 6900 duplicates without patch
>                * 0.768 s, no duplicates with patch
>        * 500000 sockets, 5000 ports / IP:
>                * 2.227 s, 41500 duplicates without patch
>                * 2.284 s, no duplicates with patch
>
> Signed-off-by: Alex Copot <alex.mihai.c@gmail.com>
> Signed-off-by: Daniel Baluta <dbaluta@ixiacom.com>
>
> ---
> Changes from v1:
>  - Fixed coding style
>  - Replaced int flag with bool
>  - Added test results
> ---
>  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    |    2 +-
>  4 files changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/include/net/inet6_connection_sock.h b/include/net/inet6_connection_sock.h
> index 3207e58..1866a67 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, bool 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..46c9e2c 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, bool 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, bool 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..13ef772 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, bool 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, false)) {
>                                                        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, false)) {
>                                                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, true)) {
>                                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..e6cee52 100644
> --- a/net/ipv6/inet6_connection_sock.c
> +++ b/net/ipv6/inet6_connection_sock.c
> @@ -28,7 +28,7 @@
>  #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, bool relax)
>  {
>        const struct sock *sk2;
>        const struct hlist_node *node;
> --
> 1.7.9.6
>
> --

Eric, David can you have a look on this?

thanks,
Daniel.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - April 13, 2012, 5:46 a.m.
From: Daniel Baluta <daniel.baluta@gmail.com>
Date: Fri, 13 Apr 2012 08:36:31 +0300

> Eric, David can you have a look on this?

It's in the patchwork queue as you can plainly see:

http://patchwork.ozlabs.org/patch/151099/

you therefore never, ever, need to ask me questions like this.

I just haven't had time to look at the patch, but when I do you'll be
the first to know.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet - April 13, 2012, 5:53 a.m.
On Fri, 2012-04-13 at 08:36 +0300, Daniel Baluta wrote:

> Eric, David can you have a look on this?


Hi Daniel

I can, but can you provide me the sources of the tests you ran ?

(I am kind of busy these days)

Thanks


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Daniel Baluta - April 13, 2012, 6:12 a.m.
On Fri, Apr 13, 2012 at 8:53 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Fri, 2012-04-13 at 08:36 +0300, Daniel Baluta wrote:
>
>> Eric, David can you have a look on this?
>
>
> Hi Daniel
>
> I can, but can you provide me the sources of the tests you ran ?
>
> (I am kind of busy these days)

Sure. You can find the sources here [1].
* prepare-test.sh, sets up 100 interfaces.
* ./testcon -c <connections> -i <ips>, runs the test.

The results shown in patch description are obtained as follows:

* 60000 sockets, 600 ports / IP:
** ./testcon -c 60000 -i 100
* 100000 sockets, 1000 ports / IP:
** ./testcon -c 100000 -i 100
* 200000 sockets, 2000 ports / IP:
** ./testcon -c 200000 -i 100
* 500000 sockets, 5000 ports / IP:
** ./testcon -c 500000 -i 100

thanks,
Daniel.

[1] http://ixlabs.cs.pub.ro/gitweb/?p=port-allocation.git;a=tree;f=testbind;hb=master
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet - April 13, 2012, 7:30 a.m.
Le vendredi 06 avril 2012 à 10:47 +0300, Alexandru Copot a écrit :
> From: Alex Copot <alex.mihai.c@gmail.com>
> 
> We must try harder to get unique (addr, port) pairs when
> doing port autoselection for sockets with SO_REUSEADDR
> option set.
> 

...

Excellent patch Alex !

Two minor coding style problems in this part :

> +			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;
> +			}
>  		}

Correct indentation, and one empty line after variable declaration :

	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;
	}



if (condition_one ||
    condition_two) {
	foo();

instead of

if (condition_on ||
	condition_two)
	foo();


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexandru Copot - April 13, 2012, 8:12 a.m.
On Fri, Apr 13, 2012 at 10:30 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Excellent patch Alex !

Thanks, Eric !

> Two minor coding style problems in this part :
> Correct indentation, and one empty line after variable declaration :

I will fix those. That empty line is missing because that's the way it
was in the code above my changes, but I will add it.

Should I send a v3 or reply here with the new patch ?

Alex Copot
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/net/inet6_connection_sock.h b/include/net/inet6_connection_sock.h
index 3207e58..1866a67 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, bool 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..46c9e2c 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, bool 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, bool 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..13ef772 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, bool 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, false)) {
 							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, false)) {
 						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, true)) {
 				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..e6cee52 100644
--- a/net/ipv6/inet6_connection_sock.c
+++ b/net/ipv6/inet6_connection_sock.c
@@ -28,7 +28,7 @@ 
 #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, bool relax)
 {
 	const struct sock *sk2;
 	const struct hlist_node *node;