Date: Wed, 3 Jul 2002 17:44:36 +1000 (EST) From: Bruce Evans <bde@zeta.org.au> To: Julian Elischer <julian@elischer.org> Cc: Andrew Gallatin <gallatin@cs.duke.edu>, <freebsd-current@FreeBSD.ORG> Subject: Re: KSE signal problems still Message-ID: <20020703173906.B15258-100000@gamplex.bde.org> In-Reply-To: <20020703165856.Q15258-100000@gamplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020703173906.B15258-100000>