Page MenuHomeFreeBSD

DSACK patch to conform fully with RFC2883
ClosedPublic

Authored by rscheff on Jul 23 2019, 5:45 PM.

Details

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

rscheff created this revision.Jul 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.

rscheff updated this revision to Diff 60065.Jul 23 2019, 9:45 PM
  • fix whitespace mixup and missing bracket
rscheff marked an inline comment as done.Jul 23 2019, 9:46 PM

Hopefully the tab/space mixup is fixed now too.

rscheff updated this revision to Diff 60073.Jul 24 2019, 2:23 AM

update rack stack with dsack (again)

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.

rscheff updated this revision to Diff 60087.Jul 24 2019, 1:10 PM
  • fix indentation

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.

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

Indentation

rrs requested changes to this revision.Jul 29 2019, 6:13 PM

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?

tuexen added inline comments.Aug 1 2019, 2:01 PM
sys/netinet/tcp_stacks/rack.c
1790 ↗(On Diff #60087)

I think is it in head: rack_bbr_common.c.

rscheff updated this revision to Diff 60429.Aug 3 2019, 8:10 PM
  • add dsack drop block in rack_bbr_common
  • lint - breaking some lines longer than 80 char
rscheff marked 2 inline comments as done.Aug 3 2019, 8:12 PM

synced up my branch to add the dsack change on drop in rack_bbr_common, as pointed out. Thx!

tuexen accepted this revision.Sep 2 2019, 6:44 PM
This revision was not accepted when it landed; it landed in state Needs Review.Sep 2 2019, 7:04 PM
This revision was automatically updated to reflect the committed changes.