Date: Sat, 8 Feb 2003 17:39:40 -0800 (PST) From: Julian Elischer <julian@elischer.org> To: Bruce Evans <bde@zeta.org.au> Cc: Tim Robbins <tjr@FreeBSD.org>, Julian Elischer <julian@FreeBSD.org>, 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 Message-ID: <200302090139.RAA31736@InterJet.elischer.org>
next in thread | raw e-mail | index | archive | help
kern_exit.c subr_prof.c
In-Reply-To: <20030208220713.O17322-100000@gamplex.bde.org>
Message-ID: <Pine.BSF.4.21.0302081734270.31703-100000@InterJet.elischer.org>
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:
> >
> > > 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.
Now that you mention it, I agree that probably the schedlock is not
needed. Proc lock should be enough. I'll check into it..
BTW Tim I didn't get any mail from you directly.. I just saw this
reply...
Peter and I were discussing bit placement in flags and this looks like a
good candidate for some moving..
>
> > 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200302090139.RAA31736>