Removes __proc_kmsg_pending()
authorBarret Rhoden <brho@cs.berkeley.edu>
Thu, 29 Sep 2011 20:12:01 +0000 (13:12 -0700)
committerKevin Klues <klueska@cs.berkeley.edu>
Thu, 3 Nov 2011 00:36:08 +0000 (17:36 -0700)
Since proc management kmsgs return and are IMMEDIATE, we no longer need
the painful logic of determining that the kernel needs to clean up in
anticipating of a 'stack killing' kmsg.

This means schedule() can run on a core, preempt the _M's vcore running
there, and give out both that vcore and the pcore without worrying about
its own code getting interrupted.

Documentation/process-internals.txt
kern/include/process.h
kern/src/monitor.c
kern/src/process.c
kern/src/resource.c

index 52a29b5..981f4c5 100644 (file)
@@ -183,15 +183,6 @@ current and didn't.
 proc_yield(): this never returns, so it eats your reference.  It will also
 decref when it abandon_core()s.
 
-__proc_give_cores() and friends: you call this while holding the lock, but it is
-possible that your core is in the corelist you gave it.  In this case, it will
-detect it, and return a bool signalling if an IPI is pending.  It will not
-consume your reference.  The reasoning behind this is that it is an internal
-function, and you may want to do other things before decreffing.  There is also
-a helper function that will unlock and possibly decref/wait for the IPI, called
-__proc_unlock_ipi_pending().  Use this when it is time to unlock.  It's just a
-helper, which may go away.
-
 abandon_core(): it was not given a reference, so it doesn't eat one.  It will
 decref when it unloads the cr3.  Note that this is a potential performance
 issue.  When preempting or killing, there are n cores that are fighting for the
@@ -203,15 +194,11 @@ amount of memory for better scalability.
 1.8 Core Request:
 ---------------------------
 core_request() is run outside of the process code (for now), though it is fairly
-intricate.  It's another function that might not return, but the reasons for
-this vary:
+intricate.  It's another function that might not return.  Historically, there
+were a few reasons for it, but now only one is true (and may not be for long):
        1: The process is moving from _S to _M so the return path to userspace won't
        happen (and sort of will on the new core / the other side), but that will
        happen when popping into userspace.
-       2: The scheduler is giving the current core to the process, which can kick
-       in via either proc_run() or __proc_give_cores().
-       3: It was a request to give up all cores, which means the current core will
-       receive an IPI (if it wasn't an async call, which isn't handled yet).
 
 For these reasons, core_request() needs to have an edible reference.
 
@@ -681,15 +668,32 @@ this.  It's possible, but unnecessary.
 
 5. current_tf
 ===========================
-current_tf is a per-core macro that returns a struct trapframe * that points
-back on the kernel stack to the user context that was running on the given core
-when an interrupt or trap happened.  Saving the reference to the TF helps
-simplify code that needs to do something with the TF (like save it and pop
-another TF).  This way, we don't need to pass the context all over the place,
-especially through code that might not care.
+Originally, current_tf is a per-core macro that returns a struct trapframe *
+that points back on the kernel stack to the user context that was running on
+the given core when an interrupt or trap happened.  Saving the reference to
+the TF helps simplify code that needs to do something with the TF (like save
+it and pop another TF).  This way, we don't need to pass the context all over
+the place, especially through code that might not care.
+
+Now, current_tf is more broadly defined as the user context that should be run
+when the kernel is ready to run a process.  In the older case, it was when the
+kernel tries to return to userspace from a trap/interrupt.  Now, current_tf
+can be set by an IPI/KMSG (like '__startcore') so that when the kernel wants
+to idle, it will find a cur_tf that it needs to run, even though we never
+trapped in on that context in the first place.
+
+Process management KMSGs now simply modify cur_tf, as if we had interrupted a
+process.  Instead of '__startcore' forcing the kernel to actually run the
+context, it will just mean we will eventually run it.  In the meantime a
+'__notify' or a '__preempt' can come in, and they will apply to the cur_tf.
+This greatly simplifies process code and code calling process code (like the
+scheduler), since we no longer need to worry about whether or not we are
+getting a "stack killing" kernel message.  Before this, code needed to care
+where it was running when managing _Ms.
 
 current_tf should go along with current.  It's the current_tf of the current
-process.  Withouth 'current', it has no meaning.
+process.  Withouth 'current', it has no meaning.  This will probably change
+slightly, since kthreads care about 'current' but not about current_tf.
 
 It does not point to kernel trapframes, which is important when we receive an
 interrupt in the kernel.  At one point, we were (hypothetically) clobbering the
index 68f91b9..1f66ea4 100644 (file)
@@ -109,17 +109,16 @@ struct vcore *vcoreid2vcore(struct proc *p, uint32_t vcoreid);
  *
  * WARNING: YOU MUST HOLD THE PROC_LOCK BEFORE CALLING THESE! */
 /* Gives process p the additional num cores listed in corelist */
-bool __proc_give_cores(struct proc *SAFE p, uint32_t *pcorelist, size_t num);
+void __proc_give_cores(struct proc *SAFE p, uint32_t *pcorelist, size_t num);
 /* Makes process p's coremap look like corelist (add, remove, etc). Not used */
-bool __proc_set_allcores(struct proc *SAFE p, uint32_t *pcorelist,
+void __proc_set_allcores(struct proc *SAFE p, uint32_t *pcorelist,
                          size_t *num, amr_t message, TV(a0t) arg0,
                          TV(a1t) arg1, TV(a2t) arg2);
 /* Takes from process p the num cores listed in corelist */
-bool __proc_take_cores(struct proc *p, uint32_t *pcorelist, size_t num,
+void __proc_take_cores(struct proc *p, uint32_t *pcorelist, size_t num,
                        amr_t message, long arg0, long arg1, long arg2);
-bool __proc_take_allcores(struct proc *p, amr_t message, long arg0, long arg1,
+void __proc_take_allcores(struct proc *p, amr_t message, long arg0, long arg1,
                           long arg2);
-void __proc_kmsg_pending(struct proc *p, bool ipi_pending);
 /* Exposed for kern/src/resource.c for now */
 void __map_vcore(struct proc *p, uint32_t vcoreid, uint32_t pcoreid);
 void __unmap_vcore(struct proc *p, uint32_t vcoreid);
@@ -127,8 +126,8 @@ void __unmap_vcore(struct proc *p, uint32_t vcoreid);
 /* Preemption management.  Some of these will change */
 void __proc_preempt_warn(struct proc *p, uint32_t vcoreid, uint64_t when);
 void __proc_preempt_warnall(struct proc *p, uint64_t when);
-bool __proc_preempt_core(struct proc *p, uint32_t pcoreid);
-bool __proc_preempt_all(struct proc *p);
+void __proc_preempt_core(struct proc *p, uint32_t pcoreid);
+void __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);
 
index 496423b..ea6fd8b 100644 (file)
@@ -473,7 +473,6 @@ int mon_notify(int argc, char *NTS *NT COUNT(argc) argv, trapframe_t *tf)
 int mon_measure(int argc, char *NTS *NT COUNT(argc) argv, trapframe_t *tf)
 {
        uint64_t begin = 0, diff = 0;
-       bool self_ipi_pending = FALSE;
        uint32_t end_refcnt = 0;
 
        if (argc < 2) {
@@ -599,9 +598,8 @@ int mon_measure(int argc, char *NTS *NT COUNT(argc) argv, trapframe_t *tf)
                                return 1;
                        }
                        begin = start_timing();
-                       self_ipi_pending = __proc_preempt_core(p, pcoreid);
+                       __proc_preempt_core(p, pcoreid);
                        spin_unlock(&p->proc_lock);
-                       __proc_kmsg_pending(p, self_ipi_pending);
                        /* done when unmapped (right before abandoning) */
                        spin_on(p->procinfo->pcoremap[pcoreid].valid);
                        diff = stop_timing(begin);
@@ -610,10 +608,9 @@ int mon_measure(int argc, char *NTS *NT COUNT(argc) argv, trapframe_t *tf)
                        spin_lock(&p->proc_lock);
                        end_refcnt = kref_refcnt(&p->p_kref) - p->procinfo->num_vcores;
                        begin = start_timing();
-                       self_ipi_pending = __proc_preempt_all(p);
+                       __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(kref_refcnt(&p->p_kref) != end_refcnt);
                        diff = stop_timing(begin);
index 55408f2..bda4485 100644 (file)
@@ -487,12 +487,11 @@ static void __set_proc_current(struct proc *p)
  * it's old core0 context, and the other cores will come in at the entry point.
  * Including in the case of preemption.
  *
- * This won't return if the current core is going to be one of the processes
- * cores (either for _S mode or for _M if it's in the vcoremap).  proc_run will
- * eat your reference if it does not return. */
+ * This won't return if the current core is going to be running the process as a
+ * _S.  It will return if the process is an _M.  Regardless, proc_run will eat
+ * your reference if it does not return. */
 void proc_run(struct proc *p)
 {
-       bool self_ipi_pending = FALSE;
        struct vcore *vc_i;
        spin_lock(&p->proc_lock);
 
@@ -541,10 +540,6 @@ void proc_run(struct proc *p)
                                /* Up the refcnt, since num_vcores are going to start using this
                                 * process and have it loaded in their 'current'. */
                                proc_incref(p, 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()))
-                                       self_ipi_pending = TRUE;
                                /* Send kernel messages to all online vcores (which were added
                                 * to the list and mapped in __proc_give_cores()), making them
                                 * turn online */
@@ -566,7 +561,6 @@ void proc_run(struct proc *p)
                         * - Note there is no guarantee this core's interrupts were on, so
                         *   it may not get the message for a while... */
                        spin_unlock(&p->proc_lock);
-                       __proc_kmsg_pending(p, self_ipi_pending);
                        break;
                default:
                        spin_unlock(&p->proc_lock);
@@ -661,8 +655,6 @@ void proc_restartcore(void)
  * get __proc_free()d. */
 void proc_destroy(struct proc *p)
 {
-       bool self_ipi_pending = FALSE;
-       
        spin_lock(&p->proc_lock);
        switch (p->state) {
                case PROC_DYING: // someone else killed this already.
@@ -983,20 +975,20 @@ void __proc_preempt_warnall(struct proc *p, uint64_t when)
 
 // TODO: function to set an alarm, if none is outstanding
 
-/* Raw function to preempt a single core.  Returns TRUE if the calling core will
- * get a kmsg.  If you care about locking, do it before calling. */
-bool __proc_preempt_core(struct proc *p, uint32_t pcoreid)
+/* Raw function to preempt a single core.  If you care about locking, do it
+ * before calling. */
+void __proc_preempt_core(struct proc *p, uint32_t pcoreid)
 {
        uint32_t vcoreid = get_vcoreid(p, pcoreid);
 
        p->procinfo->vcoremap[vcoreid].preempt_served = TRUE;
        // expects a pcorelist.  assumes pcore is mapped and running_m
-       return __proc_take_cores(p, &pcoreid, 1, __preempt, (long)p, 0, 0);
+       __proc_take_cores(p, &pcoreid, 1, __preempt, (long)p, 0, 0);
 }
 
-/* Raw function to preempt every vcore.  Returns TRUE if the calling core will
- * get a kmsg.  If you care about locking, do it before calling. */
-bool __proc_preempt_all(struct proc *p)
+/* Raw function to preempt every vcore.  If you care about locking, do it before
+ * calling. */
+void __proc_preempt_all(struct proc *p)
 {
        /* instead of doing this, we could just preempt_served all possible vcores,
         * and not just the active ones.  We would need to sort out a way to deal
@@ -1004,14 +996,13 @@ bool __proc_preempt_all(struct proc *p)
        struct vcore *vc_i;
        TAILQ_FOREACH(vc_i, &p->online_vcs, list)
                vc_i->preempt_served = TRUE;
-       return __proc_take_allcores(p, __preempt, (long)p, 0, 0);
+       __proc_take_allcores(p, __preempt, (long)p, 0, 0);
 }
 
 /* Warns and preempts a vcore from p.  No delaying / alarming, or anything.  The
  * warning will be for u usec from now. */
 void proc_preempt_core(struct proc *p, uint32_t pcoreid, uint64_t usec)
 {
-       bool self_ipi_pending = FALSE;
        uint64_t warn_time = read_tsc() + usec2tsc(usec);
 
        /* DYING could be okay */
@@ -1022,7 +1013,7 @@ void proc_preempt_core(struct proc *p, uint32_t pcoreid, uint64_t usec)
        spin_lock(&p->proc_lock);
        if (is_mapped_vcore(p, pcoreid)) {
                __proc_preempt_warn(p, get_vcoreid(p, pcoreid), warn_time);
-               self_ipi_pending = __proc_preempt_core(p, pcoreid);
+               __proc_preempt_core(p, pcoreid);
        } else {
                warn("Pcore doesn't belong to the process!!");
        }
@@ -1041,7 +1032,6 @@ void proc_preempt_core(struct proc *p, uint32_t pcoreid, uint64_t usec)
  * warning will be for u usec from now. */
 void proc_preempt_all(struct proc *p, uint64_t usec)
 {
-       bool self_ipi_pending = FALSE;
        uint64_t warn_time = read_tsc() + usec2tsc(usec);
 
        spin_lock(&p->proc_lock);
@@ -1052,7 +1042,7 @@ void proc_preempt_all(struct proc *p, uint64_t usec)
                return;
        }
        __proc_preempt_warnall(p, warn_time);
-       self_ipi_pending = __proc_preempt_all(p);
+       __proc_preempt_all(p);
        assert(!p->procinfo->num_vcores);
        /* TODO: (RMS) do this once a scheduler can handle RUNNABLE_M, and make sure
         * to schedule it */
@@ -1068,13 +1058,10 @@ void proc_preempt_all(struct proc *p, uint64_t usec)
  * free, etc. */
 void proc_give(struct proc *p, uint32_t pcoreid)
 {
-       bool self_ipi_pending = FALSE;
-
        spin_lock(&p->proc_lock);
        // expects a pcorelist, we give it a list of one
-       self_ipi_pending = __proc_give_cores(p, &pcoreid, 1);
+       __proc_give_cores(p, &pcoreid, 1);
        spin_unlock(&p->proc_lock);
-       __proc_kmsg_pending(p, self_ipi_pending);
 }
 
 /* Global version of the helper, for sys_get_vcoreid (might phase that syscall
@@ -1149,13 +1136,9 @@ static void __proc_give_a_pcore(struct proc *p, uint32_t pcore)
  * The other way would be to have this function have the side effect of changing
  * state, and finding another way to do the need_to_idle.
  *
- * The returned bool signals whether or not a stack-crushing IPI will come in
- * once you unlock after this function.
- *
  * WARNING: You must hold the proc_lock before calling this! */
-bool __proc_give_cores(struct proc *SAFE p, uint32_t *pcorelist, size_t num)
+void __proc_give_cores(struct proc *SAFE p, uint32_t *pcorelist, size_t num)
 {
-       bool self_ipi_pending = FALSE;
        switch (p->state) {
                case (PROC_RUNNABLE_S):
                case (PROC_RUNNING_S):
@@ -1193,8 +1176,6 @@ bool __proc_give_cores(struct proc *SAFE p, uint32_t *pcorelist, size_t num)
                                __proc_give_a_pcore(p, pcorelist[i]);
                                send_kernel_message(pcorelist[i], __startcore, (long)p, 0, 0,
                                                    KMSG_IMMEDIATE);
-                               if (pcorelist[i] == core_id())
-                                       self_ipi_pending = TRUE;
                        }
                        __seq_end_write(&p->procinfo->coremap_seqctr);
                        break;
@@ -1203,7 +1184,6 @@ bool __proc_give_cores(struct proc *SAFE p, uint32_t *pcorelist, size_t num)
                              __FUNCTION__);
        }
        p->resources[RES_CORES].amt_granted += num;
-       return self_ipi_pending;
 }
 
 /* Makes process p's coremap look like pcorelist (add, remove, etc).  Caller
@@ -1217,7 +1197,7 @@ bool __proc_give_cores(struct proc *SAFE p, uint32_t *pcorelist, size_t num)
  * implementing it.
  *
  * WARNING: You must hold the proc_lock before calling this! */
-bool __proc_set_allcores(struct proc *SAFE p, uint32_t *pcorelist,
+void __proc_set_allcores(struct proc *SAFE p, uint32_t *pcorelist,
                          size_t *num, amr_t message,TV(a0t) arg0,
                          TV(a1t) arg1, TV(a2t) arg2)
 {
@@ -1225,20 +1205,17 @@ bool __proc_set_allcores(struct proc *SAFE p, uint32_t *pcorelist,
 }
 
 /* Helper for the take_cores calls: takes a specific vcore from p, optionally
- * sending the message (or just unmapping), gives the pcore to the idlecoremap,
- * and returns TRUE if a self_ipi is pending. */
-static bool __proc_take_a_core(struct proc *p, struct vcore *vc, amr_t message,
+ * sending the message (or just unmapping), gives the pcore to the idlecoremap.
+ */
+static void __proc_take_a_core(struct proc *p, struct vcore *vc, amr_t message,
                                long arg0, long arg1, long arg2)
 {
-       bool self_ipi_pending = FALSE;
        /* Change lists for the vcore.  We do this before either unmapping or
         * sending the message, so the lists represent what will be very soon
         * (before we unlock, the messages are in flight). */
        TAILQ_REMOVE(&p->online_vcs, vc, list);
        TAILQ_INSERT_HEAD(&p->inactive_vcs, vc, list);
        if (message) {
-               if (vc->pcoreid == core_id())
-                       self_ipi_pending = TRUE;
                send_kernel_message(vc->pcoreid, message, arg0, arg1, arg2,
                                    KMSG_IMMEDIATE);
        } else {
@@ -1248,19 +1225,16 @@ static bool __proc_take_a_core(struct proc *p, struct vcore *vc, amr_t message,
        }
        /* give the pcore back to the idlecoremap */
        put_idle_core(vc->pcoreid);
-       return self_ipi_pending;
 }
 
 /* Takes from process p the num cores listed in pcorelist, using the given
- * message for the kernel message (__death, __preempt, etc).  Like the others
- * in this function group, bool signals whether or not an IPI is pending.
+ * message for the kernel message (__death, __preempt, etc).
  *
  * WARNING: You must hold the proc_lock before calling this! */
-bool __proc_take_cores(struct proc *p, uint32_t *pcorelist, size_t num,
+void __proc_take_cores(struct proc *p, uint32_t *pcorelist, size_t num,
                        amr_t message, long arg0, long arg1, long arg2)
 {
        uint32_t vcoreid;
-       bool self_ipi_pending = FALSE;
        switch (p->state) {
                case (PROC_RUNNABLE_M):
                        assert(!message);
@@ -1281,26 +1255,23 @@ bool __proc_take_cores(struct proc *p, uint32_t *pcorelist, size_t num,
                vcoreid = get_vcoreid(p, pcorelist[i]);
                /* Sanity check */
                assert(pcorelist[i] == get_pcoreid(p, vcoreid));
-               self_ipi_pending = __proc_take_a_core(p, vcoreid2vcore(p, vcoreid),
-                                                     message, arg0, arg1, arg2);
+               __proc_take_a_core(p, vcoreid2vcore(p, vcoreid), message, arg0, arg1,
+                                  arg2);
        }
        p->procinfo->num_vcores -= num;
        __seq_end_write(&p->procinfo->coremap_seqctr);
        p->resources[RES_CORES].amt_granted -= num;
-       return self_ipi_pending;
 }
 
 /* Takes all cores from a process, which must be in an _M state.  Cores are
  * placed back in the idlecoremap.  If there's a message, such as __death or
- * __preempt, it will be sent to the cores.  The bool signals whether or not an
- * IPI is coming in once you unlock.
+ * __preempt, it will be sent to the cores.
  *
  * WARNING: You must hold the proc_lock before calling this! */
-bool __proc_take_allcores(struct proc *p, amr_t message, long arg0, long arg1,
+void __proc_take_allcores(struct proc *p, amr_t message, long arg0, long arg1,
                           long arg2)
 {
        struct vcore *vc_i, *vc_temp;
-       bool self_ipi_pending = FALSE;
        switch (p->state) {
                case (PROC_RUNNABLE_M):
                        assert(!message);
@@ -1317,37 +1288,12 @@ bool __proc_take_allcores(struct proc *p, amr_t message, long arg0, long arg1,
        spin_unlock(&idle_lock);
        __seq_start_write(&p->procinfo->coremap_seqctr);
        TAILQ_FOREACH_SAFE(vc_i, &p->online_vcs, list, vc_temp) {
-               self_ipi_pending = __proc_take_a_core(p, vc_i,
-                                                     message, arg0, arg1, arg2);
+               __proc_take_a_core(p, vc_i, message, arg0, arg1, arg2);
        }
        p->procinfo->num_vcores = 0;
        assert(TAILQ_EMPTY(&p->online_vcs));
        __seq_end_write(&p->procinfo->coremap_seqctr);
        p->resources[RES_CORES].amt_granted = 0;
-       return self_ipi_pending;
-}
-
-/* Helper, to be used when a proc management kmsg should be on its way.  This
- * used to also unlock and then handle the message, back when the proc_lock was
- * an irqsave, and we had an IPI pending.  Now we use routine kmsgs.  If a msg
- * is pending, this needs to decref (to eat the reference of the caller) and
- * then process the message.  Unlock before calling this, since you might not
- * return.
- *
- * 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), though in most cases they will be.
- *
- * TODO: consider inlining this, so __FUNCTION__ works (will require effort in
- * core_request(). */
-void __proc_kmsg_pending(struct proc *p, bool ipi_pending)
-{
-       if (ipi_pending) {
-               proc_decref(p);
-               process_routine_kmsg(0);
-               panic("stack-killing kmsg not found in %s!!!", __FUNCTION__);
-       }
 }
 
 /* Helper to do the vcore->pcore and inverse mapping.  Hold the lock when
index ffa9cd9..e27196c 100644 (file)
@@ -41,7 +41,6 @@ ssize_t core_request(struct proc *p)
        ssize_t amt_new;
        uint32_t corelist[MAX_NUM_CPUS];
        bool need_to_idle = FALSE;
-       bool self_ipi_pending = FALSE;
        int8_t state = 0;
 
        spin_lock(&p->proc_lock);
@@ -64,7 +63,7 @@ ssize_t core_request(struct proc *p)
                /* sending death, since it's not our job to save contexts or anything in
                 * this case.  also, if this returns true, we will not return down
                 * below, and need to eat the reference to p */
-               self_ipi_pending = __proc_take_allcores(p, __death, 0, 0, 0);
+               __proc_take_allcores(p, __death, 0, 0, 0);
                __proc_set_state(p, PROC_RUNNABLE_S);
                schedule_proc(p);
                spin_unlock(&p->proc_lock);
@@ -162,10 +161,8 @@ ssize_t core_request(struct proc *p)
                                break;
                }
                /* give them the cores.  this will start up the extras if RUNNING_M. */
-               self_ipi_pending = __proc_give_cores(p, corelist, num_granted);
+               __proc_give_cores(p, corelist, num_granted);
                spin_unlock(&p->proc_lock);
-               // TODO: (RMS) think about this, esp when its called from a scheduler
-               __proc_kmsg_pending(p, self_ipi_pending);
                /* if there's a race on state (like DEATH), it'll get handled by
                 * proc_run or proc_destroy */
                if (p->state == PROC_RUNNABLE_M)