From 3bde8dd084d89630cd4c09ac5af27025069d40c4 Mon Sep 17 00:00:00 2001 From: Ali Mashtizadeh Date: Tue, 5 Sep 2023 22:18:10 -0400 Subject: [PATCH] Cleaning up a pile of bugs --- sys/amd64/critical.c | 6 ++++++ sys/include/spinlock.h | 1 + sys/kern/mutex.c | 6 ++++++ sys/kern/process.c | 11 ++++++++++- sys/kern/sched.c | 24 ++++++++++++++++-------- sys/kern/thread.c | 17 +++++++++++++++-- sys/kern/waitchannel.c | 1 - 7 files changed, 54 insertions(+), 12 deletions(-) diff --git a/sys/amd64/critical.c b/sys/amd64/critical.c index ac07f9b..95afd72 100644 --- a/sys/amd64/critical.c +++ b/sys/amd64/critical.c @@ -41,6 +41,12 @@ Critical_Exit() } } +uint32_t +Critical_Level() +{ + return lockLevel[CPU()]; +} + static void Debug_Critical(int argc, const char *argv[]) { diff --git a/sys/include/spinlock.h b/sys/include/spinlock.h index b5de060..d5ac344 100644 --- a/sys/include/spinlock.h +++ b/sys/include/spinlock.h @@ -30,6 +30,7 @@ typedef struct Spinlock void Critical_Init(); void Critical_Enter(); void Critical_Exit(); +uint32_t Critical_Level(); void Spinlock_EarlyInit(); void Spinlock_Init(Spinlock *lock, const char *name, uint64_t type); diff --git a/sys/kern/mutex.c b/sys/kern/mutex.c index cc8cbcd..07356f2 100644 --- a/sys/kern/mutex.c +++ b/sys/kern/mutex.c @@ -47,6 +47,12 @@ Mutex_Destroy(Mutex *mtx) void Mutex_Lock(Mutex *mtx) { + /* + * You cannot hold a spinlock while trying to acquire a Mutex that may + * sleep! + */ + ASSERT(Critical_Level() == 0); + Spinlock_Lock(&mtx->lock); while (mtx->status == MTX_STATUS_LOCKED) { WaitChannel_Lock(&mtx->chan); diff --git a/sys/kern/process.c b/sys/kern/process.c index 075a8f2..95b4073 100644 --- a/sys/kern/process.c +++ b/sys/kern/process.c @@ -189,9 +189,18 @@ Process_Wait(Process *proc, uint64_t pid) status = (p->pid << 16) | p->exitCode; // Release threads - TAILQ_FOREACH_SAFE(thr, &p->zombieQueue, schedQueue, thr_temp) { + Spinlock_Lock(&proc->lock); + while (!TAILQ_EMPTY(&p->zombieQueue)) { + thr = TAILQ_FIRST(&p->zombieQueue); + TAILQ_REMOVE(&p->zombieQueue, thr, schedQueue); + Spinlock_Unlock(&proc->lock); + + ASSERT(thr->proc->pid != 1); Thread_Release(thr); + + Spinlock_Lock(&proc->lock); } + Spinlock_Unlock(&proc->lock); // Release process Process_Release(p); diff --git a/sys/kern/sched.c b/sys/kern/sched.c index fb3e819..faacea5 100644 --- a/sys/kern/sched.c +++ b/sys/kern/sched.c @@ -65,6 +65,8 @@ Sched_SetWaiting(Thread *thr) { Spinlock_Lock(&schedLock); + ASSERT(thr->schedState == SCHED_STATE_RUNNING); + thr->schedState = SCHED_STATE_WAITING; TAILQ_INSERT_TAIL(&waitQueue, thr, schedQueue); thr->waitStart = KTime_GetEpochNS(); @@ -77,13 +79,6 @@ Sched_SetZombie(Thread *thr) { Process *proc = thr->proc; - Spinlock_Lock(&schedLock); - - thr->schedState = SCHED_STATE_ZOMBIE; - Spinlock_Lock(&proc->lock); - TAILQ_INSERT_TAIL(&proc->zombieQueue, thr, schedQueue); - Spinlock_Unlock(&proc->lock); - if (proc->threads == 1) { // All processes have parents except 'init' and 'kernel' ASSERT(proc->parent != NULL); @@ -95,10 +90,22 @@ Sched_SetZombie(Thread *thr) Spinlock_Unlock(&proc->parent->lock); CV_Signal(&proc->zombieProcPCV); CV_Signal(&proc->parent->zombieProcCV); - Mutex_Unlock(&proc->parent->zombieProcLock); } + /* + * Set as zombie just before releasing the zombieProcLock in case we had to + * sleep to acquire the zombieProcLock. + */ + Spinlock_Lock(&schedLock); + thr->schedState = SCHED_STATE_ZOMBIE; Spinlock_Unlock(&schedLock); + + Spinlock_Lock(&proc->lock); + TAILQ_INSERT_TAIL(&proc->zombieQueue, thr, schedQueue); + Spinlock_Unlock(&proc->lock); + + if (proc->threads == 1) + Mutex_Unlock(&proc->parent->zombieProcLock); } static void @@ -132,6 +139,7 @@ Sched_Scheduler() Spinlock_Unlock(&schedLock); return; } + ASSERT(next->schedState == SCHED_STATE_RUNNABLE); TAILQ_REMOVE(&runnableQueue, next, schedQueue); prev = curProc[CPU()]; diff --git a/sys/kern/thread.c b/sys/kern/thread.c index 51f5d8a..508bc5d 100644 --- a/sys/kern/thread.c +++ b/sys/kern/thread.c @@ -192,6 +192,9 @@ Thread_Destroy(Thread *thr) { Process *proc = thr->proc; + // Don't free kernel threads + ASSERT(proc->pid != 1); + // Free userspace stack Spinlock_Lock(&proc->lock); @@ -201,10 +204,11 @@ Thread_Destroy(Thread *thr) // Free AS PAlloc_Release((void *)thr->kstack); - Slab_Free(&threadSlab, thr); // Release process handle Process_Release(thr->proc); + + Slab_Free(&threadSlab, thr); } Thread * @@ -262,6 +266,7 @@ Thread_Wait(Thread *thr, uint64_t tid) return SYSCALL_PACK(0, status); } + // XXXURGENT TAILQ_FOREACH(t, &thr->proc->zombieQueue, schedQueue) { if (t->tid == tid) { TAILQ_REMOVE(&thr->proc->zombieQueue, t, schedQueue); @@ -293,12 +298,20 @@ ThreadKThreadEntry(TrapFrame *tf) __NO_LOCK_ANALYSIS void Thread_Dump(Thread *thr) { + const char *states[] = { + "NULL", + "RUNNABLE", + "RUNNING", + "WAITING", + "ZOMBIE" + }; + // Thread_DumpArch(thr) kprintf("space %016llx\n", thr->space); kprintf("kstack %016llx\n", thr->kstack); kprintf("tid %llu\n", thr->tid); kprintf("refCount %d\n", thr->refCount); - kprintf("state %d\n", thr->schedState); + kprintf("state %s\n", states[thr->schedState]); kprintf("ctxswtch %llu\n", thr->ctxSwitches); kprintf("utime %llu\n", thr->userTime); kprintf("ktime %llu\n", thr->kernTime); diff --git a/sys/kern/waitchannel.c b/sys/kern/waitchannel.c index 1d50547..31993b5 100644 --- a/sys/kern/waitchannel.c +++ b/sys/kern/waitchannel.c @@ -118,7 +118,6 @@ WaitChannel_WakeAll(WaitChannel *wchan) Spinlock_Lock(&wchan->lock); TAILQ_FOREACH_SAFE(thr, &wchan->chanQueue, chanQueue, thrTemp) { - thr = TAILQ_FIRST(&wchan->chanQueue); TAILQ_REMOVE(&wchan->chanQueue, thr, chanQueue); Sched_SetRunnable(thr); Thread_Release(thr);