From owner-freebsd-threads@FreeBSD.ORG Thu Apr 22 15:02:02 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 452F016A4CE; Thu, 22 Apr 2004 15:02:02 -0700 (PDT) Received: from mail.pcnet.com (mail.pcnet.com [204.213.232.4]) by mx1.FreeBSD.org (Postfix) with ESMTP id C7C5243D49; Thu, 22 Apr 2004 15:02:01 -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 i3MM20tf016670; Thu, 22 Apr 2004 18:02:00 -0400 (EDT) Date: Thu, 22 Apr 2004 18:02:00 -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: Thu, 22 Apr 2004 22:02:02 -0000 Sorry, I should have included threads@. 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 The thread code where? Everywhere msleep() is called? > 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. > > > ---------- Forwarded message ---------- > > Date: Thu, 22 Apr 2004 15:27:05 +0900 > > From: Kazuaki Oda > > To: threads@freebsd.org > > Subject: kse_release and kse_wakeup problem > > > > Hi, > > > > I think, after switching to use new sleep queue interface, there is a > > problem when using scope system threads on MP machine. > > This problem occurs when one CPU is executing kse_release and another > > is kse_wakeup. > > > > > > There is a scenario (thread A and B is on the same process): > > > > CPU0: thread A aquires PROC_LOCK in kse_release (kern_thread.c 621) > > CPU0: thread A releases PROC_LOCK in msleep (kern_synch.c 209) > > CPU1: thread B aquires PROC_LOCK in kse_wakeup (kern_thread.c 667) > > CPU1: thread B looks up kse_upcall (kern_thread.c 669-687) > > CPU1: thread B gets kse_upcall owner and this is thread A (kern_thread.c > > 689) CPU1: thread B sets KUF_DOUPCALL flag (kern_thread.c 697) > > because thread A is not on sleep queue yet, > > sleepq_abort (kern_thread.c 695) is not executed. > > CPU1: thread B releases PROC_LOCK (kern_thread.c 700) > > CPU0: thread A puts himself on sleep queue (kern_synch.c 221) > > CPU0: thread A sets TDF_SINTR flag (subr_sleepqueue.c 310) > > CPU0: thread A sleeps and context switch occurs... > > > > > > I think, thread B should call sleepq_abort and thread A should do > > upcall as soon as possible. > > The following patch is for thread A to release PROC_LOCK after putting > > himself on sleep queue and setting TDF_SINTR flag. > > I don't think this patch is so good (obviously setting TDF_SINTR here is > > not good), but enough for test. > > > > And, after patching, MySQL does not hang up on heavy load on my machine > > (P4 2.40GHz, HTT enabled). > > > > > > --- kern_synch.c.orig Thu Apr 22 14:00:40 2004 > > +++ kern_synch.c Thu Apr 22 12:20:06 2004 > > @@ -203,11 +203,6 @@ > > 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 > > @@ -219,6 +214,13 @@ > > * return from cursig(). > > */ > > sleepq_add(sq, ident, mtx, wmesg, 0); > > + if (catch) > > + td->td_flags |= TDF_SINTR; > > + 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) { > > -- > John Baldwin <>< http://www.FreeBSD.org/~jhb/ > "Power Users Use the Power to Serve" = http://www.FreeBSD.org > -- "Some folks are into open source, but me, I'm into open bar." -- Spencer F. Katt