Conversation
Signed-off-by: chije.park <chije.park@sk.com>
Signed-off-by: chije.park <chije.park@sk.com>
Signed-off-by: chije.park <chije.park@sk.com>
|
👋 Hi Chije! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
📝 WalkthroughWalkthroughThe GDS_MT backend now preserves registered GPU buffer bases and descriptor offsets. It validates descriptor ranges and adds unit and integration coverage for interior-pointer transfers. ChangesGDS interior-pointer handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/plugins/gds_mt/gds_mt_backend.cpp`:
- Around line 99-102: Rename the helper method effectiveAddr to effective_addr
and update its invocation in the surrounding code, including the call currently
near line 188, while preserving its behavior.
In `@src/plugins/gds_mt/gds_mt_backend.h`:
- Around line 50-55: Rename the public resolver gdsMtResolveRegisteredBuffer to
snake_case as gds_mt_resolve_registered_buffer in the header declaration, its
definition in gds_mt_backend.cpp, and every call site, preserving the existing
signature and behavior.
- Around line 36-39: Add `///<` documentation comments to both public members,
`devPtrBase` and `devPtrOffset`, in `gdsMtResolvedBuffer`, describing the
meaning of the base device pointer and its offset.
🪄 Autofix
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: Enterprise
Run ID: a0eb5862-42da-4d3d-aa19-483c4f0dbd88
📒 Files selected for processing (6)
src/plugins/gds_mt/gds_mt_backend.cppsrc/plugins/gds_mt/gds_mt_backend.htest/gtest/unit/meson.buildtest/gtest/unit/plugins/gds_mt/gds_mt_interior_pointer_test.cpptest/gtest/unit/plugins/gds_mt/gds_mt_offset_test.cpptest/gtest/unit/plugins/gds_mt/meson.build
| uintptr_t | ||
| effectiveAddr() const { | ||
| return reinterpret_cast<uintptr_t> (devPtrBase) + devPtrOffset; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Rename effectiveAddr to snake_case.
Rename this helper to effective_addr and update its call on Line 188.
Proposed change
- effectiveAddr() const {
+ effective_addr() const {-<< " effective_address=" << reinterpret_cast<void *> (req->effectiveAddr());
+<< " effective_address=" << reinterpret_cast<void *> (req->effective_addr());As per path instructions, use snake_case for functions.
🤖 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 `@src/plugins/gds_mt/gds_mt_backend.cpp` around lines 99 - 102, Rename the
helper method effectiveAddr to effective_addr and update its invocation in the
surrounding code, including the call currently near line 188, while preserving
its behavior.
Source: Path instructions
| struct gdsMtResolvedBuffer { | ||
| void *devPtrBase; | ||
| size_t devPtrOffset; | ||
| }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Document each public result member.
Add ///< documentation for devPtrBase and devPtrOffset. The public type currently documents only the structure.
Proposed change
struct gdsMtResolvedBuffer {
- void *devPtrBase;
- size_t devPtrOffset;
+ void *devPtrBase; ///< Base pointer registered with cuFile.
+ size_t devPtrOffset; ///< Descriptor offset from devPtrBase.
};As per path instructions, use ///< for member documentation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| struct gdsMtResolvedBuffer { | |
| void *devPtrBase; | |
| size_t devPtrOffset; | |
| }; | |
| struct gdsMtResolvedBuffer { | |
| void *devPtrBase; ///< Base pointer registered with cuFile. | |
| size_t devPtrOffset; ///< Descriptor offset from devPtrBase. | |
| }; |
🤖 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 `@src/plugins/gds_mt/gds_mt_backend.h` around lines 36 - 39, Add `///<`
documentation comments to both public members, `devPtrBase` and `devPtrOffset`,
in `gdsMtResolvedBuffer`, describing the meaning of the base device pointer and
its offset.
Source: Path instructions
| nixl_status_t | ||
| gdsMtResolveRegisteredBuffer(void *registered_base, | ||
| size_t registered_size, | ||
| uintptr_t descriptor_addr, | ||
| size_t descriptor_size, | ||
| gdsMtResolvedBuffer &resolved); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Rename the public resolver to snake_case.
gdsMtResolveRegisteredBuffer does not follow the required function naming style. Rename the declaration, definition in src/plugins/gds_mt/gds_mt_backend.cpp, and all call sites together.
As per path instructions, use snake_case for functions.
🤖 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 `@src/plugins/gds_mt/gds_mt_backend.h` around lines 50 - 55, Rename the public
resolver gdsMtResolveRegisteredBuffer to snake_case as
gds_mt_resolve_registered_buffer in the header declaration, its definition in
gds_mt_backend.cpp, and every call site, preserving the existing signature and
behavior.
Source: Path instructions
|
@Chije did you get a chance to run nixl bench on these changes? |
|
@mbapatu-spec I ran an additional NIXLBench comparison between mainline and Configuration:
Results:
The results show comparable WRITE performance and slightly higher READ The benchmark uses a 64 MiB GPU allocation with multiple 4 MiB transfer This benchmark is supplementary to the unit and GDS_MT integration tests, The full raw logs are attached as : nixlbench-pr2062-logs.tar.gz |
|
If you are doing I/O using multiple threads simultaneously with the same base_ptr, then cufile library may fallback to the slower I/O path due to the fact that base_ptr is busy with another I/O. Given that, you may not see any gain in performance defeating the purpose of your change. |
|
@tell-rebanta Thanks for pointing this out. Both versions issued the same number of GDS operations, with no POSIX Performance was effectively unchanged: READ: 13.181 GB/s (mainline) vs 13.248 GB/s (PR) Therefore, we did not observe a meaningful performance regression, and The goal of PR #2062 is to correctly preserve the cuFile-registered The current tracepoint does not expose the exact reason for selecting |
|
LGTM |
|
While I agree that the performance has not been regressed, it did not improve either with this change if that is the goal. FYI, If you enable cufile logging in TRACE mode, cufile.log file can show if it has taken registered/unregistered path or not (specifically, I would search "unregistered work item" in the log). |
Additional cuFile route-trace validationI added cuFile route-trace results from a Dynamo KVBM + vLLM workload using Test configuration:
The route trace shows a clear difference in READ handling:
In the pre-PR configuration, all READ operations used the bounce-buffer This is consistent with the intended behavior of preserving the cuFile The attached route-specific latency histograms show the distributions for
The histogram is intentionally split by route because an aggregate READ |
3 participants
What?
Fix GDS_MT handling of interior GPU pointers.
The GDS_MT backend currently treats the transfer descriptor pointer as the
cuFile registered base pointer. This is incorrect when a transfer descriptor
refers to an interior region of a larger GPU allocation.
This PR:
Fixes #2032
Why?
A GPU allocation may be registered once with cuFile while individual transfer
descriptors refer to subranges inside that allocation.
Without preserving the registered base and descriptor-relative offset:
devPtr_offset = 0, even though the larger allocation was registered at adifferent base address.
request preparation and may reach cuFile, where it can fail later as a
backend I/O error.
This change makes GDS_MT follow cuFile registered-buffer semantics for
interior GPU pointers.
How?
The GDS_MT buffer-resolution path distinguishes between:
The request preparation path validates that the complete transfer descriptor is
contained within the registered allocation before creating a transfer request.
Unit tests cover:
A separate GDS_MT integration test:
Relationship to #1856
PR #1856 proposes consolidating the GDS and GDS_MT implementations under the
cuda_gdssource tree and sharing request preparation logic.This PR is based on the current pre-consolidation GDS_MT source layout and does
not include the changes from #1856. If #1856 is merged first, this change will
need to be rebased and ported to the shared
cuda_gdsrequest-preparationlayer.
The intended follow-up is to preserve the same registered-base and
descriptor-offset semantics for both GDS and GDS_MT after the consolidation,
while ensuring that the offset is calculated and applied exactly once.
Testing
Focused validation passed:
Build environment
Validation was performed with:
http://127.0.0.1:2379ABI-incompatible
The dependency paths below are represented by environment variables because
they are machine-specific.
Build configuration
Build
Test execution
$ meson test -C build-gds-mt-pr-focused \ --no-rebuild \ --timeout-multiplier 12 \ --print-errorlogs \ --logbase=pr-focused-testlog \ posix_plugin_test \ unit \ gtest \ telemetry_benchmark \ tracing_nsys \ gds_mt_interior_pointer_integrationThe four GDS_MT offset unit tests passed as part of
nixl:unit.The two existing GDS path-mode smoke tests were excluded from this focused
validation because their runtime-directory setup issue is unrelated to #2032.
No smoke-test infrastructure changes are included in this PR.
TELEMETRY_DOCAwas disabled because the available DOCA telemetry exporterlibrary was ABI-incompatible with the required symbols. The GDS and GDS_MT
validation was executed with GDS enabled.
Summary by CodeRabbit
Bug Fixes
Tests