Details

Reviewers

tuexen
rrs

Group Reviewers

transport

Commits

rS351725: This patch improves the DSACK handling to conform with RFC 2883.

Summary

The initial Diff D19334 was rather crude. This patch improves the DSACK handling to conform with the RFC.
The lowest SACK block is used when multiple Blocks would be elegible as DSACK blocks
SACK blocks get reordered - while maintaining the ordering of SACK blocks not relevant in the DSACK context is maintained.

Test Plan

Packedrill has 95 tests around SACK / DSACK in rcv-data-segments/overlapping.

Additional testis with randomly ordered initial SACK blocks and up to 4 SACK blockx (maximum without Timestamps) will be provided

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

rscheff created this revision.Jul 23 2019, 5:45 PM

Herald added 1 blocking reviewer(s): transport. · View Herald TranscriptJul 23 2019, 5:45 PM

Herald added a subscriber: imp. · View Herald Transcript

The patch doesn't apply, since the indentation is based on spaces instead of tabs. Please fix.

tuexen added inline comments.Jul 23 2019, 9:40 PM

sys/netinet/tcp_var.h
941 ↗	(On Diff #60054)	It needs a closing parentheses at the end for the semicolon.

fix whitespace mixup and missing bracket

Harbormaster completed remote builds in B25475: Diff 60065.Jul 23 2019, 9:45 PM

Hopefully the tab/space mixup is fixed now too.

update rack stack with dsack (again)

Harbormaster completed remote builds in B25479: Diff 60073.Jul 24 2019, 2:23 AM

Added DSACK to RACK again. Validated by running packet drill DSACK testsuite against the rack stack:

[root@freebsd ~]# kldload tcp_rack
[root@freebsd ~]# sysctl net.inet.tcp.functions_available
net.inet.tcp.functions_available:
Stack D Alias PCB count
freebsd * freebsd 8
rack rack 0

[root@freebsd ~]# sysctl net.inet.tcp.functions_default=rack
net.inet.tcp.functions_default: freebsd -> rack
[root@freebsd ~]# sysctl net.inet.tcp.functions_available
net.inet.tcp.functions_available:
Stack D Alias PCB count
freebsd freebsd 8
rack * rack 0

[root@freebsd ~/tcp-testsuite/rcv-data-segments]# for i in ls overlapping-???-???.pkt

do
echo $i
packetdrill $i
done

overlapping-050-075.pkt
single word S S
net.inet.tcp.sack.enable: 1 -> 1
net.inet.tcp.hostcache.purgenow: 0 -> 0
overlapping-050-100.pkt

All 95 tests completed with no functional issues (3 timing errors in the first run, none in the 2nd).

Can not comment on TOE / HPTS interaction mentioned earlier - lack the infrastructure to validate that.

fix indentation

Harbormaster completed remote builds in B25490: Diff 60087.Jul 24 2019, 1:10 PM

This tgz file contains packetdrill tests for dsack, with the non-duplicate packets getting delivered in various orderings, before the overlapping DSACK is sent.

This is to validate that SACK block order remains maintained, while overlapping DSACK/SACK segments get shuffled first in the option block.

dsack-overlap-reorder-checks.tgz104 KBDownload

sys/netinet/tcp_sack.c
281 ↗	(On Diff #60073)	Indentation

Please validate the rack_bbr_common.c has (or does not have) the extra bit you added in the early part.

sys/netinet/tcp_stacks/rack.c
1790 ↗	(On Diff #60087)	This block may be a duplicate of whats in rack_bbr_common.c Please look in that file to verify. R

This revision now requires changes to proceed.Jul 29 2019, 6:13 PM

rscheff added inline comments.Jul 29 2019, 6:22 PM

sys/netinet/tcp_stacks/rack.c
1790 ↗	(On Diff #60087)	There is no rack_bbr_common.c in the upstream (yet). Only the header file, which doesn't contain any code. Shall I add a comment that this needs to be checked once bbr is merged to upstream?