From owner-freebsd-current Wed Mar 1 6:33:39 2000 Delivered-To: freebsd-current@freebsd.org Received: from pcnet1.pcnet.com (pcnet1.pcnet.com [204.213.232.3]) by hub.freebsd.org (Postfix) with ESMTP id B0AB037C32C; Wed, 1 Mar 2000 06:33:22 -0800 (PST) (envelope-from eischen@vigrid.com) Received: from vigrid.com (pm3-pt99.pcnet.net [206.105.29.173]) by pcnet1.pcnet.com (8.8.7/PCNet) with ESMTP id JAA28921; Wed, 1 Mar 2000 09:33:00 -0500 (EST) Message-ID: <38BD2993.A122EDAE@vigrid.com> Date: Wed, 01 Mar 2000 09:30:43 -0500 From: "Daniel M. Eischen" X-Mailer: Mozilla 4.5 [en] (X11; I; FreeBSD 4.0-CURRENT i386) X-Accept-Language: en MIME-Version: 1.0 To: John Polstra Cc: current@freebsd.org, jasone@freebsd.org Subject: Re: pthread_{suspend,resume}_np broken? References: Content-Type: multipart/mixed; boundary="------------D3633965D555A5BBF600678E" Sender: owner-freebsd-current@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG This is a multi-part message in MIME format. --------------D3633965D555A5BBF600678E Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Daniel Eischen wrote: > > On Tue, 29 Feb 2000, John Polstra wrote: > > > In article , > > Daniel Eischen wrote: > > > On Tue, 29 Feb 2000, John Polstra wrote: > > > > > > > Shouldn't the test against PS_SUSPENDED be "==" instead of "!="? > > > > > > Yes, it should be "==" instead of "!=". > > > > > > Go ahead and fix it if you want :-) > > > > Thanks. I'll ask Jordan if I may commit the fix. > > > > What about the other part of my question? I still don't understand > > why in my test program pthread_suspend_np() isn't suspending the > > thread. I think it's a separate bug from the pthread_resume_np() bug. > > Sorry, it was very late here and I missed that part. > > I see the problem. pthread_suspend_np is broke in that it only will > work if the thread is running (PS_RUNNING). Your program is always > trying to suspend a thread that is sleeping (PS_SLEEP_WAIT) so changing > the state with PTHREAD_NEW_STATE fails. > > The fix isn't as easy as just correctly setting the threads state. > Potentially, the suspended thread could be waiting on a mutex or > condition variable and be in another queue. The correct fix is > probably to save the threads old state and then set the threads state > to PS_SUSPENDED. Resuming should restore the saved thread state. > > I'll see if I can come up with a correct fix for this. Here's a quick fix. It also includes a simple fix for pthread_kill that src/libc_r/uthread/test/sigwait/sigwait.c will expose. I haven't run any other regression tests. I'll do that when I get some more time. Jason, can you also take a look at these changes and run some tests on them? Thanks, Dan Eischen eischen@vigrid.com --------------D3633965D555A5BBF600678E Content-Type: text/plain; charset=us-ascii; name="diffs" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="diffs" Index: pthread_private.h =================================================================== RCS file: /opt/b/CVS/src/lib/libc_r/uthread/pthread_private.h,v retrieving revision 1.36 diff -c -r1.36 pthread_private.h *** pthread_private.h 2000/01/20 21:53:58 1.36 --- pthread_private.h 2000/03/01 12:49:27 *************** *** 576,581 **** --- 576,583 ---- #define PTHREAD_CANCEL_NEEDED 0x0010 int cancelflags; + int suspended; + thread_continuation_t continuation; /* Index: uthread_cancel.c =================================================================== RCS file: /opt/b/CVS/src/lib/libc_r/uthread/uthread_cancel.c,v retrieving revision 1.3 diff -c -r1.3 uthread_cancel.c *** uthread_cancel.c 2000/01/19 07:04:45 1.3 --- uthread_cancel.c 2000/03/01 13:44:57 *************** *** 37,42 **** --- 37,51 ---- pthread->cancelflags |= PTHREAD_CANCELLING; break; + case PS_SUSPENDED: + /* + * This thread isn't in any scheduling + * queues; just change it's state: + */ + pthread->cancelflags |= PTHREAD_CANCELLING; + PTHREAD_SET_STATE(pthread, PS_RUNNING); + break; + case PS_SPINBLOCK: case PS_FDR_WAIT: case PS_FDW_WAIT: *************** *** 52,58 **** case PS_WAIT_WAIT: case PS_SIGSUSPEND: case PS_SIGWAIT: - case PS_SUSPENDED: /* Interrupt and resume: */ pthread->interrupted = 1; pthread->cancelflags |= PTHREAD_CANCELLING; --- 61,66 ---- Index: uthread_cond.c =================================================================== RCS file: /opt/b/CVS/src/lib/libc_r/uthread/uthread_cond.c,v retrieving revision 1.22 diff -c -r1.22 uthread_cond.c *** uthread_cond.c 2000/01/27 23:06:59 1.22 --- uthread_cond.c 2000/03/01 13:03:46 *************** *** 282,289 **** break; } ! if (interrupted != 0 && _thread_run->continuation != NULL) ! _thread_run->continuation((void *) _thread_run); _thread_leave_cancellation_point(); } --- 282,292 ---- break; } ! if (interrupted != 0) { ! if (_thread_run->continuation != NULL) ! _thread_run->continuation((void *) _thread_run); ! rval = EINTR; ! } _thread_leave_cancellation_point(); } *************** *** 449,456 **** break; } ! if (interrupted != 0 && _thread_run->continuation != NULL) ! _thread_run->continuation((void *) _thread_run); _thread_leave_cancellation_point(); } --- 452,462 ---- break; } ! if (interrupted != 0) { ! if (_thread_run->continuation != NULL) ! _thread_run->continuation((void *) _thread_run); ! rval = EINTR; ! } _thread_leave_cancellation_point(); } Index: uthread_create.c =================================================================== RCS file: /opt/b/CVS/src/lib/libc_r/uthread/uthread_create.c,v retrieving revision 1.24 diff -c -r1.24 uthread_create.c *** uthread_create.c 2000/01/19 07:04:46 1.24 --- uthread_create.c 2000/03/01 13:19:53 *************** *** 299,308 **** /* Add the thread to the linked list of all threads: */ TAILQ_INSERT_HEAD(&_thread_list, new_thread, tle); ! if (pattr->suspend == PTHREAD_CREATE_SUSPENDED) { new_thread->state = PS_SUSPENDED; ! PTHREAD_WAITQ_INSERT(new_thread); ! } else { new_thread->state = PS_RUNNING; PTHREAD_PRIOQ_INSERT_TAIL(new_thread); } --- 299,307 ---- /* Add the thread to the linked list of all threads: */ TAILQ_INSERT_HEAD(&_thread_list, new_thread, tle); ! if (pattr->suspend == PTHREAD_CREATE_SUSPENDED) new_thread->state = PS_SUSPENDED; ! else { new_thread->state = PS_RUNNING; PTHREAD_PRIOQ_INSERT_TAIL(new_thread); } Index: uthread_kern.c =================================================================== RCS file: /opt/b/CVS/src/lib/libc_r/uthread/uthread_kern.c,v retrieving revision 1.28 diff -c -r1.28 uthread_kern.c *** uthread_kern.c 2000/01/20 04:46:51 1.28 --- uthread_kern.c 2000/03/01 13:18:35 *************** *** 184,191 **** switch (_thread_run->state) { case PS_DEAD: case PS_STATE_MAX: /* to silence -Wall */ /* ! * Dead threads are not placed in any queue: */ break; --- 184,193 ---- switch (_thread_run->state) { case PS_DEAD: case PS_STATE_MAX: /* to silence -Wall */ + case PS_SUSPENDED: /* ! * Dead and suspended threads are not placed ! * in any queue: */ break; *************** *** 227,233 **** case PS_SIGSUSPEND: case PS_SIGTHREAD: case PS_SIGWAIT: - case PS_SUSPENDED: case PS_WAIT_WAIT: /* No timeouts for these states: */ _thread_run->wakeup_time.tv_sec = -1; --- 229,234 ---- Index: uthread_mutex.c =================================================================== RCS file: /opt/b/CVS/src/lib/libc_r/uthread/uthread_mutex.c,v retrieving revision 1.20 diff -c -r1.20 uthread_mutex.c *** uthread_mutex.c 2000/01/19 07:04:49 1.20 --- uthread_mutex.c 2000/03/01 13:00:56 *************** *** 610,617 **** * Check to see if this thread was interrupted and * is still in the mutex queue of waiting threads: */ ! if (_thread_run->interrupted != 0) mutex_queue_remove(*mutex, _thread_run); /* Unlock the mutex structure: */ _SPINUNLOCK(&(*mutex)->lock); --- 610,619 ---- * Check to see if this thread was interrupted and * is still in the mutex queue of waiting threads: */ ! if (_thread_run->interrupted != 0) { mutex_queue_remove(*mutex, _thread_run); + ret = EINTR; + } /* Unlock the mutex structure: */ _SPINUNLOCK(&(*mutex)->lock); Index: uthread_resume_np.c =================================================================== RCS file: /opt/b/CVS/src/lib/libc_r/uthread/uthread_resume_np.c,v retrieving revision 1.7 diff -c -r1.7 uthread_resume_np.c *** uthread_resume_np.c 1999/08/28 00:03:44 1.7 --- uthread_resume_np.c 2000/03/01 13:46:54 *************** *** 44,51 **** /* Find the thread in the list of active threads: */ if ((ret = _find_thread(thread)) == 0) { ! /* The thread exists. Is it suspended? */ ! if (thread->state != PS_SUSPENDED) { /* * Defer signals to protect the scheduling queues * from access by the signal handler: --- 44,54 ---- /* Find the thread in the list of active threads: */ if ((ret = _find_thread(thread)) == 0) { ! /* Cancel any pending suspensions: */ ! thread->suspended = 0; ! ! /* Is it currently suspended? */ ! if (thread->state == PS_SUSPENDED) { /* * Defer signals to protect the scheduling queues * from access by the signal handler: *************** *** 53,59 **** _thread_kern_sig_defer(); /* Allow the thread to run. */ ! PTHREAD_NEW_STATE(thread,PS_RUNNING); /* * Undefer and handle pending signals, yielding if --- 56,63 ---- _thread_kern_sig_defer(); /* Allow the thread to run. */ ! PTHREAD_SET_STATE(thread,PS_RUNNING); ! PTHREAD_PRIOQ_INSERT_TAIL(thread); /* * Undefer and handle pending signals, yielding if Index: uthread_sig.c =================================================================== RCS file: /opt/b/CVS/src/lib/libc_r/uthread/uthread_sig.c,v retrieving revision 1.25 diff -c -r1.25 uthread_sig.c *** uthread_sig.c 2000/01/20 04:46:52 1.25 --- uthread_sig.c 2000/03/01 14:11:30 *************** *** 149,155 **** signal_lock.access_lock = 0; else { sigaddset(&pthread->sigmask, sig); ! /* * Make sure not to deliver the same signal to * the thread twice. sigpend is potentially --- 149,155 ---- signal_lock.access_lock = 0; else { sigaddset(&pthread->sigmask, sig); ! /* * Make sure not to deliver the same signal to * the thread twice. sigpend is potentially *************** *** 160,166 **** */ if (sigismember(&pthread->sigpend, sig)) sigdelset(&pthread->sigpend, sig); ! signal_lock.access_lock = 0; _thread_sig_deliver(pthread, sig); sigdelset(&pthread->sigmask, sig); --- 160,166 ---- */ if (sigismember(&pthread->sigpend, sig)) sigdelset(&pthread->sigpend, sig); ! signal_lock.access_lock = 0; _thread_sig_deliver(pthread, sig); sigdelset(&pthread->sigmask, sig); *************** *** 461,466 **** --- 461,467 ---- case PS_RUNNING: case PS_SIGTHREAD: case PS_STATE_MAX: + case PS_SUSPENDED: break; /* *************** *** 492,498 **** case PS_SIGWAIT: case PS_SLEEP_WAIT: case PS_SPINBLOCK: - case PS_SUSPENDED: case PS_WAIT_WAIT: if ((pthread->flags & PTHREAD_FLAGS_IN_WAITQ) != 0) { PTHREAD_WAITQ_REMOVE(pthread); --- 493,498 ---- *************** *** 628,637 **** !sigismember(&pthread->sigmask, sig)) { /* Perform any state changes due to signal arrival: */ thread_sig_check_state(pthread, sig); } - - /* Increment the pending signal count. */ - sigaddset(&pthread->sigpend,sig); } } --- 628,637 ---- !sigismember(&pthread->sigmask, sig)) { /* Perform any state changes due to signal arrival: */ thread_sig_check_state(pthread, sig); + } else { + /* Increment the pending signal count. */ + sigaddset(&pthread->sigpend,sig); } } } Index: uthread_suspend_np.c =================================================================== RCS file: /opt/b/CVS/src/lib/libc_r/uthread/uthread_suspend_np.c,v retrieving revision 1.7 diff -c -r1.7 uthread_suspend_np.c *** uthread_suspend_np.c 1999/08/28 00:03:53 1.7 --- uthread_suspend_np.c 2000/03/01 13:36:11 *************** *** 36,41 **** --- 36,43 ---- #include #include "pthread_private.h" + static void finish_suspension(void *arg); + /* Suspend a thread: */ int pthread_suspend_np(pthread_t thread) *************** *** 44,66 **** /* Find the thread in the list of active threads: */ if ((ret = _find_thread(thread)) == 0) { - /* The thread exists. Is it running? */ - if (thread->state != PS_RUNNING && - thread->state != PS_SUSPENDED) { - /* The thread operation has been interrupted */ - _thread_seterrno(thread,EINTR); - thread->interrupted = 1; - } - /* * Defer signals to protect the scheduling queues from * access by the signal handler: */ _thread_kern_sig_defer(); ! /* Suspend the thread. */ ! PTHREAD_NEW_STATE(thread,PS_SUSPENDED); /* * Undefer and handle pending signals, yielding if * necessary: --- 46,127 ---- /* Find the thread in the list of active threads: */ if ((ret = _find_thread(thread)) == 0) { /* * Defer signals to protect the scheduling queues from * access by the signal handler: */ _thread_kern_sig_defer(); + + switch (thread->state) { + case PS_RUNNING: + /* + * Remove the thread from the priority queue and + * set the state to suspended: + */ + PTHREAD_PRIOQ_REMOVE(thread); + PTHREAD_SET_STATE(thread, PS_SUSPENDED); + break; + + case PS_SPINBLOCK: + case PS_FDR_WAIT: + case PS_FDW_WAIT: + case PS_POLL_WAIT: + case PS_SELECT_WAIT: + /* + * Remove these threads from the work queue + * and mark the operation as interrupted: + */ + if ((thread->flags & PTHREAD_FLAGS_IN_WORKQ) != 0) + PTHREAD_WORKQ_REMOVE(thread); + _thread_seterrno(thread,EINTR); + thread->interrupted = 1; + + /* FALLTHROUGH */ + case PS_SIGTHREAD: + case PS_SLEEP_WAIT: + case PS_WAIT_WAIT: + case PS_SIGSUSPEND: + case PS_SIGWAIT: + /* + * Remove these threads from the waiting queue and + * set their state to suspended: + */ + PTHREAD_WAITQ_REMOVE(thread); + PTHREAD_SET_STATE(thread, PS_SUSPENDED); + break; ! case PS_MUTEX_WAIT: ! case PS_COND_WAIT: ! case PS_FDLR_WAIT: ! case PS_FDLW_WAIT: ! case PS_FILE_WAIT: ! case PS_JOIN: ! /* Mark the thread as suspended: */ ! thread->suspended = 1; + /* + * Threads in these states may be in queues. + * In order to preserve queue integrity, the + * cancelled thread must remove itself from the + * queue. Mark the thread as interrupted and + * set the state to running. When the thread + * resumes, it will remove itself from the queue + * and call the suspension completion routine. + */ + thread->interrupted = 1; + _thread_seterrno(thread, EINTR); + PTHREAD_NEW_STATE(thread, PS_RUNNING); + thread->continuation = finish_suspension; + break; + + case PS_DEAD: + case PS_DEADLOCK: + case PS_STATE_MAX: + case PS_SUSPENDED: + /* Nothing needs to be done: */ + break; + } + /* * Undefer and handle pending signals, yielding if * necessary: *************** *** 69,72 **** --- 130,142 ---- } return(ret); } + + static void + finish_suspension(void *arg) + { + if (_thread_run->suspended != 0) + _thread_kern_sched_state(PS_SUSPENDED, __FILE__, __LINE__); + } + + #endif --------------D3633965D555A5BBF600678E-- To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-current" in the body of the message