fix(core): compare CUdeviceptr as int so the VMM grow fast path can run#2407
fix(core): compare CUdeviceptr as int so the VMM grow fast path can run#2407aryanputta wants to merge 1 commit into
Conversation
|
I have been assigned #2388, so I am taking this out of draft. It still covers defect 2 only and does not close the issue; I can follow up on defects 1, 3, and 4 in separate PRs if that is the split you prefer. One thing for a reviewer: workflows on this PR are still waiting on vetter validation, and I do not have an NVIDIA GPU, so |
513ebfc to
57a7a0b
Compare
|
Current head is
The PR description also records something I checked while reviewing this: the |
`modify_allocation()` asks `cuMemAddressReserve` for the address immediately after the current range, then checks whether the driver granted that exact address before taking the fast path. `cuMemAddressReserve` returns `new_ptr` as a `CUdeviceptr`. That type defines `__int__` and `__repr__` but no `__eq__` or `__richcmp__`, so `new_ptr != <int>` falls back to identity comparison and is unconditionally true. The guard is therefore always taken and `_grow_allocation_fast_path` is unreachable: every grow runs the slow path, re-reserving the whole range and remapping it, so the buffer's base pointer changes even when a contiguous extension was available. Convert with `int()` before comparing. Reported as defect 2 in NVIDIA#2388. `test_vmm_allocator_grow_dispatches_to_fast_path` stubs `cuMemAddressReserve` to grant the requested address and asserts the dispatch reaches the fast path. It fails on the current code, which records "slow". `test_vmm_allocator_grow_allocation_fast_path` already exercised `_grow_allocation_fast_path` directly, so the helper itself is covered; what was missing was coverage of the dispatch decision that reaches it. This makes a previously unreachable code path live, so it is a behavior change rather than a pure cleanup: successful contiguous grows now preserve the base pointer instead of moving it. Noted in the 1.2.0 release notes. Signed-off-by: Aryan Putta <aryansputta@gmail.com>
57a7a0b to
24f3f6c
Compare
Description
Addresses defect 2 of #2388 (the dead grow fast path). The other three defects in that issue are not touched here, so this does not close it.
modify_allocation()askscuMemAddressReservefor the address immediately after the current range, then checks whether the driver granted that exact address before taking the fast path:new_ptris aCUdeviceptr. That type defines__int__and__repr__but no__eq__or__richcmp__(cuda_bindings/cuda/bindings/driver.pyx), so comparing it against a plain int falls back to identity and is unconditionally unequal. The guard is therefore always taken,_grow_allocation_fast_pathis unreachable, and every grow runs the slow path: a full re-reserve plus remap, with the buffer's base pointer changing even when the driver did grant a contiguous extension.The fix is the
int()conversion suggested in the issue.Behavior change, not a pure cleanup
This makes a previously unreachable path live. After it, a grow the driver can satisfy contiguously preserves the base pointer instead of moving it. That is the documented intent of the fast path, but it has never actually executed, so it deserves explicit attention in review. Added to the 1.2.0 release notes for that reason. The existing test asserting a grown buffer's pointer may change (
test_vmm_allocator_grow_allocation) still passes, since it only requires a non-null handle.Since the fast path becomes reachable for the first time, I checked the one line in it that looked risky.
buf._size = new_sizestill carries aTODO: #2049 This is a real bug, accessing _size which doesn't exist. That TODO is stale on the Cython side:_sizewas moved into acdef publicblock in_buffer.pxdby #2216 and #2049 is closed, so the assignment works on a realBuffer. The# type: ignore[attr-defined]is still required because_buffer.pyidoes not expose_size, so I left both the ignore and the TODO alone rather than widening this PR. Happy to clean that up separately if you want it.Testing
test_vmm_allocator_grow_dispatches_to_fast_pathstubscuMemAddressReserveto grant exactly the address that was requested and asserts the dispatch reaches the fast path. It should fail on currentmain, recording"slow". It follows the stubbing style of the adjacenttest_vmm_allocator_grow_allocation_fast_path.That existing test covered
_grow_allocation_fast_pathby calling it directly, which is why the broken dispatch was invisible: it stays green whether or notmodify_allocationcan actually reach the helper. The gap was coverage of the decision that reaches it.Checklist