[RFC net-next] Include selection of congestion control algorithm in that which is inherited across an accept() call

[RFC net-next] Include selection of congestion control algorithm in that which is inherited across an accept() call

[Rick Jones]

From: Rick Jones <rick.jones2@hp.com>

Include congestion control algorithm in what is inherited across an
accept() call.

Signed-off-by: Rick Jones <rick.jones2@hp.com>

---

Before patch, with cubic as the default:
raj@raj-ubuntu-guest:~$ ./tcp_congestion_test  reno
Will be requesting 'reno' as the congestion control algorithm
active socket has 'reno' for congestion control
passive socket has 'reno' for congestion control
after listen() passive socket has 'reno' for congestion control
after connect() active has 'reno' for congestion control
after accept() accepted has 'cubic' for congestion control 

After patch, still with cubic as the default:
raj@raj-ubuntu-guest:~$ ./tcp_congestion_test reno
Will be requesting 'reno' as the congestion control algorithm
active socket has 'reno' for congestion control
passive socket has 'reno' for congestion control
after listen() passive socket has 'reno' for congestion control
after connect() active has 'reno' for congestion control
after accept() accepted has 'reno' for congestion control

IPv6 portion compiled only.


 include/linux/ipv6.h     |    3 +++
 include/net/inet_sock.h  |    3 +++
 net/ipv4/tcp_ipv4.c      |    3 +++
 net/ipv4/tcp_minisocks.c |    2 +-
 net/ipv6/tcp_ipv6.c      |    3 +++
 5 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/include/linux/ipv6.h b/include/linux/ipv6.h
index 0c99776..1d0dde3 100644
--- a/include/linux/ipv6.h
+++ b/include/linux/ipv6.h
@@ -265,11 +265,14 @@ static inline int inet6_iif(const struct sk_buff *skb)
 	return IP6CB(skb)->iif;
 }
 
+struct tcp_congestion_ops;
+
 struct inet6_request_sock {
 	struct in6_addr		loc_addr;
 	struct in6_addr		rmt_addr;
 	struct sk_buff		*pktopts;
 	int			iif;
+	const struct tcp_congestion_ops *cong_ops;
 };
 
 struct tcp6_request_sock {
diff --git a/include/net/inet_sock.h b/include/net/inet_sock.h
index f941964..9ec68a3 100644
--- a/include/net/inet_sock.h
+++ b/include/net/inet_sock.h
@@ -69,6 +69,8 @@ struct ip_options_data {
 	char			data[40];
 };
 
+struct tcp_congestion_ops;
+
 struct inet_request_sock {
 	struct request_sock	req;
 #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
@@ -89,6 +91,7 @@ struct inet_request_sock {
 				no_srccheck: 1;
 	kmemcheck_bitfield_end(flags);
 	struct ip_options_rcu	*opt;
+	const struct tcp_congestion_ops *cong_ops;
 };
 
 static inline struct inet_request_sock *inet_rsk(const struct request_sock *sk)
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index a9db4b1..79c02e2 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1254,6 +1254,7 @@ int tcp_v4_conn_request(struct sock *sk, struct sk_buff *skb)
 	const u8 *hash_location;
 	struct request_sock *req;
 	struct inet_request_sock *ireq;
+	struct inet_connection_sock *icsk = inet_csk(sk);
 	struct tcp_sock *tp = tcp_sk(sk);
 	struct dst_entry *dst = NULL;
 	__be32 saddr = ip_hdr(skb)->saddr;
@@ -1341,6 +1342,8 @@ int tcp_v4_conn_request(struct sock *sk, struct sk_buff *skb)
 	ireq->rmt_addr = saddr;
 	ireq->no_srccheck = inet_sk(sk)->transparent;
 	ireq->opt = tcp_v4_save_options(sk, skb);
+	ireq->cong_ops = (icsk->icsk_ca_ops) ? icsk->icsk_ca_ops :
+					&tcp_init_congestion_ops;
 
 	if (security_inet_conn_request(sk, skb, req))
 		goto drop_and_free;
diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index 945efff..be338d7 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -495,7 +495,7 @@ struct sock *tcp_create_openreq_child(struct sock *sk, struct request_sock *req,
 		newtp->frto_counter = 0;
 		newtp->frto_highmark = 0;
 
-		newicsk->icsk_ca_ops = &tcp_init_congestion_ops;
+		newicsk->icsk_ca_ops = ireq->cong_ops;
 
 		tcp_set_ca_state(newsk, TCP_CA_Open);
 		tcp_init_xmit_timers(newsk);
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 9d74eee..c689779 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1164,6 +1164,7 @@ static int tcp_v6_conn_request(struct sock *sk, struct sk_buff *skb)
 	const u8 *hash_location;
 	struct request_sock *req;
 	struct inet6_request_sock *treq;
+	struct inet_connection_sock *icsk = inet_csk(sk);
 	struct ipv6_pinfo *np = inet6_sk(sk);
 	struct tcp_sock *tp = tcp_sk(sk);
 	__u32 isn = TCP_SKB_CB(skb)->when;
@@ -1254,6 +1255,8 @@ static int tcp_v6_conn_request(struct sock *sk, struct sk_buff *skb)
 		TCP_ECN_create_request(req, tcp_hdr(skb));
 
 	treq->iif = sk->sk_bound_dev_if;
+	treq->cong_ops = (icsk->icsk_ca_ops) ? icsk->icsk_ca_ops :
+					&tcp_init_congestion_ops;
 
 	/* So that link locals have meaning */
 	if (!sk->sk_bound_dev_if &&

Re: [RFC net-next] Include selection of congestion control algorithm in that which is inherited across an accept() call

[Eric Dumazet]

Le mardi 29 novembre 2011 à 16:31 -0800, Rick Jones a écrit :
> From: Rick Jones <rick.jones2@hp.com>
> 
> Include congestion control algorithm in what is inherited across an
> accept() call.
> 
> Signed-off-by: Rick Jones <rick.jones2@hp.com>
> 

Hi Rick, thanks for working on this !

I am still not sure why adding new fields is needed, please see my
comment :

> ---
> 
...
>  include/linux/ipv6.h     |    3 +++
>  include/net/inet_sock.h  |    3 +++
>  net/ipv4/tcp_ipv4.c      |    3 +++
>  net/ipv4/tcp_minisocks.c |    2 +-
>  net/ipv6/tcp_ipv6.c      |    3 +++
>  5 files changed, 13 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/ipv6.h b/include/linux/ipv6.h
> index 0c99776..1d0dde3 100644
> --- a/include/linux/ipv6.h
> +++ b/include/linux/ipv6.h
> @@ -265,11 +265,14 @@ static inline int inet6_iif(const struct sk_buff *skb)
>  	return IP6CB(skb)->iif;
>  }
>  
> +struct tcp_congestion_ops;
> +
>  struct inet6_request_sock {
>  	struct in6_addr		loc_addr;
>  	struct in6_addr		rmt_addr;
>  	struct sk_buff		*pktopts;
>  	int			iif;
> +	const struct tcp_congestion_ops *cong_ops;
>  };
>  
>  struct tcp6_request_sock {
> diff --git a/include/net/inet_sock.h b/include/net/inet_sock.h
> index f941964..9ec68a3 100644
> --- a/include/net/inet_sock.h
> +++ b/include/net/inet_sock.h
> @@ -69,6 +69,8 @@ struct ip_options_data {
>  	char			data[40];
>  };
>  
> +struct tcp_congestion_ops;
> +
>  struct inet_request_sock {
>  	struct request_sock	req;
>  #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
> @@ -89,6 +91,7 @@ struct inet_request_sock {
>  				no_srccheck: 1;
>  	kmemcheck_bitfield_end(flags);
>  	struct ip_options_rcu	*opt;
> +	const struct tcp_congestion_ops *cong_ops;
>  };
>  
>  static inline struct inet_request_sock *inet_rsk(const struct request_sock *sk)
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index a9db4b1..79c02e2 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -1254,6 +1254,7 @@ int tcp_v4_conn_request(struct sock *sk, struct sk_buff *skb)
>  	const u8 *hash_location;
>  	struct request_sock *req;
>  	struct inet_request_sock *ireq;
> +	struct inet_connection_sock *icsk = inet_csk(sk);
>  	struct tcp_sock *tp = tcp_sk(sk);
>  	struct dst_entry *dst = NULL;
>  	__be32 saddr = ip_hdr(skb)->saddr;
> @@ -1341,6 +1342,8 @@ int tcp_v4_conn_request(struct sock *sk, struct sk_buff *skb)
>  	ireq->rmt_addr = saddr;
>  	ireq->no_srccheck = inet_sk(sk)->transparent;
>  	ireq->opt = tcp_v4_save_options(sk, skb);
> +	ireq->cong_ops = (icsk->icsk_ca_ops) ? icsk->icsk_ca_ops :
> +					&tcp_init_congestion_ops;
>  
>  	if (security_inet_conn_request(sk, skb, req))
>  		goto drop_and_free;
> diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
> index 945efff..be338d7 100644
> --- a/net/ipv4/tcp_minisocks.c
> +++ b/net/ipv4/tcp_minisocks.c
> @@ -495,7 +495,7 @@ struct sock *tcp_create_openreq_child(struct sock *sk, struct request_sock *req,
>  		newtp->frto_counter = 0;
>  		newtp->frto_highmark = 0;
>  
> -		newicsk->icsk_ca_ops = &tcp_init_congestion_ops;
> +		newicsk->icsk_ca_ops = ireq->cong_ops;


At this point, sk points to listener socket and is locked, why not
simply copy inet_csk(sk)->icsk_ca_ops to newicsk->icsk_ca_ops ?

As a matter of fact, it was already copied at newsk creation (clone of
sk)

>  
>  		tcp_set_ca_state(newsk, TCP_CA_Open);
>  		tcp_init_xmit_timers(newsk);
> diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> index 9d74eee..c689779 100644
> --- a/net/ipv6/tcp_ipv6.c
> +++ b/net/ipv6/tcp_ipv6.c
> @@ -1164,6 +1164,7 @@ static int tcp_v6_conn_request(struct sock *sk, struct sk_buff *skb)
>  	const u8 *hash_location;
>  	struct request_sock *req;
>  	struct inet6_request_sock *treq;
> +	struct inet_connection_sock *icsk = inet_csk(sk);
>  	struct ipv6_pinfo *np = inet6_sk(sk);
>  	struct tcp_sock *tp = tcp_sk(sk);
>  	__u32 isn = TCP_SKB_CB(skb)->when;
> @@ -1254,6 +1255,8 @@ static int tcp_v6_conn_request(struct sock *sk, struct sk_buff *skb)
>  		TCP_ECN_create_request(req, tcp_hdr(skb));
>  
>  	treq->iif = sk->sk_bound_dev_if;
> +	treq->cong_ops = (icsk->icsk_ca_ops) ? icsk->icsk_ca_ops :
> +					&tcp_init_congestion_ops;
>  
>  	/* So that link locals have meaning */
>  	if (!sk->sk_bound_dev_if &&


So my suggestion would be to use this two lines patch instead :

diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index 945efff..6b066e2 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -495,8 +495,6 @@ struct sock *tcp_create_openreq_child(struct sock *sk, struct request_sock *req,
 		newtp->frto_counter = 0;
 		newtp->frto_highmark = 0;
 
-		newicsk->icsk_ca_ops = &tcp_init_congestion_ops;
-
 		tcp_set_ca_state(newsk, TCP_CA_Open);
 		tcp_init_xmit_timers(newsk);
 		skb_queue_head_init(&newtp->out_of_order_queue);

Re: [RFC net-next] Include selection of congestion control algorithm in that which is inherited across an accept() call

[Eric Dumazet]

Le mercredi 30 novembre 2011 à 06:21 +0100, Eric Dumazet a écrit :

> So my suggestion would be to use this two lines patch instead :
> 
> diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
> index 945efff..6b066e2 100644
> --- a/net/ipv4/tcp_minisocks.c
> +++ b/net/ipv4/tcp_minisocks.c
> @@ -495,8 +495,6 @@ struct sock *tcp_create_openreq_child(struct sock *sk, struct request_sock *req,
>  		newtp->frto_counter = 0;
>  		newtp->frto_highmark = 0;
>  
> -		newicsk->icsk_ca_ops = &tcp_init_congestion_ops;
> -
>  		tcp_set_ca_state(newsk, TCP_CA_Open);
>  		tcp_init_xmit_timers(newsk);
>  		skb_queue_head_init(&newtp->out_of_order_queue);
> 

Please test this change and if its OK, resubmit your patch, with
appropriate Documentation change, as pointed out by Yuchung Cheng 

(file to change : Documentation/networking/ip-sysctl.txt )

You could clearly state that the congestion control eventually
chosen by the listener socket takes precedence over the system default
tcp congestion value.

Thanks !

[PATCH net-next] tcp: inherit listener congestion control for passive cnx

[Eric Dumazet]

Le mercredi 30 novembre 2011 à 06:40 +0100, Eric Dumazet a écrit :
> Le mercredi 30 novembre 2011 à 06:21 +0100, Eric Dumazet a écrit :
> 
> > So my suggestion would be to use this two lines patch instead :
> > 
> > diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
> > index 945efff..6b066e2 100644
> > --- a/net/ipv4/tcp_minisocks.c
> > +++ b/net/ipv4/tcp_minisocks.c
> > @@ -495,8 +495,6 @@ struct sock *tcp_create_openreq_child(struct sock *sk, struct request_sock *req,
> >  		newtp->frto_counter = 0;
> >  		newtp->frto_highmark = 0;
> >  
> > -		newicsk->icsk_ca_ops = &tcp_init_congestion_ops;
> > -
> >  		tcp_set_ca_state(newsk, TCP_CA_Open);
> >  		tcp_init_xmit_timers(newsk);
> >  		skb_queue_head_init(&newtp->out_of_order_queue);
> > 
> 
> Please test this change and if its OK, resubmit your patch, with
> appropriate Documentation change, as pointed out by Yuchung Cheng 

Hmm, after taking more time on this, we have to take care of module
refcounting and proper cleanup. I hope you dont mind I submit my final
version of this patch.

Thanks !

[PATCH net-next] tcp: inherit listener congestion control for passive cnx

Rick Jones reported that TCP_CONGESTION sockopt performed on a listener
was ignored for its children sockets : right after accept() the
congestion control for new socket is the system default one.

This seems an oversight of the initial design (quoted from Stephen)

Based on prior investigation and patch from Rick.

Reported-by: Rick Jones <rick.jones2@hp.com>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
CC: Stephen Hemminger <shemminger@vyatta.com>
CC: Yuchung Cheng <ycheng@google.com>
---
 Documentation/networking/ip-sysctl.txt |    3 +++
 net/ipv4/tcp_ipv4.c                    |    1 +
 net/ipv4/tcp_minisocks.c               |    4 +++-
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
index b886706..cb2b1c6 100644
--- a/Documentation/networking/ip-sysctl.txt
+++ b/Documentation/networking/ip-sysctl.txt
@@ -175,6 +175,9 @@ tcp_congestion_control - STRING
 	connections. The algorithm "reno" is always available, but
 	additional choices may be available based on kernel configuration.
 	Default is set as part of kernel configuration.
+	For passive connections, the listener congestion control choice
+	is inherited.
+	[see setsockopt(listenfd, SOL_TCP, TCP_CONGESTION, "name" ...) ]
 
 tcp_cookie_size - INTEGER
 	Default size of TCP Cookie Transactions (TCPCT) option, that may be
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index a9db4b1..c4b8b09 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1511,6 +1511,7 @@ exit:
 	return NULL;
 put_and_exit:
 	tcp_clear_xmit_timers(newsk);
+	tcp_cleanup_congestion_control(newsk);
 	bh_unlock_sock(newsk);
 	sock_put(newsk);
 	goto exit;
diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index 945efff..9dc146e 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -495,7 +495,9 @@ struct sock *tcp_create_openreq_child(struct sock *sk, struct request_sock *req,
 		newtp->frto_counter = 0;
 		newtp->frto_highmark = 0;
 
-		newicsk->icsk_ca_ops = &tcp_init_congestion_ops;
+		if (newicsk->icsk_ca_ops != &tcp_init_congestion_ops &&
+		    !try_module_get(newicsk->icsk_ca_ops->owner))
+			newicsk->icsk_ca_ops = &tcp_init_congestion_ops;
 
 		tcp_set_ca_state(newsk, TCP_CA_Open);
 		tcp_init_xmit_timers(newsk);

Re: [PATCH net-next] tcp: inherit listener congestion control for passive cnx

[Rick Jones]

On 11/30/2011 03:02 AM, Eric Dumazet wrote:
> Le mercredi 30 novembre 2011 à 06:40 +0100, Eric Dumazet a écrit :
>> Le mercredi 30 novembre 2011 à 06:21 +0100, Eric Dumazet a écrit :
>>
>>> So my suggestion would be to use this two lines patch instead :
>>>
>>> diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
>>> index 945efff..6b066e2 100644
>>> --- a/net/ipv4/tcp_minisocks.c
>>> +++ b/net/ipv4/tcp_minisocks.c
>>> @@ -495,8 +495,6 @@ struct sock *tcp_create_openreq_child(struct sock *sk, struct request_sock *req,
>>>   		newtp->frto_counter = 0;
>>>   		newtp->frto_highmark = 0;
>>>
>>> -		newicsk->icsk_ca_ops =&tcp_init_congestion_ops;
>>> -
>>>   		tcp_set_ca_state(newsk, TCP_CA_Open);
>>>   		tcp_init_xmit_timers(newsk);
>>>   		skb_queue_head_init(&newtp->out_of_order_queue);
>>>
>>
>> Please test this change and if its OK, resubmit your patch, with
>> appropriate Documentation change, as pointed out by Yuchung Cheng
>
> Hmm, after taking more time on this, we have to take care of module
> refcounting and proper cleanup. I hope you dont mind I submit my final
> version of this patch.

I do not mind at all.

rick

Re: [PATCH net-next] tcp: inherit listener congestion control for passive cnx

[Rick Jones]

>
> Rick Jones reported that TCP_CONGESTION sockopt performed on a listener
> was ignored for its children sockets : right after accept() the
> congestion control for new socket is the system default one.
>
> This seems an oversight of the initial design (quoted from Stephen)
>
> Based on prior investigation and patch from Rick.
>
> Reported-by: Rick Jones<rick.jones2@hp.com>
> Signed-off-by: Eric Dumazet<eric.dumazet@gmail.com>
> CC: Stephen Hemminger<shemminger@vyatta.com>
> CC: Yuchung Cheng<ycheng@google.com>

Tested-by: Rick Jones <rick.jones2@hp.com>

Works as expected/desired with my test program, and doesn't appear to 
affect performance measurably (loopback netperf TCP_CC test on a 1 VCPU 
guest).

rick jones

Re: [PATCH net-next] tcp: inherit listener congestion control for passive cnx

[David Miller]

From: Rick Jones <rick.jones2@hp.com>
Date: Wed, 30 Nov 2011 13:28:57 -0800

>>
>> Rick Jones reported that TCP_CONGESTION sockopt performed on a
>> listener
>> was ignored for its children sockets : right after accept() the
>> congestion control for new socket is the system default one.
>>
>> This seems an oversight of the initial design (quoted from Stephen)
>>
>> Based on prior investigation and patch from Rick.
>>
>> Reported-by: Rick Jones<rick.jones2@hp.com>
>> Signed-off-by: Eric Dumazet<eric.dumazet@gmail.com>
>> CC: Stephen Hemminger<shemminger@vyatta.com>
>> CC: Yuchung Cheng<ycheng@google.com>
> 
> Tested-by: Rick Jones <rick.jones2@hp.com>

Applied, thanks everyone.

Re: [RFC net-next] Include selection of congestion control algorithm in that which is inherited across an accept() call

[Rick Jones]

The only reason I did it the way I did was I'd not yet gotten that far 
though the maze :)   I like your two-line *much* better and will test it 
today.

rick