Kthread sleeping uses setjmp
authorBarret Rhoden <brho@cs.berkeley.edu>
Tue, 30 Dec 2014 21:19:59 +0000 (16:19 -0500)
committerBarret Rhoden <brho@cs.berkeley.edu>
Wed, 31 Dec 2014 18:03:10 +0000 (13:03 -0500)
The old save_kernel_ctx was doing the same thing as setjmp, but with more
overhead and possible some other issues that were triggering
-Wmaybe-uninitialized.

Documentation/kthreads.txt
kern/arch/riscv/entry.S
kern/arch/riscv/trap.h
kern/arch/x86/trap32.c
kern/arch/x86/trap32.h
kern/arch/x86/trap64.c
kern/arch/x86/trap64.h
kern/include/kthread.h
kern/include/trap.h
kern/src/kthread.c

index 72a5758..d87a951 100644 (file)
@@ -73,8 +73,8 @@ Freeing Stacks and Structs
 -------------------------------
 When we restart a kthread, we have to be careful about freeing the old stack and
 the struct kthread.  We need to delay the freeing of both of these until after
-we pop_kernel_ctx().  We can't free the kthread before popping it, and we are on
-the stack we need to free (until we pop to the new stack).
+we longjmp to the new kthread.  We can't free the kthread before popping it,
+and we are on the stack we need to free (until we pop to the new stack).
 
 To deal with this, we have a "spare" kthread per core, which gets assigned as
 the spare when we restart a previous kthread.  When making/suspending a kthread,
@@ -125,7 +125,7 @@ loses the race (unlikely), it can manually unwind.
 
 Note that a lot of this is probably needless worry - we have interrupts disabled
 for most of sleep_on(), though arguably we can be a little more careful with
-pcpui->spare and move the disable_irq() down to right before save_kernel_ctx().
+pcpui->spare and move the disable_irq() down to right before setjmp().
 
 What's the Deal with Stacks/Stacktops?
 -------------------------------
@@ -166,7 +166,7 @@ kthread run with its stack as the default stacktop.
 
 When restarting a kthread, we eventually will use its stack, instead of the
 current one, but we can't free the current stack until after we actually
-pop_kernel_ctx().  this is the same problem as with the struct kthread dealloc.
+longjmp() to it.  This is the same problem as with the struct kthread dealloc.
 So we can have the kthread (which we want to free later) hold on to the page we
 wanted to dealloc.  Likewise, when we would need a fresh kthread, we also need a
 page to use as the default stacktop.  So if we had a cached kthread, we then use
index 63fa295..87dee12 100644 (file)
@@ -38,6 +38,7 @@ save_kernel_tf_asm:
   STORE  ra,33*REGBYTES(a0)
   ret
 
+# Remove these (or this comment) when implementing setjmp on riscv.
   .text
   .global pop_kernel_ctx
 pop_kernel_ctx:
index 8810234..64af828 100644 (file)
 /* this is for an ipi that just wakes a core, but has no handler (for now) */
 #define I_POKE_CORE 254
 
-/* For kernel contexts, when we save/restore/move them around. */
-struct kernel_ctx {
-       /* RISCV's current pop_kernel_ctx assumes the hw_tf is the first member */
-       struct hw_trapframe             hw_tf;
-};
-
 static inline bool in_kernel(struct hw_trapframe *hw_tf)
 {
        return hw_tf->sr & SR_PS;
@@ -62,15 +56,6 @@ set_frame_pointer(uintptr_t fp)
        asm volatile("move fp, %0" : : "r"(fp) : "memory");
 }
 
-/* Save's the current kernel context into tf, setting the PC to the end of this
- * function.  Note the kernel doesn't need to save a lot.
- * Implemented with extern function to cause compiler to clobber most regs. */
-static inline void save_kernel_ctx(struct kernel_ctx *ctx)
-{
-  extern void save_kernel_tf_asm(struct hw_trapframe*);
-       save_kernel_tf_asm(&ctx->hw_tf);
-}
-
 void handle_trap(struct hw_trapframe *hw_tf);
 int emulate_fpu(struct hw_trapframe *hw_tf);
 
index 0ffa700..d1a8714 100644 (file)
 #include <kdebug.h>
 #include <kmalloc.h>
 
-/* Starts running the current TF, just using ret. */
-void pop_kernel_ctx(struct kernel_ctx *ctx)
-{
-       asm volatile ("movl %1,%%esp;           " /* move to future stack */
-                     "pushl %2;                " /* push cs */
-                     "movl %0,%%esp;           " /* move to TF */
-                     "addl $0x20,%%esp;        " /* move to tf_gs slot */
-                     "movl %1,(%%esp);         " /* write future esp */
-                     "subl $0x20,%%esp;        " /* move back to tf start */
-                     "popal;                   " /* restore regs */
-                     "popl %%esp;              " /* set stack ptr */
-                     "subl $0x4,%%esp;         " /* jump down past CS */
-                     "ret                      " /* return to the EIP */
-                     :
-                     : "g"(&ctx->hw_tf), "r"(ctx->hw_tf.tf_esp),
-                       "r"(ctx->hw_tf.tf_eip)
-                     : "memory");
-       panic("ret failed");                            /* mostly to placate your mom */
-}
-
 static void print_regs(push_regs_t *regs)
 {
        printk("  edi  0x%08x\n", regs->reg_edi);
index 4a67b23..d70a52b 100644 (file)
 #error "Do not include arch/trap32.h directly."
 #endif
 
-/* For kernel contexts, when we save/restore/move them around. */
-struct kernel_ctx {
-       struct hw_trapframe             hw_tf;
-};
-
 static inline bool in_kernel(struct hw_trapframe *hw_tf)
 {
        return (hw_tf->tf_cs & ~3) == GD_KT;
@@ -34,27 +29,6 @@ static inline uintptr_t get_hwtf_fp(struct hw_trapframe *hw_tf)
        return hw_tf->tf_regs.reg_ebp;
 }
 
-static inline void save_kernel_ctx(struct kernel_ctx *ctx)
-{
-       /* Save the regs and the future esp. */
-       /* Careful when updating; %0, %1, and %2 are not listed as clobbers */
-       asm volatile("movl %%esp,(%0);       " /* save esp in it's slot*/
-                    "pushl %%eax;           " /* temp save eax */
-                    "leal 1f,%%eax;         " /* get future eip */
-                    "movl %%eax,(%1);       " /* store future eip */
-                    "popl %%eax;            " /* restore eax */
-                    "movl %2,%%esp;         " /* move to the beginning of the tf */
-                    "addl $0x20,%%esp;      " /* move to after the push_regs */
-                    "pushal;                " /* save regs */
-                    "addl $0x44,%%esp;      " /* move to esp slot */
-                    "popl %%esp;            " /* restore esp */
-                    "1:                     " /* where this tf will restart */
-                    : 
-                    : "r"(&ctx->hw_tf.tf_esp), "r"(&ctx->hw_tf.tf_eip),
-                      "g"(&ctx->hw_tf)
-                    : "eax", "memory", "cc");
-}
-
 static inline uintptr_t x86_get_ip_hw(struct hw_trapframe *hw_tf)
 {
        return hw_tf->tf_eip;
index 3dc40aa..754c90b 100644 (file)
 #include <kdebug.h>
 #include <kmalloc.h>
 
-/* Starts running the current TF, just using ret. */
-void pop_kernel_ctx(struct kernel_ctx *ctx)
-{
-       struct sw_trapframe *tf = &ctx->sw_tf;
-       /* We're starting at rbx's spot in the sw_tf */
-       asm volatile ("movq %0, %%rsp;          "
-                                 "popq %%rbx;              "
-                                 "popq %%rbp;              "
-                                 "popq %%r12;              "
-                                 "popq %%r13;              "
-                                 "popq %%r14;              "
-                                 "popq %%r15;              "
-                                 "popq %%rax;              " /* pop rip */
-                                 "popq %%rsp;              "
-                                 "jmp *%%rax;              " /* stored rip */
-                                 : : "g"(&ctx->sw_tf.tf_rbx) : "memory");
-       panic("ret failed");
-}
-
 void print_trapframe(struct hw_trapframe *hw_tf)
 {
        static spinlock_t ptf_lock = SPINLOCK_INITIALIZER_IRQSAVE;
index 38d6370..21a87e8 100644 (file)
 #error "Do not include arch/trap64.h directly."
 #endif
 
-/* For kernel contexts, when we save/restore/move them around. */
-struct kernel_ctx {
-       struct sw_trapframe             sw_tf;
-};
-
 void print_swtrapframe(struct sw_trapframe *sw_tf);
 
 static inline bool in_kernel(struct hw_trapframe *hw_tf)
@@ -36,27 +31,6 @@ static inline uintptr_t get_hwtf_fp(struct hw_trapframe *hw_tf)
        return hw_tf->tf_rbp;
 }
 
-/* Using SW contexts for now, for x86_64 */
-static inline void save_kernel_ctx(struct kernel_ctx *ctx)
-{
-       long dummy;
-       /* not bothering with the FP fields */
-       asm volatile("mov %%rsp, 0x48(%0);   " /* save rsp in its slot*/
-                    "leaq 1f, %%rax;        " /* get future rip */
-                    "mov %%rax, 0x40(%0);   " /* save rip in its slot*/
-                    "mov %%r15, 0x38(%0);   "
-                    "mov %%r14, 0x30(%0);   "
-                    "mov %%r13, 0x28(%0);   "
-                    "mov %%r12, 0x20(%0);   "
-                    "mov %%rbp, 0x18(%0);   "
-                    "mov %%rbx, 0x10(%0);   "
-                    "1:                     " /* where this tf will restart */
-                                : "=D"(dummy) /* force rdi clobber */
-                                : "D"(&ctx->sw_tf)
-                    : "rax", "rcx", "rdx", "rsi", "r8", "r9", "r10", "r11",
-                      "memory", "cc");
-}
-
 static inline uintptr_t x86_get_ip_hw(struct hw_trapframe *hw_tf)
 {
        return hw_tf->tf_rip;
index e3bd513..8b24dff 100644 (file)
@@ -13,6 +13,7 @@
 #include <trap.h>
 #include <sys/queue.h>
 #include <atomic.h>
+#include <setjmp.h>
 
 struct proc;
 struct kthread;
@@ -26,7 +27,7 @@ TAILQ_HEAD(semaphore_tailq, semaphore);
  * a kthread is running, we make sure its stacktop is the default kernel stack,
  * meaning it will receive the interrupts from userspace. */
 struct kthread {
-       struct kernel_ctx                       context;
+       struct jmpbuf                           context;
        uintptr_t                                       stacktop;
        struct proc                                     *proc;
        struct syscall                          *sysc;
index b46b0a2..60e25d5 100644 (file)
@@ -30,10 +30,6 @@ extern inline void init_fp_state(void);
 void set_stack_top(uintptr_t stacktop);
 uintptr_t get_stack_top(void);
 
-/* It's important that this is inline and that ctx is not a stack variable */
-static inline void save_kernel_ctx(struct kernel_ctx *ctx)
-                   __attribute__((always_inline, returns_twice));
-void pop_kernel_ctx(struct kernel_ctx *ctx) __attribute__((noreturn));
 void send_nmi(uint32_t os_coreid);
 void reflect_unhandled_trap(unsigned int trap_nr, unsigned int err,
                             unsigned long aux);
index 50d5372..01bcd26 100644 (file)
@@ -107,7 +107,7 @@ void restart_kthread(struct kthread *kthread)
                pcpui->cur_proc = kthread->proc;
        }
        /* Finally, restart our thread */
-       pop_kernel_ctx(&kthread->context);
+       longjmp(&kthread->context, 1);
 }
 
 /* Kmsg handler to launch/run a kthread.  This must be a routine message, since
@@ -278,7 +278,6 @@ bool sem_trydown(struct semaphore *sem)
  * signal is already there is not optimized. */
 void sem_down(struct semaphore *sem)
 {
-       volatile bool blocking = TRUE;  /* signal to short circuit when restarting*/
        struct kthread *kthread, *new_kthread;
        register uintptr_t new_stacktop;
        struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
@@ -360,11 +359,8 @@ void sem_down(struct semaphore *sem)
        } else {
                kthread->proc = 0;
        } 
-       /* Save the context, toggle blocking for the reactivation */
-       save_kernel_ctx(&kthread->context);
-       if (!blocking)
+       if (setjmp(&kthread->context))
                goto block_return_path;
-       blocking = FALSE;                                       /* for when it starts back up */
        /* Down the semaphore.  We need this to be inline.  If we're sleeping, once
         * we unlock the kthread could be started up again and can return and start
         * trashing this function's stack, hence the weird control flow. */
@@ -412,12 +408,10 @@ unwind_sleep_prep:
 #endif /* CONFIG_KTHREAD_POISON */
 block_return_path:
        printd("[kernel] Returning from being 'blocked'! at %llu\n", read_tsc());
-       /* restart_kthread and pop_kernel_ctx do not reenable IRQs.  IRQs (and
-        * flags) are in whatever state they were in by the restart code (disabled,
-        * flags are garbage.  save_kernel_ctx expects the flags to be clobbered.
-        * ("cc")).  We need to make sure irqs are on if they were on when we
-        * started to block.  If they were already on and we short-circuited the
-        * block, it's harmless to reenable them. */
+       /* restart_kthread and longjmp did not reenable IRQs.  We need to make sure
+        * irqs are on if they were on when we started to block.  If they were
+        * already on and we short-circuited the block, it's harmless to reenable
+        * them. */
        if (irqs_were_on)
                enable_irq();
        return;