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

On Sat, 23 Jan 2021 13:25:23 -0500 Neal Cardwell wrote:

On Sat, Jan 23, 2021 at 9:15 AM Pengcheng Yang [off-list ref] wrote:

quoted

On Sat, Jan 23, 2021 at 9:27 AM "Jakub Kicinski" [off-list ref] wrote:  

quoted

On Fri, 22 Jan 2021 11:53:46 +0100 Eric Dumazet 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
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) ?  

Any guidance on backporting / fixes tag? (once the packetdrill
questions are satisfied)  

By reading the commits, we can add:
Fixes: df92c8394e6e ("tcp: fix xmit timer to only be reset if data ACKed/SACKed")  

Thanks for the fix and the packetdrill test!

Eric ran our internal packetdrill test suite and all the changes in
behavior with your patch all look like fixes to me.

Acked-by: Neal Cardwell <ncardwell@google.com>

Thanks!

The Fixes: tag you suggest also looks good to me. (It seems like
patchwork did not pick it up from your email and add it to the commit
message automatically, BTW?)

Indeed, we haven't worked that out yet in patchwork.kernel.org, but
it's no big deal, normally we can add manually. In this case seems 
like Yuchung asked for a small rewrite to the commit message so I'd
appreciate a full repost.