Re: [PATCH net] tcp: fix TLP timer not set when CA_STATE changes from DISORDER to OPEN

On Sat, Jan 23, 2021 at 5:02 AM "Yuchung Cheng" [off-list ref] wrote:

On Fri, Jan 22, 2021 at 6:37 AM Neal Cardwell [off-list ref] wrote:

quoted

On Fri, Jan 22, 2021 at 5:53 AM Eric Dumazet [off-list ref] wrote:

quoted

On Fri, Jan 22, 2021 at 11:28 AM Pengcheng Yang [off-list ref] wrote:

quoted

When CA_STATE is in DISORDER, the TLP timer is not set when receiving
an ACK (a cumulative ACK covered out-of-order data) causes CA_STATE to
change from DISORDER to OPEN. If the sender is app-limited, it can only

Could you point which line of code causes the state to flip
incorrectly due to the TLP timer setting?

I mean TLP timer is not set due to receiving an ACK that changes CA_STATE 
from DISORDER to OPEN.

Receive an ACK covered out-of-order data in disorder state:

tcp_ack()
|-tcp_set_xmit_timer()	// RTO timer is set instead of TLP timer
|  ...
|-tcp_fastretrans_alert() // change from disorder to open

quoted

quoted

quoted

wait for the RTO timer to expire and retransmit.

The reason for this is that the TLP timer is set before CA_STATE changes
in tcp_ack(), so we delay the time point of calling tcp_set_xmit_timer()
until after tcp_fastretrans_alert() returns and remove the
FLAG_SET_XMIT_TIMER from ack_flag when the RACK reorder timer is set.

This commit has two additional benefits:
1) Make sure to reset RTO according to RFC6298 when receiving ACK, to
avoid spurious RTO caused by RTO timer early expires.
2) Reduce the xmit timer reschedule once per ACK when the RACK reorder
timer is set.

Link: https://lore.kernel.org/netdev/1611139794-11254-1-git-send-email-yangpc@wangsu.com (local)
Signed-off-by: Pengcheng Yang <redacted>
Cc: Neal Cardwell <ncardwell@google.com>
---

This looks like a very nice patch, let me run packetdrill tests on it.

By any chance, have you cooked a packetdrill test showing the issue
(failing on unpatched kernel) ?

Thanks, Pengcheng. This patch looks good to me as well, assuming it
passes our packetdrill tests. I agree with Eric that it would be good
to have an explicit packetdrill test for this case.

neal

Here is a packetdrill test case:

// Enable TLP
    0 `sysctl -q net.ipv4.tcp_early_retrans=3`

// Establish a connection
   +0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
   +0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
   +0 bind(3, ..., ...) = 0
   +0 listen(3, 1) = 0

// RTT 100ms, RTO 300ms
  +.1 < S 0:0(0) win 32792 <mss 1000,sackOK,nop,nop,nop,wscale 7>
   +0 > S. 0:0(0) ack 1 <...>
  +.1 < . 1:1(0) ack 1 win 257
   +0 accept(3, ..., ...) = 4

// Send 4 data segments
   +0 write(4, ..., 4000) = 4000
   +0 > P. 1:4001(4000) ack 1

// out-of-order: ca_state turns to disorder
  +.1 < . 1:1(0) ack 1 win 257 <sack 1001:2001,nop,nop>

// ACK covered out-of-order data: ca_state turns to open,
// but RTO timer is set instead of TLP timer and the RTO 
// timer will expire at rtx_head_time+RTO (in 200ms).
   +0 < . 1:1(0) ack 2001 win 257

// Expect to send TLP packet in 2*rtt (200ms)
+.2~+.25 > P. 3001:4001(1000) ack 1


I ran this packetdrill test case on the kernel without 
the patch applied:

tlp_timer_unset.pkt:31: error handling packet: live packet 
field tcp_seq: expected: 3001 (0xbb9) vs actual: 2001 (0x7d1)
script packet:  0.644587 P. 3001:4001(1000) ack 1 
actual packet:  0.646197 . 2001:3001(1000) ack 1 win 502