fix race in holding() check in acquire()

give cpu1 a TSS and gdt for when it enters scheduler() and a pseudo proc[] entry for each cpu cpu0 waits for each other cpu to start up read() for files

fix race in holding() check in acquire()
0e84a0ec · rtm · e8d11c2e · 0e84a0ec · 0e84a0ec · 0e84a0ec
--- a/Makefile
+++ b/Makefile
@@ -71,6 +71,10 @@ echo : echo.o $(ULIB)
 	$(LD) -N -e main -Ttext 0 -o echo echo.o $(ULIB)
 	$(OBJDUMP) -S echo > echo.asm
+cat : cat.o $(ULIB)
+	$(LD) -N -e main -Ttext 0 -o cat cat.o $(ULIB)
+	$(OBJDUMP) -S cat > cat.asm
 userfs : userfs.o $(ULIB)
 	$(LD) -N -e main -Ttext 0 -o userfs userfs.o $(ULIB)
 	$(OBJDUMP) -S userfs > userfs.asm
@@ -78,8 +82,8 @@ userfs : userfs.o $(ULIB)
 mkfs : mkfs.c fs.h
 	cc -o mkfs mkfs.c
-fs.img : mkfs usertests echo
+fs.img : mkfs usertests echo cat README
-	./mkfs fs.img usertests echo
+	./mkfs fs.img usertests echo cat README
 -include *.d

--- a/Notes
+++ b/Notes
@@ -163,3 +163,81 @@ and file arguments longer than 14
 and directories longer than one sector
 kalloc() can return 0; do callers handle this right?
+why directing interrupts to cpu 1 causes trouble
+  cpu 1 turns on interrupts with no tss!
+    and perhaps a stale gdt (from boot)
+  since it has never run a process, never called setupsegs()
+  but does cpu really need the tss?
+    not switching stacks
+  fake process per cpu, just for tss?
+    seems like a waste
+  move tss to cpu[]?
+    but tss points to per-process kernel stack
+    would also give us a gdt
+  OOPS that wasn't the problem
+wait for other cpu to finish starting before enabling interrupts?
+  some kind of crash in ide_init ioapic_enable cprintf
+move ide_init before mp_start?
+  didn't do any good
+  maybe cpu0 taking ide interrupt, cpu1 getting a nested lock error
+cprintfs are screwed up if locking is off
+  often loops forever
+  hah, just use lpt alone
+looks like cpu0 took the ide interrupt and was the last to hold
+the lock, but cpu1 thinks it is nested
+cpu0 is in load_icode / printf / cons_putc
+  probably b/c cpu1 cleared use_console_lock
+cpu1 is in scheduler() / printf / acquire
+  1: init timer
+  0: init timer
+  cpu 1 initial nlock 1
+  ne0s:t iidd el_occnkt rc
+  onsole cpu 1 old caller stack 1001A5 10071D 104DFF 1049FE
+  panic: acquire
+  ^CNext at t=33002418
+  (0) [0x00100091] 0008:0x00100091 (unk. ctxt): jmp .+0xfffffffe          ; ebfe
+  (1) [0x00100332] 0008:0x00100332 (unk. ctxt): jmp .+0xfffffffe          
+why is output interleaved even before panic?
+does release turn on interrupts even inside an interrupt handler?
+overflowing cpu[] stack?
+  probably not, change from 512 to 4096 didn't do anything
+  1: init timer
+  0: init timer
+  cnpeus te11  linnitki aclo nnoolleek  cp1u
+   ss  oarltd  sccahleldeul esrt aocnk  cpu 0111 Ej6  buf1 01A3140 C5118 
+  0
+  la anic1::7 0a0c0  uuirr e
+  ^CNext at t=31691050
+  (0) [0x00100373] 0008:0x00100373 (unk. ctxt): jmp .+0xfffffffe          ; ebfe
+  (1) [0x00100091] 0008:0x00100091 (unk. ctxt): jmp .+0xfffffffe          ; ebfe
+cpu0:
+0: init timer
+nested lock console cpu 0 old caller stack 1001e6 101a34 1 0
+  (that's mpmain)
+panic: acquire
+cpu1:
+1: init timer
+cpu 1 initial nlock 1
+start scheduler on cpu 1 jmpbuf ...
+la 107000 lr ...
+  that is, nlock != 0
+maybe a race; acquire does
+  locked = 1
+  cpu = cpu()
+what if another acquire calls holding w/ locked = 1 but
+  before cpu is set?
--- a/README
+++ b/README
+This is the content of file README.
--- a/cat.c
+++ b/cat.c
+#include "user.h"
+char buf[513];
+int
+main(int argc, char *argv[])
+{
+  int fd, i, cc;
+  if(argc < 2){
+    puts("Usage: cat files...\n");
+    exit();
+  }
+  for(i = 1; i < argc; i++){
+    fd = open(argv[i], 0);
+    if(fd < 0){
+      puts("cat: cannot open ");
+      puts(argv[i]);
+      puts("\n");
+      exit();
+    }
+    while((cc = read(fd, buf, sizeof(buf) - 1)) > 0){
+      buf[cc] = '\0';
+      puts(buf);
+    }
+    if(cc < 0){
+      puts("cat: read error\n");
+      exit();
+    }
+    close(fd);
+  }
+  exit();
+}
--- a/echo.c
+++ b/echo.c
@@ -5,7 +5,7 @@ main(int argc, char *argv[])
 {
  int i;
-  for(i = 0; i < argc; i++){
+  for(i = 1; i < argc; i++){
    puts(argv[i]);
    puts(" ");
  }

--- a/fd.c
+++ b/fd.c
@@ -69,6 +69,13 @@ fd_read(struct fd *fd, char *addr, int n)
    return -1;
  if(fd->type == FD_PIPE){
    return pipe_read(fd->pipe, addr, n);
+  } else if(fd->type == FD_FILE){
+    ilock(fd->ip);
+    int cc = readi(fd->ip, addr, fd->off, n);
+    if(cc > 0)
+      fd->off += cc;
+    iunlock(fd->ip);
+    return cc;
  } else {
    panic("fd_read");
    return -1;

--- a/fd.h
+++ b/fd.h
@@ -5,6 +5,7 @@ struct fd {
  char writeable;
  struct pipe *pipe;
  struct inode *ip;
+  uint off;
 };
 extern struct fd fds[NFD];
--- a/ide.c
+++ b/ide.c
@@ -51,14 +51,14 @@ ide_init(void)
  }
  ioapic_enable (14, 1); // 14 is IRQ # for IDE
  ide_wait_ready(0);
-  cprintf ("ide_init:done\n");
+  cprintf ("cpu%d: ide_init:done\n", cpu());
 }
 void
 ide_intr(void)
 {
  acquire(&ide_lock);
-  cprintf("%d: ide_intr\n", cpu());
+  cprintf("cpu%d: ide_intr\n", cpu());
  wakeup(&request[tail]);
  release(&ide_lock);
  lapic_eoi();

--- a/ioapic.c
+++ b/ioapic.c
@@ -65,7 +65,7 @@ ioapic_init(void)
 }
 void
-ioapic_enable (int irq, int cpu)
+ioapic_enable (int irq, int cpunum)
 {
  uint l, h;
  struct ioapic *io;
@@ -76,7 +76,7 @@ ioapic_enable (int irq, int cpu)
  ioapic_write(io, IOAPIC_REDTBL_LO(irq), l);
  h = ioapic_read(io, IOAPIC_REDTBL_HI(irq));
  h &= ~IOART_DEST;
-  h |= (cpu << APIC_ID_SHIFT);  // for fun, disk interrupts to cpu 1
+  h |= (cpunum << APIC_ID_SHIFT);  // for fun, disk interrupts to cpu 1
  ioapic_write(io, IOAPIC_REDTBL_HI(irq), h);
-  cprintf("intr %d: lo 0x%x hi 0x%x\n", irq, l, h);
+  cprintf("cpu%d: intr %d: lo 0x%x hi 0x%x\n", cpu(), irq, l, h);
 }
--- a/lapic.c
+++ b/lapic.c
@@ -110,7 +110,7 @@ lapic_write(int r, int data)
 void
 lapic_timerinit(void)
 {
-  cprintf("%d: init timer\n", cpu());
+  cprintf("cpu%d: init timer\n", cpu());
  lapic_write(LAPIC_TDCR, LAPIC_X1);
  lapic_write(LAPIC_TIMER, LAPIC_CLKIN | LAPIC_PERIODIC | (IRQ_OFFSET + IRQ_TIMER));
  lapic_write(LAPIC_TCCR, 10000000);
@@ -120,7 +120,7 @@ lapic_timerinit(void)
 void
 lapic_timerintr(void)
 {
-  cprintf("%d: timer interrupt!\n", cpu());
+  cprintf("cpu%d: timer interrupt!\n", cpu());
  lapic_write (LAPIC_EOI, 0);
 }
@@ -129,7 +129,7 @@ lapic_init(int c)
 {
  uint r, lvt;
-  cprintf("lapic_init %d\n", c);
+  cprintf("cpu%d: lapic_init %d\n", c);
  lapic_write(LAPIC_DFR, 0xFFFFFFFF); // set destination format register
  r = (lapic_read(LAPIC_ID)>>24) & 0xFF; // read APIC ID
@@ -158,7 +158,7 @@ lapic_init(int c)
  while(lapic_read(LAPIC_ICRLO) & APIC_DELIVS)
    ;
-  cprintf("Done init of an apic\n");
+  cprintf("cpu%d: apic init done\n", cpu());
 }
 void
@@ -182,7 +182,8 @@ lapic_eoi(void)
 int
 cpu(void)
 {
-  return (lapic_read(LAPIC_ID)>>24) & 0xFF;
+  int x = (lapic_read(LAPIC_ID)>>24) & 0xFF;
+  return x;
 }
 void

--- a/main.c
+++ b/main.c
@@ -42,18 +42,24 @@ main0(void)
  lapic_init(mp_bcpu());
-  cprintf("\nxV6\n\n");
+  cprintf("\n\ncpu%d: booting xv6\n\n", cpu());
  pic_init(); // initialize PIC
  ioapic_init();
  kinit(); // physical memory allocator
  tvinit(); // trap vectors
-  idtinit(); // CPU's idt
+  idtinit(); // this CPU's idt register
+  // create a fake process per CPU
+  // so each CPU always has a tss and a gdt
+  for(p = &proc[0]; p < &proc[NCPU]; p++){
+    p->state = IDLEPROC;
+    p->kstack = cpus[p-proc].mpstack;
+    p->pid = p - proc;
+  }
-  // create fake process zero
+  // fix process 0 so that copyproc() will work
  p = &proc[0];
-  memset(p, 0, sizeof *p);
-  p->state = SLEEPING;
  p->sz = 4 * PAGE;
  p->mem = kalloc(p->sz);
  memset(p->mem, 0, p->sz);
@@ -63,20 +69,20 @@ main0(void)
  p->tf->es = p->tf->ds = p->tf->ss = (SEG_UDATA << 3) | 3;
  p->tf->cs = (SEG_UCODE << 3) | 3;
  p->tf->eflags = FL_IF;
-  p->pid = 0;
-  p->ppid = 0;
  setupsegs(p);
+  // init disk device
+  ide_init(); 
  mp_startthem();
  // turn on timer and enable interrupts on the local APIC
  lapic_timerinit();
  lapic_enableintr();
-  // init disk device
-  ide_init(); 
  // Enable interrupts on this processor.
+  cprintf("cpu%d: nlock %d before -- and sti\n",
+          cpu(), cpus[0].nlock);
  cpus[cpu()].nlock--;
  sti();
@@ -94,7 +100,7 @@ main0(void)
 void
 mpmain(void)
 {
-  cprintf("an application processor\n");
+  cprintf("cpu%d: starting\n", cpu());
  idtinit(); // CPU's idt
  if(cpu() == 0)
    panic("mpmain on cpu 0");
@@ -102,8 +108,13 @@ mpmain(void)
  lapic_timerinit();
  lapic_enableintr();
+  setupsegs(&proc[cpu()]);
+  cpuid(0, 0, 0, 0, 0);	// memory barrier
+  cpus[cpu()].booted = 1;
  // Enable interrupts on this processor.
-  cprintf("cpu %d initial nlock %d\n", cpu(), cpus[cpu()].nlock);
+  cprintf("cpu%d: initial nlock %d\n", cpu(), cpus[cpu()].nlock);
  cpus[cpu()].nlock--;
  sti();

--- a/mp.c
+++ b/mp.c
@@ -219,9 +219,11 @@ mp_startthem(void)
  for(c = 0; c < ncpu; c++){
    if (c == cpu()) continue;
-    cprintf ("starting processor %d\n", c);
+    cprintf ("cpu%d: starting processor %d\n", cpu(), c);
    *(uint *)(APBOOTCODE-4) = (uint) (cpus[c].mpstack) + MPSTACK; // tell it what to use for %esp
    *(uint *)(APBOOTCODE-8) = (uint)mpmain; // tell it where to jump to
    lapic_startap(cpus[c].apicid, (uint) APBOOTCODE);
+    while(cpus[c].booted == 0)
+      ;
  }
 }
--- a/proc.c
+++ b/proc.c
@@ -11,7 +11,7 @@ struct spinlock proc_table_lock = { "proc_table" };
 struct proc proc[NPROC];
 struct proc *curproc[NCPU];
-int next_pid = 1;
+int next_pid = NCPU;
 extern void forkret(void);
 extern void forkret1(struct trapframe*);
@@ -31,7 +31,7 @@ setupsegs(struct proc *p)
  // XXX it may be wrong to modify the current segment table!
  p->gdt[0] = SEG_NULL;
-  p->gdt[SEG_KCODE] = SEG(STA_X|STA_R, 0, 0xffffffff, 0);
+  p->gdt[SEG_KCODE] = SEG(STA_X|STA_R, 0, 0x100000 + 64*1024, 0); // xxx
  p->gdt[SEG_KDATA] = SEG(STA_W, 0, 0xffffffff, 0);
  p->gdt[SEG_TSS] = SEG16(STS_T32A, (uint) &p->ts,
                                sizeof(p->ts), 0);
@@ -134,8 +134,8 @@ scheduler(void)
  struct proc *p;
  int i;
-  cprintf("start scheduler on cpu %d jmpbuf %p\n", cpu(), &cpus[cpu()].jmpbuf);
+  cprintf("cpu%d: start scheduler jmpbuf %p\n",
-  cpus[cpu()].lastproc = &proc[0];
+          cpu(), &cpus[cpu()].jmpbuf);
  if(cpus[cpu()].nlock != 0){
    cprintf("la %x lr %x\n", cpus[cpu()].lastacquire, cpus[cpu()].lastrelease   );
@@ -190,6 +190,8 @@ scheduler(void)
        panic("scheduler lock");
      }
+      setupsegs(&proc[cpu()]);
      // XXX if not holding proc_table_lock panic.
    }
    release(&proc_table_lock);

--- a/proc.h
+++ b/proc.h
@@ -33,7 +33,8 @@ struct jmpbuf {
  int eip;
 };
-enum proc_state { UNUSED, EMBRYO, SLEEPING, RUNNABLE, RUNNING, ZOMBIE };
+enum proc_state { UNUSED, EMBRYO, SLEEPING, RUNNABLE, RUNNING, ZOMBIE,
+                  IDLEPROC };
 struct proc{
  char *mem; // start of process's physical memory
@@ -67,8 +68,8 @@ extern struct proc *curproc[NCPU];  // can be NULL if no proc running.
 struct cpu {
  uchar apicid;       // Local APIC ID
  struct jmpbuf jmpbuf;
-  char mpstack[MPSTACK]; // per-cpu start-up stack, only used to get into main()
+  char mpstack[MPSTACK]; // per-cpu start-up stack
-  struct proc *lastproc;  // last proc scheduled on this cpu (never NULL)
+  volatile int booted;
  int nlock; // # of locks currently held
  struct spinlock *lastacquire; // xxx debug
  struct spinlock *lastrelease; // xxx debug

--- a/spinlock.c
+++ b/spinlock.c
@@ -12,29 +12,31 @@
 extern int use_console_lock;
-int
+void
-getcallerpc(void *v)
+getcallerpcs(void *v, uint pcs[])
 {
-	return ((int*)v)[-1];
+  uint *ebp = (uint*)v - 2;
+  int i;
+  for(i = 0; i < 10 && ebp && ebp != (uint*)0xffffffff; ebp = (uint*)*ebp, i++){
+    pcs[i] = *(ebp + 1);
+  }
+  for( ; i < 10; i++)
+    pcs[i] = 0;
 }
 void
 acquire(struct spinlock * lock)
 {
-  if(holding(lock)){
+  if(holding(lock))
-    extern use_console_lock;
+    panic("acquire");
-    use_console_lock = 0;
-    cprintf("lock %s pc %x\n", lock->name ? lock->name : "", lock->pc);
-		panic("acquire");
-  }
 	if(cpus[cpu()].nlock++ == 0)
 		cli();
 	while(cmpxchg(0, 1, &lock->locked) == 1)
 		;
 	cpuid(0, 0, 0, 0, 0);	// memory barrier
-	lock->pc = getcallerpc(&lock);
+	getcallerpcs(&lock, lock->pcs);
-	lock->cpu = cpu();
+	lock->cpu = cpu() + 10;
        cpus[cpu()].lastacquire = lock;
 }
@@ -45,6 +47,8 @@ release(struct spinlock * lock)
 		panic("release");
        cpus[cpu()].lastrelease = lock;
+        lock->pcs[0] = 0;
+        lock->cpu = 0xffffffff;
 	cpuid(0, 0, 0, 0, 0);	// memory barrier
 	lock->locked = 0;
 	if(--cpus[cpu()].nlock == 0)
@@ -54,5 +58,5 @@ release(struct spinlock * lock)
 int
 holding(struct spinlock *lock)
 {
-	return lock->locked && lock->cpu == cpu();
+	return lock->locked && lock->cpu == cpu() + 10;
 }
--- a/spinlock.h
+++ b/spinlock.h
 struct spinlock {
  char *name;
  uint locked;
-  uint pc;
+  uint pcs[10];
  int cpu;
 };
--- a/syscall.c
+++ b/syscall.c
@@ -274,6 +274,7 @@ sys_open(void)
  fd->readable = 1;
  fd->writeable = 0;
  fd->ip = ip;
+  fd->off = 0;
  cp->fds[ufd] = fd;
  return ufd;

--- a/trap.c
+++ b/trap.c
@@ -34,6 +34,14 @@ void
 trap(struct trapframe *tf)
 {
  int v = tf->trapno;
+  if(cpus[cpu()].nlock){
+    cprintf("trap v %d eip %x cpu %d nlock %d\n",
+            v, tf->eip, cpu(), cpus[cpu()].nlock);
+    panic("interrupt while holding a lock");
+  }
+  if((read_eflags() & FL_IF) == 0)
+    panic("interrupt but interrupts now disabled");
  if(v == T_SYSCALL){
    struct proc *cp = curproc[cpu()];
@@ -61,16 +69,13 @@ trap(struct trapframe *tf)
      panic("trap ret esp wrong");
    if(cp->killed)
      proc_exit();
+    // XXX probably ought to lgdt on trap return
    return;
  }
  if(v == (IRQ_OFFSET + IRQ_TIMER)){
    struct proc *cp = curproc[cpu()];
    lapic_timerintr();
-    if(cpus[cpu()].nlock)
-      panic("timer interrupt while holding a lock");
-    if((read_eflags() & FL_IF) == 0)
-      panic("timer interrupt but interrupts now disabled");
    if(cp){
      // Force process exit if it has been killed
      // and the interrupt came from user space.
@@ -92,8 +97,5 @@ trap(struct trapframe *tf)
    return;
  }
-  // XXX probably ought to lgdt on trap return
  return;
 }
--- a/user.h
+++ b/user.h
@@ -10,6 +10,8 @@ int block(void);
 int kill(int);
 int panic(char*);
 int cons_puts(char*);
+int exec(char *, char **);
+int open(char *, int);
 int mknod (char*,short,short,short);
 int puts(char*);
 int puts1(char*);

--- a/userfs.c
+++ b/userfs.c
@@ -5,7 +5,8 @@
 // file system tests
 char buf[1024];
-char *args[] = { "echo", "hello", "goodbye", 0 };
+char *echo_args[] = { "echo", "hello", "goodbye", 0 };
+char *cat_args[] = { "cat", "README", 0 };
 int
 main(void)
@@ -34,6 +35,7 @@ main(void)
  } else {
    puts("open doesnotexist failed\n");
  }
-  exec("echo", args);
+  //exec("echo", echo_args);
+  exec("cat", cat_args);
  return 0;
 }