Refacror: onboard implementation for L3-L2 orch comm data path #1305
Open
ccyywwen wants to merge 8 commits into
Open
Refacror: onboard implementation for L3-L2 orch comm data path #1305ccyywwen wants to merge 8 commits into
ccyywwen wants to merge 8 commits into
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 Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds a direct host-mapped-region protocol for L3↔L2 communication, bypassing mailbox-based payload/counter transfers when available. It refactors cache maintenance and device timing into shared platform headers, converts message-queue/endpoint error handling to typed enums, wires region create/release controls through the worker manager and Python bindings, updates the message-queue example for multi-input windowed processing, and revises related documentation and tests. ChangesL3-L2 Direct Region and Platform Refactor
Estimated code review effort: 4 (Complex) | ~75 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 |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
tests/ut/cpp/common/test_l3_l2_orch_comm.cpp (1)
174-190: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding offset checks for all fields to match existing test thoroughness.
The existing
RequestAndResponseAreFixedSizePodDescriptorsOnlytest checks every field offset forL3L2OrchCommRequestandL3L2OrchCommResponse. The new test only checksmagic_versionandpayload_bytesfor the request, and skipsrequest_bytes,counter_bytes, andl3_host_pidoffsets. Adding these would catch field reordering regressions more precisely.♻️ Optional: add missing offset checks
EXPECT_EQ(offsetof(L3L2RegionCreateRequest, magic_version), 0u); + EXPECT_EQ(offsetof(L3L2RegionCreateRequest, request_bytes), sizeof(uint64_t)); EXPECT_EQ(offsetof(L3L2RegionCreateRequest, payload_bytes), sizeof(uint64_t) * 2); + EXPECT_EQ(offsetof(L3L2RegionCreateRequest, counter_bytes), sizeof(uint64_t) * 3); + EXPECT_EQ(offsetof(L3L2RegionCreateRequest, l3_host_pid), sizeof(uint64_t) * 4); EXPECT_EQ(sizeof(L3L2RegionCreateRequest), 40u); EXPECT_EQ(offsetof(L3L2RegionCreateReply, desc), 0u); EXPECT_EQ(offsetof(L3L2RegionCreateReply, access_profile), sizeof(uint64_t) * 6); + EXPECT_EQ(offsetof(L3L2RegionCreateReply, reserved), sizeof(uint64_t) * 6 + sizeof(uint32_t)); EXPECT_EQ(offsetof(L3L2RegionCreateReply, device_id), sizeof(uint64_t) * 6 + sizeof(uint32_t) * 2); + EXPECT_EQ(offsetof(L3L2RegionCreateReply, export_key), sizeof(uint64_t) * 6 + sizeof(uint32_t) * 2 + sizeof(int32_t)); EXPECT_EQ(offsetof(L3L2RegionCreateReply, backing_shm), sizeof(uint64_t) * 6 + sizeof(uint32_t) * 2 + 4u + 65u); EXPECT_EQ(offsetof(L3L2RegionCreateReply, mapping_bytes), 160u); EXPECT_EQ(sizeof(L3L2RegionCreateReply), 168u);🤖 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/common/test_l3_l2_orch_comm.cpp` around lines 174 - 190, The new fixed-layout test for L3L2RegionCreateRequest/L3L2RegionCreateReply is incomplete compared with the existing RequestAndResponseAreFixedSizePodDescriptorsOnly coverage. In LifecycleCreateWireStructsHaveFixedLayout, add offset assertions for the remaining L3L2RegionCreateRequest fields such as request_bytes, counter_bytes, and l3_host_pid so the test catches any field reordering or padding regressions, using the existing offsetof checks alongside the current static_asserts and size checks.python/simpler/worker.py (1)
1226-1230: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog best-effort cleanup failures.
These direct-region cleanup paths intentionally preserve the original error, but silently swallowing close/release failures makes leaked shared memory, ACL exports, or mappings hard to diagnose. Emit a warning to
stderrwhile continuing best-effort cleanup. Based on learnings, silent exception swallowing should be treated as a diagnostic-consistency issue rather than cited as an active Ruff rule.Also applies to: 1251-1252, 1273-1277, 3810-3817, 3826-3827, 3868-3869
🤖 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 `@python/simpler/worker.py` around lines 1226 - 1230, The best-effort cleanup exception handlers are silently swallowing failures, which makes leaked shared memory, ACL exports, or mappings hard to diagnose. Update the affected cleanup paths around cw.free(region.dev_ptr) and the other direct-region release blocks to catch the exception, emit a warning to stderr with enough context to identify the cleanup step, and then continue preserving the original error flow. Apply the same stderr warning pattern consistently in all listed cleanup handlers rather than using bare pass.Sources: Learnings, Linters/SAST tools
src/common/platform/include/aicpu/l3_l2_message_queue.h (1)
793-799: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
opparameter ofensure_live()is unused.
ensure_live(L3L2QueueOp op)only readserror_.kind;opis discarded via(void)op. Harmless, but the parameter doesn't do anything — worth removing or actually using it for diagnostics in a future pass. Not blocking.🤖 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/platform/include/aicpu/l3_l2_message_queue.h` around lines 793 - 799, The ensure_live(L3L2QueueOp op) helper in l3_l2_message_queue.h ignores its op argument and only checks error_.kind, so either remove the unused parameter from ensure_live and its callers or use L3L2QueueOp in the check/logging path instead of discarding it with (void)op. Keep the behavior unchanged while cleaning up the signature in the L3L2 queue error-handling code.src/common/platform/include/aicpu/l3_l2_orch_endpoint.h (1)
271-289: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBusy-wait polls the timer every iteration, unlike the sibling throttled pattern.
signal_wait()callsdevice_time_now_ticks()on every spin iteration. The message-queue endpoint added in the same cohort (l3_l2_message_queue.h'sInputQueue::peek()/OutputQueue::reserve()) throttles this viaspins & 1023ull == 0before re-reading the clock. On non-__aarch64__buildsdevice_time_now_ticks()goes throughstd::chrono::steady_clock::now(), so calling it unconditionally in a tight compare/timeout loop is comparatively more expensive than the throttled sibling implementation.♻️ Suggested consistency fix
uint64_t start = device_time_now_ticks(); uint64_t frequency_hz = device_time_frequency_hz(); + uint64_t spins = 0; while (true) { int32_t current = load_counter(counter_addr); *observed = current; if (l3_l2_orch_comm::compare_counter(current, cmp_value, cmp)) { return true; } - uint64_t now = device_time_now_ticks(); - if (timeout == 0 || sys_cnt_elapsed_ns(start, now, frequency_hz) >= timeout) { - set_error(...); - return false; + spins += 1; + if (timeout == 0 || (spins & 1023ull) == 0) { + uint64_t now = device_time_now_ticks(); + if (timeout == 0 || sys_cnt_elapsed_ns(start, now, frequency_hz) >= timeout) { + set_error(...); + return false; + } } }🤖 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/platform/include/aicpu/l3_l2_orch_endpoint.h` around lines 271 - 289, The busy-wait in signal_wait() is re-reading the clock on every spin, unlike the throttled pattern used in l3_l2_message_queue.h. Update the loop in l3_l2_orch_endpoint.h’s signal_wait() to only call device_time_now_ticks() periodically (for example, after a fixed spin count like InputQueue::peek() and OutputQueue::reserve()), while still preserving timeout handling through sys_cnt_elapsed_ns and set_error on timeout.
🤖 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 `@python/simpler/l3_l2_orch_comm.py`:
- Around line 265-280: The onboard reply path in validate_region_create_reply
only checks mapping_bytes for SIM_POSIX_SHM, so the descriptor layout can drift
for onboard exports. Update this function to validate reply.mapping_bytes
against the computed total_bytes for all access profiles, using the existing
helper logic around desc, _align_up, and _checked_add_u64, so both SIM and
onboard replies must match the same region layout.
In `@python/simpler/worker.py`:
- Around line 3784-3788: The onboard import path in
_l3_host_mapped_region_import_onboard is using total_bytes from descriptor
fields instead of the reply’s authoritative mapping_bytes, which can truncate
the import request. Update the call site in worker.py to pass
reply.mapping_bytes for onboard imports, matching the sim path and preserving
any padding or metadata in the exported ACL mapping size.
- Around line 1186-1192: The export-key validation in
`_l3_child_onboard_region_export()` handling leaves an already-created ACL
export open when the key is too long. Update the region onboarding flow in
`worker.py` so that if the `len(export_key)` check fails, the code explicitly
closes/rolls back the export before raising the `RuntimeError`, using the same
cleanup path that would normally run after `region.export_key` is assigned.
---
Nitpick comments:
In `@python/simpler/worker.py`:
- Around line 1226-1230: The best-effort cleanup exception handlers are silently
swallowing failures, which makes leaked shared memory, ACL exports, or mappings
hard to diagnose. Update the affected cleanup paths around
cw.free(region.dev_ptr) and the other direct-region release blocks to catch the
exception, emit a warning to stderr with enough context to identify the cleanup
step, and then continue preserving the original error flow. Apply the same
stderr warning pattern consistently in all listed cleanup handlers rather than
using bare pass.
In `@src/common/platform/include/aicpu/l3_l2_message_queue.h`:
- Around line 793-799: The ensure_live(L3L2QueueOp op) helper in
l3_l2_message_queue.h ignores its op argument and only checks error_.kind, so
either remove the unused parameter from ensure_live and its callers or use
L3L2QueueOp in the check/logging path instead of discarding it with (void)op.
Keep the behavior unchanged while cleaning up the signature in the L3L2 queue
error-handling code.
In `@src/common/platform/include/aicpu/l3_l2_orch_endpoint.h`:
- Around line 271-289: The busy-wait in signal_wait() is re-reading the clock on
every spin, unlike the throttled pattern used in l3_l2_message_queue.h. Update
the loop in l3_l2_orch_endpoint.h’s signal_wait() to only call
device_time_now_ticks() periodically (for example, after a fixed spin count like
InputQueue::peek() and OutputQueue::reserve()), while still preserving timeout
handling through sys_cnt_elapsed_ns and set_error on timeout.
In `@tests/ut/cpp/common/test_l3_l2_orch_comm.cpp`:
- Around line 174-190: The new fixed-layout test for
L3L2RegionCreateRequest/L3L2RegionCreateReply is incomplete compared with the
existing RequestAndResponseAreFixedSizePodDescriptorsOnly coverage. In
LifecycleCreateWireStructsHaveFixedLayout, add offset assertions for the
remaining L3L2RegionCreateRequest fields such as request_bytes, counter_bytes,
and l3_host_pid so the test catches any field reordering or padding regressions,
using the existing offsetof checks alongside the current static_asserts and size
checks.
🪄 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
Run ID: b1906a51-80dc-4f47-ba25-3b281ed89fca
📒 Files selected for processing (33)
docs/hardware/cache-coherency.mddocs/l3-l2-message-queue.mddocs/l3-l2-orch-comm.mdexamples/workers/l3/l3_l2_message_queue/kernels/aiv/kernel_queue_transform.cppexamples/workers/l3/l3_l2_message_queue/kernels/orchestration/l3_l2_message_queue_orch.cppexamples/workers/l3/l3_l2_message_queue/test_l3_l2_message_queue.pyexamples/workers/l3/l3_l2_orch_comm_stream/kernels/orchestration/l3_l2_orch_comm_orch.cpppython/bindings/task_interface.cpppython/bindings/worker_bind.hpython/simpler/l3_l2_message_queue.pypython/simpler/l3_l2_orch_comm.pypython/simpler/worker.pysimpler_setup/kernel_compiler.pysimpler_setup/toolchain.pysrc/a2a3/platform/include/aicpu/platform_regs.hsrc/a5/platform/include/aicpu/platform_regs.hsrc/common/hierarchical/worker.hsrc/common/hierarchical/worker_manager.cppsrc/common/hierarchical/worker_manager.hsrc/common/platform/include/aicpu/cache_maintenance.hsrc/common/platform/include/aicpu/device_time.hsrc/common/platform/include/aicpu/l3_l2_message_queue.hsrc/common/platform/include/aicpu/l3_l2_orch_endpoint.hsrc/common/platform/include/common/l3_l2_orch_comm.hsrc/common/platform/include/host/l3_l2_orch_region_access.hsrc/common/platform/onboard/aicpu/cache_ops.cppsrc/common/platform/sim/aicpu/cache_ops.cpptests/ut/cpp/common/test_l3_l2_message_queue.cpptests/ut/cpp/common/test_l3_l2_orch_comm.cpptests/ut/cpp/common/test_l3_l2_orch_endpoint.cpptests/ut/cpp/stubs/test_stubs.cpptests/ut/py/test_worker/test_l3_l2_message_queue.pytests/ut/py/test_worker/test_l3_l2_orch_comm.py
💤 Files with no reviewable changes (1)
- simpler_setup/toolchain.py
Comment on lines
+265
to
+280
| def validate_region_create_reply(reply: L3L2RegionCreateReply) -> tuple[int, int]: | ||
| desc = reply.desc | ||
| if desc.payload_bytes <= 0: | ||
| raise RuntimeError("create_l3_l2_region: reply payload_bytes must be positive") | ||
| if desc.counter_bytes <= 0 or desc.counter_bytes % 4 != 0: | ||
| raise RuntimeError("create_l3_l2_region: reply counter_bytes must be positive and a multiple of 4") | ||
| counter_offset = _align_up(desc.payload_bytes, _REGION_LAYOUT_ALIGNMENT) | ||
| total_bytes = _checked_add_u64(counter_offset, desc.counter_bytes) | ||
| expected_counter_base = _checked_add_u64(desc.payload_base, counter_offset) | ||
| if desc.counter_base != expected_counter_base: | ||
| raise RuntimeError("create_l3_l2_region: reply counter_base does not match fixed region layout") | ||
| if desc.counter_base % _REGION_LAYOUT_ALIGNMENT != 0: | ||
| raise RuntimeError("create_l3_l2_region: reply counter_base must be 64-byte aligned") | ||
| if reply.access_profile == L3L2RegionAccessProfile.SIM_POSIX_SHM and reply.mapping_bytes != total_bytes: | ||
| raise RuntimeError("create_l3_l2_region: sim reply mapping_bytes does not match descriptor layout") | ||
| return counter_offset, total_bytes |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate onboard mapping_bytes against the descriptor layout too.
SIM replies reject mismatched mapping_bytes, but onboard replies can advertise a different export size and still import using computed total_bytes. That lets parent-side bounds diverge from the child’s exported GM region size.
Suggested fix
- if reply.access_profile == L3L2RegionAccessProfile.SIM_POSIX_SHM and reply.mapping_bytes != total_bytes:
- raise RuntimeError("create_l3_l2_region: sim reply mapping_bytes does not match descriptor layout")
+ if reply.mapping_bytes != total_bytes:
+ raise RuntimeError("create_l3_l2_region: reply mapping_bytes does not match descriptor layout")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def validate_region_create_reply(reply: L3L2RegionCreateReply) -> tuple[int, int]: | |
| desc = reply.desc | |
| if desc.payload_bytes <= 0: | |
| raise RuntimeError("create_l3_l2_region: reply payload_bytes must be positive") | |
| if desc.counter_bytes <= 0 or desc.counter_bytes % 4 != 0: | |
| raise RuntimeError("create_l3_l2_region: reply counter_bytes must be positive and a multiple of 4") | |
| counter_offset = _align_up(desc.payload_bytes, _REGION_LAYOUT_ALIGNMENT) | |
| total_bytes = _checked_add_u64(counter_offset, desc.counter_bytes) | |
| expected_counter_base = _checked_add_u64(desc.payload_base, counter_offset) | |
| if desc.counter_base != expected_counter_base: | |
| raise RuntimeError("create_l3_l2_region: reply counter_base does not match fixed region layout") | |
| if desc.counter_base % _REGION_LAYOUT_ALIGNMENT != 0: | |
| raise RuntimeError("create_l3_l2_region: reply counter_base must be 64-byte aligned") | |
| if reply.access_profile == L3L2RegionAccessProfile.SIM_POSIX_SHM and reply.mapping_bytes != total_bytes: | |
| raise RuntimeError("create_l3_l2_region: sim reply mapping_bytes does not match descriptor layout") | |
| return counter_offset, total_bytes | |
| def validate_region_create_reply(reply: L3L2RegionCreateReply) -> tuple[int, int]: | |
| desc = reply.desc | |
| if desc.payload_bytes <= 0: | |
| raise RuntimeError("create_l3_l2_region: reply payload_bytes must be positive") | |
| if desc.counter_bytes <= 0 or desc.counter_bytes % 4 != 0: | |
| raise RuntimeError("create_l3_l2_region: reply counter_bytes must be positive and a multiple of 4") | |
| counter_offset = _align_up(desc.payload_bytes, _REGION_LAYOUT_ALIGNMENT) | |
| total_bytes = _checked_add_u64(counter_offset, desc.counter_bytes) | |
| expected_counter_base = _checked_add_u64(desc.payload_base, counter_offset) | |
| if desc.counter_base != expected_counter_base: | |
| raise RuntimeError("create_l3_l2_region: reply counter_base does not match fixed region layout") | |
| if desc.counter_base % _REGION_LAYOUT_ALIGNMENT != 0: | |
| raise RuntimeError("create_l3_l2_region: reply counter_base must be 64-byte aligned") | |
| if reply.mapping_bytes != total_bytes: | |
| raise RuntimeError("create_l3_l2_region: reply mapping_bytes does not match descriptor layout") | |
| return counter_offset, total_bytes |
🤖 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 `@python/simpler/l3_l2_orch_comm.py` around lines 265 - 280, The onboard reply
path in validate_region_create_reply only checks mapping_bytes for
SIM_POSIX_SHM, so the descriptor layout can drift for onboard exports. Update
this function to validate reply.mapping_bytes against the computed total_bytes
for all access profiles, using the existing helper logic around desc, _align_up,
and _checked_add_u64, so both SIM and onboard replies must match the same region
layout.
Comment on lines
+1186
to
+1192
| export_key = _l3_child_onboard_region_export(dev_ptr, total_bytes, request.l3_host_pid) | ||
| if isinstance(export_key, str): | ||
| export_key = export_key.encode("utf-8") | ||
| export_key = bytes(export_key).split(b"\x00", 1)[0] | ||
| if len(export_key) >= ACL_IPC_EXPORT_KEY_BYTES: | ||
| raise RuntimeError("CTRL_L3_L2_REGION_CREATE ACL IPC export key is too long") | ||
| region.export_key = export_key |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Close the ACL export if export-key validation fails.
_l3_child_onboard_region_export() has already created an export here, but region.export_key is only assigned after the length check. If the key is too long, rollback frees dev_ptr without closing the ACL export.
Proposed fix
export_key = _l3_child_onboard_region_export(dev_ptr, total_bytes, request.l3_host_pid)
if isinstance(export_key, str):
export_key = export_key.encode("utf-8")
export_key = bytes(export_key).split(b"\x00", 1)[0]
+ region.export_key = export_key
if len(export_key) >= ACL_IPC_EXPORT_KEY_BYTES:
raise RuntimeError("CTRL_L3_L2_REGION_CREATE ACL IPC export key is too long")
- region.export_key = export_key📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export_key = _l3_child_onboard_region_export(dev_ptr, total_bytes, request.l3_host_pid) | |
| if isinstance(export_key, str): | |
| export_key = export_key.encode("utf-8") | |
| export_key = bytes(export_key).split(b"\x00", 1)[0] | |
| if len(export_key) >= ACL_IPC_EXPORT_KEY_BYTES: | |
| raise RuntimeError("CTRL_L3_L2_REGION_CREATE ACL IPC export key is too long") | |
| region.export_key = export_key | |
| export_key = _l3_child_onboard_region_export(dev_ptr, total_bytes, request.l3_host_pid) | |
| if isinstance(export_key, str): | |
| export_key = export_key.encode("utf-8") | |
| export_key = bytes(export_key).split(b"\x00", 1)[0] | |
| region.export_key = export_key | |
| if len(export_key) >= ACL_IPC_EXPORT_KEY_BYTES: | |
| raise RuntimeError("CTRL_L3_L2_REGION_CREATE ACL IPC export key is too long") |
🤖 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 `@python/simpler/worker.py` around lines 1186 - 1192, The export-key validation
in `_l3_child_onboard_region_export()` handling leaves an already-created ACL
export open when the key is too long. Update the region onboarding flow in
`worker.py` so that if the `len(export_key)` check fails, the code explicitly
closes/rolls back the export before raising the `RuntimeError`, using the same
cleanup path that would normally run after `region.export_key` is assigned.
Comment on lines
+3784
to
+3788
| handle = _l3_host_mapped_region_import_onboard( | ||
| int(reply.device_id), | ||
| reply.export_key.decode("utf-8", "strict"), | ||
| int(total_bytes), | ||
| # Onboard imports use peer access so one parent policy covers same-device and cross-device |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use the reply’s mapping_bytes for onboard imports.
The sim path imports reply.mapping_bytes, but the onboard path imports total_bytes derived from descriptor fields. If the exported ACL mapping size ever includes padding/metadata, this truncates the import request despite the reply carrying the authoritative mapping size.
Proposed fix
handle = _l3_host_mapped_region_import_onboard(
int(reply.device_id),
reply.export_key.decode("utf-8", "strict"),
- int(total_bytes),
+ int(reply.mapping_bytes),
# Onboard imports use peer access so one parent policy covers same-device and cross-device📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| handle = _l3_host_mapped_region_import_onboard( | |
| int(reply.device_id), | |
| reply.export_key.decode("utf-8", "strict"), | |
| int(total_bytes), | |
| # Onboard imports use peer access so one parent policy covers same-device and cross-device | |
| handle = _l3_host_mapped_region_import_onboard( | |
| int(reply.device_id), | |
| reply.export_key.decode("utf-8", "strict"), | |
| int(reply.mapping_bytes), | |
| # Onboard imports use peer access so one parent policy covers same-device and cross-device |
🤖 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 `@python/simpler/worker.py` around lines 3784 - 3788, The onboard import path
in _l3_host_mapped_region_import_onboard is using total_bytes from descriptor
fields instead of the reply’s authoritative mapping_bytes, which can truncate
the import request. Update the call site in worker.py to pass
reply.mapping_bytes for onboard imports, matching the sim path and preserving
any padding or metadata in the exported ACL mapping size.
ccyywwen
force-pushed
the
l3-l2-comm-datapath-refactor-onboard-impl
branch
3 times, most recently
from
688f053 to
b6699e4
Compare
- Add the direct parent data path design for L3-L2 orch comm - Add an implementation plan with lifecycle controls, sim/onboard migration split, cleanup ordering, and validation scope
- Add lifecycle create/release controls and fixed create wire structs - Implement sim L2 Host owned region allocation with L3 Host mapping helpers - Route sim payload and counter operations through mapped-region helpers - Keep queue staging behavior behind primitive payload transfer - Add focused Python and C++ coverage for lifecycle and mapped access
- Describe L3 Host mapped-region behavior for simulation platforms - Clarify onboard payload buffers and L2 Host-mediated operations - Move queue payload-transfer staging wording to the primitive backend
- Split queue input enqueue payload handling to satisfy ruff branch limits. - Narrow optional L3 Host mapping and SharedMemory buffer types for pyright. - Apply ruff import ordering and test type narrowing.
- Export child-owned onboard GM regions through ACL IPC and import them in the L3 Host parent. - Route onboard payload and counter operations through parent-side direct helpers using ACL copies. - Document the selected counter path for the onboard direct data path.
- Keep ACL IPC export keys as bytes across bindings and worker cleanup/import paths. - Enable ACL IPC peer access only for a2a3 so A5 imports avoid the unsupported flag. - Keep queue unit tests on the legacy fake-client path while direct-path coverage stays in orch comm tests. - Use explicit uint64 overflow checks so macOS C++ UTs reject wrapped payload ranges.
- Remove the persistent L3-L2 child shared-memory service bootstrap and steady-state command path from production code. - Route primitive and queue behavior through the direct L3 Host mapped backend and keep behavior tests for timeout, poison, STOP, ERROR, backpressure, and peer abort semantics. - Drop old service sources and service-specific tests from production and unit-test builds.
ccyywwen
force-pushed
the
l3-l2-comm-datapath-refactor-onboard-impl
branch
2 times, most recently
from
7902bf0 to
7284f7b
Compare
- Release the GIL around primitive L3 Host mapped-region helpers and keep mapped-region close retryable on cleanup failure. - Roll back child regions when create reply decode or validation fails after allocation. - Sync docs and focused unit coverage with the direct mapped data path.
ccyywwen
force-pushed
the
l3-l2-comm-datapath-refactor-onboard-impl
branch
from
7284f7b to
ec9fafe
Compare
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
1 participant
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.
Summary
This PR builds on the previous PR #1291 and finishes
the onboard side of the same direct-data-path model.
Relative to the sim refactor PR, this change:
Extends L3-L2 primitive regions to onboard platforms with ACL IPC.
The L2 Host allocates the child-owned GM region, exports it through ACL IPC,
authorizes the L3 Host process, and the L3 Host imports it for direct parent
operations.
Routes onboard payload and counter operations through the L3 Host direct
backend.
Payload reads/writes and counter notify/test/wait no longer go through the
old L2 Host service command path. Onboard uses ACL copy operations against
the imported GM region; sim keeps using the parent-side mapped backing.
Handles a2a3/a5 ACL IPC differences.
ACL IPC peer access is enabled only where supported, so a5 imports avoid the
unsupported peer-access flag.
Removes the old persistent child service path.
This drops the request/response service structs, service bootstrap/shutdown
C APIs, worker-side service lifecycle plumbing, service implementation, and
service-specific C++ runner tests.
Tightens lifecycle and cleanup behavior.
Region create rolls back child allocations when reply decode/import/validation
fails, mapped-region close remains retryable on cleanup failure, and primitive
helpers release the GIL around blocking mapped-region operations.
Updates Python/C++ tests and docs to cover the direct mapped backend as the
shared model across sim and onboard.
Relationship to the sim refactor PR
The previous PR introduced the public region descriptor model and the direct
L3 Host mapped data path for simulation. This PR preserves that API and swaps
out the remaining onboard-only L2 Host-mediated steady-state path for the same
parent-side direct primitive model.
After this PR, both sim and onboard use the same high-level behavior:
the L2 AICPU endpoint.
covered at the wrapper/unit-test level.