Skip site navigation (1)Skip section navigation (2)
Date:      Sat, 8 Feb 2003 18:26:05 +1100
From:      Tim Robbins <tjr@FreeBSD.org>
To:        Julian Elischer <julian@FreeBSD.org>
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>
In-Reply-To: <200302080258.h182wGsb081303@repoman.freebsd.org>; from julian@FreeBSD.org on Fri, Feb 07, 2003 at 06:58:16PM -0800
References:  <200302080258.h182wGsb081303@repoman.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
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




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