Re: How should TCP LRO handle TH_PUSH?

The current behaviour is that if a flag other than PUSH/ACK is set,
the current aggregated frame is sent up the stack and it starts
working on a new one that begins with the packet with the unusual
flags.  However my reading of the code is that a subsequent packet
with only PUSH or ACK set could be merged into that one.

On Wed, Jan 19, 2022 at 2:33 AM Scheffenegger, Richard
<Richard.Scheffenegger@netapp.com> wrote:
>
> As discussed in the last transport call, can you please share the behavior with other TCP flags as well?
>
> URG is mostly historic
>
> ECE is tricky - presumably the flag of the last (in-sequence) packet should take precedence without AccECN. With AccECN, the new codepath where LRO provides the full sequence of all received header bits is preferred.
> CWR should be latched - if any packet in the LRO has it, it should be kept
> PSH should (conceptually) be latched, even though the FBSD stack doesn't do anything with it - other than possibly triggering an immediate ACK (good to reduce the dependency on the delayed ACK timer).
>
> What is the behavior for the other flags (FIN, RST, SYN)?
>
> Best regards,
>    Richard
>
>
> -----Original Message-----
> From: owner-freebsd-transport@freebsd.org <owner-freebsd-transport@freebsd.org> On Behalf Of Ryan Stone
> Sent: Freitag, 7. Jänner 2022 00:10
> To: <freebsd-transport@freebsd.org> <freebsd-transport@freebsd.org>
> Subject: How should TCP LRO handle TH_PUSH?
>
> NetApp Security WARNING: This is an external email. Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
>
>
>
> I've been working on writing unit tests for LRO (see my message to freebsd-testing@ for more details on this).  I've submitted reviews for two issues found by my tests that I believe to be outright bugs.
> I did find one more issue where I'm not sure whether it's really a bug or not.  If LRO sees a TCP packet that does not have TH_PUSH set, and then merges a subsequent packet that does have TH_PUSH set into it, what should the value of the TH_PUSH flag in the merged packet be?
>
> When I wrote my test I expected to see TH_PUSH set, but that isn't our current behaviour.  On the one hand I'm not sure that this is strictly correct, but on the other hand I don't think we do anything with TH_PUSH on a received packet anyway.  I did code up a proposed fix for this, but I wanted to get feedback as to whether it's worth worrying about before sending the review.  Does anybody have any opinions?
>
> Thanks,
> Ryan
>