use memory windows to invalidate request buffers; resolves issue-5729#6542
use memory windows to invalidate request buffers; resolves issue-5729#6542tpashkin wants to merge 1 commit into
Conversation
|
Note This is an automated comment that will be appended during run. Note All workloads for linux-x86_64-relwithdebinfo have completed. Tip Planned checks for linux-x86_64-relwithdebinfo.
🔴 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 4756s): some tests FAILED for commit 9097af8.
🔴 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 4394s): some tests FAILED for commit 9097af8.
🔴 linux-x86_64-relwithdebinfo target: cloud/filestore/ (test time: 4440s): some tests FAILED for commit 9097af8.
|
tpashkin
changed the title
|
Current version works, but is extremely slow. Next step would be to set up memory windows cache @EvgeniyKozev @yegorskii I'll be on vacation for the next 2 weeks, so feel free to pick this up |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9097af8dcc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| with_lock (AllocationLock) { | ||
| if (requestBytes) { | ||
| req->InBuffer = SendBuffers.AcquireBuffer(requestBytes); | ||
| req->InMemoryWindow = Verbs->CreateMemoryWindow(Connection->pd); |
There was a problem hiding this comment.
When UseMemoryWindows is left at its default false, non-empty requests still call CreateMemoryWindow here, so the normal RDMA path now depends on ibv_alloc_mw(IBV_MW_TYPE_2) succeeding. On providers or devices that do not support type-2 memory windows, or when MW resources are exhausted, ordinary requests fail allocation and disconnect even though the feature was not enabled; only allocate these windows when Config.UseMemoryWindows is true.
Useful? React with 👍 / 👎.
| << req->State); | ||
| } | ||
| } | ||
| // request has been cancelled before bind completed |
There was a problem hiding this comment.
With memory windows enabled, a cancellation or timeout while the request is in either bind state removes it from ActiveRequests; when that bind completion arrives, this ActiveRequests.Get(reqId) path falls through to the comment and never returns send to SendQueue. Unlike SendRequestCompleted, this permanently loses a send WR for each such cancellation/timeout, eventually preventing new requests from being dequeued until reconnect/flush timeout.
Useful? React with 👍 / 👎.
| .mw = req->InMemoryWindow.get(), | ||
| .rkey = req->InMemoryWindow->rkey, | ||
| .bind_info = { | ||
| .mr = req->InBuffer.GetMemoryRegion(), |
There was a problem hiding this comment.
When UseMemoryWindows is enabled, this bind uses the pooled buffer's MR, but those pools are still registered in CreateQP() with only IBV_ACCESS_LOCAL_WRITE | IBV_ACCESS_REMOTE_* and not IBV_ACCESS_MW_BIND. Providers reject IBV_WR_BIND_MW against an MR that was not registered for memory-window binding, so the new mode cannot successfully send requests until the buffer-pool registration flags include IBV_ACCESS_MW_BIND.
Useful? React with 👍 / 👎.
| NVerbs::TMemoryWindowPtr InMemoryWindow = NVerbs::NullPtr; | ||
| NVerbs::TMemoryWindowPtr OutMemoryWindow = NVerbs::NullPtr; |
There was a problem hiding this comment.
These new MW members are still alive while TRequest::~TRequest() runs its body, and that body calls FreeRequest() before member destruction. With UseMemoryWindows and a custom buffer allocation (for example requests larger than MaxChunkAlloc), FreeRequest() can deregister/free the MR backing InBuffer or OutBuffer while the corresponding MW is still bound to it; invalidate/deallocate the windows before releasing the pooled buffers.
Useful? React with 👍 / 👎.
1 participant
use memory windows to invalidate request buffers