Fix scheduler locking hierarchy

init/init.c

@@ -48,22 +48,23 @@ void app_main (void) {
   spawn (&app_thread1);
 
   for (;;) {
-    /* lock_mutex (MUTEX, RV_PRIVATE); */
+    lock_mutex (MUTEX, RV_PRIVATE);
 
     for (int i = 0; i < 3; i++)
       test ('a');
 
-    /* unlock_mutex (MUTEX, RV_PRIVATE); */
+    unlock_mutex (MUTEX, RV_PRIVATE);
   }
 }
 
 void app_thread1 (void) {
+  for (;;) {
+    lock_mutex (MUTEX, RV_PRIVATE);
+
     for (int i = 0; i < 3; i++)
       test ('b');
-  for (;;) {
-    /* lock_mutex (MUTEX, RV_PRIVATE); */
 
-    /* unlock_mutex (MUTEX, RV_PRIVATE); */
+    unlock_mutex (MUTEX, RV_PRIVATE);
   }
 
   quit ();

kernel/proc/locks.txt

@@ -0,0 +1,6 @@
+Lock hierarchy for process scheduling:
+
+1. proc_tree_lock
+2. cpu->lock
+3. proc->lock
+4. sq->lock

kernel/proc/mutex.c

@@ -19,10 +19,65 @@ void proc_cleanup_resource_mutex (struct proc* proc, struct proc_resource* resou
   proc_mutex_unlock (proc, mutex);
 }
 
+static void proc_mutex_suspend (struct proc* proc, struct proc_suspension_q* sq,
+                                spin_lock_t* resource_lock, spin_lock_ctx_t* ctxrl) {
+  spin_lock_ctx_t ctxpr, ctxcpu, ctxsq;
+  struct cpu* cpu = proc->cpu;
+
+  spin_lock (&cpu->lock, &ctxcpu);
+  spin_lock (&proc->lock, &ctxpr);
+  spin_lock (&sq->lock, &ctxsq);
+
+  spin_unlock (resource_lock, ctxrl);
+
+  atomic_store (&proc->state, PROC_SUSPENDED);
+  proc->suspension_q = sq;
+
+  list_remove (cpu->proc_run_q, &proc->cpu_run_q_link);
+  if (cpu->proc_current == proc)
+    cpu->proc_current = NULL;
+
+  list_append (sq->proc_list, &proc->suspension_link);
+
+  spin_unlock (&sq->lock, &ctxsq);
+  spin_unlock (&proc->lock, &ctxpr);
+  spin_unlock (&cpu->lock, &ctxcpu);
+
+  cpu_request_sched (cpu);
+}
+
+static void proc_mutex_resume (struct proc* proc) {
+  spin_lock_ctx_t ctxsq, ctxpr, ctxcpu;
+  struct cpu* cpu = proc->cpu;
+
+  spin_lock (&cpu->lock, &ctxcpu);
+  spin_lock (&proc->lock, &ctxpr);
+
+  struct proc_suspension_q* sq = proc->suspension_q;
+
+  if (sq) {
+    spin_lock (&sq->lock, &ctxsq);
+
+    list_remove (sq->proc_list, &proc->suspension_link);
+
+    proc->suspension_q = NULL;
+    atomic_store (&proc->state, PROC_READY);
+
+    list_append (cpu->proc_run_q, &proc->cpu_run_q_link);
+
+    spin_unlock (&sq->lock, &ctxsq);
+  }
+
+  spin_unlock (&proc->lock, &ctxpr);
+  spin_unlock (&cpu->lock, &ctxcpu);
+
+  cpu_request_sched (cpu);
+}
+
 void proc_mutex_lock (struct proc* proc, struct proc_mutex* mutex) {
   spin_lock_ctx_t ctxmt;
 
-try:
+  for (;;) {
     spin_lock (&mutex->resource->lock, &ctxmt);
 
     if (!mutex->locked || mutex->owner == proc) {
@@ -32,12 +87,8 @@ try:
       return;
     }
 
-  spin_unlock (&mutex->resource->lock, &ctxmt);
+    proc_mutex_suspend (proc, &mutex->suspension_q, &mutex->resource->lock, &ctxmt);
-
+  }
-  DEBUG ("proc->pid=%d\n", proc->pid);
-  proc_suspend (proc, &mutex->suspension_q);
-
-  goto try;
 }
 
 bool proc_mutex_unlock (struct proc* proc, struct proc_mutex* mutex) {
@@ -63,7 +114,7 @@ bool proc_mutex_unlock (struct proc* proc, struct proc_mutex* mutex) {
     spin_unlock (&mutex->suspension_q.lock, &ctxsq);
     spin_unlock (&mutex->resource->lock, &ctxmt);
 
-    proc_resume (resumed_proc);
+    proc_mutex_resume (resumed_proc);
 
     return true;
   }

kernel/proc/proc.c

@@ -346,51 +346,6 @@ void proc_kill (struct proc* proc) {
   cpu_request_sched (cpu);
 }
 
-void proc_suspend (struct proc* proc, struct proc_suspension_q* sq) {
-  spin_lock_ctx_t ctxpr, ctxcpu, ctxsq;
-  struct cpu* cpu = proc->cpu;
-
-  spin_lock (&proc->lock, &ctxpr);
-  atomic_store (&proc->state, PROC_SUSPENDED);
-  proc->suspension_q = sq;
-  spin_unlock (&proc->lock, &ctxpr);
-
-  /* remove from run q */
-  spin_lock (&cpu->lock, &ctxcpu);
-
-  list_remove (cpu->proc_run_q, &proc->cpu_run_q_link);
-  if (cpu->proc_current == proc)
-    cpu->proc_current = NULL;
-  spin_unlock (&cpu->lock, &ctxcpu);
-
-  spin_lock (&sq->lock, &ctxsq);
-  list_append (sq->proc_list, &proc->suspension_link);
-  spin_unlock (&sq->lock, &ctxsq);
-
-  cpu_request_sched (cpu);
-}
-
-void proc_resume (struct proc* proc) {
-  spin_lock_ctx_t ctxsq, ctxpr, ctxcpu;
-  struct cpu* cpu = proc->cpu;
-  struct proc_suspension_q* sq = proc->suspension_q;
-
-  spin_lock (&sq->lock, &ctxsq);
-  list_remove (sq->proc_list, &proc->suspension_link);
-  spin_unlock (&sq->lock, &ctxsq);
-
-  spin_lock (&proc->lock, &ctxpr);
-  proc->suspension_q = NULL;
-  atomic_store (&proc->state, PROC_READY);
-  spin_unlock (&proc->lock, &ctxpr);
-
-  spin_lock (&cpu->lock, &ctxcpu);
-  list_append (cpu->proc_run_q, &proc->cpu_run_q_link);
-  spin_unlock (&cpu->lock, &ctxcpu);
-
-  cpu_request_sched (cpu);
-}
-
 static void proc_irq_sched (void* arg, void* regs) {
   (void)arg;
   proc_sched (regs);

kernel/proc/proc.h

@@ -60,8 +60,6 @@ struct proc {
   struct proc_resources* resources;
 };
 
-void proc_suspend (struct proc* proc, struct proc_suspension_q* sq);
-void proc_resume (struct proc* proc);
 void proc_sched (void* regs);
 void proc_kill (struct proc* proc);
 bool proc_map (struct proc* proc, uintptr_t start_paddr, uintptr_t start_vaddr, size_t pages,

kernel/syscall/syscall.c

@@ -234,6 +234,7 @@ DEFINE_SYSCALL (sys_lock_mutex) {
   if (mutex_resource == NULL)
     return -ST_NOT_FOUND;
 
+  DEBUG ("locking %d\n", proc->pid);
   proc_mutex_lock (proc, &mutex_resource->u.mutex);
 
   return ST_OK;
@@ -252,6 +253,7 @@ DEFINE_SYSCALL (sys_unlock_mutex) {
   if (mutex_resource == NULL)
     return -ST_NOT_FOUND;
 
+  DEBUG ("unlocking %d\n", proc->pid);
   return proc_mutex_unlock (proc, &mutex_resource->u.mutex) ? ST_OK : -ST_PERMISSION_ERROR;
 }