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

Event Timeline

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

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

Hopefully the tab/space mixup is fixed now too.

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.

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
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?

sys/netinet/tcp_stacks/rack.c
1790 ↗(On Diff #60087)

I think is it in head: rack_bbr_common.c.

  • add dsack drop block in rack_bbr_common
  • lint - breaking some lines longer than 80 char

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

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.