Re: [PATCH net] tcp: restore autocorking
From: Eric Dumazet
Date: Wed, 2 May 2018 20:25:13 -0700
> When adding rb-tree for TCP retransmit queue, we inadvertently broke
> TCP autocorking.
>
> tcp_should_autocork() should really check if the rtx queue is not empty.
>
> Tested:
...
> Fixes: 75c119afe14f ("tcp: implement rb-tree based retransmit queue")
> Signed-off-by: Eric Dumazet
> Reported-by: Michael Wenig
> Tested-by: Michael Wenig
Applied and queued up for -stable, thanks Eric.
Re: [PATCH net] tcp: restore autocorking
On Wed, May 2, 2018 at 11:25 PM, Eric Dumazet wrote:
> Fixes: 75c119afe14f ("tcp: implement rb-tree based retransmit queue")
> Signed-off-by: Eric Dumazet
> Reported-by: Michael Wenig
> Tested-by: Michael Wenig
Acked-by: Soheil Hassas Yeganeh
Thank you for catching and fixing this!
Re: [PATCH net] tcp: restore autocorking
On Wed, May 2, 2018 at 11:25 PM Eric Dumazet wrote:
> When adding rb-tree for TCP retransmit queue, we inadvertently broke
> TCP autocorking.
> tcp_should_autocork() should really check if the rtx queue is not empty.
...
> Fixes: 75c119afe14f ("tcp: implement rb-tree based retransmit queue")
> Signed-off-by: Eric Dumazet
> Reported-by: Michael Wenig
> Tested-by: Michael Wenig
> ---
Acked-by: Neal Cardwell
Nice. Thanks, Eric!
neal
RE: [PATCH net] tcp: restore autocorking
Thank you, Eric, for fixing this!
Michael
-Original Message-
From: Eric Dumazet [mailto:eduma...@google.com]
Sent: Wednesday, May 2, 2018 8:25 PM
To: David S . Miller
Cc: netdev ; Eric Dumazet ;
Michael Wenig ; Eric Dumazet
Subject: [PATCH net] tcp: restore autocorking
When adding rb-tree for TCP retransmit queue, we inadvertently broke TCP
autocorking.
tcp_should_autocork() should really check if the rtx queue is not empty.
Tested:
Before the fix :
$ nstat -n;./netperf -H 10.246.7.152 -Cc -- -m 500;nstat | grep AutoCork
MIGRATED TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 10.246.7.152
() port 0 AF_INET
Recv SendSend Utilization Service Demand
Socket Socket Message Elapsed Send Recv SendRecv
Size SizeSize Time Throughput localremote local remote
bytes bytes bytessecs.10^6bits/s % S % S us/KB us/KB
54 26214450010.00 2682.85 2.47 1.59 3.618 2.329
TcpExtTCPAutoCorking33 0.0
// Same test, but forcing TCP_NODELAY
$ nstat -n;./netperf -H 10.246.7.152 -Cc -- -D -m 500;nstat | grep AutoCork
MIGRATED TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 10.246.7.152
() port 0 AF_INET : nodelay
Recv SendSend Utilization Service Demand
Socket Socket Message Elapsed Send Recv SendRecv
Size SizeSize Time Throughput localremote local remote
bytes bytes bytessecs.10^6bits/s % S % S us/KB us/KB
54 26214450010.00 1408.75 2.44 2.96 6.802 8.259
TcpExtTCPAutoCorking1 0.0
After the fix :
$ nstat -n;./netperf -H 10.246.7.152 -Cc -- -m 500;nstat | grep AutoCork
MIGRATED TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 10.246.7.152
() port 0 AF_INET
Recv SendSend Utilization Service Demand
Socket Socket Message Elapsed Send Recv SendRecv
Size SizeSize Time Throughput localremote local remote
bytes bytes bytessecs.10^6bits/s % S % S us/KB us/KB
54 26214450010.00 5472.46 2.45 1.43 1.761 1.027
TcpExtTCPAutoCorking361293 0.0
// With TCP_NODELAY option
$ nstat -n;./netperf -H 10.246.7.152 -Cc -- -D -m 500;nstat | grep AutoCork
MIGRATED TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 10.246.7.152
() port 0 AF_INET : nodelay
Recv SendSend Utilization Service Demand
Socket Socket Message Elapsed Send Recv SendRecv
Size SizeSize Time Throughput localremote local remote
bytes bytes bytessecs.10^6bits/s % S % S us/KB us/KB
54 26214450010.00 5454.96 2.46 1.63 1.775 1.174
TcpExtTCPAutoCorking315448 0.0
Fixes: 75c119afe14f ("tcp: implement rb-tree based retransmit queue")
Signed-off-by: Eric Dumazet
Reported-by: Michael Wenig
Tested-by: Michael Wenig
---
net/ipv4/tcp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index
44be7f43455e4aefde8db61e2d941a69abcc642a..c9d00ef54deca15d5760bcbe154001a96fa1e2a7
100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -697,7 +697,7 @@ static bool tcp_should_autocork(struct sock *sk, struct
sk_buff *skb, {
return skb->len < size_goal &&
sock_net(sk)->ipv4.sysctl_tcp_autocorking &&
- skb != tcp_write_queue_head(sk) &&
+ !tcp_rtx_queue_empty(sk) &&
refcount_read(&sk->sk_wmem_alloc) > skb->truesize; }
--
2.17.0.441.gb46fe60e1d-goog
Re: [PATCH net] tcp: restore autocorking
On Thu, May 3, 2018 at 4:06 AM Tariq Toukan wrote: > On 03/05/2018 6:25 AM, Eric Dumazet wrote: > > When adding rb-tree for TCP retransmit queue, we inadvertently broke > > TCP autocorking. > > > > tcp_should_autocork() should really check if the rtx queue is not empty. > > > Hi Eric, > We are glad to see that the issue that Tal investigated and reported [1] > is now addressed. > Thanks for doing that! > Tal, let’s perf test to see the effect of this fix. > Best, > Tariq > [1] https://patchwork.ozlabs.org/cover/822218/ Yes, but you never really gave any insights of what exact tests you were running :/ Sometimes the crystal ball is silent, sometimes it gives a hint :)
Re: [PATCH net] tcp: restore autocorking
On 03/05/2018 6:25 AM, Eric Dumazet wrote: When adding rb-tree for TCP retransmit queue, we inadvertently broke TCP autocorking. tcp_should_autocork() should really check if the rtx queue is not empty. Hi Eric, We are glad to see that the issue that Tal investigated and reported [1] is now addressed. Thanks for doing that! Tal, let’s perf test to see the effect of this fix. Best, Tariq [1] https://patchwork.ozlabs.org/cover/822218/
[PATCH net] tcp: restore autocorking
When adding rb-tree for TCP retransmit queue, we inadvertently broke
TCP autocorking.
tcp_should_autocork() should really check if the rtx queue is not empty.
Tested:
Before the fix :
$ nstat -n;./netperf -H 10.246.7.152 -Cc -- -m 500;nstat | grep AutoCork
MIGRATED TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 10.246.7.152
() port 0 AF_INET
Recv SendSend Utilization Service Demand
Socket Socket Message Elapsed Send Recv SendRecv
Size SizeSize Time Throughput localremote local remote
bytes bytes bytessecs.10^6bits/s % S % S us/KB us/KB
54 26214450010.00 2682.85 2.47 1.59 3.618 2.329
TcpExtTCPAutoCorking33 0.0
// Same test, but forcing TCP_NODELAY
$ nstat -n;./netperf -H 10.246.7.152 -Cc -- -D -m 500;nstat | grep AutoCork
MIGRATED TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 10.246.7.152
() port 0 AF_INET : nodelay
Recv SendSend Utilization Service Demand
Socket Socket Message Elapsed Send Recv SendRecv
Size SizeSize Time Throughput localremote local remote
bytes bytes bytessecs.10^6bits/s % S % S us/KB us/KB
54 26214450010.00 1408.75 2.44 2.96 6.802 8.259
TcpExtTCPAutoCorking1 0.0
After the fix :
$ nstat -n;./netperf -H 10.246.7.152 -Cc -- -m 500;nstat | grep AutoCork
MIGRATED TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 10.246.7.152
() port 0 AF_INET
Recv SendSend Utilization Service Demand
Socket Socket Message Elapsed Send Recv SendRecv
Size SizeSize Time Throughput localremote local remote
bytes bytes bytessecs.10^6bits/s % S % S us/KB us/KB
54 26214450010.00 5472.46 2.45 1.43 1.761 1.027
TcpExtTCPAutoCorking361293 0.0
// With TCP_NODELAY option
$ nstat -n;./netperf -H 10.246.7.152 -Cc -- -D -m 500;nstat | grep AutoCork
MIGRATED TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 10.246.7.152
() port 0 AF_INET : nodelay
Recv SendSend Utilization Service Demand
Socket Socket Message Elapsed Send Recv SendRecv
Size SizeSize Time Throughput localremote local remote
bytes bytes bytessecs.10^6bits/s % S % S us/KB us/KB
54 26214450010.00 5454.96 2.46 1.63 1.775 1.174
TcpExtTCPAutoCorking315448 0.0
Fixes: 75c119afe14f ("tcp: implement rb-tree based retransmit queue")
Signed-off-by: Eric Dumazet
Reported-by: Michael Wenig
Tested-by: Michael Wenig
---
net/ipv4/tcp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index
44be7f43455e4aefde8db61e2d941a69abcc642a..c9d00ef54deca15d5760bcbe154001a96fa1e2a7
100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -697,7 +697,7 @@ static bool tcp_should_autocork(struct sock *sk, struct
sk_buff *skb,
{
return skb->len < size_goal &&
sock_net(sk)->ipv4.sysctl_tcp_autocorking &&
- skb != tcp_write_queue_head(sk) &&
+ !tcp_rtx_queue_empty(sk) &&
refcount_read(&sk->sk_wmem_alloc) > skb->truesize;
}
--
2.17.0.441.gb46fe60e1d-goog
