From 67702cf706bce7adef472f0caa48d81ddfaeb33a Mon Sep 17 00:00:00 2001
From: Frans Kaashoek <kaashoek@mit.edu>
Date: Tue, 2 Jul 2019 09:14:47 -0400
Subject: [PATCH] Checkpoint switching to per-process locks, in attempt clarify
xv6's locking plan, which is a difficult to understand because ptable lock
protects many invariants. This implementation has a bug: once in a while xv6
unlocks a proc lock that is locked by another core.
---
kernel/exec.c | 5 +-
kernel/fs.c | 2 +-
kernel/pipe.c | 2 +-
kernel/proc.c | 194 +++++++++++++++++++++++++++------------------
kernel/proc.h | 1 +
kernel/sleeplock.c | 2 +-
kernel/spinlock.c | 8 +-
kernel/syscall.c | 3 +
kernel/sysfile.c | 2 +-
kernel/sysproc.c | 1 +
kernel/trap.c | 2 +-
kernel/uart.c | 2 +-
12 files changed, 139 insertions(+), 85 deletions(-)
diff --git a/kernel/exec.c b/kernel/exec.c
index c9af395..b21afbb 100644
--- a/kernel/exec.c
+++ b/kernel/exec.c
@@ -2,6 +2,7 @@
#include "param.h"
#include "memlayout.h"
#include "riscv.h"
+#include "spinlock.h"
#include "proc.h"
#include "defs.h"
#include "elf.h"
@@ -19,7 +20,6 @@ exec(char *path, char **argv)
struct proghdr ph;
pagetable_t pagetable = 0, oldpagetable;
struct proc *p = myproc();
- uint64 oldsz = p->sz;
begin_op();
@@ -60,6 +60,9 @@ exec(char *path, char **argv)
end_op();
ip = 0;
+ p = myproc();
+ uint64 oldsz = p->sz;
+
// Allocate two pages at the next page boundary.
// Use the second as the user stack.
sz = PGROUNDUP(sz);
diff --git a/kernel/fs.c b/kernel/fs.c
index ebd8f7a..c241b3c 100644
--- a/kernel/fs.c
+++ b/kernel/fs.c
@@ -14,8 +14,8 @@
#include "defs.h"
#include "param.h"
#include "stat.h"
-#include "proc.h"
#include "spinlock.h"
+#include "proc.h"
#include "sleeplock.h"
#include "fs.h"
#include "buf.h"
diff --git a/kernel/pipe.c b/kernel/pipe.c
index 31bf0cc..eca5959 100644
--- a/kernel/pipe.c
+++ b/kernel/pipe.c
@@ -2,9 +2,9 @@
#include "riscv.h"
#include "defs.h"
#include "param.h"
+#include "spinlock.h"
#include "proc.h"
#include "fs.h"
-#include "spinlock.h"
#include "sleeplock.h"
#include "file.h"
diff --git a/kernel/proc.c b/kernel/proc.c
index 4ae34c8..ae35105 100644
--- a/kernel/proc.c
+++ b/kernel/proc.c
@@ -2,8 +2,8 @@
#include "param.h"
#include "memlayout.h"
#include "riscv.h"
-#include "proc.h"
#include "spinlock.h"
+#include "proc.h"
#include "defs.h"
struct {
@@ -21,7 +21,7 @@ extern void forkret(void);
// for returning out of the kernel
extern void sysexit(void);
-static void wakeup1(void *chan);
+static void wakeup1(struct proc *chan);
extern char trampout[]; // trampoline.S
@@ -80,11 +80,13 @@ allocproc(void)
return 0;
found:
+ initlock(&p->lock, "proc");
+ acquire(&p->lock);
+ release(&ptable.lock);
+
p->state = EMBRYO;
p->pid = nextpid++;
- release(&ptable.lock);
-
// Allocate a page for the kernel stack.
if((p->kstack = kalloc()) == 0){
p->state = UNUSED;
@@ -177,15 +179,9 @@ userinit(void)
safestrcpy(p->name, "initcode", sizeof(p->name));
p->cwd = namei("/");
- // this assignment to p->state lets other cores
- // run this process. the acquire forces the above
- // writes to be visible, and the lock is also needed
- // because the assignment might not be atomic.
- acquire(&ptable.lock);
-
p->state = RUNNABLE;
- release(&ptable.lock);
+ release(&p->lock);
}
// Grow current process's memory by n bytes.
@@ -196,15 +192,22 @@ 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)
+ 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)
+ if((sz = uvmdealloc(p->pagetable, sz, sz + n)) == 0) {
+ release(&p->lock);
return -1;
+ }
}
p->sz = sz;
+ release(&p->lock);
return 0;
}
@@ -244,11 +247,9 @@ fork(void)
pid = np->pid;
- acquire(&ptable.lock);
-
np->state = RUNNABLE;
- release(&ptable.lock);
+ release(&np->lock);
return pid;
}
@@ -260,45 +261,65 @@ void
exit(void)
{
struct proc *p = myproc();
- struct proc *pp;
int fd;
if(p == initproc)
panic("init exiting");
+ acquire(&p->lock);
+
// Close all open files.
for(fd = 0; fd < NOFILE; fd++){
if(p->ofile[fd]){
- fileclose(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(p->cwd);
+ iput(cwd);
end_op();
+
+ acquire(&p->lock);
p->cwd = 0;
- acquire(&ptable.lock);
-
- // Parent might be sleeping in wait().
- wakeup1(p->parent);
-
- // Pass abandoned children to init.
- for(pp = ptable.proc; pp < &ptable.proc[NPROC]; pp++){
- if(pp->parent == p){
- pp->parent = initproc;
- if(pp->state == ZOMBIE)
- wakeup1(initproc);
- }
- }
-
// Jump into the scheduler, never to return.
p->state = ZOMBIE;
sched();
panic("zombie exit");
}
+void tellparent(struct proc *p, struct proc *parent) {
+ struct proc *pp;
+
+ acquire(&parent->lock);
+
+ // Parent might be sleeping in wait().
+ wakeup1(parent);
+
+ // Pass abandoned children to init.
+ for(pp = ptable.proc; pp < &ptable.proc[NPROC]; pp++){
+ if(pp->parent == p){
+ pp->parent = initproc;
+ if(pp->state == ZOMBIE) {
+ acquire(&initproc->lock);
+ wakeup1(initproc);
+ acquire(&initproc->lock);
+ }
+ }
+ }
+
+ release(&parent->lock);
+}
+
// Wait for a child process to exit and return its pid.
// Return -1 if this process has no children.
int
@@ -307,14 +328,17 @@ wait(void)
struct proc *np;
int havekids, pid;
struct proc *p = myproc();
+
+ // should p lock!
- acquire(&ptable.lock);
+ acquire(&p->lock);
for(;;){
// Scan through table looking for exited children.
havekids = 0;
for(np = ptable.proc; np < &ptable.proc[NPROC]; np++){
if(np->parent != p)
continue;
+ acquire(&np->lock);
havekids = 1;
if(np->state == ZOMBIE){
// Found one.
@@ -330,19 +354,21 @@ wait(void)
np->name[0] = 0;
np->killed = 0;
np->state = UNUSED;
- release(&ptable.lock);
+ release(&np->lock);
+ release(&p->lock);
return pid;
}
+ release(&np->lock);
}
// No point waiting if we don't have any children.
if(!havekids || p->killed){
- release(&ptable.lock);
+ release(&p->lock);
return -1;
}
-
- // Wait for children to exit. (See wakeup1 call in proc_exit.)
- sleep(p, &ptable.lock); //DOC: wait-sleep
+
+ // Wait for children to exit. (See wakeup1 call in tellparent.)
+ sleep(p, &p->lock); //DOC: wait-sleep
}
}
@@ -366,13 +392,16 @@ scheduler(void)
intr_on();
// Loop over process table looking for process to run.
- acquire(&ptable.lock);
- for(p = ptable.proc; p < &ptable.proc[NPROC]; p++){
- if(p->state != RUNNABLE)
+ for(p = ptable.proc; p < &ptable.proc[NPROC]; p++) {
+ acquire(&p->lock);
+
+ if(p->state != RUNNABLE) {
+ release(&p->lock);
continue;
+ }
// Switch to chosen process. It is the process's job
- // to release ptable.lock and then reacquire it
+ // to release its lock and then reacquire it
// before jumping back to us.
c->proc = p;
p->state = RUNNING;
@@ -382,12 +411,17 @@ scheduler(void)
// 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) {
+ tellparent(p, p->parent);
+ }
+
}
- release(&ptable.lock);
}
}
-// Enter scheduler. Must hold only ptable.lock
+// Enter scheduler. Must hold only p->lock
// and have changed proc->state. Saves and restores
// intena because intena is a property of this
// kernel thread, not this CPU. It should
@@ -400,8 +434,8 @@ sched(void)
int intena;
struct proc *p = myproc();
- if(!holding(&ptable.lock))
- panic("sched ptable.lock");
+ if(!holding(&p->lock))
+ panic("sched p->lock");
if(mycpu()->noff != 1)
panic("sched locks");
if(p->state == RUNNING)
@@ -418,10 +452,10 @@ sched(void)
void
yield(void)
{
- acquire(&ptable.lock); //DOC: yieldlock
+ acquire(&myproc()->lock); //DOC: yieldlock
myproc()->state = RUNNABLE;
sched();
- release(&ptable.lock);
+ release(&myproc()->lock);
}
// A fork child's very first scheduling by scheduler()
@@ -431,8 +465,8 @@ forkret(void)
{
static int first = 1;
- // Still holding ptable.lock from scheduler.
- release(&ptable.lock);
+ // Still holding p->lock from scheduler.
+ release(&myproc()->lock);
if (first) {
// Some initialization functions must be run in the context
@@ -451,60 +485,69 @@ forkret(void)
void
sleep(void *chan, struct spinlock *lk)
{
- struct proc *p = myproc();
-
- if(p == 0)
+ if(myproc() == 0)
panic("sleep");
if(lk == 0)
panic("sleep without lk");
- // Must acquire ptable.lock in order to
+ // Must acquire p->lock in order to
// change p->state and then call sched.
- // Once we hold ptable.lock, we can be
+ // Once we hold p->lock, we can be
// guaranteed that we won't miss any wakeup
- // (wakeup runs with ptable.lock locked),
+ // (wakeup runs with p->lock locked),
// so it's okay to release lk.
- if(lk != &ptable.lock){ //DOC: sleeplock0
- acquire(&ptable.lock); //DOC: sleeplock1
+ if(lk != &myproc()->lock){ //DOC: sleeplock0
+ acquire(&myproc()->lock); //DOC: sleeplock1
release(lk);
}
// Go to sleep.
- p->chan = chan;
- p->state = SLEEPING;
+ myproc()->chan = chan;
+ myproc()->state = SLEEPING;
sched();
// Tidy up.
- p->chan = 0;
+ myproc()->chan = 0;
// Reacquire original lock.
- if(lk != &ptable.lock){ //DOC: sleeplock2
- release(&ptable.lock);
+ if(lk != &myproc()->lock){ //DOC: sleeplock2
+ release(&myproc()->lock);
acquire(lk);
}
}
//PAGEBREAK!
-// Wake up all processes sleeping on chan.
-// The ptable lock must be held.
+// Wake up all processes sleeping on chan,
+// where chan is a proc.
static void
-wakeup1(void *chan)
+wakeup1(struct proc *chan)
{
struct proc *p;
for(p = ptable.proc; p < &ptable.proc[NPROC]; p++)
- if(p->state == SLEEPING && p->chan == chan)
+ if(p == chan && p->state == SLEEPING && p->chan == chan) {
+ if(p->state != SLEEPING || p->chan != chan)
+ panic("wakeup1");
p->state = RUNNABLE;
+ }
}
-// Wake up all processes sleeping on chan.
+// Wake up all processes sleeping on chan. Never
+// called when holding a p->lock
void
wakeup(void *chan)
{
- acquire(&ptable.lock);
- wakeup1(chan);
- release(&ptable.lock);
+ struct proc *p;
+
+ for(p = ptable.proc; p < &ptable.proc[NPROC]; p++)
+ 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);
+ }
}
// Kill the process with the given pid.
@@ -515,18 +558,19 @@ kill(int pid)
{
struct proc *p;
- acquire(&ptable.lock);
for(p = ptable.proc; p < &ptable.proc[NPROC]; p++){
if(p->pid == pid){
+ acquire(&p->lock);
+ if(p->pid != pid)
+ panic("kill");
p->killed = 1;
// Wake process from sleep if necessary.
if(p->state == SLEEPING)
p->state = RUNNABLE;
- release(&ptable.lock);
+ release(&p->lock);
return 0;
}
}
- release(&ptable.lock);
return -1;
}
diff --git a/kernel/proc.h b/kernel/proc.h
index 278e4cd..0fa61ff 100644
--- a/kernel/proc.h
+++ b/kernel/proc.h
@@ -82,6 +82,7 @@ enum procstate { UNUSED, EMBRYO, SLEEPING, RUNNABLE, RUNNING, ZOMBIE };
// Per-process state
struct proc {
+ struct spinlock lock;
char *kstack; // Bottom of kernel stack for this process
uint64 sz; // Size of process memory (bytes)
pagetable_t pagetable; // Page table
diff --git a/kernel/sleeplock.c b/kernel/sleeplock.c
index b490370..81de585 100644
--- a/kernel/sleeplock.c
+++ b/kernel/sleeplock.c
@@ -5,8 +5,8 @@
#include "defs.h"
#include "param.h"
#include "memlayout.h"
-#include "proc.h"
#include "spinlock.h"
+#include "proc.h"
#include "sleeplock.h"
void
diff --git a/kernel/spinlock.c b/kernel/spinlock.c
index 5a44a46..c238091 100644
--- a/kernel/spinlock.c
+++ b/kernel/spinlock.c
@@ -20,7 +20,7 @@ initlock(struct spinlock *lk, char *name)
// Loops (spins) until the lock is acquired.
// Holding a lock for a long time may cause
// other CPUs to waste time spinning to acquire it.
-void
+void //__attribute__ ((noinline))
acquire(struct spinlock *lk)
{
push_off(); // disable interrupts to avoid deadlock.
@@ -44,11 +44,13 @@ acquire(struct spinlock *lk)
}
// Release the lock.
-void
+void //__attribute__ ((noinline))
release(struct spinlock *lk)
{
- if(!holding(lk))
+ if(!holding(lk)) {
+ printf("%p: !holding %s %p\n", mycpu(), lk->name, lk->cpu);
panic("release");
+ }
lk->cpu = 0;
diff --git a/kernel/syscall.c b/kernel/syscall.c
index ca34f2c..8e9d51c 100644
--- a/kernel/syscall.c
+++ b/kernel/syscall.c
@@ -2,6 +2,7 @@
#include "param.h"
#include "memlayout.h"
#include "riscv.h"
+#include "spinlock.h"
#include "proc.h"
#include "syscall.h"
#include "defs.h"
@@ -170,7 +171,9 @@ dosyscall(void)
num = p->tf->a7;
if(num > 0 && num < NELEM(syscalls) && syscalls[num]) {
+ //printf("%d: syscall %d\n", p->pid, num);
p->tf->a0 = syscalls[num]();
+ //printf("%d: syscall %d -> %d\n", p->pid, num, p->tf->a0);
} else {
printf("%d %s: unknown sys call %d\n",
p->pid, p->name, num);
diff --git a/kernel/sysfile.c b/kernel/sysfile.c
index 7788de3..33f37f2 100644
--- a/kernel/sysfile.c
+++ b/kernel/sysfile.c
@@ -9,9 +9,9 @@
#include "defs.h"
#include "param.h"
#include "stat.h"
+#include "spinlock.h"
#include "proc.h"
#include "fs.h"
-#include "spinlock.h"
#include "sleeplock.h"
#include "file.h"
#include "fcntl.h"
diff --git a/kernel/sysproc.c b/kernel/sysproc.c
index e57e045..65dde26 100644
--- a/kernel/sysproc.c
+++ b/kernel/sysproc.c
@@ -4,6 +4,7 @@
#include "date.h"
#include "param.h"
#include "memlayout.h"
+#include "spinlock.h"
#include "proc.h"
int
diff --git a/kernel/trap.c b/kernel/trap.c
index 586f123..6c0d04b 100644
--- a/kernel/trap.c
+++ b/kernel/trap.c
@@ -2,8 +2,8 @@
#include "param.h"
#include "memlayout.h"
#include "riscv.h"
-#include "proc.h"
#include "spinlock.h"
+#include "proc.h"
#include "defs.h"
struct spinlock tickslock;
diff --git a/kernel/uart.c b/kernel/uart.c
index 35fac1b..b8dd664 100644
--- a/kernel/uart.c
+++ b/kernel/uart.c
@@ -2,8 +2,8 @@
#include "param.h"
#include "memlayout.h"
#include "riscv.h"
-#include "proc.h"
#include "spinlock.h"
+#include "proc.h"
#include "defs.h"
//