From owner-svn-src-all@FreeBSD.ORG Thu Mar 24 18:40:11 2011 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id C594E106564A; Thu, 24 Mar 2011 18:40:11 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from svn.freebsd.org (svn.freebsd.org [IPv6:2001:4f8:fff6::2c]) by mx1.freebsd.org (Postfix) with ESMTP id B3BDF8FC12; Thu, 24 Mar 2011 18:40:11 +0000 (UTC) Received: from svn.freebsd.org (localhost [127.0.0.1]) by svn.freebsd.org (8.14.3/8.14.3) with ESMTP id p2OIeBC5053908; Thu, 24 Mar 2011 18:40:11 GMT (envelope-from jhb@svn.freebsd.org) Received: (from jhb@localhost) by svn.freebsd.org (8.14.3/8.14.3/Submit) id p2OIeBDs053896; Thu, 24 Mar 2011 18:40:11 GMT (envelope-from jhb@svn.freebsd.org) Message-Id: <201103241840.p2OIeBDs053896@svn.freebsd.org> From: John Baldwin Date: Thu, 24 Mar 2011 18:40:11 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org X-SVN-Group: head MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cc: Subject: svn commit: r219968 - in head/sys: compat/linprocfs fs/nfsclient kern vm X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 24 Mar 2011 18:40:11 -0000 Author: jhb Date: Thu Mar 24 18:40:11 2011 New Revision: 219968 URL: http://svn.freebsd.org/changeset/base/219968 Log: 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 Modified: head/sys/compat/linprocfs/linprocfs.c head/sys/fs/nfsclient/nfs_clport.c head/sys/kern/kern_descrip.c head/sys/kern/kern_fork.c head/sys/kern/kern_proc.c head/sys/kern/kern_resource.c head/sys/kern/kern_thread.c head/sys/vm/vm_meter.c head/sys/vm/vm_pageout.c Modified: head/sys/compat/linprocfs/linprocfs.c ============================================================================== --- head/sys/compat/linprocfs/linprocfs.c Thu Mar 24 17:20:24 2011 (r219967) +++ head/sys/compat/linprocfs/linprocfs.c Thu Mar 24 18:40:11 2011 (r219968) @@ -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); Modified: head/sys/fs/nfsclient/nfs_clport.c ============================================================================== --- head/sys/fs/nfsclient/nfs_clport.c Thu Mar 24 17:20:24 2011 (r219967) +++ head/sys/fs/nfsclient/nfs_clport.c Thu Mar 24 18:40:11 2011 (r219968) @@ -1102,11 +1102,11 @@ pfind_locked(pid_t pid) LIST_FOREACH(p, PIDHASH(pid), p_hash) if (p->p_pid == pid) { + PROC_LOCK(p); if (p->p_state == PRS_NEW) { + PROC_UNLOCK(p); p = NULL; - break; } - PROC_LOCK(p); break; } return (p); Modified: head/sys/kern/kern_descrip.c ============================================================================== --- head/sys/kern/kern_descrip.c Thu Mar 24 17:20:24 2011 (r219967) +++ head/sys/kern/kern_descrip.c Thu Mar 24 18:40:11 2011 (r219968) @@ -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; Modified: head/sys/kern/kern_fork.c ============================================================================== --- head/sys/kern/kern_fork.c Thu Mar 24 17:20:24 2011 (r219967) +++ head/sys/kern/kern_fork.c Thu Mar 24 18:40:11 2011 (r219968) @@ -630,12 +630,13 @@ do_fork(struct thread *td, int flags, st /* * 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, st * 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, st */ 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 Modified: head/sys/kern/kern_proc.c ============================================================================== --- head/sys/kern/kern_proc.c Thu Mar 24 17:20:24 2011 (r219967) +++ head/sys/kern/kern_proc.c Thu Mar 24 18:40:11 2011 (r219968) @@ -291,11 +291,11 @@ pfind(pid) sx_slock(&allproc_lock); LIST_FOREACH(p, PIDHASH(pid), p_hash) if (p->p_pid == pid) { + PROC_LOCK(p); if (p->p_state == PRS_NEW) { + PROC_UNLOCK(p); p = NULL; - break; } - PROC_LOCK(p); break; } sx_sunlock(&allproc_lock); @@ -756,7 +756,6 @@ fill_kinfo_proc_only(struct proc *p, str 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, str 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")); /* Modified: head/sys/kern/kern_resource.c ============================================================================== --- head/sys/kern/kern_resource.c Thu Mar 24 17:20:24 2011 (r219967) +++ head/sys/kern/kern_resource.c Thu Mar 24 18:40:11 2011 (r219968) @@ -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; Modified: head/sys/kern/kern_thread.c ============================================================================== --- head/sys/kern/kern_thread.c Thu Mar 24 17:20:24 2011 (r219967) +++ head/sys/kern/kern_thread.c Thu Mar 24 18:40:11 2011 (r219968) @@ -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++; Modified: head/sys/vm/vm_meter.c ============================================================================== --- head/sys/vm/vm_meter.c Thu Mar 24 17:20:24 2011 (r219967) +++ head/sys/vm/vm_meter.c Thu Mar 24 18:40:11 2011 (r219968) @@ -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) { Modified: head/sys/vm/vm_pageout.c ============================================================================== --- head/sys/vm/vm_pageout.c Thu Mar 24 17:20:24 2011 (r219967) +++ head/sys/vm/vm_pageout.c Thu Mar 24 18:40:11 2011 (r219968) @@ -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; }