Skip to content

Commit abdb6cd

Browse files
committed
Rewrite reap_dead_tasks to never grab the sched lock before a task lock
Doing so contradicts the locking order used everywhere else and causes deadlocks. Un-XFAIL task-perf-spawnalot Closes #854
1 parent 25416bf commit abdb6cd

File tree

3 files changed

+47
-9
lines changed

3 files changed

+47
-9
lines changed

src/rt/rust_scheduler.cpp

Lines changed: 46 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -100,25 +100,66 @@ rust_scheduler::number_of_live_tasks() {
100100
void
101101
rust_scheduler::reap_dead_tasks(int id) {
102102
I(this, lock.lock_held_by_current_thread());
103-
for (size_t i = 0; i < dead_tasks.length(); ) {
103+
if (dead_tasks.length() == 0) {
104+
return;
105+
}
106+
107+
// First make up copy of the dead_task list with the lock held
108+
size_t dead_tasks_len = dead_tasks.length();
109+
rust_task **dead_tasks_copy = (rust_task**)
110+
srv->malloc(sizeof(rust_task*) * dead_tasks_len);
111+
for (size_t i = 0; i < dead_tasks_len; ++i) {
104112
rust_task *task = dead_tasks[i];
113+
dead_tasks_copy[i] = task;
114+
}
115+
116+
// Now drop the lock and futz with the tasks. This avoids establishing
117+
// a sched->lock then task->lock locking order, which would be devestating
118+
// to performance.
119+
lock.unlock();
120+
121+
for (size_t i = 0; i < dead_tasks_len; ++i) {
122+
rust_task *task = dead_tasks_copy[i];
105123
task->lock.lock();
106124
// Make sure this task isn't still running somewhere else...
107125
if (task->can_schedule(id)) {
108126
I(this, task->tasks_waiting_to_join.is_empty());
109-
dead_tasks.remove(task);
110127
DLOG(this, task,
111128
"deleting unreferenced dead task %s @0x%" PRIxPTR,
112129
task->name, task);
113130
task->lock.unlock();
131+
} else {
132+
task->lock.unlock();
133+
dead_tasks_copy[i] = NULL;
134+
}
135+
}
136+
137+
// Now grab the lock again and remove the tasks that were truly dead
138+
lock.lock();
139+
140+
for (size_t i = 0; i < dead_tasks_len; ++i) {
141+
rust_task *task = dead_tasks_copy[i];
142+
if (task) {
143+
dead_tasks.remove(task);
144+
}
145+
}
146+
147+
// Now unlock again because we have to actually free the dead tasks,
148+
// and that may end up wanting to do lock the task and sched locks
149+
// again (via target->send)
150+
lock.unlock();
151+
152+
for (size_t i = 0; i < dead_tasks_len; ++i) {
153+
rust_task *task = dead_tasks_copy[i];
154+
if (task) {
114155
task->deref();
115156
sync::decrement(kernel->live_tasks);
116157
kernel->wakeup_schedulers();
117-
continue;
118158
}
119-
task->lock.unlock();
120-
++i;
121159
}
160+
srv->free(dead_tasks_copy);
161+
162+
lock.lock();
122163
}
123164

124165
/**

src/rt/rust_task.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,7 @@ rust_task::rust_task(rust_scheduler *sched, rust_task_list *state,
9090

9191
rust_task::~rust_task()
9292
{
93+
I(sched, !sched->lock.lock_held_by_current_thread());
9394
DLOG(sched, task, "~rust_task %s @0x%" PRIxPTR ", refcnt=%d",
9495
name, (uintptr_t)this, ref_count);
9596

src/test/bench/task-perf-spawnalot.rs

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,3 @@
1-
// xfail-stage1
2-
// xfail-stage2
3-
// xfail-stage3
4-
51
use std;
62
import std::vec;
73
import std::task;

0 commit comments

Comments
 (0)