From owner-cvs-all Fri Feb 7 23:26:25 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 BB71A37B401; Fri, 7 Feb 2003 23:26:21 -0800 (PST) Received: from smtp03.iprimus.com.au (smtp03.iprimus.com.au [210.50.30.52]) by mx1.FreeBSD.org (Postfix) with ESMTP id A272543FBF; Fri, 7 Feb 2003 23:26:20 -0800 (PST) (envelope-from tim@robbins.dropbear.id.au) Received: from smtp02.iprimus.net.au (210.50.76.70) by smtp03.iprimus.com.au (6.7.010) id 3E3E0A81000E6B62; Sat, 8 Feb 2003 18:26:10 +1100 Received: from dilbert.robbins.dropbear.id.au ([210.50.220.220]) by smtp02.iprimus.net.au with Microsoft SMTPSVC(5.0.2195.5600); Sat, 8 Feb 2003 18:26:09 +1100 Received: from dilbert.robbins.dropbear.id.au (610hlneghdi9t0h7@localhost [127.0.0.1]) by dilbert.robbins.dropbear.id.au (8.12.6/8.12.6) with ESMTP id h187Q6kC043750; Sat, 8 Feb 2003 18:26:06 +1100 (EST) (envelope-from tim@dilbert.robbins.dropbear.id.au) Received: (from tim@localhost) by dilbert.robbins.dropbear.id.au (8.12.6/8.12.6/Submit) id h187Q5Nf043749; Sat, 8 Feb 2003 18:26:05 +1100 (EST) (envelope-from tim) Date: Sat, 8 Feb 2003 18:26:05 +1100 From: Tim Robbins To: Julian Elischer Cc: src-committers@FreeBSD.org, cvs-src@FreeBSD.org, cvs-all@FreeBSD.org, legacy-committers@FreeBSD.org Subject: Re: cvs commit: src/sys/sys proc.h src/sys/kern kern_clock.c kern_exit.c subr_prof.c Message-ID: <20030208182605.A43083@dilbert.robbins.dropbear.id.au> References: <200302080258.h182wGsb081303@repoman.freebsd.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.2.5.1i In-Reply-To: <200302080258.h182wGsb081303@repoman.freebsd.org>; from julian@FreeBSD.org on Fri, Feb 07, 2003 at 06:58:16PM -0800 X-OriginalArrivalTime: 08 Feb 2003 07:26:09.0865 (UTC) FILETIME=[59F2FB90:01C2CF43] 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 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(). 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}(). Tim To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe cvs-all" in the body of the message