From 13adeede88d026548e4d2da497f93d8024706a2b Mon Sep 17 00:00:00 2001 From: Gabor Melis Date: Fri, 19 Aug 2005 15:14:16 +0000 Subject: [PATCH] 0.9.3.66: * handle failed rt signal generation due to full kernel queue, this makes INTERRUPT-THREAD and gc_{stop,start}_the_world less deadlock prone * reduced lock contention related to INTERRUPT-THREAD with a beneficial effect on mass extinction of threads by TERMINATE-SESSION: in extreme cases it could have taken minutes to shut down a hundred threads * reduce delay in thread tests to make it run faster and perhaps more likely to trigger problems * stable on my machine when compiled with gcc4 --- NEWS | 4 +- src/code/target-thread.lisp | 26 +++---- src/compiler/generic/objdef.lisp | 2 +- src/runtime/thread.c | 148 +++++++++++++++++++++++++------------- src/runtime/thread.h | 2 +- src/runtime/x86-linux-os.c | 6 +- tests/threads.impure.lisp | 12 ++-- version.lisp-expr | 2 +- 8 files changed, 128 insertions(+), 74 deletions(-) diff --git a/NEWS b/NEWS index 3e62313..3f57aca 100644 --- a/NEWS +++ b/NEWS @@ -45,7 +45,9 @@ changes in sbcl-0.9.4 relative to sbcl-0.9.3: ** bug fix: lockup when compiled with gcc4 ** bug fix: race that allows the gc to be triggered when gc is inhibited - ** buf fix: one less memory fault in INTERRUPT-THREAD, again + ** bug fix: one less memory fault in INTERRUPT-THREAD, again + ** bug fix: gc and INTERRUPT-THREAD don't hang when the RT signal + queue is full * fixed some bugs revealed by Paul Dietz' test suite: ** CALL-NEXT-METHOD signals an error (in safe code) when the call has arguments with a different set of applicable methods from diff --git a/src/code/target-thread.lisp b/src/code/target-thread.lisp index da38ee8..f4e9fc6 100644 --- a/src/code/target-thread.lisp +++ b/src/code/target-thread.lisp @@ -549,18 +549,20 @@ won't like the effect." #!-sb-thread (funcall function) #!+sb-thread - (let ((function (coerce function 'function))) - (multiple-value-bind (res err) - ;; protect against gcing just when the ub32 address is ready - ;; to be passed to C - (sb!sys::with-pinned-objects (function) - (sb!unix::syscall ("interrupt_thread" - system-area-pointer sb!alien:unsigned-long) - thread - (thread-%sap thread) - (sb!kernel:get-lisp-obj-address function))) - (unless res - (error 'interrupt-thread-error :thread thread :errno err))))) + (if (eq thread *current-thread*) + (funcall function) + (let ((function (coerce function 'function))) + (multiple-value-bind (res err) + ;; protect against gcing just when the ub32 address is + ;; just ready to be passed to C + (sb!sys::with-pinned-objects (function) + (sb!unix::syscall ("interrupt_thread" + system-area-pointer sb!alien:unsigned-long) + thread + (thread-%sap thread) + (sb!kernel:get-lisp-obj-address function))) + (unless res + (error 'interrupt-thread-error :thread thread :errno err)))))) (defun terminate-thread (thread) #!+sb-doc diff --git a/src/compiler/generic/objdef.lisp b/src/compiler/generic/objdef.lisp index f924eb0..df4b6cc 100644 --- a/src/compiler/generic/objdef.lisp +++ b/src/compiler/generic/objdef.lisp @@ -425,7 +425,7 @@ #!+(or x86 x86-64) (pseudo-atomic-atomic) #!+(or x86 x86-64) (pseudo-atomic-interrupted) (interrupt-fun) - (interrupt-fun-lock) + (interrupt-fun-lock :c-type "volatile lispobj") (interrupt-data :c-type "struct interrupt_data *" :length #!+alpha 2 #!-alpha 1) (interrupt-contexts :c-type "os_context_t *" :rest-p t)) diff --git a/src/runtime/thread.c b/src/runtime/thread.c index 9e0e9eb..cedf3de 100644 --- a/src/runtime/thread.c +++ b/src/runtime/thread.c @@ -26,7 +26,7 @@ #define ALIEN_STACK_SIZE (1*1024*1024) /* 1Mb size chosen at random */ int dynamic_values_bytes=4096*sizeof(lispobj); /* same for all threads */ -struct thread *all_threads; +struct thread * volatile all_threads; volatile lispobj all_threads_lock; extern struct interrupt_data * global_interrupt_data; extern int linux_no_threads_p; @@ -109,10 +109,16 @@ new_thread_trampoline(struct thread *th) int result; function = th->unbound_marker; th->unbound_marker = UNBOUND_MARKER_WIDETAG; - if(arch_os_thread_init(th)==0) return 1; + if(arch_os_thread_init(th)==0) { + /* FIXME: handle error */ + lose("arch_os_thread_init failed\n"); + } /* wait here until our thread is linked into all_threads: see below */ - while(th->os_thread<1) sched_yield(); + { + volatile os_thread_t *tid=&th->os_thread; + while(*tid<1) sched_yield(); + } th->state=STATE_RUNNING; result = funcall0(function); @@ -263,6 +269,7 @@ void link_thread(struct thread *th,os_thread_t kid_tid) protect_control_stack_guard_page(th,1); /* child will not start until this is set */ th->os_thread=kid_tid; + FSHOW((stderr,"/created thread %lu\n",kid_tid)); } void create_initial_thread(lispobj initial_function) { @@ -289,6 +296,9 @@ boolean create_os_thread(struct thread *th,os_thread_t *kid_tid) sigset_t newset,oldset; boolean r=1; sigemptyset(&newset); + /* Blocking deferrable signals is enough, since gc_stop_the_world + * waits until the child leaves STATE_STARTING. And why not let gc + * proceed as soon as possible? */ sigaddset_deferrable(&newset); thread_sigmask(SIG_BLOCK, &newset, &oldset); @@ -368,47 +378,78 @@ void reap_dead_thread(struct thread *th) 32*SIGSTKSZ); } +/* Send the signo to os_thread, retry if the rt signal queue is + * full. */ +static int kill_thread_safely(os_thread_t os_thread, int signo) +{ + int r; + /* The man page does not mention EAGAIN as a valid return value + * for either pthread_kill or kill. But that's theory, this is + * practice. By waiting here we assume that the delivery of this + * signal is not necessary for the delivery of the signals in the + * queue. In other words, we _assume_ there are no deadlocks. */ + while ((r=pthread_kill(os_thread,signo))==EAGAIN) { + /* wait a bit then try again in the hope of the rt signal + * queue not being full */ + FSHOW_SIGNAL((stderr,"/rt signal queue full\n")); + /* FIXME: some kind of backoff (random, exponential) would be + * nice. */ + sleep(1); + } + return r; +} + int interrupt_thread(struct thread *th, lispobj function) { - /* A thread may also become dead after this test. */ - if ((th->state != STATE_DEAD)) { - /* In clone_threads, if A and B both interrupt C at - * approximately the same time, it does not matter: the - * second signal will be masked until the handler has - * returned from the first one. In pthreads though, we - * can't put the knowledge of what function to call into - * the siginfo, so we have to store it in the destination - * thread, and do it in such a way that A won't clobber - * B's interrupt. Hence this stupid linked list. - * - * This does depend on SIG_INTERRUPT_THREAD being queued - * (as POSIX RT signals are): we need to keep - * interrupt_fun data for exactly as many signals as are - * going to be received by the destination thread. - */ - lispobj c=alloc_cons(function,NIL); - int kill_status; - /* interrupt_thread_handler locks this spinlock with - * interrupts blocked (it does so for the sake of - * arrange_return_to_lisp_function), so we must also block - * them or else SIG_STOP_FOR_GC and all_threads_lock will find - * a way to deadlock. */ - sigset_t newset,oldset; - sigemptyset(&newset); - sigaddset_blockable(&newset); - thread_sigmask(SIG_BLOCK, &newset, &oldset); - get_spinlock(&th->interrupt_fun_lock, - (long)arch_os_get_current_thread()); - kill_status=thread_kill(th->os_thread,SIG_INTERRUPT_THREAD); - if(kill_status==0) { - ((struct cons *)native_pointer(c))->cdr=th->interrupt_fun; - th->interrupt_fun=c; + /* In clone_threads, if A and B both interrupt C at approximately + * the same time, it does not matter: the second signal will be + * masked until the handler has returned from the first one. In + * pthreads though, we can't put the knowledge of what function to + * call into the siginfo, so we have to store it in the + * destination thread, and do it in such a way that A won't + * clobber B's interrupt. Hence, this stupid linked list. + * + * This does depend on SIG_INTERRUPT_THREAD being queued (as POSIX + * RT signals are): we need to keep interrupt_fun data for exactly + * as many signals as are going to be received by the destination + * thread. + */ + lispobj c=alloc_cons(function,NIL); + sigset_t newset,oldset; + sigemptyset(&newset); + /* interrupt_thread_handler locks this spinlock with blockables + * blocked (it does so for the sake of + * arrange_return_to_lisp_function), so we must also block them or + * else SIG_STOP_FOR_GC and all_threads_lock will find a way to + * deadlock. */ + sigaddset_blockable(&newset); + thread_sigmask(SIG_BLOCK, &newset, &oldset); + if (th == arch_os_get_current_thread()) + lose("cannot interrupt current thread"); + get_spinlock(&th->interrupt_fun_lock, + (long)arch_os_get_current_thread()); + ((struct cons *)native_pointer(c))->cdr=th->interrupt_fun; + th->interrupt_fun=c; + release_spinlock(&th->interrupt_fun_lock); + thread_sigmask(SIG_SETMASK,&oldset,0); + /* Called from lisp with the thread object as a parameter. Thus, + * the object cannot be garbage collected and consequently reaped + * and joined. Because it's not joined, kill should work (even if + * the thread has died/exited). */ + { + int status=kill_thread_safely(th->os_thread,SIG_INTERRUPT_THREAD); + if (status==0) { + return 0; + } else if (status==ESRCH) { + /* This thread has exited. */ + th->interrupt_fun=NIL; + errno=ESRCH; + return -1; + } else { + lose("cannot send SIG_INTERRUPT_THREAD to thread=%lu: %d, %s", + th->os_thread,status,strerror(status)); } - release_spinlock(&th->interrupt_fun_lock); - thread_sigmask(SIG_SETMASK,&oldset,0); - return (kill_status ? -1 : 0); } - errno=EPERM; return -1; } /* stopping the world is a two-stage process. From this thread we signal @@ -423,6 +464,7 @@ int interrupt_thread(struct thread *th, lispobj function) void gc_stop_the_world() { struct thread *p,*th=arch_os_get_current_thread(); + int status; FSHOW_SIGNAL((stderr,"/gc_stop_the_world:waiting on lock, thread=%lu\n", th->os_thread)); /* keep threads from starting while the world is stopped. */ @@ -433,14 +475,15 @@ void gc_stop_the_world() for(p=all_threads; p; p=p->next) { while(p->state==STATE_STARTING) sched_yield(); if((p!=th) && (p->state==STATE_RUNNING)) { - FSHOW_SIGNAL((stderr, "/gc_stop_the_world: suspending %lu\n", + status=kill_thread_safely(p->os_thread,SIG_STOP_FOR_GC); + FSHOW_SIGNAL((stderr,"/gc_stop_the_world: suspending %lu\n", p->os_thread)); - if(thread_kill(p->os_thread,SIG_STOP_FOR_GC)==-1) { - /* we can't kill the thread; assume because it died - * since we last checked */ - p->state=STATE_DEAD; - FSHOW_SIGNAL((stderr,"/gc_stop_the_world:assuming %lu dead\n", - p->os_thread)); + if (status==ESRCH) { + /* This thread has exited. */ + gc_assert(p->state==STATE_DEAD); + } else if (status) { + lose("cannot send suspend thread=%lu: %d, %s", + p->os_thread,status,strerror(status)); } } } @@ -462,6 +505,7 @@ void gc_stop_the_world() void gc_start_the_world() { struct thread *p,*th=arch_os_get_current_thread(); + int status; /* if a resumed thread creates a new thread before we're done with * this loop, the new thread will get consed on the front of * all_threads, but it won't have been stopped so won't need @@ -477,9 +521,17 @@ void gc_start_the_world() FSHOW_SIGNAL((stderr, "/gc_start_the_world: resuming %lu\n", p->os_thread)); p->state=STATE_RUNNING; - thread_kill(p->os_thread,SIG_STOP_FOR_GC); + status=kill_thread_safely(p->os_thread,SIG_STOP_FOR_GC); + if (status) { + lose("cannot resume thread=%lu: %d, %s", + p->os_thread,status,strerror(status)); + } } } + /* If we waited here until all threads leave STATE_SUSPENDED, then + * SIG_STOP_FOR_GC wouldn't need to be a rt signal. That has some + * performance implications, but does away with the 'rt signal + * queue full' problem. */ release_spinlock(&all_threads_lock); FSHOW_SIGNAL((stderr,"/gc_start_the_world:end\n")); } diff --git a/src/runtime/thread.h b/src/runtime/thread.h index 6aa8f60..07b59dd 100644 --- a/src/runtime/thread.h +++ b/src/runtime/thread.h @@ -31,7 +31,7 @@ union per_thread_data { lispobj dynamic_values[1]; /* actually more like 4000 or so */ }; -extern struct thread *all_threads; +extern struct thread * volatile all_threads; extern int dynamic_values_bytes; #ifdef LISP_FEATURE_SB_THREAD diff --git a/src/runtime/x86-linux-os.c b/src/runtime/x86-linux-os.c index 740d382..5b55fdc 100644 --- a/src/runtime/x86-linux-os.c +++ b/src/runtime/x86-linux-os.c @@ -98,7 +98,7 @@ int arch_os_thread_init(struct thread *thread) { if (modify_ldt (1, &ldt_entry, sizeof (ldt_entry)) != 0) { modify_ldt_lock=0; /* modify_ldt call failed: something magical is not happening */ - return -1; + return 0; } __asm__ __volatile__ ("movw %w0, %%fs" : : "q" ((n << 3) /* selector number */ @@ -117,10 +117,8 @@ int arch_os_thread_init(struct thread *thread) { sigstack.ss_sp=((void *) thread)+dynamic_values_bytes; sigstack.ss_flags=0; sigstack.ss_size = 32*SIGSTKSZ; - sigaltstack(&sigstack,0); - if(sigaltstack(&sigstack,0)<0) { + if(sigaltstack(&sigstack,0)<0) lose("Cannot sigaltstack: %s\n",strerror(errno)); - } #endif return 1; } diff --git a/tests/threads.impure.lisp b/tests/threads.impure.lisp index d642a69..48bf07e 100644 --- a/tests/threads.impure.lisp +++ b/tests/threads.impure.lisp @@ -227,7 +227,7 @@ (sb-thread:make-thread (lambda () (loop repeat 25 do - (sleep (random 2d0)) + (sleep (random 0.1d0)) (princ ".") (force-output) (sb-thread:interrupt-thread @@ -244,7 +244,7 @@ ;; pseudo-atomic atomicity (format t "new thread ~A~%" c) (dotimes (i 100) - (sleep (random 1d0)) + (sleep (random 0.1d0)) (interrupt-thread c (lambda () (princ ".") (force-output) @@ -276,10 +276,10 @@ (sb-impl::atomic-incf/symbol *interrupt-count*)))) (setq *interrupt-count* 0) (dotimes (i 100) - (sleep (random 1d0)) + (sleep (random 0.1d0)) (interrupt-thread c func)) - (sleep 1) - (assert (= 100 *interrupt-count*)) + (format t "~&waiting for interrupts to arrive~%") + (loop until (= *interrupt-count* 100) do (sleep 0.1)) (terminate-thread c))) (format t "~&interrupt count test done~%") @@ -339,7 +339,7 @@ (loop do (funcall fn) (let ((errno (sb-unix::get-errno))) - (sleep (random 1.0)) + (sleep (random 0.1d0)) (unless (eql errno reference-errno) (format t "Got errno: ~A (~A) instead of ~A~%" errno diff --git a/version.lisp-expr b/version.lisp-expr index 1f4da9a..455cfd4 100644 --- a/version.lisp-expr +++ b/version.lisp-expr @@ -17,4 +17,4 @@ ;;; checkins which aren't released. (And occasionally for internal ;;; versions, especially for internal versions off the main CVS ;;; branch, it gets hairier, e.g. "0.pre7.14.flaky4.13".) -"0.9.3.65" +"0.9.3.66" -- 1.7.10.4