Pixelpipe cache: both rekey bail paths deadlock on cache->lock - #1147
Merged
Conversation
_cache_try_rekey_reuse_locked() runs with cache->lock held -- its caller takes it at pixelpipe_cache.c:2380 and does not release it before the call at :2401, and the function name says so. It takes its reference correctly, through _non_thread_safe_cache_ref_count_entry() (:2243). Both of its bail paths then dropped that reference through dt_dev_pixelpipe_cache_ref_count_entry(), which locks cache->lock unconditionally. The mutex is created with dt_pthread_mutex_init(&cache->lock, NULL) -- default attributes, non-recursive -- and dt_pthread_mutex_lock is a plain pthread_mutex_lock, so the second acquisition by the same thread does not fail, it blocks forever. Both objects resolve to the same _pixelpipe_cache singleton, so there is no second cache to save it. The thread that hangs is a pipeline worker holding the process-wide pixel cache lock, so it does not hang alone: every other pipe blocks behind it at the next cache access. Neither path is common, which is why this has survived. The first fires when a cl_mem payload attached to the entry is still borrowed by a GPU path (:2254) -- the rekey-vs-OpenCL interaction whose neighbouring hazards are issue #817's lineage in CLAUDE.md -- and the second when g_hash_table_steal_extended() fails to return the entry it was asked for (:2266). Both now use the _non_thread_safe_ variant, matching the acquisition three lines above them. Found while surveying this code for the T5 cache-wait ownership move, not by a test: no test exercises a bail path that requires an in-flight GPU reference on an entry being rekeyed. Release, Debug (-Werror), nofeatures build; export A/B 0 differing pixels.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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
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.



Not part of the refactor series — a live deadlock in master, found while surveying this code for T5. Two lines, independent of everything else, so it goes on its own.
The bug
_cache_try_rekey_reuse_locked()runs withcache->lockheld: the caller takes it atpixelpipe_cache.c:2380and does not release it before calling at:2401. The function name says as much, and it takes its reference correctly through_non_thread_safe_cache_ref_count_entry()at:2243.Both bail paths then dropped that reference through
dt_dev_pixelpipe_cache_ref_count_entry()— the variant that lockscache->lockunconditionally.Four facts make that a hang rather than a warning:
cache->lock:2380→ call at:2401, released only at:2393/:2404cache = _pixelpipe_cache(:2372)dt_pthread_mutex_init(&cache->lock, NULL), anddt_pthread_mutex_lockis a plainpthread_mutex_lockThe thread that blocks is a pipeline worker holding the process-wide pixel cache lock, so it does not hang alone — every other pipe stops at its next cache access.
Why it has survived
Neither path is common. The first fires when a
cl_mempayload attached to the entry is still borrowed by a GPU path (:2254) — the rekey-versus-OpenCL interaction whose neighbouring hazards are issue #817's lineage in CLAUDE.md. The second fires wheng_hash_table_steal_extended()fails to return the entry it was asked for (:2266).Both now use the
_non_thread_safe_variant, matching the acquisition three lines above.I checked the rest of the file for the same shape: every other call inside a
_lockedfunction isdt_dev_pixelpipe_cache_wrlock_entry, which takes the entry's rwlock rather thancache->lock— as the routinely-executed success path at:2244demonstrates.Verification
Release, Debug (
-Werror), nofeatures build; export A/B 0 differing pixels. No test covers either path — reaching the first requires an in-flight GPU reference on an entry being rekeyed — so this is verified by reading, not by exercise.🤖 Generated with Claude Code