Page MenuHomeFreeBSD

Several LRO fixes
ClosedPublic

Authored by kbowling on Feb 28 2018, 7:48 AM.

Details

Summary

This changeset contains a few tightly intermingled and additive bugfixes from Netflix and LLNW:

  1. rrs - Lets make the LRO code look for true dup-acks and window update acks fly on through and combine.
  2. rrs - Make the LRO engine a bit more aware of ack-only seq space. Lets not have it incorrectly wipe out newer acks for older acks when we have out-of-order acks (common in wifi environments).
  3. jeggleston - LRO eating window updates

Based on all of the above I think we are RFC compliant doing it this way:

https://tools.ietf.org/html/rfc1122

section 4.2.2.16

"Note that TCP has a heuristic to select the latest window update despite possible datagram reordering; as a result, it may ignore a window update with a smaller window than previously offered if neither the sequence number nor the acknowledgment number is increased."

Test Plan

These changes are in use at LLNW and Netflix

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

kbowling created this revision.Feb 28 2018, 7:48 AM
rstone accepted this revision as: rstone.Mar 7 2018, 10:32 PM
rstone added a subscriber: rstone.

I've written some unit tests that covering these cases here:

https://github.com/rysto32/sysunit/blob/master/netinet/tcp_lro.gtest.cpp#L850

I can confirm that the new behaviour is much more sane than the older behaviour.

sys/netinet/tcp_lro.c
826 ↗(On Diff #39819)

I think that you mean "old ack" here. As far as I can tell, the only way to reach this point is to have a packet with no payload and th_ack < le->ack_seq.

sys/netinet/tcp_lro.c
826 ↗(On Diff #39819)

Agreed.

kbowling updated this revision to Diff 40065.Mar 8 2018, 7:56 AM

Fix comment per rstone and jegg

rstone accepted this revision as: rstone.Mar 8 2018, 2:34 PM
rstone added inline comments.Mar 8 2018, 4:28 PM
sys/netinet/tcp_lro.c
819 ↗(On Diff #40065)

Technically we should only do this (or look at th->th_ack at all) if TH_ACK is set

sys/netinet/tcp_lro.c
819 ↗(On Diff #40065)

On the transport call we discussed that this should probably be handled in another patch as it is a generic issue with the LRO code.

sbruno accepted this revision.Mar 8 2018, 6:25 PM
sbruno added a subscriber: sbruno.

I'll grab this and shovel it in after builds are done.

gallatin accepted this revision.Mar 8 2018, 7:11 PM
This revision was not accepted when it landed; it landed in state Needs Review.Mar 9 2018, 12:08 AM
This revision was automatically updated to reflect the committed changes.