Date: Sat, 8 Feb 2003 22:06:46 -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>, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/sys proc.h src/sys/kern kern_clock.c Message-ID: <200302090606.WAA33546@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.0302081917500.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:
> >
> > 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200302090606.WAA33546>