Fixes vmm_struct_cleanup
authorBarret Rhoden <brho@cs.berkeley.edu>
Mon, 23 Mar 2015 16:24:26 +0000 (12:24 -0400)
committerBarret Rhoden <brho@cs.berkeley.edu>
Mon, 23 Mar 2015 19:18:55 +0000 (15:18 -0400)
Cleanup is called from __proc_free, and I don't want to sleep in there.

Running the existing code with the lock debugger will flip out.  Even
attempting to qlock while holding a spinlock will panic, rightly so.
The lock held happens to be the parent's child lock, which it holds when
it decrefs the child.  Regardless, proc_decref is called all over the
place, and I'd rather not sleep in the release method.

I considered removing the qlock and having a multi-state atomic_t that
we CAS on, solving the issue of multiple potential initializers and
cleanup not checking the lock.  But that's a bit of overkill.

kern/arch/x86/vmm/vmm.c
kern/arch/x86/vmm/vmm.h
kern/src/process.c

index 7a8f8d2..4af9853 100644 (file)
@@ -80,6 +80,8 @@ int vmm_struct_init(struct vmm *vmm, unsigned int nr_guest_pcores)
                qunlock(&vmm->qlock);
                return 0;
        }
+       /* Set this early, so cleanup checks the gpc array */
+       vmm->vmmcp = TRUE;
        nr_guest_pcores = MIN(nr_guest_pcores, num_cpus);
        vmm->amd = 0;
        vmm->guest_pcores = kzmalloc(sizeof(void*) * nr_guest_pcores, KMALLOC_WAIT);
@@ -92,23 +94,21 @@ int vmm_struct_init(struct vmm *vmm, unsigned int nr_guest_pcores)
                }
        }
        vmm->nr_guest_pcores = i;
-       vmm->vmmcp = TRUE;
        qunlock(&vmm->qlock);
        return i;
 }
 
-void vmm_struct_cleanup(struct vmm *vmm)
+/* Has no concurrency protection - only call this when you know you have the
+ * only ref to vmm.  For instance, from __proc_free, where there is only one ref
+ * to the proc (and thus proc.vmm). */
+void __vmm_struct_cleanup(struct vmm *vmm)
 {
-       qlock(&vmm->qlock);
-       if (!vmm->vmmcp) {
-               qunlock(&vmm->qlock);
+       if (!vmm->vmmcp)
                return;
-       }
        for (int i = 0; i < vmm->nr_guest_pcores; i++) {
                if (vmm->guest_pcores[i])
                        vmx_destroy_vcpu(vmm->guest_pcores[i]);
        }
        kfree(vmm->guest_pcores);
        vmm->vmmcp = FALSE;
-       qunlock(&vmm->qlock);
 }
index 27f248b..5945df5 100644 (file)
@@ -41,7 +41,7 @@ void vmm_init(void);
 void vmm_pcpu_init(void);
 
 int vmm_struct_init(struct vmm *vmm, unsigned int nr_guest_pcores);
-void vmm_struct_cleanup(struct vmm *vmm);
+void __vmm_struct_cleanup(struct vmm *vmm);
 
 int vm_run(uint64_t,uint64_t, uint64_t);
 int intel_vmx_start(int id);
index 68b3357..3b05c08 100644 (file)
@@ -458,7 +458,7 @@ static void __proc_free(struct kref *kref)
        assert(kref_refcnt(&p->p_kref) == 0);
        assert(TAILQ_EMPTY(&p->alarmset.list));
 
-       vmm_struct_cleanup(&p->vmm);
+       __vmm_struct_cleanup(&p->vmm);
        p->progname[0] = 0;
        cclose(p->dot);
        cclose(p->slash);