Re: [PATCH] tcp: refine autocork condition check

[Weiping Pan <panweiping3@gmail.com>]

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