-
Notifications
You must be signed in to change notification settings - Fork 577
Initial Changes for RDMA as a platform for MsQuic #5113
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
base: main
Are you sure you want to change the base?
Conversation
…quic into srsubra/msquic_rdma
@srsubra please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
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.
Review in progress, making my way through the RDMA code.
Added a few generic comments.
@@ -37,6 +38,7 @@ CxPlatDataPathInitialize( | |||
ClientRecvContextLength, | |||
UdpCallbacks, | |||
TcpCallbacks, | |||
RdmaCallbacks, |
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.
We need to clarify which design we follow for RDMA, between the "TCP / UDP" one and the "normal/xdp" one.
Which is tricky, because RDMA is a protocol (like udp/tcp) but it also largely align with what we do for xdp (co-exists as an alternative pipe to the network).
Maybe it will have to be part of a bigger refactoring of the datapath, later.
@@ -25,7 +25,7 @@ CxPlatSocketUpdateQeo( | |||
) | |||
{ | |||
if (Socket->UseTcp || (Socket->RawSocketAvailable && | |||
!IS_LOOPBACK(Offloads[0].Address))) { | |||
!IS_LOOPBACK(Offloads[0].Address))) { |
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.
nit: Random witespaces
// | ||
// The following APIs are specific for RDMA Implementation | ||
// | ||
#define CXPLAT_RDMA_FLAG_SHARE_ENDPOINT 0x00000001 // Forces sharing of the address and port |
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.
Please use an enum for flags.
@@ -2643,7 +2705,8 @@ CxPlatSocketRelease( | |||
_In_ CXPLAT_SOCKET* Socket | |||
) | |||
{ | |||
if (CxPlatRefDecrement(&Socket->RefCount)) { | |||
if (CxPlatRefDecrement(&Socket->RefCount)) | |||
{ |
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.
nit: We try to keep a consistent style. Before we conclude the PR, we will need to fix it ({
are opened on the line of the if
, a few spaces issues, etc... through the PR).
I won't comment on it for now, just a todo for later.
@@ -0,0 +1,138 @@ | |||
<Project ToolsVersion="Current"> |
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.
File checked in by mistake?
|
||
if (RdmaConnection->QueuePair) | ||
{ | ||
RdmaConnection->QueuePair->lpVtbl->Release(RdmaConnection->QueuePair); |
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.
By curiosity, what is your logic for function that your wrapped vs these direct calls?
|
||
Status = NdspiCreateOverlappedFile(RdmaAdapter, &RdmaAdapter->OverlappedFile); | ||
if (QUIC_FAILED(Status)) { | ||
QuicTraceLogError( |
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.
Here (and in other occurences with wrappers), we end up logging the same error multiple time. It might be worth chosing between loging in the wrapper or out of it and systematically apply it.
Status = QUIC_STATUS_OUT_OF_MEMORY; | ||
goto ErrorExit; | ||
} | ||
memset(RdmaConnection, 0, sizeof(RDMA_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.
memset(RdmaConnection, 0, sizeof(RDMA_CONNECTION)); | |
CxPlatZeroMemory(RdmaConnection, sizeof(RDMA_CONNECTION)); |
RemoteAddress ? | ||
((uint16_t)(CxPlatProcCurrentNumber() % Datapath->PartitionCount)) : 0; | ||
Socket->Mtu = CXPLAT_MAX_MTU; | ||
Socket->RecvBufLen = |
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.
This should probably be updated for RDMA. I don't think receive coalescing can apply
goto ErrorExit; | ||
} | ||
|
||
if (RemoteAddress != NULL) |
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.
What does it mean for a client socket to not have a remote address? Is it possible / valid?
this PR is mainly for help with debugging issues with datapath tests. A New PR will be raised only the code has completed testing