From owner-freebsd-threads@FreeBSD.ORG Mon Apr 26 05:42:05 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 93C2E16A4CE; Mon, 26 Apr 2004 05:42:05 -0700 (PDT) Received: from freefall.freebsd.org (freefall.freebsd.org [216.136.204.21]) by mx1.FreeBSD.org (Postfix) with ESMTP id 7496F43D5A; Mon, 26 Apr 2004 05:42:05 -0700 (PDT) (envelope-from davidxu@freebsd.org) Received: from freebsd.org (davidxu@localhost [127.0.0.1]) i3QCg1BO024129; Mon, 26 Apr 2004 05:42:03 -0700 (PDT) (envelope-from davidxu@freebsd.org) Message-ID: <408D0373.8050006@freebsd.org> Date: Mon, 26 Apr 2004 20:41:23 +0800 From: David Xu User-Agent: Mozilla Thunderbird 0.5 (X11/20040417) X-Accept-Language: en-us, en MIME-Version: 1.0 To: John Baldwin References: <200404231357.23096.jhb@FreeBSD.org> In-Reply-To: <200404231357.23096.jhb@FreeBSD.org> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit cc: threads@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: Mon, 26 Apr 2004 12:42:05 -0000 John Baldwin wrote: > On Thursday 22 April 2004 06:02 pm, Daniel Eischen wrote: > >>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? > > > No, in sleepq_catch_signals() just as I quoted below: > > >>>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.e. do the upcall check in sleepq_catch_signals() right where you already do > thread_suspend_check(1). The only reason you have to do this, btw, is > because the kse_release() code is trying to mess with thread state internals > using sleepq_abort(), etc. The other in-kernel code that does that (signals) > already does the check in sleepq_catch_signals() and has done the same type > of check in msleep()/tsleep() for quite a while. > > If the kse_release() stuff was just using sleep/wakeup() rather than trying to > manually abort sleeps it wouldn't have to be so intimate with the sleep > interface. > > Note that thr's thr_wakeup() and thr_sleep() manage to simulate > synchronization w/o having to abort sleeps, but it is probably also easier to > do that than for the M:N case. > I think libthr will encounters same problem as libpthread with new sleep queue code, because mtx is released too early in msleep before thread markes itself as ON_SLEEPQ, thr_suspend and thr_wakeup have same race window as kse_release and kse_wakeup. Any code wants to put synchronous bit in td_flags like these codes will be broken. David Xu