no more big kernel lock

succeeds at usertests.c pipe test

Notes

@@ -85,17 +85,44 @@ test pipe reader closes then write
 test two readers, two writers.
 test children being inherited by grandparent &c
 
-kill
+some sleep()s should be interruptible by kill()
-  sleep()ing for something
-  running at user level
-  running in kernel
-  ooh, the relevant CPU may never get a clock interrupt
-  should each cpu have its own clock?
-  where to check?
-    loops around sleep()
-    return from any trap
-  rules about being killed deep inside a system call
-  test above cases
 
 cli/sti in acquire/release should nest!
   in case you acquire two locks
+
+what would need fixing if we got rid of kernel_lock?
+  console output
+  proc_exit() needs lock on proc *array* to deallocate
+  kill() needs lock on proc *array*
+  allocator's free list
+  global fd table (really free-ness)
+  sys_close() on fd table
+  fork on proc list, also next pid
+    hold lock until public slots in proc struct initialized
+
+locks
+  init_lock
+    sequences CPU startup
+  proc_table_lock
+    also protects next_pid
+  per-fd lock *just* protects count read-modify-write
+    also maybe freeness?
+  memory allocator
+  printf
+
+wakeup needs proc_table_lock
+  so we need recursive locks?
+  or you must hold the lock to call wakeup?
+
+if locks contain proc *, they can't be used at interrupt time
+  only proc_table_lock will be used at interrupt time?
+  maybe it doesn't matter if we use curproc?
+
+in general, the table locks protect both free-ness and
+  public variables of table elements
+  in many cases you can use table elements w/o a lock
+  e.g. if you are the process, or you are using an fd
+
+why can't i get a lock in console code?
+  always triple fault
+lock code shouldn't call cprintf...

console.c

@@ -1,6 +1,9 @@
 #include <types.h>
 #include <x86.h>
 #include "defs.h"
+#include "spinlock.h"
+
+struct spinlock console_lock;
 
 /*
  * copy console output to parallel port, which you can tell
@@ -26,6 +29,8 @@ cons_putc(int c)
   unsigned short *crt = (unsigned short *) 0xB8000; // base of CGA memory
   int ind;
 
+  //acquire(&console_lock);
+
   lpt_putc(c);
 
   // cursor position, 16 bits, col + 80*row
@@ -56,6 +61,8 @@ cons_putc(int c)
   outb(crtport + 1, ind >> 8);
   outb(crtport, 15);
   outb(crtport + 1, ind);
+
+  //release(&console_lock);
 }
 
 void

defs.h

@@ -13,7 +13,7 @@ struct proc;
 struct jmpbuf;
 void setupsegs(struct proc *);
 struct proc * newproc(void);
-void swtch(void);
+void swtch(int);
 void sleep(void *);
 void wakeup(void *);
 void scheduler(void);
@@ -55,10 +55,9 @@ void lapic_enableintr(void);
 void lapic_disableintr(void);
 
 // spinlock.c
-extern uint32_t kernel_lock;
+struct spinlock;
-void acquire_spinlock(uint32_t* lock);
+void acquire(struct spinlock * lock);
-void release_spinlock(uint32_t* lock);
+void release(struct spinlock * lock);
-void release_grant_spinlock(uint32_t* lock, int cpu);
 
 // main.c
 void load_icode(struct proc *p, uint8_t *binary, unsigned size);
@@ -77,6 +76,7 @@ struct fd * fd_alloc();
 void fd_close(struct fd *);
 int fd_read(struct fd *fd, char *addr, int n);
 int fd_write(struct fd *fd, char *addr, int n);
+void fd_reference(struct fd *fd);
 
 // ide.c
 void ide_init(void);

fd.c

@@ -5,6 +5,9 @@
 #include "proc.h"
 #include "defs.h"
 #include "fd.h"
+#include "spinlock.h"
+
+struct spinlock fd_table_lock;
 
 struct fd fds[NFD];
 
@@ -22,18 +25,24 @@ fd_ualloc()
   return -1;
 }
 
+/*
+ * allocate a file descriptor structure
+ */
 struct fd *
 fd_alloc()
 {
   int i;
 
+  acquire(&fd_table_lock);
   for(i = 0; i < NFD; i++){
     if(fds[i].type == FD_CLOSED){
       fds[i].type = FD_NONE;
       fds[i].count = 1;
+      release(&fd_table_lock);
       return fds + i;
     }
   }
+  release(&fd_table_lock);
   return 0;
 }
 
@@ -69,8 +78,11 @@ fd_read(struct fd *fd, char *addr, int n)
 void
 fd_close(struct fd *fd)
 {
+  acquire(&fd_table_lock);
+
   if(fd->count < 1 || fd->type == FD_CLOSED)
     panic("fd_close");
+
   fd->count -= 1;
 
   if(fd->count == 0){
@@ -79,6 +91,19 @@ fd_close(struct fd *fd)
     } else {
       panic("fd_close");
     }
+    fd->count = 0;
     fd->type = FD_CLOSED;
   }
+  
+  release(&fd_table_lock);
+}
+
+void
+fd_reference(struct fd *fd)
+{
+  acquire(&fd_table_lock);
+  if(fd->count < 1 || fd->type == FD_CLOSED)
+    panic("fd_reference");
+  fd->count += 1;
+  release(&fd_table_lock);
 }

kalloc.c

@@ -10,6 +10,9 @@
 #include "param.h"
 #include "types.h"
 #include "defs.h"
+#include "spinlock.h"
+
+struct spinlock kalloc_lock;
 
 struct run {
   struct run *next;
@@ -54,6 +57,8 @@ kfree(char *cp, int len)
   for(i = 0; i < len; i++)
     cp[i] = 1;
 
+  acquire(&kalloc_lock);
+
   rr = &freelist;
   while(*rr){
     struct run *rend = (struct run *) ((char *)(*rr) + (*rr)->len);
@@ -63,13 +68,13 @@ kfree(char *cp, int len)
       p->len = len + (*rr)->len;
       p->next = (*rr)->next;
       *rr = p;
-      return;
+      goto out;
     }
     if(pend < *rr){
       p->len = len;
       p->next = *rr;
       *rr = p;
-      return;
+      goto out;
     }
     if(p == rend){
       (*rr)->len += len;
@@ -77,13 +82,16 @@ kfree(char *cp, int len)
         (*rr)->len += (*rr)->next->len;
         (*rr)->next = (*rr)->next->next;
       }
-      return;
+      goto out;
     }
     rr = &((*rr)->next);
   }
   p->len = len;
   p->next = 0;
   *rr = p;
+
+ out:
+  release(&kalloc_lock);
 }
 
 /*
@@ -99,20 +107,25 @@ kalloc(int n)
   if(n % PAGE)
     panic("kalloc");
 
+  acquire(&kalloc_lock);
+
   rr = &freelist;
   while(*rr){
     struct run *r = *rr;
     if(r->len == n){
       *rr = r->next;
+      release(&kalloc_lock);
       return (char *) r;
     }
     if(r->len > n){
       char *p = (char *)r + (r->len - n);
       r->len -= n;
+      release(&kalloc_lock);
       return p;
     }
     rr = &(*rr)->next;
   }
+  release(&kalloc_lock);
   return 0;
 }
 

main.c

@@ -15,8 +15,6 @@ extern char _binary_user1_start[], _binary_user1_size[];
 extern char _binary_usertests_start[], _binary_usertests_size[];
 extern char _binary_userfs_start[], _binary_userfs_size[];
 
-char buf[512];
-
 int
 main()
 {
@@ -24,8 +22,6 @@ main()
 
   if (acpu) {
     cprintf("an application processor\n");
-    release_spinlock(&kernel_lock);
-    acquire_spinlock(&kernel_lock);
     idtinit(); // CPU's idt
     lapic_init(cpu());
     lapic_timerinit();
@@ -39,7 +35,6 @@ main()
   cprintf("\nxV6\n\n");
 
   pic_init(); // initialize PIC
-  mp_init(); // multiprocessor
   kinit(); // physical memory allocator
   tvinit(); // trap vectors
   idtinit(); // CPU's idt
@@ -61,11 +56,14 @@ main()
   p->ppid = 0;
   setupsegs(p);
 
+  mp_init(); // multiprocessor
+
   // turn on timer and enable interrupts on the local APIC
   lapic_timerinit();
   lapic_enableintr();
+
   // init disk device
-  ide_init(); 
+  //ide_init(); 
 
   // become interruptable
   sti();
@@ -74,7 +72,9 @@ main()
   
   load_icode(p, _binary_usertests_start, (unsigned) _binary_usertests_size);
   //load_icode(p, _binary_userfs_start, (unsigned) _binary_userfs_size);
+  p->state = RUNNABLE;
   cprintf("loaded userfs\n");
+
   scheduler();
 
   return 0;

mp.c

@@ -391,15 +391,11 @@ mp_init()
   memmove((void *) APBOOTCODE,_binary_bootother_start, 
 	  (uint32_t) _binary_bootother_size);
 
-  acquire_spinlock(&kernel_lock);
   for(c = 0; c < ncpu; c++){
     if (cpus+c == bcpu) continue;
     cprintf ("starting processor %d\n", c);
-    release_grant_spinlock(&kernel_lock, c);
     *(unsigned *)(APBOOTCODE-4) = (unsigned) (cpus[c].mpstack) + MPSTACK; // tell it what to use for %esp
     *(unsigned *)(APBOOTCODE-8) = (unsigned)&main; // tell it where to jump to
     lapic_startap(cpus + c, (uint32_t) APBOOTCODE);
-    acquire_spinlock(&kernel_lock);
-    cprintf ("done starting processor %d\n", c);
   }
 }

pipe.c

@@ -5,6 +5,7 @@
 #include "proc.h"
 #include "defs.h"
 #include "fd.h"
+#include "spinlock.h"
 
 #define PIPESIZE 512
 
@@ -13,6 +14,7 @@ struct pipe {
   int writeopen; // write fd is still open
   int writep; // next index to write
   int readp;  // next index to read
+  struct spinlock lock;
   char data[PIPESIZE];
 };
 
@@ -32,6 +34,7 @@ pipe_alloc(struct fd **fd1, struct fd **fd2)
   p->writeopen = 1;
   p->writep = 0;
   p->readp = 0;
+  memset(&p->lock, 0, sizeof(p->lock));
   (*fd1)->type = FD_PIPE;
   (*fd1)->readable = 1;
   (*fd1)->writeable = 0;
@@ -74,16 +77,21 @@ pipe_write(struct pipe *p, char *addr, int n)
 {
   int i;
 
+  acquire(&p->lock);
+
   for(i = 0; i < n; i++){
     while(((p->writep + 1) % PIPESIZE) == p->readp){
       if(p->readopen == 0)
         return -1;
+      release(&p->lock);
       wakeup(&p->readp);
       sleep(&p->writep);
+      acquire(&p->lock);
     }
     p->data[p->writep] = addr[i];
     p->writep = (p->writep + 1) % PIPESIZE;
   }
+  release(&p->lock);
   wakeup(&p->readp);
   return i;
 }
@@ -99,12 +107,14 @@ pipe_read(struct pipe *p, char *addr, int n)
     sleep(&p->readp);
   }
 
+  acquire(&p->lock);
   for(i = 0; i < n; i++){
     if(p->readp == p->writep)
       break;
     addr[i] = p->data[p->readp];
     p->readp = (p->readp + 1) % PIPESIZE;
   }
+  release(&p->lock);
   wakeup(&p->writep);
   return i;
 }

proc.c

@@ -5,6 +5,9 @@
 #include "fd.h"
 #include "proc.h"
 #include "defs.h"
+#include "spinlock.h"
+
+struct spinlock proc_table_lock;
 
 struct proc proc[NPROC];
 struct proc *curproc[NCPU];
@@ -43,6 +46,7 @@ extern void trapret();
 /*
  * internal fork(). does not copy kernel stack; instead,
  * sets up the stack to return as if from system call.
+ * caller must set state to RUNNABLE.
  */
 struct proc *
 newproc()
@@ -51,11 +55,18 @@ newproc()
   struct proc *op;
   int fd;
 
-  for(np = &proc[1]; np < &proc[NPROC]; np++)
+  acquire(&proc_table_lock);
-    if(np->state == UNUSED)
+
+  for(np = &proc[1]; np < &proc[NPROC]; np++){
+    if(np->state == UNUSED){
+      np->state = EMBRYO;
       break;
-  if(np >= &proc[NPROC])
+    }
+  }
+  if(np >= &proc[NPROC]){
+    release(&proc_table_lock);
     return 0;
+  }
 
   // copy from proc[0] if we're bootstrapping
   op = curproc[cpu()];
@@ -64,6 +75,9 @@ newproc()
 
   np->pid = next_pid++;
   np->ppid = op->pid;
+
+  release(&proc_table_lock);
+
   np->sz = op->sz;
   np->mem = kalloc(op->sz);
   if(np->mem == 0)
@@ -72,6 +86,7 @@ newproc()
   np->kstack = kalloc(KSTACKSIZE);
   if(np->kstack == 0){
     kfree(np->mem, op->sz);
+    np->state = UNUSED;
     return 0;
   }
   setupsegs(np);
@@ -91,11 +106,9 @@ newproc()
   for(fd = 0; fd < NOFILE; fd++){
     np->fds[fd] = op->fds[fd];
     if(np->fds[fd])
-      np->fds[fd]->count += 1;
+      fd_reference(np->fds[fd]);
   }
 
-  np->state = RUNNABLE;
-
   cprintf("newproc %x\n", np);
 
   return np;
@@ -111,11 +124,20 @@ scheduler(void)
   cpus[cpu()].lastproc = &proc[0];
 
   setjmp(&cpus[cpu()].jmpbuf);
-  
+
+  op = curproc[cpu()];
+  if(op){
+    if(op->newstate <= 0 || op->newstate > ZOMBIE)
+      panic("scheduler");
+    op->state = op->newstate;
+    op->newstate = -1;
+  }
+
   // find a runnable process and switch to it
   curproc[cpu()] = 0;
   np = cpus[cpu()].lastproc + 1;
   while(1){
+    acquire(&proc_table_lock);
     for(i = 0; i < NPROC; i++){
       if(np >= &proc[NPROC])
         np = &proc[0];
@@ -123,20 +145,20 @@ scheduler(void)
         break;
       np++;
     }
-    if(i < NPROC)
+
+    if(i < NPROC){
+      np->state = RUNNING;
+      release(&proc_table_lock);
       break;
-    // cprintf("swtch %d: nothing to run %d %d\n",
+    }
-            // cpu(), proc[1].state, proc[2].state);
+    
-    release_spinlock(&kernel_lock);
+    release(&proc_table_lock);
-    acquire_spinlock(&kernel_lock);
     np = &proc[0];
   }
 
   cpus[cpu()].lastproc = np;
   curproc[cpu()] = np;
   
-  np->state = RUNNING;
-
   // h/w sets busy bit in TSS descriptor sometimes, and faults
   // if it's set in LTR. so clear tss descriptor busy bit.
   np->gdt[SEG_TSS].sd_type = STS_T32A;
@@ -155,11 +177,12 @@ scheduler(void)
 // give up the cpu by switching to the scheduler,
 // which runs on the per-cpu stack.
 void
-swtch(void)
+swtch(int newstate)
 {
   struct proc *p = curproc[cpu()];
   if(p == 0)
     panic("swtch");
+  p->newstate = newstate; // basically an argument to scheduler()
   if(setjmp(&p->jmpbuf) == 0)
     longjmp(&cpus[cpu()].jmpbuf);
 }
@@ -171,8 +194,7 @@ sleep(void *chan)
   if(p == 0)
     panic("sleep");
   p->chan = chan;
-  p->state = WAITING;
+  swtch(WAITING);
-  swtch();
 }
 
 void
@@ -180,9 +202,11 @@ wakeup(void *chan)
 {
   struct proc *p;
 
+  acquire(&proc_table_lock);
   for(p = proc; p < &proc[NPROC]; p++)
     if(p->state == WAITING && p->chan == chan)
       p->state = RUNNABLE;
+  release(&proc_table_lock);
 }
 
 // give up the CPU but stay marked as RUNNABLE
@@ -191,8 +215,7 @@ yield()
 {
   if(curproc[cpu()] == 0 || curproc[cpu()]->state != RUNNING)
     panic("yield");
-  curproc[cpu()]->state = RUNNABLE;
+  swtch(RUNNABLE);
-  swtch();
 }
 
 void
@@ -211,7 +234,7 @@ proc_exit()
     }
   }
 
-  cp->state = ZOMBIE;
+  acquire(&proc_table_lock);
 
   // wake up parent
   for(p = proc; p < &proc[NPROC]; p++)
@@ -223,6 +246,8 @@ proc_exit()
     if(p->ppid == cp->pid)
       p->pid = 1;
 
+  acquire(&proc_table_lock);
+
   // switch into scheduler
-  swtch();
+  swtch(ZOMBIE);
 }

proc.h

@@ -33,11 +33,14 @@ struct jmpbuf {
   int jb_eip;
 };
 
+enum proc_state { UNUSED, EMBRYO, WAITING, RUNNABLE, RUNNING, ZOMBIE };
+
 struct proc{
   char *mem; // start of process's physical memory
   unsigned sz; // total size of mem, including kernel stack
   char *kstack; // kernel stack, separate from mem so it doesn't move
-  enum { UNUSED, RUNNABLE, WAITING, ZOMBIE, RUNNING } state;
+  enum proc_state state;
+  enum proc_state newstate; // desired state after swtch()
   int pid;
   int ppid;
   void *chan; // sleep

spinlock.c

@@ -2,51 +2,50 @@
 #include "defs.h"
 #include "x86.h"
 #include "mmu.h"
+#include "param.h"
+#include "proc.h"
+#include "spinlock.h"
 
-#define LOCK_FREE -1
 #define DEBUG 0
 
-uint32_t kernel_lock = LOCK_FREE;
-
 int getcallerpc(void *v) {
   return ((int*)v)[-1];
 }
 
-// lock = LOCK_FREE if free, else = cpu_id of owner CPU
 void
-acquire_spinlock(uint32_t* lock)
+acquire(struct spinlock * lock)
 {
-  int cpu_id = cpu();
+  struct proc * cp = curproc[cpu()];
 
   // on a real machine there would be a memory barrier here
-  if(DEBUG) cprintf("cpu%d: acquiring at %x\n", cpu_id, getcallerpc(&lock));
+  if(DEBUG) cprintf("cpu%d: acquiring at %x\n", cpu(), getcallerpc(&lock));
-  cli();
+  if (cp && lock->p == cp && lock->locked){
-  if (*lock == cpu_id)
+    lock->count += 1;
-    panic("recursive lock");
+  } else { 
-  
+    cli();
-  while ( cmpxchg(LOCK_FREE, cpu_id, lock) != cpu_id ) { ; }
+    while ( cmpxchg(0, 1, &lock->locked) != 1 ) { ; }
-  if(DEBUG) cprintf("cpu%d: acquired at %x\n", cpu_id, getcallerpc(&lock));
+    lock->locker_pc = getcallerpc(&lock);
+    lock->count = 1;
+    lock->p = cp;
+  }
+  if(DEBUG) cprintf("cpu%d: acquired at %x\n", cpu(), getcallerpc(&lock));
 }
 
 void
-release_spinlock(uint32_t* lock)
+release(struct spinlock * lock)
 {
-  int cpu_id = cpu();
+  struct proc * cp = curproc[cpu()];
-  if(DEBUG) cprintf ("cpu%d: releasing at %x\n", cpu_id, getcallerpc(&lock));
-  if (*lock != cpu_id)
-    panic("release_spinlock: releasing a lock that i don't own\n");
-  *lock = LOCK_FREE;
-  // on a real machine there would be a memory barrier here
-  sti();
-}
 
-void
+  if(DEBUG) cprintf ("cpu%d: releasing at %x\n", cpu(), getcallerpc(&lock));
-release_grant_spinlock(uint32_t* lock, int c)
-{
-  int cpu_id = cpu();
-  if(DEBUG) cprintf ("cpu%d: release_grant to %d at %x\n", cpu_id, c, getcallerpc(&lock));
-  if (*lock != cpu_id)
-    panic("release_spinlock: releasing a lock that i don't own\n");
-  *lock = c;
-}
 
+  if(lock->p != cp || lock->count < 1 || lock->locked != 1)
+    panic("release");
+
+  lock->count -= 1;
+  if(lock->count < 1){
+    lock->p = 0;
+    cmpxchg(1, 0, &lock->locked);
+    sti();
+    // on a real machine there would be a memory barrier here
+  }
+}

syscall.c

@@ -6,6 +6,7 @@
 #include "x86.h"
 #include "traps.h"
 #include "syscall.h"
+#include "spinlock.h"
 
 /*
  * User code makes a system call with INT T_SYSCALL.
@@ -18,6 +19,8 @@
  * Return value? Error indication? Errno?
  */
 
+extern struct spinlock proc_table_lock;
+
 /*
  * fetch 32 bits from a user-supplied pointer.
  * returns 1 if addr was OK, 0 if illegal.
@@ -149,6 +152,7 @@ sys_fork()
   struct proc *np;
 
   np = newproc();
+  np->state = RUNNABLE;
   return np->pid;
 }
 
@@ -170,18 +174,21 @@ sys_wait()
 
   while(1){
     any = 0;
+    acquire(&proc_table_lock);
     for(p = proc; p < &proc[NPROC]; p++){
       if(p->state == ZOMBIE && p->ppid == cp->pid){
         kfree(p->mem, p->sz);
         kfree(p->kstack, KSTACKSIZE);
         pid = p->pid;
         p->state = UNUSED;
+        release(&proc_table_lock);
         cprintf("%x collected %x\n", cp, p);
         return pid;
       }
       if(p->state != UNUSED && p->ppid == cp->pid)
         any = 1;
     }
+    release(&proc_table_lock);
     if(any == 0){
       cprintf("%x nothing to wait for\n", cp);
       return -1;
@@ -232,14 +239,17 @@ sys_kill()
   struct proc *p;
 
   fetcharg(0, &pid);
+  acquire(&proc_table_lock);
   for(p = proc; p < &proc[NPROC]; p++){
     if(p->pid == pid && p->state != UNUSED){
       p->killed = 1;
       if(p->state == WAITING)
         p->state = RUNNABLE;
+      release(&proc_table_lock);
       return 0;
     }
   }
+  release(&proc_table_lock);
   return -1;
 }
 

trap.c

@@ -5,6 +5,7 @@
 #include "defs.h"
 #include "x86.h"
 #include "traps.h"
+#include "syscall.h"
 
 struct Gatedesc idt[256];
 struct Pseudodesc idt_pd = { 0, sizeof(idt) - 1, (unsigned) &idt };
@@ -35,12 +36,6 @@ trap(struct Trapframe *tf)
 {
   int v = tf->tf_trapno;
 
-  if(tf->tf_cs == 0x8 && kernel_lock == cpu())
-    cprintf("cpu %d: trap %d from %x:%x with lock=%d\n",
-            cpu(), v, tf->tf_cs, tf->tf_eip, kernel_lock);
-
-  acquire_spinlock(&kernel_lock); // released in trapret in trapasm.S
-
   if(v == T_SYSCALL){
     struct proc *cp = curproc[cpu()];
     if(cp == 0)
@@ -55,7 +50,8 @@ trap(struct Trapframe *tf)
       panic("trap ret but not RUNNING");
     if(tf != cp->tf)
       panic("trap ret wrong tf");
-    if(read_esp() < (unsigned)cp->kstack || read_esp() >= (unsigned)cp->kstack + KSTACKSIZE)
+    if(read_esp() < (unsigned)cp->kstack ||
+       read_esp() >= (unsigned)cp->kstack + KSTACKSIZE)
       panic("trap ret esp wrong");
     if(cp->killed)
       proc_exit();

trapasm.S

@@ -22,10 +22,6 @@ alltraps:
          * expects ESP to point to a Trapframe
          */
 trapret:
-        pushl $kernel_lock
-        call release_spinlock
-        addl $0x4, %esp
-
         popal
         popl %es
         popl %ds

usertests.c

@@ -93,8 +93,8 @@ preempt()
 main()
 {
   puts("usertests starting\n");
-  //pipe1();
+  pipe1();
-  preempt();
+  //preempt();
 
   while(1)
     ;