Nixlbench improvements - #1474
Merged
Merged
Conversation
benlwalker
requested review from
aranadive,
brminich and
ovidiusm
as code owners
March 30, 2026 16:40
|
👋 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. 🚀 |
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:
📝 WalkthroughWalkthroughAdds hugepage support, per-iteration optional memory (de)registration, and configurable pipeline depth; rounds buffer/stride to 2MB when enabled, changes allocation/free to hugepage-aware mmap/munmap, and introduces a pipelined transfer path that issues/resubmits multiple in-flight requests. Changes
Sequence Diagram(s)sequenceDiagram
participant Worker
participant Kernel
participant Reg as MemRegistry
participant Backend as BackendEngine
Worker->>Kernel: mmap hugepage allocation (MAP_HUGETLB, 2MB)
Kernel-->>Worker: mapped addr or error
alt pipeline_depth > 1
loop for each slot i (<= depth)
Worker->>Reg: (optional) registerIterationMem(slot i, iov)
Reg-->>Worker: reg handle
Worker->>Backend: post request(slot i)
Backend-->>Worker: accepted
end
loop poll/resubmit
Worker->>Backend: getXferStatus
Backend-->>Worker: completion / in-flight status
alt resubmit needed and recreate_xfer
Worker->>Reg: (optional) deregisterIterationMem(slot i)
Reg-->>Worker: deregistered
Worker->>Reg: (optional) registerIterationMem(slot i)
Reg-->>Worker: reg handle
Worker->>Backend: repost request(slot i)
Backend-->>Worker: accepted
end
end
else
Worker->>Reg: (optional) registerIterationMem
Reg-->>Worker: reg handle
Worker->>Backend: post request
Backend-->>Worker: completion
end
Worker->>Reg: deregister remaining slots (if registered)
Reg-->>Worker: done
Worker->>Kernel: freeXferMemory(mapped addr, size)
Kernel-->>Worker: unmapped
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benchmark/nixlbench/src/main.cpp`:
- Around line 118-124: Detect and prevent silent truncation by validating that
the descriptor count divides evenly by the pipeline depth before using integer
division in execTransferPipelined: compute depth (min(batch_queue_depth,
num_iter)) and check that local_iov.size() % depth == 0 (or that
entries_per_slot computed from local_iov.size() / depth would not drop
descriptors); if not, either return an error/log and abort with a clear message
mentioning batch_queue_depth, num_iter, local_iov.size(), and depth, or adjust
the depth/iteration logic to evenly distribute the remainder (e.g., reduce
batch_queue_depth or redistribute descriptors) so
createTransferDescLists/effective_batch and subsequent entries_per_slot use a
safe, non-truncating configuration.
In `@benchmark/nixlbench/src/utils/utils.cpp`:
- Around line 85-86: In loadParams(), the new gflags (reregister_mem,
batch_queue_depth, use_hugepages) are not being loaded from the config file and
batch_queue_depth is not validated; update loadParams() to assign reregister_mem
= NB_ARG(reregister_mem), use_hugepages = NB_ARG(use_hugepages), and
batch_queue_depth = NB_ARG(batch_queue_depth) (place alongside the existing
recreate_xfer handling), and add a validation check that batch_queue_depth >= 1
that prints an error and returns -1 on failure.
In `@benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp`:
- Around line 36-39: Move the _GNU_SOURCE definition so it precedes all system
headers in nixl_worker.cpp: relocate the existing `#ifndef` _GNU_SOURCE / `#define`
_GNU_SOURCE / `#endif` block to the very top of the file before any includes
(e.g., before <fcntl.h>, <filesystem>, etc.), or alternatively add -D_GNU_SOURCE
to the project's compiler flags; ensure the symbol _GNU_SOURCE is defined prior
to including headers like <sys/mman.h> so the feature test macro takes effect.
- Line 1549: The call to agent->postXferReq(slots[s].req) (and the other call at
the second occurrence) ignores its return value; modify the code that invokes
postXferReq to capture its return status, check for failure, and handle errors
appropriately (e.g., log an error including the request identifier, clean up or
free slots[s].req, and return or retry as the surrounding function's error
policy dictates). Locate both invocations of postXferReq and add consistent
error handling: assign the result to a status variable, test for non-success,
and perform required cleanup/propagation rather than proceeding silently.
- Around line 1479-1499: The current distribution computes entries_per_slot =
local_iov.size() / depth which silently drops remainder descriptors; change the
logic in the slot population code (symbols: entries_per_slot, local_iov,
remote_iov, slots, depth) to account for remainder: compute size_t base =
local_iov.size() / depth and size_t rem = local_iov.size() % depth, then for
each slot s assign base + (s < rem ? 1 : 0) entries (adjust the begin/end
offsets accordingly) so all descriptors are distributed (alternatively, if you
prefer strict validation, detect rem != 0 and return an error instead of
proceeding).
- Around line 1314-1362: getRemoteSegType() is declared but not implemented; add
a definition that returns the correct nixl_mem_t segment type for remote
registrations using the same backend-conditioned logic as used for remote
descriptor selection (mirror the pattern used for GET_SEG_TYPE(true) vs
backend-based choice). Implement getRemoteSegType() to inspect the backend or
configuration used elsewhere (same criteria as the remote descriptor logic),
return the appropriate nixl_mem_t value, and ensure registerIterationMem and
deregisterIterationMem calls to getRemoteSegType() receive the correct segment
type for remote_iov before building nixl_reg_dlist_t.
🪄 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: ff5372c9-1990-4a4c-9ff7-e7af03893e67
📒 Files selected for processing (4)
benchmark/nixlbench/src/main.cppbenchmark/nixlbench/src/utils/utils.cppbenchmark/nixlbench/src/utils/utils.hbenchmark/nixlbench/src/worker/nixl/nixl_worker.cpp
benlwalker
force-pushed
the
nixlbench-improvements
branch
from
March 30, 2026 17:49
83051f4 to
a47797b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benchmark/nixlbench/src/main.cpp`:
- Line 35: When use_hugepages is enabled, the code currently rounds stride to
HUGEPAGE_SIZE but does not ensure total_buffer_size (iov.len / buffer_size) is a
multiple of HUGEPAGE_SIZE, so offsets computed with stride can be
non-2MB-aligned; add a validation in the initialization path (where
use_hugepages, stride, and total_buffer_size are set) that checks
total_buffer_size % HUGEPAGE_SIZE == 0 and fail fast (log error and exit/return)
if not, or alternatively document in input validation that total_buffer_size
must be 2MB-aligned; reference HUGEPAGE_SIZE, use_hugepages, stride,
total_buffer_size and the iov.len/ buffer_size calculation when adding the check
and error message.
In `@benchmark/nixlbench/src/utils/utils.cpp`:
- Line 37: Remove the unused header include <cassert> from utils.cpp since
assert is not used anywhere in this file; update the top-of-file includes (where
<cassert> currently appears) to delete that line so only necessary headers
remain (e.g., retain other includes but remove the <cassert> entry).
In `@benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp`:
- Around line 1575-1580: The code calls registerIterationMem unconditionally
after deregisterIterationMem; change this so you capture and check the result of
deregisterIterationMem (when reregister is true) and only call
registerIterationMem if deregisterIterationMem returned success; if
deregisterIterationMem fails, log/propagate the error and avoid calling
registerIterationMem to prevent leaks/inconsistent state. Update the block
around deregisterIterationMem(agent, slots[s].local_iov, slots[s].remote_iov,
backend_engine) and registerIterationMem(...) to check the deregister return
code and handle failure (e.g., return error or skip re-registration) using the
same agent/slots[s]/backend_engine context.
🪄 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: 68a195fe-fcd3-4b3f-b9d6-363f9b231050
📒 Files selected for processing (4)
benchmark/nixlbench/src/main.cppbenchmark/nixlbench/src/utils/utils.cppbenchmark/nixlbench/src/utils/utils.hbenchmark/nixlbench/src/worker/nixl/nixl_worker.cpp
Contributor
|
/ok to test a47797b |
Contributor
|
/build |
benlwalker
force-pushed
the
nixlbench-improvements
branch
from
March 30, 2026 20:07
a47797b to
4b9dfce
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
benchmark/nixlbench/src/main.cpp (1)
67-71:⚠️ Potential issue | 🟡 MinorStride alignment relies on buffer_size being 2MB-aligned.
The stride rounding ensures
strideis a 2MB multiple, but the comment's claim aboutdev_offsetbeing 2MB-aligned only holds wheniov.len(buffer_size) is also 2MB-aligned. Whiletotal_buffer_sizeis validated to be 2MB-aligned inutils.cpp,buffer_size = total_buffer_size / (num_devices * num_threads)may not be 2MB-aligned if the divisor doesn't divide evenly into the 2MB granularity.Consider adding validation that
buffer_sizeis 2MB-aligned when hugepages are enabled, or update the comment to clarify the constraint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmark/nixlbench/src/main.cpp` around lines 67 - 71, The comment and stride adjustment assume iov.len (buffer_size) is 2MB-aligned but buffer_size = total_buffer_size / (num_devices * num_threads) may not preserve that alignment; update main.cpp to validate when xferBenchConfig::use_hugepages is true that buffer_size is a multiple of HUGEPAGE_SIZE (or that total_buffer_size is divisible by (num_devices * num_threads * HUGEPAGE_SIZE)) and fail/print a clear error if not, or alternatively change the comment to explicitly state the precondition; reference xferBenchConfig::use_hugepages, stride, HUGEPAGE_SIZE, buffer_size, total_buffer_size, num_devices and num_threads (and note utils.cpp already validates total_buffer_size) so the check or comment clarifies the required 2MB alignment.benchmark/nixlbench/src/utils/utils.cpp (1)
461-468:⚠️ Potential issue | 🟠 MajorAdd validation for
batch_queue_depth >= 1.The hugepage validation is correct. However,
batch_queue_depthis not validated to be >= 1. If a user specifiesbatch_queue_depth=0, it causes division by zero inexecTransferPipelinedat line 1480 ofnixl_worker.cpp:const size_t entries_per_slot = local_iov.size() / depth;Where
depth = std::min(batch_queue_depth, num_iter).🔧 Proposed fix
reregister_mem = NB_ARG(reregister_mem); batch_queue_depth = NB_ARG(batch_queue_depth); + if (batch_queue_depth < 1) { + std::cerr << "Error: --batch_queue_depth must be >= 1 (got " << batch_queue_depth << ")" + << std::endl; + return -1; + } use_hugepages = NB_ARG(use_hugepages);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmark/nixlbench/src/utils/utils.cpp` around lines 461 - 468, Validate that batch_queue_depth is >= 1 after it's parsed (where batch_queue_depth = NB_ARG(batch_queue_depth)); if it is < 1, log an error and return EXIT_FAILURE. Reference the variable batch_queue_depth and the function execTransferPipelined (which computes depth = std::min(batch_queue_depth, num_iter) and uses it to divide local_iov.size()), so ensure the check prevents depth from ever being zero by rejecting values < 1. Add the check alongside the existing hugepage/total_buffer_size validation block so invalid inputs fail fast.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp`:
- Around line 1311-1353: The remote descriptors in registerIterationMem and
deregisterIterationMem are using GET_SEG_TYPE(false) (target_seg_type) which is
wrong for storage backends; update these functions to pick the remote segment
type the same way prepareTransferDescriptors does (FILE_SEG/BLK_SEG/OBJ_SEG for
storage backends). Implement a small helper (e.g., getSegTypeForRemote or
compute_remote_seg_type) that accepts the remote flag and the backend_engine (or
inspects the same backend properties used in prepareTransferDescriptors), use
that helper when constructing nixl_reg_dlist_t remote_reg and before calling
iovListToNixlRegDlist, and apply the same change to both registerIterationMem
and deregisterIterationMem so remote registrations/deregistrations use the
correct segment type.
---
Duplicate comments:
In `@benchmark/nixlbench/src/main.cpp`:
- Around line 67-71: The comment and stride adjustment assume iov.len
(buffer_size) is 2MB-aligned but buffer_size = total_buffer_size / (num_devices
* num_threads) may not preserve that alignment; update main.cpp to validate when
xferBenchConfig::use_hugepages is true that buffer_size is a multiple of
HUGEPAGE_SIZE (or that total_buffer_size is divisible by (num_devices *
num_threads * HUGEPAGE_SIZE)) and fail/print a clear error if not, or
alternatively change the comment to explicitly state the precondition; reference
xferBenchConfig::use_hugepages, stride, HUGEPAGE_SIZE, buffer_size,
total_buffer_size, num_devices and num_threads (and note utils.cpp already
validates total_buffer_size) so the check or comment clarifies the required 2MB
alignment.
In `@benchmark/nixlbench/src/utils/utils.cpp`:
- Around line 461-468: Validate that batch_queue_depth is >= 1 after it's parsed
(where batch_queue_depth = NB_ARG(batch_queue_depth)); if it is < 1, log an
error and return EXIT_FAILURE. Reference the variable batch_queue_depth and the
function execTransferPipelined (which computes depth =
std::min(batch_queue_depth, num_iter) and uses it to divide local_iov.size()),
so ensure the check prevents depth from ever being zero by rejecting values < 1.
Add the check alongside the existing hugepage/total_buffer_size validation block
so invalid inputs fail fast.
🪄 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: a87be92a-f2af-46fa-b9e5-4c430b6549c5
📒 Files selected for processing (4)
benchmark/nixlbench/src/main.cppbenchmark/nixlbench/src/utils/utils.cppbenchmark/nixlbench/src/utils/utils.hbenchmark/nixlbench/src/worker/nixl/nixl_worker.cpp
brminich
reviewed
Apr 1, 2026
benlwalker
force-pushed
the
nixlbench-improvements
branch
from
April 1, 2026 21:24
4b9dfce to
a757118
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp (1)
1308-1350:⚠️ Potential issue | 🟠 MajorUse the backend-specific remote segment type in per-iteration registration.
Line 1322 and Line 1344 still build
remote_regwithGET_SEG_TYPE(false).prepareTransferDescriptors()at Line 1296-Line 1302 already shows that storage backends needOBJ_SEG,BLK_SEG, orFILE_SEG; with--reregister_mem, these helpers will register/deregister the wrong segment type on storage backends.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp` around lines 1308 - 1350, registerIterationMem and deregisterIterationMem currently construct remote_reg with GET_SEG_TYPE(false) which yields the wrong segment type for storage backends; update both functions to compute the remote segment type from the backend (the same logic used in prepareTransferDescriptors) and use that value when constructing nixl_reg_dlist_t remote_reg and when calling iovListToNixlRegDlist, so storage backends get OBJ_SEG/BLK_SEG/FILE_SEG as appropriate (i.e., derive the segment type from nixlBackendH *backend_engine and replace GET_SEG_TYPE(false) with that backend-specific segment type).benchmark/nixlbench/src/utils/utils.cpp (1)
461-469:⚠️ Potential issue | 🟠 MajorAlign the per-thread/device slice, not just the aggregate buffer.
Line 464 only rounds
total_buffer_size.benchmark/nixlbench/src/main.cppLine 41 later turns that into each IOV’slen, and Line 88 computes offsets with(i * stride) % iov.len; if a slice ends up as 9 MiB, that modulo still yields 1 MiB offsets even though the total was 2 MiB-aligned. Hugepage mode still hits non-2 MiB-aligned addresses unless the actual per-slice buffer size is validated or rounded too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmark/nixlbench/src/utils/utils.cpp` around lines 461 - 469, The current code only rounds total_buffer_size to HUGEPAGE_SIZE, but main.cpp uses that to set each IOV's len (iov.len) and computes offsets with (i * stride) % iov.len, so you must align the per-slice (per-IOV) size too: compute the per_slice = total_buffer_size / num_iovs (or per-thread/per-device count used when building IOVs), round per_slice up to a multiple of HUGEPAGE_SIZE, then set total_buffer_size = aligned_per_slice * num_iovs (and update any variable used for iov.len/stride accordingly) so that iov.len is hugepage-aligned and (i * stride) % iov.len cannot land at non-2MB boundaries; adjust the logic around total_buffer_size, HUGEPAGE_SIZE, and the code paths that assign iov.len/stride to use the aligned per-slice value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benchmark/nixlbench/src/main.cpp`:
- Around line 117-119: The code builds descriptor lists using a static
effective_batch = batch_size * xferBenchConfig::batch_queue_depth which can
exceed the actual per-phase pipeline depth used by execTransferPipelined (which
uses min(batch_queue_depth, num_iter)); update the logic so
createTransferDescLists uses the runtime per-phase depth (compute runtime_depth
= min(xferBenchConfig::batch_queue_depth, num_iter_for_this_phase) and use
effective_batch = batch_size * runtime_depth) or move the call to
createTransferDescLists into the per-phase loop where execTransferPipelined is
invoked (passing runtime_depth) so descriptors, divisibility checks, and
reported stats (batch_size) all match the actual pipeline depth; adjust
references to effective_batch, local_trans_lists and createTransferDescLists
accordingly.
In `@benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp`:
- Around line 1542-1552: The pipeline path currently records only aggregate and
shared timing samples (using timer.lap() and a single prepare_duration) so
Prep/Post/Tx latencies are inaccurate when batch_queue_depth > 1; modify the
logic around postXferReq, the slot lifecycle, and completion handling to record
per-request timings: add per-slot timestamp fields (e.g., slots[s].prep_ts,
slots[s].post_ts, slots[s].tx_ts), capture prep_ts before posting, set post_ts
immediately after a successful postXferReq in the post path (where
slots[s].in_flight is set), and on completion compute per-slot durations
(post_duration = post_ts - prep_ts, tx_duration = completion_ts - post_ts) and
add those to thread_stats.prepare_duration / post_duration / transfer_duration
instead of using the shared timer.lap(); ensure the same pattern is applied at
the places called out (around postXferReq, when marking in_flight, and in the
completion handler that currently uses timer.lap()).
- Around line 366-383: The buffers returned by allocateXferMemory() inside
ensureFileHasConsistencyData() may be mmap-backed when
xferBenchConfig::use_hugepages is enabled, so replace the three direct free(...)
calls with freeXferMemory(...) to properly unmap or free them: update the frees
that currently call free(check_buf, xferBenchConfig::page_size) and free(buf,
size) (the calls around the allocateXferMemory() uses in
ensureFileHasConsistencyData) to call freeXferMemory(check_buf,
xferBenchConfig::page_size) and freeXferMemory(buf, size) respectively so that
freeXferMemory() handles hugepage-aligned munmap vs free.
---
Duplicate comments:
In `@benchmark/nixlbench/src/utils/utils.cpp`:
- Around line 461-469: The current code only rounds total_buffer_size to
HUGEPAGE_SIZE, but main.cpp uses that to set each IOV's len (iov.len) and
computes offsets with (i * stride) % iov.len, so you must align the per-slice
(per-IOV) size too: compute the per_slice = total_buffer_size / num_iovs (or
per-thread/per-device count used when building IOVs), round per_slice up to a
multiple of HUGEPAGE_SIZE, then set total_buffer_size = aligned_per_slice *
num_iovs (and update any variable used for iov.len/stride accordingly) so that
iov.len is hugepage-aligned and (i * stride) % iov.len cannot land at non-2MB
boundaries; adjust the logic around total_buffer_size, HUGEPAGE_SIZE, and the
code paths that assign iov.len/stride to use the aligned per-slice value.
In `@benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp`:
- Around line 1308-1350: registerIterationMem and deregisterIterationMem
currently construct remote_reg with GET_SEG_TYPE(false) which yields the wrong
segment type for storage backends; update both functions to compute the remote
segment type from the backend (the same logic used in
prepareTransferDescriptors) and use that value when constructing
nixl_reg_dlist_t remote_reg and when calling iovListToNixlRegDlist, so storage
backends get OBJ_SEG/BLK_SEG/FILE_SEG as appropriate (i.e., derive the segment
type from nixlBackendH *backend_engine and replace GET_SEG_TYPE(false) with that
backend-specific segment type).
🪄 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: 8d598082-1fa5-41ac-847d-e297dd7d956f
📒 Files selected for processing (4)
benchmark/nixlbench/src/main.cppbenchmark/nixlbench/src/utils/utils.cppbenchmark/nixlbench/src/utils/utils.hbenchmark/nixlbench/src/worker/nixl/nixl_worker.cpp
benlwalker
force-pushed
the
nixlbench-improvements
branch
from
April 16, 2026 15:08
a757118 to
42bca20
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
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)
1374-1442:⚠️ Potential issue | 🟠 MajorPreserve per-iteration
prepare_durationsamples whenrecreate_xferis enabled.This branch now backs
reregister_mem, but Line 1442 collapses all prep time into one averaged sample.Avg Prepstill looks reasonable, yetP99 Prepbecomes meaningless because the stats vector only contains a single point. Add each iteration’s prepare sample directly instead of emitting one post-loop average.🔧 Suggested change
- nixlTime::us_t total_prepare_duration = 0; @@ - total_prepare_duration += timer.lap(); + thread_stats.prepare_duration.add(timer.lap()); @@ - // Average prepare duration across iterations - thread_stats.prepare_duration.add(total_prepare_duration / num_iter);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp` around lines 1374 - 1442, The code currently accumulates prepare time into total_prepare_duration and then adds a single averaged sample (thread_stats.prepare_duration.add(total_prepare_duration / num_iter)) after the recreate_per_iteration loop, which loses per-iteration distribution (P99). Fix by recording each iteration's prepare duration immediately after createXferReq inside the loop (i.e., call thread_stats.prepare_duration.add(timer.lap()) right after the createXferReq success check), stop accumulating into total_prepare_duration (or keep it but do not emit the average), and remove the post-loop averaged add; leave the non-recreate path that already calls thread_stats.prepare_duration.add(timer.lap()) intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benchmark/nixlbench/src/utils/utils.cpp`:
- Around line 467-474: The current hugepage rounding only adjusts
total_buffer_size; you must also ensure each per-buffer span used for
initiator/target partitions is hugepage-aligned: after computing per-buffer
spans (e.g., span_initiator = total_buffer_size / (num_initiator_dev *
num_threads) and span_target = total_buffer_size / (num_target_dev *
num_threads) or wherever partition buffer size / stride is derived), round each
span up to the next multiple of HUGEPAGE_SIZE (using the same formula as for
total_buffer_size) or validate and error if not divisible; update any dependent
calculations (stride/offsets) to use these per-buffer-aligned sizes and adjust
total_buffer_size or emit a clear error/log if alignment forces a larger total.
Ensure this logic is placed near the existing use_hugepages block in utils.cpp
and references the same symbols (use_hugepages, total_buffer_size,
HUGEPAGE_SIZE, num_initiator_dev, num_target_dev, num_threads, stride/offset
calculations).
- Around line 461-466: After reading config args (where reregister_mem and
pipeline_depth are set via NB_ARG), also validate that num_iter (the variable
parsed from NB_ARG) is > 0 and fail fast with a descriptive error and non-zero
return (e.g., return -1) if not; this prevents later UB in the recreate branch
(division by num_iter) and the pipelined branch (computing depth = 0 and using
it in local_iov.size() % depth). Add the check near the existing pipeline_depth
validation (same initialization block) so the worker never proceeds with
num_iter == 0.
In `@benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp`:
- Around line 690-706: The write loops in initBasicDescFile() and
ensureFileHasConsistencyData() only treat pwrite() errors when rc < 0, so a zero
return causes an infinite loop; change the guard in both loops to treat rc <= 0
as failure (i.e., if rc <= 0) — log an error (including strerror(errno) or a
message for rc==0), call freeXferMemory(buf, buffer_size), and return
std::nullopt just like the current error branch so the function exits safely
when pwrite makes zero progress.
---
Outside diff comments:
In `@benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp`:
- Around line 1374-1442: The code currently accumulates prepare time into
total_prepare_duration and then adds a single averaged sample
(thread_stats.prepare_duration.add(total_prepare_duration / num_iter)) after the
recreate_per_iteration loop, which loses per-iteration distribution (P99). Fix
by recording each iteration's prepare duration immediately after createXferReq
inside the loop (i.e., call thread_stats.prepare_duration.add(timer.lap()) right
after the createXferReq success check), stop accumulating into
total_prepare_duration (or keep it but do not emit the average), and remove the
post-loop averaged add; leave the non-recreate path that already calls
thread_stats.prepare_duration.add(timer.lap()) intact.
🪄 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: 976ecdf8-0e89-4daa-bf22-514585a6bb41
📒 Files selected for processing (4)
benchmark/nixlbench/src/main.cppbenchmark/nixlbench/src/utils/utils.cppbenchmark/nixlbench/src/utils/utils.hbenchmark/nixlbench/src/worker/nixl/nixl_worker.cpp
benlwalker
force-pushed
the
nixlbench-improvements
branch
from
April 16, 2026 20:14
42bca20 to
b3439b7
Compare
iyastreb
approved these changes
May 19, 2026
Contributor
|
/build |
guy-ealey-morag
approved these changes
May 19, 2026
Contributor
|
/ok to test a325aea |
Contributor
|
/build |
It's more efficient to allocate big regions of memory using hugepages. Signed-off-by: Ben Walker <ben@nvidia.com>
Allow benchmarking with per-iteration createXferReq/releaseXferReq to measure the full request lifecycle cost including preparation overhead. GUSLI still forces this on regardless of the flag. Signed-off-by: Ben Walker <ben@nvidia.com>
Simulate the LMCache/KVBM pattern where memory is registered and deregistered around every I/O. When enabled, each iteration calls registerMem before and deregisterMem after the transfer, and the transfer request is recreated per iteration since it cannot survive a deregister/re-register cycle. Signed-off-by: Ben Walker <ben@nvidia.com>
Enable pipelined execution with multiple xferReqs in flight simultaneously, matching LMCache/KVBM application patterns. The sliding window resubmits completed requests immediately rather than waiting for all to finish. Default depth of 1 preserves existing serial behavior. Interacts correctly with --recreate_xfer and --reregister_mem. Signed-off-by: Ben Walker <ben@nvidia.com>
benlwalker
force-pushed
the
nixlbench-improvements
branch
from
May 20, 2026 08:02
a325aea to
aa97529
Compare
Contributor
|
/ok to test aa97529 |
Contributor
|
/build |
benlwalker
force-pushed
the
nixlbench-improvements
branch
from
May 21, 2026 12:15
aa97529 to
68fba7e
Compare
Contributor
|
/ok to test 456b30b |
Contributor
|
/build |
brminich
approved these changes
May 22, 2026
isdrk
pushed a commit
to isdrk/nixl
that referenced
this pull request
May 25, 2026
* nixlbench: Add --use_hugepages option to use hugepages It's more efficient to allocate big regions of memory using hugepages. Signed-off-by: Ben Walker <ben@nvidia.com> * nixlbench: add --recreate_xfer option Allow benchmarking with per-iteration createXferReq/releaseXferReq to measure the full request lifecycle cost including preparation overhead. GUSLI still forces this on regardless of the flag. Signed-off-by: Ben Walker <ben@nvidia.com> * nixlbench: add --reregister_mem option Simulate the LMCache/KVBM pattern where memory is registered and deregistered around every I/O. When enabled, each iteration calls registerMem before and deregisterMem after the transfer, and the transfer request is recreated per iteration since it cannot survive a deregister/re-register cycle. Signed-off-by: Ben Walker <ben@nvidia.com> * nixlbench: add --pipeline_depth option Enable pipelined execution with multiple xferReqs in flight simultaneously, matching LMCache/KVBM application patterns. The sliding window resubmits completed requests immediately rather than waiting for all to finish. Default depth of 1 preserves existing serial behavior. Interacts correctly with --recreate_xfer and --reregister_mem. Signed-off-by: Ben Walker <ben@nvidia.com> --------- Signed-off-by: Ben Walker <ben@nvidia.com>
This was referenced Jun 2, 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
6 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?
Add four different options to nixlbench to allow simulation of more complex workloads using nixlbench
Why?
These are all hot code paths we have observed in various inference frameworks and we want to be able to recreate the NIXL API usage in nixlbench.
How?
Each commit adds a different option to add a new nixlbench behavior.
Summary by CodeRabbit
New Features
Bug Fixes / Validation