kse_release and kse_wakeup problem (fwd)

[Daniel Eischen]

Sorry, I should have included threads at .

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 at highway.ne.jp>
> > To: threads at 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 at 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