Include selection of congestion control algorithm in that which is inherited across an accept() call

RFC In review Rick Jones 4 participants 8 messages Nov 30, 2011

Subsystems (4)

Networking Network drivers Documentation TCP

lore

Share & tools

lore

Rick Jones

author

2011-11-30 00:31 UTC #0 lore raw

RFC 1/1 Include selection of congestion control algorithm in that which is inherited across an accept() call

From: Rick Jones <[email protected]>

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

Signed-off-by Rick Jones <[email protected]>

---

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(-)

include/linux/ipv6.h +3 -0

index 0c99776..1d0dde3 100644
@@ -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 {

include/net/inet_sock.h +3 -0

index f941964..9ec68a3 100644
@@ -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)

net/ipv4/tcp_ipv4.c +3 -0

index a9db4b1..79c02e2 100644
@@ -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;

net/ipv4/tcp_minisocks.c +1 -1

index 945efff..be338d7 100644
@@ -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);

net/ipv6/tcp_ipv6.c +3 -0

index 9d74eee..c689779 100644
@@ -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 &&

Eric Dumazet

2011-11-30 05:21 UTC #1 lore raw

Quoted 7 lines · Rick Jones

From: Rick Jones <[email protected]>
Include congestion control algorithm in what is inherited across an accept() call.
Signed-off-by Rick Jones <[email protected]>

Hi Rick, thanks for working on this !

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

Quoted 2 lines

---

...

Quoted 73 lines

 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(-)

include/linux/ipv6.h +3 -0

index 0c99776..1d0dde3 100644
@@ -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 {

include/net/inet_sock.h +3 -0

index f941964..9ec68a3 100644
@@ -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)

net/ipv4/tcp_ipv4.c +3 -0

index a9db4b1..79c02e2 100644
@@ -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;

net/ipv4/tcp_minisocks.c +0 -0

index 945efff..be338d7 100644

net/ipv4/tcp_minisocks.c struct sock *tcp_create_openreq_child(struct sock *sk, struct request_sock *req, · line 498

Quoted 5 lines

net/ipv4/tcp_minisocks.c +1 -1

@@ -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)

Quoted 16 lines

net/ipv4/tcp_minisocks.c +0 -0

 		tcp_set_ca_state(newsk, TCP_CA_Open);
 		tcp_init_xmit_timers(newsk);

net/ipv6/tcp_ipv6.c +1 -0

index 9d74eee..c689779 100644
@@ -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;

net/ipv6/tcp_ipv6.c static int tcp_v6_conn_request(struct sock *sk, struct sk_buff *skb) · line 1262

Quoted 8 lines

net/ipv6/tcp_ipv6.c +2 -0

@@ -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 :

net/ipv4/tcp_minisocks.c +0 -2

index 945efff..6b066e2 100644
@@ -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);

Eric Dumazet

2011-11-30 05:40 UTC #2 lore raw

Quoted 8 lines · Eric Dumazet

So my suggestion would be to use this two lines patch instead :
net/ipv4/tcp_minisocks.c +0 -0
index 945efff..6b066e2 100644

net/ipv4/tcp_minisocks.c struct sock *tcp_create_openreq_child(struct sock *sk, struct request_sock *req, · line 500

Quoted 8 lines

net/ipv4/tcp_minisocks.c +0 -2

@@ -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 !

Eric Dumazet

2011-11-30 11:02 UTC #3 lore raw

PATCH 1/1 tcp: inherit listener congestion control for passive cnx

Quoted 12 lines · Eric Dumazet

Quoted 10 lines · Eric Dumazet
So my suggestion would be to use this two lines patch instead :
net/ipv4/tcp_minisocks.c +0 -0
index 945efff..6b066e2 100644
@@ -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;
 

Quoted 9 lines

net/ipv4/tcp_minisocks.c struct sock *tcp_create_openreq_child(struct sock *sk, struct request_sock *req, · line 500
Quoted 6 lines
net/ipv4/tcp_minisocks.c +0 -2
-		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 <[email protected]>

Signed-off-by Eric Dumazet <[email protected]>

CC Stephen Hemminger <[email protected]>

CC Yuchung Cheng <[email protected]>

---
 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(-)

Documentation/networking/ip-sysctl.txt +3 -0

index b886706..cb2b1c6 100644
@@ -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

net/ipv4/tcp_ipv4.c +1 -0

index a9db4b1..c4b8b09 100644
@@ -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;

net/ipv4/tcp_minisocks.c +3 -1

index 945efff..9dc146e 100644
@@ -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);

Rick Jones

2011-11-30 17:33 UTC #4 lore raw

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

Quoted 23 lines · Eric Dumazet

Quoted 12 lines · Eric Dumazet
Quoted 10 lines · Eric Dumazet
So my suggestion would be to use this two lines patch instead :
net/ipv4/tcp_minisocks.c +0 -0
index 945efff..6b066e2 100644
@@ -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;
 
Quoted 9 lines
net/ipv4/tcp_minisocks.c struct sock *tcp_create_openreq_child(struct sock *sk, struct request_sock *req, · line 500
Quoted 6 lines
net/ipv4/tcp_minisocks.c +0 -2
-		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

Quoted 3 lines

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

Rick Jones

Tested-by on v1 1/1

2011-11-30 21:29 UTC #5 lore raw

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

In reply to Eric Dumazet

Quoted 5 lines

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.

Quoted 8 lines

This seems an oversight of the initial design (quoted from Stephen)
Based on prior investigation and patch from Rick.
Reported-by Rick Jones<[email protected]>
Signed-off-by Eric Dumazet<[email protected]>
CC Stephen Hemminger<[email protected]>
CC Yuchung Cheng<[email protected]>

Tested-by Rick Jones <[email protected]>

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

David Miller

maintainer Applied

2011-11-30 21:56 UTC #6 lore raw

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

From: Rick Jones <[email protected]>
Date: Wed, 30 Nov 2011 13:28:57 -0800

Quoted 8 lines

Quoted 8 lines
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)

Quoted 8 lines

Quoted 6 lines
Based on prior investigation and patch from Rick.
Reported-by Rick Jones<[email protected]>
Signed-off-by Eric Dumazet<[email protected]>
CC Stephen Hemminger<[email protected]>
CC Yuchung Cheng<[email protected]>
Tested-by Rick Jones <[email protected]>

Applied, thanks everyone.

Rick Jones

2011-11-30 17:21 UTC #7 lore raw

In reply to Eric Dumazet

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