From owner-freebsd-current Wed Jul 3 0:38:58 2002 Delivered-To: freebsd-current@freebsd.org Received: from mx1.FreeBSD.org (mx1.FreeBSD.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 6436137B400 for ; Wed, 3 Jul 2002 00:38:50 -0700 (PDT) Received: from mailman.zeta.org.au (mailman.zeta.org.au [203.26.10.16]) by mx1.FreeBSD.org (Postfix) with ESMTP id 778AC43E58 for ; Wed, 3 Jul 2002 00:38:49 -0700 (PDT) (envelope-from bde@zeta.org.au) Received: from bde.zeta.org.au (bde.zeta.org.au [203.2.228.102]) by mailman.zeta.org.au (8.9.3/8.8.7) with ESMTP id RAA13091; Wed, 3 Jul 2002 17:38:40 +1000 Date: Wed, 3 Jul 2002 17:44:36 +1000 (EST) From: Bruce Evans X-X-Sender: bde@gamplex.bde.org To: Julian Elischer Cc: Andrew Gallatin , Subject: Re: KSE signal problems still In-Reply-To: <20020703165856.Q15258-100000@gamplex.bde.org> Message-ID: <20020703173906.B15258-100000@gamplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-current@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG On Wed, 3 Jul 2002, Bruce Evans wrote: > Maybe just remove the foot-shooting that releases it? > > % Index: kern_sig.c > % =================================================================== > % RCS file: /home/ncvs/src/sys/kern/kern_sig.c,v > % retrieving revision 1.170 > % retrieving revision 1.171 > % diff -u -1 -r1.170 -r1.171 > % --- kern_sig.c 29 Jun 2002 02:00:01 -0000 1.170 > % +++ kern_sig.c 29 Jun 2002 17:26:18 -0000 1.171 > % @@ -1486,15 +1540,9 @@ > % */ > % - if (p->p_stat == SRUN) { > % + mtx_unlock_spin(&sched_lock); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ shoot foot > % + if (td->td_state == TDS_RUNQ || > % + td->td_state == TDS_RUNNING) { > > I think sched_lock is needed for checking td_state too (strictly to use > the result of the check, so the lock is not critical if the use doesn't > do anything harmful), but there is no lock indication for td_state > in proc.h like there used to be for p_stat. > > % + signotify(td->td_proc); > > Holding sched_lock when calling signotify() used to be an error, but that > was changed in rev.1.155. This signotify() call seems to be bogus anyway. > signotify() should only be called after the signal mask is changed. The > call to signotify() here was removed in rev.1.154 when the semantics of > signotify() was changed a little. Bogus calls to signotify() just waste > time. > > % #ifdef SMP > % - struct kse *ke; > % - struct thread *td = curthread; > % -/* we should only deliver to one thread.. but which one? */ > % - FOREACH_KSEGRP_IN_PROC(p, kg) { > % - FOREACH_KSE_IN_GROUP(kg, ke) { > % - if (ke->ke_thread == td) { > % - continue; > % - } > % - forward_signal(ke->ke_thread); > % - } > % - } > % + if (td->td_state == TDS_RUNNING && td != curthread) > % + forward_signal(td); > % #endif > > forward_signal() was called with sched_lock held in rev.1.170, and > forward_signal() still requires it to be held. I think sched_lock is > needed for checking td_state too, as above. Here it is fairly clear > that calling forward_signal() bogusly after losing a race is harmless. > It just wakes up td to look for a signal that isn't there or can't > be handled yet. Since this only happens if we lose a race, it may be > more efficient to let it happen (rarely) than to lock (always) to prevent > it happening. But we already held the lock so the locking was free > except for latency issues. > > Bruce Untested fix for thes bugs and some style bugs in tdsignal(): Index: kern_sig.c =================================================================== RCS file: /home/ncvs/src/sys/kern/kern_sig.c,v retrieving revision 1.171 diff -u -2 -r1.171 kern_sig.c --- kern_sig.c 29 Jun 2002 17:26:18 -0000 1.171 +++ kern_sig.c 3 Jul 2002 07:42:31 -0000 @@ -1468,5 +1449,5 @@ /* * The force of a signal has been directed against a single - * thread. We need to see what we can do about knocking it + * thread. We need to see what we can do about knocking it * out of any sleep it may be in etc. */ @@ -1485,8 +1466,7 @@ */ mtx_lock_spin(&sched_lock); - if ((action == SIG_DFL) && (prop & SA_KILL)) { - if (td->td_priority > PUSER) { + if (action == SIG_DFL && (prop & SA_KILL)) { + if (td->td_priority > PUSER) td->td_priority = PUSER; - } } mtx_unlock_spin(&sched_lock); @@ -1496,7 +1476,7 @@ * except that stopped processes must be continued by SIGCONT. */ - if (action == SIG_HOLD) { + if (action == SIG_HOLD) goto out; - } + mtx_lock_spin(&sched_lock); if (td->td_state == TDS_SLP) { @@ -1531,24 +1511,17 @@ } goto runfast; - /* NOTREACHED */ - } else { /* - * Other states do nothing with the signal immediatly, + * Other states do nothing with the signal immediately, * other than kicking ourselves if we are running. * It will either never be noticed, or noticed very soon. */ - mtx_unlock_spin(&sched_lock); - if (td->td_state == TDS_RUNQ || - td->td_state == TDS_RUNNING) { - signotify(td->td_proc); #ifdef SMP - if (td->td_state == TDS_RUNNING && td != curthread) - forward_signal(td); + if (td->td_state == TDS_RUNNING && td != curthread) + forward_signal(td); #endif - } + mtx_unlock_spin(&sched_lock); goto out; } - /*NOTREACHED*/ runfast: @@ -1557,7 +1530,7 @@ */ mtx_lock_spin(&sched_lock); - if (td->td_priority > PUSER) { + if (td->td_priority > PUSER) td->td_priority = PUSER; - } + run: mtx_assert(&sched_lock, MA_OWNED | MA_NOTRECURSED); To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-current" in the body of the message