Refactor: ask async-DMA provisioning for one flag, not a set of engines - #2089
Merged
Conversation
simpler_provision_dma_workspace took a DmaWorkspaceKind bitmask, but nothing could ever set more than one bit in it: dma_workspace_supported_mask() names a single engine, and the only producer was a ternary over the enable_sdma bool the Python binding already had. The set was speculative generality standing in front of a question nobody was asking. Selecting an engine is not the runtime's decision to take. Kernels choose with get_dma_workspace(args, kind) against every address that was injected, so what a caller actually needs to say is which engines it declines to have provisioned — and there is exactly one. SDMA is declinable because its workspace cannot be obtained without also creating 48 CP-process STARS streams (SdmaWorkspaceManager::Init builds the streams, then makes the 16 KB workspace their descriptor table), and a Worker holding those gets one device-reset attempt after an AICore fault instead of three. No other engine carries that, so no other engine should be conditional. Take a bool from the binding down to DeviceRunnerBase and derive the mask there as supported-minus-declined. The mask survives only below the comm seam, where it addresses per-kind slots and is the right shape. Rejecting an unsatisfiable request has to become explicit. It used to fall out of the unsupported-bits test; a derived mask has no unsupported bits by construction, so a Worker opting into SDMA on sim/a5/hbg would have degraded into an empty set and reached its first run reading a zero workspace address. Check enable_sdma against the supported mask directly instead. Guard the seam the derived mask now feeds. dma_workspace_provision() takes a set and returns one opaque handle, and dma_workspace_release() recovers the concrete provider by casting it; those agree only while at most one bit can be set. That holds today and stops holding when the supported mask widens for URMA, where a second engine would be released as the type of the first with no compile error and no runtime complaint. The check is deliberately dormant now and arms itself exactly when the assumption expires, so widening the mask yields a clear rejection and points whoever widens it at the per-kind handle the seam still needs. The signature guard in test_pipeline_contract.cpp pins parameter order against silent positional breakage. This retypes in place rather than moving, so the alias records what changed and why. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 57 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (10)
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 |
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
simpler_provision_dma_workspacetook aDmaWorkspaceKindbitmask, but nothing could ever set more than one bit in it —dma_workspace_supported_mask()names a single engine, and the only producer was a ternary over theenable_sdmabool the Python binding already had. This replaces the caller-supplied set with that bool and derives the mask insideDeviceRunnerBase.Why a set was the wrong question. Selecting an engine is already the kernel's decision:
get_dma_workspace(args, kind)reads whichever addresses were injected intoGlobalContext, and a kind that was not provisioned reads back 0. What a caller needs to say is which engines it declines, and there is exactly one — SDMA, because its workspace cannot be obtained without also creating 48 CP-process STARS streams (SdmaWorkspaceManager::Init()builds the streams first, then makes the 16 KB workspace their descriptor table), and a Worker holding those gets a single device-reset attempt after an AICore fault instead of three. No other engine carries that cost, so no other engine should be conditional.The mask survives only below the comm seam, where it addresses per-kind slots and is the right shape.
DmaWorkspaceKindanddma_workspace_addr[KIND_COUNT]are untouched — those are what URMA will need.A fail-fast that had to become explicit. Rejecting
enable_sdmaon a platform without SDMA used to fall out of the unsupported-bits test. A derived mask has no unsupported bits by construction, so a Worker opting in on sim / a5 / hbg would have silently degraded to an empty set and reached its first run reading a zero address. There is now a direct check, verified:A dormant guard on the seam.
dma_workspace_provision()takes a set and returns one opaque handle;dma_workspace_release()recovers the concrete provider by casting it. Those agree only while ≤1 bit can be set — true today, false the day the supported mask widens for URMA, where a second engine would be released as the type of the first with no compile error and no runtime complaint. The check is deliberately unreachable now and arms itself exactly when the assumption expires.API change
simpler_provision_dma_workspace's third parameter changes type (uint32_t required_mask→int enable_sdma). This isruntime_c_api.h, socodestyle.mdrule 10 Tier C applies — flagging it explicitly for maintainer review. The only consumer in-tree ischip_worker.cpp;git grepfinds no other caller, and there is no cross-repo consumer of this entry point.ChipWorker::init's async-DMA parameter changes in place (uint32_t→bool) at the same position.test_pipeline_contract.cpppins that signature against silent positional breakage and caught this; the alias is updated with the reason, since this retypes rather than reorders.Testing
pip install --no-build-isolation -e .) — compiles the binding,chip_worker.cpp, bothc_api_shared.cpp,device_runner_base.cppctest -LE requires_hardware -j4, CI's invocation)examples/a2a3/tensormap_and_ringbuffer/prefetch_async_demopasses throughtask-submit— the case that actually provisions the workspace and runs the control-path warmupenable_sdma=Trueon a2a3sim still raises rather than coming up unprovisionedSim scene tests deselect on this box under
--manual exclude, and would not exercise this anyway: on sim the entry point returnsUNSUPPORTED/0 identically before and after. The onboard run above is the meaningful signal.Docs for this subsystem are corrected separately in #2088 (file-disjoint, no dependency in either direction).