Repository navigation
Nixlbench fix deallocate memory order - #1590
Merged
ofer merged 6 commits intoJun 4, 2026
Merged
Conversation
Two bugs fixed: - deallocateMemory: deregister remote IOVs (OBJ/BLK/FILE) before local IOVs, and for OBJ deregister before cleanup to avoid use-after-free - memset in allocateMemory: use iov.len instead of buffer_size, which could be wrong when the actual allocation size differs Signed-off-by: Olivier Garaud <olivier.garaud@scality.com>
…allocateMemory Deregister memory before closing file descriptors in the FILE_SEG path, matching the fix already applied to OBJ_SEG. Backends like GDS or POSIX may need the FD open during deregistration. Signed-off-by: Olivier Garaud <olivier.garaud@scality.com>
…ocateMemory Signed-off-by: Olivier Garaud <olivier.garaud@scality.com>
oliviergaraud
requested review from
aranadive,
brminich and
ovidiusm
as code owners
April 27, 2026 11:22
|
👋 Hi oliviergaraud! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
Contributor
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughThis change sizes DRAM consistency memset to each IOV's ChangesMemory Management and Cleanup Logic
sequenceDiagram
participant Caller
participant Worker as xferBenchNixlWorker
participant Agent
participant Storage as StorageBackend
participant LocalMem as LocalMemory
Caller->>Worker: deallocateMemory()
Worker->>Agent: deregisterMem(remote_iov_list)
Agent-->>Worker: ack
alt remote has storage-backed IOVs
Worker->>Storage: cleanupBasicDescFile(fd) [track via unordered_set]
Storage-->>Worker: closed
else remote other segments (OBJ/GUSLI/FILE)
Worker->>Worker: per-IOV remote cleanup (OBJ/GUSLI/FILE branches)
end
Worker->>LocalMem: deregisterMem(local_iov_list)
LocalMem-->>Worker: ack
Worker->>Worker: per-IOV local cleanup switch (DRAM / VRAM -> cleanupBasicDesc*)
Worker-->>Caller: done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp`:
- Around line 994-1042: Add an inline comment above the cleanup loop that
explains the required invariant: remote_iovs (OBJ_SEG/BLK_SEG/FILE_SEG) must be
deregistered before the local iov_lists (DRAM_SEG/VRAM_SEG) and that for OBJ_SEG
and FILE_SEG deregistration must occur before per-iov cleanup because
deregisterMem (agent->deregisterMem) may reference object metadata or the fd;
reference the symbols remote_iovs, iov_lists, agent->deregisterMem,
cleanupBasicDescObj, cleanupBasicDescFile, and
cleanupBasicDescDram/cleanupBasicDescVram so future maintainers understand why
the current ordering is necessary and must not be reversed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dcd057b3-cd7d-476b-ae2e-6ef8421c1a42
📒 Files selected for processing (1)
benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp
ofer
previously approved these changes
Apr 27, 2026
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp (1)
1018-1025:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't close FILE descriptors per remote IOV list.
Line 918 reuses
remote_fds[file_idx].fdacross multiple descriptors whennum_files < num_buffers. With the new per-list cleanup, the first loop can close an fd that lateragent->deregisterMem()calls still need, so teardown becomes order-dependent again for shared files. Keep the new “deregister before cleanup” invariant, but defer FILE closes until allFILE_SEGderegistrations are finished, and close each backing fd once.Suggested fix
} else if (xferBenchConfig::isStorageBackend()) { for (auto &iov_list : remote_iovs) { nixl_reg_dlist_t desc_list(FILE_SEG); iovListToNixlRegDlist(iov_list, desc_list); CHECK_NIXL_ERROR(agent->deregisterMem(desc_list, &opt_args), "deregisterMem failed"); - for (auto &iov : iov_list) { - cleanupBasicDescFile(iov); - } + } + for (auto &fd_state : remote_fds) { + if (close(fd_state.fd) != 0) { + std::cerr << "Failed to close file descriptor " << fd_state.fd << ": " + << strerror(errno) << std::endl; + exit(EXIT_FAILURE); + } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp` around lines 1018 - 1025, The current per-list cleanup closes FILEs too early; instead, first perform deregistrations for every iov list (preserving the existing loop using iovListToNixlRegDlist and agent->deregisterMem on desc_list/FILE_SEG), but postpone calling cleanupBasicDescFile(FILE_SEG) until you know no other descriptors reference the same backing fd. Implement a small reference-count or seen-set keyed by the file index found in each iov (derive from remote_iovs entries or remote_fds[file_idx]): increment counts while scanning remote_iovs, do all deregisterMem calls, then do a second pass that calls cleanupBasicDescFile(iov) only when the reference count for that file reaches zero (decrementing per call) so each backing fd is closed exactly once and only after all FILE_SEG deregistrations complete.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp`:
- Around line 1018-1025: The current per-list cleanup closes FILEs too early;
instead, first perform deregistrations for every iov list (preserving the
existing loop using iovListToNixlRegDlist and agent->deregisterMem on
desc_list/FILE_SEG), but postpone calling cleanupBasicDescFile(FILE_SEG) until
you know no other descriptors reference the same backing fd. Implement a small
reference-count or seen-set keyed by the file index found in each iov (derive
from remote_iovs entries or remote_fds[file_idx]): increment counts while
scanning remote_iovs, do all deregisterMem calls, then do a second pass that
calls cleanupBasicDescFile(iov) only when the reference count for that file
reaches zero (decrementing per call) so each backing fd is closed exactly once
and only after all FILE_SEG deregistrations complete.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8d21e1f6-82c4-4733-ba44-f9836a58ab2f
📒 Files selected for processing (1)
benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp
Signed-off-by: Olivier Garaud <olivier.garaud@scality.com>
…all deregistrations Multiple IOVs can share the same backing fd due to file_idx wrapping. Split the FILE_SEG cleanup into two passes: first deregister all lists, then close each unique fd exactly once using a seen-set. Signed-off-by: Olivier Garaud <olivier.garaud@scality.com>
oliviergaraud
force-pushed
the
nixlbench_fix_deallocateMemory_order
branch
from
May 5, 2026 12:40
cd6f64d to
fb5397d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp`:
- Around line 1018-1033: The code correctly deduplicates closing backing file
descriptors using closed_fds to avoid double-close after deregisterMem, but
needs an explicit inline note tying the dedup key to a FILE_SEG backing fd;
locate the block that declares closed_fds (the loop after deregisterMem in the
same scope as remote_iovs) and add a concise comment above closed_fds
(referencing closed_fds, iov.devId, FILE_SEG, and cleanupBasicDescFile) stating
that iov.devId is a POSIX file descriptor for FILE_SEG and is being deduplicated
to ensure each backing FD is closed exactly once; keep the comment short and
explicit so future changes won’t mistakenly reuse devId for non-FD identifiers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d5ec72e3-3df4-41b8-b53e-ba6cc3d17110
📒 Files selected for processing (1)
benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp
Contributor
|
/ok to test fb5397d |
Contributor
|
/build |
iyastreb
reviewed
May 28, 2026
iyastreb
approved these changes
May 29, 2026
Contributor
|
@oliviergaraud please rebase to pick up some CI fixes |
…ocateMemory_order
Contributor
Author
done |
Contributor
|
/ok to test 02e96b6 |
Contributor
|
/build |
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
Fix memory management issues in deallocateMemory/allocateMemory:
deallocateMemory deregistered IOVs in the wrong order:
remote IOVs (OBJ/BLK/FILE) must be deregistered before local IOVs,
and for OBJ the deregistration must happen before cleanup to avoid use-after-free.
+
memset in allocateMemory use iov.len instead of buffer_size.
This could be wrong if the actual allocation size differed, but the original was not a bug as in this loop the allocation was to buffer_size anyways.
Summary by CodeRabbit
Bug Fixes
Refactor