Patch to MFC TCP reassembly rewrite to stable/11
ClosedPublic
Actions

Authored by tuexen on Jan 25 2019, 5:41 PM.

Details

Reviewers

rrs
bz
jtl

Group Reviewers

transport

Commits

rS344511: Backport the new TCP reassembly code from head to stable/11.

Summary

This patch does:

Cherry pick the changes for sys/queue.h from r334804.
Merge the changes from r338102 with manually removing the changes to files not existent in stable/11, which are sys/netinet/tcp_stacks/rack.c and sys/netinet/tcp_log_buf.h, and manually resolving the conflicts in sys/netinet/tcp_var.h.
Manually change sys/netinet/tcp_stacks/fastopen.c to make it compile with the changes in sys/netinet/tcp_var.h.
Merge the changes from r342280.
Merge the changes from r343439.
Merge the changes from r344428.

It should be noted, that sys/netinet/tcp_reass.c in stable/11 is the same as in head such that future changes can still be MFCed.

Test Plan

Run the packetdrill tests from rcv-data-segments
Run git clone https://github.com/freebsd/freebsd.git

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 22148

Event Timeline

tuexen created this revision.Jan 25 2019, 5:41 PM

Herald added a reviewer: transport. · View Herald TranscriptJan 25 2019, 5:41 PM

Herald added a subscriber: imp. · View Herald Transcript

Harbormaster completed remote builds in B22148: Diff 53204.Jan 25 2019, 5:41 PM

Ok, your prepared change looks good from a merge-to-stable11 perspective.

I haven't paid much detail to function changes but I found some problem with this code in HEAD. Can I give you another MFC on top of this in a bit? I'll add you to the review for HEAD in a few minutes when I made the changes.

Also can you please improve the commit message describing "Why?" these changes are done beyond the "Subject line"? Like a few sentences from one of the major commits to HEAD?

Given the discussions in D18968 I think you'd be safe as I'd assume we don't otherwise have TCP_REASS_LOGGING in 11, do we?
I guess rrs may or may not give you a more technical feedback.

In D18960#405325, @bz wrote:

Given the discussions in D18968 I think you'd be safe as I'd assume we don't otherwise have TCP_REASS_LOGGING in 11, do we?
I guess rrs may or may not give you a more technical feedback.

We can't use TCP_REASS_LOGGING in stable/11, since it requires black box logging. I was considering to remove all that code, but
didn't do that since that would make future MFCs to tcp_reass.c more difficult. I actually committed a change to head to get the
ifdefs right to also control the include files.

tuexen added a reviewer: jtl.Jan 26 2019, 10:07 AM

Merge r344428.

rrs accepted this revision.Feb 21 2019, 12:21 PM

This revision is now accepted and ready to land.Feb 21 2019, 12:21 PM

jtl accepted this revision.Feb 21 2019, 7:09 PM

Quick comment: DSACKs are not working in HEAD. Potentially, the tlenp is set to zero for the duplicate segment, so that tcp_update_sack_list is not called to build the DSACK entry to the next ACK. If this is the reason, this patch may break DSACK in BSD11

This is not to be a shipstopper, but a heads-up as the DSACK not working may be related hereto...

In D18960#413211, @rscheff_gmx.at wrote:

Quick comment: DSACKs are not working in HEAD. Potentially, the tlenp is set to zero for the duplicate segment, so that tcp_update_sack_list is not called to build the DSACK entry to the next ACK. If this is the reason, this patch may break DSACK in BSD11

This is not to be a shipstopper, but a heads-up as the DSACK not working may be related hereto...

Can you open a PR for this against head? Is it also broken in stable/12?

I'll look into this before MFCing this to stable/11.

Closed by commit rS344511: Backport the new TCP reassembly code from head to stable/11. (authored by tuexen). · Explain WhyFeb 25 2019, 10:38 AM

This revision was automatically updated to reflect the committed changes.

In D18960#413211, @rscheff_gmx.at wrote:

Quick comment: DSACKs are not working in HEAD. Potentially, the tlenp is set to zero for the duplicate segment, so that tcp_update_sack_list is not called to build the DSACK entry to the next ACK. If this is the reason, this patch may break DSACK in BSD11

This is not to be a shipstopper, but a heads-up as the DSACK not working may be related hereto...

I double checked that the change in this review is not the reason for DSACK not being working as you expect in head. Therefore, I have committed this change to stable/11. Any change/fix related to DSACK needs to be MFCed to stable/11 once it is in head.