Repository navigation
hk, spirv: image texel visibility and the free-memory reserve (split out of #14) - #15
Conversation
… the read
OpImageWrite with MakeTexelAvailable and OpImageRead with MakeTexelVisible
describe the texel of that very access. vtn_handle_image split their
semantics with vtn_split_barrier_semantics, which places MakeAvailable
before the operation and MakeVisible after it. That is right for a release
or acquire atomic, but it put a texel write outside its own availability
operation. nir_lower_memory_model marks only the writes that precede a
MakeAvailable barrier as coherent, so the store stayed non-coherent.
On G13C (M1 Max) a devicecoherent image payload published across
workgroups then failed: dEQP-VK.memory_model.message_passing.ext
u32.coherent.*.{device,queuefamily}.payload_local.image.guard_local.*.comp
(30 cases, deterministic, 30 of 30 in 6 of 6 runs, with and without the
dependency-tracked CDM barrier). The failing and passing shader pairs had
identical barrier and cache-flush sequences; the only difference in the
disassembly was the coherent flag on the image write. Four cache flush
variants did not change the result.
Swap the two sides for ImageRead, ImageSparseRead and ImageWrite. These
carry no Acquire or Release bits, so only the MakeTexel semantics move.
Atomics keep the old placement.
Add opimageread_vis and opimagewrite_avail to the availability and
visibility unit tests. They assert the order, which the existing tests
do not: the available barrier follows the image store and the visible
barrier precedes the image load. Both fail on the parent commit
(barrier position 10 vs load 9, and barrier 14 vs store 15) and pass with
this change; the 27 spirv_tests pass.
Results on G13C (aurora 12.6 prerelease kernel), same driver with and
without the change:
- the 30 cases: Pass in 3 of 3 runs
- memory_model.* and subgroups.basic.* (18,600 cases): 2,283 Pass,
16,317 NotSupported, 0 Fail; 30 Fail -> Pass, 0 Pass -> other
- the 20,683-case bf16, precision, cooperative_matrix and
compute.pipeline list: 0 status changes
Same lists on G13G (M1) and G14C (M2 Max) with and without the change:
the 30 cases pass in 3 of 3 runs, the memory model and subgroup list has
0 status changes (these chips already passed the 30 cases without the
change), and the 20,683-case list has 0 status changes.
heapBudget is advisory and vkAllocateMemory only checked the static heap size. GPU memory cannot be evicted or swapped, so processes that each fit in the heap can take every free page: on a 16 GB M1 two processes filling their heaps (7.55 GiB each at the old default) drove MemAvailable to 0 kB and the kernel OOM killer killed quickshell and the Hyprland unit. Read MemAvailable before creating the buffer object and return VK_ERROR_OUT_OF_DEVICE_MEMORY when an allocation of 1 MiB or more would leave less than max(1 GiB, 2% of RAM) available. Smaller allocations are not checked: below the reserve every vkAllocateMemory would fail, including the compositor's own. Imported memory is not checked, it allocates nothing. If /proc/meminfo is unreadable only the heap limit applies. Concurrent allocators can still overshoot by one allocation each. Allocations are checked from 1 MiB so that per-tensor allocators (tens of MiB per call) cannot walk past the reserve one small step at a time.
Freed device memory is parked in the driver's BO cache and still counts as used in MemAvailable. The cache is trimmed only when another BO is freed (entries older than 2 s) or when creating a BO fails (agx_bo_cache_evict_all in agx_bo_create). The reserve check runs before agx_bo_create, so after an application freed gigabytes the check saw almost no free memory and refused every allocation while the memory sat in the cache: on a 16 GB M1, an MLX process that hit the reserve at 11.75 GiB, freed everything and cleared its own cache could not allocate 1.5 GiB for about 2 s (tiny submissions for 8 s released nothing; the next allocation attempt after one failed bo create succeeded at once and MemAvailable went from 1.05 to 10.75 GiB). When the check would refuse, call agx_bo_cache_evict_all() and read MemAvailable again before returning VK_ERROR_OUT_OF_DEVICE_MEMORY.
 [Medium impact] The PR appears safe to merge; the previous test failure is addressed and no new blocking issue was found. SummaryThis PR puts image visibility barriers before reads and availability barriers after writes. It also adds a free-memory reserve and returns cached buffers before refusing an allocation.
Reviews (2) · Last reviewed commit: "hk: make the memory OOM test independent..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
GREEN at 2a9eda6 Read: the whole PR diff (3 commits, 3 files, +230). Not built or run by me; w7P reports 120 ok and the same 19 failures as the base on a native build, which I did not reproduce. spirv_to_nir.c (f2c2919): the swap for OpImageRead, OpImageSparseRead and OpImageWrite is right by the Vulkan memory model. A write with MakeTexelAvailable needs its availability operation after the write, and a read with MakeTexelVisible needs its visibility operation before the read; vtn_split_barrier_semantics puts MakeAvailable before and MakeVisible after, which is correct for atomics with Release and Acquire bits but the wrong way round for texel operands. Image reads and writes carry no Acquire or Release bits, so exchanging before and after only moves the availability and visibility bits. The block is limited to those three opcodes. One thing the comment does not claim and the code does not do: MakeTexelVisible on sampled-image fetch, gather and sample opcodes (valid in the Vulkan model) is not in this block; I did not check whether those go through another path that has the same ordering, so treat them as not covered by this fix. avail_vis.cpp: the two new tests are hand-assembled SPIR-V 1.5 modules with the VulkanMemoryModel and DeviceScope capabilities; the image operands are 0x600 (MakeTexelVisible | NonPrivateTexel) with a Device-scope constant for the read, 0x500 (MakeTexelAvailable | NonPrivateTexel) for the write. They check the barrier's semantics (MAKE_VISIBLE and MAKE_AVAILABLE), the image memory mode, device scope, no execution scope, and the order against the image load and store through intrinsic_position (visibility before the load, availability after the store). Both would fail on the old order, which is what a regression test needs. hk_device_memory.c (aa95195, 2a9eda6): the reserve is max(1 GiB, total/50) as the comment says; allocations under 1 MiB always pass; if the kernel gives no MemAvailable the check passes; the check sits only in the fresh-allocation path before agx_bo_create, so imports are not affected, and the failure takes the existing fail_alloc exit with VK_ERROR_OUT_OF_DEVICE_MEMORY. The give-back commit first retries after agx_bo_cache_evict_all, which is the right order because cached BOs still count as used. Known limits, all consistent with the comments: the check is advisory and racy between concurrent allocations, MemAvailable is host-wide inside a container, and an application that keeps retrying a refused large allocation will evict the BO cache each time. Not checked: the build, the CTS and any run; the sampled-image variants above. |
The reserve check reads the real available and total memory, so the test failed on a machine with less than about 2 GiB free. Wrap both queries with fixed values. Add cases for the reserve boundary (one byte short is refused before any BO is allocated and the BO cache is given back, exactly enough passes) and for a refusal that the cache give-back turns into a success. Not measured: this test on a machine with low free RAM. Signed-off-by: Joshua Warren <816217+joshuaswarren@users.noreply.github.com>
|
Fixed in MEASURED on a native build of that head: NOT measured: the test on a machine with low free RAM. |
|
GREEN at 276f3cd (4 commits, code read; the head moved from 2a9eda6, the new commit only changes the OOM unit test). Checked from the source at this head:
Not checked: building or running anything (the measured numbers in the description are the author's), and the CTS claims. |
chriskearney
left a comment
chriskearney
left a comment
There was a problem hiding this comment.
Source review at 276f3cd06d1897414ec1face28b54831a55d5944: the follow-up wraps both OS memory queries in hk_memory_oom, with deterministic boundary and cache-retry cases. That addresses the test's dependence on the builder's free RAM.
The production allocation guard and SPIR-V image changes are unchanged from 2a9eda69. New allocations retain the reserve/cache-retry/error-cleanup behavior; imported FDs remain outside the guard. The image barrier change stays limited to reads, sparse reads and writes, with atomics unchanged.
Source approval. The required ci / result still needs to run successfully before merge; the current Mesa CI run is action_required. This does not establish full CTS or hardware/release qualification.
|
Status, 25 Oct. Head MEASURED, native arm64 build of this head on a 16-inch M1 Max: the driver builds, NOT measured: a whole-suite CTS of exactly these three commits, and the M3 Pro. The check that is waiting is the CI approval on this PR. |
Three memory fixes, split out of #14. They can land without the compiler math. The code is the same as in #14. It sits on
integration/aurora147a62f6190.spirv: make image texels available after the write and visible before the read. A storage image write is now made available, and the read makes it visible. On the 16-inch M1 Max this fixes 30 memory model CTS failures.hk: refuse device memory that would leave too little system memory. A free-memory reserve. A refusal returns a typed out of memory error.hk: give the BO cache back before refusing for the free-memory reserve. The buffer object cache is returned first. A refusal is then never caused by cache that can be freed.The imported mappings, the placed-map ownership and the address reservations of this tree are not touched. The changes are in
hk_device_memory.candspirv_to_nir.c, with the unit testavail_vis.cpp.MEASURED:
meson testgives 120 pass and 19 fail. The same 19 fail on the base built the same way (glcpp, glsl warnings, gallium smoke), so none are new.NOT measured: a whole-suite CTS run of exactly these three commits, the memory reserve under real pressure, and the M3 Pro.