Implement Connection Pool API #4832

anrossi · 2025-02-20T20:32:04Z

Description

Provide an API for pooling connections across multiple RSS cores

Testing

Manual testing

Documentation

Documentation will be updated with the new API

src/inc/msquic.h

src/inc/quic_datapath.h

nibanks · 2025-02-20T21:07:48Z

src/platform/datapath_raw_xdp_win.c

+    if (RssConfig == NULL) {
+        Status = QUIC_STATUS_INVALID_PARAMETER;
+        goto Error;
+    }


If this is an internal function, we can probably assert this instead of checking it. SAL indicates this is a required output too.

Well it's part of the CxPlat interface, so if we make that library separate, it should validate this

If we ever actually do that, then there will be plenty to clean up. For now, simpler is better IMO.

I think I'm going to leave this here. Why would we want less validation? It's on the control path, so this isn't performance sensitive.

nit: it's not more or less validation, it is hard failure vs soft failure :)
This check means the caller might do something wrong (not provide the output buffer) and it will be harder for us to realize because we migth recover. A crash would point at the error directly.

I would prefer a DBG assert instead of a check for all internal functions, independent of if it might be exposed later.

src/platform/unittest/ToeplitzTest.cpp

codecov · 2025-02-21T05:13:41Z

Codecov Report

Attention: Patch coverage is 32.54717% with 143 lines in your changes missing coverage. Please review.

Project coverage is 86.28%. Comparing base (da4cbd2) to head (acd61c6).

Files with missing lines	Patch %	Lines
src/core/connection_pool.c	30.39%	142 Missing ⚠️
src/core/library.h	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4832      +/-   ##
==========================================
- Coverage   87.08%   86.28%   -0.80%     
==========================================
  Files          56       57       +1     
  Lines       17685    17894     +209     
==========================================
+ Hits        15401    15440      +39     
- Misses       2284     2454     +170

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/inc/msquic.h

src/inc/msquic_posix.h

src/inc/quic_datapath.h

src/platform/datapath_raw_xdp_win.c

src/inc/msquic_posix.h

src/core/connection_pool.c

src/inc/msquic.h

src/core/connection_pool.c

src/platform/toeplitz.c

src/platform/unittest/ToeplitzTest.cpp

src/inc/quic_toeplitz.h

src/platform/datapath_raw_xdp_win.c

src/core/connection_pool.c

src/core/connection.c

src/core/connection.h

src/inc/msquic.h

…tensible

guhetier · 2025-03-05T17:19:50Z

docs/api/QUIC_CONNECTION_POOL_CONFIG.md

+| Flag | Effect |
+|------|--------|
+| QUIC_CONNECTION_POOL_FLAG_NONE | Nothing |
+| QUIC_CONNECTION_POOL_FLAG_CLOSE_CONNECTIONS_ON_FAILURE | Tells the API to clean up any created *and started* connections in the pool if an error occurrs. **Note:** The application must be able to handle having connections suddenly closed with this flag. Without this flag, the application is expected to clean up remaining connections. |


"and started" -> does it means the flag causes all created connections to be closed on failure, or only started connections?

I assume the former, that is the reasonable thing to do - but that sentence can be confusing. Maybe the "started" part should be in the "Note": "The connection might have been started when it get close because of this flag, ...."

guhetier · 2025-03-05T17:32:07Z

src/core/library.h

+    void
+    )
+{
+    const uint16_t CurrentProc = (uint16_t)CxPlatProcCurrentNumber();


Is the cast needed? The function accepts a uint32_t

guhetier · 2025-03-05T17:39:31Z

src/core/connection.c

@@ -73,7 +74,8 @@ QuicConnAlloc(
    // partition can be updated accordingly.


This comment should be updated.
(the "just use the current processor" part is more nuanced now).

Ugh, github is annoying, it doesn't let me comment on a line out of its diff :(

guhetier · 2025-03-05T17:45:53Z

src/test/bin/winkernel/control.cpp

+            QuicTestConnectionPoolCreate(
+                Params->ConnPoolCreateParams.Family,
+                Params->ConnPoolCreateParams.NumberOfConnections,
+                Params->ConnPoolCreateParams.XdpSupported == TRUE,


Why the == TRUE? If this is already a "nice" boolean, there is no need for it. If it isn't yet a "nice" boolean, this could give the wrong result (and !! or != FALSE would be better).

We had a pattern (that seems to have been abandoned) where boolean values from user mode were converted for use in kernel mode with != FALSE (which is what I should have done).
These are "nice" booleans, so I should be able to use them directly. Let's try that

guhetier · 2025-03-05T17:53:51Z

src/inc/msquic.h

+DEFINE_ENUM_FLAG_OPERATORS(QUIC_CONNECTION_POOL_FLAGS);
+
+typedef struct QUIC_CONNECTION_POOL_CONFIG {
+    _In_ HQUIC Registration;


_In_ SAL annotation don't mean anything on a struct, and are not used in MsQuic currently (or at least in this file). I don't think we should introduce that pattern.
_Field_* annotations should be used if there is something to specify here.

Fair point. I'll convert the ones that can be to _Field_* and remove the rest.

guhetier · 2025-03-05T18:20:04Z

src/platform/datapath_raw_xdp_win.c

+    if (RssConfig == NULL) {
+        Status = QUIC_STATUS_INVALID_PARAMETER;
+        goto Error;
+    }


nit: it's not more or less validation, it is hard failure vs soft failure :)
This check means the caller might do something wrong (not provide the output buffer) and it will be harder for us to realize because we migth recover. A crash would point at the error directly.

docs/api/QUIC_CONNECTION_POOL_CONFIG.md

guhetier · 2025-03-05T18:30:09Z

src/core/connection_pool.c

+    QUIC_STATUS Status = QUIC_STATUS_SUCCESS;
+    uint32_t CreatedConnections = 0;
+
+    if (Config == NULL || ConnectionPool == NULL || Config->Registration == NULL ||


nit: That if test is very hard to read.
It could easily be split in several tests and potentially in a "validateConfig" function.

I'll break it up to make it easier to read, but this is an area where I disagree with making a helper function.

guhetier · 2025-03-05T18:36:31Z

src/core/connection_pool.c

+            Status);
+        goto Error;
+    } else if (RssConfig->RssSecretKeyLength < CXPLAT_TOEPLITZ_KEY_SIZE_MIN) {
+        Status = QUIC_STATUS_INVALID_STATE;


Shouldn't this one also be INTERNAL_ERROR?
Is there a scenario that can lead here, or is this check to be cautious but essentially an assert?

It could be an ASSERT because I don't think RSS can be configured with a smaller key size (on windows at least. No idea about Linux).
But I picked INVALID_STATE because the RSS configuration would be in an invalid state. Potentially difficult for the caller to understand that.

I see...
INVALID_STATE is generally used when the caller did something at the wrong time or in the wrong order.

Do you know if the user could do something that affect the rss key length? If yes, INVALID_STATE can make sense.
But if it means the OS is in a wrong config (or a config ew don't expect), I think I'd keep INTERNAL_ERROR (or some other more precise error code)

guhetier · 2025-03-05T18:47:13Z

src/core/connection_pool.c

+
+        for (; RetryCount < MaxCreationRetries; RetryCount++) {
+
+            uint32_t NewPort = QuicAddrGetPort(&LocalAddress) + 1;


IMO, everything inside the body of this loop should be extracted in a "TryCreateConnection" function, or something similar.
And everything in the parent loop should also be extracted in a "CreateConnection" function or some similar name.

Both are action that are pretty independent from all the pre-processing you need to do first, it will help a lot being able to abstract them.

nibanks · 2025-03-06T13:33:55Z

scripts/prepare-machine.ps1

+        $RssSeedPath = (Join-Path $SetupPath tcprssseed.exe)
+        if (!(Test-Path $RssSeedPath)) { Write-Error "Missing file: $RssSeedPath" }
+        Invoke-Expression "$RssSeedPath set aa55aa55aa55aa55aa55aa55aa55aa55aa55aa55aa55aa55aa55aa55aa55aa55aa55aa55aa55aa55"


Could you put a comment to say why we need this?

nibanks · 2025-03-06T13:37:09Z

src/platform/datapath_raw_xdp_win.c

+    HANDLE InterfaceHandle = NULL;
+    HRESULT Result = XdpInterfaceOpen(InterfaceIndex, &InterfaceHandle);
+    if (FAILED(Result)) {
+        QuicTraceLogError(


In general, we should be using the following format for error traces (not logs) so they are collected in the low volume scenario:

QuicTraceEvent( LibraryErrorStatus, "[ lib] ERROR, %u, %s.", WsaError, "WSAStartup");

nibanks · 2025-03-06T13:39:05Z

src/test/MsQuicTests.h

+#define IOCTL_QUIC_RUN_VALIDATE_CONNECTION_POOL_CREATE \
+QUIC_CTL_CODE(132, METHOD_BUFFERED, FILE_WRITE_DATA)


Suggested change

#define IOCTL_QUIC_RUN_VALIDATE_CONNECTION_POOL_CREATE \

QUIC_CTL_CODE(132, METHOD_BUFFERED, FILE_WRITE_DATA)

#define IOCTL_QUIC_RUN_VALIDATE_CONNECTION_POOL_CREATE \

QUIC_CTL_CODE(132, METHOD_BUFFERED, FILE_WRITE_DATA)

nibanks · 2025-03-06T13:41:36Z

src/test/lib/HandshakeTest.cpp

+        if (This->StartedConnectionCount > This->MaxConnections) {
+            return false;
+        }
+        This->Connections[This->StartedConnectionCount++] =


What happens if/when this is called in parallel on two threads/cpus?

nibanks · 2025-03-06T13:43:03Z

src/core/connection_pool.c

+    _In_ size_t ServerNameLength
+    )
+{
+    char* ServerNameCopy = CXPLAT_ALLOC_NONPAGED(ServerNameLength + 1, QUIC_POOL_SERVERNAME);


nit: offsets in this function are all off.

nibanks · 2025-03-06T13:44:33Z

src/core/connection_pool.c

+    Definitions for the MsQuic Connection Pool API, which allows clients to
+    create a pool of connections that are spread across RSS cores.
+
+--*/


Code coverage is still claiming a lot of the lines in this file are uncovered...

I believe this is because code coverage doesn't run with xdp. Probably should change that...

src/core/connection_pool.c

nibanks · 2025-03-06T13:49:40Z

src/core/connection_pool.c

+        (Config->CibirIds != NULL && Config->CibirIdLength == 0) ||
+        (Config->CibirIds == NULL && Config->CibirIdLength != 0)) {
+        Status = QUIC_STATUS_INVALID_PARAMETER;
+        QuicTraceLogError(


Remember, QuicTraceLog* functions are effectively verbose level 'printf' style traces. If you want to be able to efficiently capture an error trace, you will need to use QuicTraceEvent. Perhaps this is fine for invalid parameter calls (I don't think we even have these at all for api.c), but please keep in mind for all your error logging.

nibanks · 2025-03-06T13:50:47Z

src/core/connection_pool.c

+        return Status;
+    }
+
+    QuicTraceEvent(


FYI, in general, for other functions in api.c we are logging api enter/exit even for INVALID_PARAMETER cases.

Right, but I can't dereference an untrusted pointer without validating it isn't NULL. So either I log entry after the validation, or I log entry without any parameter, which seems to be the requirement for this scenario.

nibanks · 2025-03-06T13:51:35Z

src/core/connection_pool.c

+
+    Status = CxPlatDataPathRssConfigGet(InterfaceIndex, &RssConfig);
+    if (QUIC_FAILED(Status)) {
+        QuicTraceLogError(


In general, you don't need to log an error if the function itself already logs an error.

nibanks added Area: Performance Area: API labels Feb 20, 2025

nibanks reviewed Feb 20, 2025

View reviewed changes

src/inc/msquic.h Outdated Show resolved Hide resolved

nibanks reviewed Feb 20, 2025

View reviewed changes

src/inc/msquic.h Outdated Show resolved Hide resolved

nibanks reviewed Feb 20, 2025

View reviewed changes

src/inc/msquic.h Outdated Show resolved Hide resolved

nibanks reviewed Feb 20, 2025

View reviewed changes

src/inc/quic_datapath.h Show resolved Hide resolved

nibanks reviewed Feb 20, 2025

View reviewed changes

src/platform/unittest/ToeplitzTest.cpp Show resolved Hide resolved

nibanks reviewed Feb 20, 2025

View reviewed changes

src/platform/unittest/ToeplitzTest.cpp Show resolved Hide resolved