From 26f306113a1b4057ac1f59050213c8f62c3a211a Mon Sep 17 00:00:00 2001
From: Frans Kaashoek <kaashoek@mit.edu>
Date: Tue, 2 Jul 2019 19:29:14 -0400
Subject: 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().
---
kernel/proc.c | 115 +++++++++++++++++++++++++---------------------------------
1 file changed, 50 insertions(+), 65 deletions(-)
(limited to 'kernel')
diff --git a/kernel/proc.c b/kernel/proc.c
index 8ea09b5..7265174 100644
--- a/kernel/proc.c
+++ b/kernel/proc.c
@@ -28,7 +28,11 @@ extern char trampout[]; // trampoline.S
void
procinit(void)
{
+ struct proc *p;
+
initlock(&ptable.lock, "ptable");
+ for(p = ptable.proc; p < &ptable.proc[NPROC]; p++)
+ initlock(&p->lock, "proc");
}
// Must be called with interrupts disabled,
@@ -60,6 +64,16 @@ myproc(void) {
return p;
}
+int
+allocpid() {
+ int pid;
+
+ acquire(&ptable.lock);
+ pid = nextpid++;
+ release(&ptable.lock);
+ return pid;
+}
+
//PAGEBREAK: 32
// Look in the process table for an UNUSED proc.
// If found, change state to EMBRYO and initialize
@@ -70,22 +84,19 @@ allocproc(void)
{
struct proc *p;
- acquire(&ptable.lock);
-
- for(p = ptable.proc; p < &ptable.proc[NPROC]; p++)
- if(p->state == UNUSED)
+ for(p = ptable.proc; p < &ptable.proc[NPROC]; p++) {
+ acquire(&p->lock);
+ if(p->state == UNUSED) {
goto found;
-
- release(&ptable.lock);
+ } else {
+ release(&p->lock);
+ }
+ }
return 0;
found:
- initlock(&p->lock, "proc");
- acquire(&p->lock);
- release(&ptable.lock);
-
p->state = EMBRYO;
- p->pid = nextpid++;
+ p->pid = allocpid();
// Allocate a page for the kernel stack.
if((p->kstack = kalloc()) == 0){
@@ -217,22 +228,17 @@ growproc(int n)
uint sz;
struct proc *p = myproc();
- acquire(&p->lock);
-
sz = p->sz;
if(n > 0){
if((sz = uvmalloc(p->pagetable, sz, sz + n)) == 0) {
- release(&p->lock);
return -1;
}
} else if(n < 0){
if((sz = uvmdealloc(p->pagetable, sz, sz + n)) == 0) {
- release(&p->lock);
return -1;
}
}
p->sz = sz;
- release(&p->lock);
return 0;
}
@@ -294,24 +300,17 @@ exit(void)
if(p == initproc)
panic("init exiting");
- acquire(&p->lock);
-
// Close all open files.
for(fd = 0; fd < NOFILE; fd++){
if(p->ofile[fd]){
struct file *f = p->ofile[fd];
- release(&p->lock);
-
fileclose(f);
-
- acquire(&p->lock);
p->ofile[fd] = 0;
}
}
struct inode *cwd = p->cwd;
- release(&p->lock);
-
+
begin_op();
iput(cwd);
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
// Per-CPU process scheduler.
// Each CPU calls scheduler() after setting itself up.
@@ -420,27 +401,31 @@ scheduler(void)
{
struct proc *p;
struct cpu *c = mycpu();
- int next = 0;
c->proc = 0;
for(;;){
// Enable interrupts on this processor.
intr_on();
- if((p = find_runnable(next)) != 0) {
- next = (next + 1) & NPROC;
- // Switch to chosen process. It is the process's job
- // to release its lock and then reacquire it
- // before jumping back to us.
- c->proc = p;
- swtch(&c->scheduler, &p->context);
-
- // Process is done running for now.
- // It should have changed its p->state before coming back.
- c->proc = 0;
- release(&p->lock);
- if(p->state == ZOMBIE) {
- reparent(p);
+ for(p = ptable.proc; p < &ptable.proc[NPROC]; p++) {
+ acquire(&p->lock);
+ if(p->state == RUNNABLE) {
+ // Switch to chosen process. It is the process's job
+ // to release its lock and then reacquire it
+ // before jumping back to us.
+ p->state = RUNNING;
+ c->proc = p;
+ swtch(&c->scheduler, &p->context);
+
+ // Process is done running for now.
+ // It should have changed its p->state before coming back.
+ c->proc = 0;
+ release(&p->lock);
+ if(p->state == ZOMBIE) {
+ reparent(p);
+ }
+ } else {
+ release(&p->lock);
}
}
}
@@ -477,10 +462,11 @@ sched(void)
void
yield(void)
{
- acquire(&myproc()->lock); //DOC: yieldlock
- myproc()->state = RUNNABLE;
+ struct proc *p = myproc();
+ acquire(&p->lock); //DOC: yieldlock
+ p->state = RUNNABLE;
sched();
- release(&myproc()->lock);
+ release(&p->lock);
}
// A fork child's very first scheduling by scheduler()
@@ -567,14 +553,13 @@ wakeup(void *chan)
{
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) {
- acquire(&p->lock);
- if(p->state != SLEEPING || p->chan != chan)
- panic("wakeup");
p->state = RUNNABLE;
- release(&p->lock);
}
+ release(&p->lock);
+ }
}
// Kill the process with the given pid.
--
cgit v1.2.3