Fixes irqsave issue with CVs
authorBarret Rhoden <brho@cs.berkeley.edu>
Mon, 5 Nov 2012 21:22:34 +0000 (13:22 -0800)
committerBarret Rhoden <brho@cs.berkeley.edu>
Mon, 5 Nov 2012 21:22:34 +0000 (13:22 -0800)
Using spinlock_irqsave like that isn't sufficient.  The spinlock_irqsave
really only works across a single lock/unlock pair (it saves the irq
state in the lock).  When we unlock, we potentially reenable irqs.

During the while loop (which enforces ordering), we basically are
holding a sort of lock.  Signallers run from irq context could interrupt
us, deadlocking the system.

Instead, we just have the caller of the CV tell us if we want IRQs off
or not.  Pay attention to the note in trap.c.

In the future, I might rewrite the kthread code to use CVs instead of
semaphores, which would get rid of the while loop/ordering.  Regardless,
I still prefer having the cv_*_irqsave() functions.

kern/arch/i686/trap.c
kern/include/kthread.h
kern/src/kthread.c

index e9e8f08..1e8d6fc 100644 (file)
@@ -482,6 +482,10 @@ void page_fault_handler(struct trapframe *tf)
        if ((tf->tf_cs & 3) == 0) {
                print_trapframe(tf);
                panic("Page Fault in the Kernel at 0x%08x!", fault_va);
+               /* if we want to do something like kill a process or other code, be
+                * aware we are in a sort of irq-like context, meaning the main kernel
+                * code we 'interrupted' could be holding locks - even irqsave locks.
+                * Send yourself a kernel message to do this sort of work. */
        }
        /* safe to reenable after rcr2 */
        enable_irq();
index 93d6d11..576f692 100644 (file)
@@ -106,9 +106,15 @@ void kthread_yield(void);
 void cv_init(struct cond_var *cv);
 void cv_lock(struct cond_var *cv);
 void cv_unlock(struct cond_var *cv);
-void cv_wait_and_unlock(struct cond_var *cv);
+void cv_lock_irqsave(struct cond_var *cv, int8_t *state);
+void cv_unlock_irqsave(struct cond_var *cv, int8_t *state);
+void cv_wait_and_unlock(struct cond_var *cv);  /* does not mess with irqs */
 void cv_wait(struct cond_var *cv);
+void __cv_signal(struct cond_var *cv);
+void __cv_broadcast(struct cond_var *cv);
 void cv_signal(struct cond_var *cv);
 void cv_broadcast(struct cond_var *cv);
+void cv_signal_irqsave(struct cond_var *cv, int8_t *state);
+void cv_broadcast_irqsave(struct cond_var *cv, int8_t *state);
 
 #endif /* ROS_KERN_KTHREAD_H */
index a977714..12cfdd2 100644 (file)
@@ -286,12 +286,24 @@ void cv_init(struct cond_var *cv)
 
 void cv_lock(struct cond_var *cv)
 {
-       spin_lock_irqsave(&cv->lock);
+       spin_lock(&cv->lock);
 }
 
 void cv_unlock(struct cond_var *cv)
 {
-       spin_unlock_irqsave(&cv->lock);
+       spin_unlock(&cv->lock);
+}
+
+void cv_lock_irqsave(struct cond_var *cv, int8_t *irq_state)
+{
+       disable_irqsave(irq_state);
+       cv_lock(cv);
+}
+
+void cv_unlock_irqsave(struct cond_var *cv, int8_t *irq_state)
+{
+       cv_unlock(cv);
+       enable_irqsave(irq_state);
 }
 
 /* Helper to clarify the wait/signalling code */
@@ -303,12 +315,14 @@ static int nr_sem_waiters(struct semaphore *sem)
        return retval;
 }
 
-/* Comes in locked */
+/* Comes in locked.  Note we don't mess with enabling/disabling irqs.  The
+ * initial cv_lock would have disabled irqs (if applicable), and we don't mess
+ * with that setting at all. */
 void cv_wait_and_unlock(struct cond_var *cv)
 {
        unsigned long nr_prev_waiters;
        nr_prev_waiters = cv->nr_waiters++;
-       spin_unlock_irqsave(&cv->lock);
+       spin_unlock(&cv->lock);
        /* Wait til our turn.  This forces an ordering of all waiters such that the
         * order in which they wait is the order in which they down the sem. */
        while (nr_prev_waiters != nr_sem_waiters(&cv->sem))
@@ -320,7 +334,7 @@ void cv_wait_and_unlock(struct cond_var *cv)
        sleep_on(&cv->sem);
 }
 
-/* Comes in locked */
+/* Comes in locked.  Note cv_lock does not disable irqs. */
 void cv_wait(struct cond_var *cv)
 {
        cv_wait_and_unlock(cv);
@@ -340,9 +354,8 @@ static void sem_wake_one(struct semaphore *sem)
        kthread_runnable(kthread);
 }
 
-void cv_signal(struct cond_var *cv)
+void __cv_signal(struct cond_var *cv)
 {
-       spin_lock_irqsave(&cv->lock);
        /* Can't short circuit this stuff.  We need to make sure any waiters that
         * made it past upping the cv->nr_waiters has also downed the sem.
         * Otherwise we muck with nr_waiters, which could break the ordering
@@ -354,17 +367,42 @@ void cv_signal(struct cond_var *cv)
                cv->nr_waiters--;
                sem_wake_one(&cv->sem);
        }
-       spin_unlock_irqsave(&cv->lock);
 }
 
-void cv_broadcast(struct cond_var *cv)
+void __cv_broadcast(struct cond_var *cv)
 {
-       spin_lock_irqsave(&cv->lock);
        while (cv->nr_waiters != nr_sem_waiters(&cv->sem))
                cpu_relax();
        while (cv->nr_waiters) {
                cv->nr_waiters--;
                sem_wake_one(&cv->sem);
        }
-       spin_unlock_irqsave(&cv->lock);
+}
+
+void cv_signal(struct cond_var *cv)
+{
+       spin_lock(&cv->lock);
+       __cv_signal(cv);
+       spin_unlock(&cv->lock);
+}
+
+void cv_broadcast(struct cond_var *cv)
+{
+       spin_lock(&cv->lock);
+       __cv_broadcast(cv);
+       spin_unlock(&cv->lock);
+}
+
+void cv_signal_irqsave(struct cond_var *cv, int8_t *irq_state)
+{
+       disable_irqsave(irq_state);
+       cv_signal(cv);
+       enable_irqsave(irq_state);
+}
+
+void cv_broadcast_irqsave(struct cond_var *cv, int8_t *irq_state)
+{
+       disable_irqsave(irq_state);
+       cv_broadcast(cv);
+       enable_irqsave(irq_state);
 }