kernel SMP interruptibility fixes.

Last year, right before I sent xv6 to the printer, I changed the
SETGATE calls so that interrupts would be disabled on entry to
interrupt handlers, and I added the nlock++ / nlock-- in trap()
so that interrupts would stay disabled while the hw handlers
(but not the syscall handler) did their work.  I did this because
the kernel was otherwise causing Bochs to triple-fault in SMP
mode, and time was short.

Robert observed yesterday that something was keeping the SMP
preemption user test from working.  It turned out that when I
simplified the lapic code I swapped the order of two register
writes that I didn't realize were order dependent.  I fixed that
and then since I had everything paged in kept going and tried
to figure out why you can't leave interrupts on during interrupt
handlers.  There are a few issues.

First, there must be some way to keep interrupts from "stacking
up" and overflowing the stack.  Keeping interrupts off the whole
time solves this problem -- even if the clock tick handler runs
long enough that the next clock tick is waiting when it finishes,
keeping interrupts off means that the handler runs all the way
through the "iret" before the next handler begins.  This is not
really a problem unless you are putting too many prints in trap
-- if the OS is doing its job right, the handlers should run
quickly and not stack up.

Second, if xv6 had page faults, then it would be important to
keep interrupts disabled between the start of the interrupt and
the time that cr2 was read, to avoid a scenario like:

   p1 page faults [cr2 set to faulting address]
   p1 starts executing trapasm.S
   clock interrupt, p1 preempted, p2 starts executing
   p2 page faults [cr2 set to another faulting address]
   p2 starts, finishes fault handler
   p1 rescheduled, reads cr2, sees wrong fault address

Alternately p1 could be rescheduled on the other cpu, in which
case it would still see the wrong cr2.  That said, I think cr2
is the only interrupt state that isn't pushed onto the interrupt
stack atomically at fault time, and xv6 doesn't care.  (This isn't
entirely hypothetical -- I debugged this problem on Plan 9.)

Third, and this is the big one, it is not safe to call cpu()
unless interrupts are disabled.  If interrupts are enabled then
there is no guarantee that, between the time cpu() looks up the
cpu id and the time that it the result gets used, the process
has not been rescheduled to the other cpu.  For example, the
very commonly-used expression curproc[cpu()] (aka the macro cp)
can end up referring to the wrong proc: the code stores the
result of cpu() in %eax, gets rescheduled to the other cpu at
just the wrong instant, and then reads curproc[%eax].

We use curproc[cpu()] to get the current process a LOT.  In that
particular case, if we arranged for the current curproc entry
to be addressed by %fs:0 and just use a different %fs on each
CPU, then we could safely get at curproc even with interrupts
disabled, since the read of %fs would be atomic with the read
of %fs:0.  Alternately, we could have a curproc() function that
disables interrupts while computing curproc[cpu()].  I've done
that last one.

Even in the current kernel, with interrupts off on entry to trap,
interrupts are enabled inside release if there are no locks held.
Also, the scheduler's idle loop must be interruptible at times
so that the clock and disk interrupts (which might make processes
runnable) can be handled.

In addition to the rampant use of curproc[cpu()], this little
snippet from acquire is wrong on smp:

  if(cpus[cpu()].nlock == 0)
    cli();
  cpus[cpu()].nlock++;

because if interrupts are off then we might call cpu(), get
rescheduled to a different cpu, look at cpus[oldcpu].nlock, and
wrongly decide not to disable interrupts on the new cpu.  The
fix is to always call cli().  But this is wrong too:

  if(holding(lock))
    panic("acquire");
  cli();
  cpus[cpu()].nlock++;

because holding looks at cpu().  The fix is:

  cli();
  if(holding(lock))
    panic("acquire");
  cpus[cpu()].nlock++;

I've done that, and I changed cpu() to complain the first time
it gets called with interrupts disabled.  (It gets called too
much to complain every time.)

I added new functions splhi and spllo that are like acquire and
release but without the locking:

  void
  splhi(void)
  {
    cli();
    cpus[cpu()].nsplhi++;
  }

  void
  spllo(void)
  {
    if(--cpus[cpu()].nsplhi == 0)
      sti();
  }

and I've used those to protect other sections of code that refer
to cpu() when interrupts would otherwise be disabled (basically
just curproc and setupsegs).  I also use them in acquire/release
and got rid of nlock.

I'm not thrilled with the names, but I think the concept -- a
counted cli/sti -- is sound.  Having them also replaces the
nlock++/nlock-- in trap.c and main.c, which is nice.

Final note: it's still not safe to enable interrupts in
the middle of trap() between lapic_eoi and returning
to user space.  I don't understand why, but we get a
fault on pop %es because 0x10 is a bad segment
descriptor (!) and then the fault faults trying to go into
a new interrupt because 0x8 is a bad segment descriptor too!
Triple fault.  I haven't debugged this yet.

diff --git a/defs.h b/defs.h
index b9e0e98bacf94f566f79126409a1b8d13e0c261d..4d2cba92efa518d654cab0614430ae5ec8b898f1 100644 (file)
--- a/defs.h
+++ b/defs.h
@@ -92,6 +92,7 @@ int             pipewrite(struct pipe*, char*, int);
 
 // proc.c
 struct proc*    copyproc(struct proc*);
+struct proc*    curproc();
 void            exit(void);
 int             growproc(int);
 int             kill(int);
@@ -114,6 +115,8 @@ void            getcallerpcs(void*, uint*);
 int             holding(struct spinlock*);
 void            initlock(struct spinlock*, char*);
 void            release(struct spinlock*);
+void            splhi();
+void            spllo();
 
 // string.c
 int             memcmp(const void*, const void*, uint);

diff --git a/dot-bochsrc b/dot-bochsrc
index 8c609bdb102f64c782c3aec9750d1cf8bb65e06d..ddd5d93dbe8cc5f0dc03fb64fc2378e5b5cfa50a 100755 (executable)
--- a/dot-bochsrc
+++ b/dot-bochsrc
@@ -107,7 +107,7 @@ romimage: file=$BXSHARE/BIOS-bochs-latest
 #  650Mhz Athlon K-7 with Linux 2.4.4/egcs-2.91.66  2 to  2.5 Mips
 #  400Mhz Pentium II with Linux 2.0.36/egcs-1.0.3   1 to  1.8 Mips
 #=======================================================================
-cpu: count=2, ips=10000000, quantum=1
+cpu: count=2, ips=10000000, quantum=1, reset_on_triple_fault=0
 
 #=======================================================================
 # MEGS

diff --git a/lapic.c b/lapic.c
index d62c8eaa09e1d1cb7f5dc21d3885a2127449bd8e..a7878fd151fbb731e4f8b334ae51bfa6a5f74426 100644 (file)
--- a/lapic.c
+++ b/lapic.c
@@ -2,7 +2,10 @@
 // See Chapter 8 & Appendix C of Intel processor manual volume 3.
 
 #include "types.h"
+#include "defs.h"
 #include "traps.h"
+#include "mmu.h"
+#include "x86.h"
 
 // Local APIC registers, divided by 4 for use as uint[] indices.
 #define ID      (0x0020/4)   // ID
@@ -84,6 +87,25 @@ lapic_init(int c)
 int
 cpu(void)
 {
+  // Cannot call cpu when interrupts are enabled:
+  // result not guaranteed to last long enough to be used!
+  // Would prefer to panic but even printing is chancy here:
+  // everything, including cprintf, calls cpu, at least indirectly
+  // through acquire and release.
+  if(read_eflags()&FL_IF){
+    static int n;
+    int i;
+    uint pcs[10];
+
+    if(n++%999 == 0){
+      getcallerpcs((uint*)read_ebp() + 2, pcs);
+      cprintf("cpu called from %x with interrupts enabled: stk");
+      for(i=0; i<10 && pcs[i] && pcs[i] != -1; i++)
+        cprintf(" %x", pcs[i]);
+      cprintf("\n");
+    }
+  }
+
   if(lapic)
     return lapic[ID]>>24;
   return 0;

diff --git a/main.c b/main.c
index 87dfd3e902f480acca04df7266474e2587f08328..9e33708e31dbb4dcba7ae65fcc1424a233276de5 100644 (file)
--- a/main.c
+++ b/main.c
@@ -18,9 +18,9 @@ main(void)
   // clear BSS
   memset(edata, 0, end - edata);
 
-  // Prevent release() from enabling interrupts.
+  // splhi() every processor during bootstrap.
   for(i=0; i<NCPU; i++)
-    cpus[i].nlock = 1;
+    cpus[i].nsplhi = 1;
 
   mp_init(); // collect info about this machine
   bcpu = mp_bcpu();
@@ -47,12 +47,14 @@ main(void)
   bootothers();    // boot other CPUs
   if(!ismp)
     timer_init(); // uniprocessor timer
+  cprintf("ismp %d\n", ismp);
+  cprintf("userinit\n");
   userinit();      // first user process
 
   // enable interrupts on this processor.
-  cpus[cpu()].nlock--;
-  sti();
+  spllo();
 
+  cprintf("scheduler\n");
   scheduler();
 }
 
@@ -66,10 +68,7 @@ mpmain(void)
   setupsegs(0);
   cpuid(0, 0, 0, 0, 0);  // memory barrier
   cpus[cpu()].booted = 1;
-
-  // Enable interrupts on this processor.
-  cpus[cpu()].nlock--;
-  sti();
+  spllo();
 
   scheduler();
 }

diff --git a/proc.c b/proc.c
index 1c705e9cc1b642d41a4e6eda32439f58aeed07c3..d060445db959b6d23a672e907192da91c1476336 100644 (file)
--- a/proc.c
+++ b/proc.c
@@ -9,7 +9,6 @@
 struct spinlock proc_table_lock;
 
 struct proc proc[NPROC];
-struct proc *curproc[NCPU];
 static struct proc *initproc;
 
 int nextpid = 1;
@@ -61,6 +60,7 @@ growproc(int n)
   cp->mem = newmem;
   kfree(oldmem, cp->sz);
   cp->sz += n;
+  setupsegs(cp);
   return cp->sz - n;
 }
 
@@ -71,6 +71,7 @@ setupsegs(struct proc *p)
 {
   struct cpu *c;
   
+  splhi();
   c = &cpus[cpu()];
   c->ts.ss0 = SEG_KDATA << 3;
   if(p)
@@ -93,6 +94,7 @@ setupsegs(struct proc *p)
 
   lgdt(c->gdt, sizeof(c->gdt));
   ltr(SEG_TSS << 3);
+  spllo();
 }
 
 // Create a new process copying p as the parent.
@@ -176,6 +178,19 @@ userinit(void)
   initproc = p;
 }
 
+// Return currently running process.
+// XXX comment better
+struct proc*
+curproc(void)
+{
+  struct proc *p;
+
+  splhi();
+  p = cpus[cpu()].curproc;
+  spllo();
+  return p;
+}
+
 //PAGEBREAK: 42
 // Per-CPU process scheduler.
 // Each CPU calls scheduler() after setting itself up.
@@ -188,12 +203,14 @@ void
 scheduler(void)
 {
   struct proc *p;
+  struct cpu *c;
   int i;
 
   for(;;){
     // Loop over process table looking for process to run.
     acquire(&proc_table_lock);
-
+    
+    c = &cpus[cpu()];
     for(i = 0; i < NPROC; i++){
       p = &proc[i];
       if(p->state != RUNNABLE)
@@ -202,14 +219,14 @@ scheduler(void)
       // Switch to chosen process.  It is the process's job
       // to release proc_table_lock and then reacquire it
       // before jumping back to us.
-      cp = p;
+      c->curproc = p;
       setupsegs(p);
       p->state = RUNNING;
-      swtch(&cpus[cpu()].context, &p->context);
+      swtch(&c->context, &p->context);
 
       // Process is done running for now.
       // It should have changed its p->state before coming back.
-      cp = 0;
+      c->curproc = 0;
       setupsegs(0);
     }
 
@@ -222,11 +239,13 @@ scheduler(void)
 void
 sched(void)
 {
+  if(read_eflags()&FL_IF)
+    panic("sched interruptible");
   if(cp->state == RUNNING)
     panic("sched running");
   if(!holding(&proc_table_lock))
     panic("sched proc_table_lock");
-  if(cpus[cpu()].nlock != 1)
+  if(cpus[cpu()].nsplhi != 1)
     panic("sched locks");
 
   swtch(&cp->context, &cpus[cpu()].context);

diff --git a/proc.h b/proc.h
index f76e2641dbc5f231f6e236c1a75cc72e22297eac..ad37271fdaff41a0b1de16cd91a79f8007529448 100644 (file)
--- a/proc.h
+++ b/proc.h
@@ -49,25 +49,23 @@ struct proc {
 //   fixed-size stack
 //   expandable heap
 
-// Arrange that cp point to the struct proc that this
-// CPU is currently running.  Such preprocessor 
-// subterfuge can be confusing, but saves a lot of typing.
-extern struct proc *curproc[NCPU];  // Current (running) process per CPU
-#define cp (curproc[cpu()])  // Current process on this CPU
-
-
 #define MPSTACK 512
 
 // Per-CPU state
 struct cpu {
   uchar apicid;               // Local APIC ID
+  struct proc *curproc;       // Process currently running.
   struct context context;     // Switch here to enter scheduler
   struct taskstate ts;        // Used by x86 to find stack for interrupt
   struct segdesc gdt[NSEGS];  // x86 global descriptor table
   char mpstack[MPSTACK];      // Per-CPU startup stack
   volatile int booted;        // Has the CPU started?
-  int nlock;                  // Number of locks currently held
+  int nsplhi;                 // Depth of splhi nesting.
 };
 
 extern struct cpu cpus[NCPU];
 extern int ncpu;
+
+// "cp" is a short alias for curproc().
+// It gets used enough to make this worthwhile.
+#define cp curproc()

diff --git a/spinlock.c b/spinlock.c
index 891f72c5b17227417935795afa50973bad37a720..61ae0931eb344a77c57275db4123a9d7ee4cab27 100644 (file)
--- a/spinlock.c
+++ b/spinlock.c
@@ -25,13 +25,10 @@ initlock(struct spinlock *lock, char *name)
 void
 acquire(struct spinlock *lock)
 {
+  splhi();
   if(holding(lock))
     panic("acquire");
 
-  if(cpus[cpu()].nlock == 0)
-    cli();
-  cpus[cpu()].nlock++;
-
   while(cmpxchg(0, 1, &lock->locked) == 1)
     ;
 
@@ -62,8 +59,7 @@ release(struct spinlock *lock)
   cpuid(0, 0, 0, 0, 0);  // memory barrier (see Ch 7, IA-32 manual vol 3)
 
   lock->locked = 0;
-  if(--cpus[cpu()].nlock == 0)
-    sti();
+  spllo();
 }
 
 // Record the current call stack in pcs[] by following the %ebp chain.
@@ -91,3 +87,26 @@ holding(struct spinlock *lock)
   return lock->locked && lock->cpu == cpu() + 10;
 }
 
+
+
+// XXX!
+// Better names?  Better functions?  
+
+void
+splhi(void)
+{
+  cli();
+  cpus[cpu()].nsplhi++;
+}
+
+void
+spllo(void)
+{
+  if(read_eflags()&FL_IF)
+    panic("spllo - interruptible");
+  if(--cpus[cpu()].nsplhi < 0)
+    panic("spllo");
+  if(cpus[cpu()].nsplhi == 0)
+    sti();
+}
+

diff --git a/sysproc.c b/sysproc.c
index 8d7d9cc6c0e6952138c5f58e4d0dca7c7d6b8e79..4a9c8de3371e17be54b4fe1b0ec3ba9ad45f367a 100644 (file)
--- a/sysproc.c
+++ b/sysproc.c
@@ -54,7 +54,6 @@ sys_sbrk(void)
     return -1;
   if((addr = growproc(n)) < 0)
     return -1;
-  setupsegs(cp);
   return addr;
 }
 

diff --git a/trap.c b/trap.c
index f6a6ce6db5a6459f5d522e4398a569070089157f..9e8d09ae647a61aa5ab8bed9de851b52512ca794 100644 (file)
--- a/trap.c
+++ b/trap.c
@@ -44,8 +44,8 @@ trap(struct trapframe *tf)
     return;
   }
 
-  // Make sure interrupts stay off during handler.
-  cpus[cpu()].nlock++;
+  // No interrupts during interrupt handling.
+  splhi();
 
   switch(tf->trapno){
   case IRQ_OFFSET + IRQ_TIMER:
@@ -84,7 +84,13 @@ trap(struct trapframe *tf)
     cp->killed = 1;
   }
   
-  cpus[cpu()].nlock--;
+  // Undo splhi but do not enable interrupts.
+  // If you change this to spllo() you can get a 
+  // triple fault by just typing too fast at the prompt.
+  // An interrupt stops us right here, and when that
+  // interrupt tries to return, somehow the segment
+  // registers are all invalid.
+  --cpus[cpu()].nsplhi;
 
   // Force process exit if it has been killed and is in user space.
   // (If it is still executing in the kernel, let it keep running 

diff --git a/x86.h b/x86.h
index 2ac51e5150d32bdc3894307f5c60677360a1d8a9..a24214d89cfbdad4bba8b2e2f7f55002dec8fe1e 100644 (file)
--- a/x86.h
+++ b/x86.h
@@ -39,6 +39,15 @@ outsl(int port, const void *addr, int cnt)
                    "cc");
 }
 
+static inline uint
+read_ebp(void)
+{
+  uint ebp;
+  
+  asm volatile("movl %%ebp, %0" : "=a" (ebp));
+  return ebp;
+}
+
 struct segdesc;
 
 static inline void