From 8e6fa660f2c1f211fc86935cd1a7860e817af057 Mon Sep 17 00:00:00 2001 From: John Baldwin Date: Thu, 24 Mar 2011 18:40:11 +0000 Subject: [PATCH] Fix some locking nits with the p_state field of struct proc: - Hold the proc lock while changing the state from PRS_NEW to PRS_NORMAL in fork to honor the locking requirements. While here, expand the scope of the PROC_LOCK() on the new process (p2) to avoid some LORs. Previously the code was locking the new child process (p2) after it had locked the parent process (p1). However, when locking two processes, the safe order is to lock the child first, then the parent. - Fix various places that were checking p_state against PRS_NEW without having the process locked to use PROC_LOCK(). Every place was already locking the process, just after the PRS_NEW check. - Remove or reduce the use of PROC_SLOCK() for places that were checking p_state against PRS_NEW. The PROC_LOCK() alone is sufficient for reading the current state. - Reorder fill_kinfo_proc() slightly so it only acquires PROC_SLOCK() once. MFC after: 1 week --- sys/compat/linprocfs/linprocfs.c | 2 -- sys/fs/nfsclient/nfs_clport.c | 8 ++++---- sys/kern/kern_descrip.c | 6 ++++-- sys/kern/kern_fork.c | 11 ++++------- sys/kern/kern_proc.c | 20 ++++++++------------ sys/kern/kern_resource.c | 6 ++---- sys/kern/kern_thread.c | 4 ++-- sys/vm/vm_meter.c | 3 --- sys/vm/vm_pageout.c | 10 ++++------ 9 files changed, 28 insertions(+), 42 deletions(-) diff --git a/sys/compat/linprocfs/linprocfs.c b/sys/compat/linprocfs/linprocfs.c index 3ff9443eca2..b3d4e9ca4d6 100644 --- a/sys/compat/linprocfs/linprocfs.c +++ b/sys/compat/linprocfs/linprocfs.c @@ -740,7 +740,6 @@ linprocfs_doprocstatus(PFS_FILL_ARGS) if (P_SHOULDSTOP(p)) { state = "T (stopped)"; } else { - PROC_SLOCK(p); switch(p->p_state) { case PRS_NEW: state = "I (idle)"; @@ -770,7 +769,6 @@ linprocfs_doprocstatus(PFS_FILL_ARGS) state = "? (unknown)"; break; } - PROC_SUNLOCK(p); } fill_kinfo_proc(p, &kp); diff --git a/sys/fs/nfsclient/nfs_clport.c b/sys/fs/nfsclient/nfs_clport.c index 595c90876cd..10e1b8eb904 100644 --- a/sys/fs/nfsclient/nfs_clport.c +++ b/sys/fs/nfsclient/nfs_clport.c @@ -1102,11 +1102,11 @@ pfind_locked(pid_t pid) LIST_FOREACH(p, PIDHASH(pid), p_hash) if (p->p_pid == pid) { - if (p->p_state == PRS_NEW) { - p = NULL; - break; - } PROC_LOCK(p); + if (p->p_state == PRS_NEW) { + PROC_UNLOCK(p); + p = NULL; + } break; } return (p); diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c index bca83b56171..a9793683d09 100644 --- a/sys/kern/kern_descrip.c +++ b/sys/kern/kern_descrip.c @@ -2634,9 +2634,11 @@ sysctl_kern_file(SYSCTL_HANDLER_ARGS) xf.xf_size = sizeof(xf); sx_slock(&allproc_lock); FOREACH_PROC_IN_SYSTEM(p) { - if (p->p_state == PRS_NEW) - continue; PROC_LOCK(p); + if (p->p_state == PRS_NEW) { + PROC_UNLOCK(p); + continue; + } if (p_cansee(req->td, p) != 0) { PROC_UNLOCK(p); continue; diff --git a/sys/kern/kern_fork.c b/sys/kern/kern_fork.c index a29dc17daa0..ebd4e6d08cd 100644 --- a/sys/kern/kern_fork.c +++ b/sys/kern/kern_fork.c @@ -630,12 +630,13 @@ do_fork(struct thread *td, int flags, struct proc *p2, struct thread *td2, /* * Set the child start time and mark the process as being complete. */ + PROC_LOCK(p2); + PROC_LOCK(p1); microuptime(&p2->p_stats->p_start); PROC_SLOCK(p2); p2->p_state = PRS_NORMAL; PROC_SUNLOCK(p2); - PROC_LOCK(p1); #ifdef KDTRACE_HOOKS /* * Tell the DTrace fasttrap provider about the new process @@ -643,11 +644,8 @@ do_fork(struct thread *td, int flags, struct proc *p2, struct thread *td2, * p_state is PRS_NORMAL since the fasttrap module will use pfind() * later on. */ - if (dtrace_fasttrap_fork) { - PROC_LOCK(p2); + if (dtrace_fasttrap_fork) dtrace_fasttrap_fork(p1, p2); - PROC_UNLOCK(p2); - } #endif if ((p1->p_flag & (P_TRACED | P_FOLLOWFORK)) == (P_TRACED | P_FOLLOWFORK)) { @@ -660,12 +658,11 @@ do_fork(struct thread *td, int flags, struct proc *p2, struct thread *td2, */ td->td_dbgflags |= TDB_FORK; td->td_dbg_forked = p2->p_pid; - PROC_LOCK(p2); td2->td_dbgflags |= TDB_STOPATFORK; _PHOLD(p2); p2_held = 1; - PROC_UNLOCK(p2); } + PROC_UNLOCK(p2); if ((flags & RFSTOPPED) == 0) { /* * If RFSTOPPED not requested, make child runnable and diff --git a/sys/kern/kern_proc.c b/sys/kern/kern_proc.c index e02b5b285cd..34cc57071d4 100644 --- a/sys/kern/kern_proc.c +++ b/sys/kern/kern_proc.c @@ -291,11 +291,11 @@ pfind(pid) sx_slock(&allproc_lock); LIST_FOREACH(p, PIDHASH(pid), p_hash) if (p->p_pid == pid) { - if (p->p_state == PRS_NEW) { - p = NULL; - break; - } PROC_LOCK(p); + if (p->p_state == PRS_NEW) { + PROC_UNLOCK(p); + p = NULL; + } break; } sx_sunlock(&allproc_lock); @@ -756,7 +756,6 @@ fill_kinfo_proc_only(struct proc *p, struct kinfo_proc *kp) kp->ki_sigcatch = ps->ps_sigcatch; mtx_unlock(&ps->ps_mtx); } - PROC_SLOCK(p); if (p->p_state != PRS_NEW && p->p_state != PRS_ZOMBIE && p->p_vmspace != NULL) { @@ -782,12 +781,11 @@ fill_kinfo_proc_only(struct proc *p, struct kinfo_proc *kp) kp->ki_swtime = (ticks - p->p_swtick) / hz; kp->ki_pid = p->p_pid; kp->ki_nice = p->p_nice; - rufetch(p, &kp->ki_rusage); - kp->ki_runtime = cputick2usec(p->p_rux.rux_runtime); - PROC_SUNLOCK(p); kp->ki_start = p->p_stats->p_start; timevaladd(&kp->ki_start, &boottime); PROC_SLOCK(p); + rufetch(p, &kp->ki_rusage); + kp->ki_runtime = cputick2usec(p->p_rux.rux_runtime); calcru(p, &kp->ki_rusage.ru_utime, &kp->ki_rusage.ru_stime); PROC_SUNLOCK(p); calccru(p, &kp->ki_childutime, &kp->ki_childstime); @@ -1213,13 +1211,11 @@ sysctl_kern_proc(SYSCTL_HANDLER_ARGS) /* * Skip embryonic processes. */ - PROC_SLOCK(p); + PROC_LOCK(p); if (p->p_state == PRS_NEW) { - PROC_SUNLOCK(p); + PROC_UNLOCK(p); continue; } - PROC_SUNLOCK(p); - PROC_LOCK(p); KASSERT(p->p_ucred != NULL, ("process credential is NULL for non-NEW proc")); /* diff --git a/sys/kern/kern_resource.c b/sys/kern/kern_resource.c index 5bc3da59b79..66b6e2db432 100644 --- a/sys/kern/kern_resource.c +++ b/sys/kern/kern_resource.c @@ -142,11 +142,9 @@ getpriority(td, uap) uap->who = td->td_ucred->cr_uid; sx_slock(&allproc_lock); FOREACH_PROC_IN_SYSTEM(p) { - /* Do not bother to check PRS_NEW processes */ - if (p->p_state == PRS_NEW) - continue; PROC_LOCK(p); - if (p_cansee(td, p) == 0 && + if (p->p_state == PRS_NORMAL && + p_cansee(td, p) == 0 && p->p_ucred->cr_uid == uap->who) { if (p->p_nice < low) low = p->p_nice; diff --git a/sys/kern/kern_thread.c b/sys/kern/kern_thread.c index 5ef98642914..29bdaa20e03 100644 --- a/sys/kern/kern_thread.c +++ b/sys/kern/kern_thread.c @@ -981,7 +981,9 @@ tdfind(lwpid_t tid, pid_t pid) td = NULL; break; } + PROC_LOCK(td->td_proc); if (td->td_proc->p_state == PRS_NEW) { + PROC_UNLOCK(td->td_proc); td = NULL; break; } @@ -990,12 +992,10 @@ tdfind(lwpid_t tid, pid_t pid) LIST_REMOVE(td, td_hash); LIST_INSERT_HEAD(TIDHASH(td->td_tid), td, td_hash); - PROC_LOCK(td->td_proc); rw_wunlock(&tidhash_lock); return (td); } } - PROC_LOCK(td->td_proc); break; } run++; diff --git a/sys/vm/vm_meter.c b/sys/vm/vm_meter.c index e0f7e32c70b..f1fd4fe33fe 100644 --- a/sys/vm/vm_meter.c +++ b/sys/vm/vm_meter.c @@ -130,15 +130,12 @@ vmtotal(SYSCTL_HANDLER_ARGS) if (p->p_flag & P_SYSTEM) continue; PROC_LOCK(p); - PROC_SLOCK(p); switch (p->p_state) { case PRS_NEW: - PROC_SUNLOCK(p); PROC_UNLOCK(p); continue; break; default: - PROC_SUNLOCK(p); FOREACH_THREAD_IN_PROC(p, td) { thread_lock(td); switch (td->td_state) { diff --git a/sys/vm/vm_pageout.c b/sys/vm/vm_pageout.c index e64a5ecf7fd..a780f39cd88 100644 --- a/sys/vm/vm_pageout.c +++ b/sys/vm/vm_pageout.c @@ -1281,14 +1281,13 @@ vm_pageout_oom(int shortage) FOREACH_PROC_IN_SYSTEM(p) { int breakout; - if (p->p_state != PRS_NORMAL) - continue; if (PROC_TRYLOCK(p) == 0) continue; /* * If this is a system, protected or killed process, skip it. */ - if ((p->p_flag & (P_INEXEC | P_PROTECTED | P_SYSTEM)) || + if (p->p_state != PRS_NORMAL || + (p->p_flag & (P_INEXEC | P_PROTECTED | P_SYSTEM)) || (p->p_pid == 1) || P_KILLED(p) || ((p->p_pid < 48) && (swap_pager_avail != 0))) { PROC_UNLOCK(p); @@ -1651,14 +1650,13 @@ vm_daemon() FOREACH_PROC_IN_SYSTEM(p) { vm_pindex_t limit, size; - if (p->p_state != PRS_NORMAL) - continue; /* * if this is a system process or if we have already * looked at this process, skip it. */ PROC_LOCK(p); - if (p->p_flag & (P_INEXEC | P_SYSTEM | P_WEXIT)) { + if (p->p_state != PRS_NORMAL || + p->p_flag & (P_INEXEC | P_SYSTEM | P_WEXIT)) { PROC_UNLOCK(p); continue; }