iflib: Record TCP checksum info in iflib for ixl(4)

[erj]

Summary

This is a combination of two patches from ixl-iflib in the intel-wired-ethernet GitHub repository.

3da3c7ccaaf0 iflib: Save TCP header length during packet parsing when CSUM_IP6_TCP is set
9adced371d84 iflib: Save TCP header length when TCP checksum offload is requested.

[sbruno]

Was this the patch that we discussed on our call? Does this fix your TX checksum problems on IXL?

[gallatin]

In D15558#328453, @sbruno wrote:

Was this the patch that we discussed on our call? Does this fix your TX checksum problems on IXL?

Yes and Yes.

Unfortunately, I've now proceeded to the "entire box hangs" portion of the game. Even serial console break-to-debugger is ignored. I'm going to try NMIs next..

[shurd]

FreeBSD/sys/net/iflib.c

2769

It looks like these macros are always tested with IS_TSO4()... a combined macro would be better.

IS_TX_OFFLOAD4() or something checking CSUM_IP_TSO and CSUM_IP_TCP together.

2872

This can be moved inside the previous block so we're not relying on compiler magic to DTRT (same for below).

2924

This looks like an accident... this bit is already set or we wouldn't be here.

[jeffrey.e.pieper_intel.com]

In D15558#328462, @gallatin wrote:

In D15558#328453, @sbruno wrote:

Was this the patch that we discussed on our call? Does this fix your TX checksum problems on IXL?

Yes and Yes.

Unfortunately, I've now proceeded to the "entire box hangs" portion of the game. Even serial console break-to-debugger is ignored. I'm going to try NMIs next..

At least you see what I'm seeing now :)

[erj]

• iflib: Fixup changes made to record TCP checksum info in iflib for ixl

[shurd]

Looks good, but not forcing CSUM_TCP_IPV6 for TSO6 may need a closer look.

sys/net/iflib.c

2915 ↗

(On Diff #42961)

Non-TCP TSO won't need the tcp header and sequence number information though. The IS_TSO4() block below fills in the generic information.

2976 ↗

(On Diff #42961)

Did you confirm that the CSUM_TCP_IPV6 csum flag *is* set on -current? Not forcing CSUM_TCP_IPV6 seems like a significant change here. I'll take a look at what "the rest of the flow requires it" means in this context.

[erj]

sys/net/iflib.c

2915 ↗

(On Diff #42961)

I think my comment there was really asking if iflib is supposed to support non-TCP TSO.

Does FreeBSD mean TSO to be "TCP segmentation offload" like in the ifconfig(8) man page, or "Transmit segmentation offload" like in the Intel datasheets?

2976 ↗

(On Diff #42961)

Err, looking at this again, I shouldn't have reverted it. I think yesterday I read the flag as being TSO instead of TCP and agreed that it was redundant.

[gallatin]

In D15558#328462, @gallatin wrote:

In D15558#328453, @sbruno wrote:

Was this the patch that we discussed on our call? Does this fix your TX checksum problems on IXL?

Yes and Yes.

Unfortunately, I've now proceeded to the "entire box hangs" portion of the game. Even serial console break-to-debugger is ignored. I'm going to try NMIs next..

The "entire box hangs" was due to a pre-release CPU/Mobo, and also hung with a chelsio nic. Sorry for the distraction.

[gallatin]

sys/net/iflib.c

2915 ↗

(On Diff #42961)

FreeBSD does not support anything but TCP TSO. So checking for IPPROTO_TCP is redundant, and can be skipped.

2976 ↗

(On Diff #42961)

And setting it is safe, because you've already nuked the checksum that would have been there if it had not been set.

Speaking of that.. It would be nice if FreeBSD handled TSO pseudo hdr checksums based on a length of 0 like windows, since that's what most ASICs want. At Myricom, we had a special mode in the NIC to handle linux/freebsd TSO where we subtracted out the len from the csum we were passed, but most NICs don't have that. I suppose a change like that might be too disruptive, but it would allow cleaning up a ton of places where drivers call in*_cksum_pseudo() to get the checksum in the form they want.

[erj]

• iflib: Remove comment and restore/fix setting CSUM_IP6_TCP flag during TSO6

[erj]

sys/net/iflib.c

2915 ↗

(On Diff #42961)

I don't want to remove it in this patch because I think it's outside the scope I wanted for it. Maybe the entire function needs a revisit / documenting.

[erj]

• iflib: Move stats increment in IPv6 case to match IPv4 case

[shurd]

[erj]

@gallatin I'm assuming you no longer have any objections to this patch?

[gallatin]

Sure.. Those checks for TCP in the TSO case should probably turn into asserts eventually..