Final word on the locking fiasco?

Change pushcli / popcli so that they can never turn on
interrupts unexpectedly.  That is, if interrupts are on,
then pushcli(); popcli(); turns them off and back on, but
if they are off to begin with, then pushcli(); popcli(); is
a no-op.

I think our fundamental mistake was having a primitive
(release and then popcli nee spllo) that could turn
interrupts on at unexpected moments instead of being
explicit about when we want to start allowing interrupts.

With the new semantics, all the manual fiddling of ncli
to force interrupts off in certain sections goes away.
In return, we must explicitly mark the places where
we want to enable interrupts unconditionally, by calling sti().
There is only one: inside the scheduler loop.
This commit is contained in:
rsc 2007-09-27 21:25:37 +00:00
parent f97f0d2b3d
commit ab08960f64
5 changed files with 24 additions and 33 deletions

23
main.c
View file

@ -12,19 +12,13 @@ static void mpmain(void) __attribute__((noreturn));
int int
main(void) main(void)
{ {
int bcpu, i;
extern char edata[], end[]; extern char edata[], end[];
// clear BSS // clear BSS
memset(edata, 0, end - edata); memset(edata, 0, end - edata);
// pushcli() every processor during bootstrap.
for(i=0; i<NCPU; i++)
cpus[i].ncli = 1; // no interrupts during bootstrap
mp_init(); // collect info about this machine mp_init(); // collect info about this machine
bcpu = mp_bcpu(); lapic_init(mp_bcpu());
lapic_init(bcpu);
cprintf("\ncpu%d: starting xv6\n\n", cpu()); cprintf("\ncpu%d: starting xv6\n\n", cpu());
pinit(); // process table pinit(); // process table
@ -40,17 +34,13 @@ main(void)
if(!ismp) if(!ismp)
timer_init(); // uniprocessor timer timer_init(); // uniprocessor timer
userinit(); // first user process userinit(); // first user process
bootothers(); // start other processors
// Allocate scheduler stacks and boot the other CPUs. // Finish setting up this processor in mpmain.
for(i=0; i<ncpu; i++)
cpus[i].stack = kalloc(KSTACKSIZE);
bootothers();
// Switch to our scheduler stack and continue with mpmain.
asm volatile("movl %0, %%esp" : : "r" (cpus[bcpu].stack+KSTACKSIZE));
mpmain(); mpmain();
} }
// Bootstrap processor gets here after setting up the hardware.
// Additional processors start here. // Additional processors start here.
static void static void
mpmain(void) mpmain(void)
@ -62,7 +52,6 @@ mpmain(void)
setupsegs(0); setupsegs(0);
cpuid(0, 0, 0, 0, 0); // memory barrier cpuid(0, 0, 0, 0, 0); // memory barrier
cpus[cpu()].booted = 1; cpus[cpu()].booted = 1;
popcli();
scheduler(); scheduler();
} }
@ -73,6 +62,7 @@ bootothers(void)
extern uchar _binary_bootother_start[], _binary_bootother_size[]; extern uchar _binary_bootother_start[], _binary_bootother_size[];
uchar *code; uchar *code;
struct cpu *c; struct cpu *c;
char *stack;
// Write bootstrap code to unused memory at 0x7000. // Write bootstrap code to unused memory at 0x7000.
code = (uchar*)0x7000; code = (uchar*)0x7000;
@ -83,7 +73,8 @@ bootothers(void)
continue; continue;
// Fill in %esp, %eip and start code on cpu. // Fill in %esp, %eip and start code on cpu.
*(void**)(code-4) = c->stack + KSTACKSIZE; stack = kalloc(KSTACKSIZE);
*(void**)(code-4) = stack + KSTACKSIZE;
*(void**)(code-8) = mpmain; *(void**)(code-8) = mpmain;
lapic_startap(c->apicid, (uint)code); lapic_startap(c->apicid, (uint)code);

9
proc.c
View file

@ -179,7 +179,6 @@ userinit(void)
} }
// Return currently running process. // Return currently running process.
// XXX comment better
struct proc* struct proc*
curproc(void) curproc(void)
{ {
@ -206,11 +205,13 @@ scheduler(void)
struct cpu *c; struct cpu *c;
int i; int i;
c = &cpus[cpu()];
for(;;){ for(;;){
// Enable interrupts on this processor.
sti();
// Loop over process table looking for process to run. // Loop over process table looking for process to run.
acquire(&proc_table_lock); acquire(&proc_table_lock);
c = &cpus[cpu()];
for(i = 0; i < NPROC; i++){ for(i = 0; i < NPROC; i++){
p = &proc[i]; p = &proc[i];
if(p->state != RUNNABLE) if(p->state != RUNNABLE)
@ -229,8 +230,8 @@ scheduler(void)
c->curproc = 0; c->curproc = 0;
setupsegs(0); setupsegs(0);
} }
release(&proc_table_lock); release(&proc_table_lock);
} }
} }

2
proc.h
View file

@ -56,9 +56,9 @@ struct cpu {
struct context context; // Switch here to enter scheduler struct context context; // Switch here to enter scheduler
struct taskstate ts; // Used by x86 to find stack for interrupt struct taskstate ts; // Used by x86 to find stack for interrupt
struct segdesc gdt[NSEGS]; // x86 global descriptor table struct segdesc gdt[NSEGS]; // x86 global descriptor table
char *stack;
volatile int booted; // Has the CPU started? volatile int booted; // Has the CPU started?
int ncli; // Depth of pushcli nesting. int ncli; // Depth of pushcli nesting.
int intena; // Were interrupts enabled before pushcli?
}; };
extern struct cpu cpus[NCPU]; extern struct cpu cpus[NCPU];

View file

@ -88,15 +88,19 @@ holding(struct spinlock *lock)
} }
// Pushcli/popcli are like cli/sti except that they are matched:
// XXX! // it takes two popcli to undo two pushcli. Also, if interrupts
// Better names? Better functions? // are off, then pushcli, popcli leaves them off.
void void
pushcli(void) pushcli(void)
{ {
int eflags;
eflags = read_eflags();
cli(); cli();
cpus[cpu()].ncli++; if(cpus[cpu()].ncli++ == 0)
cpus[cpu()].intena = eflags & FL_IF;
} }
void void
@ -106,7 +110,7 @@ popcli(void)
panic("popcli - interruptible"); panic("popcli - interruptible");
if(--cpus[cpu()].ncli < 0) if(--cpus[cpu()].ncli < 0)
panic("popcli"); panic("popcli");
if(cpus[cpu()].ncli == 0) if(cpus[cpu()].ncli == 0 && cpus[cpu()].intena)
sti(); sti();
} }

5
trap.c
View file

@ -44,9 +44,6 @@ trap(struct trapframe *tf)
return; return;
} }
// No interrupts during interrupt handling.
pushcli();
switch(tf->trapno){ switch(tf->trapno){
case IRQ_OFFSET + IRQ_TIMER: case IRQ_OFFSET + IRQ_TIMER:
if(cpu() == 0){ if(cpu() == 0){
@ -84,8 +81,6 @@ trap(struct trapframe *tf)
cp->killed = 1; cp->killed = 1;
} }
popcli();
// Force process exit if it has been killed and is in user space. // 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 // (If it is still executing in the kernel, let it keep running
// until it gets to the regular system call return.) // until it gets to the regular system call return.)