From owner-cvs-all Sat Feb 8 4:53:56 2003 Delivered-To: cvs-all@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id C64B037B401; Sat, 8 Feb 2003 04:53:50 -0800 (PST) Received: from mailman.zeta.org.au (mailman.zeta.org.au [203.26.10.16]) by mx1.FreeBSD.org (Postfix) with ESMTP id B751C43F3F; Sat, 8 Feb 2003 04:53:48 -0800 (PST) (envelope-from bde@zeta.org.au) Received: from katana.zip.com.au (katana.zip.com.au [61.8.7.246]) by mailman.zeta.org.au (8.9.3/8.8.7) with ESMTP id XAA03499; Sat, 8 Feb 2003 23:53:30 +1100 Date: Sat, 8 Feb 2003 23:53:35 +1100 (EST) From: Bruce Evans X-X-Sender: bde@gamplex.bde.org To: Tim Robbins Cc: Julian Elischer , , , , Subject: Re: cvs commit: src/sys/sys proc.h src/sys/kern kern_clock.c kern_exit.c subr_prof.c In-Reply-To: <20030208182605.A43083@dilbert.robbins.dropbear.id.au> Message-ID: <20030208220713.O17322-100000@gamplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-cvs-all@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG On Sat, 8 Feb 2003, Tim Robbins wrote: > On Fri, Feb 07, 2003 at 06:58:16PM -0800, Julian Elischer wrote: > > > julian 2003/02/07 18:58:16 PST > > > > Modified files: > > sys/sys proc.h > > sys/kern kern_clock.c kern_exit.c subr_prof.c > > Log: > > A little infrastructure, preceding some upcoming changes > > to the profiling and statistics code. > > > > Submitted by: DavidXu@ > > Reviewed by: peter@ > > I don't understand how the locking in addupc_task() can be correct. > > Block 1: > + PROC_LOCK(p); > + mtx_lock_spin(&sched_lock); > + if (!(p->p_sflag & PS_PROFIL)) { > + mtx_unlock_spin(&sched_lock); > + PROC_UNLOCK(p); > + return; > + } > + p->p_profthreads++; > + mtx_unlock_spin(&sched_lock); > + PROC_UNLOCK(p); > > Block 2: > + PROC_LOCK(p); > + if (--p->p_profthreads == 0) { > + if (p->p_sflag & PS_STOPPROF) { > + wakeup(&p->p_profthreads); > + stop = 0; > + } > } > [...] > + PROC_UNLOCK(p); > > It looks like sched_lock should be locked before accessing p_sflag in > "Block 2". In "Block 1", I don't see why the proc needs to be locked > while accessing p_sflag; that block could be rewritten to only hold one > lock at a time. Protecting p_profthreads with sched_lock instead of the > proc lock might help, too, since every access to p_profthreads is done > (or could easily be done) while holding sched_lock. This would make it > unnecessary to hold the proc lock while calling stopprofclock(). I think the sched locking is basically bogus. PS_PROFIL and PS_STOPPROF are only changed in thread context, so proc locking should be enough to lock them (although it doesn't lock all of p_sflag). PS_PROFIL may need to be in p_sflag since it is read in interrupt context. PS_STOPPROF seems to be completely misplaced since it is only accessed in thread context. Everything could be locked by sched_lock, but we already have too much of that. > Locking for the profil() syscall seems to be broken, too. A comment says: > /* Block profile interrupts while changing state. */ > but it doesn't actually block anything. It looks like there are races > between it and addupc_{intr,task}(). This seems to have been broken early in SMPng. The comment is about locking using splstatclock() but that is now null. sched locking is needed. I think this doesn't cause problems except probably on sparc64's because only sparc64 has non-dummy [fu]swintr() functions. Bruce To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe cvs-all" in the body of the message