Page MenuHomeFreeBSD

Patch to MFC TCP reassembly rewrite to stable/11
ClosedPublic

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

Details

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

Event Timeline

tuexen created this revision.Jan 25 2019, 5:41 PM
bz added a comment.Jan 25 2019, 6:47 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?

bz accepted this revision as: bz.Jan 25 2019, 9:18 PM

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
tuexen edited the summary of this revision. (Show Details)EditedFeb 21 2019, 10:17 AM
tuexen updated this revision to Diff 54177.

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

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.

This revision was automatically updated to reflect the committed changes.

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.