Date: Wed, 3 Jul 2002 01:37:30 -0700 (PDT) From: Julian Elischer <julian@elischer.org> To: Bruce Evans <bde@zeta.org.au> Cc: Andrew Gallatin <gallatin@cs.duke.edu>, freebsd-current@FreeBSD.ORG Subject: Re: KSE signal problems still Message-ID: <Pine.BSF.4.21.0207030034230.549-100000@InterJet.elischer.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:
> On Tue, 2 Jul 2002, Julian Elischer wrote:
>
> Maybe just remove the foot-shooting that releases it?
Yes I'm rationalising it at the moment..
turns out that just holding it for all of tdsignal works well.
Also removing it from setrunnable() is ok as all the callers I could find
have already locked it.
I checked in a stopgap to stop panics but I'm reworking it now.
the trouble is that thread semantics are really not well
defined for multi thread processes.
What does it mean to make a process run when it has many threads?
Should ALL threads be awakened, or is it enough if ONE thread awakens to
deliver the thread.
For right now it's mostly important that single threaded processs act
as they used to. We can always change how multithreaded processes
work.
>
> % 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.
Signotify is already calledin psignal so I've removed this one
from my version.
>
> % #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.
much of what you say will be in my next commit
I told Andrew Gallatin that I would work on cleaning up
tdsignal and maybe psignal tonight, so that's what I've been doing..
it's not perfect tough..
but it clears it up a bit..
I'm just testing it at the moment.
>
> Bruce
>
>
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?Pine.BSF.4.21.0207030034230.549-100000>