Rename datapath callback types for transport generalization#6070
Rename datapath callback types for transport generalization#6070guhetier wants to merge 2 commits into
Conversation
37280b8 to
73f7864
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6070 +/- ##
==========================================
- Coverage 85.93% 85.66% -0.28%
==========================================
Files 60 60
Lines 18798 18798
==========================================
- Hits 16155 16103 -52
- Misses 2643 2695 +52 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
73f7864 to
2939c25
Compare
The binding trace events refer to the socket object generically, not specifically a UDP socket. Rename for clarity since bindings can now use different transport types (UDP, TCP, RDMA). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rename datapath callback type names for consistency: - CXPLAT_UDP_DATAPATH_CALLBACKS -> CXPLAT_DATAPATH_DGRAM_CALLBACKS - CXPLAT_TCP_DATAPATH_CALLBACKS -> CXPLAT_DATAPATH_CONN_CALLBACKS Variable/field names (UdpCallbacks, TcpCallbacks, UdpHandlers, TcpHandlers) are kept as-is since they are used in transport-specific contexts where the protocol is known. The type naming reflects the transport model (datagram vs connection- oriented) rather than specific protocols (UDP/TCP), since RDMA RC is connection-oriented but not a stream. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
2939c25 to
cb073b4
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates MsQuic’s internal datapath callback type names to be transport-generic (datagram vs. connection-oriented), aligning terminology with planned support for transports beyond UDP/TCP. It also updates binding trace strings to use “Socket” terminology and includes the corresponding CLOG/ETW artifacts.
Changes:
- Renamed
CXPLAT_{UDP,TCP}_DATAPATH_CALLBACKStoCXPLAT_DATAPATH_{DGRAM,CONN}_CALLBACKSand updated call sites across tools, tests, perf code, and platform implementations. - Updated binding trace strings from
Udp=...toSocket=...in code and ETW/CLOG metadata, including regenerated Linux CLOG headers andclog.sidecar. - Updated datapath initialization signatures and handler storage types to use the new callback struct names.
Reviewed changes
Copilot reviewed 20 out of 22 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tools/recvfuzz/recvfuzz.cpp | Switch recvfuzz datapath callback struct to CXPLAT_DATAPATH_DGRAM_CALLBACKS. |
| src/tools/lb/loadbalancer.cpp | Update load balancer tool to use CXPLAT_DATAPATH_DGRAM_CALLBACKS. |
| src/tools/attack/attack.cpp | Update attack tool datapath callbacks to new datagram callback type. |
| src/test/lib/QuicDrill.cpp | Update QuicDrill datapath callback struct to new datagram callback type. |
| src/platform/unittest/DataPathTest.cpp | Update unit tests to use new datagram/connection callback struct types and updated init wrapper signature. |
| src/platform/platform_internal.h | Update internal datapath common handler storage and DataPathInitialize prototype to new callback types. |
| src/platform/datapath_xplat.c | Update CxPlatDataPathInitialize signature to new callback types. |
| src/platform/datapath_winuser.c | Update DataPathInitialize signature to new callback types (winuser). |
| src/platform/datapath_winkernel.c | Update DataPathInitialize signature to new callback types (winkernel). |
| src/platform/datapath_kqueue.c | Update UDP handler storage and init signature to new callback types (kqueue). |
| src/platform/datapath_iouring.c | Update DataPathInitialize signature to new callback types (io_uring). |
| src/platform/datapath_epoll.c | Update DataPathInitialize signature to new callback types (epoll). |
| src/perf/lib/Tcp.h | Update perf TCP engine callback declaration to new connection callback type. |
| src/perf/lib/Tcp.cpp | Update perf TCP engine callback definition to new connection callback type. |
| src/perf/lib/SecNetPerfMain.cpp | Update secnetperf’s datapath callback struct to new datagram callback type. |
| src/manifest/MsQuicEtw.man | Update ETW string resources to use “Socket” instead of “Udp” for binding events. |
| src/manifest/clog.sidecar | Update CLOG trace strings/hashes for binding events to “Socket”. |
| src/inc/quic_datapath.h | Rename the callback struct typedefs and datapath init prototype to the new datagram/connection callback types. |
| src/generated/linux/binding.c.clog.h.lttng.h | Regenerated Linux LTTng CLOG header reflecting updated binding trace strings. |
| src/generated/linux/binding.c.clog.h | Regenerated Linux CLOG header reflecting updated binding trace strings. |
| src/core/library.c | Update library datapath initialization to use new datagram callback struct type. |
| src/core/binding.c | Update binding trace strings from “Udp” to “Socket” for created/rundown events. |
| // | ||
| // UDP Callback function pointers used by the datapath. | ||
| // | ||
| typedef struct CXPLAT_UDP_DATAPATH_CALLBACKS { | ||
| typedef struct CXPLAT_DATAPATH_DGRAM_CALLBACKS { |
| // | ||
| // TCP Callback function pointers used by the datapath. | ||
| // | ||
| typedef struct CXPLAT_TCP_DATAPATH_CALLBACKS { | ||
| typedef struct CXPLAT_DATAPATH_CONN_CALLBACKS { |
| // | ||
| // The UDP callback function pointers. | ||
| // | ||
| CXPLAT_UDP_DATAPATH_CALLBACKS UdpHandlers; | ||
| CXPLAT_DATAPATH_DGRAM_CALLBACKS UdpHandlers; |
| // | ||
| // The TCP callback function pointers. | ||
| // | ||
| CXPLAT_TCP_DATAPATH_CALLBACKS TcpHandlers; | ||
| CXPLAT_DATAPATH_CONN_CALLBACKS TcpHandlers; |
| // | ||
| // UDP handlers. | ||
| // | ||
| CXPLAT_UDP_DATAPATH_CALLBACKS UdpHandlers; | ||
| CXPLAT_DATAPATH_DGRAM_CALLBACKS UdpHandlers; |
There was a problem hiding this comment.
Generally agree it would be best to do a comprehensive refactor.
There was a problem hiding this comment.
I'll fix the comments, in most places.
In that one specifically, "UDP handlers" is accurate, because we are in a specific datapath that deals with UDP (while from the core layer point of view, it shouldn't matter)
| _In_opt_ const CXPLAT_UDP_DATAPATH_CALLBACKS* UdpCallbacks, | ||
| _In_opt_ const CXPLAT_TCP_DATAPATH_CALLBACKS* TcpCallbacks, | ||
| _In_opt_ const CXPLAT_DATAPATH_DGRAM_CALLBACKS* UdpCallbacks, | ||
| _In_opt_ const CXPLAT_DATAPATH_CONN_CALLBACKS* TcpCallbacks, |
There was a problem hiding this comment.
From a naming perspective, I'd follow the BSD socket types, since their abstraction is about as good as any:
DGRAM_CALLBACKS for a datapath built upon SOCK_DGRAM semantics
STREAM_CALLBACKS for a datapath built upon SOCK_STREAM semantics instead of CONN_CALLBACKS
This is imperfect, for sure, but should be most intuitive as long as our set of datapath models roughly align with BSD socket semantics.
There was a problem hiding this comment.
I considered STREAM, but what hold me is that for RDMA, for instance, we have a connection-oriented, reliable, datagram-oriented protocol, but not a stream.
If you think STREAM is more intuitive and is worth the slight over-use, I'm ok going with it.
| _In_ uint32_t ClientRecvContextLength, | ||
| _In_opt_ const CXPLAT_UDP_DATAPATH_CALLBACKS* UdpCallbacks, | ||
| _In_opt_ const CXPLAT_TCP_DATAPATH_CALLBACKS* TcpCallbacks, | ||
| _In_opt_ const CXPLAT_DATAPATH_DGRAM_CALLBACKS* UdpCallbacks, |
There was a problem hiding this comment.
If we're claiming this is more abstracted, should we also rename the variables from Udp/Tcp? Or is that out of scope for now / forever?
There was a problem hiding this comment.
It makes sense to do it here.
I first did a big research replace and reverted it because for most instances, UdpCallbacks is actually specifically about UDP, but this definition is generic.
I would like to improve the documentation around the internal data path layer - we sure have had a lot of design questions lately. If there's a reasonable spot to start a skeleton, let's do that so future PRs have one less reason to say "no" to documentation. |
I'll add a skeleton doc we can populate progressively, it is as good of a time as any. Note this isn't about datapath refactoring (yet), but prep work toward supporting connection-oriented transports in the core layer. |
Description
Rename datapath callback types to make them more generic based on datagram or connection-oriented transport, since MsQuic is going to support other transports than UDP/TCP.
Testing
CI
Documentation
N/A, the types are internal