nixlbench: derive storage taxonomy from supported mem types - #1658
Open
benlwalker wants to merge 2 commits into
Open
benlwalker wants to merge 2 commits into
benlwalker wants to merge 2 commits into
Conversation
|
👋 Hi benlwalker! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
benlwalker
force-pushed
the
nixlbench-no-backend
branch
from
May 20, 2026 08:57
5cb9bc4 to
6900ad2
Compare
Contributor
|
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:
📝 WalkthroughWalkthroughBackend memory segment types are discovered at config-load time via a temporary NIXL agent, cached in ChangesBackend Memory Type Discovery and Storage Predicates
🎯 4 (Complex) | ⏱️ ~60 minutes sequenceDiagram
participant Config as xferBenchConfig
participant Agent as nixlAgent
participant Worker as xferBenchNixlWorker
participant Plugin as BackendPlugin
Config->>Agent: discoverPluginMems(backend)
Agent->>Plugin: getPluginParams()
Plugin-->>Agent: plugin params (mem types)
Agent-->>Config: populate backend_mems
Worker->>Agent: createBackend()
Agent->>Plugin: instantiate backend
Agent->>Worker: getBackendParams() (mem types)
Worker-->>Config: update backend_mems (if non-empty)
🚥 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp (1)
855-887:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftBlock-storage setup still switches on
backend == GUSLI.
prepareTransferDescriptors()anddeallocateMemory()now useisBlkStorageBackend(), but allocation and IOV exchange still use the hardcoded GUSLI check. A non-GUSLI backend that reportsBLK_SEGwill therefore be registered/exchanged through the FILE path here and then consumed as BLK later, which breaks transfer setup and cleanup.Also applies to: 1127-1155
🤖 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 855 - 887, The code currently branches on the literal GUSLI backend when building block IOVs and registering BLK_SEG descriptors (the block in nixl_worker.cpp that iterates gusli_devices, calls initBasicDescBlk, iovListToNixlRegDlist and agent->registerMem), causing non-GUSLI block backends to be misrouted; change the conditional to use the same predicate used elsewhere (xferBenchConfig::isBlkStorageBackend() or the equivalent helper) instead of XFERBENCH_BACKEND_GUSLI, and ensure you iterate the correct device vector for block backends (use the blk-device list used by prepareTransferDescriptors()/deallocateMemory()), call initBasicDescBlk for those devices, and register BLK_SEG via registerMem so allocation/IOV exchange and cleanup are consistent for all BLK backends.benchmark/nixlbench/src/utils/utils.cpp (1)
968-1046:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftBlock-backend consistency validation is still GUSLI-only.
This new
isStorageBackend()guard now routes everyBLK_SEGbackend into the storage readback path, but onlyXFERBENCH_BACKEND_GUSLIgets block-specific handling below. Any other block backend falls through topread(iov.devId, ...), wheredevIdis not guaranteed to be a host file descriptor.🤖 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/utils/utils.cpp` around lines 968 - 1046, The new xferBenchConfig::isStorageBackend() branch sends all block (BLK_SEG) backends into the storage readback path but only XFERBENCH_BACKEND_GUSLI has block-specific handling; other backends fall through to pread(iov.devId, ...) where devId may not be a host FD. Fix by adding a branch when xferBenchConfig::backend != XFERBENCH_BACKEND_GUSLI to either (a) resolve iov.devId to a host device path/FD like the gusli_devs lookup (use gusli_devs or equivalent mapping of device_id -> device_path and open that path with O_RDONLY/O_DIRECT) and call pread on the opened FD, or (b) explicitly error out with a clear message if no mapping exists; update the code paths that currently call pread(iov.devId, addr, len, iov.addr) to instead open the correct path and use the returned fd (and close it) so pread is always passed a valid FD.
🤖 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/utils/utils.cpp`:
- Around line 392-395: The cached backend_mems must be cleared before reloading
config so stale NIXL taxonomy doesn't persist; in parseConfig() reset or clear
the backend_mems container (e.g., call backend_mems.clear() or assign an empty
container) before any backend-specific discovery and before calling
discoverPluginMems(backend), so that when switching to an nvshmem config
isStorageBackend() will use fresh mems; update parseConfig() to perform this
reset at the start (or immediately before backend_mems is repopulated).
In `@benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp`:
- Around line 124-129: The constructor currently only checks
xferBenchConfig::backend_mems.empty() but later still uses a hardcoded else-if
chain against specific backend names (preserving the old allowlist), so update
the constructor logic that handles backend selection (look for variables
backend_name, xferBenchConfig::backend, and xferBenchConfig::backend_mems) to
stop using the hardcoded else-if chain and instead accept any backend present in
xferBenchConfig::backend_mems: replace the else-if block with a lookup/selection
that matches backend_name against keys or entries in backend_mems (or fall back
to the first available entry) and remove the exit(EXIT_FAILURE) for unknown
names so new aliases/backends in backend_mems are allowed. Ensure any subsequent
code uses the selected memory type from backend_mems rather than the removed
allowlist.
---
Outside diff comments:
In `@benchmark/nixlbench/src/utils/utils.cpp`:
- Around line 968-1046: The new xferBenchConfig::isStorageBackend() branch sends
all block (BLK_SEG) backends into the storage readback path but only
XFERBENCH_BACKEND_GUSLI has block-specific handling; other backends fall through
to pread(iov.devId, ...) where devId may not be a host FD. Fix by adding a
branch when xferBenchConfig::backend != XFERBENCH_BACKEND_GUSLI to either (a)
resolve iov.devId to a host device path/FD like the gusli_devs lookup (use
gusli_devs or equivalent mapping of device_id -> device_path and open that path
with O_RDONLY/O_DIRECT) and call pread on the opened FD, or (b) explicitly error
out with a clear message if no mapping exists; update the code paths that
currently call pread(iov.devId, addr, len, iov.addr) to instead open the correct
path and use the returned fd (and close it) so pread is always passed a valid
FD.
In `@benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp`:
- Around line 855-887: The code currently branches on the literal GUSLI backend
when building block IOVs and registering BLK_SEG descriptors (the block in
nixl_worker.cpp that iterates gusli_devices, calls initBasicDescBlk,
iovListToNixlRegDlist and agent->registerMem), causing non-GUSLI block backends
to be misrouted; change the conditional to use the same predicate used elsewhere
(xferBenchConfig::isBlkStorageBackend() or the equivalent helper) instead of
XFERBENCH_BACKEND_GUSLI, and ensure you iterate the correct device vector for
block backends (use the blk-device list used by
prepareTransferDescriptors()/deallocateMemory()), call initBasicDescBlk for
those devices, and register BLK_SEG via registerMem so allocation/IOV exchange
and cleanup are consistent for all BLK backends.
🪄 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: 9715946f-a584-4a57-8a9c-72e9dda445b5
📒 Files selected for processing (3)
benchmark/nixlbench/src/utils/utils.cppbenchmark/nixlbench/src/utils/utils.hbenchmark/nixlbench/src/worker/nixl/nixl_worker.cpp
Contributor
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)
benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp (1)
1029-1045:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
BLK_SEGhandling is only half-converted to the new taxonomy.These paths now treat every
BLK_SEGbackend generically, but setup is still name-gated onbackend == GUSLIinallocateMemory()andexchangeIOV()(Lines 855-886 and Lines 1133-1155). For any other block backend or alias, registration still goes through theFILE_SEGpath while transfer prep and teardown useBLK_SEG, which will misclassify descriptors and can break deregistration/transfer creation.Suggested direction
- } else if (XFERBENCH_BACKEND_GUSLI == xferBenchConfig::backend) { + } else if (xferBenchConfig::isBlkStorageBackend()) { // block-storage backend uses block device descriptors ... - } else if (xferBenchConfig::isStorageBackend()) { + } else if (xferBenchConfig::isStorageBackend()) { // file-storage backend ...Apply the same split in
exchangeIOV()and the DRAMmem_dev_idselection so block-storage backends stay on theBLK_SEGpath end to end, while keeping the truly GUSLI-specific parameter parsing/workarounds inside the GUSLI-only branches.Also applies to: 1237-1243
🤖 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 1029 - 1045, The BLK_SEG path is only partially applied: registration/deregistration is still using FILE_SEG for non-GUSLI block backends—fix allocateMemory(), exchangeIOV(), and the DRAM mem_dev_id selection so that xferBenchConfig::isBlkStorageBackend() (not just backend == GUSLI) uses the BLK_SEG branch end-to-end; ensure functions referenced (allocateMemory, exchangeIOV) create/prepare descriptors and call iovListToNixlRegDlist with BLK_SEG for all block-storage backends, keep GUSLI-specific parameter parsing only inside backend == GUSLI branches, and make cleanupBasicDescBlk/cleanupBasicDescFile and deregisterMem calls use the matching segment type (BLK_SEG vs FILE_SEG) consistently.
🤖 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/utils/utils.cpp`:
- Around line 778-800: The three predicate methods
xferBenchConfig::isStorageBackend, xferBenchConfig::isObjStorageBackend, and
xferBenchConfig::isBlkStorageBackend use single-line if statements without
braces which violates clang-format/CI rules; update each method so the if bodies
are wrapped in braces (i.e., replace "if (m == X) return true;" with a braced
block that contains the return statement) and keep the rest of the logic
unchanged.
---
Outside diff comments:
In `@benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp`:
- Around line 1029-1045: The BLK_SEG path is only partially applied:
registration/deregistration is still using FILE_SEG for non-GUSLI block
backends—fix allocateMemory(), exchangeIOV(), and the DRAM mem_dev_id selection
so that xferBenchConfig::isBlkStorageBackend() (not just backend == GUSLI) uses
the BLK_SEG branch end-to-end; ensure functions referenced (allocateMemory,
exchangeIOV) create/prepare descriptors and call iovListToNixlRegDlist with
BLK_SEG for all block-storage backends, keep GUSLI-specific parameter parsing
only inside backend == GUSLI branches, and make
cleanupBasicDescBlk/cleanupBasicDescFile and deregisterMem calls use the
matching segment type (BLK_SEG vs FILE_SEG) consistently.
🪄 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: 120020dc-7352-4137-bd43-23f9f5badb3a
📒 Files selected for processing (3)
benchmark/nixlbench/src/utils/utils.cppbenchmark/nixlbench/src/utils/utils.hbenchmark/nixlbench/src/worker/nixl/nixl_worker.cpp
benlwalker
force-pushed
the
nixlbench-no-backend
branch
from
May 20, 2026 09:28
6900ad2 to
07553ad
Compare
Contributor
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)
1029-1036:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
BLK_SEGsupport is only partially generalized.These new
isBlkStorageBackend()branches assume any backend reportingBLK_SEGshould use block descriptors, butallocateMemory()andexchangeIOV()still special-case onlyXFERBENCH_BACKEND_GUSLI. For any non-GUSLI block backend, the worker will still build/registerFILE_SEGpaths there, then constructBLK_SEGtransfer descriptors here and deregisterBLK_SEGon teardown. That mismatch will break the generic block-storage path this PR introduces.Also applies to: 1237-1241
🤖 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 1029 - 1036, The code uses isBlkStorageBackend() to deregister BLK_SEG descriptors (cleanupBasicDescBlk / iovListToNixlRegDlist / deregisterMem) but allocateMemory() and exchangeIOV() still only special-case XFERBENCH_BACKEND_GUSLI, causing a FILE_SEG vs BLK_SEG mismatch; update allocateMemory() and exchangeIOV() to detect isBlkStorageBackend() (in addition to GUSLI) and allocate/register block descriptors (BLK_SEG) instead of file descriptors so the descriptor type used when building/registering memory matches the type used in cleanupBasicDescBlk(), iovListToNixlRegDlist(), and the deregisterMem call. Ensure all paths that create/track iovs use the same segment kind (BLK_SEG) for non-GUSLI block backends and adjust any helper logic that currently assumes FILE_SEG.
🤖 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 `@benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp`:
- Around line 1029-1036: The code uses isBlkStorageBackend() to deregister
BLK_SEG descriptors (cleanupBasicDescBlk / iovListToNixlRegDlist /
deregisterMem) but allocateMemory() and exchangeIOV() still only special-case
XFERBENCH_BACKEND_GUSLI, causing a FILE_SEG vs BLK_SEG mismatch; update
allocateMemory() and exchangeIOV() to detect isBlkStorageBackend() (in addition
to GUSLI) and allocate/register block descriptors (BLK_SEG) instead of file
descriptors so the descriptor type used when building/registering memory matches
the type used in cleanupBasicDescBlk(), iovListToNixlRegDlist(), and the
deregisterMem call. Ensure all paths that create/track iovs use the same segment
kind (BLK_SEG) for non-GUSLI block backends and adjust any helper logic that
currently assumes FILE_SEG.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c1da5af0-8387-4b2c-81c2-209fc85ca9e0
📒 Files selected for processing (3)
benchmark/nixlbench/src/utils/utils.cppbenchmark/nixlbench/src/utils/utils.hbenchmark/nixlbench/src/worker/nixl/nixl_worker.cpp
Contributor
|
/ok to test f678c24 |
Contributor
|
/build |
Contributor
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)
857-889:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftBlock-storage routing still depends on the literal
GUSLIbackend name.These branches are still deciding the block-storage path by
xferBenchConfig::backend == XFERBENCH_BACKEND_GUSLIinstead of the discovered taxonomy. Even if the constructor is relaxed, a backend or alias that reportsBLK_SEGwill still fall into the generic file path here and build the wrong remote descriptors/offset handling.Also applies to: 1135-1157
🤖 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 857 - 889, The code routes block-storage logic based on the literal xferBenchConfig::backend == XFERBENCH_BACKEND_GUSLI; change it to detect block-segment backends from device metadata instead and build descriptors per-device. Specifically, replace the top-level branch that checks XFERBENCH_BACKEND_GUSLI with a check that inspects each gusli_devices entry (e.g., device.device_type or the device's segment/type field that indicates BLK_SEG) and only call initBasicDescBlk(device_id, dev_offset, buffer_size) for devices that report block segments; keep the file-consistency check (ensureFileHasConsistencyData) only for devices labeled as file ('F'), and use the corresponding file descriptor init path for those. Apply the same device-type-driven branching at the other occurrence noted (around the 1135–1157 region) so any backend/alias reporting BLK_SEG uses the block path instead of relying on the literal GUSLI backend name.
♻️ Duplicate comments (2)
benchmark/nixlbench/src/utils/utils.cpp (1)
354-356:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReset
backend_memsbefore every config reload.
backend_memsis process-wide static state, but it is only repopulated inside the NIXL branch. IfloadParams()runs again for annvshmemconfig, the previous NIXL taxonomy survives andisStorageBackend()will keep reading stale mem types.Suggested fix
int xferBenchConfig::loadParams(void) { std::unique_ptr<toml::table> tbl; + backend_mems.clear(); if (!FLAGS_config_file.empty()) {🤖 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/utils/utils.cpp` around lines 354 - 356, Clear the process-global backend_mems before repopulating in xferBenchConfig::loadParams so stale NIXL entries don't survive an nvshmem config; specifically, at the start of loadParams (or before the NIXL handling branch) call the appropriate reset/clear on backend_mems (e.g., backend_mems.clear() or reassign an empty container) so subsequent isStorageBackend() sees only the current config's mem types and then continue to repopulate backend_mems inside the existing NIXL branch as before.benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp (1)
135-310:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe constructor still preserves the backend allowlist.
backend_mems.empty()already proves discovery found the plugin, but this exact-name chain still aborts on any alias or new backend name that reports supported mem types successfully. That keeps the old allowlist behavior the PR is trying to remove.Suggested fix
- } else { - std::cerr << "Unsupported NIXLBench backend: " << xferBenchConfig::backend << std::endl; - exit(EXIT_FAILURE); - } + } else { + std::cout << "Using default parameters for backend: " << xferBenchConfig::backend + << std::endl; + }🤖 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 135 - 310, The constructor still enforces an exact-name allowlist instead of trusting discovery — stop aborting based on the name chain and accept any backend that reports supported memory types; specifically, in the constructor (where xferBenchConfig::backend is compared to XFERBENCH_BACKEND_* constants) replace the exact-name gating with a check of backend_mems.empty() (or equivalent discovery result) so that if backend_mems is non-empty the backend is accepted even if the name is an alias or new name; update the branches that currently rely on name equality to only apply backend-specific param setup when appropriate but do not exit/deny when discovery succeeded.
🤖 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 `@benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp`:
- Around line 857-889: The code routes block-storage logic based on the literal
xferBenchConfig::backend == XFERBENCH_BACKEND_GUSLI; change it to detect
block-segment backends from device metadata instead and build descriptors
per-device. Specifically, replace the top-level branch that checks
XFERBENCH_BACKEND_GUSLI with a check that inspects each gusli_devices entry
(e.g., device.device_type or the device's segment/type field that indicates
BLK_SEG) and only call initBasicDescBlk(device_id, dev_offset, buffer_size) for
devices that report block segments; keep the file-consistency check
(ensureFileHasConsistencyData) only for devices labeled as file ('F'), and use
the corresponding file descriptor init path for those. Apply the same
device-type-driven branching at the other occurrence noted (around the 1135–1157
region) so any backend/alias reporting BLK_SEG uses the block path instead of
relying on the literal GUSLI backend name.
---
Duplicate comments:
In `@benchmark/nixlbench/src/utils/utils.cpp`:
- Around line 354-356: Clear the process-global backend_mems before repopulating
in xferBenchConfig::loadParams so stale NIXL entries don't survive an nvshmem
config; specifically, at the start of loadParams (or before the NIXL handling
branch) call the appropriate reset/clear on backend_mems (e.g.,
backend_mems.clear() or reassign an empty container) so subsequent
isStorageBackend() sees only the current config's mem types and then continue to
repopulate backend_mems inside the existing NIXL branch as before.
In `@benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp`:
- Around line 135-310: The constructor still enforces an exact-name allowlist
instead of trusting discovery — stop aborting based on the name chain and accept
any backend that reports supported memory types; specifically, in the
constructor (where xferBenchConfig::backend is compared to XFERBENCH_BACKEND_*
constants) replace the exact-name gating with a check of backend_mems.empty()
(or equivalent discovery result) so that if backend_mems is non-empty the
backend is accepted even if the name is an alias or new name; update the
branches that currently rely on name equality to only apply backend-specific
param setup when appropriate but do not exit/deny when discovery succeeded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2841dfc1-dc6c-4702-9543-df7d1cf8c230
📒 Files selected for processing (2)
benchmark/nixlbench/src/utils/utils.cppbenchmark/nixlbench/src/worker/nixl/nixl_worker.cpp
benlwalker
force-pushed
the
nixlbench-no-backend
branch
from
May 21, 2026 12:13
f678c24 to
742cb96
Compare
Contributor
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)
307-310:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe hardcoded backend else-if chain still exits for unknown backends.
A past review suggested changing this
elseblock to allow new backends to proceed with default parameters rather than exiting. The current code still exits for any backend not explicitly listed in the if-else chain (lines 135-306), which preserves the allowlist behavior the PR aims to remove.If a new backend plugin is discovered (passes the
backend_mems.empty()check at line 127) but isn't in the if-else chain, it will hit this exit path despite having valid memory types.Suggested fix
} else { - std::cerr << "Unsupported NIXLBench backend: " << xferBenchConfig::backend << std::endl; - exit(EXIT_FAILURE); + std::cout << "Using default parameters for backend: " << xferBenchConfig::backend + << std::endl; }🤖 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 307 - 310, The current final else branch that prints "Unsupported NIXLBench backend" and calls exit(EXIT_FAILURE) prevents new valid backend plugins from running when they aren't enumerated in the explicit if/else chain; replace that exit path in the block handling xferBenchConfig::backend with a non-fatal fallback: log a warning that the backend is unrecognized but detected, do not call exit, and allow execution to continue using the discovered memory types (the backend_mems list) and default transfer parameters; specifically update the else handling around xferBenchConfig::backend so it no longer terminates the process but instead proceeds with default configuration for unknown backends.
🤖 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 `@benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp`:
- Around line 307-310: The current final else branch that prints "Unsupported
NIXLBench backend" and calls exit(EXIT_FAILURE) prevents new valid backend
plugins from running when they aren't enumerated in the explicit if/else chain;
replace that exit path in the block handling xferBenchConfig::backend with a
non-fatal fallback: log a warning that the backend is unrecognized but detected,
do not call exit, and allow execution to continue using the discovered memory
types (the backend_mems list) and default transfer parameters; specifically
update the else handling around xferBenchConfig::backend so it no longer
terminates the process but instead proceeds with default configuration for
unknown backends.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1f5726fb-e7d6-415c-85a9-7e5ef534699e
📒 Files selected for processing (3)
benchmark/nixlbench/src/utils/utils.cppbenchmark/nixlbench/src/utils/utils.hbenchmark/nixlbench/src/worker/nixl/nixl_worker.cpp
Contributor
|
/build |
ofer
reviewed
May 24, 2026
Contributor
|
/build |
1 similar comment
Contributor
|
/build |
iyastreb
reviewed
May 28, 2026
ofer
force-pushed
the
nixlbench-no-backend
branch
from
May 31, 2026 07:40
8316f12 to
0d30f56
Compare
Contributor
|
/ok to test 0d30f56 |
ofer
force-pushed
the
nixlbench-no-backend
branch
from
June 1, 2026 07:00
0d30f56 to
022e916
Compare
Contributor
|
/ok to test 022e916 |
ofer
force-pushed
the
nixlbench-no-backend
branch
from
June 2, 2026 13:40
022e916 to
3cd91ba
Compare
Contributor
|
/ok to test 3cd91ba |
benlwalker
force-pushed
the
nixlbench-no-backend
branch
from
June 4, 2026 16:21
3cd91ba to
19d7a5c
Compare
iyastreb
previously approved these changes
Jun 5, 2026
isStorageBackend() and isObjStorageBackend() compared the configured
backend string against a hardcoded set of names, and nixl_worker.cpp
maintained a separate allowlist of "supported" backend strings. Adding
a new backend or registering an existing plugin under a different name
required touching both lists in the bench.
Cache nixl_mem_list_t for the configured backend in xferBenchConfig.
Populate it pre-instantiation via a transient discovery nixlAgent and
getPluginParams (so the predicate works before the worker exists, for
the runtime / role / metadata-exchange gates), then refresh it from
getBackendParams after createBackend so plugins that add mem types at
runtime (LIBFABRIC adds VRAM_SEG when CUDA HMEM is detected) are
reflected. Rewrite the predicates against the cached mem set and add
isBlkStorageBackend().
Replace the explicit backend allowlist in xferBenchNixlWorker with a
non-empty-mems check. Drop the GPUNETIO special case in
checkConsistency: GPUNETIO reports {DRAM_SEG, VRAM_SEG} so the network
branch handles it correctly for every op/seg combination, while the
storage branch's WRITE path called pread(iov.devId, ...) -- harmless
for file backends where devId is the fd, but undefined for GPUNETIO
where devId is a CUDA device id.
Sites that operate on the GUSLI-specific gusli_devices structure
(parsed from per-backend GUSLI config) and the GUSLI-only
ensureFileHasConsistencyData helper remain keyed on the GUSLI backend
name. Per-backend parameter parsing and the GUSLI recreate_xfer /
auto direct-I/O workarounds are unchanged.
Signed-off-by: Ben Walker <ben@nvidia.com>
benlwalker
force-pushed
the
nixlbench-no-backend
branch
from
June 16, 2026 17:59
19d7a5c to
5b0f587
Compare
iyastreb
approved these changes
Jun 17, 2026
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
5 participants
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?
Re-implement isStorageBackend() and isObjStorageBackend() by querying supported memory types rather than a hard coded list of backend names.
Why?
Long term, we'd like to not require nixlbench changes each time a new backend is added. This is one small piece of that puzzle.
How?
isStorageBackend() and isObjStorageBackend() compared the configured backend string against a hardcoded set of names, and nixl_worker.cpp maintained a separate allowlist of "supported" backend strings. Adding a new backend or registering an existing plugin under a different name required touching both lists in the bench.
Cache nixl_mem_list_t for the configured backend in xferBenchConfig. Populate it pre-instantiation via a transient discovery nixlAgent and getPluginParams (so the predicate works before the worker exists, for the runtime / role / metadata-exchange gates), then refresh it from getBackendParams after createBackend so plugins that add mem types at runtime (LIBFABRIC adds VRAM_SEG when CUDA HMEM is detected) are reflected. Rewrite the predicates against the cached mem set and add isBlkStorageBackend().
Summary by CodeRabbit