Page MenuHomeFreeBSD

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

Authored by erj on May 24 2018, 6:16 PM.

Details

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 OK
Unit
No Unit Test Coverage
Build Status
Buildable 16853
Build 16732: arc lint + arc unit

Event Timeline

erj created this revision.May 24 2018, 6:16 PM
sbruno added a subscriber: sbruno.May 24 2018, 6:17 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

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.

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 updated this revision to Diff 42961.May 24 2018, 9:27 PM
erj edited the summary of this revision. (Show Details)
  • iflib: Fixup changes made to record TCP checksum info in iflib for ixl
shurd added a comment.May 25 2018, 7:33 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.

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
erj updated this revision to Diff 43022.May 25 2018, 10:52 PM
  • iflib: Remove comment and restore/fix setting CSUM_IP6_TCP flag during TSO6
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.

erj updated this revision to Diff 43024.May 25 2018, 11:18 PM
  • iflib: Move stats increment in IPv6 case to match IPv4 case
erj added a comment.Jun 4 2018, 5:55 PM

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

gallatin accepted this revision.Jun 4 2018, 8:13 PM

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

This revision is now accepted and ready to land.Jun 4 2018, 8:13 PM
This revision was automatically updated to reflect the committed changes.