cuda_buffer: adopt device memory allocated outside the backend - #16
Open
ayusmans wants to merge 17 commits into
Open
cuda_buffer: adopt device memory allocated outside the backend#16ayusmans wants to merge 17 commits into
ayusmans wants to merge 17 commits into
Conversation
Reap completed read events, reduce synchronization CPU usage, and replace filesystem sockets with abstract Unix sockets.
Expose the CUDA buffer implementation and ROS dependencies so downstream Bazel targets can compile against the public API.
Keep driver API symbols unresolved in the static library so remote workers do not require a host-installed libcuda stub.
Use the toolkit driver target so CUDA buffer consumers link against the stub and load the host driver at runtime.
Expose the experimental tensor interface to Bazel conversion libraries.
Add Bazel target for cuda_buffer See merge request Isaac/github/rosidl_buffer_backends!1
fix: correct CUDA event lifecycle and IPC sockets See merge request Isaac/github/rosidl_buffer_backends!2
Expose the CUDA buffer implementation and ROS dependencies so downstream Bazel targets can compile against the public API.
Keep driver API symbols unresolved in the static library so remote workers do not require a host-installed libcuda stub.
Use the toolkit driver target so CUDA buffer consumers link against the stub and load the host driver at runtime.
Expose the experimental tensor interface to Bazel conversion libraries.
Resolve event lifecycle conflicts in favor of the finalized upstream implementation.
chore: sync GitHub main and preserve Bazel targets See merge request Isaac/github/rosidl_buffer_backends!3
`from_input_buffer`'s promotion branch assumed that a buffer which is not a `CudaBufferImpl` is CPU-backed, and read it with `rosidl::Buffer::data()`. That accessor is CPU-only by contract: it calls `throw_if_not_cpu_backend()` and raises `std::runtime_error` for every other backend. So handing `from_input_buffer` a payload from any third-party non-CPU backend does not cost a copy, it throws. That is reachable today. A transport that delivers device memory under its own `BufferImplBase` -- rmw_halos does -- produces exactly this shape, and a node that sets `acceptable_buffer_backends` to "any" and then calls `from_input_buffer` on what it receives hits it on the first device payload. It will also become reachable from inside this package the day the CUDA backend supports host storage, since the branch keys on the concrete impl type rather than on where the bytes actually live. Use `to_vector()`, which rosidl_buffer documents as the explicit conversion and which is correct for every backend. The CPU case keeps using `data()`: its storage outlives the call, so there is no reason to pay for a staging copy or the synchronize. Only the other branch needs both, because the staging vector is local and the copy has to land before it goes away. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`allocate_buffer()` is the only way to get a CUDA-backed `rosidl::Buffer`, so
every payload has to be born in this backend's VMM pool. A zero-copy transport
cannot work that way: its wire memory is a shared slot it allocated and mapped
itself -- an NvSciBuf attachment, in the case that motivated this -- and the
best a producer can do today is fill a pool block and let the transport copy it
into the slot. One device-to-device copy per message that nothing needs.
Add the other half of the allocator. `adopt_buffer()` wraps memory this backend
did not create and returns an ordinary `rosidl::Buffer` over it: same "cuda"
backend name, same read and write handles, same event ordering, same
`to_vector()` and `clone()`. Nothing downstream can tell the two apart, which
is the point -- an application should not have to know whether its message is
backed by cudaMalloc or by a transport slot.
Two things adopted storage will not do, both because it is on loan:
* It is never freed. `CudaBuffer::adopt()` installs a deleter that releases
the caller's keepalive and frees nothing.
* It is never reallocated. `resize()` narrows in place and refuses to grow,
rather than silently swapping the caller's slot for a pool block and
turning every subsequent "zero copy" into a copy. `shrink_buffer()` is the
supported way to say "I filled less than all of it", which a producer given
fixed-size storage has no other way to express -- `Buffer::resize()` throws
for every non-CPU backend.
The keepalive rides in the deleter rather than in a member of its own, and that
placement is the contract, not an implementation detail. `~CudaBuffer` hands
the pointer to the `BufferRecycler`, which destroys it only after synchronizing
every outstanding read and write event -- so a keepalive released there tells
the owner "done" once the GPU has finished, not when the last C++ reference
drops. A member would be destroyed while a kernel was still reading.
Also factors the write-event creation shared by both paths into
`make_buffer_write_event()`; the pool allocator's copy is unchanged apart from
now calling it.
Tests: five cases that need no GPU, because `adopt()` makes no CUDA calls --
backend identity, keepalive held-then-released, narrow-in-place, growth
refusal through both entry points, and argument validation. One GPU case round
trips real device memory through the handle API and asserts the read handle
hands back the adopted pointer rather than a promoted copy, then frees the
allocation afterwards to prove adoption did not take ownership.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`srcs` was an explicit list, written when the Bazel target was added and never updated when `src/cuda_buffer_impl.cpp` arrived with the process-wide pool fix (f3ee218). That commit moved `get_or_create_global_pool()` out of line and added the definition to the new file; CMakeLists.txt picked it up, BUILD.bazel did not. So the symbol was declared in the header, called from `CudaBufferImpl<T>`, and compiled into nothing. Nothing in this repository noticed, because a shared library links happily with undefined symbols. It fails in the first consumer that builds an *executable* against the header: undefined reference to `cuda_buffer_backend::get_or_create_global_pool()' Glob instead of listing. The failure mode of a stale list here is a link error in somebody else's repository, several commits after the mistake, with nothing in this one to point at. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
2 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.
allocate_buffer()is the only way to get a CUDA-backedrosidl::Buffer, so everypayload has to be born in this backend's VMM pool. A zero-copy transport can't work
that way: its wire memory is a shared slot it allocated and mapped itself — an
NvSciBuf attachment, in the case that prompted this — so the best a producer can do
today is fill a pool block and let the transport copy it into the slot. That's one
device-to-device copy per message that nothing needs.
This adds the other half of the allocator, so a transport can hand its own memory to
cuda_bufferand an application publishes over it with no copy at all. Theapplication can't tell the difference, which is the point: whether a message is
backed by the pool or by a transport slot shouldn't be something node authors think
about.