fix(kernel): clear the latched CUDA error before raising in pinned_tensor - #144
Open
rakhimovv wants to merge 2 commits into
Open
fix(kernel): clear the latched CUDA error before raising in pinned_tensor#144rakhimovv wants to merge 2 commits into
rakhimovv wants to merge 2 commits into
Conversation
…nsor TORCH_CHECK reports a failed CUDA call but does not read the status out of the runtime, so the error stays latched in the calling thread. The errors raised here are non-sticky and the context remains usable, but the next unrelated CUDA call picks up the stale status and fails as if it were its own. That turns a handled exception into a process-wide fault. host_banks.pin() catches the RuntimeError from host_register() and re-raises a friendlier message; any torch call made while handling that failure would report the stale cudaHostRegister error instead of its own. Route every CUDA call in the file through an FT_CUDA_CHECK macro that drains the status with cudaGetLastError() before raising. The two cudaDeviceGetAttribute calls in host_ptr_identity() were previously unchecked and are now checked as well. Check messages are unchanged. Add a test that drives the failure path directly and asserts the context survives it.
…on-fatal Two corrections to the previous commit. cudaFreeHost was the one call left uncovered, and it is the worst place to leak a status: it runs as a from_blob deleter during GC, with no exception to attribute the failure to. It drains unconditionally and never throws. host_ptr_identity's two cudaDeviceGetAttribute calls previously discarded their status with uva/reg pre-initialised to 0 -- the same idiom as driver_cuda_version's "stays 0 when no driver is installed", i.e. a deliberate fallback rather than an oversight. Raising there turned an unqueryable attribute into a fatal error on the offload path, and _host_ptr_identity is lru_cached, which does not cache exceptions, so it would have re-raised on every call. Drain without raising and keep the 0 fallback: "no identity" routes device_ptr through host_device_ptr, the real translation, which is correct everywhere. Also soften the macro comment: these calls report whatever is latched on the thread, so "every error below is non-sticky" was a stronger claim than holds. Draining leaves the context usable for the failures these calls originate; a sticky error latched elsewhere re-latches on the next call.
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
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.
Thanks for FreeToken — it set up and ran cleanly on a fresh H100 box, and the offload design was a pleasure to read through.
While running the suite I hit a failure that turned out to be worth a fix. Happy to adjust anything here, including dropping it if you would rather solve it differently.
What breaks
Most CUDA calls in
python/freetoken/kernel/csrc/pinned_tensor.cppare guarded withTORCH_CHECK(err == cudaSuccess, ...); two attribute queries and the deleter were notchecked at all.
TORCH_CHECKreports the error to the caller but never reads it out of theruntime, so the failure stays latched in the calling thread. The failures these calls can
originate are non-sticky — the context is still perfectly usable — but the next unrelated
CUDA call picks up the stale status and fails as if it were its own.
The result is that a handled exception from this extension silently poisons the rest
of the process.
Related: #125 fixes the test-side symptom of the same CUDA 13 behaviour; see
"Relationship to #125" below.
Observed failure chain
On this box the full suite fails twice, and the second failure is collateral from the
first:
tests/kernels/test_pinned_tensor.py::test_host_device_ptr_is_identity_under_uvacalls
host_device_ptr()on unregistered pageable memory. On CUDA 13 that returnscudaErrorInvalidValue, so the extension raises:The error is never drained. Later in the same pytest process,
tests/kernels/test_triton_attention.py::test_paged_triton_attention_matches_reference[None-256]dies on its very first line —
q = torch.randn(4, num_q_heads, head_dim, device=device)—with:
That the second failure is collateral and not a Triton bug is easy to confirm: run that
file on its own and it is green.
A direct probe shows the error is merely uncleared rather than fatal: calling
host_device_ptron pageable memory, thencudaGetLastError()through ctypes, returns1(cudaErrorInvalidValue) on the first read and0on the second, after whichtorch.randn(4, device="cuda")succeeds normally.Probe output before the change:
and after:
Why this is a real bug, not a test artifact
The clearest live example is
kernel/backend.py:driver_cuda_version(), which wraps theextension call in
except Exception: return Noneand carries on using CUDA afterwards —it runs at config time on the main thread (via
moe/nvfp4_backends.py), so a latchedstatus there lands on whatever CUDA call the engine makes next.
cudaFreeHostis the other one that matters, and for a different reason: it is afrom_blobdeleter, so it runs during GC with no exception to attribute a failure to. Astatus left latched there has no visible origin at all.
pinned.device_ptr()short-circuits on_host_ptr_identity(), so the failing pointertranslation is not itself on the hot path on Linux.
The test suite is where it showed up first because pytest keeps one process alive across
files; a long-running server has the same property.
The fix
An
FT_CUDA_CHECKmacro that callscudaGetLastError()to drain the latched statusbefore raising, on the seven calls that already raised:
cudaMallocHost,cudaHostAlloc,cudaGetDevice,cudaHostGetDevicePointer,cudaHostRegisterandcudaDriverGetVersion.The message text of each check is unchanged — old and new both take
TORCH_CHECK'svariadic path, so the strings are byte-identical and it is still a
c10::Error.The two remaining sites drain but deliberately do not raise:
cudaFreeHostin thefree_pinneddeleter, because a deleter must not throw.cudaDeviceGetAttributecalls inhost_ptr_identity(), which previouslydiscarded their status with
uva/regpre-initialised to0— the same idiom asdriver_cuda_version()'s// stays 0 when no driver is installed. That fallback isdeliberate and it is the safe answer: "no identity" routes
device_ptr()throughhost_device_ptr(), the real translation, which is correct on every platform. Raisingthere would turn an unqueryable attribute into a fatal error on the offload path, and
_host_ptr_identityislru_cached, which does not cache exceptions — so it wouldre-raise on every call rather than once.
That covers all nine CUDA calls in the file.
Why a new macro rather than
C10_CUDA_CHECK, which drains the same way:_pinned_tensoris a
CppExtensionlinking onlycudart, notlibc10_cuda, andC10_CUDA_CHECKwouldalso replace these specific messages with a generic
"CUDA error: ...".Relationship to #125
#125 fixes the first failure above from the test side: it stops
test_host_device_ptr_is_identity_under_uvafrom passing a pageable pointer at all, onthe correct grounds that unregistered memory was never inside
host_device_ptr'scontract. That is a fair reading and I have no objection to it.
It does not address what this PR is about. #125 changes only
tests/kernels/test_pinned_tensor.pyand states that runtime code is unchanged, soafter it lands the C++ still leaves its error latched — the suite just stops containing
a call that triggers it. Any real
cudaHostRegisterorcudaHostGetDevicePointerfailure, of the kind
host_banks.pin()is written to catch, still poisons whatever CUDAcall comes next.
The two changes are complementary rather than competing, and both are needed for a green
suite on CUDA 13. This PR deliberately does not touch
test_host_device_ptr_is_identity_under_uva, to stay out of #125's way.Test added
A new test,
test_failed_pinned_call_leaves_the_context_usable, drives the failure pathdirectly and asserts the property production depends on:
It uses
host_register, nothost_device_ptr, so it does not overlap #125's subjectmatter and does not depend on any pageable-pointer behaviour.
Before this change it fails on the line after the
raisesblock:After it, it passes.
Before / after
All runs on
bd372b6, same box, same install.Whole suite, unmodified tree:
failing on
tests/kernels/test_pinned_tensor.py::test_host_device_ptr_is_identity_under_uvaand
tests/kernels/test_triton_attention.py::test_paged_triton_attention_matches_reference[None-256].Whole suite with this PR applied:
(Wall-clock is omitted deliberately: the two runs differ mostly in JIT kernel-cache warmth,
not in anything this change does.)
The Triton collateral failure is gone and the new test passes. The one remaining failure
is
test_host_device_ptr_is_identity_under_uva, which is #125's to fix and which thisbranch leaves untouched.
Isolating the new test against the C++ change alone — same tree, extension rebuilt each
time:
Tested on
0.1.2, at commitbd372b6uv pip install -e ".[accel,dev]"Commands:
uv pip install -e ".[accel,dev]" .venv/bin/python -m pytest tests/ -v -rA .venv/bin/python -m pytest tests/kernels/test_triton_attention.py