Page MenuHomeFreeBSD
Differential D18624

improvements to support code for RFC6675
ClosedPublic
Actions

Authored by rscheff on Dec 20 2018, 7:44 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 7, 1:32 PM
Unknown Object (File)
Wed, Nov 6, 12:06 AM
Unknown Object (File)
Tue, Nov 5, 10:45 AM
Unknown Object (File)
Sun, Oct 20, 4:11 PM
Unknown Object (File)
Wed, Oct 16, 2:21 PM
Unknown Object (File)
Wed, Oct 16, 8:43 AM
Unknown Object (File)
Tue, Oct 15, 6:22 PM
Unknown Object (File)
Tue, Oct 15, 5:22 AM
View All Files
Subscribers
imp
melifaro

Details

Reviewers
hiren
jtl
lstewart
rrs
tuexen
thj
rgrimes
cc
jason_eggnet.com
kbowling
Group Reviewers
transport
Commits
rS365292: MFC r364195: Improve SACK support code for RFC6675 and PRR
rS364195: Improve SACK support code for RFC6675 and PRR
Summary

Added proper accounting of sacked_bytes and (per-ACK) delivered data.

Previous code was susceptible to improperly calculate these values, when

a) more than 3 (or 4, without TS) holes were to be reported
b) simplistic clients only sending a SACK block covering the last received data segment (simplistic IoT devices)
c) malicious receivers sending duplicate SACK blocks in the ACK to deflate pipe (possibly trigger burst transmissions by the sender).

As net.inet.tcp.rfc6675 controls this particular feature, and is disabled by default, the above issues are not critical.

This patch will allow more aspects of RFC6675 to be implemented. One is when Loss Recovery is invoked, the other is the Rescue Retransmission when TCP is application limited.

Test Plan

base:

10 ## 12 13 ## 15 16 ## 18 19 ## 21 22 ## 24 25 ## 27 28 ## 30
               11                   14    17 20    23 26    29

old 6675-pipe:

10 ## 12 13 ## 15 16 ## 18 19 ## 21 22 ## 24 25 ## 27 28 ## 30
               11
         14,17    20,23    26,29

D18624 6675-pipe:

10 ## 12 13 ## 15 16 ## 18 19 ## 21 22 ## 24 25 ## 27 28 ## 30
               11                                     14    17
         20,23    26,29

First, an example of the non-RFC6675-pipe SACK loss recovery packet sequence:

// Testing base-stack SACK loss recovery
// with only a single SACK option block
// returned from the receiver

--tolerance_usecs=100000

0.0 `sync` // in case of crash
+0.01 `sysctl net.inet.tcp.cc.algorithm=newreno`
+0.01 `sysctl net.inet.tcp.initcwnd_segments=10`
+0.01 `sysctl net.inet.tcp.ecn.enable=0`
+0.01 `sysctl net.inet.tcp.rfc6675_pipe=0`
+0.01 `sysctl net.inet.tcp.hostcache.purgenow=1`

// Create a listening TCP socket.
+0.50 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
+0.01 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
+0.01 setsockopt(3, SOL_SOCKET, SO_SNDBUF, [1048576], 4) = 0
+0.01 setsockopt(3, SOL_SOCKET, SO_DEBUG, [1], 4) = 0
+0.01 bind(3, ..., ...) = 0
+0.01 listen(3, 1) = 0

// Establish a TCP connection.
+0.04 < S  0:0(0) win 65535 <mss 1000, sackOK, wscale 10, eol, nop, nop >
+0.00 > S. 0:0(0) ack 1 win 65535 <mss 1460,nop,wscale 6,sackOK,eol,eol>
+0.00 <  . 1:1(0) ack 1 win 65535
+0.00 accept(3, ..., ...) = 4
+0.01 setsockopt(4, SOL_SOCKET, SO_DEBUG, [1], 4) = 0

// ACK initial window, to grow cwnd to sufficient size
1.00 write(4, ..., 30000) = 30000
+0 > .    1:1001(1000) ack 1
+0 < .    1:1(0) ack 1001 win 65535
+0 > . 1001:2001(1000) ack 1
+0 < .    1:1(0) ack 2001 win 65535
+0 > . 2001:3001(1000) ack 1
+0 < .    1:1(0) ack 3001 win 65535
+0 > . 3001:4001(1000) ack 1
+0 < .    1:1(0) ack 4001 win 65535
+0 > . 4001:5001(1000) ack 1
+0 < .    1:1(0) ack 5001 win 65535
+0 > . 5001:6001(1000) ack 1
+0 < .    1:1(0) ack 6001 win 65535
+0 > . 6001:7001(1000) ack 1
+0 < .    1:1(0) ack 7001 win 65535
+0 > . 7001:8001(1000) ack 1
+0 < .    1:1(0) ack 8001 win 65535
+0 > . 8001:9001(1000) ack 1
+0 < .    1:1(0) ack 9001 win 65535
+0 > . 9001:10001(1000) ack 1
+0 < .    1:1(0) ack 10001 win 65535

// in the next window, we simulate the losses
+0 > . 10001:11001(1000) ack 1  // drop
+0 > . 11001:12001(1000) ack 1
+0 > . 12001:13001(1000) ack 1
+0 > . 13001:14001(1000) ack 1  // drop
+0 > . 14001:15001(1000) ack 1
+0 > . 15001:16001(1000) ack 1
+0 > . 16001:17001(1000) ack 1  // drop
+0 > . 17001:18001(1000) ack 1
+0 > . 18001:19001(1000) ack 1
+0 > . 19001:20001(1000) ack 1  // drop
+0 > . 20001:21001(1000) ack 1
+0 > . 21001:22001(1000) ack 1
+0 > . 22001:23001(1000) ack 1  // drop
+0 > . 23001:24001(1000) ack 1
+0 > . 24001:25001(1000) ack 1
+0 > . 25001:26001(1000) ack 1  // drop
+0 > . 26001:27001(1000) ack 1
+0 > . 27001:28001(1000) ack 1
+0 > . 28001:29001(1000) ack 1  // drop
+0 > P. 29001:30001(1000) ack 1

// and now we validate the ordering of the retransmissions
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 11001:12001, nop, nop>
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 11001:13001, nop, nop>
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 14001:15001, nop, nop>
* > . 10001:11001(1000) ack 1
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 14001:16001, nop, nop>
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 17001:18001, nop, nop>
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 17001:19001, nop, nop>
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 20001:21001, nop, nop>
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 20001:22001, nop, nop>
* > . 13001:14001(1000) ack 1
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 23001:24001, nop, nop>
* > . 16001:17001(1000) ack 1
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 23001:25001, nop, nop>
* > . 19001:20001(1000) ack 1
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 26001:27001, nop, nop>
* > . 22001:23001(1000) ack 1
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 26001:28001, nop, nop>
* > . 25001:26001(1000) ack 1
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 29001:30001, nop, nop>
* > . 28001:29001(1000) ack 1
+0.01 < .  1:1(0) ack 13001 win 65535 <sack 29001:30001, nop, nop>
+0.01 < .  1:1(0) ack 16001 win 65535 <sack 29001:30001, nop, nop>
+0.01 < .  1:1(0) ack 19001 win 65535 <sack 29001:30001, nop, nop>
+0.01 < .  1:1(0) ack 22001 win 65535 <sack 29001:30001, nop, nop>
+0.01 < .  1:1(0) ack 25001 win 65535 <sack 29001:30001, nop, nop>
+0.01 < .  1:1(0) ack 28001 win 65535 <sack 29001:30001, nop, nop>
+0.01 < .  1:1(0) ack 30001 win 65535
+1.00 close(4) = 0

`sysctl net.inet.tcp.rfc6675_pipe=0`

Then, the old RFC6675-pipe sequence:

// Testing D18624 RFC6675 pipe SACK loss recovery
// with only a single SACK option block
// returned from the receiver

--tolerance_usecs=100000

0.0 `sync` // in case of crash
+0.01 `sysctl net.inet.tcp.cc.algorithm=newreno`
+0.01 `sysctl net.inet.tcp.initcwnd_segments=10`
+0.01 `sysctl net.inet.tcp.ecn.enable=0`
+0.01 `sysctl net.inet.tcp.rfc6675_pipe=1`
+0.01 `sysctl net.inet.tcp.hostcache.purgenow=1`

// Create a listening TCP socket.
+0.50 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
+0.01 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
+0.01 setsockopt(3, SOL_SOCKET, SO_SNDBUF, [1048576], 4) = 0
+0.01 setsockopt(3, SOL_SOCKET, SO_DEBUG, [1], 4) = 0
+0.01 bind(3, ..., ...) = 0
+0.01 listen(3, 1) = 0

// Establish a TCP connection.
+0.04 < S  0:0(0) win 65535 <mss 1000, sackOK, wscale 10, eol, nop, nop >
+0.00 > S. 0:0(0) ack 1 win 65535 <mss 1460,nop,wscale 6,sackOK,eol,eol>
+0.00 <  . 1:1(0) ack 1 win 65535
+0.00 accept(3, ..., ...) = 4
+0.01 setsockopt(4, SOL_SOCKET, SO_DEBUG, [1], 4) = 0

// ACK initial window, to grow cwnd to sufficient size
1.00 write(4, ..., 30000) = 30000
+0 > .    1:1001(1000) ack 1
+0 < .    1:1(0) ack 1001 win 65535
+0 > . 1001:2001(1000) ack 1
+0 < .    1:1(0) ack 2001 win 65535
+0 > . 2001:3001(1000) ack 1
+0 < .    1:1(0) ack 3001 win 65535
+0 > . 3001:4001(1000) ack 1
+0 < .    1:1(0) ack 4001 win 65535
+0 > . 4001:5001(1000) ack 1
+0 < .    1:1(0) ack 5001 win 65535
+0 > . 5001:6001(1000) ack 1
+0 < .    1:1(0) ack 6001 win 65535
+0 > . 6001:7001(1000) ack 1
+0 < .    1:1(0) ack 7001 win 65535
+0 > . 7001:8001(1000) ack 1
+0 < .    1:1(0) ack 8001 win 65535
+0 > . 8001:9001(1000) ack 1
+0 < .    1:1(0) ack 9001 win 65535
+0 > . 9001:10001(1000) ack 1
+0 < .    1:1(0) ack 10001 win 65535

// in the next window, we simulate the losses
+0 > . 10001:11001(1000) ack 1  // drop
+0 > . 11001:12001(1000) ack 1
+0 > . 12001:13001(1000) ack 1
+0 > . 13001:14001(1000) ack 1  // drop
+0 > . 14001:15001(1000) ack 1
+0 > . 15001:16001(1000) ack 1
+0 > . 16001:17001(1000) ack 1  // drop
+0 > . 17001:18001(1000) ack 1
+0 > . 18001:19001(1000) ack 1
+0 > . 19001:20001(1000) ack 1  // drop
+0 > . 20001:21001(1000) ack 1
+0 > . 21001:22001(1000) ack 1
+0 > . 22001:23001(1000) ack 1  // drop
+0 > . 23001:24001(1000) ack 1
+0 > . 24001:25001(1000) ack 1
+0 > . 25001:26001(1000) ack 1  // drop
+0 > . 26001:27001(1000) ack 1
+0 > . 27001:28001(1000) ack 1
+0 > . 28001:29001(1000) ack 1  // drop
+0 > P. 29001:30001(1000) ack 1

// and now we validate the ordering of the retransmissions
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 11001:12001, nop, nop>
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 11001:13001, nop, nop>
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 14001:15001, nop, nop>
* > . 10001:11001(1000) ack 1
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 14001:16001, nop, nop>
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 17001:18001, nop, nop>
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 17001:19001, nop, nop>
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 20001:21001, nop, nop>
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 20001:22001, nop, nop>
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 23001:24001, nop, nop>
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 23001:25001, nop, nop>
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 26001:27001, nop, nop>
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 26001:28001, nop, nop>
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 29001:30001, nop, nop>
+0.01 < .  1:1(0) ack 13001 win 65535 <sack 29001:30001, nop, nop>
* > . 13001:14001(1000) ack 1
* > . 16001:17001(1000) ack 1
+0.01 < .  1:1(0) ack 16001 win 65535 <sack 29001:30001, nop, nop>
* > . 19001:20001(1000) ack 1
* > . 22001:23001(1000) ack 1
+0.01 < .  1:1(0) ack 19001 win 65535 <sack 29001:30001, nop, nop>
* > . 25001:26001(1000) ack 1
* > . 28001:29001(1000) ack 1
+0.01 < .  1:1(0) ack 22001 win 65535 <sack 29001:30001, nop, nop>
+0.01 < .  1:1(0) ack 25001 win 65535 <sack 29001:30001, nop, nop>
+0.01 < .  1:1(0) ack 28001 win 65535 <sack 29001:30001, nop, nop>
+0.01 < .  1:1(0) ack 30001 win 65535

+1.00 close(4) = 0

`sysctl net.inet.tcp.rfc6675_pipe=0`

And finally, the change with this Patch:

// Testing D18624 RFC6675 pipe SACK loss recovery
// with only a single SACK option block
// returned from the receiver

--tolerance_usecs=100000

0.0 `sync` // in case of crash
+0.01 `sysctl net.inet.tcp.cc.algorithm=newreno`
+0.01 `sysctl net.inet.tcp.initcwnd_segments=10`
+0.01 `sysctl net.inet.tcp.ecn.enable=0`
+0.01 `sysctl net.inet.tcp.rfc6675_pipe=1`
+0.01 `sysctl net.inet.tcp.hostcache.purgenow=1`

// Create a listening TCP socket.
+0.50 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
+0.01 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
+0.01 setsockopt(3, SOL_SOCKET, SO_SNDBUF, [1048576], 4) = 0
+0.01 setsockopt(3, SOL_SOCKET, SO_DEBUG, [1], 4) = 0
+0.01 bind(3, ..., ...) = 0
+0.01 listen(3, 1) = 0

// Establish a TCP connection.
+0.04 < S  0:0(0) win 65535 <mss 1000, sackOK, wscale 10, eol, nop, nop >
+0.00 > S. 0:0(0) ack 1 win 65535 <mss 1460,nop,wscale 6,sackOK,eol,eol>
+0.00 <  . 1:1(0) ack 1 win 65535
+0.00 accept(3, ..., ...) = 4
+0.01 setsockopt(4, SOL_SOCKET, SO_DEBUG, [1], 4) = 0

// ACK initial window, to grow cwnd to sufficient size
1.00 write(4, ..., 30000) = 30000
+0 > .    1:1001(1000) ack 1
+0 < .    1:1(0) ack 1001 win 65535
+0 > . 1001:2001(1000) ack 1
+0 < .    1:1(0) ack 2001 win 65535
+0 > . 2001:3001(1000) ack 1
+0 < .    1:1(0) ack 3001 win 65535
+0 > . 3001:4001(1000) ack 1
+0 < .    1:1(0) ack 4001 win 65535
+0 > . 4001:5001(1000) ack 1
+0 < .    1:1(0) ack 5001 win 65535
+0 > . 5001:6001(1000) ack 1
+0 < .    1:1(0) ack 6001 win 65535
+0 > . 6001:7001(1000) ack 1
+0 < .    1:1(0) ack 7001 win 65535
+0 > . 7001:8001(1000) ack 1
+0 < .    1:1(0) ack 8001 win 65535
+0 > . 8001:9001(1000) ack 1
+0 < .    1:1(0) ack 9001 win 65535
+0 > . 9001:10001(1000) ack 1
+0 < .    1:1(0) ack 10001 win 65535

// in the next window, we simulate the losses
+0 > . 10001:11001(1000) ack 1  // drop
+0 > . 11001:12001(1000) ack 1
+0 > . 12001:13001(1000) ack 1
+0 > . 13001:14001(1000) ack 1  // drop
+0 > . 14001:15001(1000) ack 1
+0 > . 15001:16001(1000) ack 1
+0 > . 16001:17001(1000) ack 1  // drop
+0 > . 17001:18001(1000) ack 1
+0 > . 18001:19001(1000) ack 1
+0 > . 19001:20001(1000) ack 1  // drop
+0 > . 20001:21001(1000) ack 1
+0 > . 21001:22001(1000) ack 1
+0 > . 22001:23001(1000) ack 1  // drop
+0 > . 23001:24001(1000) ack 1
+0 > . 24001:25001(1000) ack 1
+0 > . 25001:26001(1000) ack 1  // drop
+0 > . 26001:27001(1000) ack 1
+0 > . 27001:28001(1000) ack 1
+0 > . 28001:29001(1000) ack 1  // drop
+0 > P. 29001:30001(1000) ack 1

// and now we validate the ordering of the retransmissions
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 11001:12001, nop, nop>
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 11001:13001, nop, nop>
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 14001:15001, nop, nop>
* > . 10001:11001(1000) ack 1
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 14001:16001, nop, nop>
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 17001:18001, nop, nop>
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 17001:19001, nop, nop>
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 20001:21001, nop, nop>
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 20001:22001, nop, nop>
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 23001:24001, nop, nop>
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 23001:25001, nop, nop>
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 26001:27001, nop, nop>
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 26001:28001, nop, nop>
* > . 13001:14001(1000) ack 1
+0.01 < .  1:1(0) ack 10001 win 65535 <sack 29001:30001, nop, nop>
* > . 16001:17001(1000) ack 1
+0.01 < .  1:1(0) ack 13001 win 65535 <sack 29001:30001, nop, nop>
* > . 19001:20001(1000) ack 1
* > . 22001:23001(1000) ack 1
+0.01 < .  1:1(0) ack 16001 win 65535 <sack 29001:30001, nop, nop>
* > . 25001:26001(1000) ack 1
* > . 28001:29001(1000) ack 1
+0.01 < .  1:1(0) ack 19001 win 65535 <sack 29001:30001, nop, nop>
+0.01 < .  1:1(0) ack 22001 win 65535 <sack 29001:30001, nop, nop>
+0.01 < .  1:1(0) ack 25001 win 65535 <sack 29001:30001, nop, nop>
+0.01 < .  1:1(0) ack 28001 win 65535 <sack 29001:30001, nop, nop>
+0.01 < .  1:1(0) ack 30001 win 65535

+1.00 close(4) = 0

`sysctl net.inet.tcp.rfc6675_pipe=0`

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 32880
Build 30283: arc lint + arc unit

Event Timeline

rscheff created this revision.Dec 20 2018, 7:44 PM
Herald added a reviewer: transport. · View Herald TranscriptDec 20 2018, 7:44 PM
Harbormaster completed remote builds in B21664: Diff 52202.Dec 20 2018, 7:44 PM
rscheff updated this revision to Diff 52204.Dec 20 2018, 7:54 PM
  • whitespace and comment changes
Harbormaster completed remote builds in B21666: Diff 52204.Dec 20 2018, 7:54 PM
rscheff updated this revision to Diff 52245.Dec 22 2018, 8:11 PM
  • lots of debugging code.
Harbormaster completed remote builds in B21682: Diff 52245.Dec 22 2018, 8:11 PM
rscheff updated this revision to Diff 52246.Dec 22 2018, 8:25 PM
  • fixed accounting error with partial or cumulative acks
Harbormaster completed remote builds in B21683: Diff 52246.Dec 22 2018, 8:25 PM
rscheff updated this revision to Diff 52247.Dec 22 2018, 8:46 PM
  • Another accounting oversight. RFC6675 allows a partial ack with SACK info to immediately enter fast recovery, where this could result in inappropriate tracking of sacked_bytes.
Harbormaster completed remote builds in B21684: Diff 52247.Dec 22 2018, 8:46 PM
rscheff added a reviewer: tuexen.Jan 10 2019, 1:26 PM
rscheff mentioned this in D18892: Phase 2 to add Proportional Rate Reduction (RFC6937) to FreeBSD.Jan 18 2019, 4:45 PM
rscheff mentioned this in D18985: New SACK (RFC6675).Jan 26 2019, 9:29 PM
rscheff added a comment.Jan 27 2019, 12:32 AM

A packetdrill script to verfiy the proper operation of the 6675 pipe calculation.

Scenario: Client can only provide incomplete SACK information (1 block only), and expiriences 2 drops in one window - first one starts loss recovery, and the 2nd drop is not consecutive with the first. Without the proper accounting, this will delay the opening of the cwnd for the retransmissions, or subsequent new transmissions. .Worst case (as demonstrated in this script) is the 2nd drop happening 180° out of phase, in the middle of the window. This will only allow the original retransmission through, but not the 2nd retransmission or any additional data as cwnd won't be opened up.

With this patch, cwnd will grow, as expected, during the 2nd half of the window, considering the two dropped segments. (and with PRR, retransmissions will be "dithered" over the entire window, as necessary.

rfc6675-accounting-test.pkt5 KBDownload

rscheff updated this revision to Diff 55582.EditedMar 29 2019, 9:01 AM
  • fixing curly bracket if branch, semantically moving check for new sack data to tcp_input
Harbormaster completed remote builds in B23372: Diff 55582.Mar 29 2019, 9:01 AM
Harbormaster completed remote builds in B23372: Diff 55582.
rscheff updated this revision to Diff 55586.Mar 29 2019, 9:42 AM
  • fixing curly bracket if branch
  • semantically moving check for new sack data to tcp_input
  • fixed wrong deref to TO struct
Harbormaster completed remote builds in B23373: Diff 55586.Mar 29 2019, 9:42 AM
rscheff added inline comments.Mar 29 2019, 9:45 AM
sys/netinet/tcp_sack.c
658

brackets in the if clause missing, sack_changed needs to be set too.

756

Move this to tcp_input:2670, as sack_changed should semantically caputer any changes to the scoreboard. Which does happen on partial / cumulative acks.

jtl added a comment.Jul 29 2019, 7:16 PM

This looks like it does what is described. As I understand, the use of delivered_data will be covered by a separate review. It looks like this will slightly change the way sacked_bytes is calculated. The change is probably a good thing, but it is worth verifying (and I haven't done this yet) that the updated calculation will work correctly.

rscheff added reviewers: thj, rgrimes, cc.Dec 4 2019, 8:06 PM
jtl added a comment.Feb 27 2020, 2:09 PM
In D18624#458099, @jtl wrote:

This looks like it does what is described. As I understand, the use of delivered_data will be covered by a separate review. It looks like this will slightly change the way sacked_bytes is calculated. The change is probably a good thing, but it is worth verifying (and I haven't done this yet) that the updated calculation will work correctly.

@rrs & @lstewart : This has been in review for 6+ months. Can one of you please verify that correcting the way sacked_bytes is calculated will do no harm to CC calculations?

Thanks!

Herald added a subscriber: melifaro. · View Herald TranscriptFeb 27 2020, 2:09 PM
rscheff updated this revision to Diff 73020.Jun 12 2020, 1:16 PM
  • Merge branch 'master'
  • whitespace fix in comment
Harbormaster completed remote builds in B31663: Diff 73020.Jun 12 2020, 1:16 PM
rgrimes accepted this revision.Jun 18 2020, 5:46 PM
rscheff added inline comments.Jun 18 2020, 6:07 PM
sys/netinet/tcp_input.c
2684

Add comment why this additional check for presence of a SACK block has been added (semantic of variable sack_changed now tracks scoreboard changes due to partial ACKs (without SACK blocks) too, but these should be excluded from increasing the dupthesh counter.

rscheff added a reviewer: jason_eggnet.com.Jun 23 2020, 9:01 AM
rscheff added a comment.Jun 23 2020, 9:07 AM

This improvement of the SACK scoreboard accounting is actually what Jason seems to have asked for in D3971 when the RFC6675 pipe calculation was added.

As mentioned verbally, this improvement will address slight differences of the pipe value which can occur due to incomplete tracking of the received segements due to only looking on the currently received SACK blocks in the ACK. Once more than 3 holes are present in a window, the previous RFC6675 pipe calculation would end up with a too large pipe estimate, not sending SACK retransmissions as early as allowed and suffering a higher probability of having to resort to RTO as a last resort for recovering multiple losses.

rscheff updated this revision to Diff 73502.Jun 23 2020, 9:40 AM
  • add comment to explain the semantic change to sack_changed
Harbormaster completed remote builds in B31890: Diff 73502.Jun 23 2020, 9:40 AM
rscheff edited the test plan for this revision. (Show Details)Jun 23 2020, 9:40 AM
rscheff edited the test plan for this revision. (Show Details)Jun 23 2020, 10:24 AM
rgrimes accepted this revision.Jun 23 2020, 2:12 PM

This has been in process for 15 months and should be evolving in tree rather than in a review at this point, IMHO. If no one objects to it being committed in 7 days I am asking that Richard go ahead and commit it.

rscheff added a comment.Aug 8 2020, 7:43 PM

It's been 6 weeks now (5 weeks after the suggested "speak up now" deadline), still require a transport sign-off, before committing...

rscheff updated this revision to Diff 75615.Aug 8 2020, 8:23 PM
rscheff edited the test plan for this revision. (Show Details)
  • Merge branch 'master'
Harbormaster completed remote builds in B32880: Diff 75615.Aug 8 2020, 8:23 PM
kbowling accepted this revision.Aug 10 2020, 8:52 PM
This revision is now accepted and ready to land.Aug 10 2020, 8:52 PM
rgrimes accepted this revision.Aug 13 2020, 2:05 PM
Closed by commit rS364195: Improve SACK support code for RFC6675 and PRR (authored by rscheff). · Explain WhyAug 13 2020, 4:30 PM
This revision was automatically updated to reflect the committed changes.
rscheff added a commit: rS364195: Improve SACK support code for RFC6675 and PRR.
Herald added a subscriber: imp. · View Herald TranscriptAug 13 2020, 4:30 PM
rscheff added a commit: rS365292: MFC r364195: Improve SACK support code for RFC6675 and PRR.Sep 3 2020, 8:38 AM
rscheff mentioned this in D28819: PRR: Avoid accounting left-edge twice in partial ACK..Feb 20 2021, 6:39 PM

Revision Contents
Changeset List

PathSize
sys/
netinet/
11 lines
31 lines
8 lines

Diff 75615

View Options

sys/netinet/tcp_input.c

Loading...
View Options

sys/netinet/tcp_sack.c

Loading...
View Options

sys/netinet/tcp_var.h

Loading...