From 00e571155ce6f836a7f78e7a725c1b3de7868b5a Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Sun, 12 Jul 2009 18:33:37 -0700 Subject: [PATCH] more doc tweaks --- TRICKS | 26 +++++++++++++------------- pipe.c | 40 ++++++++++++++++++++-------------------- proc.c | 38 ++++++++++++++++++-------------------- web/l-coordination.html | 2 +- 4 files changed, 52 insertions(+), 54 deletions(-) diff --git a/TRICKS b/TRICKS index b538834..0338279 100644 --- a/TRICKS +++ b/TRICKS @@ -4,6 +4,9 @@ might be worth pointing out in a longer explanation or in class. --- +[2009/07/12: No longer relevant; forkret1 changed +and this is now cleaner.] + forkret1 in trapasm.S is called with a tf argument. In order to use it, forkret1 copies the tf pointer into %esp and then jumps to trapret, which pops the @@ -45,21 +48,21 @@ always. There is a (harmless) race in pushcli, which does - eflags = read_eflags(); + eflags = readeflags(); cli(); - if(cpus[cpu()].ncli++ == 0) - cpus[cpu()].intena = eflags & FL_IF; + if(c->ncli++ == 0) + c->intena = eflags & FL_IF; Consider a bottom-level pushcli. If interrupts are disabled already, then the right thing happens: read_eflags finds that FL_IF is not set, -and intena = 1. If interrupts are enabled, then +and intena = 0. If interrupts are enabled, then it is less clear that the right thing happens: -the read_eflags can execute, then the process +the readeflags can execute, then the process can get preempted and rescheduled on another cpu, and then once it starts running, perhaps with interrupts disabled (can happen since the scheduler -only disables interrupts once per scheduling loop, +only enables interrupts once per scheduling loop, not every time it schedules a process), it will incorrectly record that interrupts *were* enabled. This doesn't matter, because if it was safe to be @@ -112,17 +115,13 @@ processors will need it. --- -The code in sys_fork needs to read np->pid before +The code in fork needs to read np->pid before setting np->state to RUNNABLE. int - sys_fork(void) + fork(void) { - int pid; - struct proc *np; - - if((np = copyproc(cp)) == 0) - return -1; + ... pid = np->pid; np->state = RUNNABLE; return pid; @@ -134,3 +133,4 @@ get reused for a different process (with a new pid), all before the return statement. So it's not safe to just do "return np->pid;". +This works because proc.h marks the pid as volatile. diff --git a/pipe.c b/pipe.c index 7f18e98..3032775 100644 --- a/pipe.c +++ b/pipe.c @@ -9,12 +9,12 @@ #define PIPESIZE 512 struct pipe { - int readopen; // read fd is still open - int writeopen; // write fd is still open - uint writep; // next index to write - uint readp; // next index to read struct spinlock lock; char data[PIPESIZE]; + uint nread; // number of bytes read + uint nwrite; // number of bytes written + int readopen; // read fd is still open + int writeopen; // write fd is still open }; int @@ -30,8 +30,8 @@ pipealloc(struct file **f0, struct file **f1) goto bad; p->readopen = 1; p->writeopen = 1; - p->writep = 0; - p->readp = 0; + p->nwrite = 0; + p->nread = 0; initlock(&p->lock, "pipe"); (*f0)->type = FD_PIPE; (*f0)->readable = 1; @@ -60,10 +60,10 @@ pipeclose(struct pipe *p, int writable) acquire(&p->lock); if(writable){ p->writeopen = 0; - wakeup(&p->readp); + wakeup(&p->nread); } else { p->readopen = 0; - wakeup(&p->writep); + wakeup(&p->nwrite); } if(p->readopen == 0 && p->writeopen == 0) { release(&p->lock); @@ -80,19 +80,19 @@ pipewrite(struct pipe *p, char *addr, int n) acquire(&p->lock); for(i = 0; i < n; i++){ - while(p->writep == p->readp + PIPESIZE) { + while(p->nwrite == p->nread + PIPESIZE) { //DOC: pipewrite-full if(p->readopen == 0 || cp->killed){ release(&p->lock); return -1; } - wakeup(&p->readp); - sleep(&p->writep, &p->lock); + wakeup(&p->nread); + sleep(&p->nwrite, &p->lock); //DOC: pipewrite-sleep } - p->data[p->writep++ % PIPESIZE] = addr[i]; + p->data[p->nwrite++ % PIPESIZE] = addr[i]; } - wakeup(&p->readp); + wakeup(&p->nread); //DOC: pipewrite-wakeup1 release(&p->lock); - return i; + return n; } int @@ -101,19 +101,19 @@ piperead(struct pipe *p, char *addr, int n) int i; acquire(&p->lock); - while(p->readp == p->writep && p->writeopen){ + while(p->nread == p->nwrite && p->writeopen){ //DOC: pipe-empty if(cp->killed){ release(&p->lock); return -1; } - sleep(&p->readp, &p->lock); + sleep(&p->nread, &p->lock); //DOC: piperead-sleep } - for(i = 0; i < n; i++){ - if(p->readp == p->writep) + for(i = 0; i < n; i++){ //DOC: piperead-copy + if(p->nread == p->nwrite) break; - addr[i] = p->data[p->readp++ % PIPESIZE]; + addr[i] = p->data[p->nread++ % PIPESIZE]; } - wakeup(&p->writep); + wakeup(&p->nwrite); //DOC: piperead-wakeup release(&p->lock); return i; } diff --git a/proc.c b/proc.c index aa6c4ec..4333512 100644 --- a/proc.c +++ b/proc.c @@ -290,8 +290,8 @@ sleep(void *chan, struct spinlock *lk) // guaranteed that we won't miss any wakeup // (wakeup runs with ptable.lock locked), // so it's okay to release lk. - if(lk != &ptable.lock){ - acquire(&ptable.lock); + if(lk != &ptable.lock){ //DOC: sleeplock0 + acquire(&ptable.lock); //DOC: sleeplock1 release(lk); } @@ -304,7 +304,7 @@ sleep(void *chan, struct spinlock *lk) cp->chan = 0; // Reacquire original lock. - if(lk != &ptable.lock){ + if(lk != &ptable.lock){ //DOC: sleeplock2 release(&ptable.lock); acquire(lk); } @@ -393,7 +393,6 @@ exit(void) } // Jump into the scheduler, never to return. - cp->killed = 0; cp->state = ZOMBIE; sched(); panic("zombie exit"); @@ -412,22 +411,21 @@ wait(void) // Scan through table looking for zombie children. havekids = 0; for(p = ptable.proc; p < &ptable.proc[NPROC]; p++){ - if(p->state == UNUSED) + if(p->parent != cp) continue; - if(p->parent == cp){ - havekids = 1; - if(p->state == ZOMBIE){ - // Found one. - kfree(p->mem, p->sz); - kfree(p->kstack, KSTACKSIZE); - pid = p->pid; - p->state = UNUSED; - p->pid = 0; - p->parent = 0; - p->name[0] = 0; - release(&ptable.lock); - return pid; - } + havekids = 1; + if(p->state == ZOMBIE){ + // Found one. + pid = p->pid; + kfree(p->mem, p->sz); + kfree(p->kstack, KSTACKSIZE); + p->state = UNUSED; + p->pid = 0; + p->parent = 0; + p->name[0] = 0; + p->killed = 0; + release(&ptable.lock); + return pid; } } @@ -438,7 +436,7 @@ wait(void) } // Wait for children to exit. (See wakeup1 call in proc_exit.) - sleep(cp, &ptable.lock); + sleep(cp, &ptable.lock); //DOC: wait-sleep } } diff --git a/web/l-coordination.html b/web/l-coordination.html index b2f9f0d..79b578b 100644 --- a/web/l-coordination.html +++ b/web/l-coordination.html @@ -47,7 +47,7 @@

Sleep and wakeup - usage

Let's consider implementing a producer/consumer queue -(like a pipe) that can be used to hold a single non-null char pointer: +(like a pipe) that can be used to hold a single non-null pointer: 
 struct pcq {