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

nibanks · 2025-02-20T20:56:19Z

src/inc/msquic.h

+typedef
+_IRQL_requires_max_(PASSIVE_LEVEL)
+_Check_return_
+QUIC_STATUS
+(QUIC_API * QUIC_SIMPLE_CONN_POOL_CREATE_FN)(
+    _In_ HQUIC* Registration,
+    _In_ QUIC_CONNECTION_CALLBACK_HANDLER Handler,
+    _In_opt_ void* Context,
+    _In_opt_ const char* ServerName,
+    _In_opt_ const QUIC_ADDR* ServerAddress,
+    _In_ uint16_t ServerPort,
+    _In_ uint32_t NumberOfConnections,
+    _Out_writes_bytes_(NumberOfConnections * sizeof(HQUIC))
+        HQUIC** ConnectionPool
+    );


I think the extra API table is unnecessary. Let's just add this function directly to the MsQuic table and eliminate all the rest.

But with the second level API table, we can extend the connection pool API to more than just an array of connections. I don't know how future-looking we should be with this design

Let's keep it as simple as possible. As long as it's wrapped by preview flag we can change later if necessary.

nibanks · 2025-02-20T20:57:43Z

src/inc/msquic.h

+_IRQL_requires_max_(PASSIVE_LEVEL)
+_Check_return_
+QUIC_STATUS
+(QUIC_API * QUIC_SIMPLE_CONN_POOL_CREATE_FN)(


Let's call this just QUIC_CONNECTION_POOL_CREATE_FN, and the usage below ConnectionPoolCreate. I'd rather not include simple in the name. We can keep it wrapped in QUIC_API_ENABLE_PREVIEW_FEATURES to allow for possible changes in the future if we want to make it more complicated.

nibanks · 2025-02-20T20:58:25Z

src/inc/msquic.h

@@ -1657,6 +1713,8 @@ typedef struct QUIC_API_TABLE {
    QUIC_STREAM_PROVIDE_RECEIVE_BUFFERS_FN
                                        StreamProvideReceiveBuffers; // Available from v2.5
 #endif
+    QUIC_CONNECTION_POOL_API_OPEN_FN     ConnectionPoolApiOpen; // Available from v2.5


Please put it in QUIC_API_ENABLE_PREVIEW_FEATURES, make sure it aligns with the other names and align the comment too.

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.

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

src/platform/unittest/ToeplitzTest.cpp

+#include <stdio.h>
+
+#ifdef QUIC_CLOG
+#include "ToeplitzTest.cpp.clog.h"


There's a clog file you need to add this too I think, for this to work.

src/platform/unittest/ToeplitzTest.cpp

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

Codecov Report

Attention: Patch coverage is 1.38889% with 142 lines in your changes missing coverage. Please review.

Project coverage is 86.19%. Comparing base (2c81e07) to head (54be6ca).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
src/core/connection_pool.c	0.00%	142 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4832      +/-   ##
==========================================
- Coverage   87.30%   86.19%   -1.12%     
==========================================
  Files          56       57       +1     
  Lines       17634    17778     +144     
==========================================
- Hits        15396    15324      -72     
- Misses       2238     2454     +216

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

nibanks · 2025-02-21T14:26:08Z

src/inc/msquic.h

+//
+
+//
+// Creates a simple connection pool with NumberOfConnections connections


Suggested change

// Creates a simple connection pool with NumberOfConnections connections

// Creates a simple pool of NumberOfConnections connections,

nibanks · 2025-02-21T14:26:39Z

src/inc/msquic.h

+//
+// Creates a simple connection pool with NumberOfConnections connections
+// all with the same Context and Handler, and puts them in the
+// caller-supplied array.


Mention that the goal of this pool is to evenly spread these connections across RSS as much as possible.

nibanks · 2025-02-21T14:28:05Z

src/inc/msquic.h

+    _In_ HQUIC Registration,
+    _In_ QUIC_CONNECTION_CALLBACK_HANDLER Handler,
+    _In_opt_ void* Context,
+    _In_opt_ const char* ServerName,


Since ServerName can also be an IP address (as a string), let's remove the ServerAddress variable. Again, we're trying to make this as simple as possible.

nibanks · 2025-02-21T14:29:36Z

src/inc/msquic.h

+    _In_ uint16_t ServerPort,
+    _In_ uint16_t NumberOfConnections,
+    _Out_writes_bytes_(NumberOfConnections * sizeof(HQUIC))
+        HQUIC** ConnectionPool


Pointer to an array of pointers to handles is a lot of indirection. I'm wondering if we can just take a pointer to an array of handles...

nibanks · 2025-02-21T14:32:19Z

src/inc/msquic_posix.h

+#define QUIC_ADDR_EPHEMERAL_PORT_MIN 49152
+#define QUIC_ADDR_EPHEMERAL_PORT_MAX 65535


At least on Windows, I think these ranges are customizable. Are they on Linux as well?

nibanks · 2025-02-21T14:33:31Z

src/inc/quic_datapath.h

+#define CXPLAT_RSS_HASH_TYPE_IPV4        0x001
+#define CXPLAT_RSS_HASH_TYPE_TCP_IPV4    0x002
+#define CXPLAT_RSS_HASH_TYPE_UDP_IPV4    0x004
+#define CXPLAT_RSS_HASH_TYPE_IPV6        0x008
+#define CXPLAT_RSS_HASH_TYPE_TCP_IPV6    0x010
+#define CXPLAT_RSS_HASH_TYPE_UDP_IPV6    0x020
+#define CXPLAT_RSS_HASH_TYPE_IPV6_EX     0x040
+#define CXPLAT_RSS_HASH_TYPE_TCP_IPV6_EX 0x080
+#define CXPLAT_RSS_HASH_TYPE_UDP_IPV6_EX 0x100


Why are types set up as flags? Can you set all of them simultaneously? I'd assume they are mutually exclusive.

And, should these be an enum?

nibanks · 2025-02-21T14:35:39Z

src/platform/datapath_raw_xdp_win.c

+        RawRssConfig->HashSecretKeySize +
+        RawRssConfig->IndirectionTableSize;
+    CXPLAT_RSS_CONFIG* NewRssConfig =
+        (CXPLAT_RSS_CONFIG*)CXPLAT_ALLOC_NONPAGED(


You used PAGED above and NONPAGED here. I know it doesn't really matter in user mode, but we might adapt this for kernel one day, so we should get it right. I'd say PAGED for both, as this likely wouldn't need to be called at DISPATCH.

nibanks · 2025-02-21T14:37:57Z

src/inc/msquic_posix.h

+inline
+void
+QuicAddrIncrementPort(
+    _Inout_ QUIC_ADDR* Addr
+    )
+{
+    uint32_t Port = QuicAddrGetPort(Addr);
+    Port += 1;
+    if (Port > QUIC_ADDR_EPHEMERAL_PORT_MAX) {
+        Port = QUIC_ADDR_EPHEMERAL_PORT_MIN;
+    }
+    QuicAddrSetPort(Addr, (uint16_t)Port);
+}
+


This function already uses the platform abstraction layer to get/set the port and is duplicated 3 times. Finally, it's only used in one place (connection pool). I say we remove this helper and just do the logic directly in the connection pool to eliminate all the duplication.

nibanks · 2025-02-21T14:41:52Z

src/core/connection_pool.c

+            }
+        }
+        //
+        // This is safe because the RssProcessor array is the same size as the indirection table.


If you add a debug assert above that they are the same, SAL should be happy.

nibanks · 2025-02-21T14:53:53Z

src/core/connection_pool.c

+        }
+        CreatedConnections++;
+
+        Status = MsQuicSetParam(Connections[i], QUIC_PARAM_CONN_REMOTE_ADDRESS, sizeof(ResolvedRemoteAddress), &ResolvedRemoteAddress);


Use QuicConnSetParam instead. Also, you can debug assert the result, because it can only fail if you mess up the code here (i.e. invalid parameters). Same for local address.

nibanks · 2025-02-21T14:54:45Z

src/inc/msquic.h

+(QUIC_API * QUIC_CONN_POOL_CREATE_FN)(
+    _In_ HQUIC Registration,
+    _In_ QUIC_CONNECTION_CALLBACK_HANDLER Handler,
+    _In_opt_ void* Context,


They probably need to pass in an array of context pointers, a unique one for each.

nibanks · 2025-02-21T15:05:44Z

src/core/connection_pool.c

+
+        } while (TRUE);
+
+        Status = MsQuicConnectionOpen(Registration, Handler, Context, &Connections[i]);


Call QuicConnAlloc directly. Another thing: By default, creating a connection uses the "current partition" (i.e., current processor) to affinitize the connection, which isn't really what you want. You want it to be on the RSS partition. So, I would actually recommend modifying QuicConnAlloc to expose a new input parameter for PartitionIndex so you can set it explicitly.

nibanks · 2025-02-21T15:44:49Z

src/core/connection_pool.c

+        //
+        // The connection was created successfully, add it to the count for this processor.
+        //
+        ConnectionCounts[RssProcIndex]++;


You're going to have to call QuicConnStart otherwise the port you tried to find that was open might get taken. And, really, you're asking for trouble by using a model of 'try to find an open port, then stop using it, and hope you can get it again'. You should find an initial ephemeral port, and then start creating and starting connections on ports until you've got enough successfully started connections.

Implement Connection Pool API

af58125

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

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

anrossi added 2 commits February 20, 2025 20:37

Add implementation of connection pool

188603b

Update CLOG and CSharp

54be6ca

nibanks reviewed Feb 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Connection Pool API #4832

Implement Connection Pool API #4832

anrossi commented Feb 20, 2025

nibanks Feb 20, 2025

anrossi Feb 20, 2025

nibanks Feb 20, 2025

nibanks Feb 20, 2025

nibanks Feb 20, 2025

nibanks Feb 20, 2025

anrossi Feb 20, 2025

nibanks Feb 21, 2025

nibanks Feb 20, 2025

codecov bot commented Feb 21, 2025

nibanks Feb 21, 2025

nibanks Feb 21, 2025

nibanks Feb 21, 2025 •

edited

Loading

nibanks Feb 21, 2025

nibanks Feb 21, 2025

nibanks Feb 21, 2025

nibanks Feb 21, 2025

nibanks Feb 21, 2025

nibanks Feb 21, 2025

nibanks Feb 21, 2025

nibanks Feb 21, 2025

nibanks Feb 21, 2025

nibanks Feb 21, 2025

nibanks Feb 21, 2025

	// Creates a simple connection pool with NumberOfConnections connections
	// Creates a simple pool of NumberOfConnections connections,

		#define QUIC_ADDR_EPHEMERAL_PORT_MIN 49152
		#define QUIC_ADDR_EPHEMERAL_PORT_MAX 65535


		} while (TRUE);

		Status = MsQuicConnectionOpen(Registration, Handler, Context, &Connections[i]);

Implement Connection Pool API #4832

Are you sure you want to change the base?

Implement Connection Pool API #4832

Conversation

anrossi commented Feb 20, 2025

Description

Testing

Documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 21, 2025

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nibanks Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nibanks Feb 21, 2025 •

edited

Loading