Skip site navigation (1)Skip section navigation (2)
Date:      Sat, 8 Feb 2003 23:53:35 +1100 (EST)
From:      Bruce Evans <bde@zeta.org.au>
To:        Tim Robbins <tjr@FreeBSD.org>
Cc:        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 kern_exit.c subr_prof.c
Message-ID:  <20030208220713.O17322-100000@gamplex.bde.org>
In-Reply-To: <20030208182605.A43083@dilbert.robbins.dropbear.id.au>

next in thread | previous in thread | raw e-mail | index | archive | help
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-src" in the body of the message




Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20030208220713.O17322-100000>