From 0e84a0ec6e7893dad13dff9a958c5bc987b79c82 Mon Sep 17 00:00:00 2001 From: rtm Date: Tue, 8 Aug 2006 19:58:06 +0000 Subject: [PATCH] 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 --- Makefile | 8 ++++-- Notes | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ README | 1 + cat.c | 35 ++++++++++++++++++++++++ echo.c | 2 +- fd.c | 7 +++++ fd.h | 1 + ide.c | 4 +-- ioapic.c | 6 ++--- lapic.c | 11 ++++---- main.c | 35 +++++++++++++++--------- mp.c | 4 ++- proc.c | 10 ++++--- proc.h | 7 ++--- spinlock.c | 28 +++++++++++--------- spinlock.h | 2 +- syscall.c | 1 + trap.c | 16 ++++++----- user.h | 2 ++ userfs.c | 6 +++-- 20 files changed, 209 insertions(+), 55 deletions(-) create mode 100644 README create mode 100644 cat.c diff --git a/Makefile b/Makefile index c0153ad..cc83f3e 100644 --- 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 - ./mkfs fs.img usertests echo +fs.img : mkfs usertests echo cat README + ./mkfs fs.img usertests echo cat README -include *.d diff --git a/Notes b/Notes index 22658fc..2291621 100644 --- 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? diff --git a/README b/README new file mode 100644 index 0000000..ce0bbfb --- /dev/null +++ b/README @@ -0,0 +1 @@ +This is the content of file README. diff --git a/cat.c b/cat.c new file mode 100644 index 0000000..8154ae2 --- /dev/null +++ b/cat.c @@ -0,0 +1,35 @@ +#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(); +} diff --git a/echo.c b/echo.c index 89e19a9..5d0c5a4 100644 --- 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(" "); } diff --git a/fd.c b/fd.c index 9ce6bae..8332454 100644 --- 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; diff --git a/fd.h b/fd.h index 442ee34..1be1cf0 100644 --- 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]; diff --git a/ide.c b/ide.c index d6bef6d..af509fc 100644 --- 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(); diff --git a/ioapic.c b/ioapic.c index 776f895..b926863 100644 --- 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); } diff --git a/lapic.c b/lapic.c index f299b86..161d0a5 100644 --- 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 diff --git a/main.c b/main.c index 021fb51..c6e27a5 100644 --- 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 fake process zero + // 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; + } + + // 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(); diff --git a/mp.c b/mp.c index ece46a4..341b545 100644 --- 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) + ; } } diff --git a/proc.c b/proc.c index b67810e..0254c2e 100644 --- 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); - cpus[cpu()].lastproc = &proc[0]; + cprintf("cpu%d: start scheduler jmpbuf %p\n", + 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); diff --git a/proc.h b/proc.h index a273141..c6f5be6 100644 --- 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() - struct proc *lastproc; // last proc scheduled on this cpu (never NULL) + char mpstack[MPSTACK]; // per-cpu start-up stack + volatile int booted; int nlock; // # of locks currently held struct spinlock *lastacquire; // xxx debug struct spinlock *lastrelease; // xxx debug diff --git a/spinlock.c b/spinlock.c index bde6e46..5a0fd23 100644 --- a/spinlock.c +++ b/spinlock.c @@ -12,29 +12,31 @@ extern int use_console_lock; -int -getcallerpc(void *v) +void +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)){ - extern use_console_lock; - use_console_lock = 0; - cprintf("lock %s pc %x\n", lock->name ? lock->name : "", lock->pc); - panic("acquire"); - } + if(holding(lock)) + 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); - lock->cpu = cpu(); + getcallerpcs(&lock, lock->pcs); + 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; } diff --git a/spinlock.h b/spinlock.h index ee48a7e..f866b4c 100644 --- a/spinlock.h +++ b/spinlock.h @@ -1,6 +1,6 @@ struct spinlock { char *name; uint locked; - uint pc; + uint pcs[10]; int cpu; }; diff --git a/syscall.c b/syscall.c index 498078e..ce6e22d 100644 --- 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; diff --git a/trap.c b/trap.c index ccbc754..7031223 100644 --- 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; } diff --git a/user.h b/user.h index cf98816..d869338 100644 --- 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*); diff --git a/userfs.c b/userfs.c index 0b2e9c3..b11f3eb 100644 --- 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; }