Breaks up the proc_lock to use the mm_lock
authorBarret Rhoden <brho@cs.berkeley.edu>
Fri, 16 Sep 2011 23:01:02 +0000 (16:01 -0700)
committerKevin Klues <klueska@cs.berkeley.edu>
Thu, 3 Nov 2011 00:36:07 +0000 (17:36 -0700)
While this has been needed for a long time, it now is critical due to
event code holding the proc_lock and then posting an INDIR (which may
mmap()).  It's a fairly rare condition (which I didn't get yet), but it
will deadlock.

As it is now, we're okay, so long as our mmap() call doesn't try a
tlbshootdown (which needs the proclock for now).

kern/include/env.h
kern/include/mm.h
kern/include/process.h
kern/src/event.c
kern/src/mm.c
kern/src/process.c
kern/src/ucq.c

index 65ae839..e9fe00f 100644 (file)
@@ -58,6 +58,7 @@ struct proc {
        // Address space
        pde_t *COUNT(NPDENTRIES) env_pgdir;                     // Kernel virtual address of page dir
        physaddr_t env_cr3;                     // Physical address of page dir
+       spinlock_t mm_lock;             /* Protects page tables and VMRs (mem mgmt) */
        struct vmr_tailq vm_regions;
 
        // Per process info and data pages
index 3ffb02a..547ab76 100644 (file)
@@ -74,7 +74,7 @@ int mprotect(struct proc *p, uintptr_t addr, size_t len, int prot);
 int munmap(struct proc *p, uintptr_t addr, size_t len);
 int handle_page_fault(struct proc *p, uintptr_t va, int prot);
 
-/* These assume the memory/proc_lock is held already */
+/* These assume the mm_lock is held already */
 void *__do_mmap(struct proc *p, uintptr_t addr, size_t len, int prot, int flags,
                 struct file *f, size_t offset);
 int __do_mprotect(struct proc *p, uintptr_t addr, size_t len, int prot);
index 3eac075..68f91b9 100644 (file)
@@ -136,10 +136,7 @@ void proc_preempt_all(struct proc *p, uint64_t usec);
 struct proc *switch_to(struct proc *new_p);
 void switch_back(struct proc *new_p, struct proc *old_proc);
 void abandon_core(void);
-
-/* Hold the proc_lock, since it'll use the vcoremapping to send an unmapping
- * message for the region from start to end.  */
-void __proc_tlbshootdown(struct proc *p, uintptr_t start, uintptr_t end);
+void proc_tlbshootdown(struct proc *p, uintptr_t start, uintptr_t end);
 
 /* Kernel message handlers for process management */
 void __startcore(struct trapframe *tf, uint32_t srcid, long a0, long a1,
index 4f17c5b..0fbeaa5 100644 (file)
@@ -249,7 +249,9 @@ ultimate_fallback:
        /* At this point, we can't find one.  This could be due to a (hopefully
         * rare) weird yield/request storm, or more commonly because the lists were
         * empty and the process is simply WAITING (yielded all of its vcores and is
-        * waiting on an event).  Time for the ultimate fallback: locking. */
+        * waiting on an event).  Time for the ultimate fallback: locking.  Note
+        * that when we __alert_vcore(), there is a chance we need to mmap, which
+        * grabs the mm_lock. */
        spin_lock(&p->proc_lock);
        if (p->state != PROC_WAITING) {
                /* We need to check the online and bulk_preempt lists again, now that we are
index 444ee73..6d35f16 100644 (file)
@@ -7,8 +7,8 @@
  *
  * In general, error checking / bounds checks are done in the main function
  * (e.g. mmap()), and the work is done in a do_ function (e.g. do_mmap()).
- * Versions of those functions that are called when the memory lock (proc_lock
- * for now) is already held begin with __ (e.g. __do_munmap()).
+ * Versions of those functions that are called when the memory lock (mm_lock) is
+ * already held begin with __ (e.g. __do_munmap()).
  *
  * Note that if we were called from kern/src/syscall.c, we probably don't have
  * an edible reference to p. */
@@ -335,10 +335,9 @@ void *mmap(struct proc *p, uintptr_t addr, size_t len, int prot, int flags,
 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(&p->proc_lock);
+       spin_lock(&p->mm_lock);
        void *ret = __do_mmap(p, addr, len, prot, flags, file, offset);
-       spin_unlock(&p->proc_lock);
+       spin_unlock(&p->mm_lock);
        return ret;
 }
 
@@ -423,9 +422,9 @@ int mprotect(struct proc *p, uintptr_t addr, size_t len, int prot)
                set_errno(ENOMEM);
                return -1;
        }
-       spin_lock(&p->proc_lock);
+       spin_lock(&p->mm_lock);
        int ret = __do_mprotect(p, addr, len, prot);
-       spin_unlock(&p->proc_lock);
+       spin_unlock(&p->mm_lock);
        return ret;
 }
 
@@ -492,7 +491,7 @@ int __do_mprotect(struct proc *p, uintptr_t addr, size_t len, int prot)
                vmr = next_vmr;
        }
        if (shootdown_needed)
-               __proc_tlbshootdown(p, addr, addr + len);
+               proc_tlbshootdown(p, addr, addr + len);
        return 0;
 }
 
@@ -510,9 +509,9 @@ int munmap(struct proc *p, uintptr_t addr, size_t len)
                set_errno(EINVAL);
                return -1;
        }
-       spin_lock(&p->proc_lock);
+       spin_lock(&p->mm_lock);
        int ret = __do_munmap(p, addr, len);
-       spin_unlock(&p->proc_lock);
+       spin_unlock(&p->mm_lock);
        return ret;
 }
 
@@ -552,7 +551,7 @@ int __do_munmap(struct proc *p, uintptr_t addr, size_t len)
                vmr = next_vmr;
        }
        if (shootdown_needed)
-               __proc_tlbshootdown(p, addr, addr + len);
+               proc_tlbshootdown(p, addr, addr + len);
        return 0;
 }
 
@@ -562,14 +561,14 @@ 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(&p->proc_lock);
+       spin_lock(&p->mm_lock);
        int ret = __handle_page_fault(p, va, prot);
-       spin_unlock(&p->proc_lock);
+       spin_unlock(&p->mm_lock);
        return ret;
 }
 
 /* Returns 0 on success, or an appropriate -error code.  Assumes you hold the
- * appropriate lock.
+ * mm_lock.
  *
  * Notes: if your TLB caches negative results, you'll need to flush the
  * appropriate tlb entry.  Also, you could have a weird race where a present PTE
index e360391..448e3d2 100644 (file)
@@ -316,6 +316,7 @@ error_t proc_alloc(struct proc **pp, struct proc *parent)
        memset(&p->resources, 0, sizeof(p->resources));
        memset(&p->env_ancillary_state, 0, sizeof(p->env_ancillary_state));
        memset(&p->env_tf, 0, sizeof(p->env_tf));
+       spinlock_init(&p->mm_lock);
        TAILQ_INIT(&p->vm_regions); /* could init this in the slab */
        /* Initialize the vcore lists, we'll build the inactive list so that it includes
         * all vcores when we initialize procinfo.  Do this before initing procinfo. */
@@ -1419,15 +1420,16 @@ void switch_back(struct proc *new_p, struct proc *old_proc)
  * shootdown and batching our messages.  Should do the sanity about rounding up
  * and down in this function too.
  *
- * Hold the proc_lock before calling this.
- *
  * Would be nice to have a broadcast kmsg at this point.  Note this may send a
  * message to the calling core (interrupting it, possibly while holding the
  * proc_lock).  We don't need to process routine messages since it's an
  * immediate message. */
-void __proc_tlbshootdown(struct proc *p, uintptr_t start, uintptr_t end)
+void proc_tlbshootdown(struct proc *p, uintptr_t start, uintptr_t end)
 {
        struct vcore *vc_i;
+       /* TODO: we might be able to avoid locking here in the future (we must hit
+        * all online, and we can check __mapped).  it'll be complicated. */
+       spin_lock(&p->proc_lock);
        switch (p->state) {
                case (PROC_RUNNING_S):
                        tlbflush();
@@ -1448,6 +1450,7 @@ void __proc_tlbshootdown(struct proc *p, uintptr_t start, uintptr_t end)
                        warn("Unexpected case %s in %s", procstate2str(p->state),
                             __FUNCTION__);
        }
+       spin_unlock(&p->proc_lock);
 }
 
 /* Kernel message handler to start a process's context on this core.  Tightly
index 0e15b49..b0e64ec 100644 (file)
@@ -63,6 +63,9 @@ grab_lock:
                /* Warn if we have a ridiculous amount of pages in the ucq */
                if (atomic_fetch_and_add(&ucq->nr_extra_pgs, 1) > UCQ_WARN_THRESH)
                        warn("Over %d pages in ucq %08p!\n", UCQ_WARN_THRESH, ucq);
+               /* Giant warning: don't ask for anything other than anonymous memory at
+                * a non-fixed location.  o/w, it may cause a TLB shootdown, which grabs
+                * the proc_lock, and potentially deadlock the system. */
                new_page = (struct ucq_page*)do_mmap(p, 0, PGSIZE,
                                                     PROT_READ | PROT_WRITE,
                                                     MAP_ANON | MAP_POPULATE, 0, 0);