Skip to content

Commit a9994a9

Browse files
committed
thr-job: fix a data race when destroying a queue
thr_queue_drain() seemed to assume that no other thread is running the queue when used and thus started with an `atomic_store(&q->running_on, id)` without taking care of the current `running_on` value. But when destroying a queue (`thr_queue_destroy`) TSAN reported this race: WARNING: ThreadSanitizer: data race Write of size 8 by main thread: #0 free <null> Intersec#1 libc_free ./src/core/mem.blk:140:9 Intersec#2 mp_ifree ./src/core/mem.blk:351:5 Intersec#3 thr_queue_delete ./src/core/thr-job.blk:574:1 Intersec#4 thr_queue_drain ./src/core/thr-job.blk:618:9 Intersec#5 thr_queue_sync ./src/core/thr-job.blk:705:13 Intersec#6 thr_queue_destroy ./src/core/thr-job.blk:730:9 #7 test_queue ./tests/zchk-thrjob.blk:381:9 #8 __z_thrjobs_block_invoke_4 ./tests/zchk-thrjob.blk:647:9 #9 z_thrjobs ./tests/zchk-thrjob.blk:648:7 #10 z_run ./src/core/z.blk:1545:9 #11 main ./tests/zchk.c:1206:12 Previous atomic write of size 8 at 0x7210000001e0 by thread T28: #0 thr_queue_drain ./src/core/thr-job.blk:614:5 Intersec#1 thr_queue_run ./src/core/thr-job.blk:624:5 Intersec#2 job_run ./src/core/thr-job.blk:285:9 Intersec#3 thr_run_deque_entry ./src/core/thr-job.blk:381:12 Intersec#4 thr_job_try_steal ./src/core/thr-job.blk:473:20 Intersec#5 thr_job_steal ./src/core/thr-job.blk:513:15 Intersec#6 thr_job_main ./src/core/thr-job.blk:884:25 #7 thr_hooks_wrapper ./src/core/thr.c:89:11 Indeed when finishing to drain the queue, another would finished by: do { […] } while (!mpsc_queue_drain_end(&it, &thr_qnode_destroy)); atomic_compare_exchange_strong(&q->running_on, &id, THR_QUEUE_NOT_RUNNING); So after removing the last element of the queue (`mpsc_queue_drain_end`) the queue's `running_on` is reset to THR_QUEUE_NOT_RUNNING. But when destroying, the sole condition to immediately drain the queue is: if (mpsc_queue_push(&q->q, &n->qnode)) { thr_queue_drain(q); So the race is obvious here, as soon as the queued is emptied, the destroying thread could already be freeing the queue while the previous thread could still be trying to reset `q->running_on`. To fix we now actually wait for the queue to be release before draining it again. Change-Id: I159d6426ec7ace01d0e7aaf685a7e32a6bf31749 Priv-Id: 07021192cda3217760c2ba85fc0fa12af17ef20f
1 parent 26ab265 commit a9994a9

1 file changed

Lines changed: 10 additions & 1 deletion

File tree

src/core/thr-job.blk

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -590,6 +590,7 @@ static void thr_queue_drain(thr_queue_t *q)
590590
bool wipe = false;
591591
mpsc_it_t it;
592592
ssize_t id;
593+
ssize_t prev_id = THR_QUEUE_NOT_RUNNING;
593594

594595
if (unlikely(mpsc_queue_looks_empty(&q->q))) {
595596
/* The queue should not be marked empty *and* scheduled. the only
@@ -601,7 +602,15 @@ static void thr_queue_drain(thr_queue_t *q)
601602
}
602603

603604
id = thr_id();
604-
atomic_store(&q->running_on, id);
605+
while (unlikely(!atomic_compare_exchange_strong(&q->running_on, &prev_id,
606+
id)))
607+
{
608+
if (unlikely(id == prev_id)) {
609+
/* Looks like we already are the one running this queue. */
610+
assert(false);
611+
break;
612+
}
613+
}
605614
mpsc_queue_drain_start(&it, &q->q);
606615
do {
607616
mpsc_node_t *m = mpsc_queue_drain_fast(&it, &thr_queue_run_node,

0 commit comments

Comments
 (0)