Details

Reviewers

gallatin
mmacy
shurd

Commits

rS334779: iflib: Record TCP checksum info in iflib when TCP checksum is requested

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.

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 16853
Build 16732: arc lint + arc unit

Event Timeline

erj created this revision.May 24 2018, 6:16 PM

Herald added subscribers: ae, imp. · View Herald TranscriptMay 24 2018, 6:16 PM

Harbormaster completed remote builds in B16821: Diff 42946.May 24 2018, 6:16 PM

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

erj added 1 blocking reviewer(s): shurd.May 24 2018, 6:20 PM

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..

erj edited the summary of this revision. (Show Details)May 24 2018, 6:30 PM

shurd added inline comments.May 24 2018, 7:13 PM

FreeBSD/sys/net/iflib.c
2769 ↗	(On Diff #42946)	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 ↗	(On Diff #42946)	This can be moved inside the previous block so we're not relying on compiler magic to DTRT (same for below).
2924 ↗	(On Diff #42946)	This looks like an accident... this bit is already set or we wouldn't be here.

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 :)

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

Harbormaster completed remote builds in B16830: Diff 42961.May 24 2018, 9:27 PM

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

sys/net/iflib.c
2915	Non-TCP TSO won't need the tcp header and sequence number information though. The IS_TSO4() block below fills in the generic information.
2981–2982	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 added inline comments.May 25 2018, 7:52 PM

sys/net/iflib.c
2915	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?
2981–2982	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.

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 requested changes to this revision.May 25 2018, 8:27 PM

gallatin added inline comments.

sys/net/iflib.c
2915	FreeBSD does not support anything but TCP TSO. So checking for IPPROTO_TCP is redundant, and can be skipped.
2981–2982	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.

This revision now requires changes to proceed.May 25 2018, 8:27 PM

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

Harbormaster completed remote builds in B16853: Diff 43022.May 25 2018, 10:52 PM

erj marked an inline comment as done.May 25 2018, 11:17 PM

erj added inline comments.

sys/net/iflib.c
2915	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.