Page MenuHomeFreeBSD

Preserve ECN bits when merging frames in LRO
Needs RevisionPublic

Authored by rstone on Sep 8 2016, 2:16 AM.

Details

Reviewers
hiren
rscheff
Group Reviewers
transport
Summary

Currently LRO uses the L3 header from the first packet in the
flow basically verbatim. This is incorrect when ECN is in use.
If any packet in the flow has experienced congrestion and the
network has signalled this by setting the CE bits in the L3 header
it needs to be signalled in the L3 header that we send up the
stack.

Test Plan

Wrote a C wrapper around ng_source that injects TCP packets for a single flow into a network. I set CE only on the second packet of the flow and confirmed via tcpdump that without the fix, CE was not set without the fix. I repeated the test with the fix and confirmed that CE was set as expected. I tested both IPv4 and v6.

Diff Detail

Event Timeline

rstone retitled this revision from to Preserve ECN bits when merging frames in LRO.
rstone edited the test plan for this revision. (Show Details)
rstone updated this object.
rstone edited edge metadata.
hiren edited edge metadata.

Talked to @rstone about adding some comments to make code more readable.

This revision is now accepted and ready to land.Oct 23 2016, 11:34 PM

Is this still alive? As ECN has become more interesting recently, something like this is probably needed for RFC3168 ECN flows.

Also, forgive my ignorance, but can the LRO code add some side-band information to the upstream - e.g. number of packets coalesced, or number of CE marks observed? This would be very relevant in context of Accurate ECN. Where for RFC3168 it is sufficience that any segment marked as CE to appear in the final header, for AccECN the actual number of the CE marks, and also the number of coalesced packets would be relevant, to arrive at an appropriate alpha (reduction) value.

By just moving any number of CE marks to a single, the AccECN CC reaction would be more strong, shedding more bandwidth than appropriate...

rscheff requested changes to this revision.Jan 19 2020, 7:45 PM

Also, I would suggest to make the IPv6 TOS shift/mask a seperate Diff, and apply that to all the places where iptos is needed in the TCP stacks (plural) currently.

This revision now requires changes to proceed.Jan 19 2020, 7:45 PM
In D7821#510126, @rscheff_gmx.at wrote:

Is this still alive? As ECN has become more interesting recently, something like this is probably needed for RFC3168 ECN flows.

Also, forgive my ignorance, but can the LRO code add some side-band information to the upstream - e.g. number of packets coalesced, or number of CE marks observed? This would be very relevant in context of Accurate ECN. Where for RFC3168 it is sufficience that any segment marked as CE to appear in the final header, for AccECN the actual number of the CE marks, and also the number of coalesced packets would be relevant, to arrive at an appropriate alpha (reduction) value.

By just moving any number of CE marks to a single, the AccECN CC reaction would be more strong, shedding more bandwidth than appropriate...

Unfortunately there's nowhere in the mbuf where we can include that kind of metadata. The best way to try and handle is probably a GRO-style approach like Linux uses, in which packets from the same flow pass through the stack as a group but the packets themselves are left untouched.

Anyway, I completely forgot about this review. I'll try to find the time to dig out the original patch and clean it up again. It's definitely still relevant.