Add: the Buffer/Tensor wire ABI and owner-side create_buffer - #1599
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughIntroduces a typed BufferHandle and tensor-view ABI with canonical identities, backend descriptors, validation, blob serialization, Python materialization, worker-owned shared-memory allocation, bindings, documentation, and C++/Python tests. ChangesBufferHandle ABI
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/ut/cpp/types/test_buffer_handle.cpp (1)
71-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
EnumValuesAreFrozendoesn't pinBackendKind::FORK_COW.Every other
BackendKindvalue (0-4) is checked here butFORK_COW = 5is missing, even though the test's stated purpose is to "pin the sizes, enum values, and the blob codec from the outside". Trivial to add for parity with the rest of this test.✅ Proposed addition
EXPECT_EQ(static_cast<uint8_t>(BackendKind::DEVICE_MALLOC), 4); + EXPECT_EQ(static_cast<uint8_t>(BackendKind::FORK_COW), 5); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/ut/cpp/types/test_buffer_handle.cpp` around lines 71 - 82, Add an assertion in BufferHandleAbi.EnumValuesAreFrozen that verifies BackendKind::FORK_COW converts to uint8_t value 5, preserving the existing checks for BackendKind values 0–4.src/common/task_interface/buffer_handle.h (1)
313-328: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
BufferRefBlobView::ref(i)has no bounds check oni.
ref(i)memcpy'ssizeof(BufferRef)bytes atref_bytes + i * sizeof(BufferRef)with no check thati < ref_count(ori >= 0). Current callers (bufferref_blob_descriptors/refs/scalarsintask_interface.cpp) all loopi < view.ref_count, so this isn't exploitable today, but the function is documented as the single shared extraction point for three receive boundaries, so a future caller passing an unchecked index would read out of the validated region.Optional: add an
i < ref_countassert/throw insideref()itself for defense-in-depth.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/common/task_interface/buffer_handle.h` around lines 313 - 328, Add bounds validation to BufferRefBlobView::ref before computing the byte offset or calling memcpy, rejecting indices below zero or greater than or equal to ref_count with the established assertion or exception mechanism. Preserve the existing BufferRef validation and extraction behavior for valid indices.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/buffer-handle-abi.md`:
- Around line 174-186: Add a VMM_WINDOW row to the Backends table, documenting
that it materializes as the device VA carved by allocate_domain and is used for
communication-domain window buffers. Keep it alongside DEVICE_MALLOC and
preserve the table’s wire-ABI completeness statement.
In `@python/simpler/buffer_handle.py`:
- Around line 239-242: Update the layout comment immediately above
_BUFFER_REF_TAIL to use the actual wire sizes: BufferRef 144 B and
BufferHandleDescriptor 88 B, while preserving the existing field description and
size assertion.
In `@src/common/task_interface/buffer_handle.h`:
- Around line 80-96: Update validate_buffer_ref to reject any BufferRef whose
backend_kind is FORK_COW unless access is exactly READ, preserving the existing
address_space/backend_kind validation for all other combinations. Add or extend
C++ validation tests to cover FORK_COW with WRITE and READWRITE access as
rejected, while retaining acceptance for FORK_COW with READ.
---
Nitpick comments:
In `@src/common/task_interface/buffer_handle.h`:
- Around line 313-328: Add bounds validation to BufferRefBlobView::ref before
computing the byte offset or calling memcpy, rejecting indices below zero or
greater than or equal to ref_count with the established assertion or exception
mechanism. Preserve the existing BufferRef validation and extraction behavior
for valid indices.
In `@tests/ut/cpp/types/test_buffer_handle.cpp`:
- Around line 71-82: Add an assertion in BufferHandleAbi.EnumValuesAreFrozen
that verifies BackendKind::FORK_COW converts to uint8_t value 5, preserving the
existing checks for BackendKind values 0–4.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d442b3d1-f5cd-43eb-b613-298f4efd8dd7
📒 Files selected for processing (12)
docs/README.mddocs/buffer-handle-abi.mdmkdocs.ymlpython/bindings/task_interface.cpppython/simpler/buffer_handle.pypython/simpler/worker.pysrc/common/task_interface/buffer_handle.hsrc/common/task_interface/data_type.hsrc/common/task_interface/tensor.htests/ut/cpp/CMakeLists.txttests/ut/cpp/types/test_buffer_handle.cpptests/ut/py/test_buffer_handle.py
💤 Files with no reviewable changes (1)
- src/common/task_interface/tensor.h
b2e2bc9 to
57a89fd
Compare
|
Went through the CodeRabbit findings — all five held up against the current code, none was a false positive. Fixed in 9fe7bb9. The one that mattered:
On the docstring-coverage warning (32% vs 80%): added docstrings where they state a contract that is not evident from the signature — the two wire enums, Verified after the fixes: pyut 913 passed, cpput 67/67, a2a3sim scene tests 43 passed. |
47a6e42 to
202c8c7
Compare
29abadc to
4b74067
Compare
|
Naming direction from the repository-wide audit is now captured in #1676. The relevant rule is: software identifiers use the owning entity ( For this PR, the canonical model should be: The current head does not yet satisfy that model:
The important merge condition for #1599 is not a repository-wide rename. It is that the new ABI does not establish a second public meaning for |
a427a61 to
9aef900
Compare
At L3 and above a task argument is a raw pointer plus a `child_memory` bool, so the receiver has to guess what it was handed. This adds the typed, self-describing replacement: a canonical identity, a backend descriptor, and a strided view, which together let a consumer resolve a buffer exactly across the L3→L2 and L4→L3 boundaries with no side table and no address rewriting. Nothing dispatches a `Tensor` yet — `TaskArgs` still carries the device POD — so this is behavior-neutral. It lands on its own because the byte layout is frozen once it ships, and it should be read in its own context rather than inside the cutover that consumes it. Because nothing dispatches it, `Tensor` stays in `simpler.buffer` and is not re-exported from `simpler.task_interface`. `TaskArgs.add_tensor` still takes a `ChipTensor`, and a public type whose own submit call rejects it is worse than no public type at all; it joins that module in the cutover that makes `TaskArgs` carry it. A test pins the two facts to each other, so whichever moves first fails until the other follows. Three types. `Buffer` is an owned backing with a lifecycle, which stays with the Worker that created it. `Tensor` is the argument a user builds and submits: the buffer descriptor embedded whole, plus a view, and no address — at submit time none exists, since a POSIX_SHM backing maps to a different VA in every process and a DEVICE_MALLOC one is valid only on its owner chip. `ChipTensor` is the POD the L2 runtime ABI reads, which must carry an address because the kernel dereferences it. There is no new transport. A `Tensor` rides the TaskArgs mailbox blob that `write_blob` / `read_blob` already implement; the cutover swaps that blob's element from `ChipTensor` to `Tensor` and moves `TaskArgsView` with it, leaving `ChipTensor` only in `ChipStorageTaskArgs`. A second blob codec beside the existing one would be the same structure twice, so this commit ships the types and their gate and leaves the wire where it is. The names follow codestyle rule 13: an L3+ domain object takes the unprefixed name, the chip context takes the `Chip` prefix, and a public type is spelled the same in C++ and Python. `Buffer` and `Tensor` are therefore global in both languages — no namespace and no per-language alias — which hw-native-sys#1681 made possible by moving the device POD off the global `Tensor` name. `TENSOR_STRIDE_BYTES` and `TENSOR_CHILD_MEMORY_OFFSET`, which describe that POD, become `CHIP_TENSOR_STRIDE_BYTES` / `CHIP_TENSOR_CHILD_MEMORY_OFFSET` so they cannot be read as belonging to the wire type exported beside them. Rule 13 also asks that one public type be one type, so `CanonicalIdentity`, `BufferDescriptor` and `Tensor` are the C++ structs bound directly rather than a Python re-encoding of the same bytes. One layout definition instead of two, one validator instead of a full one and a weaker one, and no `struct.unpack` between a receiver and its arguments. `Buffer` and `ImportRegistry` stay Python — they own a `SharedMemory` and a process-local mapping cache, neither of which is ABI. No bytes cross that binding in either direction: the types expose their fields and not their encoding. Python builds a `Tensor` and receives one already decoded, and the sole path from wire bytes to a `Tensor` is the mailbox blob's, inside C++. Withholding the encoding is what keeps `validate_tensor` a gate — a second way in is a second thing to remember to validate on. It also makes a class of mistake unwritable rather than merely fixed: keying an import registry on packed bytes splits one backing in two the moment their padding differs, and with no packed bytes to reach for, that key cannot be written. The malformed-bytes cases are therefore exercised where they can be built, in the C++ tests. `CanonicalIdentity` is fixed-length with no length field, so hashing and comparison cannot read past it whatever bytes arrive — a structural property rather than one a validator has to enforce. Padding is excluded from equality, from hashing, and from `pack()`, so two decodes of one backing can never key differently; `ImportRegistry` keys on the identity itself for the same reason, and map-once holds however the wire padding arrived. Generation 0 is reserved for uninitialized and rejected. `validate_tensor` is the single gate every trust boundary runs. It bounds `body_len` and `ndims`, checks the address_space x backend matrix, requires strides > 0 and a known dtype, rejects a view extending past its backing, and requires FORK_COW to grant READ only: a copy-on-write page splits on the consumer's first write into a private copy the owner never sees, so a write grant there would be silently unobservable rather than an error. Its descriptor half is `validate_buffer_descriptor`, split out because a descriptor also arrives on its own, with no view attached. `AddressSpace` and `MAX_TENSOR_DIMS` sit where their readers are: the rank bound is shared by both tensor types and lives in data_type.h, while the address space is a `BufferDescriptor` field alone and lives with it in buffer.h. `Worker.create_buffer` allocates an owner-side `Buffer` over POSIX shm, and every one it hands out is registered so teardown can unlink the backing. Release is a retryable cleanup-journal entry alongside the host buffers: a `Buffer` whose close fails keeps its registry entry, so a later `close()` re-drives it instead of leaking the shm silently. Also bounds-checks `get_element_size`, which indexed its table with a raw `u8` that now arrives from the wire.
9aef900 to
491d95a
Compare
…heck (#1703) #1599 froze the byte layout and shipped `validate_tensor` as the receive-side gate every later boundary is meant to run. Three things in it do not hold up, and a frozen ABI is the wrong place to leave them. No field, offset, or enum value changes: every static_assert in buffer.h is untouched. `tensor_extent_bytes` summed `(shapes[i]-1)*strides[i]` into a uint64. Both fields are u32, so a single product already reaches ~2^64 and the multiply by the element size overflows on top of it: the extent wrapped to a small value and `validate_tensor` accepted the view as in-bounds. One dimension is enough — shapes=(2147483649,), strides=(2147483648,), FLOAT32 validated against a 4-byte backing while addressing ~16 EiB — and the path is reachable from Python through `buffer.tensor(...)`. The arithmetic now saturates, and an extent that lands on the saturation sentinel is refused by name rather than compared against `nbytes`, so a descriptor claiming an absurd `nbytes` cannot buy the view back. `tensors_overlap` inherited the same wrap, where the consequence is worse than a rejected argument: two fully overlapping views compare as disjoint, which at the dependency layer is a missed edge rather than an error. Its end offsets saturate too, and its test sets a nonzero `byte_offset` so the end-offset addition is what overflows — with an origin of zero the case passes against the unfixed code and guards nothing. `BufferDescriptor::operator==` bounded a memcmp by an unvalidated `body_len`. It is clamped to DESC_MAX_BYTES, so the one length field in the header cannot bound a read past the array it indexes. The arbitrary-bytes pass asserted magic / generation / body_len / ndims on its survivors but not the footprint invariant, which is why 4096 random blobs never caught the overflow; it asserts it now. The L3+ child check in `create_buffer` counted chip and sub children only, so an L4 whose children are local L3 Workers was refused. A next-level child is a forked process that maps a POSIX_SHM backing by name exactly as a chip or sub child does, so it can consume the buffer; `_next_level_shms` counts now, and the error message names all three shapes. `create_buffer` had no test: the new ones cover the gate in each child shape, the childless L3+ refusal, the L2 leaf that needs no child at all, buffer id uniqueness and the single nonce within one incarnation, and that a failing close is reported while its registry entry stays for the cleanup journal to retry. `wrap_fork_inherited` derived `backend_kind` from `access`, tagging FORK_SHM whenever the grant was not READ. FORK_SHM and FORK_COW are opposite kernel write semantics, not two spellings of one grant, and inferring either from the other makes a read-only MAP_SHARED backing inexpressible: it is tagged FORK_COW, and FORK_COW's READ-only rule then locks it there. The caller holds the mmap and is the only party that knows which it is, so it states the tag; the default pair stays the safe one, FORK_COW with READ. The function had no caller, so nothing depends on the old signature. Four pieces of text described machinery that does not exist. `ImportRegistry.materialize` pointed a caller holding raw bytes at `BufferDescriptor.unpack`, and the test module said it pins a `pack`/`unpack` round trip; neither exists, and withholding the encoding is exactly what keeps construction the only way in. `validate_tensor`'s comment claimed the validator stands behind materialization as well — it does not, since `ImportRegistry.materialize` takes an already-decoded descriptor and adds no endpoint check, and a header that freezes an ABI should not overstate its own gate. `docs/buffer-abi.md` named the wire type `simpler.task_interface.Tensor` while the same page's status note, the module and a test all say it stays in `simpler.buffer` until the cutover. Its `h.shm.buf` sample now records that it is transitional: byte access belongs on the view, since a device backing has no `shm` and code written against one forks by backend. The endpoint x address_space matrix stays open — `materialize` still resolves a DEVICE backing for a host endpoint. That is a behavioural gate with its own test surface, not a comment fix.
The buffer ABI: how L3+ tasks name their data
At L3 and above a task argument is a raw pointer plus a
child_memorybool, so the receiver has toguess what it was handed. This lands the typed, self-describing replacement — a canonical identity, a
backend descriptor, and a strided view — which together let a consumer resolve a buffer exactly
across the L3→L2 and L4→L3 boundaries with no side table and no address rewriting.
User guide:
docs/buffer-abi.md, including the"Why
TensorandChipTensorare two types" section with the two rejected unifications.Nothing dispatches a
Tensoryet —TaskArgsstill carries the device POD — so this isbehavior-neutral. It lands on its own because the byte layout is frozen once it ships, and it should
be read in its own context rather than inside the cutover that consumes it.
What lands here
src/common/task_interface/buffer.hvalidate_buffer_descriptor/validate_tensor+tensor_extent_bytes/tensors_overlap, every offset and size pinned bystatic_assertpython/bindings/task_interface.cpppython/simpler/buffer.pyBuffer(owns a POSIX shm), thewrap_*constructors,ImportRegistry(map-once by canonical identity)python/simpler/worker.pyWorker.create_buffer, released through the retryable cleanup journalThree types.
Bufferis an owned backing with a lifecycle, which stays with the Worker that createdit.
Tensoris the argument a user builds and submits: the buffer descriptor embedded whole, plus aview, and no address — at submit time none exists, since a POSIX_SHM backing maps to a different VA
in every process and a DEVICE_MALLOC one is valid only on its owner chip.
ChipTensoris the POD theL2 runtime ABI reads, which must carry an address because the kernel dereferences it.
Naming — the model from #1676 / #1681
Rebased onto #1681, which moved the device POD off the global
Tensorname. That is what frees theplain name for the L3+ type, so
BufferandTensorare global in C++ and spelled identically inPython — no namespace, no per-language alias, no second public meaning for
Tensor.TENSOR_STRIDE_BYTESandTENSOR_CHILD_MEMORY_OFFSET, which describe the device POD, becomeCHIP_TENSOR_STRIDE_BYTES/CHIP_TENSOR_CHILD_MEMORY_OFFSETso they cannot be read as belonging tothe wire type exported beside them.
Bound, not mirrored — and no bytes cross the binding
CanonicalIdentity,BufferDescriptorandTensorare the C++ structs bound directly rather than aPython re-encoding of the same bytes: one layout definition instead of two, one validator instead of
a full one and a weaker one.
BufferandImportRegistrystay Python — they own aSharedMemoryand a process-local mapping cache, neither of which is ABI.
The types expose their fields and not their encoding: there is no
pack()/unpack(). Pythonbuilds a
Tensorand receives one already decoded, and the sole path from wire bytes to aTensorruns inside C++. Withholding the encoding is what keeps
validate_tensora gate rather than ahabit — a second way in is a second thing to remember to validate on.
Measured on the receive path, decoding a 4-tensor blob into objects: 37 µs → 0.44 µs.
No second transport
A
Tensorrides the TaskArgs mailbox blob thatwrite_blob/read_blobalready implement intask_args.h. The cutover swaps that blob's element fromChipTensortoTensorand movesTaskArgsViewwith it, leavingChipTensoronly inChipStorageTaskArgs. A second blob codecbeside the existing one would be the same structure twice, so this PR ships the types and their gate
and leaves the wire where it is.
Tensoris foundation-only until the wire flipTaskArgs.add_tensorstill takes aChipTensor, soTensorlives insimpler.bufferand is notre-exported from
simpler.task_interface. A public type whose own submit call rejects it is worsethan no public type; it joins that module in the cutover that makes
TaskArgscarry it.test_wire_tensor_stays_off_the_public_submit_surfacepins the two facts to each other — whichevermoves first fails until the other follows.
A latent defect, found and fixed
ImportRegistrykeyed onidentity.pack(), andpack()emitted_pad, which defeats thepadding-insensitive equality and hashing the model rests on: two decodes of one backing landed in two
registry entries, so map-once silently did not hold. Caught with a failing repro first. The registry
now keys on the identity itself; with
pack()gone the mistake is no longer expressible at all,which is a stronger guarantee than the regression test it replaces.
Today the padding is zero end to end, so this is latent — it becomes real the moment the cutover puts
descriptors on a live mailbox.
What is deliberately NOT here
TaskArgsstill carries the device POD, sobuffer.tensor(...)has no consumerand the submit-time checks the doc describes are not reachable.
alloc_shared_tensor,alloc_child_tensor), device-memory ownership movingonto the
Worker, comm-domainVMM_WINDOWbuffers, and the example/scene-test migration.ChipTaskArgs<ChipTensor>naming at the chip boundary (Support: define layered runtime naming #1676 point 4). Worth flagging for whoeverpicks it up:
ChipTaskArgsis already taken by the device-side orchestration arg insrc/{arch}/runtime/*/pto_types.h, while the host-side type isChipStorageTaskArgs.Verification
pyut— 1131 passed, 6 skippedcpput— 81/81 (no-hardware subset);test_bufferis 13 cases, including field-by-fielddecode rejection, a fixed-seed 4096-iteration arbitrary-bytes pass, and padding-insensitive
identity keying
a2a3simanda5simfull scene suites rc=0 on the immediately preceding revision; re-runningon this one (the delta since is binding surface, tests and docs only — no runtime path)
The malformed-bytes cases live in
tests/ut/cpp/types/test_buffer.cpprather than Python, becausewith no
unpack()those states cannot be built from Python at all — which is the point.