Add TX checksum offload for Windows XDP #5080
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5080 +/- ##
==========================================
+ Coverage 86.47% 86.94% +0.47%
==========================================
Files 59 59
Lines 18035 18035
==========================================
+ Hits 15595 15680 +85
+ Misses 2440 2355 -85 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
For clarity, the TX offloading capabilities were already there (set IP->headerCheckSum = 0) in Would a better title for this PR be: "Set correct socket options for TX checksum offloads"? |
Until now, the common |
|
It looks like there are 2 sources of truth now for offload enablement.
1 is only used in QTIP code path. Is this expected? |
|
very nit: "Describe the purpose of and changes within this Pull Request." we probably don't need the placeholder in the PR describption. |
Thanks, I meant to update QTIP too, but got forgot in the chaos. Great catch. |
This reverts commit 4ee654a.
|
Converting back to draft status while experimentation continues. |
This reverts commit fe32431.
| OptionLength < XDP_SIZEOF_CHECKSUM_CONFIGURATION_REVISION_1 || | ||
| ChecksumConfig.Header.Revision != XDP_CHECKSUM_CONFIGURATION_REVISION_1 || | ||
| ChecksumConfig.Header.Size < XDP_SIZEOF_CHECKSUM_CONFIGURATION_REVISION_1) { |
There was a problem hiding this comment.
So, if XDP starts returning a version 2, it breaks our support for the offload? If so, that doesn't seem like a great design.
There was a problem hiding this comment.
Version 2 is by definition a breaking change, and version 2 has not yet been defined. The only legal action the application can take here is to ignore the payload.
There was a problem hiding this comment.
I agree that if we make it version 2, it's breaking, but should we instead of a different socket option to query version 2?
There was a problem hiding this comment.
I don't have a strong opinion, other than that XDP already consistently uses this pattern everywhere, and I don't see much justification to deviate here.
| "XskSetSockopt(XSK_SOCKOPT_TX_OFFLOAD_CHECKSUM)"); | ||
| } | ||
| } else { | ||
| QuicTraceEvent( |
There was a problem hiding this comment.
This is a duplicate log in the QUIC_FAILED case above.
There was a problem hiding this comment.
I'm not sure - GetTxOffloadConfig can return QUIC_SUCCEEDED and TxChecksumOffload can be false.
|
There's a code defect (probably introduced by myself) that's not part of your PR. If we don't rely on these flags for xdp anymore, maybe delete these lines? In datapath_raw_xdp_win.c: The second line should be setting the transport layer xsum. |
Description
Describe the purpose of and changes within this Pull Request.
This PR adds support for TX checksum offload for Windows XDP. The offload may not be supported for at least the following reasons:
Note the version of the XDP runtime in MsQuic is not being updated to a prerelease build with checksum support, so this code will not be practically enabled yet in CI/netperf/etc.
Testing
Do any existing tests cover this change? Are new tests needed?
We rely on the XDP interop tests for correctness, though this is not robust if the VMs running the interop test do not advertise checksum support. I've also manually run interop tests between XDP client and the full stack IOCP server in netperf, which we should eventually onboard both for performance and conformance coverage.
Documentation
Is there any documentation impact for this change?
Mostly self-explanatory implementation details.