Commit: 1652db3df328b39771de42b2820a2d5b07cc1d32
Parent: 76eb99259d42ec6f886cfd81aaa4e78f9c1afb2e
Author: Randy Palamar
Date: Fri, 13 Jun 2025 10:34:44 -0600
core: resolve lock up bug
The bug went as follows:
1. Library wants to dispatch a compute
- Takes lock and waits trying to acquire lock again
2. Main Thread Starts a compute
3. Compute thread releases the lock and starts computing
4. Library acquires lock.
-- TIMING --
5. Main thread sees that lock is high and tries to start a compute.
- sets internal flag indicating that it shouldn't start a new compute
until compute thread tells it that it has started this one
6. Library releases the lock.
7. Compute Thread empties the queue _BEFORE_ library has sent the next compute and goes to sleep
-- TIMING --
8. Library uploads new data and tries to start a compute.
9. Main thread sees that lock is high but flag indicating that it has already started compute
is still set
- compute thread never had reason to clear it before going to sleep since
there was no compute when it went to sleep.
10. Compute thread never wakes up from this path again. (it would wake up and clear the flag
if we used one of the other ways of waking it up, for example reloading a compute shader)
This can be avoided by dropping the starting_compute flag and just
using the compute worker asleep flag as the guard. Obviously the
race still exists but I don't know if its critical to be fixed.
Diffstat:
4 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/beamformer.c b/beamformer.c
@@ -588,7 +588,6 @@ complete_queue(BeamformerCtx *ctx, BeamformWorkQueue *q, Arena arena, iptr gl_co
ctx->os.shared_memory_region_unlock(&ctx->shared_memory,
sm->locks, work->lock);
}
- atomic_store_u32(&ctx->starting_compute, 0);
i32 mask = 1 << (BeamformerSharedMemoryLockKind_Parameters - 1);
if (sm->dirty_regions & mask) {
@@ -729,7 +728,7 @@ DEBUG_EXPORT BEAMFORMER_FRAME_STEP_FN(beamformer_frame_step)
BeamformerSharedMemory *sm = ctx->shared_memory.region;
BeamformerParameters *bp = &sm->parameters;
- if (sm->locks[BeamformerSharedMemoryLockKind_DispatchCompute] && !ctx->starting_compute) {
+ if (sm->locks[BeamformerSharedMemoryLockKind_DispatchCompute] && ctx->os.compute_worker.asleep) {
if (sm->start_compute_from_main) {
BeamformWork *work = beamform_work_queue_push(ctx->beamform_work_queue);
ImagePlaneTag tag = ctx->beamform_frames[ctx->display_frame_index].image_plane_tag;
@@ -759,7 +758,6 @@ DEBUG_EXPORT BEAMFORMER_FRAME_STEP_FN(beamformer_frame_step)
}
atomic_store_u32(&sm->start_compute_from_main, 0);
}
- atomic_store_u32(&ctx->starting_compute, 1);
ctx->os.wake_waiters(&ctx->os.compute_worker.sync_variable);
}
diff --git a/beamformer.h b/beamformer.h
@@ -171,7 +171,6 @@ typedef struct {
GLParams gl;
uv2 window_size;
- b32 starting_compute;
b32 should_exit;
Arena ui_backing_store;
diff --git a/helpers/ogl_beamformer_lib.c b/helpers/ogl_beamformer_lib.c
@@ -270,6 +270,7 @@ beamformer_start_compute(i32 timeout_ms)
if (check_shared_memory()) {
if (lib_try_lock(BeamformerSharedMemoryLockKind_DispatchCompute, 0)) {
if (lib_try_lock(BeamformerSharedMemoryLockKind_DispatchCompute, timeout_ms)) {
+ /* TODO(rnp): non-critical race condition */
lib_release_lock(BeamformerSharedMemoryLockKind_DispatchCompute);
result = 1;
}
diff --git a/static.c b/static.c
@@ -232,9 +232,9 @@ function OS_THREAD_ENTRY_POINT_FN(compute_worker_thread_entry_point)
if (atomic_cas_u32(&ctx->sync_variable, &expected, 1))
break;
- ctx->asleep = 1;
+ atomic_store_u32(&ctx->asleep, 1);
os_wait_on_value(&ctx->sync_variable, 1, -1);
- ctx->asleep = 0;
+ atomic_store_u32(&ctx->asleep, 0);
}
beamformer_complete_compute(ctx->user_context, ctx->arena, ctx->gl_context);
}