proc_lock is no longer an irqsave
authorBarret Rhoden <brho@cs.berkeley.edu>
Sun, 25 Apr 2010 02:09:51 +0000 (19:09 -0700)
committerKevin Klues <klueska@cs.berkeley.edu>
Thu, 3 Nov 2011 00:35:44 +0000 (17:35 -0700)
Do not grab it from interrupt context.  Technically, this includes the
monitor, if you have the CONFIG_MONITOR_ON_INT flag set.  I'll address
those issues in a later patch.  Calling schedule() from a timer
interrupt also does this.  That'll get fixed later, and probably won't
cause a problem for now.

Documentation/kernel_messages.txt
Documentation/process-internals.txt
kern/src/manager.c
kern/src/mm.c
kern/src/monitor.c
kern/src/process.c
kern/src/resource.c
kern/src/syscall.c

index bfbde61..692993d 100644 (file)
@@ -32,6 +32,17 @@ get processed next, at the latest.  Even if the routine function doesn't return,
 once interrupts are reenabled (like when popping to userspace), the
 __kernel_message() handler will fire again.
 
+Immediate kernel messages are executed in interrupt context.  Routine messages
+may technically be done in interrupt context (it's a nebulous term) because they
+are executed because of an interrupt handler, but from the kernel's perspective
+they are like executing in regular context (like when a process makes a syscall,
+aka real "process context").  This is because there are no concerns about the
+kernel holding locks or otherwise "interrupting" its own execution.  Routine
+messages are a little different than just trapping into the kernel, since the
+functions don't have to return and may result in clobbering the kernel stack.
+Also note that this behavior is dependent on where we call
+process_routine_kmsg().  Don't call it somewhere you need to return to.
+
 History:
 --------------------------------
 A bit of history: we used to use "immediate" messages (when all messages were
index b9ff0e5..c70ee55 100644 (file)
@@ -12,7 +12,8 @@ Contents:
 3. Leaving the Kernel Stack
 4. Preemption and Notification Issues
 5. Current_tf
-6. TBD
+6. Locking!
+7. TBD
 
 1. Reference Counting
 ===========================
@@ -683,5 +684,60 @@ iret.  At which point, if the code path is deep enough that we don't want to
 carry the TF pointer, we may revisit this.  Until then, current_tf is just for
 userspace contexts, and is simply stored in per_cpu_info.
 
-6. TBD
+6. Locking!
+===========================
+6.1: proc_lock
+---------------------------
+Currently, all locking is done on the proc_lock.  It's main goal is to protect
+the vcore mapping (vcore->pcore and vice versa).  As of Apr 2010, it's also used
+to protect changes to the address space and the refcnt.  Eventually the refcnt
+will be handled with atomics, and the address space will have it's own MM lock.  
+
+We grab the proc_lock all over the place, but we try to avoid it whereever
+possible - especially in kernel messages or other places that will be executed
+in parallel.  One place we do grab it but would like to not is in proc_yield().  
+We don't always need to grab the proc lock.  Here are some examples:
+
+6.1.1: Lockless Notifications:
+-------------
+We don't lock when sending a notification.  We want the proc_lock to not be an
+irqsave lock (discussed below).  Since we might want to send a notification from
+interrupt context, we can't grab the proc_lock if it's a regular lock.  
+
+This is okay, since the proc_lock is only protecting the vcoremapping.  We could
+accidentally send the notification to the wrong pcore.  The __notif handler
+checks to make sure it is the right process, and all _M processes should be able
+to handle spurious notifications.  This assumes they are still _M.
+
+If we send it to the wrong pcore, there is a danger of losing the notif, since
+it didn't go to the correct vcore.  That would happen anyway, (the vcore is
+unmapped, or in the process of mapping).  The notif_pending flag will be caught
+when the vcore is started up next time (and that flag was set before reading the
+vcoremap).
+
+6.1.2: Local get_vcoreid():
+-------------
+It's not necessary to lock while checking the vcoremap if you are checking for
+the core you are running on (e.g. pcoreid == core_id()).  This is because all
+unmappings of a vcore are done on the receive side of a routine kmsg, and that
+code cannot run concurrently with the code you are running.  
+
+6.2: irqsave
+---------------------------
+The proc_lock used to be an irqsave lock (meaning it disables interrupts and can
+be grabbed from interrupt context).  We made it a regular lock for a couple
+reasons.  The immediate one was it was causing deadlocks due to some other
+ghetto things (blocking on the frontend server, for instance).  More generally,
+we don't want to disable interrupts for long periods of time, so it was
+something worth doing anyway.  
+
+This means that we cannot grab the proc_lock from interrupt context.  This
+includes having schedule called from an interrupt handler (like the
+timer_interrupt() handler), since it will call proc_run.  Right now, we actually
+do this, which we shouldn't, and that will eventually get fixed.  The right
+answer is that the actual work of running the scheduler should be a routine
+kmsg, similar to how Linux sets a bit in the kernel that it checks on the way
+out to see if it should run the scheduler or not.
+
+7. TBD
 ===========================
index 475eb94..200ad95 100644 (file)
@@ -71,10 +71,10 @@ void manager_brho(void)
                        //p = kfs_proc_create(kfs_lookup_path("roslib_mproctests"));
                        //p = kfs_proc_create(kfs_lookup_path("roslib_spawn"));
                        // being proper and all:
-                       spin_lock_irqsave(&p->proc_lock);
+                       spin_lock(&p->proc_lock);
                        __proc_set_state(p, PROC_RUNNABLE_S);
                        // normal single-cored way
-                       spin_unlock_irqsave(&p->proc_lock);
+                       spin_unlock(&p->proc_lock);
                        proc_run(p);
                        proc_decref(p, 1);
                        #if 0
@@ -83,7 +83,7 @@ void manager_brho(void)
                        __proc_set_state(p, PROC_RUNNING_S);
                        __proc_set_state(p, PROC_RUNNABLE_M);
                        p->resources[RES_CORES].amt_wanted = 5;
-                       spin_unlock_irqsave(&p->proc_lock);
+                       spin_unlock(&p->proc_lock);
                        core_request(p);
                        panic("This is okay");
                        #endif
@@ -94,10 +94,10 @@ void manager_brho(void)
                        udelay(10000000);
                        // this is a ghetto way to test restarting an _M
                                printk("\nattempting to ghetto preempt...\n");
-                               spin_lock_irqsave(&p->proc_lock);
+                               spin_lock(&p->proc_lock);
                                proc_take_allcores(p, __death);
                                __proc_set_state(p, PROC_RUNNABLE_M);
-                               spin_unlock_irqsave(&p->proc_lock);
+                               spin_unlock(&p->proc_lock);
                                udelay(5000000);
                                printk("\nattempting to restart...\n");
                                core_request(p); // proc still wants the cores
@@ -106,9 +106,9 @@ void manager_brho(void)
                                printk("taking 3 cores from p\n");
                                for (int i = 0; i < num; i++)
                                        corelist[i] = 7-i; // 7, 6, and 5
-                               spin_lock_irqsave(&p->proc_lock);
+                               spin_lock(&p->proc_lock);
                                proc_take_cores(p, corelist, &num, __death);
-                               spin_unlock_irqsave(&p->proc_lock);
+                               spin_unlock(&p->proc_lock);
                                udelay(5000000);
                                printk("Killing p\n");
                                proc_destroy(p);
index a6b7995..33232eb 100644 (file)
@@ -54,9 +54,9 @@ void *do_mmap(struct proc *p, uintptr_t addr, size_t len, int prot, int flags,
               struct file* file, size_t offset)
 {
        // TODO: grab the appropriate mm_lock
-       spin_lock_irqsave(&p->proc_lock);
+       spin_lock(&p->proc_lock);
        void* ret = __do_mmap(p,addr,len,prot,flags,file,offset);
-       spin_unlock_irqsave(&p->proc_lock);
+       spin_unlock(&p->proc_lock);
        return ret;
 }
 
@@ -168,9 +168,9 @@ int mprotect(struct proc* p, void* addr, size_t len, int prot)
                return -1;
        }
 
-       spin_lock_irqsave(&p->proc_lock);
+       spin_lock(&p->proc_lock);
        int ret = __mprotect(p,addr,len,prot);
-       spin_unlock_irqsave(&p->proc_lock);
+       spin_unlock(&p->proc_lock);
 
        return ret;
 }
@@ -241,9 +241,9 @@ int handle_page_fault(struct proc* p, uintptr_t va, int prot)
        if(prot != PROT_READ && prot != PROT_WRITE && prot != PROT_EXEC)
                panic("bad prot!");
 
-       spin_lock_irqsave(&p->proc_lock);
+       spin_lock(&p->proc_lock);
        int ret = __handle_page_fault(p,va,prot);
-       spin_unlock_irqsave(&p->proc_lock);
+       spin_unlock(&p->proc_lock);
        return ret;
 }
        
index 14dc2f0..590d6a5 100644 (file)
@@ -266,10 +266,10 @@ int mon_kfs_run(int argc, char *NTS *NT COUNT(argc) argv, trapframe_t *tf)
        }
        struct proc *p = kfs_proc_create(kfs_inode);
        // go from PROC_CREATED->PROC_RUNNABLE_S
-       spin_lock_irqsave(&p->proc_lock); // might not be necessary for a mon function
+       spin_lock(&p->proc_lock); // might not be necessary for a mon function
        __proc_set_state(p, PROC_RUNNABLE_S);
        schedule_proc(p);
-       spin_unlock_irqsave(&p->proc_lock);
+       spin_unlock(&p->proc_lock);
        proc_decref(p, 1); // let go of the reference created in proc_create()
        // Should never return from schedule (env_pop in there)
        // also note you may not get the process you created, in the event there
@@ -330,7 +330,7 @@ int mon_procinfo(int argc, char *NTS *NT COUNT(argc) argv, trapframe_t *tf)
                        printk("No such proc\n");
                        return 1;
                }
-               spin_unlock_irqsave(&p->proc_lock);
+               spin_unlock(&p->proc_lock);
                proc_decref(p, 1);
        } else if (!strcmp(argv[1], "kill")) {
                if (argc != 3) {
index 3e72665..e22a1a3 100644 (file)
@@ -379,10 +379,10 @@ bool proc_controls(struct proc *actor, struct proc *target)
 void proc_run(struct proc *p)
 {
        bool self_ipi_pending = FALSE;
-       spin_lock_irqsave(&p->proc_lock);
+       spin_lock(&p->proc_lock);
        switch (p->state) {
                case (PROC_DYING):
-                       spin_unlock_irqsave(&p->proc_lock);
+                       spin_unlock(&p->proc_lock);
                        printk("Process %d not starting due to async death\n", p->pid);
                        // if we're a worker core, smp_idle, o/w return
                        if (!management_core())
@@ -405,10 +405,11 @@ void proc_run(struct proc *p)
                                p->env_refcnt--; // TODO: (REF) use incref
                        /* We don't want to process routine messages here, since it's a bit
                         * different than when we perform a syscall in this process's
-                        * context.  We keep interrupts disabled so that if there was a
+                        * context.  We want interrupts disabled so that if there was a
                         * routine message on the way, we'll get the interrupt once we pop
-                        * back to userspace.  Note the use of a regular unlock. */
+                        * back to userspace.  */
                        spin_unlock(&p->proc_lock);
+                       disable_irq();
                        __proc_startcore(p, &p->env_tf);
                        break;
                case (PROC_RUNNABLE_M):
@@ -439,16 +440,12 @@ void proc_run(struct proc *p)
                         * death message.  Otherwise, it would look like a new process.  So
                         * we hold the lock til after we send our message, which prevents a
                         * possible death message.
-                        * - Likewise, we need interrupts to be disabled, in case one of the
-                        *   messages was for us, and reenable them after letting go of the
-                        *   lock.  This is done by spin_lock_irqsave, so be careful if you
-                        *   change this.
                         * - Note there is no guarantee this core's interrupts were on, so
                         *   it may not get the message for a while... */
                        __proc_unlock_ipi_pending(p, self_ipi_pending);
                        break;
                default:
-                       spin_unlock_irqsave(&p->proc_lock);
+                       spin_unlock(&p->proc_lock);
                        panic("Invalid process state %p in proc_run()!!", p->state);
        }
 }
@@ -536,7 +533,7 @@ void proc_restartcore(struct proc *p, trapframe_t *tf)
 void proc_destroy(struct proc *p)
 {
        bool self_ipi_pending = FALSE;
-       spin_lock_irqsave(&p->proc_lock);
+       spin_lock(&p->proc_lock);
 
        /* TODO: (DEATH) look at this again when we sort the __death IPI */
        if (current == p)
@@ -670,13 +667,13 @@ void proc_yield(struct proc *SAFE p, bool being_nice)
        if (being_nice && !vc->preempt_pending)
                return;
 
-       spin_lock_irqsave(&p->proc_lock); /* horrible scalability.  =( */
+       spin_lock(&p->proc_lock); /* horrible scalability.  =( */
 
        /* fate is sealed, return and take the preempt message on the way out.
         * we're making this check while holding the lock, since the preemptor
         * should hold the lock when sending messages. */
        if (vc->preempt_served) {
-               spin_unlock_irqsave(&p->proc_lock);
+               spin_unlock(&p->proc_lock);
                return;
        }
        /* no need to preempt later, since we are yielding (nice or otherwise) */
@@ -695,7 +692,7 @@ void proc_yield(struct proc *SAFE p, bool being_nice)
                        /* Ghetto, for OSDI: */
                        printk("[K] Process %d is yielding on vcore %d\n", p->pid, get_vcoreid(p, core_id()));
                        if (p->procinfo->num_vcores == 1) {
-                               spin_unlock_irqsave(&p->proc_lock);
+                               spin_unlock(&p->proc_lock);
                                return;
                        }
 #endif /* __CONFIG_OSDI__ */
@@ -720,7 +717,7 @@ void proc_yield(struct proc *SAFE p, bool being_nice)
                        panic("Weird state(%s) in %s()", procstate2str(p->state),
                              __FUNCTION__);
        }
-       spin_unlock_irqsave(&p->proc_lock);
+       spin_unlock(&p->proc_lock);
        proc_decref(p, 1); // need to eat the ref passed in.
        /* Clean up the core and idle.  For mgmt cores, they will ultimately call
         * manager, which will call schedule() and will repick the yielding proc. */
@@ -757,16 +754,18 @@ void do_notify(struct proc *p, uint32_t vcoreid, unsigned int notif,
        if (nm->flags & NOTIF_IPI && !vcpd->notif_pending) {
                vcpd->notif_pending = TRUE;
                if (vcpd->notif_enabled) {
-                       spin_lock_irqsave(&p->proc_lock);
-                               if ((p->state & PROC_RUNNING_M) && // TODO: (VC#) (_S state)
-                                             (p->procinfo->vcoremap[vcoreid].valid)) {
-                                       printk("[kernel] sending notif to vcore %d\n", vcoreid);
-                                       send_kernel_message(p->procinfo->vcoremap[vcoreid].pcoreid,
-                                                           __notify, p, 0, 0, KMSG_ROUTINE);
-                               } else { // TODO: think about this, fallback, etc
-                                       warn("Vcore unmapped, not receiving an active notif");
-                               }
-                       spin_unlock_irqsave(&p->proc_lock);
+                       /* GIANT WARNING: we aren't using the proc-lock to protect the
+                        * vcoremap.  We want to be able to use this from interrupt context,
+                        * and don't want the proc_lock to be an irqsave.
+                        */
+                       if ((p->state & PROC_RUNNING_M) && // TODO: (VC#) (_S state)
+                                     (p->procinfo->vcoremap[vcoreid].valid)) {
+                               printk("[kernel] sending notif to vcore %d\n", vcoreid);
+                               send_kernel_message(p->procinfo->vcoremap[vcoreid].pcoreid,
+                                                   __notify, p, 0, 0, KMSG_ROUTINE);
+                       } else { // TODO: think about this, fallback, etc
+                               warn("Vcore unmapped, not receiving an active notif");
+                       }
                }
        }
 }
@@ -799,20 +798,20 @@ uint32_t proc_get_vcoreid(struct proc *SAFE p, uint32_t pcoreid)
 {
        uint32_t vcoreid;
        // TODO: the code currently doesn't track the vcoreid properly for _S (VC#)
-       spin_lock_irqsave(&p->proc_lock);
+       spin_lock(&p->proc_lock);
        switch (p->state) {
                case PROC_RUNNING_S:
-                       spin_unlock_irqsave(&p->proc_lock);
+                       spin_unlock(&p->proc_lock);
                        return 0; // TODO: here's the ugly part
                case PROC_RUNNING_M:
                        vcoreid = get_vcoreid(p, pcoreid);
-                       spin_unlock_irqsave(&p->proc_lock);
+                       spin_unlock(&p->proc_lock);
                        return vcoreid;
                case PROC_DYING: // death message is on the way
-                       spin_unlock_irqsave(&p->proc_lock);
+                       spin_unlock(&p->proc_lock);
                        return 0;
                default:
-                       spin_unlock_irqsave(&p->proc_lock);
+                       spin_unlock(&p->proc_lock);
                        panic("Weird state(%s) in %s()", procstate2str(p->state),
                              __FUNCTION__);
        }
@@ -1023,7 +1022,11 @@ bool __proc_take_allcores(struct proc *SAFE p, amr_t message,
  * might cause an IPI to be sent.  There should already be a kmsg waiting for
  * us, since when we checked state to see a message was coming, the message had
  * already been sent before unlocking.  Note we do not need interrupts enabled
- * for this to work (you can receive a message before its IPI by polling).
+ * for this to work (you can receive a message before its IPI by polling).  Also
+ * note that the actual interrupt will have often arrived earlier, since
+ * interrupts are usually enabled in process code.  This function is not as
+ * necessary as it once was, but still represents a useful thing.  It'll get
+ * changed soon.
  *
  * TODO: consider inlining this, so __FUNCTION__ works (will require effort in
  * core_request(). */
@@ -1031,11 +1034,11 @@ void __proc_unlock_ipi_pending(struct proc *p, bool ipi_pending)
 {
        if (ipi_pending) {
                p->env_refcnt--; // TODO: (REF) (atomics)
-               spin_unlock_irqsave(&p->proc_lock);
+               spin_unlock(&p->proc_lock);
                process_routine_kmsg();
                panic("stack-killing kmsg not found in %s!!!", __FUNCTION__);
        } else {
-               spin_unlock_irqsave(&p->proc_lock);
+               spin_unlock(&p->proc_lock);
        }
 }
 
@@ -1067,12 +1070,12 @@ void __unmap_vcore(struct proc *p, uint32_t vcoreid)
  * TODO: (REF) change to use CAS / atomics. */
 void proc_incref(struct proc *p, size_t count)
 {
-       spin_lock_irqsave(&p->proc_lock);
+       spin_lock(&p->proc_lock);
        if (p->env_refcnt)
                p->env_refcnt += count;
        else
                panic("Tried to incref a proc with no existing refernces!");
-       spin_unlock_irqsave(&p->proc_lock);
+       spin_unlock(&p->proc_lock);
 }
 
 /* When the kernel is done with a process, it decrements its reference count.
@@ -1081,13 +1084,15 @@ void proc_incref(struct proc *p, size_t count)
  * with the previous function (incref)
  *
  * TODO: (REF) change to use CAS.  Note that when we do so, we may be holding
- * the process lock when calling __proc_free(). */
+ * the process lock when calling __proc_free().  Think about what order to do
+ * those calls in (unlock, then decref?), and the race with someone unlocking
+ * while someone else is __proc_free()ing. */
 void proc_decref(struct proc *p, size_t count)
 {
-       spin_lock_irqsave(&p->proc_lock);
+       spin_lock(&p->proc_lock);
        p->env_refcnt -= count;
        size_t refcnt = p->env_refcnt; // need to copy this in so it's not reloaded
-       spin_unlock_irqsave(&p->proc_lock);
+       spin_unlock(&p->proc_lock);
        // if we hit 0, no one else will increment and we can check outside the lock
        if (!refcnt)
                __proc_free(p);
@@ -1268,7 +1273,7 @@ void print_proc_info(pid_t pid)
                return;
        }
        spinlock_debug(&p->proc_lock);
-       spin_lock_irqsave(&p->proc_lock);
+       spin_lock(&p->proc_lock);
        printk("struct proc: %p\n", p);
        printk("PID: %d\n", p->pid);
        printk("PPID: %d\n", p->ppid);
@@ -1289,6 +1294,6 @@ void print_proc_info(pid_t pid)
                       p->resources[i].amt_wanted, p->resources[i].amt_granted);
        printk("Vcore 0's Last Trapframe:\n");
        print_trapframe(&p->env_tf);
-       spin_unlock_irqsave(&p->proc_lock);
+       spin_unlock(&p->proc_lock);
        proc_decref(p, 1); /* decref for the pid2proc reference */
 }
index 2024518..79c4957 100644 (file)
@@ -42,7 +42,7 @@ ssize_t core_request(struct proc *p)
        bool need_to_idle = FALSE;
        bool self_ipi_pending = FALSE;
 
-       spin_lock_irqsave(&p->proc_lock);
+       spin_lock(&p->proc_lock);
        /* check to see if this is a full deallocation.  for cores, it's a
         * transition from _M to _S.  Will be issues with handling this async. */
        if (!p->resources[RES_CORES].amt_wanted) {
@@ -65,10 +65,10 @@ ssize_t core_request(struct proc *p)
                  p->resources[RES_CORES].amt_granted;
        if (amt_new < 0) {
                p->resources[RES_CORES].amt_wanted = p->resources[RES_CORES].amt_granted;
-               spin_unlock_irqsave(&p->proc_lock);
+               spin_unlock(&p->proc_lock);
                return -EINVAL;
        } else if (amt_new == 0) {
-               spin_unlock_irqsave(&p->proc_lock);
+               spin_unlock(&p->proc_lock);
                return 0;
        }
        // else, we try to handle the request
@@ -150,7 +150,7 @@ ssize_t core_request(struct proc *p)
                        abandon_core();
                }
        } else { // nothing granted, just return
-               spin_unlock_irqsave(&p->proc_lock);
+               spin_unlock(&p->proc_lock);
        }
        return num_granted;
 }
@@ -166,12 +166,12 @@ error_t resource_req(struct proc *p, int type, size_t amt_wanted,
                printk("[kernel] Async requests treated synchronously for now.\n");
 
        /* set the desired resource amount in the process's resource list. */
-       spin_lock_irqsave(&p->proc_lock);
+       spin_lock(&p->proc_lock);
        size_t old_amount = p->resources[type].amt_wanted;
        p->resources[type].amt_wanted = amt_wanted;
        p->resources[type].amt_wanted_min = MIN(amt_wanted_min, amt_wanted);
        p->resources[type].flags = flags;
-       spin_unlock_irqsave(&p->proc_lock);
+       spin_unlock(&p->proc_lock);
 
        // no change in the amt_wanted
        if (old_amount == amt_wanted)
index 15e35a0..e5f6bfb 100644 (file)
@@ -219,7 +219,7 @@ static error_t sys_proc_run(struct proc *p, unsigned pid)
        if (!target)
                return -EBADPROC;
        // note we can get interrupted here. it's not bad.
-       spin_lock_irqsave(&p->proc_lock);
+       spin_lock(&p->proc_lock);
        // make sure we have access and it's in the right state to be activated
        if (!proc_controls(p, target)) {
                proc_decref(target, 1);
@@ -231,7 +231,7 @@ static error_t sys_proc_run(struct proc *p, unsigned pid)
                __proc_set_state(target, PROC_RUNNABLE_S);
                schedule_proc(target);
        }
-       spin_unlock_irqsave(&p->proc_lock);
+       spin_unlock(&p->proc_lock);
        proc_decref(target, 1);
        return retval;
 }
@@ -490,7 +490,7 @@ static intreg_t sys_munmap(struct proc* p, void* addr, size_t len)
 static void* sys_brk(struct proc *p, void* addr) {
        ssize_t range;
 
-       spin_lock_irqsave(&p->proc_lock);
+       spin_lock(&p->proc_lock);
 
        if((addr < p->procinfo->heap_bottom) || (addr >= (void*)BRK_END))
                goto out;
@@ -511,7 +511,7 @@ static void* sys_brk(struct proc *p, void* addr) {
        p->heap_top = addr;
 
 out:
-       spin_unlock_irqsave(&p->proc_lock);
+       spin_unlock(&p->proc_lock);
        return p->heap_top;
 }