Remove "early routine kmsg" context
authorBarret Rhoden <brho@cs.berkeley.edu>
Tue, 24 Jul 2018 17:47:04 +0000 (13:47 -0400)
committerBarret Rhoden <brho@cs.berkeley.edu>
Wed, 25 Jul 2018 15:25:46 +0000 (11:25 -0400)
The purpose of was to detect if an RKM/kthread had blocked since we
launched the message.  If it did, then we could be on another core, and we
didn't want to continue checking routine messages from the *old* core.

This was a little hokey, and the intent was to keep us in the PRKM while()
loop for those messages that didn't block.  However, if we just call
smp_idle() after each KMSG, it is essentially the same code.  Both cases
would do the same thing: clear the kthread's flags, report a quiescent
state, disable IRQs, reread core_id/pcpui, then check for another message.

Just calling smp_idle() adds a function call or two to the loop, but it
removes the early RKM checks and clarifies the code.  I imagine the
performance is negligble either way.

The clarity is worth a lot more.  There was at least one bug with the old
code: when we set the pcpui->cur_kthread's flags, pcpui was stale.  That
means we could have clobbered another core's kthread's flags.  At worst,
that could turn off IS_KTASK and turn on SAVE_ADDR_SPACE.  I'm not sure how
that would break anything specifically.

We had a couple cases of RKMs getting ignored (sitting in the core's queue,
but the core had halted).  I don't see exactly how problems in this area
could have caused that, so we'll see.  So far so good.

Signed-off-by: Barret Rhoden <brho@cs.berkeley.edu>
Documentation/kernel_messages.txt
kern/include/trap.h
kern/src/kthread.c
kern/src/smp.c
kern/src/trap.c

index 21e6c9a..b2991de 100644 (file)
@@ -78,12 +78,6 @@ probably receive a timestamp field, among other things.  Routine messages also
 will replace the old workqueue, which hasn't really been used in 40 months or
 so.
 
-Blocking:
---------------------------------
-If a routine kernel message blocks (or has a chance to block), it must
-smp_idle() at the end.  If it were to return to PRKM(), it could be on a new
-core, due to kthread migration.  We might have a way to enforce this later.
-
 To Return or Not:
 --------------------------------
 Routine k_msgs do not have to return.  Immediate messages must.  The distinction
index 5f389e6..0d2b680 100644 (file)
@@ -179,8 +179,7 @@ void __kmsg_trampoline(uint32_t srcid, long a0, long a1, long a2);
 #define __CTX_IRQ_D_MASK                       ((1 << 8) - 1)
 #define __CTX_KTRAP_D_MASK                     ((1 << 8) - 1)
 #define __CTX_NESTED_CTX_MASK          ((1 << 16) - 1)
-#define __CTX_EARLY_RKM                                (1 << (__CTX_FLAG_SHIFT + 0))
-#define __CTX_RCU_CB                           (1 << (__CTX_FLAG_SHIFT + 1))
+#define __CTX_RCU_CB                           (1 << (__CTX_FLAG_SHIFT + 0))
 
 /* Basic functions to get or change depths */
 
@@ -202,12 +201,6 @@ void __kmsg_trampoline(uint32_t srcid, long a0, long a1, long a2);
 #define dec_ktrap_depth(pcpui)                                                 \
        ((pcpui)->__ctx_depth -= 1 << __CTX_KTRAP_D_SHIFT)
 
-#define set_rkmsg(pcpui)                                                       \
-       ((pcpui)->__ctx_depth |= __CTX_EARLY_RKM)
-
-#define clear_rkmsg(pcpui)                                                     \
-       ((pcpui)->__ctx_depth &= ~__CTX_EARLY_RKM)
-
 #define set_rcu_cb(pcpui)                                                      \
        ((pcpui)->__ctx_depth |= __CTX_RCU_CB)
 
@@ -221,9 +214,6 @@ void __kmsg_trampoline(uint32_t srcid, long a0, long a1, long a2);
 #define in_irq_ctx(pcpui)                                                      \
        (irq_depth(pcpui))
 
-#define in_early_rkmsg_ctx(pcpui)                                              \
-       ((pcpui)->__ctx_depth & __CTX_EARLY_RKM)
-
 #define in_rcu_cb_ctx(pcpui)                                                   \
        ((pcpui)->__ctx_depth & __CTX_RCU_CB)
 
index 1457a3b..c9d1b77 100644 (file)
@@ -183,8 +183,6 @@ static void __launch_kthread(uint32_t srcid, long a0, long a1, long a2)
        struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
        struct proc *cur_proc = pcpui->cur_proc;
 
-       /* Make sure we are a routine kmsg */
-       assert(in_early_rkmsg_ctx(pcpui));
        if (pcpui->owning_proc && pcpui->owning_proc != kthread->proc) {
                /* Some process should be running here that is not the same as the
                 * kthread.  This means the _M is getting interrupted or otherwise
@@ -198,9 +196,7 @@ static void __launch_kthread(uint32_t srcid, long a0, long a1, long a2)
        }
        /* o/w, just run the kthread.  any trapframes that are supposed to run or
         * were interrupted will run whenever the kthread smp_idles() or otherwise
-        * finishes.  We also need to clear the RKMSG context since we will not
-        * return from restart_kth. */
-       clear_rkmsg(pcpui);
+        * finishes. */
        restart_kthread(kthread);
        assert(0);
 }
index 55beb2f..a070a0c 100644 (file)
@@ -63,16 +63,12 @@ static void __attribute__((noreturn)) __smp_idle(void *arg)
 {
        struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
 
-       disable_irq();  /* might not be needed - need to look at KMSGs closely */
-       clear_rkmsg(pcpui);
        pcpui->cur_kthread->flags = KTH_DEFAULT_FLAGS;
-       enable_irq();   /* one-shot change to get any IRQs before we halt later */
-       disable_irq();
        while (1) {
                /* This might wake a kthread (the gp ktask), so be sure to run PRKM
-                * after reporting the quiescent state.  Note that after each RKM
-                * finishes, we'll also rerun rcu_report_qs(). */
+                * after reporting the quiescent state. */
                rcu_report_qs();
+               /* If this runs an RKM, we'll call smp_idle from the top. */
                process_routine_kmsg();
                try_run_proc();
                cpu_bored();            /* call out to the ksched */
@@ -89,6 +85,7 @@ static void __attribute__((noreturn)) __smp_idle(void *arg)
 
 void smp_idle(void)
 {
+       disable_irq();
        __reset_stack_pointer(0, get_stack_top(), __smp_idle);
 }
 
index 2673c9f..ea474b3 100644 (file)
@@ -203,62 +203,36 @@ static kernel_message_t *get_next_rkmsg(struct per_cpu_info *pcpui)
        return kmsg;
 }
 
-/* Runs routine kernel messages.  This might not return.  In the past, this
- * would also run immediate messages, but this is unnecessary.  Immediates will
- * run whenever we reenable IRQs.  We could have some sort of ordering or
- * guarantees between KMSG classes, but that's not particularly useful at this
- * point.
+/* Runs a routine kernel message.  If we execute a message, this does not
+ * return, but instead will call smp_idle().
  *
- * Note this runs from normal context, with interruptes disabled.  However, a
- * particular RKM could enable interrupts - for instance __launch_kthread() will
- * restore an old kthread that may have had IRQs on. */
+ * Note that routine messages do not have to return, but almost all of them do.
+ * If you're thinking of changing this, take a look at __launch_kthread. */
 void process_routine_kmsg(void)
 {
        uint32_t pcoreid = core_id();
        struct per_cpu_info *pcpui = &per_cpu_info[pcoreid];
        struct kernel_message msg_cp, *kmsg;
 
-       /* Important that callers have IRQs disabled.  When sending cross-core RKMs,
-        * the IPI is used to keep the core from going to sleep - even though RKMs
-        * aren't handled in the kmsg handler.  Check smp_idle() for more info. */
+       /* Important that callers have IRQs disabled when checking for RKMs.  When
+        * sending cross-core RKMs, the IPI is used to keep the core from going to
+        * sleep - even though RKMs aren't handled in the kmsg handler. */
        assert(!irq_is_enabled());
-       while ((kmsg = get_next_rkmsg(pcpui))) {
-               /* Copy in, and then free, in case we don't return */
-               msg_cp = *kmsg;
-               kmem_cache_free(kernel_msg_cache, (void*)kmsg);
-               assert(msg_cp.dstid == pcoreid);        /* caught a brutal bug with this */
-               set_rkmsg(pcpui);                                       /* we're now in early RKM ctx */
-               /* The kmsg could block.  If it does, we want the kthread code to know
-                * it's not running on behalf of a process, and we're actually spawning
-                * a kernel task.  While we do have a syscall that does work in an RKM
-                * (change_to), it's not really the rest of the syscall context. */
-               pcpui->cur_kthread->flags = KTH_KTASK_FLAGS;
-               pcpui_trace_kmsg(pcpui, (uintptr_t)msg_cp.pc);
-               msg_cp.pc(msg_cp.srcid, msg_cp.arg0, msg_cp.arg1, msg_cp.arg2);
-               /* And if we make it back, be sure to restore the default flags.  If we
-                * never return, but the kthread exits via some other way (smp_idle()),
-                * then smp_idle() will deal with the flags.  The default state includes
-                * 'not a ktask'. */
-               pcpui->cur_kthread->flags = KTH_DEFAULT_FLAGS;
-               /* PRKM is like a cooperative ksched, and our 'thread' just yielded.  If
-                * this is too much, we can do something more limited, e.g. wait for
-                * idle, check a pcpui bit that means 'check in', etc. */
-               rcu_report_qs();
-               /* If we aren't still in early RKM, it is because the KMSG blocked
-                * (thus leaving early RKM, finishing in default context) and then
-                * returned.  This is a 'detached' RKM.  Must idle in this scenario,
-                * since we might have migrated or otherwise weren't meant to PRKM
-                * (can't return twice).  Also note that this may involve a core
-                * migration, so we need to reread pcpui.*/
-               cmb();
-               pcpui = &per_cpu_info[core_id()];
-               if (!in_early_rkmsg_ctx(pcpui))
-                       smp_idle();
-               clear_rkmsg(pcpui);
-               /* Some RKMs might turn on interrupts (perhaps in the future) and then
-                * return. */
-               disable_irq();
-       }
+       kmsg = get_next_rkmsg(pcpui);
+       if (!kmsg)
+               return;
+       msg_cp = *kmsg;
+       kmem_cache_free(kernel_msg_cache, kmsg);
+       assert(msg_cp.dstid == pcoreid);
+       /* The kmsg could block.  If it does, we want the kthread code to know it's
+        * not running on behalf of a process, and we're actually spawning a kernel
+        * task.  While we do have a syscall that does work in an RKM (change_to),
+        * it's not really the rest of the syscall context.  When we return or
+        * otherwise call smp_idle, smp_idle will reset these flags. */
+       pcpui->cur_kthread->flags = KTH_KTASK_FLAGS;
+       pcpui_trace_kmsg(pcpui, (uintptr_t)msg_cp.pc);
+       msg_cp.pc(msg_cp.srcid, msg_cp.arg0, msg_cp.arg1, msg_cp.arg2);
+       smp_idle();
 }
 
 /* extremely dangerous and racy: prints out the immed and routine kmsgs for a