[PATCH] tcp: refine autocork condition check

[Weiping Pan]

Inspired by commit b2532eb9abd8 (tcp: fix ooo_okay setting vs Small Queues).

The last check in tcp_should_autocork() was meant to check that whether we
only have an ACK in Qdisc/NIC queues, or if TX completion was delayed after we
processed ACK packet, if so, we should push the packet immediately instead of
corking it.
Therefore we should compare sk_wmem_alloc with SKB_TRUESIZE(1) instead of
skb->truesize.

After this patch, tcp should have more chances to be corked, and the
performance should be a little better. And netperf shows that this patch
works as expected.

./super_netperf.sh 300 -H 10.16.42.249 -t TCP_STREAM -- -m 1 -M 1
speed TCPAutoCorking
Before patch: 169.38 222278
After patch: 173.27 232988

Signed-off-by: Weiping Pan <***@gmail.com>
---
net/ipv4/tcp.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 461003d..44f0bc6 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -647,7 +647,7 @@ static bool tcp_should_autocork(struct sock *sk, struct sk_buff *skb,
return skb->len < size_goal &&
sysctl_tcp_autocorking &&
skb != tcp_write_queue_head(sk) &&
- atomic_read(&sk->sk_wmem_alloc) > skb->truesize;
+ atomic_read(&sk->sk_wmem_alloc) > SKB_TRUESIZE(1);
}

static void tcp_push(struct sock *sk, int flags, int mss_now,
@@ -675,7 +675,7 @@ static void tcp_push(struct sock *sk, int flags, int mss_now,
/* It is possible TX completion already happened
* before we set TSQ_THROTTLED.
*/
- if (atomic_read(&sk->sk_wmem_alloc) > skb->truesize)
+ if (atomic_read(&sk->sk_wmem_alloc) > SKB_TRUESIZE(1))
return;
}

[Eric Dumazet]

Post by Weiping Pan
Inspired by commit b2532eb9abd8 (tcp: fix ooo_okay setting vs Small Queues).
The last check in tcp_should_autocork() was meant to check that whether we
only have an ACK in Qdisc/NIC queues, or if TX completion was delayed after we
processed ACK packet, if so, we should push the packet immediately instead of
corking it.
Therefore we should compare sk_wmem_alloc with SKB_TRUESIZE(1) instead of
skb->truesize.
After this patch, tcp should have more chances to be corked, and the
performance should be a little better. And netperf shows that this patch
works as expected.
./super_netperf.sh 300 -H 10.16.42.249 -t TCP_STREAM -- -m 1 -M 1
speed TCPAutoCorking
Before patch: 169.38 222278
After patch: 173.27 232988

I do not see how this patch changes anything on this workload, I suspect
noise in your tests ? Full nstat output would give some hints maybe.

TCP_STREAM netperfs send no ACK packets at all.

I am concerned that this patch adds some latencies, and this wont be
seen with your TCP_STREAM test.

Autocorking is a trade off between throughput and latencies.

We need extensive tests, using TCP_RR with various sizes.

Existing behavior is telling that if a prior packet is in qdisc, and
this skb has a bigger truesize, we do not autocork.


In practice, you might hold now packets that are quite big, (more than
SKB_WITH_OVERHEAD(2048 - MAX_TCP_HEADER) bytes of payload.

Typical cases is applications using two writes, one to send a small
header, one for the body of the request/answer.

Existing code is better because we allow the second send() to be pushed
to the qdisc/NIC, before first send is TX completed.

[Weiping Pan]

Post by Eric Dumazet

Post by Weiping Pan
Inspired by commit b2532eb9abd8 (tcp: fix ooo_okay setting vs Small Queues).
The last check in tcp_should_autocork() was meant to check that whether we
only have an ACK in Qdisc/NIC queues, or if TX completion was delayed after we
processed ACK packet, if so, we should push the packet immediately instead of
corking it.
Therefore we should compare sk_wmem_alloc with SKB_TRUESIZE(1) instead of
skb->truesize.
After this patch, tcp should have more chances to be corked, and the
performance should be a little better. And netperf shows that this patch
works as expected.
./super_netperf.sh 300 -H 10.16.42.249 -t TCP_STREAM -- -m 1 -M 1
speed TCPAutoCorking
Before patch: 169.38 222278
After patch: 173.27 232988

I do not see how this patch changes anything on this workload, I suspect
noise in your tests ? Full nstat output would give some hints maybe.
TCP_STREAM netperfs send no ACK packets at all.
I am concerned that this patch adds some latencies, and this wont be
seen with your TCP_STREAM test.
Autocorking is a trade off between throughput and latencies.
We need extensive tests, using TCP_RR with various sizes.

With different packet size (1 128 1024 4096 10240 65536 131072),
TCP_RR test shows that the throughput does not have much difference,
and so with CPU usage and TCPAutoCorking times.

I thought pure ack would block the next data packet, but actually it is not.
I think that is because pure ack is sent out before the next data
packet is ready for transmit,
or it can be piggybacked.

Post by Eric Dumazet
Existing behavior is telling that if a prior packet is in qdisc, and
this skb has a bigger truesize, we do not autocork.
In practice, you might hold now packets that are quite big, (more than
SKB_WITH_OVERHEAD(2048 - MAX_TCP_HEADER) bytes of payload.
Typical cases is applications using two writes, one to send a small
header, one for the body of the request/answer.
Existing code is better because we allow the second send() to be pushed
to the qdisc/NIC, before first send is TX completed.

Agreed.

thanks
Weiping Pan