Date: Wed, 01 Mar 2000 09:30:43 -0500
From: "Daniel M. Eischen" <eischen@vigrid.com>
To: John Polstra <jdp@polstra.com>
Cc: current@freebsd.org, jasone@freebsd.org
Subject: Re: pthread_{suspend,resume}_np broken?
Message-ID: <38BD2993.A122EDAE@vigrid.com>
References: <Pine.SUN.3.91.1000301064035.7639A-100000@pcnet1.pcnet.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <Pine.SUN.3.91.1000229224700.20669A@pcnet1.pcnet.com>,
> > Daniel Eischen <eischen@vigrid.com> 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 <pthread.h>
#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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?38BD2993.A122EDAE>