Add support for both Qtip and UDP traffic for Server Listeners. #4803
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4803 +/- ##
==========================================
- Coverage 86.20% 83.95% -2.26%
==========================================
Files 56 56
Lines 17726 17769 +43
==========================================
- Hits 15281 14918 -363
- Misses 2445 2851 +406 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ProjectsByJackHe
added
Partner: SQL
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -6641,6 +6657,25 @@ QuicConnParamSet( | |||
| break; | |||
| } | |||
|
|
|||
| case QUIC_PARAM_CONN_QTIP: { | |||
There was a problem hiding this comment.
Instead of making a param for this, I think it'd be better to indicate the UseQTIP state in the QUIC_NEW_CONNECTION_INFO struct that's indicated in the NEW_CONNECTION event to listeners.
There was a problem hiding this comment.
I am removing this in the next few iterations. We are fully migrating off of params to settings. I wanted to get the settings stuff working before deleting code.
| QuicTraceLogConnInfo( | ||
| IndicateQTIPSettingsBubbledDown, | ||
| Connection, | ||
| "Bubbling down UseQTIP setting: %hhu", | ||
| UdpConfig.UseQTIP); |
There was a problem hiding this comment.
This log should be unnecessary, IMO, because QuicConnApplyNewSettings would log the value of every settings.
| @@ -1770,6 +1770,11 @@ QuicConnStart( | |||
|
|
|||
| CXPLAT_TEL_ASSERT(Path->Binding == NULL); | |||
|
|
|||
| QuicConnApplyNewSettings( | |||
There was a problem hiding this comment.
We want to make sure we don't also call this in QuicConnSetConfiguration for the client path now (but you do for server).
| @@ -578,6 +577,8 @@ typedef struct CXPLAT_UDP_CONFIG { | |||
| uint8_t CibirIdOffsetSrc; // CIBIR ID offset in source CID | |||
| uint8_t CibirIdOffsetDst; // CIBIR ID offset in destination CID | |||
| uint8_t CibirId[6]; // CIBIR ID data | |||
|
|
|||
| uint8_t UseQTIP; | |||
| if (IoMode && IsValue(IoMode, "qtip")) { | ||
| Config->Flags |= QUIC_EXECUTION_CONFIG_FLAG_QTIP; | ||
| SetConfig = true; | ||
| MsQuicSettings Settings; | ||
| Settings.SetQtipEnabled(TRUE); | ||
| Settings.SetGlobal(); | ||
| } |
There was a problem hiding this comment.
Now that this doesn't have to be global, let's move this logic to PerfClient.cpp, and set the Configurations settings.
| @@ -113,7 +113,9 @@ class QuicTestEnvironment : public ::testing::Environment { | |||
| #if defined(QUIC_API_ENABLE_PREVIEW_FEATURES) | |||
| if (UseQTIP) { | |||
| Config.PollingIdleTimeoutUs = 10000; | |||
| Config.Flags |= QUIC_EXECUTION_CONFIG_FLAG_QTIP; | |||
| MsQuicSettings Settings; | |||
| Settings.SetQtipEnabled(TRUE); | |||
There was a problem hiding this comment.
| Settings.SetQtipEnabled(TRUE); | |
| Settings.SetQtipEnabled(true); |
C++ code uses true instead of TRUE.
| @@ -578,6 +577,9 @@ typedef struct CXPLAT_UDP_CONFIG { | |||
| uint8_t CibirIdOffsetSrc; // CIBIR ID offset in source CID | |||
| uint8_t CibirIdOffsetDst; // CIBIR ID offset in destination CID | |||
| uint8_t CibirId[6]; // CIBIR ID data | |||
|
|
|||
| uint8_t UseQTIP; | |||
| uint8_t IgnoreRawSocketFailure; | |||
There was a problem hiding this comment.
Please use socket flags instead
Description
Currently, we have an execution parameter (QUIC_EXECUTION_CONFIG_FLAG_QTIP) that you set to either process packets via QTIP or normal QUIC/UDP traffic.
For a server listener, that means you can only talk to clients with the same execution profile as you; you either deal over QTIP or QUIC/UDP.
This is inconvenient and sometimes a blocker when you want your listener to accept from clients with varying execution parameters (QTIP through the Azure load balancer, normal QUIC/UDP for on-prem clients)
Changes
The main behavior change is that now all Server listeners created will accept both QTIP and QUIC+udp traffic by default when XDP is configured and running correctly.
To support this new design and make testing a bit easier, we also add a knob at the connection layer to control whether a connection created is going to send QUIC+udp/QTIP.
This pull-request also updates the behavior of QUIC_EXECUTION_CONFIG_FLAG_QTIP: If you set this flag, all client connections created will default to using QTIP unless otherwise specified. Note that the QUIC_EXECUTION_CONFIG_FLAG_QTIP does not impact server Listeners as they will always accept both QUIC/QTIP traffic assuming XDP is configured and running correctly.
This PR adds an API (connection Get/Set parameter) for client connections to specify whether the connection sends normal QUIC and/or TCP traffic.
Closes: #3516
Testing
Modified a datatest to alternate between creating normal QUIC connections and QTIP connections, where they all ping the same listener. The expectation is all the tests pass.
Documentation
QUIC_EXECUTION_CONFIG_FLAG_QTIPandQUIC_PARAM_CONN_QTIPwill be documented in a follow up PR.