Fix a lost wakeup bug: the disk driver's wakeup() can run after the

reading process acquired p->lock and released virtio lock in sleep(),
but before the process had set p->status to SLEEPING, because the
wakeup tested p->status without holding p's lock.  Thus, wakeup can
complete without seeing any process SLEEPING and then p sets p->status
to SLEEPING.

Fix some other issues:

- Don't initialize proc lock in allocproc(), because freeproc() sets
np->state = UNUSED and allocproc() can choose np and calls initlock()
on the process's lock, releasing np's lock accidentally.  Move
initializing proc's lock to init.

- Protect nextpid using ptable.lock (and move into its own function)

Some clean up:
- Don't acquire p->lock when it p is used in a private way (e.g., exit()/grow()).
- Move find_runnable() back into scheduler().
This commit is contained in:
Frans Kaashoek 2019-07-02 19:29:14 -04:00
parent 1e4d7065d6
commit 26f306113a

View file

@ -28,7 +28,11 @@ extern char trampout[]; // trampoline.S
void void
procinit(void) procinit(void)
{ {
struct proc *p;
initlock(&ptable.lock, "ptable"); initlock(&ptable.lock, "ptable");
for(p = ptable.proc; p < &ptable.proc[NPROC]; p++)
initlock(&p->lock, "proc");
} }
// Must be called with interrupts disabled, // Must be called with interrupts disabled,
@ -60,6 +64,16 @@ myproc(void) {
return p; return p;
} }
int
allocpid() {
int pid;
acquire(&ptable.lock);
pid = nextpid++;
release(&ptable.lock);
return pid;
}
//PAGEBREAK: 32 //PAGEBREAK: 32
// Look in the process table for an UNUSED proc. // Look in the process table for an UNUSED proc.
// If found, change state to EMBRYO and initialize // If found, change state to EMBRYO and initialize
@ -70,22 +84,19 @@ allocproc(void)
{ {
struct proc *p; struct proc *p;
acquire(&ptable.lock); for(p = ptable.proc; p < &ptable.proc[NPROC]; p++) {
acquire(&p->lock);
for(p = ptable.proc; p < &ptable.proc[NPROC]; p++) if(p->state == UNUSED) {
if(p->state == UNUSED)
goto found; goto found;
} else {
release(&ptable.lock); release(&p->lock);
}
}
return 0; return 0;
found: found:
initlock(&p->lock, "proc");
acquire(&p->lock);
release(&ptable.lock);
p->state = EMBRYO; p->state = EMBRYO;
p->pid = nextpid++; p->pid = allocpid();
// Allocate a page for the kernel stack. // Allocate a page for the kernel stack.
if((p->kstack = kalloc()) == 0){ if((p->kstack = kalloc()) == 0){
@ -217,22 +228,17 @@ growproc(int n)
uint sz; uint sz;
struct proc *p = myproc(); struct proc *p = myproc();
acquire(&p->lock);
sz = p->sz; sz = p->sz;
if(n > 0){ if(n > 0){
if((sz = uvmalloc(p->pagetable, sz, sz + n)) == 0) { if((sz = uvmalloc(p->pagetable, sz, sz + n)) == 0) {
release(&p->lock);
return -1; return -1;
} }
} else if(n < 0){ } else if(n < 0){
if((sz = uvmdealloc(p->pagetable, sz, sz + n)) == 0) { if((sz = uvmdealloc(p->pagetable, sz, sz + n)) == 0) {
release(&p->lock);
return -1; return -1;
} }
} }
p->sz = sz; p->sz = sz;
release(&p->lock);
return 0; return 0;
} }
@ -294,24 +300,17 @@ exit(void)
if(p == initproc) if(p == initproc)
panic("init exiting"); panic("init exiting");
acquire(&p->lock);
// Close all open files. // Close all open files.
for(fd = 0; fd < NOFILE; fd++){ for(fd = 0; fd < NOFILE; fd++){
if(p->ofile[fd]){ if(p->ofile[fd]){
struct file *f = p->ofile[fd]; struct file *f = p->ofile[fd];
release(&p->lock);
fileclose(f); fileclose(f);
acquire(&p->lock);
p->ofile[fd] = 0; p->ofile[fd] = 0;
} }
} }
struct inode *cwd = p->cwd; struct inode *cwd = p->cwd;
release(&p->lock);
begin_op(); begin_op();
iput(cwd); iput(cwd);
end_op(); end_op();
@ -389,24 +388,6 @@ wait(void)
} }
} }
// Loop over process table looking for process to run.
struct proc *find_runnable(int start) {
struct proc *p;
acquire(&ptable.lock);
for(int i = start; i < start+NPROC; i++) {
p = &ptable.proc[i % NPROC];
acquire(&p->lock);
if(p->state == RUNNABLE) {
p->state = RUNNING;
release(&ptable.lock);
return p;
}
release(&p->lock);
}
release(&ptable.lock);
return 0;
}
//PAGEBREAK: 42 //PAGEBREAK: 42
// Per-CPU process scheduler. // Per-CPU process scheduler.
// Each CPU calls scheduler() after setting itself up. // Each CPU calls scheduler() after setting itself up.
@ -420,27 +401,31 @@ scheduler(void)
{ {
struct proc *p; struct proc *p;
struct cpu *c = mycpu(); struct cpu *c = mycpu();
int next = 0;
c->proc = 0; c->proc = 0;
for(;;){ for(;;){
// Enable interrupts on this processor. // Enable interrupts on this processor.
intr_on(); intr_on();
if((p = find_runnable(next)) != 0) { for(p = ptable.proc; p < &ptable.proc[NPROC]; p++) {
next = (next + 1) & NPROC; acquire(&p->lock);
// Switch to chosen process. It is the process's job if(p->state == RUNNABLE) {
// to release its lock and then reacquire it // Switch to chosen process. It is the process's job
// before jumping back to us. // to release its lock and then reacquire it
c->proc = p; // before jumping back to us.
swtch(&c->scheduler, &p->context); p->state = RUNNING;
c->proc = p;
swtch(&c->scheduler, &p->context);
// Process is done running for now. // Process is done running for now.
// It should have changed its p->state before coming back. // It should have changed its p->state before coming back.
c->proc = 0; c->proc = 0;
release(&p->lock); release(&p->lock);
if(p->state == ZOMBIE) { if(p->state == ZOMBIE) {
reparent(p); reparent(p);
}
} else {
release(&p->lock);
} }
} }
} }
@ -477,10 +462,11 @@ sched(void)
void void
yield(void) yield(void)
{ {
acquire(&myproc()->lock); //DOC: yieldlock struct proc *p = myproc();
myproc()->state = RUNNABLE; acquire(&p->lock); //DOC: yieldlock
p->state = RUNNABLE;
sched(); sched();
release(&myproc()->lock); release(&p->lock);
} }
// A fork child's very first scheduling by scheduler() // A fork child's very first scheduling by scheduler()
@ -567,14 +553,13 @@ wakeup(void *chan)
{ {
struct proc *p; struct proc *p;
for(p = ptable.proc; p < &ptable.proc[NPROC]; p++) for(p = ptable.proc; p < &ptable.proc[NPROC]; p++) {
acquire(&p->lock);
if(p->state == SLEEPING && p->chan == chan) { if(p->state == SLEEPING && p->chan == chan) {
acquire(&p->lock);
if(p->state != SLEEPING || p->chan != chan)
panic("wakeup");
p->state = RUNNABLE; p->state = RUNNABLE;
release(&p->lock);
} }
release(&p->lock);
}
} }
// Kill the process with the given pid. // Kill the process with the given pid.