remove p->lock from addrun/delrun; caller must acquire

8cb44655 · Robert Morris · 5f3d853d · 8cb44655 · 8cb44655 · 8cb44655
--- a/Makefile
+++ b/Makefile
@@ -247,6 +247,9 @@ qemu-nox-gdb: fs.img xv6.img .gdbinit
 	@echo "*** Now run 'gdb'." 1>&2
 	$(QEMU) -nographic $(QEMUOPTS) -S $(QEMUGDB)
+mtrace-nox-gdb: fs.img xv6.img mscan.syms mscan.kern .gdbinit
+	$(MTRACE) -nographic $(QEMUOPTS) $(MTRACEOPTS) -S -gdb tcp::$(GDBPORT)
 # CUT HERE
 # prepare dist for students
 # after running make dist, probably want to

--- a/condvar.c
+++ b/condvar.c
@@ -55,12 +55,14 @@ cv_wakeup(struct condvar *cv)
  acquire(&cv->lock);
  while(cv->waiters) {
    struct proc *p = cv->waiters;
+    acquire(&p->lock);
    if(p->state != SLEEPING || p->oncv != cv)
      panic("cv_wakeup");
    struct proc *nxt = p->cv_next;
    p->cv_next = 0;
    p->oncv = 0;
    addrun(p);
+    release(&p->lock);
    cv->waiters = nxt;
  }
  release(&cv->lock);

--- a/file.c
+++ b/file.c
@@ -5,6 +5,7 @@
 #include "condvar.h"
 #include "fs.h"
 #include "file.h"
+#include "stat.h"
 struct devsw devsw[NDEV];
 struct {
@@ -117,6 +118,8 @@ filewrite(struct file *f, char *addr, int n)
    return pipewrite(f->pipe, addr, n);
  if(f->type == FD_INODE){
    ilock(f->ip);
+    if(f->ip->type == 0 || f->ip->type == T_DIR)
+      panic("filewrite but 0 or T_DIR");
    if((r = writei(f->ip, addr, f->off, n)) > 0)
      f->off += r;
    iunlock(f->ip);

--- a/fs.c
+++ b/fs.c
@@ -585,9 +585,6 @@ namex(char *path, int nameiparent, char *name)
    ip = idup(proc->cwd);
  while((path = skipelem(path, name)) != 0){
-    // XXX do we need to ilock(ip)?
-    // hopefully not, would be nice to have
-    // lock-free namecache hits.
    next = 0;
    if(nameiparent == 0)
      next = nc_lookup(ip, name);

--- a/namecache.c
+++ b/namecache.c
@@ -3,10 +3,12 @@
 //
 // to do:
 //   use ns.c namespaces
-//   invalidation (for rename, unlink)
+//   does directory inum need to be locked around ns_lookup?
-//   does directory inum need to be locked?
 //   need a lock to make table lookup and iget atomic?
+//     unlink/lookup race?
+//     better: inode generation #
 //   insert when file created, not just looked up
+//   eviction
 //
 #include "types.h"
@@ -32,7 +34,7 @@ struct nce {
  char name[DIRSIZ];
  uint cinum; // child inumber
 };
-#define NCE 32
+#define NCE 128
 struct nce nce[NCE];
 void
@@ -85,7 +87,8 @@ nc_insert(struct inode *dir, char *name, struct inode *ip)
    return;
  }
-  for(int i = 0; i < NCE; i++){
+  int i;
+  for(i = 0; i < NCE; i++){
    e = &nce[i];
    if(e->valid == 0){
      e->valid = 1;
@@ -96,6 +99,8 @@ nc_insert(struct inode *dir, char *name, struct inode *ip)
      break;
    }
  }
+  if(i >= NCE)
+    cprintf("namecache full\n");
  release(&nc_lock);
 }

--- a/proc.c
+++ b/proc.c
@@ -98,15 +98,22 @@ addrun1(struct runq *rq, struct proc *p)
  STAILQ_FOREACH(q, &rq->runq, run_next)
    if (q == p)
      panic("addrun1: already on queue");
-  acquire(&p->lock);
  p->state = RUNNABLE;
  STAILQ_INSERT_TAIL(&rq->runq, p, run_next);
-  release(&p->lock);
 }
+// Mark a process RUNNABLE and add it to the runq
+// of its cpu. Caller must hold p->lock so that
+// some other core doesn't start running the
+// process before the caller has finished setting
+// the process up, and to cope with racing callers
+// e.g. two wakeups on same process. and to
+// allow atomic addrun(); sched();
 void
 addrun(struct proc *p)
 {
+  if(!holding(&p->lock))
+    panic("addrun no p->lock");
  acquire(&runqs[p->cpuid].lock);
  //  cprintf("%d: addrun %d\n", cpunum(), p->pid);
  addrun1(&runqs[p->cpuid], p);
@@ -119,9 +126,7 @@ delrun1(struct runq *rq, struct proc *p)
  struct proc *q, *nq;
  STAILQ_FOREACH_SAFE(q, &rq->runq, run_next, nq) {
    if (q == p) {
-      acquire(&p->lock);
      STAILQ_REMOVE(&rq->runq, q, proc, run_next);
-      release(&p->lock);
      return;
    }
  }
@@ -131,6 +136,8 @@ delrun1(struct runq *rq, struct proc *p)
 void
 delrun(struct proc *p)
 {
+  if(!holding(&p->lock))
+    panic("delrun no p->lock");
  acquire(&runq->lock);
  // cprintf("%d: delrun %d\n", cpunum(), p->pid);
  delrun1(runq, p);
@@ -167,7 +174,9 @@ userinit(void)
  safestrcpy(p->name, "initcode", sizeof(p->name));
  p->cwd = namei("/");
+  acquire(&p->lock);
  addrun(p);
+  release(&p->lock);
 }
 // Grow/shrink current process's memory by n bytes.
@@ -294,14 +303,16 @@ fork(int flags)
    if(proc->ofile[i])
      np->ofile[i] = filedup(proc->ofile[i]);
  np->cwd = idup(proc->cwd);
  pid = np->pid;
-  addrun(np);
  safestrcpy(np->name, proc->name, sizeof(proc->name));
  acquire(&proc->lock);
  SLIST_INSERT_HEAD(&proc->childq, np, child_next);
  release(&proc->lock);
+  acquire(&np->lock);
+  addrun(np);
+  release(&np->lock);
  //  cprintf("%d: fork done (pid %d)\n", proc->pid, pid);
  return pid;
 }
@@ -420,11 +431,10 @@ migrate(void)
    if (idle[c]) {    // OK if there is a race
      // cprintf("migrate to %d\n", c);
      p = proc;
+      acquire(&p->lock);
      p->curcycles = 0;
      p->cpuid = c;
      addrun(p);
-      acquire(&p->lock);
-      p->state = RUNNABLE;
      sched();
      release(&proc->lock);
      return;
@@ -443,6 +453,7 @@ steal(void)
      continue;
    acquire(&runqs[c].lock);
    STAILQ_FOREACH(p, &runqs[c].runq, run_next) {
+      acquire(&p->lock);
      if (p->state != RUNNABLE)
        panic("non-runnable proc on runq");
      if (p->curcycles > MINCYCTHRESH) {
@@ -452,8 +463,10 @@ steal(void)
 	p->curcycles = 0;
 	p->cpuid = cpu->id;
 	addrun(p);
+        release(&p->lock);
 	return 1;
      }
+      release(&p->lock);
    }
    release(&runqs[c].lock);
  }
@@ -557,9 +570,8 @@ sched(void)
 void
 yield(void)
 {
-  addrun(proc);
  acquire(&proc->lock);  //DOC: yieldlock
+  addrun(proc);
  sched();
  release(&proc->lock);
 }

--- a/usertests.c
+++ b/usertests.c
@@ -5,6 +5,7 @@
 #include "fcntl.h"
 #include "syscall.h"
 #include "traps.h"
+#include "xv6-mtrace.h"
 char buf[2048];
 char name[3];
@@ -1464,17 +1465,109 @@ bigargtest(void)
  wait();
 }
+void
+uox(char *name, char *data)
+{
+  int fd = open(name, O_CREATE|O_RDWR);
+  if(fd < 0){
+    printf(stdout, "creat %s failed\n", name);
+    exit();
+  }
+  if(write(fd, "xx", 2) != 2){
+    printf(stdout, "write failed\n");
+    exit();
+  }
+  close(fd);
+}
+// test concurrent unlink / open.
+void
+unopentest(void)
+{
+  printf(stdout, "concurrent unlink/open\n");
+  int pid = fork(0);
+  if(pid == 0){
+    while(1){
+      for(int i = 0; i < 1; i++){
+        char name[32];
+        name[0] = 'f';
+        name[1] = 'A' + i;
+        name[2] = '\0';
+        int fd = open(name, O_RDWR);
+        if(fd >= 0)
+          close(fd);
+        fd = open(name, O_RDWR);
+        if(fd >= 0){
+          if(write(fd, "y", 1) != 1){
+            printf(stdout, "write %s failed\n", name);
+            exit();
+          }
+          close(fd);
+        }
+      }
+    }
+  }
+  for(int iters = 0; iters < 1000; iters++){
+    for(int i = 0; i < 1; i++){
+      char name[32];
+      name[0] = 'f';
+      name[1] = 'A' + i;
+      name[2] = '\0';
+      uox(name, "xxx");
+      if(unlink(name) < 0){
+        printf(stdout, "unlink %s failed\n", name);
+        exit();
+      }
+      // reallocate that inode
+      name[0] = 'g';
+      if(mkdir(name) != 0){
+        printf(stdout, "mkdir %s failed\n", name);
+        exit();
+      }
+    }
+    for(int i = 0; i < 10; i++){
+      char name[32];
+      name[0] = 'f';
+      name[1] = 'A' + i;
+      name[2] = '\0';
+      unlink(name);
+      name[0] = 'g';
+      unlink(name);
+    }
+  }
+  kill(pid);
+  wait();
+  printf(stdout, "concurrent unlink/open ok\n");
+}
+// for mtrace-magic.h
+char*
+strncpy(char *s, const char *t, int n)
+{
+  int tlen = strlen((char *)t);
+  memmove(s, (char *)t, n > tlen ? tlen : n);
+  if (n > tlen)
+    s[tlen] = 0;
+  return s;
+}
 int
 main(int argc, char *argv[])
 {
  printf(1, "usertests starting\n");
+  mtrace_enable_set(1, "xv6-forktest");
  if(open("usertests.ran", 0) >= 0){
    printf(1, "already ran user tests -- rebuild fs.img\n");
    exit();
  }
  close(open("usertests.ran", O_CREATE));
+  // unopentest();
  bigargtest();
  bsstest();
  sbrktest();