-
Notifications
You must be signed in to change notification settings - Fork 529
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
Support Client Migration (Client Side) #4218
base: main
Are you sure you want to change the base?
Conversation
@microsoft-github-policy-service agree |
@@ -909,6 +909,9 @@ typedef struct QUIC_SCHANNEL_CREDENTIAL_ATTRIBUTE_W { | |||
#define QUIC_PARAM_CONN_STATISTICS_V2 0x05000016 // QUIC_STATISTICS_V2 | |||
#define QUIC_PARAM_CONN_STATISTICS_V2_PLAT 0x05000017 // QUIC_STATISTICS_V2 | |||
#define QUIC_PARAM_CONN_ORIG_DEST_CID 0x05000018 // uint8_t[] | |||
#define QUIC_PARAM_CONN_ADD_LOCAL_ADDRESS 0x05000019 // QUIC_ADDR | |||
#define QUIC_PARAM_CONN_REMOVE_LOCAL_ADDRESS 0x0500001A // QUIC_ADDR | |||
#define QUIC_PARAM_CONN_LOCAL_UNUSED_DEST_CID_COUNT 0x0500001B // uint16_t |
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 this for test only? Do real clients really need this? If it's test only, please put in msquicp.h
.
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.
Real clients also need 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.
I'd really prefer to be able to abstract away the complexities of connection IDs from clients. Why exactly do clients need this information? To know if they can add a new path?
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.
Yes. This information is needed to know when clients can add a new local address. But this is no longer needed if we adopt a new way in which clients defer sending a path challenge until they receive a new ConID.
@@ -2523,7 +2523,6 @@ CxPlatRecvDataReturn( | |||
DATAPATH_RX_IO_BLOCK* IoBlock = | |||
CXPLAT_CONTAINING_RECORD(Datagram, DATAPATH_RX_PACKET, Data)->IoBlock; | |||
|
|||
CXPLAT_DBG_ASSERT(Binding == NULL || Binding == IoBlock->Binding); |
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.
Why?
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.
When a connection binds more than one bindings (i.e. after adding a new local address on the client side), we cannot assure that the Datagrams in RecvDataChain come from the same binding.
QUIC_PARAM_CONN_STATISTICS_V2_PLAT, | ||
&Size, | ||
&Stats)); | ||
TEST_EQUAL(Stats.RecvDroppedPackets, 0); |
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.
How does this line help? Why would the client drop a packet? Also, are there other test conditions we can add here to determine if the new path worked?
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.
The intention is that I would like to check whether the client succeeded in adding the source connection IDs to a newly opened bindings (if it failed, it will drop a packet whose destination is a newly added local address).
And I think that an event notification about the validation of a path is helpful for both testing and real application.
@@ -746,8 +768,24 @@ QuicLookupAddLocalCid( | |||
Hash); | |||
|
|||
if (ExistingConnection == NULL) { | |||
Result = | |||
QuicLookupInsertLocalCid(Lookup, Hash, SourceCid, TRUE); | |||
QUIC_CID_HASH_ENTRY* CID = |
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.
Just a thought, but we might need to start using pools for allocating and freeing all these entries so they don't hurt perf.
|
||
BOOLEAN Collision = FALSE; | ||
|
||
CxPlatDispatchLockAcquire(&MsQuicLib.DatapathLock); |
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.
Why do we have to grab a global lock here? That's not great, and definitely could cause perf issues.
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 agree with you. I've tried to remove a global lock in masa-koz#5.
In this change, we will replace all the existing source connection IDs if we find the collision in adding a source connection ID to a newly opened bindings.
src/core/connection.c
Outdated
if (!Connection->State.Started) { | ||
Status = QUIC_STATUS_INVALID_STATE; | ||
break; | ||
} |
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.
Why not? It seems kind of intuitive to me that a client could create a connection, queue all paths it wants to use and then start it. No?
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 agree with you. So, I've also implemented this. masa-koz#4
src/core/connection.c
Outdated
break; | ||
} | ||
|
||
if (Connection->PathsCount == QUIC_MAX_PATH_COUNT) { |
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.
Put this check up before creating a new destination CID. Also, return 'invalid state' instead?
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 agree with you. I will change this.
src/core/connection.c
Outdated
|
||
QUIC_CID_LIST_ENTRY* NewDestCid = QuicConnGetUnusedDestCid(Connection); | ||
if (NewDestCid == NULL) { | ||
Status = QUIC_STATUS_INVALID_STATE; |
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 wonder if we can support a model where we always queue the path (somehow) even if we don't have a CID to use for it, yet...
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've implemented this.
https://github.com/masa-koz/msquic/pull/3/files
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.
Thanks. The hardest part with this change overall will likely be that we need to make sure we have lots of tests for all the different scenarios.
src/core/connection.c
Outdated
break; | ||
} | ||
|
||
QUIC_CID_LIST_ENTRY* NewDestCid = QuicConnGetUnusedDestCid(Connection); |
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.
Does this need to be cleaned up at all the failure cases below?
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 suppose that cleaning up is not needed because UsedLocally is not set yet. Isn't it?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4218 +/- ##
==========================================
- Coverage 85.33% 84.20% -1.14%
==========================================
Files 56 56
Lines 15358 15538 +180
==========================================
- Hits 13105 13083 -22
- Misses 2253 2455 +202 ☔ View full report in Codecov by Sentry. |
_In_ QUIC_BINDING* BindingSrc, | ||
_In_ QUIC_BINDING* BindingDest, | ||
_In_ QUIC_CONNECTION* Connection | ||
QuicBindingRemoveRemoteHash( |
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.
Does this mean there's no way to move source CIDs between bindings anymore?
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.
No. We can move source CIDs between old binding and new binding by setting QUIC_PARAM_CONN_LOCAL_ADDRESS. In this case, we will call QuicBindingAddAllSourceConnectionIDs() for new binding and QuicBindingRemoveAllSourceConnectionIDs() for old binding.
|
||
QUIC_BINDING* Binding = | ||
CXPLAT_CONTAINING_RECORD(Link, QUIC_BINDING, Link); | ||
QUIC_CONNECTION* Connection1 = |
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.
Can we use a more descriptive name instead of Connection1
? Like SearchConnection
or CollisionConnection
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 agree. I will change this.
_IRQL_requires_max_(PASSIVE_LEVEL) | ||
static | ||
QUIC_STATUS | ||
QuicConnRemoveLocalAddress( |
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.
There doesn't appear to be any test coverage for the Remove address function here. Could you add a test for that?
@@ -221,6 +243,7 @@ QuicConnGetPathForPacket( | |||
&& QuicAddrGetFamily(&Packet->Route->RemoteAddress) == QuicAddrGetFamily(&Connection->Paths[i].Route.RemoteAddress) | |||
&& QuicAddrCompareIp(&Packet->Route->RemoteAddress, &Connection->Paths[i].Route.RemoteAddress) | |||
&& QuicAddrCompare(&Packet->Route->LocalAddress, &Connection->Paths[i].Route.LocalAddress)) { | |||
QuicLibraryReleaseBinding(Connection->Paths[i].Binding); |
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.
Where is this reference added?
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.
In the case of server, QuicLibraryTryAddRefBinding() was called in QuicConnGetPathForPacket(). In the case of client, QuicBindingGetLocalAddress() was called in QuicConnAddLocalAddress().
By the way, the non-null check should be added.
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.
First of all, your changes look great, thanks for doing this!
I'd like to see some more tests. Since you're adding new APIs, could you add some negative test cases for SetParam(QUIC_CONN_ADD_LOCAL_ADDRESS)
/SetParam(QUIC_CONN_REMOVE_LOCAL_ADDRESS)
. For example, tests that add/remove an address that is not valid, add/remove an address before the connection is started, add the same address twice/remove the same address twice, a test that adds more addresses than the maximum path count, and one that adds an address that isn't on the system, but is valid, e.g. 100.64.10.20.
In the Path tests, I'd appreciate a test that adds an address and removes the old one and verifies no traffic drops. If you have time, also a test that removes all addresses from a connection.
Thanks again!
@masa-koz Are you going to have time to add the requested extra tests? If not, do you mind if we add them later? |
Description
This PR adds the support of Client Migration (Client SIde). Fix #1946.
Testing
I've added two test.
Documentation
In this PR, the following three Connection level Parameters will be added.