From owner-cvs-all Sat Feb 8 22: 6:59 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 7A14F37B406; Sat, 8 Feb 2003 22:06:56 -0800 (PST) Received: from sccrmhc01.attbi.com (sccrmhc01.attbi.com [204.127.202.61]) by mx1.FreeBSD.org (Postfix) with ESMTP id D40CC43FEA; Sat, 8 Feb 2003 22:06:50 -0800 (PST) (envelope-from julian@elischer.org) Received: from interjet.elischer.org (12-232-168-4.client.attbi.com[12.232.168.4]) by sccrmhc01.attbi.com (sccrmhc01) with ESMTP id <2003020906064900100g2l9ne>; Sun, 9 Feb 2003 06:06:49 +0000 Received: from localhost (localhost.elischer.org [127.0.0.1]) by InterJet.elischer.org (8.9.1a/8.9.1) with ESMTP id WAA33546; Sat, 8 Feb 2003 22:06:48 -0800 (PST) Message-Id: <200302090606.WAA33546@InterJet.elischer.org> Date: Sat, 8 Feb 2003 22:06:46 -0800 (PST) From: Julian Elischer To: Bruce Evans Cc: Tim Robbins , Julian Elischer , cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/sys proc.h src/sys/kern kern_clock.c 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 kern_exit.c subr_prof.c In-Reply-To: <20030208220713.O17322-100000@gamplex.bde.org> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII On Sat, 8 Feb 2003, Bruce Evans wrote: > On Sat, 8 Feb 2003, Tim Robbins wrote: > > > On Fri, Feb 07, 2003 at 06:58:16PM -0800, Julian Elischer wrote: > > > > 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); This is the kind of thing that makes opensource good. Sometimes you can look at something 'til you are blue in the face and not see a problem.. but SOMEONE will see it. As for the question of the schedlock it's required that you hold schedlock to use p_sflag. Normally you want to hold the lock while you do some other operation in order to make the read of p_sflag and that other operation atomic. In Block A the two operations that must be made atomic are: 1/ discovering PS_PROFIL is set, and 2/ incrementing p->p_profthreads if you didn't have the sched lock set then some other process could unset PS_PROFIL before you have a chnace to increment p->p_profthreads. In Block B, the race we would be worried about is that someone might set PS_STOPPROF, after we have decided that it is not set, but the only code that can do that is in stopprofclock() and it must first get the proc lock. Since we have the proc lock, we are safe that the race we are worried about cannot happen (which is just as well as we don't want to have to hold the schedlock through the wakeup() anyhow. So it turns out the code is correct. > > > > 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(). Sched lock has a much more disruptive effect on the system. > > 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. PS_STOPPROF is accessed in inerrupt context too I think. never-the-less it's worth looking at how it might be improved julian To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe cvs-all" in the body of the message