Repository navigation
nixlbench: fix stats computation and memory management bugs - #1548
oliviergaraud wants to merge 4 commits into
Conversation
… printStats Throughput needs total iterations (all threads combined) since wall_time covers all threads running in parallel. Latency needs per-thread iterations since each thread runs that many sequentially. Signed-off-by: Olivier Garaud <olivier.garaud@scality.com>
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>
|
👋 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. 🚀 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughCompute metrics using per-thread iteration counts and adjust large-block scaling; initialize DRAM buffers using each I/O vector's length; defer local segment cleanup until after remote/backend deregistration and perform backend-specific cleanup ordering. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Worker
participant RemoteBackend
participant LocalSegments
Worker->>LocalSegments: allocateMemory(iov_list)
Note right of LocalSegments: DRAM memset using each `iov.len`
Worker->>RemoteBackend: begin remote deregister/cleanup
RemoteBackend-->>Worker: remote cleanup complete (object/block/file backends run cleanupBasicDesc*)
Worker->>LocalSegments: deregister/cleanup local iov_lists
LocalSegments-->>Worker: local cleanup complete (cleanupBasicDescDram/cleanupBasicDescVram)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…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>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp (1)
1003-1011: 🧹 Nitpick | 🔵 TrivialNit: GUSLI branch keeps the old cleanup-before-deregister order.
For consistency with the new OBJ/FILE pattern, consider flipping to deregister-then-cleanup here too. Currently safe only because
cleanupBasicDescBlkis a no-op (L717–720); if it ever grows a body, this branch will silently regress to the bug this PR is fixing.♻️ Proposed reorder for consistency
} else if (xferBenchConfig::backend == XFERBENCH_BACKEND_GUSLI) { for (auto &iov_list : remote_iovs) { - for (auto &iov : iov_list) { - cleanupBasicDescBlk(iov); - } nixl_reg_dlist_t desc_list(BLK_SEG); iovListToNixlRegDlist(iov_list, desc_list); CHECK_NIXL_ERROR(agent->deregisterMem(desc_list, &opt_args), "deregisterMem failed"); + for (auto &iov : iov_list) { + cleanupBasicDescBlk(iov); + } } } else if (xferBenchConfig::isStorageBackend()) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp` around lines 1003 - 1011, GUSLI branch currently calls cleanupBasicDescBlk before deregistering, which can regress if cleanupBasicDescBlk gains behavior; change the loop over remote_iovs to first build desc_list via iovListToNixlRegDlist and call agent->deregisterMem(desc_list, &opt_args) (use CHECK_NIXL_ERROR) and only after successful deregistration call cleanupBasicDescBlk for each iov; update the block handling XFERBENCH_BACKEND_GUSLI to match the deregister-then-cleanup order used in OBJ/FILE paths and reference remote_iovs, iovListToNixlRegDlist, deregisterMem, and cleanupBasicDescBlk when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp`:
- Around line 1003-1011: GUSLI branch currently calls cleanupBasicDescBlk before
deregistering, which can regress if cleanupBasicDescBlk gains behavior; change
the loop over remote_iovs to first build desc_list via iovListToNixlRegDlist and
call agent->deregisterMem(desc_list, &opt_args) (use CHECK_NIXL_ERROR) and only
after successful deregistration call cleanupBasicDescBlk for each iov; update
the block handling XFERBENCH_BACKEND_GUSLI to match the deregister-then-cleanup
order used in OBJ/FILE paths and reference remote_iovs, iovListToNixlRegDlist,
deregisterMem, and cleanupBasicDescBlk when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ed372933-0d39-4891-935b-dc13db6c112b
📒 Files selected for processing (1)
benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp
…ocateMemory Signed-off-by: Olivier Garaud <olivier.garaud@scality.com>
There was a problem hiding this comment.
The allocation in the nixl_worker is more elegant than the original, but the original was not a bug as in this loop the allocation was to buffer_size anyways.
I would suggest separating the PR into two as well, one for the memory management and one for the statistic calculation change.
What?
Fix two bugs in nixlbench:
Incorrect iteration counts in
printStats: Throughput was computedusing per-thread iterations instead of total iterations (all threads
combined), and latency used total iterations instead of per-thread
iterations. Since wall time covers all threads running in parallel,
throughput needs the total count, while latency (per-thread sequential)
needs the per-thread count.
Memory management issues in
deallocateMemory/allocateMemory:deallocateMemoryderegistered 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.
memsetinallocateMemoryusedbuffer_sizeinstead ofiov.len,which could be wrong when the actual allocation size differs.
Summary by CodeRabbit
Bug Fixes
Refactor