Skip to content

Issue 242 thor benchmark followups - #248

Open
RamyaGuru wants to merge 4 commits into
mainfrom
issue-242-thor-benchmark-followups
Open

Issue 242 thor benchmark followups#248
RamyaGuru wants to merge 4 commits into
mainfrom
issue-242-thor-benchmark-followups

Conversation

@RamyaGuru

Copy link
Copy Markdown
Collaborator

Addresses #242. All issues reproduced on current main (676b260) before being fixed.

1. Size queue memory regions against batch_size, not just ring depth.
adjust_memory_regions() sized MRs relative to the NIC ring only, ignoring that
is_tx_burst_available() needs 2x batch_size free on top of the ring — so at the shipped
batch_size: 10240 its own 3x-ring bump target (24576) sat below the 28672 TX requires and
silently wedged TX at bursts=0. Floor is now max(1.5x ring, ring + 2x batch_size) and the
bump target max(3x ring, ring + 4x batch_size), plus a one-shot diagnostic for bursts the
pool can never satisfy.

2. Fail fast when a device memory region has no GPUDirect path.
With dma-buf unsupported and nvidia_peermem absent, registration logged "Successfully
registered" and then died two functions later as a bare Invalid argument; it now fails at
registration, naming the CUDA device and what else CUDA can see so "wrong GPU selected" is
readable off one line.

3. Allow overriding the container CUDA base image.
scripts/build-container.sh overwrote an exported DAQIRI_OS_BASE_IMAGE, pinning CUDA to
13.1.0 and making the script unusable on hosts with an older driver; it now honors a pre-set
value and adds CUDA_VERSION / UBUNTU_VERSION, with default output unchanged.

Testing. On IGX Thor with a ConnectX-7 cable loopback: bracketed the deadlock boundary
exactly, confirmed both previously-wedging configs now run at line rate with zero drops
(corroborated by ethtool -S wire counters), reproduced the GPUDirect failure on the Tegra
iGPU, and verified CUDA 13.0 inits where 13.1 returns cuInit=803. A/B against main with
matched build configuration shows no regressions across gpudirect, hds, the 4-queue
config, sw_loopback, and reorder_seq.

Note: #242's BAR1 premise is incorrect — the card has 8 GiB, not 256 MiB — so nothing
forces a smaller ring and the per-queue ring-depth YAML field the issue proposes is not
included; the sizing formula was the actual bug.

scripts/build-container.sh assigned DAQIRI_OS_BASE_IMAGE unconditionally inside
the BASE_IMAGE case, so an exported value was silently discarded and the CUDA
base was effectively pinned to 13.1.0. A container whose CUDA runtime is newer
than the host driver cannot initialize CUDA at all, which makes the script
unusable on hosts with an older driver: on IGX Thor (driver 580.00 / CUDA 13.0)
a 13.1 image fails with cuInit=803, "system has unsupported display driver /
cuda driver combination". cuda-compat forward compatibility does not apply to
Tegra, so there is no runtime workaround.

Honor a pre-set DAQIRI_OS_BASE_IMAGE, and add CUDA_VERSION (default 13.1.0, so
the default build is unchanged) and UBUNTU_VERSION shorthands for the cuda base.
BASE_IMAGE keeps its cuda|torch selector semantics and torch is unaffected.

Testing (IGX Thor, driver 580.00 / CUDA 13.0):
- Resolution paths: default still yields cuda:13.1.0-devel-ubuntu24.04
  unchanged; CUDA_VERSION=13.0.0 yields 13.0.0; an exported
  DAQIRI_OS_BASE_IMAGE overrides both; BASE_IMAGE=torch unaffected; an invalid
  BASE_IMAGE still exits 1.
- Built --target base on cuda:13.0.0-devel-ubuntu24.04 and compared CUDA driver
  init against the host driver: 13.0 gives cuInit=0 and enumerates the discrete
  RTX PRO 6000; the 13.1 default gives cuInit=803 (unsupported display driver /
  cuda driver combination), count=-1.

Assisted-by: Claude
Signed-off-by: Ramya Gurunathan <rgurunathan@nvidia.com>
adjust_memory_regions() sized queue-backed MRs purely relative to the NIC
descriptor ring: it bumped anything below 1.5x ring up to 3x ring. That models
worker refill headroom but ignores the TX path, where is_tx_burst_available()
refuses a burst unless 2x batch_size buffers are free *on top of* whatever the
ring is holding. With the shipped batch_size of 10240 and the default 8192-deep
ring, the 3x ring bump target (24576) is below the 28672 that TX requires, so
the bump produced a configuration that wedges TX permanently at bursts=0 while
logging that it had just prevented a deadlock. Nothing reported an error: 0
missed, 0 errored, exit 0, and only one burst ever reaching the wire.

Take the max of both constraints instead: floor at
max(1.5x ring, ring + 2x batch_size) and bump to
max(3x ring, ring + 4x batch_size). Small-batch configs keep their previous
sizing, so this only raises the bar where batch_size demands it.

Also report the one remaining route to a silent TX stall that config sizing
cannot cover: an application asking for a burst larger than its configured
batch_size. is_tx_burst_available() now distinguishes ordinary backpressure from
a request the pool can never satisfy however long it drains, and logs that once
per queue with the num_bufs needed. A short pool that will drain stays silent.

Testing (IGX Thor, ConnectX-7 dual-port cable loopback, 8 KB frames unless noted):

- Bracketed the deadlock boundary at ring 8192 / batch 10240. 24576 (the old
  bump target) and 26624 wedge at bursts=1; 28672 (= ring + 2x batch) runs.
- Measured where starvation clears: 28672 still logs 1.4e9
  rx_mbuf_allocation_errors, 38912 logs 1.7e7, 49152 (= ring + 4x batch) is the
  first value with zero alloc errors and zero drops. 51200 (what the example
  configs ship) is also clean, which is consistent with the new target.
- The two configs that previously wedged (explicit num_bufs 24576, and
  num_bufs 8000 auto-bumped into 24576) now run at line rate with 0 missed and
  0 alloc errors. Wire counters via ethtool -S corroborate: before the fix only
  10,242 frames left the NIC in 10 s.
- Oversized-burst guard: a bench batch of 40000 against a 51200 pool logs the
  new error exactly once and names the required num_bufs.
- No regressions, A/B against main with matched build configuration
  (CMAKE_CUDA_ARCHITECTURES and DAQIRI_BUILD_PYTHON aligned; an earlier mismatch
  made reorder_seq look artificially better on this branch):
  daqiri_bench_raw_gpudirect, daqiri_bench_raw_hds (2 MRs per queue, which
  exercises the new per-MR max-batch map), the 4-queue config,
  daqiri_bench_raw_sw_loopback, and daqiri_bench_raw_reorder_seq all match
  baseline within run-to-run variance.
- Shipped example configs are unaffected: all raw configs in examples/ and
  applications/ already clear both thresholds, so none are bumped and no new
  warnings appear.

Assisted-by: Claude
Signed-off-by: Ramya Gurunathan <rgurunathan@nvidia.com>
When CU_DEVICE_ATTRIBUTE_DMA_BUF_SUPPORTED is 0, register_memory_regions() fell
back to rte_extmem_register() and logged "Successfully registered external
memory" even with nvidia_peermem absent, i.e. with no usable GPUDirect path at
all. The failure then surfaced two functions later in map_memory_regions() as a
bare rte_dev_dma_map "Invalid argument", by which point nothing identified the
cause. The common trigger is selecting the wrong GPU: on Tegra/IGX the container
runtime exposes only the integrated GPU unless the discrete one is passed
through explicitly, and an integrated GPU cannot export a dma-buf.

Treat dma-buf unsupported plus peermem absent as the unsatisfiable combination it
is and fail there, naming the CUDA device by ordinal, name, UUID and integrated
status. Report what else CUDA can see, so a user who picked the wrong ordinal is
pointed at the discrete device to use instead, and one whose container exposes no
discrete GPU is told that rather than being sent after peermem, which does not
apply to Tegra. Decode cuGetErrorString on the cuMemGetHandleForAddressRange
failure path too, and note the kernel and driver prerequisites there, matching
the ibverbs engine's message from #239.

Before, on the Thor iGPU:
  WARN     dma-buf not supported for device 0. Attempting to use nvidia-peermem
  INFO     Successfully registered external memory for Data_TX_GPU
  CRITICAL Could not DMA map EXT memory: -1 err=Invalid argument

After:
  CRITICAL Memory region 'Data_TX_GPU' requests kind: device on CUDA device
           0 ('NVIDIA Thor', integrated) GPU-a7c66ad2-... , which cannot export a
           dma-buf, and the nvidia_peermem module is not loaded -- there is no
           usable GPUDirect path for this device. CUDA sees 1 device(s), none of
           them discrete. On Tegra/IGX the container runtime exposes only the
           integrated GPU unless the discrete GPU is passed through explicitly
           (CDI: --device nvidia.com/gpu=N); alternatively use kind: host_pinned.

Uses only libcuda, which is already linked; no new dependency. The discrete-GPU
path is unchanged and still registers via dma-buf.

Testing (IGX Thor):
- Reproduced the original failure by exposing only the Tegra iGPU
  (--runtime=nvidia --gpus all, which forces csv mode; the iGPU reports
  INTEGRATED=1 and DMA_BUF_SUPPORTED=0) and confirmed the new message replaces
  it, with the misleading "Successfully registered" and "Invalid argument" lines
  gone and init aborting at registration.
- Discrete-GPU path unchanged: with the RTX PRO 6000 exposed via CDI,
  DMA_BUF_SUPPORTED=1 and every raw bench still registers through dma-buf
  ("dma-buf supported for device 0") and runs at line rate.

Assisted-by: Claude
Signed-off-by: Ramya Gurunathan <rgurunathan@nvidia.com>
@greptile-apps

greptile-apps Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR fixes the previously unguarded GPUDirect fallback and improves DPDK memory-pool sizing and diagnostics.

  • Guards every legacy nvidia_peermem fallback when DMA-BUF export is unavailable or fails.
  • Sizes queue-backed memory regions using both descriptor-ring depth and batch requirements.
  • Adds a one-shot diagnostic for permanently unsatisfiable TX bursts.
  • Allows container CUDA and Ubuntu base-image versions to be overridden.
  • Updates configuration and onboarding documentation for the changed behavior.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
src/engine_dpdk.cpp The previously unguarded DMA-BUF export-failure fallback now checks for nvidia_peermem; initialization abort and resource cleanup remain correctly wired.
src/engines/dpdk/daqiri_dpdk_engine.cpp Memory-region sizing now accounts for batch headroom, and unsatisfiable TX burst requests receive a rate-limited diagnostic.
src/engines/dpdk/daqiri_dpdk_engine.h Adds per-queue atomic state used to emit the oversized-burst diagnostic only once.
scripts/build-container.sh Honors explicit base-image overrides and derives the default CUDA image from configurable CUDA and Ubuntu versions.
docs/api-reference/configuration.md Documents the revised raw-DPDK memory-region floor and automatic bump target.
docs/getting-started.md Documents host-driver compatibility and container base-image version overrides.
docs/tutorials/configuration-walkthrough.md Updates the annotated num_bufs guidance to match the new ring-and-batch sizing formula.
AGENTS.md Records the container-build overrides and DPDK memory-region sizing behavior for maintainers.

Reviews (2): Last reviewed commit: "#242 - Guard every peermem fallback, not..." | Re-trigger Greptile

Comment thread src/engine_dpdk.cpp
… case

The previous commit only fail-fasted when CU_DEVICE_ATTRIBUTE_DMA_BUF_SUPPORTED
was 0. Two other paths still fell back to rte_extmem_register() without checking
that nvidia_peermem was loaded: when the device advertises dma-buf but
cuMemGetHandleForAddressRange fails, and when the DPDK build predates
rte_extmem_register_dmabuf. In both cases registration can still succeed and the
failure resurfaces later in map_memory_regions() as the same non-actionable
rte_dev_dma_map "Invalid argument" this change set out to remove.

Factor the check into peermem_fallback_ok() and apply it at all three fallback
sites, so the actionable message is emitted wherever GPUDirect has no path left.
The pre-24.11 site closes the exported fd before bailing out.

Reported by Greptile on #248.

Testing: re-verified both ends on IGX Thor. The Tegra iGPU still fails fast at
registration with the device named and no "Successfully registered" or "Invalid
argument" lines; the discrete RTX PRO 6000 still takes the dma-buf path
("dma-buf supported for device 0") and runs at line rate.

Assisted-by: Claude
Signed-off-by: Ramya Gurunathan <rgurunathan@nvidia.com>
@RamyaGuru
RamyaGuru requested a review from cliffburdick August 18, 2026 15:47
"MR '{}' had num_bufs={}, below the {} required by a {}-deep NIC ring and "
"batch_size={} (ring + 2x batch); TX would wedge with no error and the RX refill "
"path would starve. Bumping to {} (ring + 4x batch), the smallest size measured "
"free of mbuf allocation errors.",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these warnings tell the user what to do? ie increase num_bufs?

Comment thread src/engine_dpdk.cpp
DAQIRI_LOG_CRITICAL(
"Memory region '{}' requests kind: device on CUDA device {}, but {}, and the "
"nvidia_peermem module is not loaded -- there is no usable GPUDirect path for this "
"device. {}.",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest trying host_pinned?

@cliffburdick cliffburdick left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does anything here apply to ibverbs path as well?

@RamyaGuru

RamyaGuru commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator Author

Does anything here apply to ibverbs path as well?

Yes, good call. I think at least item 2 on the list would also need to be ported over to ibverbs. I'll make sure I've gone through all the ibverbs testing and adjust the warning/helper messages based on your comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants