kse_release and kse_wakeup problem (fwd)

[David Xu]

John Baldwin wrote:
> On Thursday 22 April 2004 06:02 pm, Daniel Eischen wrote:
> 
>>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?
> 
> 
> 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