MagicBeanDev/Longshot

Watch

Fork 101

fix(vk): read and publish a compute program's pipeline atomically #621

Merged

HumanBean merged 1 commit from jcabarca/Longshot:fix/vk-compute-pipeline-race into main

2026-11-15 19:18:44 +00:00

jcabarca commented

2026-11-15 00:47:40 +00:00

Contributor

Type: fix · Risk: low · Tested on: Windows, AMD Radeon RX 9070 XT · Needs: none

Why

A compute shader's pipeline is built on worker threads while other threads record dispatches and read it, with plain reads and writes and no lock around the first read. That is a data race (undefined behaviour in C++): a dispatch can see a half-published pipeline, or one written in place before it was made, and bind a handle that is not ready.

How to reproduce

Seen on: not seen in the game: found by reading dispatch() in gpu/vk/vk_draw.cpp; a race like this shows only now and then, with no fixed steps
Settings and switches: defaults
Steps: game-free: python -I -B tests/test_compute_pipeline_atomic.py on main (copy the test from this PR); it lists every plain use of compute_pipeline in vk_draw.cpp
Expected: every read and write of a compute program's pipeline is atomic, and the pipeline is published only once made
Actual: on main 14 lines read or write it plainly, and the synchronous build passes &p.compute_pipeline straight to cached_compute(); the test fails

What changed

Every read and write of Program::compute_pipeline in vk_draw.cpp now goes through std::atomic_ref<VkPipeline>, with release stores and acquire loads. dispatch() reads it once for the bind. The synchronous build makes the pipeline into a local and publishes it only once it is made.

vk: dispatch() in gpu/vk/vk_draw.cpp: a cpipe() lambda loads the pipeline with acquire; the checks before ParLock, the synchronous build's check and the bind use it (optimized ? optimized : pipe, keeping main's Intel quick compute path).
vk: the dispatch's build worker (its cache path, its build and its failure callback) and precompile_run() publish with a release store.
vk: the synchronous build calls cached_compute(&ci, &built) on a local and stores built only on success.
tests: tests/test_compute_pipeline_atomic.py (new) fails if any use of compute_pipeline in vk_draw.cpp is not through std::atomic_ref<VkPipeline>(, or if the synchronous build writes it in place.
tests: tests/test_intel_quick_compute.py looks for the two lines this PR changes in their atomic spelling (the publish in the worker build and the bind); what it checks is unchanged. tests/test_jobs_failure.py's fixture gets the VkPipeline alias the failure callback now names.

The bug, step by step:

dispatch() reads p.compute_pipeline (if (!compute_wait && !p.compute_pipeline ...), ParLock plk(!p.compute_pipeline)) before taking any lock.
At the same time the build worker queued by an earlier dispatch writes q.compute_pipeline = made; (or = c; from the cache, = VK_NULL_HANDLE; on failure), and precompile_run() writes p.compute_pipeline = made;.
A plain read concurrent with a plain write of the same object is a data race, which is undefined behaviour; nothing orders the pipeline's creation before the reader sees the handle.
The synchronous build passes &p.compute_pipeline to cached_compute(), so the field holds a value while the driver is still creating the pipeline, and other threads can read it.

Before (dispatch()):

ParLock plk(!p.compute_pipeline);
if (!p.compute_pipeline && !taau_key(p.key)) {
    ...
    if (const VkResult made = cached_compute(&ci, &p.compute_pipeline); made != VK_SUCCESS) {
        ...
        p.compute_pipeline = VK_NULL_HANDLE;

After:

ParLock plk(!cpipe());
if (!cpipe() && !taau_key(p.key)) {
    ...
    VkPipeline built = VK_NULL_HANDLE;   // (published only once made: other threads read it meanwhile)
    if (const VkResult made = cached_compute(&ci, &built); made != VK_SUCCESS) {
        ...
    }
    std::atomic_ref<VkPipeline>(p.compute_pipeline).store(built, std::memory_order_release);

Start reading at cpipe() in dispatch(), then the worker's auto build = [job, on_worker, made_how] and precompile_run().

Measurements

Verdict: it fixes the bug: tests/test_compute_pipeline_atomic.py fails on main (14 plain uses) and passes with this PR (every use atomic), in both interleaved runs. No speed change is expected or measured: an acquire load or release store of a pointer is a plain load or store on x86-64 and ARM64.

Hardware: AMD Radeon RX 9070 XT, Windows; the test itself is game-free (it reads source, it does not use the GPU); Python 3.12
Scene: none, game-free
Method: tests/test_compute_pipeline_atomic.py from this PR against each side's gpu/vk/vk_draw.cpp (a checkout of each commit, the PR's test copied into main's), 2 runs each, interleaved (main, PR, main, PR); the result is pass/fail, so the median is the result both runs gave
How to reproduce: python -I -B tests/test_compute_pipeline_atomic.py in a checkout of each side (copy the test into main's)

	Before: main @ `82886e89`	After: this PR @ `e37f3aa0`
`tests/test_compute_pipeline_atomic.py`, run 1	fails	passes
`tests/test_compute_pipeline_atomic.py`, run 2	fails	passes
Median	fails	passes

run 1, main @ 82886e89:
compute_pipeline used without std::atomic_ref:
9174: p.compute_pipeline = made;
9816: if (use.on) use.pipeline(p.compute_pipeline != VK_NULL_HANDLE, false);
9817: if (!compute_wait && !p.compute_pipeline && !taau_key(p.key) && p.compute_state.load(std::memory_order_acquire) != 2) {
9850: q.compute_pipeline = c;
9869: q.compute_pipeline = made;
9886: p.compute_pipeline = cached;
9892: q.compute_pipeline = VK_NULL_HANDLE;
9917: if (p.failed || !p.compute_pipeline) SKIP_HERE();
9919: ParLock plk(!p.compute_pipeline);
9920: if (!p.compute_pipeline && !taau_key(p.key)) {   // (a stood-in network is never compiled)
9929: if (const VkResult made = cached_compute(&ci, &p.compute_pipeline); made != VK_SUCCESS) {
9932: p.compute_pipeline = VK_NULL_HANDLE;
10000: if (!p.compute_pipeline) SKIP_HERE();
10009: vkCmdBindPipeline(cmd, VK_PIPELINE_BIND_POINT_COMPUTE, optimized ? optimized : p.compute_pipeline);
exit 1
run 1, this PR @ e37f3aa0:
compute_pipeline: 7 uses in vk_draw.cpp, all atomic
exit 0
run 2, main @ 82886e89: (the same 14 lines)
exit 1
run 2, this PR @ e37f3aa0:
compute_pipeline: 7 uses in vk_draw.cpp, all atomic
exit 0

What the test checks:

Every use of compute_pipeline in vk_draw.cpp (comments and longer names like compute_pipeline_info left out) has std::atomic_ref<VkPipeline>( right before it.
cached_compute(&ci, &p.compute_pipeline) is gone: the synchronous build no longer writes the field in place.
It stops with "this test needs updating" if fewer than 5 uses are left, so it cannot pass by the code moving elsewhere.

Flags and settings

None.

Testing

Tested: on this branch, Windows, Python 3.12: test_compute_pipeline_atomic passes (fails on main), and the other checks that read the changed code pass: test_intel_quick_compute, test_jobs_failure, test_driver_faults, test_fallback_loaded, test_fallback_size_policy, test_intel_compiler_gate, test_mesh_fallback, test_mesh_fallback_failed_once, test_module_ids, test_pipeline_state_computes, test_precompile_compute_guard, test_program_cache_one_variant, test_warm_bounded.

Built and played: this commit builds on Windows (llvm-mingw back end) and on Linux (longshot_vk.so, without and with AMD FSR 4). Played on Windows, AMD Radeon RX 9070 XT: the game runs normally.

Not tested: the race itself: it shows only now and then, with no steps that reach it on purpose. test_fsr4_tune was not run to the end: it needs spirv-val, which this PC does not have (it stops the same way on main). Linux was built, not run.

Must be tested: None: the change is the same on every GPU and OS (atomic loads and stores of a pointer), and the game-free test covers it.

Risks

What could break: a missed compute_pipeline use would keep a plain access, but the test fails on any. vk_programs.cpp still writes the field plainly while it loads programs; this PR does not touch it.

Turning it off: revert the commit.

Compatibility: Nothing changes.

Checklist

Everyone

One topic.
The title is a Conventional Commit (type(scope): description) of the status line's type.
Why gives the reason plainly and links the issues that fit (the tracker searched). (no issue fits)
Measurements has a verdict, or one line saying why there is nothing to measure.
Testing names what was tested, what wasn't, and what must still be tested, where and how.
Rebased on current main.
python -I -B tests/run_all.py passes (any failure that also fails on main is named). (the checks that read the changed code, on this PR, Windows: all pass but test_fsr4_tune, which also fails on main here: no spirv-val)
No game files, dumps, build output or generated files.
third_party/ untouched (a new folder with a PROVENANCE.md instead).
The license is OK: GPL-3.0-or-later, nothing incompatible and nothing decompiled.

When it applies

AI assistance recorded with Assisted-by: / Co-Authored-By: trailers.
A game-free test added for the fix, failing on main and passing here.
A check under tests/ that reads changed source updated in this PR, not weakened. (tests/test_compute_pipeline_atomic.py new; tests/test_intel_quick_compute.py follows the atomic spelling, same checks)
Host code builds on both Windows (llvm-mingw) and Linux, with UTF-8 paths. (built on Windows with llvm-mingw and on Linux, longshot_vk.so without and with AMD FSR 4; std::atomic_ref is C++20, which the build already uses (-std=c++20), and vk_memory.cpp already uses it; no OS-specific code or paths)

**Type:** fix · **Risk:** low · **Tested on:** Windows, AMD Radeon RX 9070 XT · **Needs:** none ## Why A compute shader's pipeline is built on worker threads while other threads record dispatches and read it, with plain reads and writes and no lock around the first read. That is a data race (undefined behaviour in C++): a dispatch can see a half-published pipeline, or one written in place before it was made, and bind a handle that is not ready. ## How to reproduce **Seen on:** not seen in the game: found by reading `dispatch()` in `gpu/vk/vk_draw.cpp`; a race like this shows only now and then, with no fixed steps **Settings and switches:** defaults **Steps:** game-free: `python -I -B tests/test_compute_pipeline_atomic.py` on main (copy the test from this PR); it lists every plain use of `compute_pipeline` in `vk_draw.cpp` **Expected:** every read and write of a compute program's pipeline is atomic, and the pipeline is published only once made **Actual:** on main 14 lines read or write it plainly, and the synchronous build passes `&p.compute_pipeline` straight to `cached_compute()`; the test fails ## What changed Every read and write of `Program::compute_pipeline` in `vk_draw.cpp` now goes through `std::atomic_ref<VkPipeline>`, with release stores and acquire loads. `dispatch()` reads it once for the bind. The synchronous build makes the pipeline into a local and publishes it only once it is made. - vk: `dispatch()` in `gpu/vk/vk_draw.cpp`: a `cpipe()` lambda loads the pipeline with acquire; the checks before `ParLock`, the synchronous build's check and the bind use it (`optimized ? optimized : pipe`, keeping main's Intel quick compute path). - vk: the dispatch's build worker (its cache path, its build and its failure callback) and `precompile_run()` publish with a release store. - vk: the synchronous build calls `cached_compute(&ci, &built)` on a local and stores `built` only on success. - tests: `tests/test_compute_pipeline_atomic.py` (new) fails if any use of `compute_pipeline` in `vk_draw.cpp` is not through `std::atomic_ref<VkPipeline>(`, or if the synchronous build writes it in place. - tests: `tests/test_intel_quick_compute.py` looks for the two lines this PR changes in their atomic spelling (the publish in the worker build and the bind); what it checks is unchanged. `tests/test_jobs_failure.py`'s fixture gets the `VkPipeline` alias the failure callback now names. The bug, step by step: 1. `dispatch()` reads `p.compute_pipeline` (`if (!compute_wait && !p.compute_pipeline ...)`, `ParLock plk(!p.compute_pipeline)`) before taking any lock. 2. At the same time the build worker queued by an earlier dispatch writes `q.compute_pipeline = made;` (or `= c;` from the cache, `= VK_NULL_HANDLE;` on failure), and `precompile_run()` writes `p.compute_pipeline = made;`. 3. A plain read concurrent with a plain write of the same object is a data race, which is undefined behaviour; nothing orders the pipeline's creation before the reader sees the handle. 4. The synchronous build passes `&p.compute_pipeline` to `cached_compute()`, so the field holds a value while the driver is still creating the pipeline, and other threads can read it. Before (`dispatch()`): ```cpp ParLock plk(!p.compute_pipeline); if (!p.compute_pipeline && !taau_key(p.key)) { ... if (const VkResult made = cached_compute(&ci, &p.compute_pipeline); made != VK_SUCCESS) { ... p.compute_pipeline = VK_NULL_HANDLE; ``` After: ```cpp ParLock plk(!cpipe()); if (!cpipe() && !taau_key(p.key)) { ... VkPipeline built = VK_NULL_HANDLE; // (published only once made: other threads read it meanwhile) if (const VkResult made = cached_compute(&ci, &built); made != VK_SUCCESS) { ... } std::atomic_ref<VkPipeline>(p.compute_pipeline).store(built, std::memory_order_release); ``` Start reading at `cpipe()` in `dispatch()`, then the worker's `auto build = [job, on_worker, made_how]` and `precompile_run()`. ## Measurements **Verdict:** it fixes the bug: `tests/test_compute_pipeline_atomic.py` fails on main (14 plain uses) and passes with this PR (every use atomic), in both interleaved runs. No speed change is expected or measured: an acquire load or release store of a pointer is a plain load or store on x86-64 and ARM64. Hardware: AMD Radeon RX 9070 XT, Windows; the test itself is game-free (it reads source, it does not use the GPU); Python 3.12 Scene: none, game-free Method: `tests/test_compute_pipeline_atomic.py` from this PR against each side's `gpu/vk/vk_draw.cpp` (a checkout of each commit, the PR's test copied into main's), 2 runs each, interleaved (main, PR, main, PR); the result is pass/fail, so the median is the result both runs gave How to reproduce: `python -I -B tests/test_compute_pipeline_atomic.py` in a checkout of each side (copy the test into main's) | | Before: main @ 82886e89 | After: this PR @ e37f3aa0 | |---|---|---| | `tests/test_compute_pipeline_atomic.py`, run 1 | fails | passes | | `tests/test_compute_pipeline_atomic.py`, run 2 | fails | passes | | Median | fails | passes | ``` run 1, main @ 82886e89: compute_pipeline used without std::atomic_ref: 9174: p.compute_pipeline = made; 9816: if (use.on) use.pipeline(p.compute_pipeline != VK_NULL_HANDLE, false); 9817: if (!compute_wait && !p.compute_pipeline && !taau_key(p.key) && p.compute_state.load(std::memory_order_acquire) != 2) { 9850: q.compute_pipeline = c; 9869: q.compute_pipeline = made; 9886: p.compute_pipeline = cached; 9892: q.compute_pipeline = VK_NULL_HANDLE; 9917: if (p.failed || !p.compute_pipeline) SKIP_HERE(); 9919: ParLock plk(!p.compute_pipeline); 9920: if (!p.compute_pipeline && !taau_key(p.key)) { // (a stood-in network is never compiled) 9929: if (const VkResult made = cached_compute(&ci, &p.compute_pipeline); made != VK_SUCCESS) { 9932: p.compute_pipeline = VK_NULL_HANDLE; 10000: if (!p.compute_pipeline) SKIP_HERE(); 10009: vkCmdBindPipeline(cmd, VK_PIPELINE_BIND_POINT_COMPUTE, optimized ? optimized : p.compute_pipeline); exit 1 run 1, this PR @ e37f3aa0: compute_pipeline: 7 uses in vk_draw.cpp, all atomic exit 0 run 2, main @ 82886e89: (the same 14 lines) exit 1 run 2, this PR @ e37f3aa0: compute_pipeline: 7 uses in vk_draw.cpp, all atomic exit 0 ``` What the test checks: - Every use of `compute_pipeline` in `vk_draw.cpp` (comments and longer names like `compute_pipeline_info` left out) has `std::atomic_ref<VkPipeline>(` right before it. - `cached_compute(&ci, &p.compute_pipeline)` is gone: the synchronous build no longer writes the field in place. - It stops with "this test needs updating" if fewer than 5 uses are left, so it cannot pass by the code moving elsewhere. ### Flags and settings None. ## Testing **Tested:** on this branch, Windows, Python 3.12: `test_compute_pipeline_atomic` passes (fails on main), and the other checks that read the changed code pass: `test_intel_quick_compute`, `test_jobs_failure`, `test_driver_faults`, `test_fallback_loaded`, `test_fallback_size_policy`, `test_intel_compiler_gate`, `test_mesh_fallback`, `test_mesh_fallback_failed_once`, `test_module_ids`, `test_pipeline_state_computes`, `test_precompile_compute_guard`, `test_program_cache_one_variant`, `test_warm_bounded`. **Built and played:** this commit builds on Windows (llvm-mingw back end) and on Linux (`longshot_vk.so`, without and with AMD FSR 4). Played on Windows, AMD Radeon RX 9070 XT: the game runs normally. **Not tested:** the race itself: it shows only now and then, with no steps that reach it on purpose. `test_fsr4_tune` was not run to the end: it needs `spirv-val`, which this PC does not have (it stops the same way on main). Linux was built, not run. **Must be tested:** None: the change is the same on every GPU and OS (atomic loads and stores of a pointer), and the game-free test covers it. ## Risks **What could break:** a missed `compute_pipeline` use would keep a plain access, but the test fails on any. `vk_programs.cpp` still writes the field plainly while it loads programs; this PR does not touch it. **Turning it off:** revert the commit. **Compatibility:** Nothing changes. ## Checklist ### Everyone - [x] One topic. - [x] The title is a Conventional Commit (`type(scope): description`) of the status line's type. - [x] Why gives the reason plainly and links the issues that fit (the tracker searched). (no issue fits) - [x] Measurements has a verdict, or one line saying why there is nothing to measure. - [x] Testing names what was tested, what wasn't, and what must still be tested, where and how. - [x] Rebased on current `main`. - [x] `python -I -B tests/run_all.py` passes (any failure that also fails on `main` is named). (the checks that read the changed code, on this PR, Windows: all pass but `test_fsr4_tune`, which also fails on main here: no `spirv-val`) - [x] No game files, dumps, build output or generated files. - [x] `third_party/` untouched (a new folder with a `PROVENANCE.md` instead). - [x] The license is OK: GPL-3.0-or-later, nothing incompatible and nothing decompiled. ### When it applies - [x] AI assistance recorded with `Assisted-by:` / `Co-Authored-By:` trailers. - [x] A game-free test added for the fix, failing on `main` and passing here. - [x] A check under `tests/` that reads changed source updated in this PR, not weakened. (`tests/test_compute_pipeline_atomic.py` new; `tests/test_intel_quick_compute.py` follows the atomic spelling, same checks) - [x] Host code builds on both Windows (llvm-mingw) and Linux, with UTF-8 paths. (built on Windows with llvm-mingw and on Linux, `longshot_vk.so` without and with AMD FSR 4; `std::atomic_ref` is C++20, which the build already uses (`-std=c++20`), and `vk_memory.cpp` already uses it; no OS-specific code or paths)

jcabarca added 1 commit

2026-11-15 00:47:41 +00:00

fix(vk): read and publish a compute program's pipeline atomically

CI / Conventional Commit title (pull_request) Blocked by required conditions

Details

CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Blocked by required conditions

Details

CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Blocked by required conditions

Details

CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Blocked by required conditions

Details

CI / Workflow lint (actionlint) (pull_request) Blocked by required conditions

Details

Third-party parts / As their notes say (pull_request) Blocked by required conditions

Details

Third-party parts / The same as upstream (pull_request) Blocked by required conditions

Details

PR description / Description follows the template (pull_request_target) Successful in 3s

Details

Website / Website on Cloudflare (pull_request_target) Has been skipped

Details

e37f3aa017

dispatch() read Program::compute_pipeline before taking ParLock while the dispatch's build worker (including its cache path), its failure callback and the precompile's threads wrote it: a data race (undefined behaviour), and the synchronous build wrote the pipeline in place before it was made. Every use in vk_draw.cpp now goes through std::atomic_ref<VkPipeline> (release stores, acquire loads), dispatch() reads it once for the bind, and the synchronous build makes the pipeline into a local and publishes it only when made.

tests/test_compute_pipeline_atomic.py fails on main and passes with the fix; test_jobs_failure.py's fixture gets the VkPipeline alias the extracted callback now names.

Assisted-by: GitHub Copilot

jcabarca changed title from ~~WIP: fix(vk): read and publish a compute program's pipeline atomically~~ to fix(vk): read and publish a compute program's pipeline atomically

2026-11-15 00:56:53 +00:00

mooki added the

labels

2026-11-15 13:56:30 +00:00

MaikelJSQ referenced this pull request

2026-11-15 18:05:02 +00:00

Game: Longshot closed unexpectedly while you were playing #859

HumanBean approved these changes

2026-11-15 19:03:42 +00:00

HumanBean left a comment

Read the diff, the surrounding dispatch()/precompile_run() code, and the other users of Program::compute_pipeline. In vk_draw.cpp every access now goes through std::atomic_ref with release stores and acquire loads. The synchronous build also makes the pipeline into a local and publishes it only on success, which fixes the in-place write by cached_compute(&ci, &p.compute_pipeline). The bind reads the pipeline once into pipe, so the check and the bind can't disagree, and the Intel quick-compute path is kept. The two plain writes in vk_programs.cpp (lines 214 and 239) set it on a program that is not yet published, so they are not a race. The test edits only follow the new spelling and don't weaken the checks. I did not run anything. Approving.

Read the diff, the surrounding `dispatch()`/`precompile_run()` code, and the other users of `Program::compute_pipeline`. In `vk_draw.cpp` every access now goes through `std::atomic_ref` with release stores and acquire loads. The synchronous build also makes the pipeline into a local and publishes it only on success, which fixes the in-place write by `cached_compute(&ci, &p.compute_pipeline)`. The bind reads the pipeline once into `pipe`, so the check and the bind can't disagree, and the Intel quick-compute path is kept. The two plain writes in `vk_programs.cpp` (lines 214 and 239) set it on a program that is not yet published, so they are not a race. The test edits only follow the new spelling and don't weaken the checks. I did not run anything. Approving.

HumanBean merged commit 86abfce535 into main

2026-11-15 19:18:44 +00:00

HumanBean deleted branch fix/vk-compute-pipeline-race

2026-11-15 19:18:44 +00:00

HumanBean referenced this pull request from a commit

2026-11-15 19:18:45 +00:00

fix(vk): read and publish a compute program's pipeline atomically (#621)