Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix deadlock on zombie_threads_mtx #234

Merged
merged 6 commits into from
Apr 6, 2017
Merged

Fix deadlock on zombie_threads_mtx #234

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1e9b098
Unlock the zombie threads mtx before leaving critical section
rafalcieslak Mar 27, 2017
979353a
Implemented thread_join using a cv
rafalcieslak Mar 29, 2017
5c45237
Renamed td_waitcv to td_waitcv
rafalcieslak Mar 29, 2017
954305d
Minor cleanup
rafalcieslak Apr 2, 2017
0a29387
Merge branch 'master' into fix_zombie_mtx_deadlock
rafalcieslak Apr 2, 2017
ab910d2
Merge branch 'master' into fix_zombie_mtx_deadlock
rafalcieslak Apr 4, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions include/thread.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
#include <context.h>
#include <exception.h>
#include <sleepq.h>
#include <mutex.h>
#include <condvar.h>

typedef uint8_t td_prio_t;
typedef uint32_t tid_t;
Expand All @@ -25,6 +27,9 @@ typedef struct thread {
TAILQ_ENTRY(thread) td_zombieq; /* a link on zombie queue */
char *td_name;
tid_t td_tid;
/* Locks*/
mtx_t td_lock;
condvar_t td_waitcv;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move these to the top of the structure and describe the purpose of td_waitcv.

/* thread state */
enum { TDS_INACTIVE = 0x0, TDS_WAITING, TDS_READY, TDS_RUNNING } td_state;
uint32_t td_flags; /* TDF_* flags */
Expand Down
25 changes: 16 additions & 9 deletions sys/thread.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,9 @@ thread_t *thread_create(const char *name, void (*fn)(void *), void *arg) {
td->td_kstack.stk_base = (void *)PG_VADDR_START(td->td_kstack_obj);
td->td_kstack.stk_size = PAGESIZE;

mtx_init(&td->td_lock, MTX_DEF);
cv_init(&td->td_waitcv, "td_waitcv");

ctx_init(td, fn, arg);

/* Do not lock the mutex if this call to thread_create was done before any
Expand Down Expand Up @@ -119,6 +122,8 @@ noreturn void thread_exit(int exitcode) {

log("Thread '%s' {%p} has finished.", td->td_name, td);

mtx_lock(&td->td_lock);

/* Thread must not exit while in critical section! However, we can't use
assert here, because assert also calls thread_exit. Thus, in case this
condition is not met, we'll log the problem, and try to fix the problem. */
Expand All @@ -131,16 +136,18 @@ noreturn void thread_exit(int exitcode) {
fdtab_release(td->td_fdtable);

mtx_lock(&zombie_threads_mtx);
TAILQ_INSERT_TAIL(&zombie_threads, td, td_zombieq);
mtx_unlock(&zombie_threads_mtx);

critical_enter();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we don't need critical_enter here as long as scheduler respects td_lock mutex. Please have a look at preferred solution kern_thread.c:488. You'll need to add sched_abandon function that removes a thread from scheduler and makes sure it will not ever be returned to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I understand the solution you've linked to. The entire function is marked with a comment:

Always called with scheduler locked.

which suggests that there is an external mechanism which prevents the scheduler from running while a thread is exiting. The thread mutex gets locked only at the very last moment, which also hints at a different synchronization mechanism besides this mutex.

Also, I do not understand what do you mean by:

as long as scheduler respects td_lock mutex.

Do we want to prevent the scheduler from switching context to threads whose mutex is locked? Should the scheduler lock (or, actually, try-lock) the thread mutex when picking next thread? For this to work well w/o obvious deadlock, we would also have to prevent preemption of threads who own their own thread mutex. I'm under the impression that this could lead to complex problems.

I'd appreciate some detailed description of the idea you want me to implement.

TAILQ_INSERT_TAIL(&zombie_threads, td, td_zombieq);
td->td_exitcode = exitcode;
sleepq_broadcast(&td->td_exitcode);
td->td_state = TDS_INACTIVE;
{
td->td_exitcode = exitcode;
td->td_state = TDS_INACTIVE;
cv_signal(&td->td_waitcv);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cv_broadcast to wake up all thread that awaits td exit?

mtx_unlock(&td->td_lock);
}
critical_leave();

mtx_unlock(&zombie_threads_mtx);

sched_yield();

/* sched_yield will return immediately when scheduler is not active */
Expand All @@ -153,10 +160,10 @@ void thread_join(thread_t *p) {
thread_t *otd = p;
log("Joining '%s' {%p} with '%s' {%p}", td->td_name, td, otd->td_name, otd);

critical_enter();
mtx_lock(&otd->td_lock);
while (otd->td_state != TDS_INACTIVE)
sleepq_wait(&otd->td_exitcode, "Joining threads");
critical_leave();
cv_wait(&otd->td_waitcv, &otd->td_lock);
mtx_unlock(&otd->td_lock);
}

/* It would be better to have a hash-map from tid_t to thread_t,
Expand Down