Added krefs, used them for process refcounting
authorBarret Rhoden <brho@cs.berkeley.edu>
Thu, 29 Jul 2010 01:09:40 +0000 (18:09 -0700)
committerKevin Klues <klueska@cs.berkeley.edu>
Thu, 3 Nov 2011 00:35:50 +0000 (17:35 -0700)
krefs: the approved solution for reference counting.

Removes old proc_increfs, decrefs, manual env_refcnting, cries for
lockless refcounting, a couple bugs, and eleven TODOs!

15 files changed:
Documentation/process-internals.txt
kern/arch/i686/process.c
kern/arch/i686/trap.c
kern/arch/sparc/process.c
kern/arch/sparc/trap.c
kern/include/env.h
kern/include/kref.h [new file with mode: 0644]
kern/include/process.h
kern/include/ros/common.h
kern/src/manager.c
kern/src/monitor.c
kern/src/process.c
kern/src/resource.c
kern/src/schedule.c
kern/src/syscall.c

index 3ef4e58..3e6bcb8 100644 (file)
@@ -20,23 +20,21 @@ Contents:
 ===========================
 1.1 Basics:
 ---------------------------
-Reference counts (proc_refcnt) exist to keep a process alive.  Eventually, this
-will probably turn into a regular "kernel design pattern", like it is in Linux
-(http://lwn.net/Articles/336224/).  The style of reference counting we use for
-processes is similar to a kref:
+Reference counts exist to keep a process alive.  We use krefs for this, similar
+to Linux's kref:
 - Can only incref if the current value is greater than 0, meaning there is
   already a reference to it.  It is a bug to try to incref on something that has
   no references, so always make sure you incref something that you know has a
-  reference.  If you don't know, you need to get it manually (CAREFULLY!) or use
-  pid2proc (which is a careful way of doing this).  If you incref and there are
-  0 references, the kernel will panic.  Fix your bug / don't incref random
-  pointers.
+  reference.  If you don't know, you need to get it from pid2proc (which is a
+  careful way of doing this).  pid2proc kref_get()s on the reference that is
+  stored inside it.  If you incref and there are 0 references, the kernel will
+  panic.  Fix your bug / don't incref random pointers.
 - Can always decref.
 - When the decref returns 0, perform some operation.  This does some final
   cleanup on the object.
-
-For a process, proc_destroy() decrefs, and other codes using the proc also
-decref.  The last one to decref calls proc_free to do the final cleanup.
+- Process code is trickier since we frequently make references from 'current'
+  (which isn't too bad), but also because we often do not return and need to be
+  careful about the references we passed in to a no-return function.
 
 1.2 Brief History of the Refcnt:
 ---------------------------
@@ -60,7 +58,12 @@ protected by a refcnt.
 ---------------------------
 +1 for existing.
 - The fact that the process is supposed to exist is worth +1.  When it is time
-  to die, we decref, and it will eventually be cleaned up.
+  to die, we decref, and it will eventually be cleaned up.  This existence is
+  based on it's presence in the hash table, which is a stored reference.
+- The hash table is a bit tricky.  We need to kref_get() when it is locked, so
+  we know we have a valid kref to get.  We don't need to lock the list to
+  kref_put the process, since once it is removed from the hash, we now own that
+  reference.  
 
 +1 for someone using it or planning to use it.
 - This includes simply having a pointer to the proc, since presumably you will
index ac798e7..70c6fea 100644 (file)
@@ -66,6 +66,6 @@ void __abandon_core(void)
 {
        asm volatile ("movw %%ax,%%gs; lldt %%ax" :: "a"(0));
        lcr3(boot_cr3);
-       proc_decref(current, 1);
+       kref_put(&current->kref);
        set_current_proc(NULL);
 }
index f4f8c35..889564a 100644 (file)
@@ -193,12 +193,12 @@ trap_dispatch(trapframe_t *tf)
                        assert(tf->tf_cs != GD_KT);
 
                        // syscall code wants an edible reference for current
-                       proc_incref(current, 1);
+                       kref_get(&current->kref, 1);
                        tf->tf_regs.reg_eax =
                                syscall(current, tf->tf_regs.reg_eax, tf->tf_regs.reg_edx,
                                        tf->tf_regs.reg_ecx, tf->tf_regs.reg_ebx,
                                        tf->tf_regs.reg_edi, tf->tf_regs.reg_esi);
-                       proc_decref(current, 1);
+                       kref_put(&current->kref);
                        break;
                default:
                        // Unexpected trap: The user process or the kernel has a bug.
@@ -207,7 +207,7 @@ trap_dispatch(trapframe_t *tf)
                                panic("Damn Damn!  Unhandled trap in the kernel!");
                        else {
                                warn("Unexpected trap from userspace");
-                               proc_incref(current, 1);
+                               kref_get(&current->kref, 1);
                                proc_destroy(current);
                                return;
                        }
@@ -352,7 +352,7 @@ page_fault_handler(trapframe_t *tf)
                cprintf("[%08x] user fault va %08x ip %08x from core %d\n",
                        current->pid, fault_va, tf->tf_eip, core_id());
                print_trapframe(tf);
-               proc_incref(current, 1);
+               kref_get(&current->kref, 1);
                proc_destroy(current);
        }
 }
@@ -371,7 +371,7 @@ void sysenter_callwrapper(struct trapframe *tf)
                set_current_tf(tf);
 
        // syscall code wants an edible reference for current
-       proc_incref(current, 1);
+       kref_get(&current->kref, 1);
        tf->tf_regs.reg_eax = (intreg_t) syscall(current,
                                                 tf->tf_regs.reg_eax,
                                                 tf->tf_regs.reg_esi,
@@ -379,7 +379,7 @@ void sysenter_callwrapper(struct trapframe *tf)
                                                 tf->tf_regs.reg_ebx,
                                                 tf->tf_regs.reg_edi,
                                                 0);
-       proc_decref(current, 1);
+       kref_put(&current->kref);
        /*
         * careful here - we need to make sure that this current is the right
         * process, which could be weird if the syscall blocked.  it would need to
index 3ea9af3..1a00388 100644 (file)
@@ -49,6 +49,6 @@ void proc_set_syscall_retval(trapframe_t *SAFE tf, intreg_t value)
 void __abandon_core(void)
 {
        lcr3(boot_cr3);
-       proc_decref(current, 1);
+       kref_put(&current->kref);
        set_current_proc(NULL);
 }
index 0924530..7ed17a6 100644 (file)
@@ -282,7 +282,7 @@ unhandled_trap(trapframe_t* state)
                spin_unlock(&screwup_lock);
 
                assert(current);
-               proc_incref(current, 1);
+               kref_get(&current->kref, 1);
                proc_destroy(current);
 
                panic("I shouldn't have gotten here!");
@@ -423,9 +423,9 @@ handle_syscall(trapframe_t* state)
        set_current_tf(state);
 
        // syscall code wants an edible reference for current
-       proc_incref(current, 1);
+       kref_get(&current->kref, 1);
        state->gpr[8] = syscall(current,num,a1,a2,a3,a4,a5);
-       proc_decref(current, 1);
+       kref_put(&current->kref);
 
        proc_restartcore(current,state);
 }
index 3f5c672..d36ed41 100644 (file)
@@ -30,7 +30,7 @@ struct proc {
        pid_t ppid;                 // Parent's PID
        pid_t exitcode;                         // exit() param or main() return value
        uint32_t state;                         // Status of the process
-       uint32_t env_refcnt;            // Reference count of kernel contexts using this
+       struct kref kref;               /* Refcnt */
        uint32_t env_flags;
        uint32_t env_entry;
 
diff --git a/kern/include/kref.h b/kern/include/kref.h
new file mode 100644 (file)
index 0000000..33c8d32
--- /dev/null
@@ -0,0 +1,74 @@
+/* Copyright (c) 2010 The Regents of the University of California
+ * Barret Rhoden <brho@cs.berkeley.edu>
+ * See LICENSE for details.
+ *
+ * Kernel reference counting, based on Linux's kref:
+ * - http://www.kroah.com/linux/talks/ols_2004_kref_paper/
+ *          Reprint-Kroah-Hartman-OLS2004.pdf
+ * - http://lwn.net/Articles/336224/
+ * - Linux's Documentation/kref.txt
+ *
+ * We differ a bit in that we currently ref count items that are on lists.  If
+ * an item is stored on a list, that counts as a reference.  No need to lock
+ * around kref_put, nor do you need to kref_get your list reference *if* you
+ * take the reference out of the list.  You need to kref_get() (if you want to
+ * use the reference later) before allowing someone else access to the list,
+ * which is still IAW Linux's style.  They might even do this for some lists. If
+ * we have lists that are unsynchronized where two threads can get references to
+ * the same item at the same time, then we'll need to lock to deal with that.
+ *
+ * We also allow incrementing by more than one, which helps in some cases.  We
+ * don't allow decrementing by more than one to catch bugs (for now).
+ *
+ * As far as usage goes, kref users don't make much of a distinction between
+ * internal and external references yet.
+ *
+ * kref rules (paraphrasing the linux ones):
+ * 1. If you pass a pointer somewhere or store it, kref_get() it first.  You can
+ * do this with no locking if you have a valid reference.
+ * 2. When you are done, kref_put() it.  You can usually do this without
+ * locking.
+ * 3. If you never kref_get without already holding a valid reference, you don't
+ * need to lock for Rule 2.  If you ever grab a reference without already having
+ * one, you need some form of sync to prevent a kref_put() from happening
+ * while you kref_get().
+ *
+ * The closest we get to mucking with it is with the proc hash table, though we
+ * don't require a lock on every proc kref_put().  If you're
+ * curious about these sorts of things, note how easy it is for a list where you
+ * are added or removed (like the runnable list) compared to a structure where
+ * we make a copy of the reference (like the pid2proc hash). */
+
+#include <atomic.h>
+#include <assert.h>
+
+/* Current versions of Linux pass in 'release' on the kref_put() callsite to
+ * save on space in whatever struct these are embedded in.  We don't, since it's
+ * a little more complicated and we will probably change the release function a
+ * lot for subsystems in development. */
+struct kref {
+       atomic_t refcount;
+       void (*release)(struct kref *kref);
+};
+
+static void kref_init(struct kref *kref, void (*release)(struct kref *kref),
+                      unsigned int init)
+{
+       assert(release);
+       atomic_init(&kref->refcount, init);
+       kref->release = release;
+}
+
+static struct kref *kref_get(struct kref *kref, unsigned int inc)
+{
+       assert(atomic_read(&kref->refcount));
+       atomic_add(&kref->refcount, inc);
+       return kref;
+}
+
+static void kref_put(struct kref *kref)
+{
+       assert(atomic_read(&kref->refcount) > 0);               /* catch some bugs */
+       if (atomic_sub_and_test(&kref->refcount, 1))
+               kref->release(kref);
+}
index e4dc7ff..8237d10 100644 (file)
@@ -12,6 +12,7 @@
 #include <ros/notification.h>
 #include <trap.h>
 #include <atomic.h>
+#include <kref.h>
 
 /* Process States.  Not 100% on the names yet.  RUNNABLE_* are waiting to go to
  * RUNNING_*.  For instance, RUNNABLE_M is expecting to go to RUNNING_M.  It
@@ -125,10 +126,6 @@ bool __proc_preempt_all(struct proc *p);
 void proc_preempt_core(struct proc *p, uint32_t pcoreid, uint64_t usec);
 void proc_preempt_all(struct proc *p, uint64_t usec);
 
-/* Will probably have generic versions of these later. */
-void proc_incref(struct proc *SAFE p, size_t count);
-void proc_decref(struct proc *SAFE p, size_t count);
 /* Allows the kernel to figure out what process is running on this core.  Can be
  * used just like a pointer to a struct proc.  Need these to be macros due to
  * some circular dependencies with smp.h. */
index 0a2203f..5a6b43a 100644 (file)
@@ -102,6 +102,11 @@ static inline uint32_t ROUNDUPPWR2(uint32_t value)
 #define offsetof(type, member)  ((size_t) (&((type*)0)->member))
 #endif
 
+/* Return the container/struct holding the object 'ptr' points to */
+#define container_of(ptr, type, member) ({                                     \
+       (type*)((char*)ptr - offsetof(type, member));                             \
+})
+
 // Ivy currently can only handle 63 bits (OCaml thing), so use this to make
 // a uint64_t programatically
 #define UINT64(upper, lower) ( (((uint64_t)(upper)) << 32) | (lower) )
index b224db2..f4f9391 100644 (file)
@@ -64,7 +64,7 @@ void manager(void)
        __proc_set_state((p), PROC_RUNNABLE_S);                                      \
        spin_unlock(&(p)->proc_lock);                                                \
        proc_run((p));                                                               \
-       proc_decref((p), 1);
+       kref_put(&(p)->kref);
 
 #define quick_proc_create(x, p, f)                                               \
        (f) = path_to_file((x));                                                     \
@@ -87,7 +87,7 @@ void manager(void)
        for (int i = 0; i < (c); i++)                                                \
                cache_color_alloc(llc_cache, p->cache_colors_map);                       \
        proc_run((p));                                                               \
-       proc_decref((p), 1);
+       kref_put(&(p)->kref);
 
 #define quick_proc_color_create(x, p, c, f)                                      \
        (f) = path_to_file((x));                                                     \
index 7f03f24..d89738b 100644 (file)
@@ -299,7 +299,7 @@ int mon_bin_run(int argc, char *NTS *NT COUNT(argc) argv, trapframe_t *tf)
        __proc_set_state(p, PROC_RUNNABLE_S);
        schedule_proc(p);
        spin_unlock(&p->proc_lock);
-       proc_decref(p, 1); /* let go of the reference created in proc_create() */
+       kref_put(&p->kref); /* let go of the reference created in proc_create() */
        /* TODO: (REF) need to dealloc when this hits 0, atomic/concurrent/etc */
        atomic_dec(&program->f_refcnt);
        /* Should never return from schedule (env_pop in there) also note you may
@@ -358,7 +358,7 @@ int mon_procinfo(int argc, char *NTS *NT COUNT(argc) argv, trapframe_t *tf)
                        return 1;
                }
                proc_destroy(p);
-               proc_decref(p, 1);
+               kref_put(&p->kref);
        } else {
                printk("Bad option\n");
                return 1;
@@ -458,11 +458,12 @@ int mon_notify(int argc, char *NTS *NT COUNT(argc) argv, trapframe_t *tf)
        } else {
                proc_notify(p, num, 0);
        }
-       proc_decref(p, 1);
+       kref_put(&p->kref);
        return 0;
 }
 
-/* Micro-benchmarky Measurements */
+/* Micro-benchmarky Measurements.  This is really fragile code that probably
+ * won't work perfectly, esp as the kernel evolves. */
 int mon_measure(int argc, char *NTS *NT COUNT(argc) argv, trapframe_t *tf)
 {
        uint64_t begin = 0, diff = 0;
@@ -493,14 +494,15 @@ int mon_measure(int argc, char *NTS *NT COUNT(argc) argv, trapframe_t *tf)
                begin = start_timing();
 #ifdef __CONFIG_APPSERVER__
                printk("Warning: this will be inaccurate due to the appserver.\n");
-               end_refcnt = p->env_refcnt - p->procinfo->num_vcores - 1;
+               end_refcnt = atomic_read(&p->kref.refcount) -
+                            p->procinfo->num_vcores - 1;
 #endif /* __CONFIG_APPSERVER__ */
                proc_destroy(p);
-               proc_decref(p, 1);
+               kref_put(&p->kref);
 #ifdef __CONFIG_APPSERVER__
                /* Won't be that accurate, since it's not actually going through the
                 * __proc_free() path. */
-               spin_on(p->env_refcnt != end_refcnt);   
+               spin_on(atomic_read(&p->kref.refcount) != end_refcnt);  
 #else
                /* this is a little ghetto. it's not fully free yet, but we are also
                 * slowing it down by messing with it, esp with the busy waiting on a
@@ -527,14 +529,15 @@ int mon_measure(int argc, char *NTS *NT COUNT(argc) argv, trapframe_t *tf)
                        spin_on(p->procinfo->pcoremap[pcoreid].valid);
                        diff = stop_timing(begin);
                } else { /* preempt all cores, warned but no delay */
-                       end_refcnt = p->env_refcnt - p->procinfo->num_vcores;
+                       end_refcnt = atomic_read(&p->kref.refcount)
+                                    - p->procinfo->num_vcores;
                        begin = start_timing();
                        proc_preempt_all(p, 1000000);
                        /* a little ghetto, implies no one is using p */
-                       spin_on(p->env_refcnt != end_refcnt);
+                       spin_on(atomic_read(&p->kref.refcount) != end_refcnt);
                        diff = stop_timing(begin);
                }
-               proc_decref(p, 1);
+               kref_put(&p->kref);
        } else if (!strcmp(argv[1], "preempt-warn")) {
                if (argc < 3) {
                        printk("Give me a pid number.\n");
@@ -571,7 +574,7 @@ int mon_measure(int argc, char *NTS *NT COUNT(argc) argv, trapframe_t *tf)
                        spin_on(p->procinfo->num_vcores > 1);
                        diff = stop_timing(begin);
                }
-               proc_decref(p, 1);
+               kref_put(&p->kref);
        } else if (!strcmp(argv[1], "preempt-raw")) {
                if (argc < 3) {
                        printk("Give me a pid number.\n");
@@ -600,17 +603,18 @@ int mon_measure(int argc, char *NTS *NT COUNT(argc) argv, trapframe_t *tf)
                        /* TODO: (RMS), if num_vcores == 0, RUNNABLE_M, schedule */
                } else { /* preempt all cores, no warning or waiting */
                        spin_lock(&p->proc_lock);
-                       end_refcnt = p->env_refcnt - p->procinfo->num_vcores;
+                       end_refcnt = atomic_read(&p->kref.refcount)
+                                    - p->procinfo->num_vcores;
                        begin = start_timing();
                        self_ipi_pending = __proc_preempt_all(p);
                        /* TODO: (RMS), RUNNABLE_M, schedule */
                        spin_unlock(&p->proc_lock);
                        __proc_kmsg_pending(p, self_ipi_pending);
                        /* a little ghetto, implies no one else is using p */
-                       spin_on(p->env_refcnt != end_refcnt);
+                       spin_on(atomic_read(&p->kref.refcount) != end_refcnt);
                        diff = stop_timing(begin);
                }
-               proc_decref(p, 1);
+               kref_put(&p->kref);
        } else {
                printk("Bad option\n");
                return 1;
index e470358..2a9bef5 100644 (file)
@@ -56,6 +56,7 @@ static uint32_t get_busy_vcoreid(struct proc *SAFE p, uint32_t prev);
 static bool is_mapped_vcore(struct proc *p, uint32_t pcoreid);
 static uint32_t get_vcoreid(struct proc *p, uint32_t pcoreid);
 static uint32_t get_pcoreid(struct proc *p, uint32_t vcoreid);
+static void __proc_free(struct kref *kref);
 
 /* PID management. */
 #define PID_MAX 32767 // goes from 0 to 32767, with 0 reserved
@@ -160,15 +161,19 @@ int __proc_set_state(struct proc *p, uint32_t state)
        return 0;
 }
 
-/* Returns a pointer to the proc with the given pid, or 0 if there is none */
+/* Returns a pointer to the proc with the given pid, or 0 if there is none.
+ * Note this makes a copy of the reference stored in the hash table (which is
+ * the proc existing).  Need to do this while locking the table, in case someone
+ * else subsequently removes it from the table, then kref_put()s it to 0 before
+ * we can get it.  Don't push the locking into the hashtable without dealing
+ * with this. */
 struct proc *pid2proc(pid_t pid)
 {
        spin_lock(&pid_hash_lock);
        struct proc *p = hashtable_search(pid_hash, (void*)pid);
-       spin_unlock(&pid_hash_lock);
-       /* if the refcnt was 0, decref and return 0 (we failed). (TODO) */
        if (p)
-               proc_incref(p, 1); // TODO:(REF) to do this all atomically and not panic
+               kref_get(&p->kref, 1);
+       spin_unlock(&pid_hash_lock);
        return p;
 }
 
@@ -266,6 +271,9 @@ error_t proc_alloc(struct proc **pp, struct proc *parent)
                kmem_cache_free(proc_cache, p);
                return -ENOFREEPID;
        }
+       /* one reference for the proc existing (in the hash table),
+        * and one for the ref we pass back */
+       kref_init(&p->kref, __proc_free, 2);
        spin_lock(&pid_hash_lock);
        hashtable_insert(pid_hash, (void*)p->pid, p);
        spin_unlock(&pid_hash_lock);
@@ -275,7 +283,6 @@ error_t proc_alloc(struct proc **pp, struct proc *parent)
        p->exitcode = 0;
        p->ppid = parent ? parent->pid : 0;
        p->state = PROC_CREATED; // shouldn't go through state machine for init
-       p->env_refcnt = 2; // one for the object, one for the ref we pass back
        p->env_flags = 0;
        p->env_entry = 0; // cheating.  this really gets set later
        p->procinfo->heap_bottom = (void*)UTEXT;
@@ -346,16 +353,17 @@ struct proc *proc_create(struct file *prog, char **argv, char **envp)
        return p;
 }
 
-/* This is called by proc_decref, once the last reference to the process is
+/* This is called by kref_put(), once the last reference to the process is
  * gone.  Don't call this otherwise (it will panic).  It will clean up the
  * address space and deallocate any other used memory. */
-static void __proc_free(struct proc *p)
+static void __proc_free(struct kref *kref)
 {
+       struct proc *p = container_of(kref, struct proc, kref);
        physaddr_t pa;
 
        printd("[PID %d] freeing proc: %d\n", current ? current->pid : 0, p->pid);
        // All parts of the kernel should have decref'd before __proc_free is called
-       assert(p->env_refcnt == 0);
+       assert(atomic_read(&p->kref.refcount) == 0);
 
        close_all_files(&p->open_files);
        frontend_proc_free(p);  /* TODO: please remove me one day */
@@ -365,7 +373,8 @@ static void __proc_free(struct proc *p)
                        cache_color_free(llc_cache, p->cache_colors_map);
                cache_colors_map_free(p->cache_colors_map);
        }
-
+       /* Give our PID back */
+       put_free_pid(p->pid);
        /* Flush all mapped pages in the user portion of the address space */
        env_user_mem_free(p, 0, UVPT);
        /* These need to be free again, since they were allocated with a refcnt. */
@@ -376,12 +385,6 @@ static void __proc_free(struct proc *p)
        p->env_pgdir = 0;
        p->env_cr3 = 0;
 
-       /* Remove self from the pid hash, return PID.  Note the reversed order. */
-       spin_lock(&pid_hash_lock);
-       if (!hashtable_remove(pid_hash, (void*)p->pid))
-               panic("Proc not in the pid table in %s", __FUNCTION__);
-       spin_unlock(&pid_hash_lock);
-       put_free_pid(p->pid);
        atomic_dec(&num_envs);
 
        /* Dealloc the struct proc */
@@ -436,7 +439,7 @@ void proc_run(struct proc *p)
                        /* __proc_startcore assumes the reference we give it is for current.
                         * Decref if current is already properly set. */
                        if (p == current)
-                               p->env_refcnt--; // TODO: (REF) use incref
+                               kref_put(&p->kref);
                        /* 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 want interrupts disabled so that if there was a
@@ -454,7 +457,7 @@ void proc_run(struct proc *p)
                                __proc_set_state(p, PROC_RUNNING_M);
                                /* Up the refcnt, since num_vcores are going to start using this
                                 * process and have it loaded in their 'current'. */
-                               p->env_refcnt += p->procinfo->num_vcores; // TODO: (REF) use incref
+                               kref_get(&p->kref, p->procinfo->num_vcores);
                                /* If the core we are running on is in the vcoremap, we will get
                                 * an IPI (once we reenable interrupts) and never return. */
                                if (is_mapped_vcore(p, core_id()))
@@ -510,7 +513,7 @@ static void __proc_startcore(struct proc *p, trapframe_t *tf)
                 * rarely happen, since we usually proactively leave process context,
                 * but is the fallback. */
                if (current)
-                       proc_decref(current, 1);
+                       kref_put(&current->kref);
                set_current_proc(p);
        }
        /* need to load our silly state, preferably somewhere other than here so we
@@ -599,7 +602,7 @@ void proc_destroy(struct proc *p)
                        // here's how to do it manually
                        if (current == p) {
                                lcr3(boot_cr3);
-                               proc_decref(p, 1); // this decref is for the cr3
+                               kref_put(&p->kref);             /* this decref is for the cr3 */
                                current = NULL;
                        }
                        #endif
@@ -628,15 +631,25 @@ void proc_destroy(struct proc *p)
                              __FUNCTION__);
        }
        __proc_set_state(p, PROC_DYING);
-       /* this decref is for the process in general */
-       p->env_refcnt--; // TODO (REF)
-       //proc_decref(p, 1);
-
+       /* This kref_put() is for the process in general (its existence in the hash
+        * table).  Note we do it after unlocking the hash table, since once it is
+        * gone, no one can get it to kref_get() it.  We also do it after unlocking,
+        * since it is possible that we are the releaser (though not when a
+        * self_ipi is pending, so only when death was remote (we're not current)).
+        *
+        * Also note that we don't give the PID back until __proc_free().  This is
+        * because not everyone is done with the process yet, although you won't
+        * find the proc in any lists, nor will it get reused anytime soon. */
+       spin_lock(&pid_hash_lock);
+       if (!hashtable_remove(pid_hash, (void*)p->pid))
+               panic("Proc not in the pid table in %s", __FUNCTION__);
+       spin_unlock(&pid_hash_lock);
        /* Unlock and possible decref and wait.  A death IPI should be on its way,
         * either from the RUNNING_S one, or from proc_take_cores with a __death.
         * in general, interrupts should be on when you call proc_destroy locally,
         * but currently aren't for all things (like traphandlers). */
        spin_unlock(&p->proc_lock);
+       kref_put(&p->kref);             /* for the hashtable ref */
        __proc_kmsg_pending(p, self_ipi_pending);
        return;
 }
@@ -769,7 +782,7 @@ void proc_yield(struct proc *SAFE p, bool being_nice)
                              __FUNCTION__);
        }
        spin_unlock(&p->proc_lock);
-       proc_decref(p, 1); // need to eat the ref passed in.
+       kref_put(&p->kref);                     /* need to eat the ref passed in */
        /* TODO: (RMS) If there was a change to the idle cores, try and give our
         * core to someone who was preempted. */
        /* Clean up the core and idle.  For mgmt cores, they will ultimately call
@@ -1076,8 +1089,7 @@ bool __proc_give_cores(struct proc *SAFE p, uint32_t *pcorelist, size_t num)
                case (PROC_RUNNING_M):
                        /* Up the refcnt, since num cores are going to start using this
                         * process and have it loaded in their 'current'. */
-                       // TODO: (REF) use proc_incref once we have atomics
-                       p->env_refcnt += num;
+                       kref_get(&p->kref, num);
                        __seq_start_write(&p->procinfo->coremap_seqctr);
                        for (int i = 0; i < num; i++) {
                                free_vcoreid = get_free_vcoreid(p, free_vcoreid);
@@ -1236,7 +1248,7 @@ bool __proc_take_allcores(struct proc *SAFE p, amr_t message,
 void __proc_kmsg_pending(struct proc *p, bool ipi_pending)
 {
        if (ipi_pending) {
-               proc_decref(p, 1);
+               kref_put(&p->kref);
                process_routine_kmsg();
                panic("stack-killing kmsg not found in %s!!!", __FUNCTION__);
        }
@@ -1260,46 +1272,6 @@ void __unmap_vcore(struct proc *p, uint32_t vcoreid)
        p->procinfo->pcoremap[p->procinfo->vcoremap[vcoreid].pcoreid].valid = FALSE;
 }
 
-/* This takes a referenced process and ups the refcnt by count.  If the refcnt
- * was already 0, then someone has a bug, so panic.  Check out the Documentation
- * for brutal details about refcnting.
- *
- * Implementation aside, the important thing is that we atomically increment
- * only if it wasn't already 0.  If it was 0, panic.
- *
- * TODO: (REF) change to use CAS / atomics. */
-void proc_incref(struct proc *p, size_t count)
-{
-       spin_lock_irqsave(&p->proc_lock);
-       if (p->env_refcnt)
-               p->env_refcnt += count;
-       else
-               panic("Tried to incref a proc with no existing references!");
-       spin_unlock_irqsave(&p->proc_lock);
-}
-
-/* When the kernel is done with a process, it decrements its reference count.
- * When the count hits 0, no one is using it and it should be freed.  "Last one
- * out" actually finalizes the death of the process.  This is tightly coupled
- * 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().  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);
-       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);
-       // if we hit 0, no one else will increment and we can check outside the lock
-       if (!refcnt)
-               __proc_free(p);
-       if (refcnt < 0)
-               panic("Too many decrefs!");
-}
-
 /* Stop running whatever context is on this core, load a known-good cr3, and
  * 'idle'.  Note this leaves no trace of what was running. This "leaves the
  * process's context. */
@@ -1347,7 +1319,7 @@ void __startcore(trapframe_t *tf, uint32_t srcid, void *a0, void *a1, void *a2)
        assert(p_to_run);
        /* the sender of the amsg increfed, thinking we weren't running current. */
        if (p_to_run == current)
-               proc_decref(p_to_run, 1);
+               kref_put(&p_to_run->kref);
        vcoreid = get_vcoreid(p_to_run, pcoreid);
        vcpd = &p_to_run->procdata->vcore_preempt_data[vcoreid];
        printd("[kernel] startcore on physical core %d for process %d's vcore %d\n",
@@ -1501,12 +1473,7 @@ void print_allpids(void)
 void print_proc_info(pid_t pid)
 {
        int j = 0;
-       /* Doing this without the incref! careful! (avoiding deadlocks) TODO (REF)*/
-       //struct proc *p = pid2proc(pid);
-       spin_lock(&pid_hash_lock);
-       struct proc *p = hashtable_search(pid_hash, (void*)pid);
-       spin_unlock(&pid_hash_lock);
-       // not concerned with a race on the state...
+       struct proc *p = pid2proc(pid);
        if (!p) {
                printk("Bad PID.\n");
                return;
@@ -1517,7 +1484,7 @@ void print_proc_info(pid_t pid)
        printk("PID: %d\n", p->pid);
        printk("PPID: %d\n", p->ppid);
        printk("State: 0x%08x\n", p->state);
-       printk("Refcnt: %d\n", p->env_refcnt);
+       printk("Refcnt: %d\n", atomic_read(&p->kref.refcount) - 1);
        printk("Flags: 0x%08x\n", p->env_flags);
        printk("CR3(phys): 0x%08x\n", p->env_cr3);
        printk("Num Vcores: %d\n", p->procinfo->num_vcores);
@@ -1545,5 +1512,5 @@ void print_proc_info(pid_t pid)
        //print_trapframe(&p->env_tf);
        /* no locking / unlocking or refcnting */
        // spin_unlock(&p->proc_lock);
-       // proc_decref(p, 1); /* decref for the pid2proc reference */
+       kref_put(&p->kref);
 }
index f8e710a..f6d42a6 100644 (file)
@@ -160,7 +160,7 @@ ssize_t core_request(struct proc *p)
                 * (just like in proc_destroy).  it also needs to decref, to consume the
                 * reference that came into this function (since we don't return).  */
                if (need_to_idle) {
-                       proc_decref(p, 1);
+                       kref_put(&p->kref);
                        abandon_core();
                }
        } else { // nothing granted, just return
index a5ce0f1..f49426e 100644 (file)
@@ -32,8 +32,8 @@ void schedule_init(void)
 
 void schedule_proc(struct proc *p)
 {
-       p->env_refcnt++; // TODO (REF) (usually is called while locked)
-       //proc_incref(p, 1); /* up the refcnt since we are storing the reference */
+       /* up the refcnt since we are storing the reference */
+       kref_get(&p->kref, 1);
        spin_lock_irqsave(&runnablelist_lock);
        printd("Scheduling PID: %d\n", p->pid);
        TAILQ_INSERT_TAIL(&proc_runnablelist, p, proc_link);
@@ -44,15 +44,15 @@ void schedule_proc(struct proc *p)
 /* TODO: race here.  it's possible that p was already removed from the
  * list (by schedule()), while proc_destroy is trying to remove it from the
  * list.  schedule()'s proc_run() won't actually run it (since it's DYING), but
- * this code will probably fuck up. */
+ * this code will probably fuck up.  Having TAILQ_REMOVE not hurt will help. */
 void deschedule_proc(struct proc *p)
 {
        spin_lock_irqsave(&runnablelist_lock);
        printd("Descheduling PID: %d\n", p->pid);
        TAILQ_REMOVE(&proc_runnablelist, p, proc_link);
        spin_unlock_irqsave(&runnablelist_lock);
-       p->env_refcnt--; // TODO (REF) (usually is called while locked)
-       //proc_decref(p, 1); /* down the refcnt, since its no longer stored */
+       /* down the refcnt, since its no longer stored */
+       kref_put(&p->kref);
        return;
 }
 
@@ -73,7 +73,7 @@ void schedule(void)
                printd("PID of proc i'm running: %d\n", p->pid);
                /* proc_run will either eat the ref, or we'll decref manually. */
                proc_run(p);
-               proc_decref(p, 1);
+               kref_put(&p->kref);
        } else {
                spin_unlock_irqsave(&runnablelist_lock);
                /* while this is problematic, we really don't have anything to
index 7b3819d..a43baae 100644 (file)
@@ -236,7 +236,7 @@ static int sys_proc_create(struct proc *p, char *path, size_t path_l,
                return -1;
        }
        pid = new_p->pid;
-       proc_decref(new_p, 1);  /* give up the reference created in proc_create() */
+       kref_put(&new_p->kref); /* give up the reference created in proc_create() */
        atomic_dec(&program->f_refcnt);         /* TODO: REF / KREF */
        return pid;
 }
@@ -253,17 +253,17 @@ static error_t sys_proc_run(struct proc *p, unsigned pid)
        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);
+               kref_put(&target->kref);
                retval = -EPERM;
        } else if (target->state != PROC_CREATED) {
-               proc_decref(target, 1);
+               kref_put(&target->kref);
                retval = -EINVAL;
        } else {
                __proc_set_state(target, PROC_RUNNABLE_S);
                schedule_proc(target);
        }
        spin_unlock(&p->proc_lock);
-       proc_decref(target, 1);
+       kref_put(&target->kref);
        return retval;
 }
 
@@ -281,20 +281,20 @@ static error_t sys_proc_destroy(struct proc *p, pid_t pid, int exitcode)
                return -1;
        }
        if (!proc_controls(p, p_to_die)) {
-               proc_decref(p_to_die, 1);
+               kref_put(&p_to_die->kref);
                set_errno(current_tf, EPERM);
                return -1;
        }
        if (p_to_die == p) {
                // syscall code and pid2proc both have edible references, only need 1.
                p->exitcode = exitcode;
-               proc_decref(p, 1);
+               kref_put(&p_to_die->kref);
                printd("[PID %d] proc exiting gracefully (code %d)\n", p->pid,exitcode);
        } else {
                printd("[%d] destroying proc %d\n", p->pid, p_to_die->pid);
        }
        proc_destroy(p_to_die);
-       proc_decref(p_to_die, 1);
+       kref_put(&p_to_die->kref);
        return ESUCCESS;
 }
 
@@ -368,7 +368,7 @@ static ssize_t sys_fork(env_t* e)
        /* for now, just copy the contents of every present page in the entire
         * address space. */
        if (env_user_mem_walk(e, 0, UMAPTOP, &copy_page, env)) {
-               proc_decref(env,2);
+               proc_destroy(env);      /* this is prob what you want, not decref by 2 */
                set_errno(current_tf,ENOMEM);
                return -1;
        }
@@ -473,7 +473,9 @@ static ssize_t sys_trywait(env_t* e, pid_t pid, int* status)
                }
 
                // if the wait succeeded, decref twice
-               proc_decref(p,1 + (ret == 0));
+               if (ret == 0)
+                       kref_put(&p->kref);
+               kref_put(&p->kref);
                return ret;
        }
 
@@ -552,7 +554,7 @@ static ssize_t sys_shared_page_alloc(env_t* p1,
        page_t* page;
        error_t e = upage_alloc(p1, &page,1);
        if (e < 0) {
-               proc_decref(p2, 1);
+               kref_put(&p2->kref);
                return e;
        }
 
@@ -560,7 +562,7 @@ static ssize_t sys_shared_page_alloc(env_t* p1,
                        (void*SNT)UTEXT, (void*SNT)UTOP, p2_flags);
        if (p2_addr == NULL) {
                page_free(page);
-               proc_decref(p2, 1);
+               kref_put(&p2->kref);
                return -EFAIL;
        }
 
@@ -569,11 +571,11 @@ static ssize_t sys_shared_page_alloc(env_t* p1,
        if(p1_addr == NULL) {
                page_remove(p2->env_pgdir, p2_addr);
                page_free(page);
-               proc_decref(p2, 1);
+               kref_put(&p2->kref);
                return -EFAIL;
        }
        *addr = p1_addr;
-       proc_decref(p2, 1);
+       kref_put(&p2->kref);
        return ESUCCESS;
 }
 
@@ -598,14 +600,14 @@ static int sys_notify(struct proc *p, int target_pid, unsigned int notif,
                return -1;
        }
        if (!proc_controls(p, target)) {
-               proc_decref(target, 1);
+               kref_put(&target->kref);
                set_errno(current_tf, EPERM);
                return -1;
        }
        /* if the user provided a notif_event, copy it in and use that */
        if (u_ne) {
                if (memcpy_from_user(p, &local_ne, u_ne, sizeof(struct notif_event))) {
-                       proc_decref(target, 1);
+                       kref_put(&target->kref);
                        set_errno(current_tf, EINVAL);
                        return -1;
                }
@@ -613,7 +615,7 @@ static int sys_notify(struct proc *p, int target_pid, unsigned int notif,
        } else {
                proc_notify(target, notif, 0);
        }
-       proc_decref(target, 1);
+       kref_put(&target->kref);
        return 0;
 }
 
@@ -1198,7 +1200,7 @@ intreg_t process_generic_syscalls(struct proc *p, size_t max)
         * the *p). */
        // TODO: ought to be unnecessary, if you called this right, kept here for
        // now in case anyone actually uses the ARSCs.
-       proc_incref(p, 1);
+       kref_get(&p->kref, 1);
 
        // max is the most we'll process.  max = 0 means do as many as possible
        while (RING_HAS_UNCONSUMED_REQUESTS(sysbr) && ((!max)||(count < max)) ) {
@@ -1228,7 +1230,7 @@ intreg_t process_generic_syscalls(struct proc *p, size_t max)
        }
        // load sane page tables (and don't rely on decref to do it for you).
        lcr3(boot_cr3);
-       proc_decref(p, 1);
+       kref_put(&p->kref);
        return (intreg_t)count;
 }