| Message ID | 1650967419-2150-1-git-send-email-yangpc@wangsu.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d9157f6806d1499e173770df1f1b234763de5c79 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] tcp: fix F-RTO may not work correctly when receiving DSACK | expand |
On Tue, Apr 26, 2022 at 6:04 AM Pengcheng Yang <yangpc@wangsu.com> wrote: > > Currently DSACK is regarded as a dupack, which may cause > F-RTO to incorrectly enter "loss was real" when receiving > DSACK. > > Packetdrill to demonstrate: > > // Enable F-RTO and TLP > 0 `sysctl -q net.ipv4.tcp_frto=2` > 0 `sysctl -q net.ipv4.tcp_early_retrans=3` > 0 `sysctl -q net.ipv4.tcp_congestion_control=cubic` > > // 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 10ms, RTO 210ms > +.1 < S 0:0(0) win 32792 <mss 1000,sackOK,nop,nop,nop,wscale 7> > +0 > S. 0:0(0) ack 1 <...> > +.01 < . 1:1(0) ack 1 win 257 > +0 accept(3, ..., ...) = 4 > > // Send 2 data segments > +0 write(4, ..., 2000) = 2000 > +0 > P. 1:2001(2000) ack 1 > > // TLP > +.022 > P. 1001:2001(1000) ack 1 > > // Continue to send 8 data segments > +0 write(4, ..., 10000) = 10000 > +0 > P. 2001:10001(8000) ack 1 > > // RTO > +.188 > . 1:1001(1000) ack 1 > > // The original data is acked and new data is sent(F-RTO step 2.b) > +0 < . 1:1(0) ack 2001 win 257 > +0 > P. 10001:12001(2000) ack 1 > > // D-SACK caused by TLP is regarded as a dupack, this results in > // the incorrect judgment of "loss was real"(F-RTO step 3.a) > +.022 < . 1:1(0) ack 2001 win 257 <sack 1001:2001,nop,nop> > > // Never-retransmitted data(3001:4001) are acked and > // expect to switch to open state(F-RTO step 3.b) > +0 < . 1:1(0) ack 4001 win 257 > +0 %{ assert tcpi_ca_state == 0, tcpi_ca_state }% > > Fixes: e33099f96d99 ("tcp: implement RFC5682 F-RTO") > Signed-off-by: Pengcheng Yang <yangpc@wangsu.com> > --- Thanks for the fix and test! Both look good to me. The patch passes all of our team's packetdrill tests, and this new test passes as well. Acked-by: Neal Cardwell <ncardwell@google.com> Tested-by: Neal Cardwell <ncardwell@google.com> thanks, neal
On Thu, Apr 28, 2022 at 10:06 AM Neal Cardwell <ncardwell@google.com> wrote: > > > Thanks for the fix and test! Both look good to me. The patch passes > all of our team's packetdrill tests, and this new test passes as well. > > Acked-by: Neal Cardwell <ncardwell@google.com> > Tested-by: Neal Cardwell <ncardwell@google.com> > Indeed, thank you all for a nice changelog, nice patch, and exhaustive tests from Neal. Reviewed-by: Eric Dumazet <edumazet@google.com>
Hello: This patch was applied to netdev/net.git (master) by Jakub Kicinski <kuba@kernel.org>: On Tue, 26 Apr 2022 18:03:39 +0800 you wrote: > Currently DSACK is regarded as a dupack, which may cause > F-RTO to incorrectly enter "loss was real" when receiving > DSACK. > > Packetdrill to demonstrate: > > // Enable F-RTO and TLP > 0 `sysctl -q net.ipv4.tcp_frto=2` > 0 `sysctl -q net.ipv4.tcp_early_retrans=3` > 0 `sysctl -q net.ipv4.tcp_congestion_control=cubic` > > [...] Here is the summary with links: - [net] tcp: fix F-RTO may not work correctly when receiving DSACK https://git.kernel.org/netdev/net/c/d9157f6806d1 You are awesome, thank you!
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index 1595b76..c54accc 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -3867,7 +3867,8 @@ static int tcp_ack(struct sock *sk, const struct sk_buff *skb, int flag) tcp_process_tlp_ack(sk, ack, flag); if (tcp_ack_is_dubious(sk, flag)) { - if (!(flag & (FLAG_SND_UNA_ADVANCED | FLAG_NOT_DUP))) { + if (!(flag & (FLAG_SND_UNA_ADVANCED | + FLAG_NOT_DUP | FLAG_DSACKING_ACK))) { num_dupack = 1; /* Consider if pure acks were aggregated in tcp_add_backlog() */ if (!(flag & FLAG_DATA))
Currently DSACK is regarded as a dupack, which may cause F-RTO to incorrectly enter "loss was real" when receiving DSACK. Packetdrill to demonstrate: // Enable F-RTO and TLP 0 `sysctl -q net.ipv4.tcp_frto=2` 0 `sysctl -q net.ipv4.tcp_early_retrans=3` 0 `sysctl -q net.ipv4.tcp_congestion_control=cubic` // 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 10ms, RTO 210ms +.1 < S 0:0(0) win 32792 <mss 1000,sackOK,nop,nop,nop,wscale 7> +0 > S. 0:0(0) ack 1 <...> +.01 < . 1:1(0) ack 1 win 257 +0 accept(3, ..., ...) = 4 // Send 2 data segments +0 write(4, ..., 2000) = 2000 +0 > P. 1:2001(2000) ack 1 // TLP +.022 > P. 1001:2001(1000) ack 1 // Continue to send 8 data segments +0 write(4, ..., 10000) = 10000 +0 > P. 2001:10001(8000) ack 1 // RTO +.188 > . 1:1001(1000) ack 1 // The original data is acked and new data is sent(F-RTO step 2.b) +0 < . 1:1(0) ack 2001 win 257 +0 > P. 10001:12001(2000) ack 1 // D-SACK caused by TLP is regarded as a dupack, this results in // the incorrect judgment of "loss was real"(F-RTO step 3.a) +.022 < . 1:1(0) ack 2001 win 257 <sack 1001:2001,nop,nop> // Never-retransmitted data(3001:4001) are acked and // expect to switch to open state(F-RTO step 3.b) +0 < . 1:1(0) ack 4001 win 257 +0 %{ assert tcpi_ca_state == 0, tcpi_ca_state }% Fixes: e33099f96d99 ("tcp: implement RFC5682 F-RTO") Signed-off-by: Pengcheng Yang <yangpc@wangsu.com> --- net/ipv4/tcp_input.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)