Skip to content

Implement the standalone CUDA runtime (t4a-cuda-runtime) - #2

Merged
shinaoka merged 16 commits into
mainfrom
design/cuda-runtime
Oct 20, 2026
Merged

shinaoka merged 16 commits into
mainfrom
design/cuda-runtime

Conversation

@shinaoka

@shinaoka shinaoka commented Oct 20, 2026 •

Copy link
Copy Markdown
Member

Implements the standalone CUDA runtime from #1, following the design in
docs/design/cuda-runtime.md. This does not switch tenferro's backend, port its kernel catalog, or
publish a crate; adoption stays a separate maintainer decision.

What is included

  • crates/t4a-cuda-runtime (publish = false), depending only on cudarc 0.19.8 (low-level Driver/NVRTC
    bindings only) and thiserror.
  • One real non-default stream, one explicit CUmemPool and one allocation-domain identity per runtime;
    opening the same ordinal twice gives independent runtimes that may share the primary context.
  • Stream-ordered device buffers (last Drop enqueues cuMemFreeAsync; no per-buffer lease or event),
    bounded pinned staging, explicit ordered upload/download, and cross-runtime copies (same-device bracket
    and staged multi-GPU with per-chunk source witnesses).
  • Serialised admission (reserve → enqueue → commit), completion epochs for keepalives, quarantine of
    uncertain allocations, and leak-on-unproved-drain teardown.
  • Context-free NVRTC artifacts (ArtifactCache, full-identity keys), per-runtime module cache, checked
    launch descriptors, and an unsafe raw consumer seam.
  • Tests: module-local src/<module>/tests/, public-contract tests/host_contract.rs,
    gpu_single.rs, gpu_multi.rs, gpu_faults.rs; cfg(test) failure injection after enqueue;
    examples/warm_chain.rs; focused CPU-only CI workflow.

Validation performed

A100 80GB PCIe, driver 580.126.09, CUDA 12.6 toolkit/NVRTC:

  • cargo fmt --all --check
  • cargo clippy -j 16 --workspace --all-targets --all-features -- -D warnings
  • cargo test -j 16 --workspace (CPU-only), doctests, cargo doc --workspace --no-deps
  • cargo test -j 16 -p t4a-cuda-runtime --features gpu-tests --lib --test host_contract --test gpu_single --test gpu_faults -- --test-threads=1: 34 unit + 6 single-GPU contract + 1 isolated illegal-address fault
    test pass. This covers two runtimes on one ordinal, distinct streams/domains, kernel → copy → kernel →
    readback without caller waits, immediate source drop, dropped/forgotten download handles, bounded staging
    over several chunks, module reuse/eviction, context restore after success and unwind, cross-thread last
    drop, and raw-scope poisoning on panic.
  • examples/warm_chain (release, taskset -c 0): 200 warm submissions, zero per-launch events, ~1.95 µs per
    launch, and an indicative (unpaired, shared-host) direct-cudarc comparison.

Not run

  • tests/gpu_multi.rs compiles but was not executed: this host has one physical GPU. On such a host it
    fails with an explicit message rather than passing silently. Staged-copy correctness, opposing transfers
    and cross-device failure propagation still need a real two-GPU run.
  • No hosted GPU CI was used; no vendor-consumer (cuBLAS) test is included.

Review status

Independent GPT reviews ran before implementation and twice after it. The findings that were verified and
fixed include: serialised admission vs observation, reserve-before-enqueue, quarantine/broken handling on
every post-enqueue failure, leak-on-failed-drain teardown, complete artifact/module identity, NVRTC symbol
probing and program cleanup, frontier pruning under pressure, and transitive upstream witnesses.

Known open items (not claimed as done)

Counters now cover admitted launches, device allocations, zero fills, completion events (including
cross-runtime ones recorded by a copy) and host waits. Tests cover owned-pinned upload on both routes, source
mutation after upload, immediate source overwrite after copy admission, and the source-back-wait injection
point.

# Severity Item Exit criteria
1 BLOCKER for adoption The physical two-GPU lane was not run (tests/gpu_multi.rs compiles). Correctness across distinct primary contexts, a third runtime observing source events, opposing transfers and cross-device failure propagation are unverified; the forced-staged test exercises the chunk ring only. Run the target on a two-GPU host; record device names, driver and toolkit.
2 MINOR A paired latency measurement is absent; published numbers stay labelled indicative. Run the paired protocol on a dedicated host when a performance claim is needed.
3 OPTIONAL No vendor-consumer (cuBLAS/cuFFT) test; the design lists it as optional and it would need a vendor loader in dev-dependencies. Add when a consumer needs that seam.
4 NOTE Requests above 64 KiB alignment and more than 32 kernel arguments are rejected; the two-GPU target does not yet cover opposing transfers or chunk-pressure recycling. Documented behaviour / test gap.

Deliberately out of scope

P2P fast path, persisted artifacts, CUDA Graph, NCCL, fused kernel catalog, registry publication, and any
tenferro backend change.

@shinaoka shinaoka left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Audit of e375c95 (implementation)

Scope: full read of context.rs, memory.rs, retirement.rs, transfer.rs, runtime.rs, raw.rs and their module tests; compile.rs, kernel.rs, error.rs, the four tests/*.rs, examples/warm_chain.rs and the CI workflow were reviewed with an independent pass and the findings re-verified against the source. FFI usage was checked against the unpacked cudarc 0.19.8 crate. cargo clippy --all-targets --all-features -D warnings and the CPU-only cargo test pass here too (macOS, no driver). No GPU was available to me, so nothing below is a hardware result.

Verdict: not mergeable yet. The lifetime model is implemented the way the design says, and the FFI layer is careful (ProgramGuard on every path, NUL-terminated image for cuModuleLoadData, kernelParams kept alive across the call, no CudaSlice/launch builder/new_stream). The problems are a handful of real bugs outside the items you already listed, an over-claimed performance/evidence section, and acceptance rows without a test.

Process note. Issue #1 and the design's §8 both say implementation starts after API-shape approval, and no approval was given. I am reviewing the code on its merits, but the API surface (Arg, RawResource, LaunchConfig, CompileRequest, the stats fields) is being reviewed for the first time in this PR, so expect naming changes.

A. Must fix before merge (new, not in the PR's open-items table)

A1. ContextHolder::drop clears the thread's current context while another holder still retains it (src/context.rs:186-204). Two runtimes on one ordinal share the primary context. If the last Arc<ContextHolder> of runtime A is dropped on a thread that currently has that context active on behalf of runtime B (for example a consumer drops the last Buffer of A inside B's with_raw closure, or any CtxScope of B is live), previous == self.context and the code does cuCtxSetCurrent(NULL). B's next vendor call in that closure fails with CUDA_ERROR_INVALID_CONTEXT, and B's scope later "restores" a context it never cleared. Fix: after cuDevicePrimaryCtxRelease, if previous == self.context, re-set it and only fall back to NULL when cuCtxSetCurrent reports the handle is gone (or query cuDevicePrimaryCtxGetState). Add a test: two runtimes on ordinal 0, drop A's last buffer inside B's raw closure, assert cuCtxGetCurrent is unchanged.

A2. Staging chunks are not portable pinned memory (src/retirement.rs:140, malloc_host(len, 0)). The staged cross-device route D2H-copies from the source context into a chunk allocated under the destination context. Without CU_MEMHOSTALLOC_PORTABLE that memory is page-locked only for the allocating context; on platforms without UVA across both devices (Windows WDDM, mixed-mode setups) the source-side cuMemcpyDtoHAsync degrades to a pageable copy, which is synchronous and breaks the ordering assumptions in copy_staged. The design says "portable pinned chunks" explicitly. Pass CU_MEMHOSTALLOC_PORTABLE; it costs nothing.

A3. An empty CUBIN is compiled, cached and only fails at bind (src/compile.rs:301-319). nvrtcGetCUBINSize returns 0 for a virtual arch such as compute_80 with OutputKind::Cubin; cubin_bytes returns Ok(vec![]), the artifact is inserted in the cache, and the error surfaces later as InvalidArgument("artifact image is empty") from kernel.rs:255, unrelated to the cause. Treat size 0 as Error::Compile and do not cache.

A4. ModuleCache is not a cache (src/kernel.rs:150-215). Entries hold Weak<ModuleInner>, so the cache never keeps a module alive; once every BoundFunction is dropped the module is unloaded and the next bind reloads it. module_cache_entries / module_cache_bytes / clear_modules() / module_entries therefore describe a deduplication table of currently live modules, not the bounded retained cache the design (§6) and the rustdoc describe. Either hold Arc and evict by the configured bounds, or rename the config/stats and fix the docs. The acceptance row "clearing a module cache with work outstanding is safe" currently passes for the trivial reason that there is nothing to clear.

A5. ModuleInner::drop ignores the activation result it says it honours (src/kernel.rs:41-45). The comment says "a failure to activate leaks the module", but let _scope = self.holder.activate(); discards the Result and cuModuleUnload runs regardless. Same pattern in PinnedChunk::drop, Witness::drop, Completion::drop. Make it if let Ok(_scope) = ... { unload } else { leak + count }.

A6. The warm-chain numbers in README/§9 are not supported by the example (examples/warm_chain.rs).

  • "0 per-launch events" is a literal string (:94-97); there is no event-record, runtime-launch or host-wait counter anywhere in Stats/RetirementStats, so the claim cannot be verified by the example or by any test. Every launch pushes a module keepalive into the open epoch and seal_if_half_locked records an event every max_pending_records/2 = 512 launches; 200 iterations simply never reach it. The true statement is "one event per 512 launches, amortised", and it needs a counter to say so.
  • The "direct cudarc" baseline includes cuStreamSynchronize inside the timed window (:188-193) while the runtime number is submission time only. The per-launch comparison is not comparable; please either match the windows or remove the comparison. Keeping "indicative" as the label does not make an apples-to-oranges number informative.
  • The design's §6 counters (NVRTC attempts/successes at the call boundary, runtime-managed launches, allocations/zero-fills, events, host waits) are not implemented; artifact_cache_misses is incremented before validate()/nvrtc_version() and is used as the "NVRTC attempts" proxy. Until those exist, the acceptance row "warm execution calls NVRTC zero times" has no evidence.

B. Agree with your open items 1 and 3, with one addition

  • Item 1 (witness memoisation): confirmed. Witness::query re-queries completed upstream events on every call, and cuEventQuery on a context that later hit a sticky error returns that error for an already-completed event, so the §5 rule "a later unrelated failure on A is not retroactively a failure of an already confirmed transfer" is violated by the implementation today. A OnceLock<Result<(),Error>>-style terminal state per witness fixes both the retroactive failure and the repeated recursion.
  • Item 3 (same-thread re-entry): confirmed. during_admission holds the operation mutex across the user closure (raw.rs:109-137); PendingDownload::poll/wait and Completion::poll call domain.observe() which takes the same mutex. A std Mutex relocked on the same thread deadlocks. Cheapest fix: record the owning ThreadId in the admission guard and return a typed error from observe on re-entry.
  • Addition: RawScope::device_ptr/device_ptr_range (raw.rs:53-66) accept any same-domain buffer, registered or not; the rustdoc says "registered with this scope" and the design says spans are given only for registered objects. Registration currently only pins through the borrow in resources. Either enforce (keep a set of registered pointers) or correct the docs.

C. Should fix (not blocking)

  • kernel::launch validates no argument count or size (kernel.rs:315-337). cuFuncGetParamInfo exists in the 12080 bindings; use it when available so a short args slice cannot make the driver read past params. An empty Buffer has device_ptr() == 0 and is passed as a NULL kernel argument without comment (memory.rs:255); document or reject.
  • Module lookup keys on entry (kernel.rs:191), so binding two entry points of one artifact loads the module twice and module_loads reports 2.
  • Each launch does three heap allocations (slots, params, Box<Arc<ModuleInner>>) plus a lease push; §6 asks for no needless per-argument allocation on the warm path. A fixed-size SmallVec-like array for ≤ 16 args and deduplicating the module pin per open epoch would remove all of it.
  • PendingDownload::poll after a successful Some returns Some(vec![]) on the next call (transfer.rs:290-295); make the second call an error or keep the data.
  • Artifact lookup compares key and request but not compiler_version (compile.rs:356-362); harmless in-process, but it contradicts §6.
  • Header names are embedded as // header: {name} (compile.rs:81-83); only NUL is validated, so a name containing a newline injects source. Validate like options.
  • from_driver maps any CUDA_ERROR_OUT_OF_MEMORY (including from cuModuleLoadData/cuLaunchKernel) to OutOfMemory{bytes:0} and loses context (error.rs:120-126).
  • context::init calls cuInit(0) directly and discards the result, then calls drv::init() again (context.rs:64-66). cuEventSynchronize is in REQUIRED_SYMBOLS but never used; cuDeviceGetName is used but not probed.
  • UnknownEntryPoint mapping lives in Runtime::bind rather than kernel::bind, and nothing tests it.
  • tests/host_contract.rs module docs say the tests run "without initialising CUDA"; two of them call cuInit(0) on a host with a driver. Say "without creating a context".
  • CI: no GPU job exists (the workflow only comments that one should), and cargo doc runs without RUSTDOCFLAGS=-D warnings; the README sentence "rustdoc passes" should say warnings are allowed, or add the flag.
  • Test hygiene: injected_post_enqueue_failure_quarantines_the_source ends with assert!(observe().is_err() || pending_records == 0), which cannot fail in practice; runtime_is_send_but_not_sync asserts only Send; misaligned_requests_are_rejected_without_leaking says 64 KiB in the comment and tests alignment 3. unsafe { nvrtc::compile_program(...) } (compile.rs:257) lacks // SAFETY:.

D. Acceptance rows with no test (beyond your item 4)

Distinct pool identities and runtime-local pool/cache stats for two runtimes (no pool id is exposed at all); BoundFunction from the other runtime rejected at launch; Arg::DevicePtr from the other domain rejected at launch; source overwritten after copy admission with the copy still holding the old bytes; a zero-host-wait assertion for the warm chain (needs the counters from A6); the CUBIN compile→bind→launch path; context restored after the final ContextHolder destructor; capacity-driven module eviction (moot until A4); a real cuLaunchKernel or vendor call inside with_raw (only memset is exercised); UnknownEntryPoint; a launch that fails synchronously (block of 2048 threads, oversized shared_bytes) returning Error::Driver. In gpu_faults.rs the only case is the illegal address; the five lanes from §8 (query source event from C under its own context, partial multi-chunk enqueue failure, A→B→C after dropping the original handles, confirmed A→B then unrelated A fault, no recovery claim on reopen) are absent. gpu_faults.rs:50-62 also checks only the with_raw Result, not the inner CUresult, so a driver that reports the fault synchronously makes the test fail in a misleading way.

E. Confirmed correct, for the record

Stream-ordered Buffer drop with no lease; reserve→enqueue→commit under one operation lock; chunk abandonment (leak) on post-enqueue failure; upload copying into staging before the DMA; upload_owned over-budget falling back to the staged path and releasing the input; same-device copy bracket (src_ready → D2D → dst_done → source back-wait) with both events retained as witnesses; per-chunk source events on the staged route with destination-local commit; Domain::drop requiring a drained retirement state before destroying pool/stream and leaking otherwise; Witness::query activating its own context and never synchronising a foreign event; CtxScope restore on unwind; assume_init via ManuallyDrop/ptr::read; Vec<MaybeUninit<u8>> to Vec<u8> conversion.

What I need before I look again

A1 to A6 fixed with the tests named above, your items 1 and 3 fixed, the counters from A6 added, and the README/§9 performance and evidence sentences rewritten to what the counters show. The two-GPU lane (your item 5) stays a hard gate for adoption but not for merging this crate as publish = false; please keep the "not run" wording until it has been run.

@shinaoka
shinaoka merged commit 9260015 into main Oct 20, 2026
2 checks passed
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.

1 participant