Date: Thu, 22 Apr 2004 18:02:00 -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.10404221759560.15790-100000@pcnet5.pcnet.com> In-Reply-To: <200404220954.00469.jhb@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <kaakun@highway.ne.jp>
> > 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 <jhb@FreeBSD.org> <>< 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.GSO.4.10.10404221759560.15790-100000>