Date: Fri, 23 Apr 2004 01:32:49 -0400 (EDT) From: Daniel Eischen <eischen@vigrid.com> To: John Baldwin <jhb@FreeBSD.org> Cc: davidxu@FreeBSD.org Subject: Re: kse_release and kse_wakeup problem (fwd) Message-ID: <Pine.GSO.4.10.10404230109020.17629-100000@pcnet5.pcnet.com> In-Reply-To: <200404220954.00469.jhb@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 22 Apr 2004, John Baldwin wrote:
> On Thursday 22 April 2004 09:01 am, Daniel Eischen wrote:
> > What do you guys think of this patch?
>
> I think that the thread code should check for the upcall case the same way
> that we check for signals by calliing cursig() in sleepq_catch_signals(),
> that is:
>
> /* Mark thread as being in an interruptible sleep. */
> mtx_lock_spin(&sched_lock);
> MPASS(TD_ON_SLEEPQ(td));
> td->td_flags |= TDF_SINTR;
> mtx_unlock_spin(&sched_lock);
> sleepq_release(wchan);
>
> /* See if there are any pending signals for this thread. */
> PROC_LOCK(p);
> mtx_lock(&p->p_sigacts->ps_mtx);
> sig = cursig(td);
> mtx_unlock(&p->p_sigacts->ps_mtx);
> if (sig == 0 && thread_suspend_check(1))
> sig = SIGSTOP;
> PROC_UNLOCK(p);
>
> /*
> * If there were pending signals and this thread is still on
> * the sleep queue, remove it from the sleep queue.
> */
>
> The thread_suspend_check() code should also be checking for the UPCALL case
> and as well. Maybe something like:
>
> if (sig == 0 && thread_suspend_check(1))
> sig == SIGSTOP;
> if (sig == 0 && thread_upcall_check())
> doing_upcall = 1;
>
> and then dequeue if we are doing an upcall just like we do if there is already
> a signal. This mirrors the way we handle signals since UPCALLs are a kind of
> special cased signal. The patch below has incorrect locking (td_flags could
> get trashed) by the way.
I think td_flags can get (conditionally) set just before the sched_lock
is released (before GIANT is dropped).
Other than that, the patch makes sense to me. It just makes sure
that the thread is put on the sleep queue before the mutex is
released. As long as the mutex is held while fiddling with
KUF_DOUPCALL, it seems like that should work.
I'm not sure what you are trying to say above. Should the thread
code not be using msleep()? I don't like that it has to know a
little something about sleepq. Usually with mutexes, CV's, and
the like, you don't wake up particular threads which is what
we need to do in this case.
Index: kern_synch.c
===================================================================
RCS file: /opt/FreeBSD/cvs/src/sys/kern/kern_synch.c,v
retrieving revision 1.246
diff -u -r1.246 kern_synch.c
--- kern_synch.c 12 Mar 2004 19:06:18 -0000 1.246
+++ kern_synch.c 23 Apr 2004 05:19:37 -0000
@@ -202,16 +202,13 @@
}
}
}
+ if (catch)
+ td->td_flags |= TDF_SINTR;
mtx_unlock_spin(&sched_lock);
CTR5(KTR_PROC, "msleep: thread %p (pid %d, %s) on %s (%p)",
td, p->p_pid, p->p_comm, wmesg, ident);
DROP_GIANT();
- if (mtx != NULL) {
- mtx_assert(mtx, MA_OWNED | MA_NOTRECURSED);
- WITNESS_SAVE(&mtx->mtx_object, mtx);
- mtx_unlock(mtx);
- }
/*
* We put ourselves on the sleep queue and start our timeout
@@ -223,6 +220,11 @@
* return from cursig().
*/
sleepq_add(sq, ident, mtx, wmesg, 0);
+ if (mtx != NULL) {
+ mtx_assert(mtx, MA_OWNED | MA_NOTRECURSED);
+ WITNESS_SAVE(&mtx->mtx_object, mtx);
+ mtx_unlock(mtx);
+ }
if (timo)
sleepq_set_timeout(ident, timo);
if (catch) {
--
Dan Eischen
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.GSO.4.10.10404230109020.17629-100000>