-
Notifications
You must be signed in to change notification settings - Fork 577
Refactor Global Execution Config Flags #5059
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5059 +/- ##
==========================================
- Coverage 87.19% 86.70% -0.50%
==========================================
Files 59 59
Lines 17927 17993 +66
==========================================
- Hits 15632 15600 -32
- Misses 2295 2393 +98 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the large amount of merge conflicts and file changes, would it make sense to split this PR into:
- additions to the Settings API (CI all passing)
- changes to the datapath and tests (CI all passing, minus the tools being built)
- changes to the tools (secnetperf, ... etc.) (CI all passing, rebasing off of previous PR)
src/core/connection_pool.c
Outdated
(QUIC_CONFIGURATION*)Config->Configuration; | ||
CXPLAT_SOCKET_FLAGS SocketFlags = CXPLAT_SOCKET_FLAG_NONE; | ||
|
||
if (MsQuicLib.Settings.IsSet.XdpEnabled) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this logic is wrong in the case of a setting that is TRUE
by default (when not set).
It should be:
- always set the setting from
MsQuicLib.Settings.*
- if
ConnectinoConfig->Settings.IsSet.*
, then override
Same for all settings there.
CxPlatDataPathGetSupportedFeatures( | ||
_In_ CXPLAT_DATAPATH* Datapath | ||
_In_ CXPLAT_DATAPATH* Datapath, | ||
_In_ CXPLAT_SOCKET_FLAGS SocketFlags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a specific reason to use socket flags here instead of a "IncludeRawDatapath" boolean?
Most socket flags don't mean anything in this context, and callers seems to be crafting socket flags just for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It allows us to return features based on the type of socket, such as TCP or PCP or XDP. Right now, it's just for XDP, but in the future we might want to adapt further.
@@ -132,6 +123,8 @@ CxPlatSocketCreateUdp( | |||
) | |||
{ | |||
QUIC_STATUS Status = QUIC_STATUS_SUCCESS; | |||
BOOLEAN CreateRaw = Config->Flags & CXPLAT_SOCKET_FLAG_XDP; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we fail if a connection requests XDP but the raw datapath failed to initialize?
In a more generic way, is XDP always considered best-effort or should the app check it is available and decide what to do explicitly?
It seems error prone to let a connection go through the normal path silently, and that's not something we can change anymore after this leave preview.
For RIO, creating the socket fails is RIO is requested but not available, we should try to be consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now, XDP is best effort, because we want to allow a listener socket to not have to know if XDP is installed or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. It might be something to revisit at some point IMO, to be consistent and avoid unexpected behavior at the app layer. Maybe we can add a "best effort" flag in the future or something.
Description
Refactors the XDP and RIO global execution flags to now be settings of the connections. This resulted in simplification of datapath initialization since they no longer needed the config at all. Also, this allowed for eliminating usage in a couple other places, such as the test code, which made it possible to make the struct all preview.
Testing
CI/CD
Documentation
N/A