* [PATCH net] tcp: restore autocorking
@ 2018-05-03 3:25 Eric Dumazet
2018-05-03 11:06 ` Tariq Toukan
` (4 more replies)
0 siblings, 5 replies; 7+ messages in thread
From: Eric Dumazet @ 2018-05-03 3:25 UTC (permalink / raw)
To: David S . Miller; +Cc: netdev, Eric Dumazet, Michael Wenig, Eric Dumazet
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 Send Send Utilization Service Demand
Socket Socket Message Elapsed Send Recv Send Recv
Size Size Size Time Throughput local remote local remote
bytes bytes bytes secs. 10^6bits/s % S % S us/KB us/KB
540000 262144 500 10.00 2682.85 2.47 1.59 3.618 2.329
TcpExtTCPAutoCorking 33 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 Send Send Utilization Service Demand
Socket Socket Message Elapsed Send Recv Send Recv
Size Size Size Time Throughput local remote local remote
bytes bytes bytes secs. 10^6bits/s % S % S us/KB us/KB
540000 262144 500 10.00 1408.75 2.44 2.96 6.802 8.259
TcpExtTCPAutoCorking 1 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 Send Send Utilization Service Demand
Socket Socket Message Elapsed Send Recv Send Recv
Size Size Size Time Throughput local remote local remote
bytes bytes bytes secs. 10^6bits/s % S % S us/KB us/KB
540000 262144 500 10.00 5472.46 2.45 1.43 1.761 1.027
TcpExtTCPAutoCorking 361293 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 Send Send Utilization Service Demand
Socket Socket Message Elapsed Send Recv Send Recv
Size Size Size Time Throughput local remote local remote
bytes bytes bytes secs. 10^6bits/s % S % S us/KB us/KB
540000 262144 500 10.00 5454.96 2.46 1.63 1.775 1.174
TcpExtTCPAutoCorking 315448 0.0
Fixes: 75c119afe14f ("tcp: implement rb-tree based retransmit queue")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Michael Wenig <mwenig@vmware.com>
Tested-by: Michael Wenig <mwenig@vmware.com>
---
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net] tcp: restore autocorking
2018-05-03 3:25 [PATCH net] tcp: restore autocorking Eric Dumazet
@ 2018-05-03 11:06 ` Tariq Toukan
2018-05-03 13:55 ` Eric Dumazet
2018-05-03 14:27 ` Michael Wenig
` (3 subsequent siblings)
4 siblings, 1 reply; 7+ messages in thread
From: Tariq Toukan @ 2018-05-03 11:06 UTC (permalink / raw)
To: Eric Dumazet, David S . Miller, Tal Gilboa
Cc: netdev, Michael Wenig, Eric Dumazet
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/
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] tcp: restore autocorking
2018-05-03 11:06 ` Tariq Toukan
@ 2018-05-03 13:55 ` Eric Dumazet
0 siblings, 0 replies; 7+ messages in thread
From: Eric Dumazet @ 2018-05-03 13:55 UTC (permalink / raw)
To: Tariq Toukan
Cc: David Miller, Tal Gilboa, netdev, Michael Wenig, Eric Dumazet
On Thu, May 3, 2018 at 4:06 AM Tariq Toukan <tariqt@mellanox.com> 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 :)
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH net] tcp: restore autocorking
2018-05-03 3:25 [PATCH net] tcp: restore autocorking Eric Dumazet
2018-05-03 11:06 ` Tariq Toukan
@ 2018-05-03 14:27 ` Michael Wenig
2018-05-03 14:48 ` Neal Cardwell
` (2 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Michael Wenig @ 2018-05-03 14:27 UTC (permalink / raw)
To: Eric Dumazet, David S . Miller; +Cc: netdev, Eric Dumazet
Thank you, Eric, for fixing this!
Michael
-----Original Message-----
From: Eric Dumazet [mailto:edumazet@google.com]
Sent: Wednesday, May 2, 2018 8:25 PM
To: David S . Miller <davem@davemloft.net>
Cc: netdev <netdev@vger.kernel.org>; Eric Dumazet <edumazet@google.com>; Michael Wenig <mwenig@vmware.com>; Eric Dumazet <eric.dumazet@gmail.com>
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 Send Send Utilization Service Demand
Socket Socket Message Elapsed Send Recv Send Recv
Size Size Size Time Throughput local remote local remote
bytes bytes bytes secs. 10^6bits/s % S % S us/KB us/KB
540000 262144 500 10.00 2682.85 2.47 1.59 3.618 2.329
TcpExtTCPAutoCorking 33 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 Send Send Utilization Service Demand
Socket Socket Message Elapsed Send Recv Send Recv
Size Size Size Time Throughput local remote local remote
bytes bytes bytes secs. 10^6bits/s % S % S us/KB us/KB
540000 262144 500 10.00 1408.75 2.44 2.96 6.802 8.259
TcpExtTCPAutoCorking 1 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 Send Send Utilization Service Demand
Socket Socket Message Elapsed Send Recv Send Recv
Size Size Size Time Throughput local remote local remote
bytes bytes bytes secs. 10^6bits/s % S % S us/KB us/KB
540000 262144 500 10.00 5472.46 2.45 1.43 1.761 1.027
TcpExtTCPAutoCorking 361293 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 Send Send Utilization Service Demand
Socket Socket Message Elapsed Send Recv Send Recv
Size Size Size Time Throughput local remote local remote
bytes bytes bytes secs. 10^6bits/s % S % S us/KB us/KB
540000 262144 500 10.00 5454.96 2.46 1.63 1.775 1.174
TcpExtTCPAutoCorking 315448 0.0
Fixes: 75c119afe14f ("tcp: implement rb-tree based retransmit queue")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Michael Wenig <mwenig@vmware.com>
Tested-by: Michael Wenig <mwenig@vmware.com>
---
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; }
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] tcp: restore autocorking
2018-05-03 3:25 [PATCH net] tcp: restore autocorking Eric Dumazet
2018-05-03 11:06 ` Tariq Toukan
2018-05-03 14:27 ` Michael Wenig
@ 2018-05-03 14:48 ` Neal Cardwell
2018-05-03 14:52 ` Soheil Hassas Yeganeh
2018-05-03 15:29 ` David Miller
4 siblings, 0 replies; 7+ messages in thread
From: Neal Cardwell @ 2018-05-03 14:48 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, Netdev, mwenig, Eric Dumazet
On Wed, May 2, 2018 at 11:25 PM Eric Dumazet <edumazet@google.com> 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 <edumazet@google.com>
> Reported-by: Michael Wenig <mwenig@vmware.com>
> Tested-by: Michael Wenig <mwenig@vmware.com>
> ---
Acked-by: Neal Cardwell <ncardwell@google.com>
Nice. Thanks, Eric!
neal
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] tcp: restore autocorking
2018-05-03 3:25 [PATCH net] tcp: restore autocorking Eric Dumazet
` (2 preceding siblings ...)
2018-05-03 14:48 ` Neal Cardwell
@ 2018-05-03 14:52 ` Soheil Hassas Yeganeh
2018-05-03 15:29 ` David Miller
4 siblings, 0 replies; 7+ messages in thread
From: Soheil Hassas Yeganeh @ 2018-05-03 14:52 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S . Miller, netdev, Michael Wenig, Eric Dumazet
On Wed, May 2, 2018 at 11:25 PM, Eric Dumazet <edumazet@google.com> wrote:
> Fixes: 75c119afe14f ("tcp: implement rb-tree based retransmit queue")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Michael Wenig <mwenig@vmware.com>
> Tested-by: Michael Wenig <mwenig@vmware.com>
Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
Thank you for catching and fixing this!
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net] tcp: restore autocorking
2018-05-03 3:25 [PATCH net] tcp: restore autocorking Eric Dumazet
` (3 preceding siblings ...)
2018-05-03 14:52 ` Soheil Hassas Yeganeh
@ 2018-05-03 15:29 ` David Miller
4 siblings, 0 replies; 7+ messages in thread
From: David Miller @ 2018-05-03 15:29 UTC (permalink / raw)
To: edumazet; +Cc: netdev, mwenig, eric.dumazet
From: Eric Dumazet <edumazet@google.com>
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 <edumazet@google.com>
> Reported-by: Michael Wenig <mwenig@vmware.com>
> Tested-by: Michael Wenig <mwenig@vmware.com>
Applied and queued up for -stable, thanks Eric.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-05-03 15:29 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-03 3:25 [PATCH net] tcp: restore autocorking Eric Dumazet
2018-05-03 11:06 ` Tariq Toukan
2018-05-03 13:55 ` Eric Dumazet
2018-05-03 14:27 ` Michael Wenig
2018-05-03 14:48 ` Neal Cardwell
2018-05-03 14:52 ` Soheil Hassas Yeganeh
2018-05-03 15:29 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.