Proc code locks before disabling IRQs
authorBarret Rhoden <brho@cs.berkeley.edu>
Mon, 14 May 2012 22:08:09 +0000 (15:08 -0700)
committerBarret Rhoden <brho@cs.berkeley.edu>
Wed, 5 Sep 2012 21:43:58 +0000 (14:43 -0700)
While we cannot grab the lock with interrupts off, we *can* disable
after grabbing the lock, but only under certain circumstances.  This is
because we send the kmsgs with the lock held.  Once we hold the lock, we
know no one will send proc mgmt kmsgs except us.  And it is really
important that we don't send any in these functions.  (imagine trying to
do a __preempt, when you send something to your own core and then try to
__map_vcore, and never get the message).

Also note that we must use immediate messages (which we've been doing
for a while now).  If we didn't use immediate kmsgs for __preempt,
someone (for instance) yielding or change_to-ing would spin on the lock
til they got the lock, but would never get the message since the lock
holder was waiting on them to do the __preempt.

Btw, in case it wasn't clear, the lock holder is spinning in
__map_vcore(), which is where we wait on the completion of the __preempt
kmsg.

kern/src/process.c

index a856ea8..474b92d 100644 (file)
@@ -919,10 +919,6 @@ void proc_yield(struct proc *SAFE p, bool being_nice)
        struct vcore *vc;
        struct preempt_data *vcpd;
        int8_t state = 0;
-       /* Need to disable before even reading vcoreid, since we could be unmapped
-        * by a __preempt or __death.  _S also needs ints disabled, so we'll just do
-        * it immediately. */
-       disable_irqsave(&state);
        /* Need to lock before checking the vcoremap to find out who we are, in case
         * we're getting __preempted and __startcored, from a remote core (in which
         * case we might have come in thinking we were vcore X, but had X preempted
@@ -931,6 +927,11 @@ void proc_yield(struct proc *SAFE p, bool being_nice)
         * preempt the core, then decide to give it back with another grant in
         * between. */
        spin_lock(&p->proc_lock); /* horrible scalability.  =( */
+       /* Need to disable before even reading vcoreid, since we could be unmapped
+        * by a __preempt or __death.  _S also needs ints disabled, so we'll just do
+        * it immediately.  Finally, we need to disable AFTER locking.  It is a bug
+        * to grab the lock while irq's are disabled. */
+       disable_irqsave(&state);
        switch (p->state) {
                case (PROC_RUNNING_S):
                        if (!being_nice) {
@@ -1724,11 +1725,12 @@ void proc_change_to_vcore(struct proc *p, uint32_t new_vcoreid,
        struct vcore *caller_vc, *new_vc;
        struct event_msg preempt_msg = {0};
        int8_t state = 0;
+       /* Need to lock before reading the vcoremap, like in yield.  Need to do this
+        * before disabling irqs (deadlock with incoming proc mgmt kmsgs) */
+       spin_lock(&p->proc_lock);
        /* Need to disable before even reading caller_vcoreid, since we could be
         * unmapped by a __preempt or __death, like in yield. */
        disable_irqsave(&state);
-       /* Need to lock before reading the vcoremap, like in yield */
-       spin_lock(&p->proc_lock);
        /* new_vcoreid is already runing, abort */
        if (vcore_is_mapped(p, new_vcoreid))
                goto out_failed;