Repository navigation
honeykrisp: map placed memory at the allocation size - #8
joshuaswarren wants to merge 11 commits into
Conversation
A VK_EXT_map_memory_placed mapping covered the whole BO. The BO cache can return a BO that is larger than the allocation, for example 0x104000 bytes for a 0x100000 byte request. The mapping then covered the page after the allocation in application memory. The conformance test writes guard bytes there and reads them back, and it failed with a crash. Record the length of the live mapping in the BO and use it to unmap. Map a placed mapping with the allocation size, not the BO size. Report VK_ERROR_MEMORY_MAP_FAILED when the map fails, in place of an assert that is compiled out in release builds. A placed mapping can also stay on a BO that goes back to the cache. The next placed map of that BO then returned the old address and made no new mapping. Unmap a placed mapping when its memory is freed, and drop any old mapping before a new placed map. The virtio path takes the new size argument. It was not run.
Review follow-up for the placed map change. Return VK_ERROR_MEMORY_MAP_FAILED if a placed map ends at another address than the one the application asked for, and drop that mapping. The old assert for this was compiled out in release builds. A placed map without a range covers the whole allocation from offset 0. Return an error for any other offset or size. Refuse a placed map when the BO is shared through an import and already mapped, so a mapping that another memory object holds is not torn down. Use one helper for the length of a live mapping in the unmap and the reserve paths. A reserve unmap of a BO with no mapping does nothing.
The placed length now comes from the allocation size. Return an error if it ever exceeds the BO, so mmap cannot map past the GEM object.
|
GREEN at ddfb4cf, with two small notes. I read the full diff against main at 7cda785. I ran no build. Checked:
Notes, neither blocking:
Not covered, as the body says. The shared-BO guard and the offset and size rejection path have no test. Virtio is not run. The Gallium driver is not built. |
|
Both notes are open items. I did not change the branch for either.
|
|
Maintainer routing request for PR 8. This is a request for a merge contact, not a claim that review is finished. It fixes the crash of Question: who can merge into this repository's |
|
Second routing request for PR 8, about 14 hours after the first. It is still a request for a merge contact, not a claim that review is finished. Nothing on the head has changed since my first note above. The review notes and the list of what was run are in the description and the two review comments. Question: who can merge into this repository's main branch? If there is a different place to ask, a link is enough. My account has pull access only, so I cannot merge or mark anything ready. |
|
Status, 25 Oct: no review since I asked for a merge contact. This is the base of the whole-suite run of #6: without it the suite crashes in |
|
Status, 25 Oct, for the landing order (#10 first, then #8, then #9, then #6). Head MEASURED: it is in the control arm of the whole-suite CTS of the #6 stack on a 16-inch M1 Max and a 13-inch M1 (main plus #8, #9 and #10 against that plus #6). The memory_model and map_placed groups ran without a Pass to Fail change from the control arm to the candidate arm. NOT measured: a CTS run with only this change against main. |
|
Batch 1 for main: #10, then #8, then #9, in that order. All three apply to main at
Stack check, one boot on a 13-inch M1, These are targeted checks, not the full conformance suite. The CTS binary's source commit is not independently verified. #6 is not part of this batch. Its run over the touched test groups at its current head has not finished, and three The author account on these PRs has pull-only access, so a maintainer has to merge. Tell us if you want a different order or a different check first. |
|
@maralcbr could you review this current head for landing after #10? The exact-head test evidence and remaining scope limits are in the PR body. The requested landing order is #10, then #8, then #9. Our account cannot assign formal reviewers or merge here; please route this to a main-branch maintainer if needed. |
chriskearney
left a comment
chriskearney
left a comment
There was a problem hiding this comment.
A placed map can invalidate a live image's internal host-copy pointer at ddfb4cf.
A non-sparse image bind stores plane->map = agx_bo_map(mem->bo) + offset in hk_image.c:1334. That internal mapping does not set mem->map. A subsequent application vkMapMemory2KHR with VK_MEMORY_MAP_PLACED_BIT_EXT can therefore reach this call while the image is still bound. The new unconditional agx_bo_unmap(bo) removes that internal mapping and replaces bo->_map, but the image keeps its old pointer. The host-image-copy paths later dereference planes[plane].map (for example hk_image.c:1444 and :1523), which can now be unmapped or reused memory.
The refcount check does not cover this case: binding the image keeps the memory object alive through the application's lifetime rules; it does not create another BO reference. This is distinct from main's existing wrong-address behavior when a placed map finds a cached mapping. Replacing the mapping fixes that address result while introducing a dangling pointer for this valid lifetime combination.
Please keep the internal image mapping independent of the application mapping, or otherwise preserve/reestablish every retained pointer, and add host-image-copy coverage with a placed map after image binding and after unmap. The placed-map contract does not invalidate a bound image when an application maps its memory.
This finding is from source review, not a new device crash or a CTS result. Independent runtime reproduction and the shared regression benchmarks are still pending; their results will be kept separate from this source finding.
| agx_bo_map_placed(struct agx_bo *bo, void *fixed_addr, size_t size) | ||
| { | ||
| if (fixed_addr) | ||
| agx_bo_unmap(bo); |
There was a problem hiding this comment.
This unmap can invalidate hk_image_plane.map, retained when the same allocation was bound to an image. mem->map is still NULL for that internal map and the BO refcount can still be 1, so neither guard above excludes the sequence. Please preserve an independent internal mapping or repair the retained image pointers.
|
Confirmed from the source path: the BO mapping can be retained by a bound image even when mem->map is NULL, so the refcount guard is insufficient. I am withdrawing the READY status and adding a regression for image binding followed by placed map, host copy and unmap before changing the shared mapping path. No runtime reproduction or device result is claimed yet. |
A bound image retains an internal host-copy mapping (hk_image_plane_bind, plane->map = agx_bo_map(mem->bo) + offset) with no mem->map and no BO reference, so a later application placed map reaches agx_bo_map_placed, drops the BO mapping, and leaves the retained plane pointers dangling for the host-copy paths. The new libasahi_placed_map_test drives the real agx_device.h mapping code with the exact bind, placed-map, unmap and reserve expressions from hk_image.c and hk_device_memory.c. The only mock is the kernel mmap, which hands back anonymous pages. Cases: placed map then plain unmap, placed map then VK_MEMORY_UNMAP_RESERVE_BIT_EXT, and two images aliased on one memory object. Fails at ddfb4cf; passes once the internal mapping is independent of the application mapping.
Review follow-up on the CPU fixture: back the BO with one tmpfile (ftruncate to the BO size) and hand out MAP_SHARED mappings, so the internal image mapping, the application mapping and the placed mapping are separate mappings of the same BO contents. The fixture now checks coherence in both directions (retained vs earlier application mapping, placed mapping vs bind-offset contents) in addition to lifetime after unmap. The placed target keeps its PROT_NONE reservation mapped until the MAP_FIXED placed map replaces it. Every resource failure now FAILs the case; the SKIP paths are gone. The shim tracks and cleans up all mappings and the backing file per case. A map-before-bind control case covers the application-map-then-bind order that the proposed fix's stash logic must handle. Case bodies collapse into one data-driven runner; no new framework.
A bound image retains a BO mapping independently of VkDeviceMemory.map. Replacing that mapping for a placed application map left host-copy pointers dangling even with one BO reference. Give internal and application mappings separate slots. Return mmap results from the native and virtio primitives instead of swapping shared state; release both mappings with the BO and preserve placed-map size/error paths. The four CPU regression cases failed on fixture head 9c3850f and now pass: plain unmap, reserve unmap, aliased images, and application-map-before-bind. They use real mapping helpers with shared file-backed kernel mmap emulation. Vulkan entrypoint/device coverage and full driver compilation remain separate qualification gates; no hardware readiness claim.
 [Medium impact] The source changes appear safe to merge, with a non-blocking test coverage gap; the PR's separate qualification gates still apply. Findings
SummaryApplication mappings now belong to each
This review checked source only. It does not replace the device qualification or landing order stated in the PR description.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (mem->map && mem->placed_map) | ||
| agx_bo_unmap(mem->bo); |
There was a problem hiding this comment.
hk_FreeMemory can now unmap memory still used by another imported memory object. Export and reimport a BO, placed-map the first object while _map is empty, then normally map the second object. The shared-BO check allows this order, and the normal map returns the same _map.
Freeing the first object now unmaps that address even though the second object remains mapped and holds a BO reference. Later reads or writes through the second object's pointer can crash. Give each memory object its own application mapping, or prevent this sharing before returning the pointer.
There was a problem hiding this comment.
Confirmed in the source: after A creates a placed application map, B can normally map the same BO and receive A's cached pointer; freeing A then unmaps B's live mapping. This head remains NOT READY. I am adding a failing-first two-object/free-one regression and removing BO ownership of application mappings: each VkDeviceMemory will own its app pointer and length, while the BO keeps its stable driver/decoder mapping. No device reproduction is claimed, and no re-review request is being sent before the regression and fix are verified.
|
The follow-up tracing and imported-memory findings expose a remaining ownership error in this head: a BO can be shared by multiple VkDeviceMemory objects, so it cannot own their replaceable application mapping. I am keeping this NOT READY and moving application pointer/length ownership to each memory object, while restoring the stable BO mapping used by driver and decoder callers. New trace and two-importer lifetime regressions will run failing-first. The earlier four CPU cases pass and the full local driver compiles, but neither covers these follow-up cases or replaces device qualification. |
chriskearney
left a comment
chriskearney
left a comment
There was a problem hiding this comment.
At c70e537, trace mode crashes at the first queue submission. This confirms at runtime the tracing finding above, which your 23:25 note already plans to fix.
How it was run: an x86_64 host with Mesa's asahi drm-shim (G13G profile; no kernel, no GPU), and Honeykrisp built from this exact head (release, NDEBUG). A small Vulkan program records a transform feedback draw and calls vkQueueSubmit with ASAHI_MESA_DEBUG=trace.
| Driver | Result |
|---|---|
main 7cda7850ed |
decode completes; 25,311-byte agxdecode dump |
previous head ddfb4cf114 |
decode completes; same dump size |
this head c70e5372 |
SIGSEGV in the first vkQueueSubmit; the dump stops at 78 bytes |
Backtrace: __agxdecode_fetch_gpu_mem (decode.c:136) ← agxdecode_stateful ← agxdecode_drm_cmdbuf ← queue_submit ← vkQueueSubmit.
Cause:
- With tracing on,
agx_bo_createcallsagx_bo_map(), thenagxdecode_track_alloc()stores a copy of the BO (agx_bo.c:397-399). agx_bo_map()now fills only_imap, so the stored copy has_map == NULL.- decode.c:102, :136 and :976 still read
mem->_map/cmdbuf->_map.
Control: a local scratch change that reads _imap before _map at those three lines removes the crash, and the dump is byte-identical to main's. The scratch change only tests the cause; it is not a proposed final shape. The program's transform feedback data check fails under drm-shim on every driver, because nothing executes without a GPU; that check is not part of this result.
The crash is the same in a local merge of main with #10, this head and #9 (d38e71924527392fca5b09c23b9aa6017c52bc2b).
Separately, in the same host setup this head fixes the image-bind/placed-map case from the earlier review. It also fixes host copy after vkUnmapMemory2 and after VK_MEMORY_UNMAP_RESERVE_BIT_EXT, which main fails too. All 6 cases pass, with the image contents read back. These are host CPU-path results, not device or CTS results. Approval still needs the trace path fixed and the two-importer case the bot raised.
…ced-map regression Follow-up findings on the independent-slots change: decode.c reads bo->_map directly (agxdecode_grab_mapped, fetch_gpu_mem, cmdstream) and wrap.c populates ._map at BO creation, so the stable BO mapping must keep existing and stay pointer-stable across application maps; and two memory objects sharing one imported BO must map independently, since an ordinary map currently returns another importer's placed mapping and that importer's unmap destroys it. Two new cases on the same tmpfile-backed shared kernel seam: the decoder-style stable mapping must match agx_bo_map, stay coherent with a placed application mapping and survive placed map + unmap with an unchanged pointer; and two importers (refcnt 2) must get distinct mappings with mutual coherence, where the first importer's unmap leaves the second's mapping valid. Application-map call sites of these cases adapt to the per-memory mapping shape in the production fix commit; assertions do not change. All resource failures FAIL; no SKIP paths.
|
Observational benchmark of the current heads on two M1 Macs. This is not an approval. At What was compared
Workload: the Vulkan benchmark helper from omarchy-m-test GPU suite 1.
Results: median change of the control against the baseline. These are latencies, so lower is better. † marks a case where the min–max ranges of the two arms don't overlap.
Limits
|
…regression The user-required free scenario: two memory objects share one imported BO and one is freed. The new case runs the BO-relevant hk_FreeMemory sequence for the freed object - the placed-unmap condition of hk_device_memory.c:283 and the real agx_bo_unreference refcount drop - and requires the survivor's mapping to stay valid and coherent, with the shared BO left at refcount 1. The heap accounting, ext-BO tracking and vk_device_memory_destroy tail of hk_FreeMemory need a live vk_device and stay behind the admitted device gate.
The free-path case calls the real agx_bo_unreference, which lives in agx_bo.c in libasahi_lib; the test target never linked the library, so the parent's red run failed at link with an undefined reference instead of exercising the refcount. Mirror the libasahi_tests dependency pattern: link_with libasahi_lib, which propagates its own dependency closure (dep_libdrm, idep_nir, libvdrm, libasahi_decode, idep_libagx, idep_libpoly) to the consumer. No test or assertion changed.
Parent-observed red at e14884a59caa (pr8-owner-red3.log): the stable BO mapping was absent because agx_bo_map populated only _imap, and two memory objects sharing an imported BO shared one replaceable mapping - importer B's ordinary map returned importer A's placed mapping, so freeing or unmapping A destroyed B's pointer. Restore bo->_map as the single stable driver-internal mapping and delete the _imap slot: agx_bo_map creates and returns _map (whole BO), decode.c and wrap.c keep reading and populating it directly, the BO cache keeps it as before, and agx_bo_free releases it. Application mappings leave the BO entirely: hk_MapMemory2KHR obtains a fresh mapping per memory object from the returning bo_mmap primitive (placed maps at the requested fixed address, ordinary maps whole-BO at a kernel-chosen address) into the existing mem->map plus a new mem->map_size; hk_UnmapMemory2KHR plain and reserve paths, the placed-address mismatch error path and vkFreeMemory release exactly that object's mapping. The shared-BO guard is removed rather than layered: with no replaceable mapping on the BO there is nothing left for it to protect, and two importers now map independently with coherence coming from the shared pages. Offset/size placed contract, mmap failure handling, the already-mapped check and error results are unchanged. The regression keeps its assertions and adapts only the application call expressions to the per-memory shape. Red lineage cited in the test header (8af285bd30c, e14884a59ca). No device run: hardware, CTS and precision gates hold.
The stable BO mapping always covers the whole BO. Its length is already bo->size. Application mapping lengths belong to each memory object, and the old placed-map flag has no readers after the ownership fix.
| munmap(map_a, ALLOC_SIZE); | ||
| map_a = NULL; | ||
| agx_bo_unreference(&m.dev, &m.bo); |
There was a problem hiding this comment.
The new importer tests create and remove application mappings themselves through ops.bo_mmap and munmap. They never call hk_MapMemory2KHR, hk_UnmapMemory2KHR, or hk_FreeMemory, where the ownership fix lives. If those functions start reusing or unmapping another object's pointer again, these tests would still pass.
Move the per-object mapping code into shared helpers that these tests call, or add tests through the Vulkan entrypoints.
There was a problem hiding this comment.
CHANGES at 4e269c5.
Confirmed: the current CPU import cases repeat map and unmap expressions. They do not guard the Vulkan memory code itself. I am moving that code into helpers used by both the driver and tests. Full import and teardown still need a device run.
The independent host checks and native suite results stay bound to this head and the stated union. They are not a GPU or CTS pass. This PR stays not ready.
There was a problem hiding this comment.
Thanks for confirming the gap and keeping the PR not ready. At 4e269c57, the helper refactor is not in this head yet: hk_MapMemory2KHR/hk_UnmapMemory2KHR still perform the mapping operations directly, and the CPU importer/free cases still call ops.bo_mmap, munmap, and agx_bo_unreference themselves. The tests therefore still don’t guard the Vulkan memory logic I flagged.
Once the shared helpers are in place and the CPU cases call them, that should address this specific test-coverage concern. The live-device checks and other stated qualification gaps remain separate gates.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
|
Independent follow-up at 4e269c5. The source correction addresses the mapping-ownership findings: the BO retains its stable internal/decoder mapping, while each VkDeviceMemory owns its application pointer and length. This is a source/host validation update, not device qualification or merge approval. On an x86_64 address-honouring drm-shim, using the same preserved repro programs and controls:
Separately, the exact clean author union of #10 b87e745, this #8 head, and #9 a8960b7 (local tip 9460a17b877, tree c0404e39) built natively for aarch64 with all 49 registered Meson tests passing, including both Asahi regression targets. No build overlay was needed: #9 now supplies its own idep_nir dependency. Production and test package payloads are byte-identical, independently checked. Still untested on this head: actual Mac/GPU execution, CTS, and the complete heap-accounting/external-BO/Vulkan teardown path for shared imported memory. The earlier observational benchmark stays bound to the old stack and does not qualify this successor. The PR's current device-qualification hold remains. |
Current status: NOT READY (30 October)
Head
4e269c57f09a926a4e785c5e48788325c8b36846.The earlier ownership error is fixed at the source level. A BO shared by several memory objects no longer owns a replaceable mapping. The BO keeps one stable internal mapping for the driver and decoder. Each memory object now owns the mapping it hands to the application. An import pair maps independently, and freeing one leaves the other valid.
A CPU regression suite covers placed maps, reserve unmaps, aliases, the stable mapping, import pairs and the free path. It fails on the previous head and passes on this one. The real reference count is exercised. The suite is CPU only.
The heap accounting, external BO tracking and Vulkan teardown tail of FreeMemory still need a live device. Placed-map-after-bind needs device coverage. This head has no device results. Earlier device results below were measured on older heads and do not cover this change.
Earlier description and evidence
Current status: NOT READY TO MERGE (29 October)
Head
ddfb4cf114a6e72b690581ff6fb2fe8447d11ffa: READY status withdrawn following Chris's source-review finding. A placed application mapping can unmap the BO mapping still retained in a bound image's host-copy pointer;mem->map == NULLand a single BO reference do not exclude this lifetime.Required next steps: reproduce the image-bind → placed-map → host-copy → unmap sequence, preserve the internal mapping independently or repair every retained pointer, and rerun the relevant regressions and exact-head review. The finding is source evidence, not a measured device crash. Earlier targeted results below do not cover this sequence.
Landing order remains #10 first, then corrected/qualified #8, then #9. #9 must wait for #8's correction and qualification.
Historical evidence and original PR description
READY TO MERGE after #10.
Head:
ddfb4cf114a6e72b690581ff6fb2fe8447d11ffa.GitHub reports MERGEABLE against
main: no text conflicts. This is not a test pass.The exact-head tests and GREEN source review below cover this commit. They are targeted checks, not full CTS. Merge it after #10 lands. Preserve this unchanged head; no new run or rebase is needed just to update its status. An authorized maintainer can land it next in the agreed order.
Earlier review, source and test notes are kept below with their original bindings. They do not change this status.
What this fixes
The conformance test
dEQP-VK.memory.map_placed.unmap_reserve.size_1048576crashes the process when it runs after many other memory tests. Four cases in the neighbouringexact_sizegroup fail in that run.omacom/mesamain at7cda7850edhas this crash.The cause is in the placed map path of
VK_EXT_map_memory_placed:vkFreeMemory.What this changes
vkFreeMemoryunmaps a placed mapping that its own memory object holds, before the BO goes back to the cache.VK_ERROR_MEMORY_MAP_FAILED. The old assert for this is compiled out in release builds. The same error covers a placed map at a different address than the one requested, a placed map with an offset other than 0 or a size other than the allocation size, and a placed map of a BO that an import shares and that is already mapped. The driver reportsmemoryMapRangePlacedas false, soVUID-VkMemoryMapInfo-flags-09571andVUID-VkMemoryMapInfo-flags-09572make offset 0 and sizeVK_WHOLE_SIZEor the allocation size the only valid placed map.Test results
The chunk runs use the library bytes of the built package, one run per arm. Control is the package of main at
7cda7850ed, built with the same recipe as the candidate. Candidate is the package of this branch atddfb4cf114.The first 1000 cases of the memory group (chunk 0 of its 7 chunks), the chunk where the control crashes. The 16-inch M1 Max and the 13-inch M1 give the same table.
Exactly 5 of the 1000 rows differ on both chips. No other row differs.
map_placed.exact_size.size_4096map_placed.exact_size.size_8192map_placed.exact_size.size_65536map_placed.exact_size.size_1048576map_placed.unmap_reserve.size_1048576All 13
map_placedcases pass on the candidate.A small test program, run on the 16-inch M1 Max, one boot, with a native build of this branch, one test per process. It covers an ordinary map, an ordinary map then a reserve unmap, a cached larger BO with a placed map (plain and reserve unmap), a placed map after a free without unmap, and an application page at a freed placed address. The control crashes on the cached-BO case with a 0x104000 byte placed map, returns the wrong pointer after a free without unmap, and leaves the placed range mapped after
vkFreeMemory. The branch passes all six tests with placed lengths of 0x100000 and 0x10000.What this does not show
The conformance test expects exact page boundaries here (see
vktMemoryMapPlacedTests.cpp). I found no sentence in the extension text that requires it.Observational two-M1 comparison (not approval)
Chris's comparison binds to main
7cda7850edversus the old control stackd38e7192452(#10b87e7456, #8c70e5372, #911ae82d8) plus build-only overlay645169e3901adding #9's missingidep_nir. It is not evidence for a future ownership-repair head.The 13-inch M1 and M1 Air each ran seven common-path workloads, one warm-up and three measured rounds, plus clean/corrupted compute controls: 30 valid runs per arm, all four arms 30/30. Driver attribution, clean environment, no new kernel fault lines and drained process groups were checked per run. Same aarch64 recipes passed 47/47 registered tests on main and 49/49 on the control. Overall lab jobs still failed a separate known provider-attribution check; those failed verdicts are not being relabelled as passes.
Median latency change of control versus baseline (negative is lower):
These are descriptive n=3 measurements without CPU frequency/affinity control, not statistical significance or equivalence. The source comment identifies which min/max ranges did not overlap. No placed maps, host image copies, capture/replay buffers or transform feedback were exercised, so this qualifies none of the three fixes and does not close the tracing or imported-object defects. The external-display path was not tested. Both Macs were restored afterward. Post-fix correctness and any new-head observations must retain their own source bindings.