Message ID | 8e0610510498c7f6ecbe2e99ab6044030f93f792.1413369212.git.panweiping3@gmail.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
On Wed, 2014-10-15 at 18:34 +0800, Weiping Pan wrote: > 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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/15/2014 07:47 PM, Eric Dumazet wrote: > On Wed, 2014-10-15 at 18:34 +0800, Weiping Pan wrote: >> 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. > > 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }
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 <panweiping3@gmail.com> --- net/ipv4/tcp.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)