From owner-freebsd-threads@FreeBSD.ORG Thu Apr 22 22:32:51 2004 Return-Path: Delivered-To: freebsd-threads@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 20BA716A4CE; Thu, 22 Apr 2004 22:32:51 -0700 (PDT) Received: from mail.pcnet.com (mail.pcnet.com [204.213.232.4]) by mx1.FreeBSD.org (Postfix) with ESMTP id B173143D2F; Thu, 22 Apr 2004 22:32:50 -0700 (PDT) (envelope-from eischen@vigrid.com) Received: from mail.pcnet.com (mail.pcnet.com [204.213.232.4]) by mail.pcnet.com (8.12.10/8.12.1) with ESMTP id i3N5Wntf025368; Fri, 23 Apr 2004 01:32:49 -0400 (EDT) Date: Fri, 23 Apr 2004 01:32:49 -0400 (EDT) From: Daniel Eischen X-Sender: eischen@pcnet5.pcnet.com To: John Baldwin In-Reply-To: <200404220954.00469.jhb@FreeBSD.org> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII cc: threads@FreeBSD.org cc: davidxu@FreeBSD.org Subject: Re: kse_release and kse_wakeup problem (fwd) X-BeenThere: freebsd-threads@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Threading on FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 23 Apr 2004 05:32:51 -0000 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