Copies out current_tf to pcpui
authorBarret Rhoden <brho@cs.berkeley.edu>
Fri, 19 Nov 2010 02:30:02 +0000 (18:30 -0800)
committerKevin Klues <klueska@cs.berkeley.edu>
Thu, 3 Nov 2011 00:35:56 +0000 (17:35 -0700)
This is in response to a pretty bad bug/design issue.  The documentation
has a lot about it and about what to expect in future patches.  The
TODOs and checks in kthread.c will go away in a couple patches.

Ultimately, we want to avoid copying the TF multiple times, but that's
asm for another day.  Recommend writing the location of actual_tf at the
top of the stacktop (avoid computing it, simplify the code, etc).

Note: this compiles on sparc, and might actually work!  Hard to tell,
with that glibc-ish bug floating around.

Documentation/kthreads.txt
kern/arch/i686/apic.h
kern/arch/i686/trap.c
kern/arch/sparc/trap.c
kern/include/process.h
kern/include/smp.h
kern/src/kthread.c

index f131f8f..3ded61c 100644 (file)
@@ -26,9 +26,9 @@ which process context (possibly none) that was running.
 
 We also get a few other benefits, such as the ability to pick and choose which
 kthreads to run where and when.  Users of kthreads should not assume that the
-core_id() stayed the same across function calls.  
+core_id() stayed the same across blocking calls.  
 
-We can also use this infrastructure of other cases where we might want to start
+We can also use this infrastructure in other cases where we might want to start
 on a new stack.  One example is when we deal with low memory.  We may have to do
 a lot of work, but only need to do a little to allow the original thread (that
 might have failed on a page_alloc) to keep running, while we want the memory
@@ -114,12 +114,14 @@ stack variables (I don't trust the register variable).  As soon as we unlock,
 the kthread could be restarted (in theory), and it could start to clobber the
 stack in later function calls.
 
-So it is possible that we lose the semaphore race and shouldn't sleep.  We unwind the
-sleep prep work.  An alternative was to only do the prep work if we won the
-race, but that would mean we have to do a lot of work in that delicate period of
-"I'm on the queue but it is unlocked" - work that requires touching the stack.
-Or we could just hold the lock for a longer period of time, which I don't care
-to do.
+So it is possible that we lose the semaphore race and shouldn't sleep.  We
+unwind the sleep prep work.  An alternative was to only do the prep work if we
+won the race, but that would mean we have to do a lot of work in that delicate
+period of "I'm on the queue but it is unlocked" - work that requires touching
+the stack.  Or we could just hold the lock for a longer period of time, which
+I don't care to do.  What we do now is try and down the semaphore early (the
+early bailout), and if it fails then try to sleep (unlocked).  If it then
+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
@@ -208,7 +210,9 @@ As a final case, what will we do for processes that were interrupted by
 something that wants to block, but wasn't servicing a syscall?  We probably
 shouldn't have these (I don't have a good example of when we'd want it, and a
 bunch of reasons why we wouldn't), but if we do, then it might be okay anyway -
-the kthread is just holding that proc alive for a bit.
+the kthread is just holding that proc alive for a bit.  Page faults are a bit
+different - they are something the process wants at least.  I was thinking more
+about unrelated async events.  Still, shouldn't be a big deal.
 
 Kmsgs and Kthreads
 -------------------------------
@@ -243,7 +247,8 @@ similar to what happens in Linux (call schedule() on the way out, from what I
 recall).  If it was a _M, things are a bit more complicated, since this should
 only happen if the kthread is for that process (and probably a bunch of other
 things - like they said it was okay to interrupt their vcore to finish the
-syscall).
+syscall).  Note - this might not be accurate anymore (see discussions on
+current_tf).
 
 To a certain extent, routine kmsgs don't seem like a nice fit, when we really
 want to be calling schedule().  Though if you think of it as the enactment of a
@@ -252,3 +257,125 @@ sense.  The scheduling decision (as of now) was made in the interrupt handler
 when it decided to send the kernel msg.  In the future, we could split this into
 having the handler make the kthread active, and have the scheduler called to
 decide where and when to run the kthread.
+
+Current_tf, Returning Twice, and Blocking
+--------------------------------
+One of the reasons for decoupling kthreads from a vcore or the notion of a
+kernel thread per user processs/task is so that when the kernel blocks (on a
+syscall or wherever), it can return to the process.  This is the essence of the
+asynchronous kernel/syscall interface (though it's not limited to syscalls
+(pagefaults!!)).  Here is what we want it to be able to handle:
+- When a process traps (syscall, trap, or actual interrupt), the process regains
+  control when the kernel is done or when it blocks.
+- Any kernel path can block at any time.
+- Kernel control paths need to not "return twice", but we don't want to have to
+  go through acrobatics in the code to prevent this.
+
+There are a couple of approaches I considered, and it involves the nature of
+"current_tf", and a brutal bug.  Current_tf is a pointer to the trapframe of the
+process that was interrupted/trapped, and is what user context ought to be
+running on this core if we return.  Current_tf is 'made' when the kernel saves
+the context at the top of the interrupt stack (aka 'stacktop').  Then the
+kernel's call path proceeds down the same stack.  This call path may get blocked
+in a kthread.  When we block, we want to restart the current_tf.  There is a
+coupling between the kthread's stack and the storage of current_tf (contents,
+not the pointer (which is in pcpui)).
+
+This coupling presents a problem when we are in userspace and get interrupted,
+and that interrupt wants to restart a kthread.  In this case, current_tf points
+to the interrupt stack, but then we want to switch to the kthread's stack.  This
+is okay.  When that kthread wants to block again, it needs to switch back to
+another stack.  Up until this commit, it was jumping to the top of the old stack
+it was on, clobbering current_tf (took about 8-10 hours to figure this out).
+While we could just make sure to save space for current_tf, it doesn't solve the
+problem: namely that the current_tf concept is not bound to a specific kernel
+stack (kthread or otherwise).  We could have cases where more than one kthread
+starts up on a core and we end up freeing the page that holds current_tf (since
+it is a stack we no longer need).  We don't want to bother keeping stacks around
+just to hold the current_tf.  Part of the nature of this weird coupling is that
+a given kthread might or might not have the current_tf at the top of its stack.
+What a pain in the ass...
+
+The right answer is to decouple current_tf from kthread stacks.  There are two
+ways to do this.  In both ways, current_tf retains its role of the context the
+kernel restarts (or saves) when it goes back to a process, and is independent of
+blocking kthreads.  SPOILER: solution 1 is not the one I picked
+
+1) All traps/interrupts come in on one stack per core.  That stack never changes
+(regardless of blocking), and current_tf is stored at the top.  Kthreads sort of
+'dispatch' / turn into threads from this event-like handling code.  This
+actually sounds really cool!
+
+2) The contents of current_tf get stored in per-cpu-info (pcpui), thereby
+clearly decoupling it from any execution context.  Any thread of execution can
+block without any special treatment (though interrupt handlers shouldn't do
+this).  We handle the "returning twice" problem at the point of return.
+
+One nice thing about 1) is that it might make stack management easier (we
+wouldn't need to keep a spare page, since it's the default core stack).  2) is
+also tricky since we need to change some entry point code to write the TF to
+pcpui (or at least copy-out for now).
+
+The main problem with 1) is that you need to know and have code to handle when
+you "become" a kthread and are allowed to block.  It also prevents us making
+changes such that all executing contexts are kthreads (which sort of is what is
+going on, even if they don't have a struct yet).
+
+While considering 1), here's something I wanted to say: "every thread of
+execution, including a KMSG, needs to always return (and thus not block), or
+never return (and be allowed to block)."  To "become" a kthread, we'd need to
+have code that jumps stacks, and once it jumps it can never return.  It would
+have to go back to some place such as smp_idle().
+
+The jumping stacks isn't a problem, and whatever we jump to would just have to
+have smp_idle() at the end.  The problem is that this is a pain in the ass to
+work with in reality.  But wait!  Don't we do that with batched syscalls right
+now?  Yes (though we should be using kmsgs instead of the hacked together
+workqueue spread across smp_idle() and syscall.c), and it is a pain in the ass.
+It is doable with syscalls because we have that clearly defined point
+(submitting vs processing).  But what about other handlers, such as the page
+fault handler?  It could block, and lots of other handlers could block too.  All
+of those would need to have a jump point (in trap.c).  We aren't even handling
+events anymore, we are immediately jumping to other stacks, using our "event
+handler" to hold current_tf and handle how we return to current_tf.  Don't
+forget about other code (like the boot code) that wants to block.  Simply put,
+option 1 creates a layer that is a pain to work with, cuts down on the
+flexibility of the kernel to block when it wants, and doesn't handle the issue
+at its source.
+
+The issue about having a defined point in the code that you can't return back
+across (which is where 1 would jump stacks) is about "returning twice."  Imagine
+a syscall that doesn't block.  It traps into the kernel, does its work, then
+returns.  Now imagine a syscall that blocks.  Most of these calls are going to
+block on occasion, but not always (imagine the read was filled from the page
+cache).  These calls really need to handle both situations.  So in one instance,
+the call blocks.  Since we're async, we return to userspace early (pop the
+current_tf).  Now, when that kthread unblocks, its code is going to want to
+finish and unroll its stack, then pop back to userspace.  This is the 'returning
+twice' problem.  Note that a *kthread* never returns twice.  This is what makes
+the idea of magic jumping points we can't return back across (and tying that to
+how we block in the kernel) painful.
+
+The way I initially dealt with this was by always calling smp_idle(), and having
+smp_idle decide what to do.  I also used it as a place to dispatch batched
+syscalls, which is what made smp_idle() more attractive.  However, after a bit,
+I realized the real nature of returning twice: current_tf.  If we forget about
+the batching for a second, all we really need to do is not return twice.  The
+best place to do that is at the place where we consider returning to userspace:
+proc_restartcore().  Instead of calling smp_idle() all the time (which was in
+essence a "you can now block" point), and checking for current_tf to return,
+just check in restartcore to see if there is a tf to restart.  If there isn't,
+then we smp_idle().  And don't forget to handle the cases where we want to start
+and env_tf (which we ought to point current_tf to in proc_run()).
+
+As a side note, we ought to use kmsgs for batched syscalls - it will help with
+preemption latencies.  At least for all but the first syscall (which can be
+called directly).  Instead of sending a retval via current_tf about how many
+started, just put that info in the syscall struct's flags (which might help the
+remote syscall case - no need for a response message, though there are still a
+few differences (no failure model other than death)).
+
+Note that page faults will still be tricky, but at least now we don't have to
+worry about points of no return.  We just check if there is a current_tf to
+restart.  The tricky part is communicating that the PF was sorted when there
+wasn't an explicit syscall made.
index 7f45a62..380e989 100644 (file)
@@ -46,7 +46,7 @@
 #define LAPIC_TIMER_INIT                       (LAPIC_BASE + 0x380)
 #define LAPIC_TIMER_CURRENT                    (LAPIC_BASE + 0x390)
 #define LAPIC_TIMER_DIVIDE                     (LAPIC_BASE + 0x3e0)
-#define LAPIC_TIMER_DEFAULT_VECTOR     0xeb
+#define LAPIC_TIMER_DEFAULT_VECTOR     0xeb            /* Aka 235, IRQ203 */
 #define LAPIC_TIMER_DEFAULT_DIVISOR    0xa // This is 128.  Ref SDM 3.a 9.6.4
 // IPI Interrupt Command Register
 #define LAPIC_IPI_ICR_LOWER                    (LAPIC_BASE + 0x300)
index 1fd56a8..4b03b4b 100644 (file)
@@ -90,7 +90,12 @@ void set_stack_top(uintptr_t stacktop)
 /* Note the check implies we only are on a one page stack (or the first page) */
 uintptr_t get_stack_top(void)
 {
-       uintptr_t stacktop = per_cpu_info[core_id()].tss->ts_esp0;
+       struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
+       uintptr_t stacktop;
+       /* so we can check this in interrupt handlers (before smp_boot()) */
+       if (!pcpui->tss)
+               return ROUNDUP(read_esp(), PGSIZE);
+       stacktop = pcpui->tss->ts_esp0;
        if (stacktop != ROUNDUP(read_esp(), PGSIZE))
                panic("Bad stacktop: %08p esp one is %08p\n", stacktop,
                      ROUNDUP(read_esp(), PGSIZE));
@@ -269,17 +274,24 @@ env_pop_ancillary_state(env_t* e)
        // Here's where you'll restore FP/MMX/XMM regs
 }
 
+/* Helper.  For now, this copies out the TF to pcpui, and sets the tf to use it.
+ * Eventually, we ought to do this in trapentry.S */
+static void set_current_tf(struct per_cpu_info *pcpui, struct trapframe **tf)
+{
+       pcpui->actual_tf = **tf;
+       pcpui->cur_tf = &pcpui->actual_tf;
+       *tf = &pcpui->actual_tf;
+}
+
 void trap(struct trapframe *tf)
 {
+       struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
+       /* Copy out the TF for now, set tf to point to it.  */
+       if (!in_kernel(tf))
+               set_current_tf(pcpui, &tf);
+
        printd("Incoming TRAP %d on core %d, TF at %p\n", tf->tf_trapno, core_id(),
               tf);
-       /* Note we are not preemptively saving the TF in the env_tf.  We do maintain
-        * a reference to it in current_tf (a per-cpu pointer).
-        * In general, only save the tf and any silly state once you know it
-        * is necessary (blocking).  And only save it in env_tf when you know you
-        * are single core (PROC_RUNNING_S) */
-       if (!in_kernel(tf))
-               set_current_tf(tf);
        if ((tf->tf_cs & ~3) != GD_UT && (tf->tf_cs & ~3) != GD_KT) {
                print_trapframe(tf);
                panic("Trapframe with invalid CS!");
@@ -289,15 +301,18 @@ void trap(struct trapframe *tf)
         * kernel, we should just return naturally.  Note that current and tf need
         * to still be okay (might not be after blocking) */
        if (in_kernel(tf))
-               return;
+               return; /* TODO: think about this, might want a helper instead. */
        proc_restartcore(current, tf);
        assert(0);
 }
 
 void irq_handler(struct trapframe *tf)
 {
+       struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
+       /* Copy out the TF for now, set tf to point to it. */
        if (!in_kernel(tf))
-               set_current_tf(tf);
+               set_current_tf(pcpui, &tf);
+
        //if (core_id())
                printd("Incoming IRQ, ISR: %d on core %d\n", tf->tf_trapno, core_id());
 
@@ -332,7 +347,7 @@ void irq_handler(struct trapframe *tf)
         * kernel, we should just return naturally.  Note that current and tf need
         * to still be okay (might not be after blocking) */
        if (in_kernel(tf))
-               return;
+               return; /* TODO: think about this, might want a helper instead. */
        proc_restartcore(current, tf);
        assert(0);
 }
@@ -377,11 +392,14 @@ void sysenter_init(void)
 /* This is called from sysenter's asm, with the tf on the kernel stack. */
 void sysenter_callwrapper(struct trapframe *tf)
 {
-       struct per_cpu_info* coreinfo = &per_cpu_info[core_id()];
+       struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
+       /* Copy out the TF for now, set tf to point to it. */
+       if (!in_kernel(tf))
+               set_current_tf(pcpui, &tf);
+
        if (in_kernel(tf))
                panic("sysenter from a kernel TF!!");
-       coreinfo->cur_tf = tf;
-       coreinfo->tf_retval_loc = &(tf->tf_regs.reg_eax);
+       pcpui->tf_retval_loc = &(tf->tf_regs.reg_eax);
        /* Set up and run the async calls */
        prep_syscalls(current, (struct syscall*)tf->tf_regs.reg_eax,
                      tf->tf_regs.reg_esi);
@@ -461,6 +479,10 @@ void __kernel_message(struct trapframe *tf)
        per_cpu_info_t *myinfo = &per_cpu_info[core_id()];
        kernel_message_t msg_cp, *k_msg;
 
+       /* Copy out the TF for now, set tf to point to it. */
+       if (!in_kernel(tf))
+               set_current_tf(myinfo, &tf);
+
        lapic_send_eoi();
        while (1) { // will break out when there are no more messages
                /* Try to get an immediate message.  Exec and free it. */
index e60bc63..0fe5be3 100644 (file)
@@ -115,6 +115,16 @@ sysenter_init(void)
 {
 }
 
+/* Helper.  For now, this copies out the TF to pcpui, and sets the tf to use it.
+ * Eventually, we ought to do this in trap_entry.S.  Honestly, do whatever you
+ * want with this.  The **tf is for convenience in x86. */
+static void set_current_tf(struct per_cpu_info *pcpui, struct trapframe **tf)
+{
+       pcpui->actual_tf = **tf;
+       pcpui->cur_tf = &pcpui->actual_tf;
+       *tf = &pcpui->actual_tf;
+}
+
 static int
 format_trapframe(trapframe_t *tf, char* buf, int bufsz)
 {
@@ -216,8 +226,9 @@ static kernel_message_t *get_next_amsg(struct kernel_msg_list *list_head,
  * you can send yourself an IPI, and that IPIs can get squashed like on x86. */
 void handle_ipi(trapframe_t* tf)
 {
+       struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
        if (!in_kernel(tf))
-               set_current_tf(tf);
+               set_current_tf(pcpui, &tf);
        else if((void*)tf->pc == &cpu_halt) // break out of the cpu_halt loop
                advance_pc(tf);
 
@@ -424,14 +435,15 @@ fp_disabled(trapframe_t* state)
 void
 handle_pop_tf(trapframe_t* state)
 {
-       set_current_tf(state);
+       struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
+       set_current_tf(pcpui, &state);
 
-       trapframe_t tf;
+       trapframe_t tf, *tf_p = &tf;
        if(memcpy_from_user(current,&tf,(void*)state->gpr[8],sizeof(tf)))
                proc_destroy(current);
 
        proc_secure_trapframe(&tf);
-       set_current_tf(&tf);
+       set_current_tf(pcpui, &tf_p);
        proc_restartcore(current,&tf);
 }
 
@@ -446,6 +458,7 @@ handle_set_tf(trapframe_t* state)
 void
 handle_syscall(trapframe_t* state)
 {
+       struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
        uint32_t num = state->gpr[1];
        uint32_t a1 = state->gpr[8];
        uint32_t a2 = state->gpr[9];
@@ -457,7 +470,7 @@ handle_syscall(trapframe_t* state)
        enable_irq();
        struct per_cpu_info* coreinfo = &per_cpu_info[core_id()];
 
-       set_current_tf(state);
+       set_current_tf(pcpui, &state);
 
        coreinfo->tf_retval_loc = &(state->gpr[8]);
 
index b3cff35..3f4900f 100644 (file)
@@ -139,11 +139,8 @@ void proc_preempt_all(struct proc *p, uint64_t usec);
  * due to some circular dependencies with smp.h.  This is done here instead of
  * elsewhere (like trap.h) for other elliptical reasons.  Note the distinction
  * between kernel and user contexts.  The kernel always returns to its nested,
- * interrupted contexts via iret/etc.  We don't always do that for user
- * contexts. */
+ * interrupted contexts via iret/etc.  We never do that for user contexts. */
 #define current_tf per_cpu_info[core_id()].cur_tf
-#define set_current_tf(tf) ({ assert(!in_kernel(tf)); \
-                              per_cpu_info[core_id()].cur_tf = (tf); })
 
 void abandon_core(void);
 /* Hold the proc_lock, since it'll use the vcoremapping to send an unmapping
index 9e60344..c94a63b 100644 (file)
@@ -26,8 +26,10 @@ struct per_cpu_info {
        /* Process management */
        // cur_proc should be valid on all cores that are not management cores.
        struct proc *cur_proc;
-       trapframe_t *cur_tf;
+       struct trapframe *cur_tf;       /* user tf we came in on (can be 0) */
+       struct trapframe actual_tf;     /* storage for cur_tf */
        struct kthread *spare;          /* useful when restarting */
+
        /* Syscall management */
        struct syscall *cur_sysc;       /* ptr is into cur_proc's address space */
        struct syscall *next_syscs;     /* other batched ones to do next */
index 5698c91..ae272e7 100644 (file)
@@ -40,7 +40,7 @@ void sleep_on(struct semaphore *sem)
        if (sem->nr_signals > 0) {
                sem->nr_signals--;
                spin_unlock(&sem->lock);
-               goto block_return_path;
+               goto block_return_path_np;
        }
        /* we're probably going to sleep, so get ready.  We'll check again later. */
        spin_unlock(&sem->lock);
@@ -91,6 +91,9 @@ void sleep_on(struct semaphore *sem)
        /* Switch to the core's default stack.  After this, don't use local
         * variables.  TODO: we shouldn't be using new_stacktop either, can't always
         * trust the register keyword (AFAIK). */
+       /* TODO: we shouldn't do this if new_stacktop is on the same page as cur_tf */
+       assert(ROUNDDOWN((uintptr_t)current_tf, PGSIZE) !=
+              kthread->stacktop - PGSIZE);
        set_stack_pointer(new_stacktop);
        smp_idle();
        /* smp_idle never returns */
@@ -111,7 +114,8 @@ unwind_sleep_prep:
        /* save the "freshly alloc'd" stack/page, not the one we came in on */
        kthread->stacktop = new_stacktop;
 block_return_path:
-       printd("Returning from being 'blocked'!\n");
+       printd("Returning from being 'blocked'! at %llu\n", read_tsc());
+block_return_path_np:
        enable_irqsave(&irq_state);
        return;
 }
@@ -130,6 +134,10 @@ void restart_kthread(struct kthread *kthread)
         * free our current kthread *before* popping it, nor can we free the current
         * stack until we pop to the kthread's stack). */
        if (pcpui->spare) {
+               /* TODO: we shouldn't free a page holding current_tf */
+               assert(ROUNDDOWN((uintptr_t)current_tf, PGSIZE) !=
+                      kthread->stacktop - PGSIZE);
+               /* this should probably have a rounddown, since it's not always the top */
                page_decref(kva2page((void*)kthread->stacktop - PGSIZE));
                kmem_cache_free(kthread_kcache, pcpui->spare);
        }