use acquire/release to force order for pid=np->pid;np->state=RUNNING
for bug reported by symingz@gmail.com and cs1100254@cse.iitd.ernet.in
This commit is contained in:
		
							parent
							
								
									86188d9d49
								
							
						
					
					
						commit
						020c8e2384
					
				
					 3 changed files with 18 additions and 10 deletions
				
			
		
							
								
								
									
										16
									
								
								TRICKS
									
										
									
									
									
								
							
							
						
						
									
										16
									
								
								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. | ||||
|  |  | |||
							
								
								
									
										10
									
								
								proc.c
									
										
									
									
									
								
							
							
						
						
									
										10
									
								
								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"); | ||||
|   } | ||||
| } | ||||
| 
 | ||||
| 
 | ||||
|  |  | |||
							
								
								
									
										2
									
								
								proc.h
									
										
									
									
									
								
							
							
						
						
									
										2
									
								
								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
 | ||||
|  |  | |||
		Loading…
	
	Add table
		Add a link
		
	
		Reference in a new issue
	
	 Robert Morris
						Robert Morris