Consolidate GDS and GDS_MT under cuda_gds - #1856
maheshrbapatu wants to merge 11 commits into
Conversation
|
👋 Hi maheshrbapatu! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
|
@maheshrbapatu I am tracking this because if it gets merged it will impact the AIS_MT and hip_ais plugins that enable DGS/AIS for AMD GPUs that I am working on a PR for. See issue #1781. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🤖 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/cuda_gds/gds_backend.cpp`:
- Around line 31-46: The new internal types in gds_backend.cpp use PascalCase,
but the naming convention here requires lowerCamelCase for
types/functions/members. Rename FileSegData and MemSegData to lowerCamelCase
equivalents, and update all references in the nixlGdsMetadata-related code that
construct or store these structs so the identifiers stay consistent across the
backend implementation.
- Around line 190-196: The current validation in gds_backend.cpp allows
FILE_SEG-to-FILE_SEG transfers because the check in the backend path only
rejects when neither side is a file; update the logic around the existing
FILE_SEG type checks to require exactly one FILE_SEG endpoint and explicitly
return NIXL_ERR_INVALID_PARAM when both local and remote are FILE_SEG. Keep the
rest of the flow in the backend transfer handling unchanged, but ensure the
file/memory role selection derived from local.getType() and remote.getType()
cannot proceed for file-to-file cases.
- Around line 200-224: Validate that the memory and file descriptors describe
the same transfer length before creating each GdsXferReq in gds_backend.cpp. In
the loop that builds reqs, compare mem_desc.len and file_desc.len and return
NIXL_ERR_INVALID_PARAM (or equivalent) when they differ, so mismatched pairs are
rejected before reqs.push_back. Use the existing mem_desc/file_desc handling in
the transfer setup block to locate the check.
- Around line 220-224: `prepXfer` currently stores only the raw CUfileHandle_t
in `GdsXferReq`, which can let the underlying FILE_SEG owner be destroyed before
batch or Taskflow submission. Update `GdsXferReq` to hold a
std::shared_ptr<gdsFileHandle> alongside the copied request fields, set it from
`file_data->handle` in `prepXfer`, and change the batch/MT consumer paths to
read `req.file_handle->cu_fhandle` at submission time so the handle stays alive
until use.
In `@src/plugins/cuda_gds/gds_backend.h`:
- Line 37: The new transfer type name in the CUDA GDS backend header should
follow the lowerCamelCase naming rule. Rename GdsXferReq to gdsXferReq in the
type declaration and update all uses of that type across the CUDA GDS codebase,
especially any constructors, function signatures, or member declarations that
reference it, so the identifier remains consistent everywhere.
- Line 133: The header guard in gds_backend.h still uses the old short symbol,
so update the `#ifndef/`#define pair and the closing `#endif` comment to the
repository-relative guard NIXL_SRC_PLUGINS_CUDA_GDS_GDS_BACKEND_H. Make sure the
unique guard symbols in the file match exactly so the header uses the full
path-based naming convention consistently.
In `@src/plugins/cuda_gds/gds_batch_engine.cpp`:
- Around line 150-164: Mark failed or short-completed entries in
gds_batch_engine.cpp as inactive before exiting the loop in the batch-processing
path. In the CUfileIOEvents_t handling inside the batch engine function, when
event.status is not CUFILE_COMPLETE or event.ret does not match
params->u.batch.size, clear the corresponding batch entry’s active flag before
returning NIXL_ERR_BACKEND so the caller does not try to cancel an
already-reported event and can reclaim the pool entry safely. Use the existing
batch-entry bookkeeping around io_batch_events, event.cookie, and params to
locate and update the right entry.
In `@src/plugins/cuda_gds/gds_batch_engine.h`:
- Around line 17-18: The header guard in gds_batch_engine.h uses a short local
macro instead of the required repository-relative NIXL guard. Update the guard
symbols in the header so the existing `#ifndef`, `#define`, and closing `#endif`
comment all use NIXL_SRC_PLUGINS_CUDA_GDS_GDS_BATCH_ENGINE_H, matching the full
path-based convention used across the repo.
In `@src/plugins/cuda_gds/gds_mt_engine.cpp`:
- Around line 141-145: Make checkXfer safe after the transfer completes by
guarding against reuse of gds_handle->running_transfer after it has been
consumed. In gds_mt_engine.cpp, update checkXfer to call get() only once when
the future is ready, and then clear or reset the future state so later polls do
not call wait_for on an invalid future. Use the running_transfer member and
checkXfer as the key symbols to locate and fix the completion path.
- Around line 131-134: The postXfer path in gds_mt_engine.cpp is overwriting
nixlGdsMtReqH::running_transfer while a previous transfer may still be active,
which can invalidate the prior graph’s request storage. Update the gds_mt
engine’s postXfer logic to mirror the batch engine’s active repost guard: check
the existing running_transfer and only allow a new executor_->run(...) once the
prior future is ready/finished, otherwise reject the repost before touching the
Taskflow future.
In `@src/plugins/cuda_gds/gds_mt_engine.h`:
- Around line 17-18: The header guard in gds_mt_engine.h uses the wrong symbol;
update the include guard in the file’s top-level guard block to the
repository-relative NIXL form. Replace __GDS_MT_ENGINE_H with the full
path-derived guard name NIXL_SRC_PLUGINS_CUDA_GDS_GDS_MT_ENGINE_H, and ensure
the matching closing directive uses the same symbol so the guard stays
consistent.
In `@src/plugins/cuda_gds/gds_mt_plugin.cpp`:
- Around line 23-28: Wrap the translation-unit helper getGdsMtBackendOptions()
in an anonymous namespace so it has internal linkage, and keep gds_mt_plugin_t
outside if it needs external visibility; this fixes the .cpp helper rule
violation by making the file-local helper truly internal.
In `@src/plugins/cuda_gds/gds_plugin.cpp`:
- Around line 21-28: Wrap the file-local helper getGdsBackendOptions() in an
anonymous namespace so it has internal linkage, and keep the gds_plugin_t alias
outside only if it still needs external visibility. Make the change in the
source file around getGdsBackendOptions and any other internal-only helpers in
this .cpp to match docs/CodeStyle.md.
In `@src/plugins/cuda_gds/gds_utils.h`:
- Line 82: Update the header guard in gds_utils.h from the reserved
__GDS_UTILS_H form to the repository-relative guard
NIXL_SRC_PLUGINS_CUDA_GDS_GDS_UTILS_H, and make sure the top `#ifndef/`#define
pair and the closing `#endif` all match. Use the existing gds_utils.h guard
symbols as the only target, since the guard must follow the NIXL_ +
path-to-upper_snake_case convention.
In `@test/gtest/gds.cpp`:
- Around line 388-391: The shared-fd regression check in the test around
runWriteThenRead should not only verify NIXL_SUCCESS; it must prove the read
actually restored data. In the test case using mem.trim() and file_b.trim(),
clear the buffer before invoking runWriteThenRead and then assert the buffer
contents afterward match kPattern, so the nixlAgent boundary test validates
preserved transfer behavior rather than only backend success. Apply the same
change to the other shared-fd regression case referenced by the same helper
flow.
- Around line 1-456: This file is failing the repository format check, so
reformat the entire gds.cpp test file with clang-format before merging. Focus on
preserving the existing behavior contract tests while applying standard
formatting consistently across helpers like runTransfer, dramRoundTrip, and the
GdsBackend test cases. After formatting, ensure the file no longer differs from
the expected style.
- Around line 308-323: The setup in the GDS tests leaves successful
registrations live when a later step in the chained initialization fails, so
update the affected blocks in gtest/gds.cpp to track each registerMem success
separately and always unwind it before freeing buffers or closing fds. Use the
existing agent.registerMem, agent.deregisterMem, trim, and createXferReq flow,
but replace the short-circuit && pattern with explicit cleanup paths so any
successful registration is deregistered even if a subsequent registerMem or
posix_memalign step fails.
🪄 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: Enterprise
Run ID: 80531f2d-11f6-434f-9d55-96943db1b4c6
📒 Files selected for processing (24)
CODEOWNERSsrc/plugins/cuda_gds/README.mdsrc/plugins/cuda_gds/gds_backend.cppsrc/plugins/cuda_gds/gds_backend.hsrc/plugins/cuda_gds/gds_batch_engine.cppsrc/plugins/cuda_gds/gds_batch_engine.hsrc/plugins/cuda_gds/gds_mt_engine.cppsrc/plugins/cuda_gds/gds_mt_engine.hsrc/plugins/cuda_gds/gds_mt_plugin.cppsrc/plugins/cuda_gds/gds_plugin.cppsrc/plugins/cuda_gds/gds_utils.cppsrc/plugins/cuda_gds/gds_utils.hsrc/plugins/cuda_gds/meson.buildsrc/plugins/gds_mt/gds_mt_backend.cppsrc/plugins/gds_mt/gds_mt_backend.hsrc/plugins/gds_mt/gds_mt_utils.cppsrc/plugins/gds_mt/gds_mt_utils.hsrc/plugins/gds_mt/meson.buildsrc/plugins/meson.buildsrc/utils/file/README.mdtest/gtest/gds.cpptest/gtest/meson.buildtest/unit/plugins/cuda_gds/nixl_gds_test.cpptest/unit/plugins/gds_mt/nixl_gds_mt_test.cpp
💤 Files with no reviewable changes (6)
- src/plugins/gds_mt/gds_mt_backend.cpp
- src/plugins/gds_mt/gds_mt_utils.h
- src/plugins/gds_mt/gds_mt_backend.h
- src/plugins/gds_mt/gds_mt_utils.cpp
- CODEOWNERS
- src/plugins/gds_mt/meson.build
| for (size_t i = 0; i < buf_cnt; i++) { | ||
| const nixlMetaDesc &mem_desc = is_local_file ? remote[i] : local[i]; | ||
| const nixlMetaDesc &file_desc = is_local_file ? local[i] : remote[i]; | ||
|
|
||
| // Add all requests to batch | ||
| for (size_t i = 0; i < batch_size; i++) { | ||
| const auto& req = requests[start_idx + i]; | ||
| if (!req.addr || !req.fh) { | ||
| returnBatchToPool(batch); | ||
| void *base_addr = (void *)mem_desc.addr; | ||
| if (!base_addr) { | ||
| return NIXL_ERR_INVALID_PARAM; | ||
| } | ||
|
|
||
| nixl_status_t status = batch->addToBatch(req.fh, req.addr, req.size, | ||
| req.file_offset, 0, req.op); | ||
| if (status != NIXL_SUCCESS) { | ||
| returnBatchToPool(batch); | ||
| return NIXL_ERR_INVALID_PARAM; | ||
| const auto *md = static_cast<const nixlGdsMetadata *>(file_desc.metadataP); | ||
| if (!md) { | ||
| NIXL_ERROR << "GDS: missing FILE_SEG metadata at xfer time"; | ||
| return NIXL_ERR_NOT_FOUND; | ||
| } | ||
| } | ||
|
|
||
| nixl_status_t status = batch->submitBatch(0); | ||
| if (status != NIXL_SUCCESS) { | ||
| returnBatchToPool(batch); | ||
| return NIXL_ERR_BACKEND; | ||
| } | ||
|
|
||
| batch_list.push_back(batch); | ||
| return NIXL_SUCCESS; | ||
| } | ||
|
|
||
| nixl_status_t nixlGdsEngine::checkXfer(nixlBackendReqH* handle) const | ||
| { | ||
| nixlGdsBackendReqH *gds_handle = (nixlGdsBackendReqH *)handle; | ||
|
|
||
| if (gds_handle->batch_io_list.empty()) { | ||
| gds_handle->needs_prep = true; | ||
| return NIXL_SUCCESS; | ||
| } | ||
|
|
||
| nixl_status_t status = NIXL_SUCCESS; | ||
| for (auto* batch : gds_handle->batch_io_list) { | ||
| status = batch->checkStatus(); | ||
|
|
||
| if (status == NIXL_IN_PROG) { | ||
| return status; | ||
| const auto *file_data = std::get_if<FileSegData>(&md->data_); | ||
| if (!file_data || !file_data->handle) { | ||
| NIXL_ERROR << "GDS: file metadata is not a FILE_SEG variant"; | ||
| return NIXL_ERR_NOT_FOUND; | ||
| } | ||
|
|
||
| if (status < 0) { | ||
| batch->cancelBatch(); | ||
| } | ||
| returnBatchToPool(batch); | ||
| reqs.push_back(GdsXferReq{base_addr, | ||
| mem_desc.len, | ||
| (size_t)file_desc.addr, | ||
| file_data->handle->cu_fhandle, | ||
| (operation == NIXL_READ) ? CUFILE_READ : CUFILE_WRITE}); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate matching descriptor lengths before building the request.
req.size is taken from mem_desc.len while file_desc.len is ignored, so a mismatched pair can read/write outside the FILE_SEG descriptor’s intended range or silently under-transfer. Reject mismatches before push_back.
Proposed fix
const nixlMetaDesc &mem_desc = is_local_file ? remote[i] : local[i];
const nixlMetaDesc &file_desc = is_local_file ? local[i] : remote[i];
+ if (mem_desc.len != file_desc.len) {
+ NIXL_ERROR << "GDS: memory and file descriptor lengths must match";
+ return NIXL_ERR_INVALID_PARAM;
+ }
+
void *base_addr = (void *)mem_desc.addr;📝 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.
| for (size_t i = 0; i < buf_cnt; i++) { | |
| const nixlMetaDesc &mem_desc = is_local_file ? remote[i] : local[i]; | |
| const nixlMetaDesc &file_desc = is_local_file ? local[i] : remote[i]; | |
| // Add all requests to batch | |
| for (size_t i = 0; i < batch_size; i++) { | |
| const auto& req = requests[start_idx + i]; | |
| if (!req.addr || !req.fh) { | |
| returnBatchToPool(batch); | |
| void *base_addr = (void *)mem_desc.addr; | |
| if (!base_addr) { | |
| return NIXL_ERR_INVALID_PARAM; | |
| } | |
| nixl_status_t status = batch->addToBatch(req.fh, req.addr, req.size, | |
| req.file_offset, 0, req.op); | |
| if (status != NIXL_SUCCESS) { | |
| returnBatchToPool(batch); | |
| return NIXL_ERR_INVALID_PARAM; | |
| const auto *md = static_cast<const nixlGdsMetadata *>(file_desc.metadataP); | |
| if (!md) { | |
| NIXL_ERROR << "GDS: missing FILE_SEG metadata at xfer time"; | |
| return NIXL_ERR_NOT_FOUND; | |
| } | |
| } | |
| nixl_status_t status = batch->submitBatch(0); | |
| if (status != NIXL_SUCCESS) { | |
| returnBatchToPool(batch); | |
| return NIXL_ERR_BACKEND; | |
| } | |
| batch_list.push_back(batch); | |
| return NIXL_SUCCESS; | |
| } | |
| nixl_status_t nixlGdsEngine::checkXfer(nixlBackendReqH* handle) const | |
| { | |
| nixlGdsBackendReqH *gds_handle = (nixlGdsBackendReqH *)handle; | |
| if (gds_handle->batch_io_list.empty()) { | |
| gds_handle->needs_prep = true; | |
| return NIXL_SUCCESS; | |
| } | |
| nixl_status_t status = NIXL_SUCCESS; | |
| for (auto* batch : gds_handle->batch_io_list) { | |
| status = batch->checkStatus(); | |
| if (status == NIXL_IN_PROG) { | |
| return status; | |
| const auto *file_data = std::get_if<FileSegData>(&md->data_); | |
| if (!file_data || !file_data->handle) { | |
| NIXL_ERROR << "GDS: file metadata is not a FILE_SEG variant"; | |
| return NIXL_ERR_NOT_FOUND; | |
| } | |
| if (status < 0) { | |
| batch->cancelBatch(); | |
| } | |
| returnBatchToPool(batch); | |
| reqs.push_back(GdsXferReq{base_addr, | |
| mem_desc.len, | |
| (size_t)file_desc.addr, | |
| file_data->handle->cu_fhandle, | |
| (operation == NIXL_READ) ? CUFILE_READ : CUFILE_WRITE}); | |
| for (size_t i = 0; i < buf_cnt; i++) { | |
| const nixlMetaDesc &mem_desc = is_local_file ? remote[i] : local[i]; | |
| const nixlMetaDesc &file_desc = is_local_file ? local[i] : remote[i]; | |
| if (mem_desc.len != file_desc.len) { | |
| NIXL_ERROR << "GDS: memory and file descriptor lengths must match"; | |
| return NIXL_ERR_INVALID_PARAM; | |
| } | |
| void *base_addr = (void *)mem_desc.addr; | |
| if (!base_addr) { | |
| return NIXL_ERR_INVALID_PARAM; | |
| } | |
| const auto *md = static_cast<const nixlGdsMetadata *>(file_desc.metadataP); | |
| if (!md) { | |
| NIXL_ERROR << "GDS: missing FILE_SEG metadata at xfer time"; | |
| return NIXL_ERR_NOT_FOUND; | |
| } | |
| const auto *file_data = std::get_if<FileSegData>(&md->data_); | |
| if (!file_data || !file_data->handle) { | |
| NIXL_ERROR << "GDS: file metadata is not a FILE_SEG variant"; | |
| return NIXL_ERR_NOT_FOUND; | |
| } | |
| reqs.push_back(GdsXferReq{base_addr, | |
| mem_desc.len, | |
| (size_t)file_desc.addr, | |
| file_data->handle->cu_fhandle, | |
| (operation == NIXL_READ) ? CUFILE_READ : CUFILE_WRITE}); |
🤖 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/cuda_gds/gds_backend.cpp` around lines 200 - 224, Validate that
the memory and file descriptors describe the same transfer length before
creating each GdsXferReq in gds_backend.cpp. In the loop that builds reqs,
compare mem_desc.len and file_desc.len and return NIXL_ERR_INVALID_PARAM (or
equivalent) when they differ, so mismatched pairs are rejected before
reqs.push_back. Use the existing mem_desc/file_desc handling in the transfer
setup block to locate the check.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR consolidates GDS and GDS_MT under ChangesGDS/GDS_MT Plugin Consolidation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant nixlGdsEngine
participant TransferEngine
participant cuFile
Caller->>nixlGdsEngine: prepXfer(descriptors)
nixlGdsEngine->>TransferEngine: finalizePrep(gdsXferReq list)
Caller->>TransferEngine: postXfer(handle)
TransferEngine->>cuFile: submit batch or Taskflow operations
Caller->>TransferEngine: checkXfer(handle)
TransferEngine->>cuFile: poll completion
TransferEngine-->>Caller: transfer status
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/plugins/cuda_gds/gds_mt_plugin.cpp (1)
40-50: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep
nixl_plugin_fini()in the dynamic-plugin branch.When
GDS_MTis built as a static plugin, this translation unit is archived into the static library, so the generic C symbolnixl_plugin_finiis emitted even though the static entry point iscreateStaticGDS_MTPlugin(). That breaks the static/dynamic split and can collide with other static plugins that also carry anixl_plugin_finisymbol.Proposed fix
`#else` extern "C" NIXL_PLUGIN_EXPORT nixlBackendPlugin * nixl_plugin_init() { return gds_mt_plugin_t::create(NIXL_PLUGIN_API_VERSION, "GDS_MT", "0.1.0", getGdsMtBackendOptions(), {DRAM_SEG, VRAM_SEG, FILE_SEG}); } -#endif extern "C" NIXL_PLUGIN_EXPORT void nixl_plugin_fini() {} +#endif🤖 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/cuda_gds/gds_mt_plugin.cpp` around lines 40 - 50, `nixl_plugin_fini()` is currently emitted unconditionally from `nixl_plugin_init()`’s translation unit, which causes the static plugin archive to export the generic cleanup symbol even though static builds use `createStaticGDS_MTPlugin()`. Move the `nixl_plugin_fini()` definition into the dynamic-plugin-only path for `gds_mt_plugin_t`/`nixl_plugin_init`, or guard it so it is compiled only when building the dynamic plugin variant, keeping the static and dynamic entry points separate.
🤖 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/meson.build`:
- Around line 49-58: The explicit GDS/GDS_MT enable path in the meson logic can
still be silently skipped when gds_path is unset, because cuda_gds/meson.build
exits via subdir_done() instead of failing. Update the gating around
enabled_plugins.get('GDS')/enabled_plugins.get('GDS_MT') and the cuda_gds subdir
handling so an explicitly requested plugin errors out when gds_path is empty,
rather than completing successfully without building the plugin.
---
Outside diff comments:
In `@src/plugins/cuda_gds/gds_mt_plugin.cpp`:
- Around line 40-50: `nixl_plugin_fini()` is currently emitted unconditionally
from `nixl_plugin_init()`’s translation unit, which causes the static plugin
archive to export the generic cleanup symbol even though static builds use
`createStaticGDS_MTPlugin()`. Move the `nixl_plugin_fini()` definition into the
dynamic-plugin-only path for `gds_mt_plugin_t`/`nixl_plugin_init`, or guard it
so it is compiled only when building the dynamic plugin variant, keeping the
static and dynamic entry points separate.
🪄 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: Enterprise
Run ID: 4857ec8e-148f-4340-b27f-96a5e22ff2c9
📒 Files selected for processing (24)
CODEOWNERSsrc/plugins/cuda_gds/README.mdsrc/plugins/cuda_gds/gds_backend.cppsrc/plugins/cuda_gds/gds_backend.hsrc/plugins/cuda_gds/gds_batch_engine.cppsrc/plugins/cuda_gds/gds_batch_engine.hsrc/plugins/cuda_gds/gds_mt_engine.cppsrc/plugins/cuda_gds/gds_mt_engine.hsrc/plugins/cuda_gds/gds_mt_plugin.cppsrc/plugins/cuda_gds/gds_plugin.cppsrc/plugins/cuda_gds/gds_utils.cppsrc/plugins/cuda_gds/gds_utils.hsrc/plugins/cuda_gds/meson.buildsrc/plugins/gds_mt/gds_mt_backend.cppsrc/plugins/gds_mt/gds_mt_backend.hsrc/plugins/gds_mt/gds_mt_utils.cppsrc/plugins/gds_mt/gds_mt_utils.hsrc/plugins/gds_mt/meson.buildsrc/plugins/meson.buildsrc/utils/file/README.mdtest/gtest/gds.cpptest/gtest/meson.buildtest/unit/plugins/cuda_gds/nixl_gds_test.cpptest/unit/plugins/gds_mt/nixl_gds_mt_test.cpp
💤 Files with no reviewable changes (6)
- src/plugins/gds_mt/meson.build
- src/plugins/gds_mt/gds_mt_utils.h
- src/plugins/gds_mt/gds_mt_utils.cpp
- CODEOWNERS
- src/plugins/gds_mt/gds_mt_backend.h
- src/plugins/gds_mt/gds_mt_backend.cpp
| if enabled_plugins.get('GDS') or enabled_plugins.get('GDS_MT') | ||
| if (disable_gds_backend or not cuda_dep.found()) and is_explicit_enable | ||
| if disable_gds_backend | ||
| error('GDS plugin requested but GDS backend is disabled') | ||
| error('GDS/GDS_MT plugin requested but GDS backend is disabled') | ||
| else | ||
| error('GDS plugin requested but CUDA dependency not found') | ||
| error('GDS/GDS_MT plugin requested but CUDA dependency not found') | ||
| endif | ||
| elif not disable_gds_backend and cuda_dep.found() | ||
| subdir('cuda_gds') | ||
| endif |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don't silently skip explicitly enabled GDS plugins when gds_path is unset.
This unified gate now treats disable_gds_backend and missing CUDA as the only fatal conditions, but src/plugins/cuda_gds/meson.build still calls subdir_done() when get_option('gds_path') == ''. An explicit GDS or GDS_MT enable can therefore succeed without producing the requested plugin or any error.
Proposed fix
# The "GDS" and "GDS_MT" backends are now produced by a single unified plugin
# directory (cuda_gds), which shares one engine and exposes both names. The
# subdir is entered once if either name is enabled; it builds the per-name
# entry points internally based on enabled_plugins.
disable_gds_backend = get_option('disable_gds_backend')
+gds_path = get_option('gds_path')
if enabled_plugins.get('GDS') or enabled_plugins.get('GDS_MT')
- if (disable_gds_backend or not cuda_dep.found()) and is_explicit_enable
+ if (disable_gds_backend or not cuda_dep.found() or gds_path == '') and is_explicit_enable
if disable_gds_backend
error('GDS/GDS_MT plugin requested but GDS backend is disabled')
+ elif gds_path == ''
+ error('GDS/GDS_MT plugin requested but gds_path is not set')
else
error('GDS/GDS_MT plugin requested but CUDA dependency not found')
endif
- elif not disable_gds_backend and cuda_dep.found()
+ elif not disable_gds_backend and cuda_dep.found() and gds_path != ''
subdir('cuda_gds')
endif
endif📝 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.
| if enabled_plugins.get('GDS') or enabled_plugins.get('GDS_MT') | |
| if (disable_gds_backend or not cuda_dep.found()) and is_explicit_enable | |
| if disable_gds_backend | |
| error('GDS plugin requested but GDS backend is disabled') | |
| error('GDS/GDS_MT plugin requested but GDS backend is disabled') | |
| else | |
| error('GDS plugin requested but CUDA dependency not found') | |
| error('GDS/GDS_MT plugin requested but CUDA dependency not found') | |
| endif | |
| elif not disable_gds_backend and cuda_dep.found() | |
| subdir('cuda_gds') | |
| endif | |
| if enabled_plugins.get('GDS') or enabled_plugins.get('GDS_MT') | |
| disable_gds_backend = get_option('disable_gds_backend') | |
| gds_path = get_option('gds_path') | |
| if (disable_gds_backend or not cuda_dep.found() or gds_path == '') and is_explicit_enable | |
| if disable_gds_backend | |
| error('GDS/GDS_MT plugin requested but GDS backend is disabled') | |
| elif gds_path == '' | |
| error('GDS/GDS_MT plugin requested but gds_path is not set') | |
| else | |
| error('GDS/GDS_MT plugin requested but CUDA dependency not found') | |
| endif | |
| elif not disable_gds_backend and cuda_dep.found() and gds_path != '' | |
| subdir('cuda_gds') | |
| endif | |
| endif |
🤖 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/meson.build` around lines 49 - 58, The explicit GDS/GDS_MT enable
path in the meson logic can still be silently skipped when gds_path is unset,
because cuda_gds/meson.build exits via subdir_done() instead of failing. Update
the gating around enabled_plugins.get('GDS')/enabled_plugins.get('GDS_MT') and
the cuda_gds subdir handling so an explicitly requested plugin errors out when
gds_path is empty, rather than completing successfully without building the
plugin.
|
👀 Investigating |
|
🤖 CI Triage Agent — This is a clear and straightforward failure. The logs are complete and unambiguous. Summary: The "PR Size Check" GitHub Actions job failed because PR #1856 exceeds the repository's 500-line modification limit. Root cause: The workflow counts lines added to existing (modified) files, excluding subprojects, via: For this PR the count came to 503 lines, which is 3 over the hard cap of 500. The check is working as designed: This is not a flaky test, infra problem, or hang — it's a policy gate intentionally failing because the change is too large. There were no large time gaps in the log (entire job ran in ~1 second), so nothing is hung. Implicated commit: [REDACTED:Hex High Entropy String] (the head commit of PR #1856 on branch File: The PR Size Check workflow (e.g. Suggested fix: This is a real policy violation, not a CI bug. Choose one:
Note: only 3 lines over, so trimming whitespace/comment churn or splitting off a tiny refactor may be enough to pass. Related: PR #1856 (this PR). No related issues found relevant to a code defect, since this is a size-policy gate.
|
1852914 to
681a270
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/plugins/cuda_gds/gds_backend.h (1)
37-42: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRename
file_offsetto lowerCamelCase.
gdsXferReqis a public struct type in a touchedsrc/**/*.h; its member should befileOffsetto match the member naming rule. As per path instructions, “Naming: lowerCamelCase for types/funcs/members.”🤖 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/cuda_gds/gds_backend.h` around lines 37 - 42, The public struct gdsXferReq currently uses a snake_case member name that violates the lowerCamelCase naming rule for members in touched headers. Rename the file_offset field to fileOffset in gdsXferReq and update any code that reads or writes that member to use the new name consistently.Source: Path instructions
test/gtest/gds.cpp (1)
295-309: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFail when setup registration does not happen.
If either DRAM registration fails, this test skips the rejection assertion and still passes after cleanup. Keep the cleanup path, but record a non-fatal failure so the validation test cannot pass without exercising
createXferReq.Proposed fix
const bool da_registered = agent.registerMem(da, &ep) == NIXL_SUCCESS; const bool db_registered = da_registered && agent.registerMem(db, &ep) == NIXL_SUCCESS; + EXPECT_TRUE(da_registered) << "failed to register first DRAM descriptor"; + EXPECT_TRUE(db_registered) << "failed to register second DRAM descriptor"; if (da_registered && db_registered) {🤖 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 `@test/gtest/gds.cpp` around lines 295 - 309, The GDS rejection test in gtest::LogIgnoreGuard setup currently skips the createXferReq validation whenever registerMem fails, which lets the test pass without exercising the expected failure path. In the gtest/gds.cpp test around the da_registered/db_registered setup, keep the cleanup behavior but add a non-fatal test failure or assertion when either registration does not succeed so the test cannot silently pass; preserve the existing EXPECT_NE/EXPECT_EQ checks on agent.createXferReq, validation_error, and prepare_error when registration succeeds.src/plugins/cuda_gds/gds_batch_engine.cpp (1)
289-304: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate file-offset overflow before chunking.
Line 298 can wrap
req.file_offset + current_offsetwhen a descriptor starts nearSIZE_MAX, causing a chunk to read/write the wrong file range. Reject requests wherefile_offset + sizeoverflows before splitting.Proposed fix
for (const gdsXferReq &req : reqs) { if (!req.addr) { return NIXL_ERR_INVALID_PARAM; } + if (req.size > std::numeric_limits<size_t>::max() - req.file_offset) { + NIXL_ERROR << "GDS: file offset overflow for transfer size " << req.size; + return NIXL_ERR_INVALID_PARAM; + } const size_t chunks = (req.size / max_request_size) + ((req.size % max_request_size) != 0);🤖 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/cuda_gds/gds_batch_engine.cpp` around lines 289 - 304, Validate the file offset before splitting requests in gds_batch_engine.cpp’s chunking loop inside the gds_batch_engine path that builds gdsXferReq chunks. Reject any request where req.file_offset + req.size would overflow size_t before entering the while loop, and only then compute chunk.file_offset from req.file_offset plus current_offset. Use the existing gdsXferReq/request_list flow to place the check as early as possible so oversized descriptors are not chunked.
🤖 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/cuda_gds/gds_backend.cpp`:
- Around line 35-41: The constructors in the gds_backend struct definitions use
initializer lists on the same line as the signature, which violates the C++
style rule. Update fileSegData and memSegData so their constructor initializer
lists are broken onto the next line before the colon, keeping the signatures and
initializers aligned with the project’s constructor formatting convention.
---
Outside diff comments:
In `@src/plugins/cuda_gds/gds_backend.h`:
- Around line 37-42: The public struct gdsXferReq currently uses a snake_case
member name that violates the lowerCamelCase naming rule for members in touched
headers. Rename the file_offset field to fileOffset in gdsXferReq and update any
code that reads or writes that member to use the new name consistently.
In `@src/plugins/cuda_gds/gds_batch_engine.cpp`:
- Around line 289-304: Validate the file offset before splitting requests in
gds_batch_engine.cpp’s chunking loop inside the gds_batch_engine path that
builds gdsXferReq chunks. Reject any request where req.file_offset + req.size
would overflow size_t before entering the while loop, and only then compute
chunk.file_offset from req.file_offset plus current_offset. Use the existing
gdsXferReq/request_list flow to place the check as early as possible so
oversized descriptors are not chunked.
In `@test/gtest/gds.cpp`:
- Around line 295-309: The GDS rejection test in gtest::LogIgnoreGuard setup
currently skips the createXferReq validation whenever registerMem fails, which
lets the test pass without exercising the expected failure path. In the
gtest/gds.cpp test around the da_registered/db_registered setup, keep the
cleanup behavior but add a non-fatal test failure or assertion when either
registration does not succeed so the test cannot silently pass; preserve the
existing EXPECT_NE/EXPECT_EQ checks on agent.createXferReq, validation_error,
and prepare_error when registration succeeds.
🪄 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: Enterprise
Run ID: ac9d6c94-d684-470b-8c0f-374e2db281dd
📒 Files selected for processing (10)
src/plugins/cuda_gds/gds_backend.cppsrc/plugins/cuda_gds/gds_backend.hsrc/plugins/cuda_gds/gds_batch_engine.cppsrc/plugins/cuda_gds/gds_batch_engine.hsrc/plugins/cuda_gds/gds_mt_engine.cppsrc/plugins/cuda_gds/gds_mt_engine.hsrc/plugins/cuda_gds/gds_mt_plugin.cppsrc/plugins/cuda_gds/gds_plugin.cppsrc/plugins/cuda_gds/gds_utils.htest/gtest/gds.cpp
| fileSegData(std::shared_ptr<gdsFileHandle> h, uint64_t id) : handle(std::move(h)), dev_id(id) {} | ||
| }; | ||
|
|
||
| struct memSegData { | ||
| gdsMemBuf buf; | ||
|
|
||
| memSegData(void *addr, size_t size, int flags) : buf(addr, size, flags) {} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Break constructor initializer lists before the colon.
Lines 35 and 41 keep the initializer list on the signature line; split before : to match the C++ style rule. As per path instructions, “Constructor initializer lists: break before the colon.”
Proposed style fix
- fileSegData(std::shared_ptr<gdsFileHandle> h, uint64_t id) : handle(std::move(h)), dev_id(id) {}
+ fileSegData(std::shared_ptr<gdsFileHandle> h, uint64_t id)
+ : handle(std::move(h)), dev_id(id) {}
...
- memSegData(void *addr, size_t size, int flags) : buf(addr, size, flags) {}
+ memSegData(void *addr, size_t size, int flags)
+ : buf(addr, size, flags) {}📝 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.
| fileSegData(std::shared_ptr<gdsFileHandle> h, uint64_t id) : handle(std::move(h)), dev_id(id) {} | |
| }; | |
| struct memSegData { | |
| gdsMemBuf buf; | |
| memSegData(void *addr, size_t size, int flags) : buf(addr, size, flags) {} | |
| fileSegData(std::shared_ptr<gdsFileHandle> h, uint64_t id) | |
| : handle(std::move(h)), dev_id(id) {} | |
| }; | |
| struct memSegData { | |
| gdsMemBuf buf; | |
| memSegData(void *addr, size_t size, int flags) | |
| : buf(addr, size, flags) {} |
🤖 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/cuda_gds/gds_backend.cpp` around lines 35 - 41, The constructors
in the gds_backend struct definitions use initializer lists on the same line as
the signature, which violates the C++ style rule. Update fileSegData and
memSegData so their constructor initializer lists are broken onto the next line
before the colon, keeping the signatures and initializers aligned with the
project’s constructor formatting convention.
Source: Path instructions
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)
src/plugins/cuda_gds/gds_batch_engine.cpp (1)
264-277: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject transfers whose file range overflows
size_t.
chunk.file_offset = req.file_offset + current_offsetassumesreq.file_offset + req.sizecannot wrap. Validate that range before chunking, otherwise a descriptor nearSIZE_MAXcan wrap and target the wrong file region.Proposed fix
if (!req.addr) { return NIXL_ERR_INVALID_PARAM; } + if (req.size > std::numeric_limits<size_t>::max() - req.file_offset) { + return NIXL_ERR_INVALID_PARAM; + } const size_t chunks = (req.size / max_request_size) + ((req.size % max_request_size) != 0);🤖 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/cuda_gds/gds_batch_engine.cpp` around lines 264 - 277, Reject transfers whose file range can wrap `size_t` before chunking in `gds_batch_engine.cpp`. In the `gdsBatchEngine::...` request loop, validate that `req.file_offset + req.size` does not overflow (and that `req.file_offset` itself is valid) before computing `chunks` or later using `chunk.file_offset = req.file_offset + current_offset`. If the range would overflow, return `NIXL_ERR_INVALID_PARAM`.
🤖 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.
Outside diff comments:
In `@src/plugins/cuda_gds/gds_batch_engine.cpp`:
- Around line 264-277: Reject transfers whose file range can wrap `size_t`
before chunking in `gds_batch_engine.cpp`. In the `gdsBatchEngine::...` request
loop, validate that `req.file_offset + req.size` does not overflow (and that
`req.file_offset` itself is valid) before computing `chunks` or later using
`chunk.file_offset = req.file_offset + current_offset`. If the range would
overflow, return `NIXL_ERR_INVALID_PARAM`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 7ca73ae9-d250-48ba-a115-f39af08a2248
📒 Files selected for processing (10)
src/plugins/cuda_gds/gds_backend.cppsrc/plugins/cuda_gds/gds_backend.hsrc/plugins/cuda_gds/gds_batch_engine.cppsrc/plugins/cuda_gds/gds_batch_engine.hsrc/plugins/cuda_gds/gds_mt_engine.cppsrc/plugins/cuda_gds/gds_mt_engine.hsrc/plugins/cuda_gds/gds_mt_plugin.cppsrc/plugins/cuda_gds/gds_plugin.cppsrc/plugins/cuda_gds/gds_utils.htest/gtest/gds.cpp
681a270 to
0c87363
Compare
|
@maheshrbapatu I have some patches coming to enable AMD AIS_MT plugin. I would prefer to do that after this work is merged if it is going to get merged. So I do not cause you more refactor work. Can you comment on your ETA for addressing @ofer review comments? That way I can determine if I need to move forward based on main or your PR. Thanks! |
Hi @sbates130272 appreciate you flagging this! I'm currently running performance benchmarks to validate there are no regressions before addressing @ofer's review comments. Hardware availability is the main constraint, so realistically I'm looking at ~2 weeks before this is ready to merge. If you're able to share your target timeline, I'd love to sync up and make sure we're not stepping on each other's toes. |
|
@maheshrbapatu I am probably at about the same timeline. Let me work up my PR on top of yours and then my testing helps you. Stay tuned. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/plugins/cuda_gds/gds_mt_engine.cpp (2)
137-146: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winMake
checkXfersafe after completion.Line 144 consumes the future via
get(). A subsequent status poll (user may callgetXferStatusrepeatedly) then invokeswait_foron an invalid future, throwingstd::future_error. Guard for an invalid/consumed future.Proposed fix
auto *gds_handle = static_cast<nixlGdsMtReqH *>(handle); + if (!gds_handle->running_transfer.valid()) { + return gds_handle->overall_status.load(); + } + if (gds_handle->running_transfer.wait_for(std::chrono::seconds(0)) != std::future_status::ready) { return NIXL_IN_PROG; } gds_handle->running_transfer.get(); return gds_handle->overall_status.load();🤖 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/cuda_gds/gds_mt_engine.cpp` around lines 137 - 146, Update nixlGdsMtEngine::checkXfer to handle an invalid or already-consumed running_transfer before calling wait_for, returning the stored overall_status for completed transfers. Preserve NIXL_IN_PROG while a valid future is still pending, and only call get() when the future is valid and ready.
130-135: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winReject active reposts before overwriting the Taskflow future.
postXferreassignsrunning_transferwithout checking whether the previous transfer is still in flight; the old future's shared state is dropped (not joined) while its graph still referencesrequest_list. Mirror the batch engine's active-repost guard and only proceed once a ready future is consumed.Proposed fix
auto *gds_handle = static_cast<nixlGdsMtReqH *>(handle); + if (gds_handle->running_transfer.valid()) { + if (gds_handle->running_transfer.wait_for(std::chrono::seconds(0)) != + std::future_status::ready) { + return NIXL_ERR_REPOST_ACTIVE; + } + gds_handle->running_transfer.get(); + } + gds_handle->overall_status.store(NIXL_SUCCESS); gds_handle->running_transfer = executor_->run(gds_handle->taskflow);🤖 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/cuda_gds/gds_mt_engine.cpp` around lines 130 - 135, Update postXfer to guard against an active running_transfer before assigning a new executor_->run result: mirror the batch engine’s active-repost check, reject reposts while the existing future is not ready, and consume the ready future before launching the next taskflow. Preserve the existing status reset and NIXL_IN_PROG return for accepted transfers.src/plugins/meson.build (1)
49-58: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDon't silently skip explicitly enabled GDS/GDS_MT plugins when
gds_pathis unset.This gate treats only
disable_gds_backendand missing CUDA as fatal, butsrc/plugins/cuda_gds/meson.buildstill callssubdir_done()whenget_option('gds_path') == ''. An explicitGDS/GDS_MTenable therefore succeeds without producing the plugin or any error.Proposed fix
disable_gds_backend = get_option('disable_gds_backend') +gds_path = get_option('gds_path') if enabled_plugins.get('GDS') or enabled_plugins.get('GDS_MT') - if (disable_gds_backend or not cuda_dep.found()) and is_explicit_enable + if (disable_gds_backend or not cuda_dep.found() or gds_path == '') and is_explicit_enable if disable_gds_backend error('GDS/GDS_MT plugin requested but GDS backend is disabled') + elif gds_path == '' + error('GDS/GDS_MT plugin requested but gds_path is not set') else error('GDS/GDS_MT plugin requested but CUDA dependency not found') endif - elif not disable_gds_backend and cuda_dep.found() + elif not disable_gds_backend and cuda_dep.found() and gds_path != '' subdir('cuda_gds') endif endif🤖 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/meson.build` around lines 49 - 58, Update the explicit GDS/GDS_MT enablement gate for enabled_plugins so an unset gds_path is treated as a fatal configuration error alongside disable_gds_backend and missing CUDA; otherwise preserve the existing conditional subdir('cuda_gds') behavior for valid configurations.
🤖 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/cuda_gds/gds_mt_engine.cpp`:
- Around line 44-72: Update both error logging paths in runCuFileOp for
cuFileRead and cuFileWrite to use the existing thread-safe nixl_strerror(errno)
helper instead of strerror(errno), preserving the current failure status and
return behavior.
---
Duplicate comments:
In `@src/plugins/cuda_gds/gds_mt_engine.cpp`:
- Around line 137-146: Update nixlGdsMtEngine::checkXfer to handle an invalid or
already-consumed running_transfer before calling wait_for, returning the stored
overall_status for completed transfers. Preserve NIXL_IN_PROG while a valid
future is still pending, and only call get() when the future is valid and ready.
- Around line 130-135: Update postXfer to guard against an active
running_transfer before assigning a new executor_->run result: mirror the batch
engine’s active-repost check, reject reposts while the existing future is not
ready, and consume the ready future before launching the next taskflow. Preserve
the existing status reset and NIXL_IN_PROG return for accepted transfers.
In `@src/plugins/meson.build`:
- Around line 49-58: Update the explicit GDS/GDS_MT enablement gate for
enabled_plugins so an unset gds_path is treated as a fatal configuration error
alongside disable_gds_backend and missing CUDA; otherwise preserve the existing
conditional subdir('cuda_gds') behavior for valid configurations.
🪄 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: Enterprise
Run ID: 3479c34b-def1-42f3-845d-231d67184612
📒 Files selected for processing (24)
CODEOWNERSsrc/plugins/cuda_gds/README.mdsrc/plugins/cuda_gds/gds_backend.cppsrc/plugins/cuda_gds/gds_backend.hsrc/plugins/cuda_gds/gds_batch_engine.cppsrc/plugins/cuda_gds/gds_batch_engine.hsrc/plugins/cuda_gds/gds_mt_engine.cppsrc/plugins/cuda_gds/gds_mt_engine.hsrc/plugins/cuda_gds/gds_mt_plugin.cppsrc/plugins/cuda_gds/gds_plugin.cppsrc/plugins/cuda_gds/gds_utils.cppsrc/plugins/cuda_gds/gds_utils.hsrc/plugins/cuda_gds/meson.buildsrc/plugins/gds_mt/gds_mt_backend.cppsrc/plugins/gds_mt/gds_mt_backend.hsrc/plugins/gds_mt/gds_mt_utils.cppsrc/plugins/gds_mt/gds_mt_utils.hsrc/plugins/gds_mt/meson.buildsrc/plugins/meson.buildsrc/utils/file/README.mdtest/gtest/gds.cpptest/gtest/meson.buildtest/unit/plugins/cuda_gds/nixl_gds_test.cpptest/unit/plugins/gds_mt/nixl_gds_mt_test.cpp
💤 Files with no reviewable changes (5)
- src/plugins/gds_mt/gds_mt_utils.h
- src/plugins/gds_mt/gds_mt_backend.h
- src/plugins/gds_mt/gds_mt_utils.cpp
- src/plugins/gds_mt/meson.build
- src/plugins/gds_mt/gds_mt_backend.cpp
| void | ||
| runCuFileOp(const gdsXferReq *req, std::atomic<nixl_status_t> *overall_status) { | ||
| ssize_t nbytes = 0; | ||
| if (req->op == CUFILE_READ) { | ||
| nbytes = cuFileRead(req->fh, req->addr, req->size, req->file_offset, 0); | ||
| if (nbytes < 0) { | ||
| NIXL_ERROR << "GDS_MT: cuFileRead failed: " << strerror(errno); | ||
| overall_status->store(NIXL_ERR_BACKEND); | ||
| return; | ||
| } | ||
| } else if (req->op == CUFILE_WRITE) { | ||
| nbytes = cuFileWrite(req->fh, req->addr, req->size, req->file_offset, 0); | ||
| if (nbytes < 0) { | ||
| NIXL_ERROR << "GDS_MT: cuFileWrite failed: " << strerror(errno); | ||
| overall_status->store(NIXL_ERR_BACKEND); | ||
| return; | ||
| } | ||
| } else { | ||
| overall_status->store(NIXL_ERR_INVALID_PARAM); | ||
| return; | ||
| } | ||
|
|
||
| if ((size_t)nbytes != req->size) { | ||
| NIXL_ERROR << "GDS_MT: error: short " << ((req->op == CUFILE_READ) ? "read: " : "write: ") | ||
| << nbytes << " out of " << req->size << " bytes - address=" << req->addr; | ||
| overall_status->store(NIXL_ERR_BACKEND); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and surrounding lines.
sed -n '1,220p' src/plugins/cuda_gds/gds_mt_engine.cpp | cat -n
echo
echo "---- search for strerror usages ----"
rg -n "strerror\\(|strerror_r\\(" src/plugins/cuda_gds src | cat -n
echo
echo "---- search for Taskflow worker context in file ----"
rg -n "taskflow|worker|enqueue|async|parallel|std::thread|thread" src/plugins/cuda_gds/gds_mt_engine.cpp | cat -nRepository: ai-dynamo/nixl
Length of output: 17128
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether the file includes headers that already provide thread-safe errno messaging.
sed -n '1,120p' src/plugins/cuda_gds/gds_mt_engine.cpp | cat -nRepository: ai-dynamo/nixl
Length of output: 5066
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Narrow read of the relevant function and adjacent includes.
sed -n '1,140p' src/plugins/cuda_gds/gds_mt_engine.cpp | cat -nRepository: ai-dynamo/nixl
Length of output: 6003
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,180p' src/plugins/cuda_gds/gds_mt_engine.cpp | cat -n
printf '\n---- strerror usages ----\n'
rg -n "strerror\\(|strerror_r\\(" src/plugins/cuda_gds src | cat -n
printf '\n---- threaded context ----\n'
rg -n "Taskflow|worker|thread|std::thread|async|enqueue|future" src/plugins/cuda_gds/gds_mt_engine.cpp | cat -nRepository: ai-dynamo/nixl
Length of output: 17006
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "FILE:"
nl -ba src/plugins/cuda_gds/gds_mt_engine.cpp | sed -n '1,140p'
echo
echo "STRERROR:"
rg -n "strerror\\(|strerror_r\\(" src/plugins/cuda_gds src || trueRepository: ai-dynamo/nixl
Length of output: 198
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the helper already used elsewhere for errno formatting.
sed -n '110,170p' src/utils/common/nixl_log.h | cat -nRepository: ai-dynamo/nixl
Length of output: 1040
Use the existing thread-safe errno helper here.
strerror(errno) can race across Taskflow worker threads; switch both error paths to nixl_strerror(errno) instead.
🤖 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/cuda_gds/gds_mt_engine.cpp` around lines 44 - 72, Update both
error logging paths in runCuFileOp for cuFileRead and cuFileWrite to use the
existing thread-safe nixl_strerror(errno) helper instead of strerror(errno),
preserving the current failure status and return behavior.
Re-home the AMD hipFile work onto the consolidated cuda_gds structure from PR ai-dynamo#1856. The original commit moved GDS_MT to src/plugins/mt/gds/ and put AIS beside it in mt/ais/; ai-dynamo#1856 instead folded GDS_MT into cuda_gds/, so the mt/ layout no longer has a premise and is dropped entirely. Introduce FileEngineBase in src/utils/file/ holding the trivial nixlBackendEngine overrides shared by local file backends (notif/remote/local support, supported mems, connect/disconnect, loadLocalMD/unloadMD), and re-parent nixlGdsEngine onto it so those overrides live in one place. The vestigial template<Derived> parameter from the original FileMtEngineBase is dropped; it was never used. Add src/plugins/rocm_ais/ mirroring cuda_gds: nixlAisEngine is the abstract platform base (hipFile driver lifecycle, registration, queryMem, prepXfer validation and descriptor translation) with finalizePrep as the single backend-specific preparation hook, and nixlAisMtEngine is the Taskflow leaf exposing the "AIS_MT" name. A hipFile batch engine slots in alongside it later via the same hook. The Taskflow request machinery remains duplicated between nixlGdsMtEngine and nixlAisMtEngine; sharing it is deferred to a follow-up rather than guessing at an abstraction now. Also register AIS_MT in registerBuiltinPlugins(); the original branch defined createStaticAIS_MTPlugin() but never wired it, so static builds silently omitted the backend. Note: the rocm_ais Meson block gates on hip_dep, which is introduced by the follow-up HIP-detection commit; the tree does not configure until that lands. Co-Authored-By: Riley Dixon <riley.dixon@amd.com>
H100 performance results
|
|
🤖 CI Triage Agent — TL;DR: The "PR Size Check" gate failed because PR #1856 modifies 570 lines in existing files (excluding subprojects), exceeding the workflow's hard 500-line limit; either split the PR or reduce the diff to ≤500 lines. Full analysisSummary: The Root cause: The workflow computes added lines to modified files via Implicated commit: [REDACTED:Hex High Entropy String] (PR #1856, branch File: Suggested fix: Reduce the PR size to within the 500-line limit — split PR #1856 into smaller, logically separated PRs (e.g. separate the GDS batch taskflow implementation from tests/refactors). Note the check only counts lines in modified ( Related: none
|
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 `@src/plugins/meson.build`:
- Around line 49-58: Keep batch-only GDS independent of Taskflow: in
src/plugins/meson.build lines 49-58, require taskflow_proj only when building
the MT engine; in src/plugins/cuda_gds/meson.build lines 34-47, omit
gds_mt_engine.cpp and taskflow_proj for batch-only targets; and in
src/plugins/cuda_gds/gds_plugin.cpp lines 40-47, expose mode=mt only when MT
support is built.
🪄 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: 9232e4ca-77d0-4a03-9899-a34fe09dbfb6
📒 Files selected for processing (5)
src/plugins/cuda_gds/README.mdsrc/plugins/cuda_gds/gds_plugin.cppsrc/plugins/cuda_gds/meson.buildsrc/plugins/meson.buildtest/gtest/gds.cpp
| if (disable_gds_backend or not cuda_dep.found() or not taskflow_proj.found()) and is_explicit_enable | ||
| if disable_gds_backend | ||
| error('GDS_MT plugin requested but GDS backend is disabled') | ||
| elif not cuda_dep.found() | ||
| error('GDS_MT plugin requested but CUDA dependency not found') | ||
| error('GDS/GDS_MT plugin requested but GDS backend is disabled') | ||
| elif not taskflow_proj.found() | ||
| error('GDS/GDS_MT plugin requested but Taskflow dependency not found') | ||
| else | ||
| error('GDS_MT plugin requested but Taskflow dependency not found') | ||
| error('GDS/GDS_MT plugin requested but CUDA dependency not found') | ||
| endif | ||
| elif not disable_gds_backend and cuda_dep.found() and taskflow_proj.found() | ||
| subdir('gds_mt') | ||
| subdir('cuda_gds') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Keep batch-only GDS independent of Taskflow.
The unified build now rejects GDS when Taskflow is unavailable. This changes the batch backend dependency contract even when users do not enable GDS_MT. The PR objective requires retaining the GDS batch strategy.
src/plugins/meson.build#L49-L58: Require Taskflow only for configurations that build an MT engine.src/plugins/cuda_gds/meson.build#L34-L47: Do not compilegds_mt_engine.cppor addtaskflow_projto a batch-only GDS target.src/plugins/cuda_gds/gds_plugin.cpp#L40-L47: Do not exposemode=mtin a batch-only build, or make that mode conditional on MT support.
📍 Affects 3 files
src/plugins/meson.build#L49-L58(this comment)src/plugins/cuda_gds/meson.build#L34-L47src/plugins/cuda_gds/gds_plugin.cpp#L40-L47
🤖 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/meson.build` around lines 49 - 58, Keep batch-only GDS
independent of Taskflow: in src/plugins/meson.build lines 49-58, require
taskflow_proj only when building the MT engine; in
src/plugins/cuda_gds/meson.build lines 34-47, omit gds_mt_engine.cpp and
taskflow_proj for batch-only targets; and in src/plugins/cuda_gds/gds_plugin.cpp
lines 40-47, expose mode=mt only when MT support is built.
|
🤖 CI Triage Agent — TL;DR: The "PR Size Check" workflow failed because PR #1856 modifies 682 lines in existing files (excluding subprojects), exceeding the hard 500-line limit enforced by Full analysisSummary: The Root cause: This is a policy check, not a bug. The workflow computes Implicated commit: [REDACTED:Hex High Entropy String] (PR #1856, branch File: Suggested fix: This is working as intended — the fix is to reduce the PR's footprint, not the CI. Concrete options:
Related: none Note: the checkout log contains a redacted git auth header (
|
Re-home the AMD hipFile work onto the consolidated cuda_gds structure from PR ai-dynamo#1856. The original commit moved GDS_MT to src/plugins/mt/gds/ and put AIS beside it in mt/ais/; ai-dynamo#1856 folded GDS_MT into cuda_gds/ instead, so the mt/ layout loses its premise and is dropped. Add FileEngineBase in src/utils/file/ with the trivial nixlBackendEngine overrides shared by local file backends, and re-parent nixlGdsEngine onto it. This replaces FileMtEngineBase; its template<Derived> parameter was unused. Add src/plugins/rocm_ais/ mirroring cuda_gds: nixlAisEngine is the abstract platform base (driver lifecycle, registration, queryMem, prepXfer validation and descriptor translation) with finalizePrep as the one backend-specific hook, and nixlAisMtEngine is the Taskflow leaf exposing "AIS_MT". A hipFile batch engine slots in later via the same hook. Behavior deltas against the original AIS_MT engine, all aligning it with the GDS_MT it was derived from: - finalizePrep emplaces one Taskflow task per request instead of a single task looping over all of them. hipFileRead/Write are blocking, so the task count is what sets the I/O queue depth for a request; with one task, thread_count had no effect within a request. Both pre-1856 GDS_MT and ai-dynamo#1856's nixlGdsMtEngine::finalizePrep emplace per request. NEEDS REVIEW: this puts concurrent hipFile calls in flight for the first time. - File-to-file and null-address requests are rejected, matching ai-dynamo#1856. Pre-1856 GDS_MT accepted both. - thread_count parses via nixl::getBackendParamDefaulted rather than a throwing std::stoul, matching GDS_MT since ai-dynamo#1595. A malformed value now falls back to the default instead of throwing from the constructor. The early-exit on overall_status has no GDS_MT counterpart and is kept from the original, though it is now best-effort rather than sequential. The Taskflow request machinery stays duplicated between nixlGdsMtEngine and nixlAisMtEngine; sharing it is deferred rather than guessing now. Also register AIS_MT in registerBuiltinPlugins(). The original branch defined createStaticAIS_MTPlugin() but never wired it, so static builds silently omitted the backend. Co-Authored-By: Riley Dixon <riley.dixon@amd.com>
|
🤖 CI Triage Agent — TL;DR: The "PR Size Check" gate failed because PR #1856 modifies 686 lines in existing files (excluding subprojects), exceeding the workflow's hard 500-line cap. Split the PR into smaller pieces (or, if justified, raise/waive the limit in the workflow). Full analysisSummary: The Root cause: The workflow computes Implicated commit: PR #1856 head File: Suggested fix: Reduce the PR's footprint below 500 changed lines — split Related: none found.
|
vvenkates27
left a comment
vvenkates27
left a comment
There was a problem hiding this comment.
Thanks for this consolidation work— shared engine and keeping the GDS_MT name is a good call until we are able to modify all integrations to use GDS with appropriate mode. I think from my experience the default for GDS should be mt as most people prefer that. But given we have mt separately we should do that when mt is removed.
releaseXferReq() isn't in this diff. What's new is GDS releaseReqH cancelling and pooling batch handles on abort-while-IN_PROG; today that path deletes/leaks instead of reuse. Cancel-on-post/check-error reuse I think we already had.
The one I'd like eyes on is recycling cancelled cuFile batches from releaseReqH — either we test cancel → submit on the same handle, or we keep the old "don't reuse on abort" behavior for now so abort stays compatible with current GDS.
| if (batch->cancelBatch() == NIXL_SUCCESS) { | ||
| // TODO: Establish and test cancel-to-resubmit semantics for every | ||
| // cuFile I/O path before immediately reusing a batch handle. | ||
| returnBatchToPool(batch); |
There was a problem hiding this comment.
I'd skip putting this back in the pool after cancel. If I'm reading it right, abort-while-IN_PROG now does checkXfer → releaseReqH → here, so the next transfer can submit on the same CUfileBatchHandle_t. Today we don't recycle on abort — we delete the req and ~nixlGdsIOBatch bails if the batch is still in flight. Not pretty, but we also don't resubmit on a cancelled handle.
cancel + returnBatchToPool on post/check errors is already there, so I'm not trying to unwind that. Using it from releaseReqH is the new bit, and the TODO above sounds like cancel → reset → submit isn't locked down yet. Until we have a test for that, destroying and doing a fresh cuFileBatchIOSetUp (or just leaving the slot unused) seems safer. Same idea for the checkXfer error path that calls this helper.
There was a problem hiding this comment.
Good catch — you're right that canceling and returning these batches to the pool from releaseReqH is new behavior in this refactor. I'll keep this PR behavior-compatible with the pre-consolidation path: release will no longer cancel or return in-flight batches to the pool. The existing post/check error handling will remain unchanged. We'll handle safe destroy-and-replacement, together with the relevant cancel/reuse tests, as a separate follow-up.
| nixl_status_t | ||
| nixlGdsBatchEngine::releaseReqH(nixlBackendReqH *handle) const { | ||
| auto *gds_handle = static_cast<nixlGdsBatchReqH *>(handle); | ||
| if (cancelAndReclaimBatches(gds_handle->batch_io_list) != NIXL_SUCCESS) { |
There was a problem hiding this comment.
One more thing on cancel failure: if cancelBatch() fails we keep the pointer on the handle, then still delete the req. Those batches live in batch_storage_; batch_io_list is only a checkout, so I don't think they ever get back to the pool. Later posts could hit "pool exhausted". Today checkXfer always returnBatchToPool after a failed batch, even if cancel fails, so we don't strand slots that way.
I'd rather not delete the req while anything is still checked out — destroy/replace those objects first, or fail release and leave the handle around.
There was a problem hiding this comment.
Agreed — leaving slots unavailable is not a good long-term solution, and repeated aborts can reduce usable pool capacity. For this consolidation, I'll preserve the old release behavior rather than introduce unverified reuse of canceled handles. The cancel-failure case in releaseReqH goes away because release will no longer call cancelBatch. I'll address replenishing retired slots through safe destroy-and-replacement in a follow-up.
Keep batch as the default and preserve the standalone GDS_MT compatibility entry point.
0fe79ba to
d4feb85
Compare
|
🤖 CI Triage Agent — TL;DR: The "PR Size Check" GitHub Actions job failed because PR #1856 modifies 684 lines in existing files (excluding subprojects), exceeding the workflow's hard limit of 500. The fix is to split the PR into smaller pieces (or adjust the limit if intentional). Full analysisSummary: The Root cause: The workflow computes This is an intentional size-policy gate doing its job — not a build/test/infra failure. Note also the check only counts Implicated commit: [REDACTED:Hex High Entropy String] (the PR head merged as 6fc047e) — i.e. the PR content itself, not a specific regressing commit. File: Suggested fix: This is a legitimate policy trip, so resolve it at the PR level rather than "fixing" CI:
No source-code investigation or infra telemetry is warranted here — the check is a deliberate gate and behaved as designed. Related: none
|
|
🤖 CI Triage Agent — TL;DR: This isn't a code or infra defect — the Full analysisSummary: GitHub Actions job Root cause: The workflow computes added lines on the PR merge commit with Implicated commit: No defective commit — the gate itself was added in File: Suggested fix: Split PR #1856 into a stack of smaller PRs under 500 modified-file lines each — e.g. separate the GDS batch taskflow core change from test/benchmark/plugin-glue changes and land them sequentially. Two things worth knowing before restructuring: (1) moving code into new files doesn't count, since Related: PR #1856 (the failing PR); gate introduced by #627. Other currently-open PRs surfaced by search that are likely hitting or near the same gate: #2215, #2200, #1793, #2243 — if several large PRs are routinely blocked, that's an argument for adding a documented override mechanism to the workflow.
|
|
/build |
|
/ok to test 05a7c2c |
|
🤖 CI Triage Agent — TL;DR: PR #1856 added a new Full analysisSummary: Two distinct failures in build #3099: Root cause:
Implicated commit: [REDACTED:Hex High Entropy String] (branch File: Suggested fix:
Related: PR #1856 (this change); PR #1577 "Use liburing wrap" — the tarball-wrap pattern to follow
|
|
🤖 CI Triage Agent — TL;DR: The AWS (GPU-less) validation job "failed" 13 gtests that actually all SKIPPED — the new GDS tests probe backend availability by calling Full analysisSummary: Root cause: Not a functional test failure. The runner ( Implicated commit: The GDS test suite on this branch — File: Suggested fix: Make the availability probe log-clean on non-GPU hosts:
Related: PR #1856 (this change); PR #1909 "TEST/GTEST: Fail on unexpectedly skipped tests" and PR #1743 "TEST/GTEST: Run in single process" touch the same skip/exit-code handling. |
vvenkates27
left a comment
vvenkates27
left a comment
There was a problem hiding this comment.
Overall this is ready once CI passes.
@maheshrbapatu need a follow up PR for documentation after this merges.
Docs: Fern still treats GDS and GDS_MT as two separate backends and never mentions mode. Please match src/plugins/cuda_gds/README.md in fern/docs/pages/user-guide/backends/gds.md and gds-mt.md.
GDS defaults to batch; mode=mt selects the MT engine. thread_count applies only with mode=mt; batch knobs apply only with mode=batch.
GDS_MT stays a compatibility name for the MT engine (thread_count only). createBackend("GDS") / createBackend("GDS_MT") defaults are unchanged.
6 participants
What?
Consolidate the standalone
GDS_MTsources intosrc/plugins/cuda_gdswhile preserving both public backend names and exposing both transfer strategies through the primaryGDSname.GDSuses the cuFile batch engine by default and acceptsmode=batchormode=mtto select the batch or multi-threaded engine.GDS_MTcontinues to expose the multi-threaded engine as a compatibility backend name while downstream users migrate toGDSwithmode=mt.queryMem, validation, and request preparation.get_backend_options.Backend selection
GDSmode=batchGDSmode=mtGDS_MTWhy?
The two backends intentionally submit I/O differently, but the surrounding cuFile integration was duplicated across separate plugin directories. Keeping that code in sync made registration and lifetime fixes easy to apply to one backend but miss in the other.
This change gives the common behavior one implementation, exposes both strategies through
GDS, and retainsGDS_MTas a compatibility path.Closes #1855.
How?
nixlGdsEngineis now the abstract shared base.nixlGdsBatchEngineandnixlGdsMtEngineinherit from it and implement only their backend-specific preparation, posting, completion, and release behavior.Meson builds a shared internal GDS engine archive and two plugin entry points.
GDSlinks both concrete engines and selects one frommode;GDS_MTis a thin MT-only compatibility entry point. Dynamic and static discovery continue to expose both backend names. BecauseGDSnow supportsmode=mt, Taskflow is required when either entry point is built.The class and runtime flow diagrams are in the issue design comment.
Compatibility and scope
This PR preserves the backend names, plugin versions, supported memory types, path-mode behavior, GDS/GDS_MT mutual exclusion, and existing completion/release behavior. Existing
GDScalls withoutmodestill select the batch strategy, and existingGDS_MTcalls still select the Taskflow MT strategy.GDSadds the optionalmode=batch|mtselector and advertises the parameters for both strategies.GDS_MTis not removed in this PR. Consumers can migrate toGDSwithmode=mt; the standalone compatibility name can be removed in a later release after downstream users have migrated.Multi-CPU batch submission, DRAM-path changes, split-request
devPtr_offsethandling, and nonblocking release/cancellation redesign are intentionally left for follow-up changes.Validation
batch_limit=1to force four sub-batches.-0.461%across three pairs and needs more rounds before calling it a regression. An earlier same-file six-pair 32/64 MiB control also found no reproducible slowdown.git diff --checkpassed.One baseline GDS READ sweep hit the existing NVFS
nvfs_bio:209assertion; its immediate baseline retry passed, and no PR run failed.GB200 performance by I/O size
Test system: dual-socket NVIDIA Grace (144 Neoverse-V2 cores), 4 x GB200 GPUs, and 8 x Samsung 3.5 TB NVMe drives using ext4, with two drives mapped to each GPU. The software stack was Linux 6.14 aarch64 with 64 KiB pages, NVIDIA driver 580.105.08, CUDA 13.0, cuFile 1.19.0.76, and
nvidia_fs2.26.Sequential direct-VRAM I/O across 8 NVMe drives and 4 GB200 GPUs, with 16 entries/workers and a 16 GiB working set per drive. Throughput is aggregate GiB/s. The 1 MiB point used five paired base/PR1 rounds; 512 KiB and 16 MiB used three. GDSIO is a matched cross-run reference; baseline and PR1 were measured in paired, alternating-order rounds.
Only x6 and x0 are shown because they match the two NIXL execution models: GDS uses the cuFile batch API and is compared with x6
GPU_BATCH; GDS_MT issues synchronouscuFileRead/cuFileWriteoperations from multiple workers and is compared with x0GPUD. The other GDSIO modes use CPU/page-cache staging, asynchronous stream APIs, or vectored APIs that these NIXL backends do not use, so they are not like-for-like references.The read gap versus GDSIO shrinks as I/O size grows, while writes remain close to the matched GDSIO reference. PR1 follows the same size curve as baseline. All 8 consistency cases and 704 corrected performance processes passed. Full methodology, base values, pair ranges, CPU results, and retained-artifact details are in the independent GB200 report.
Summary by CodeRabbit
mode=batch|mt, with batch remaining the default.FILE_SEGregistrations against the same underlying file.Summary by CodeRabbit
New Features
Documentation
Tests