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

Event Timeline

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 edited the summary of this revision. (Show Details)

Merge r344428.

This revision is now accepted and ready to land.Feb 21 2019, 12:21 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.

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.