Avoid needless TLB flush when restarting kthreads
authorBarret Rhoden <brho@cs.berkeley.edu>
Mon, 3 Oct 2016 19:27:10 +0000 (15:27 -0400)
committerBarret Rhoden <brho@cs.berkeley.edu>
Thu, 6 Oct 2016 19:41:48 +0000 (15:41 -0400)
If we're about to run on a core where our address space was arleady loaded,
we don't need to reload it.  Doing so actually triggers a TLB flush.

Regarding changing this comment:

/* In the future, we could check owning_proc. If it isn't set, we
 * could clear current and transfer the refcnt to kthread->proc. */

Although that is true, it's a bit dangerous and we'd need to measure to
know if its worth the hassle.  The intent was that if owning_proc !=
current, then this kthread is running 'detached' from its process.  This
could be a syscall that briefly woke up and went back to sleep.  We could
avoid the incref and another decref shortly (when current gets cleared in
smp_idle()->abandon_core()) by transferring the ref.

The issue is that when we clear current, we also need to load a different
page table, since it's possible that the process will be freed before this
core ends up running another page table.  So we could do this optimization,
but then we'd need to load a page table, which is a TLB flush.  Then maybe
we'd be switching back to the process again, since we don't know that the
*next* kthread to run isn't also for this process.  So it's not clear that
avoiding the atomic ops (incref/decref) and moving up the TLB flush was
worth the hassle.

Signed-off-by: Barret Rhoden <brho@cs.berkeley.edu>
kern/src/kthread.c

index 4ec3723..d89b44c 100644 (file)
@@ -98,15 +98,22 @@ void restart_kthread(struct kthread *kthread)
 #endif /* CONFIG_KTHREAD_POISON */
        /* Only change current if we need to (the kthread was in process context) */
        if (kthread->proc) {
-               /* Load our page tables before potentially decreffing cur_proc */
-               lcr3(kthread->proc->env_cr3);
-               /* Might have to clear out an existing current.  If they need to be set
-                * later (like in restartcore), it'll be done on demand. */
-               if (pcpui->cur_proc)
-                       proc_decref(pcpui->cur_proc);
-               /* We also transfer our counted ref from kthread->proc to cur_proc */
-               pcpui->cur_proc = kthread->proc;
-               kthread->proc = 0;
+               if (kthread->proc == pcpui->cur_proc) {
+                       /* We're already loaded, but we do need to drop the extra ref stored
+                        * in kthread->proc. */
+                       proc_decref(kthread->proc);
+                       kthread->proc = 0;
+               } else {
+                       /* Load our page tables before potentially decreffing cur_proc */
+                       lcr3(kthread->proc->env_cr3);
+                       /* Might have to clear out an existing current.  If they need to be
+                        * set later (like in restartcore), it'll be done on demand. */
+                       if (pcpui->cur_proc)
+                               proc_decref(pcpui->cur_proc);
+                       /* Transfer our counted ref from kthread->proc to cur_proc. */
+                       pcpui->cur_proc = kthread->proc;
+                       kthread->proc = 0;
+               }
        }
        /* Finally, restart our thread */
        longjmp(&kthread->context, 1);
@@ -409,7 +416,9 @@ void sem_down(struct semaphore *sem)
                kthread->proc = current;
                assert(kthread->proc);
                /* In the future, we could check owning_proc. If it isn't set, we could
-                * clear current and transfer the refcnt to kthread->proc. */
+                * clear current and transfer the refcnt to kthread->proc.  If so, we'll
+                * need to reset the cr3 to something (boot_cr3 or owning_proc's cr3),
+                * which might not be worth the potentially excessive TLB flush. */
                proc_incref(kthread->proc, 1);
        } else {
                assert(kthread->proc == 0);