Details

Reviewers

tuexen
• hselasky
gallatin

Group Reviewers

transport

Commits

rG4e0ce82b53e2: TCP Lro has a loss of timestamp precision and reorders packets.

Summary

A while back Hans optimized the LRO code. This is great but one
optimization he did degrades the timestamp precision so that
all flushed LRO entries end up with the same LRO timestamp
if there is not a hardware timestamp. The intent of the LRO timestamp
is to get as close to the time that the packet arrived as possible. Without
the LRO queuing this works out fine since a binuptime is taken and then
the rx_common code is called. But when you go through the queue path
you end up *not* updating the M_LRO_TSTMP fields.

Another issue in the LRO code is several places that cause packet reordering. In
general TCP can handle reordering but it can cause extra un-needed retransmission
as well as other oddities. We will fix all of the reordering problems.

Lets fix this so that we restore the precision to the timestamp.

Test Plan

Use BB logs to make sure we no longer have the same
timestamp in all packets on one LRO flush like we do now.

I have been testing these changes on several machines using
the hw-timestamps and making sure that the order is correct (which
is how I found the reordered packets in the first place).

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

rrs created this revision.Aug 4 2022, 3:33 PM

Herald added 1 blocking reviewer(s): transport. · View Herald TranscriptAug 4 2022, 3:33 PM

Herald added subscribers: glebius, melifaro. · View Herald Transcript

rrs requested review of this revision.Aug 4 2022, 3:33 PM

rrs added a reviewer: gallatin.Aug 4 2022, 5:24 PM

Drew suggested we use the mbufqueue flag as an indication that
we have stacks that are interested in the M_TSTMP_LRO so we only
do it if thats true.

gallatin accepted this revision.Aug 4 2022, 6:30 PM

Just found an interesting bug when SACKs are mixing in with compressed acks. We can
push the acks in the wrong order by remembering the last compressed ack we were filling.
We should NULL the cmp field when it turns out we have to add a packet that is
not-compressable.

gallatin accepted this revision.Aug 4 2022, 11:14 PM

• hselasky added inline comments.Aug 5 2022, 2:07 AM

sys/netinet/tcp_lro.c
1958	Should this code block be after the IFCAP_LRO check or not?
1960	Fix comment: check if device is not LRO capable

After a bit of work and digging I have found on top of the timestamp
issue where LRO is reordering packets. Some in response to the
compressed ack and mbuf queuing. Others in just the normal
old flush paths. In any event this is not desirable for TCP, we need
to see the packets arrive in order. The most horrible one (for TCP)
is where the peer sends a RST followed quickly by a new SYN. This
will get reordered into SYN first then RST.

Which means it will take a SYN retransmission to actually establish
the new connection.

• hselasky added inline comments.Aug 22 2022, 12:36 PM

sys/netinet/tcp_lro.c
118	The indention for these two lines is not the same.
662	Testing the same thing twice?? Expand current "if" instead?
1191	This looks like a separate change?
1937	__predict_false() ??
1960	Ping

Fix a nit from review of Peter Lei

rrs marked an inline comment as done.Aug 22 2022, 3:39 PM

rrs added inline comments.

sys/netinet/tcp_lro.c
1191	It was needed for testing in order to query back the ifp when a compressed ack arrives. The field should have been filled in all along but was missed somehow when we added the compressed acks. The testing gig I have has a way to pull from the card the clocking sync information to dump in BB logs and of course with an ifp == NULL you crash :)