diff --git a/TRICKS b/TRICKS index 0338279..8d1439f 100644 --- a/TRICKS +++ b/TRICKS @@ -116,21 +116,25 @@ processors will need it. --- The code in fork needs to read np->pid before -setting np->state to RUNNABLE. +setting np->state to RUNNABLE. The following +is not a correct way to do this: int fork(void) { ... - pid = np->pid; np->state = RUNNABLE; - return pid; + return np->pid; // oops } After setting np->state to RUNNABLE, some other CPU might run the process, it might exit, and then it might 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;". +before the return statement. So it's not safe to just +"return np->pid". Even saving a copy of np->pid before +setting np->state isn't safe, since the compiler is +allowed to re-order statements. -This works because proc.h marks the pid as volatile. +The real code saves a copy of np->pid, then acquires a lock +around the write to np->state. The acquire() prevents the +compiler from re-ordering. diff --git a/proc.c b/proc.c index bcdbfea..e19539c 100644 --- a/proc.c +++ b/proc.c @@ -153,10 +153,16 @@ fork(void) if(proc->ofile[i]) np->ofile[i] = filedup(proc->ofile[i]); np->cwd = idup(proc->cwd); + + safestrcpy(np->name, proc->name, sizeof(proc->name)); pid = np->pid; + + // lock to force the compiler to emit the np->state write last. + acquire(&ptable.lock); np->state = RUNNABLE; - safestrcpy(np->name, proc->name, sizeof(proc->name)); + release(&ptable.lock); + return pid; } @@ -455,5 +461,3 @@ procdump(void) cprintf("\n"); } } - - diff --git a/proc.h b/proc.h index 6561ad3..3b9c3ac 100644 --- a/proc.h +++ b/proc.h @@ -57,7 +57,7 @@ struct proc { pde_t* pgdir; // Page table char *kstack; // Bottom of kernel stack for this process enum procstate state; // Process state - volatile int pid; // Process ID + int pid; // Process ID struct proc *parent; // Parent process struct trapframe *tf; // Trap frame for current syscall struct context *context; // swtch() here to run process