[ruby-core:33456] [Request for Comment] avoid timer thread

[ruby-core:33456] [Request for Comment] avoid timer thread

[SASADA Koichi]

[-- Attachment #1: Type: text/plain, Size: 1058 bytes --]

Hi,

I attached a patch to avoid timer thread.  Your reviews are welcome.

- Timer thread -> Interrupt thread
  - Two tasks
    - For Signal delivery
    - For Guarantee to interrupt ruby threads
  - wake up if needed
  - Communicate with POSIX semaphore.
    - Communicate with Signal handlers
    - Communicate with Ruby threads
  - Rarely wake-up (to reduce power consumption :))

- Ruby thread context switch timing is known by setitimer (SIGVTALRM)
  - set a timer using setitimer() if there is
    at least one  waiting thread
  - disable a timer if there are no waiting threads


Known issues:
  - MacOS X (Darwin Kernel Version 9.8.0) doesn't have sem_timedwait().
    - workaround for MacOSX user:
      Replace sem_timedwait() with usleep(10000).
  - No Windows support
  - stick with some tests on several environments

----
Japanese / にほんご

タイマスレッドを無くすパッチを書いてみました。結局、ヘルパースレッド的な
ものは残っているけれど、あんまり走りません。レビュー頂けると幸いです。

-- 
// SASADA Koichi at atdot dot net

[-- Attachment #2: avoid_timerthread.patch --]
[-- Type: text/plain, Size: 15515 bytes --]

Index: thread_pthread.c
===================================================================
--- thread_pthread.c	(revision 29964)
+++ thread_pthread.c	(working copy)
@@ -11,6 +11,7 @@
 
 #ifdef THREAD_SYSTEM_DEPENDENT_IMPLEMENTATION
 
+#include <semaphore.h>
 #include "gc.h"
 
 #ifdef HAVE_SYS_RESOURCE_H
@@ -48,6 +49,20 @@ gvl_show_waiting_threads(rb_vm_t *vm)
     }
 }
 
+static void
+native_set_timer(long usec)
+{
+    struct itimerval it;
+    int r;
+
+    it.it_interval.tv_sec = 0;
+    it.it_interval.tv_usec = usec; /* 10ms */
+    it.it_value.tv_sec = 0;
+    it.it_value.tv_usec = usec;
+    r = setitimer(ITIMER_VIRTUAL, &it, 0);
+    if (r != 0) rb_bug_errno("setitimer", r);
+}
+
 #if !GVL_SIMPLE_LOCK
 static void
 gvl_waiting_push(rb_vm_t *vm, rb_thread_t *th)
@@ -64,6 +79,11 @@ gvl_waiting_push(rb_vm_t *vm, rb_thread_
     }
     th = vm->gvl.waiting_threads;
     vm->gvl.waiting++;
+
+    if (vm->gvl.waiting == 1) {
+	/* invoke timer */
+	native_set_timer(10000 /* 10ms */);
+    }
 }
 
 static void
@@ -71,6 +91,11 @@ gvl_waiting_shift(rb_vm_t *vm, rb_thread
 {
     vm->gvl.waiting_threads = vm->gvl.waiting_threads->native_thread_data.gvl_next;
     vm->gvl.waiting--;
+
+    if (vm->gvl.waiting == 0) {
+	/* stop timer */
+	native_set_timer(0);
+    }
 }
 #endif
 
@@ -85,7 +110,6 @@ gvl_acquire(rb_vm_t *vm, rb_thread_t *th
 	if (GVL_DEBUG) fprintf(stderr, "gvl acquire (%p): sleep\n", th);
 	gvl_waiting_push(vm, th);
         if (GVL_DEBUG) gvl_show_waiting_threads(vm);
-
 	while (vm->gvl.acquired != 0 || vm->gvl.waiting_threads != th) {
 	    native_cond_wait(&th->native_thread_data.gvl_cond, &vm->gvl.lock);
 	}
@@ -297,9 +321,10 @@ static rb_thread_lock_t signal_thread_li
 static pthread_key_t ruby_native_thread_key;
 
 static void
-null_func(int i)
+vtalrm_handler(int i)
 {
-    /* null */
+    rb_vm_t *vm = GET_VM(); /* TODO: fix me for Multi-VM */
+    RUBY_VM_SET_TIMER_INTERRUPT(vm->running_thread);
 }
 
 static rb_thread_t *
@@ -314,7 +339,14 @@ ruby_thread_set_native(rb_thread_t *th)
     return pthread_setspecific(ruby_native_thread_key, th) == 0;
 }
 
-static void native_thread_init(rb_thread_t *th);
+static void
+native_thread_init(rb_thread_t *th)
+{
+    native_cond_initialize(&th->native_thread_data.sleep_cond);
+    native_cond_initialize(&th->native_thread_data.gvl_cond);
+    ruby_thread_set_native(th);
+    posix_signal(SIGVTALRM, vtalrm_handler);
+}
 
 void
 Init_native_thread(void)
@@ -325,15 +357,6 @@ Init_native_thread(void)
     th->thread_id = pthread_self();
     native_thread_init(th);
     native_mutex_initialize(&signal_thread_list_lock);
-    posix_signal(SIGVTALRM, null_func);
-}
-
-static void
-native_thread_init(rb_thread_t *th)
-{
-    native_cond_initialize(&th->native_thread_data.sleep_cond);
-    native_cond_initialize(&th->native_thread_data.gvl_cond);
-    ruby_thread_set_native(th);
 }
 
 static void
@@ -536,8 +559,6 @@ thread_start_func_1(void *th_ptr)
     return 0;
 }
 
-void rb_thread_create_control_thread(void);
-
 struct cached_thread_entry {
     volatile rb_thread_t **th_area;
     pthread_cond_t *cond;
@@ -816,6 +837,8 @@ struct signal_thread_list {
     struct signal_thread_list *next;
 };
 
+sem_t interrupt_sem;
+
 #ifndef __CYGWIN__
 static struct signal_thread_list signal_thread_list_anchor = {
     0, 0, 0,
@@ -870,6 +893,7 @@ add_signal_thread_list(rb_thread_t *th)
 	    th->native_thread_data.signal_thread_list = list;
 	});
     }
+    sem_post(&interrupt_sem);
 }
 #endif
 
@@ -896,9 +920,9 @@ remove_signal_thread_list(rb_thread_t *t
     }
 }
 
-static pthread_t timer_thread_id;
-static pthread_cond_t timer_thread_cond = PTHREAD_COND_INITIALIZER;
-static pthread_mutex_t timer_thread_lock = PTHREAD_MUTEX_INITIALIZER;
+static pthread_t interrupt_thread_id;
+static pthread_cond_t interrupt_thread_cond = PTHREAD_COND_INITIALIZER;
+static pthread_mutex_t interrupt_thread_lock = PTHREAD_MUTEX_INITIALIZER;
 
 static struct timespec *
 get_ts(struct timespec *ts, unsigned long nsec)
@@ -915,22 +939,40 @@ get_ts(struct timespec *ts, unsigned lon
 }
 
 static void *
-thread_timer(void *dummy)
+interrupt_thread_func(void *dummy)
 {
     struct timespec ts;
 
-    native_mutex_lock(&timer_thread_lock);
-    native_cond_broadcast(&timer_thread_cond);
-#define WAIT_FOR_10MS() native_cond_timedwait(&timer_thread_cond, &timer_thread_lock, get_ts(&ts, PER_NANO/100))
+    /* sync with creater thread */
+    native_mutex_lock(&interrupt_thread_lock);
+    native_cond_broadcast(&interrupt_thread_cond);
+    native_mutex_unlock(&interrupt_thread_lock);
+
     while (system_working > 0) {
-	int err = WAIT_FOR_10MS();
-	if (err == ETIMEDOUT);
-	else if (err == 0 || err == EINTR) {
-	    if (rb_signal_buff_size() == 0) break;
+#if !defined(__CYGWIN__) && !defined(__SYMBIAN32__)
+	if (signal_thread_list_anchor.next) {
+	    struct timespec ts;
+	    sem_timedwait(&interrupt_sem, get_ts(&ts, PER_NANO/100));
+	}
+	else
+#endif
+	  sem_wait(&interrupt_sem);
+
+	switch (errno) {
+	  case EINVAL:
+	  case EDEADLK:
+	   rb_bug_errno("interrupt_thread_core/sem_wait or sem_timedwait", errno);
+	}
+
+	if (rb_signal_buff_size() > 0) {
+	    /* signal deliverly */
+	    rb_vm_t *vm = GET_VM();
+	    rb_threadptr_check_signal(vm->main_thread);
 	}
-	else rb_bug_errno("thread_timer/timedwait", err);
 
 #if !defined(__CYGWIN__) && !defined(__SYMBIAN32__)
+	/* Send a signal to interrupt specific ruby thread */
+	/* to cancel blocking region */
 	if (signal_thread_list_anchor.next) {
 	    FGLOCK(&signal_thread_list_lock, {
 		struct signal_thread_list *list;
@@ -942,59 +984,59 @@ thread_timer(void *dummy)
 	    });
 	}
 #endif
-	timer_thread_function(dummy);
     }
-    native_mutex_unlock(&timer_thread_lock);
     return NULL;
 }
 
-static void
-rb_thread_create_timer_thread(void)
+/* called from signal handler */
+void
+rb_thread_set_signal(void)
 {
-    rb_enable_interrupt();
+    sem_post(&interrupt_sem);
+}
 
-    if (!timer_thread_id) {
+static void
+rb_thread_create_interrupt_thread(void)
+{
+    if (!interrupt_thread_id) {
 	pthread_attr_t attr;
 	int err;
 
+	sem_init(&interrupt_sem, 0, 0);
+
 	pthread_attr_init(&attr);
 #ifdef PTHREAD_STACK_MIN
 	pthread_attr_setstacksize(&attr,
 				  PTHREAD_STACK_MIN + (THREAD_DEBUG ? BUFSIZ : 0));
 #endif
-	native_mutex_lock(&timer_thread_lock);
-	err = pthread_create(&timer_thread_id, &attr, thread_timer, 0);
+	native_mutex_lock(&interrupt_thread_lock);
+	err = pthread_create(&interrupt_thread_id, &attr, interrupt_thread_func, 0);
 	if (err != 0) {
-	    native_mutex_unlock(&timer_thread_lock);
+	    native_mutex_unlock(&interrupt_thread_lock);
 	    fprintf(stderr, "[FATAL] Failed to create timer thread (errno: %d)\n", err);
 	    exit(EXIT_FAILURE);
 	}
-	native_cond_wait(&timer_thread_cond, &timer_thread_lock);
-	native_mutex_unlock(&timer_thread_lock);
+
+	native_cond_wait(&interrupt_thread_cond, &interrupt_thread_lock);
+	native_mutex_unlock(&interrupt_thread_lock);
     }
     rb_disable_interrupt(); /* only timer thread recieve signal */
 }
 
 static int
-native_stop_timer_thread(void)
+native_stop_interrupt_thread(void)
 {
     int stopped;
-    native_mutex_lock(&timer_thread_lock);
     stopped = --system_working <= 0;
-    if (stopped) {
-	native_cond_signal(&timer_thread_cond);
-    }
-    native_mutex_unlock(&timer_thread_lock);
-    if (stopped) {
-	native_thread_join(timer_thread_id);
-    }
+    sem_post(&interrupt_sem);
+    native_thread_join(interrupt_thread_id);
     return stopped;
 }
 
 static void
-native_reset_timer_thread(void)
+native_reset_interrupt_thread(void)
 {
-    timer_thread_id = 0;
+    interrupt_thread_id = 0;
 }
 
 #ifdef HAVE_SIGALTSTACK
Index: vm_core.h
===================================================================
--- vm_core.h	(revision 29961)
+++ vm_core.h	(working copy)
@@ -641,9 +641,10 @@ VALUE rb_vm_make_proc(rb_thread_t *th, c
 VALUE rb_vm_make_env_object(rb_thread_t *th, rb_control_frame_t *cfp);
 void rb_vm_gvl_destroy(rb_vm_t *vm);
 
-void rb_thread_start_timer_thread(void);
-void rb_thread_stop_timer_thread(void);
-void rb_thread_reset_timer_thread(void);
+void rb_thread_start_interrupt_thread(void);
+void rb_thread_stop_interrupt_thread(void);
+void rb_thread_reset_interrupt_thread(void);
+
 void *rb_thread_call_with_gvl(void *(*func)(void *), void *data1);
 int ruby_thread_has_gvl_p(void);
 VALUE rb_make_backtrace(void);
Index: thread.c
===================================================================
--- thread.c	(revision 29961)
+++ thread.c	(working copy)
@@ -193,7 +193,6 @@ rb_thread_s_debug_set(VALUE self, VALUE 
 #endif
 NOINLINE(static int thread_start_func_2(rb_thread_t *th, VALUE *stack_start,
 					VALUE *register_stack_start));
-static void timer_thread_function(void *);
 
 #if   defined(_WIN32)
 #include "thread_win32.c"
@@ -367,7 +366,7 @@ rb_thread_terminate_all(void)
 	}
 	POP_TAG();
     }
-    rb_thread_stop_timer_thread();
+    rb_thread_stop_interrupt_thread();
 }
 
 static void
@@ -2684,48 +2683,25 @@ rb_threadptr_check_signal(rb_thread_t *m
     }
 }
 
-static void
-timer_thread_function(void *arg)
-{
-    rb_vm_t *vm = GET_VM(); /* TODO: fix me for Multi-VM */
-
-    /* for time slice */
-    RUBY_VM_SET_TIMER_INTERRUPT(vm->running_thread);
-
-    /* check signal */
-    rb_threadptr_check_signal(vm->main_thread);
-
-#if 0
-    /* prove profiler */
-    if (vm->prove_profile.enable) {
-	rb_thread_t *th = vm->running_thread;
-
-	if (vm->during_gc) {
-	    /* GC prove profiling */
-	}
-    }
-#endif
-}
-
 void
-rb_thread_stop_timer_thread(void)
+rb_thread_stop_interrupt_thread(void)
 {
-    if (timer_thread_id && native_stop_timer_thread()) {
-	native_reset_timer_thread();
+    if (interrupt_thread_id && native_stop_interrupt_thread()) {
+	native_reset_interrupt_thread();
     }
 }
 
 void
-rb_thread_reset_timer_thread(void)
+rb_thread_reset_interrupt_thread(void)
 {
-    native_reset_timer_thread();
+    native_reset_interrupt_thread();
 }
 
 void
-rb_thread_start_timer_thread(void)
+rb_thread_start_interrupt_thread(void)
 {
     system_working = 1;
-    rb_thread_create_timer_thread();
+    rb_thread_create_interrupt_thread();
 }
 
 static int
@@ -4389,7 +4365,7 @@ Init_Thread(void)
 	}
     }
 
-    rb_thread_create_timer_thread();
+    rb_thread_create_interrupt_thread();
 
     (void)native_mutex_trylock;
 }
Index: eval.c
===================================================================
--- eval.c	(revision 29961)
+++ eval.c	(working copy)
@@ -35,7 +35,6 @@ VALUE rb_eSysStackError;
 /* initialize ruby */
 
 void rb_clear_trace_func(void);
-void rb_thread_stop_timer_thread(void);
 
 void rb_call_inits(void);
 void Init_heap(void);
@@ -118,8 +117,6 @@ ruby_finalize(void)
     ruby_finalize_1();
 }
 
-void rb_thread_stop_timer_thread(void);
-
 int
 ruby_cleanup(volatile int ex)
 {
@@ -160,7 +157,7 @@ ruby_cleanup(volatile int ex)
     ex = error_handle(ex);
     ruby_finalize_1();
     POP_TAG();
-    rb_thread_stop_timer_thread();
+    rb_thread_stop_interrupt_thread();
 
 #if EXIT_SUCCESS != 0 || EXIT_FAILURE != 1
     switch (ex) {
Index: process.c
===================================================================
--- process.c	(revision 29961)
+++ process.c	(working copy)
@@ -989,16 +989,12 @@ proc_detach(VALUE obj, VALUE pid)
 char *strtok();
 #endif
 
-void rb_thread_stop_timer_thread(void);
-void rb_thread_start_timer_thread(void);
-void rb_thread_reset_timer_thread(void);
-
 static int forked_child = 0;
 
 #define before_exec() \
-    (rb_enable_interrupt(), (void)(forked_child ? 0 : (rb_thread_stop_timer_thread(), 1)))
+    (rb_enable_interrupt(), (void)(forked_child ? 0 : (rb_thread_stop_interrupt_thread(), 1)))
 #define after_exec() \
-  (rb_thread_reset_timer_thread(), rb_thread_start_timer_thread(), forked_child = 0, rb_disable_interrupt())
+  (rb_thread_reset_interrupt_thread(), rb_thread_start_interrupt_thread(), forked_child = 0, rb_disable_interrupt())
 #define before_fork() before_exec()
 #define after_fork() (GET_THREAD()->thrown_errinfo = 0, after_exec())
 
Index: signal.c
===================================================================
--- signal.c	(revision 29961)
+++ signal.c	(working copy)
@@ -522,11 +522,15 @@ ruby_nativethread_signal(int signum, sig
 #endif
 #endif
 
+void rb_thread_set_signal(void);
+
 static RETSIGTYPE
 sighandler(int sig)
 {
     ATOMIC_INC(signal_buff.cnt[sig]);
     ATOMIC_INC(signal_buff.size);
+    rb_thread_set_signal();
+
 #if !defined(BSD_SIGNAL) && !defined(POSIX_SIGNAL)
     ruby_signal(sig, sighandler);
 #endif
@@ -556,8 +560,6 @@ rb_disable_interrupt(void)
 #if USE_TRAP_MASK
     sigset_t mask;
     sigfillset(&mask);
-    sigdelset(&mask, SIGVTALRM);
-    sigdelset(&mask, SIGSEGV);
     pthread_sigmask(SIG_SETMASK, &mask, NULL);
 #endif
 }
@@ -1057,6 +1059,44 @@ int ruby_enable_coredump = 0;
 #define ruby_enable_coredump 0
 #endif
 
+#if HAVE_PTHREAD
+#include <pthread.h>
+#include <signal.h>
+
+static VALUE
+sigset2hash(sigset_t *set)
+{
+    VALUE hash = rb_hash_new();
+    const struct signals *sigs;
+
+    for (sigs = siglist; sigs->signm; sigs++) {
+	if (sigs->signo == 0) continue;
+	rb_hash_aset(hash, rb_str_new2(sigs->signm), sigismember(set, sigs->signo) ? Qtrue : Qfalse);
+	fprintf(stderr, "signal: SIG%-6s is %d\n", sigs->signm, sigismember(set, sigs->signo));
+    }
+
+    return hash;
+}
+
+static VALUE
+sigmask_of_current_thread(VALUE klass)
+{
+    sigset_t oldset;
+    int r;
+    r = pthread_sigmask(0, NULL, &oldset);
+    if (r != 0) {rb_bug_errno("pthread_sigmask", r);}
+    return sigset2hash(&oldset);
+}
+
+static VALUE
+sigmask_of_current_process(VALUE klass)
+{
+    sigset_t oldset;
+    sigprocmask(0, NULL, &oldset);
+    return sigset2hash(&oldset);
+}
+#endif
+
 /*
  * Many operating systems allow signals to be sent to running
  * processes. Some signals have a defined effect on the process, while
@@ -1102,6 +1142,11 @@ Init_signal(void)
     rb_define_global_function("trap", sig_trap, -1);
     rb_define_module_function(mSignal, "trap", sig_trap, -1);
     rb_define_module_function(mSignal, "list", sig_list, 0);
+
+#if HAVE_PTHREAD
+    rb_define_module_function(mSignal, "sigmask_of_current_thread", sigmask_of_current_thread, 0);
+    rb_define_module_function(mSignal, "sigmask_of_current_process", sigmask_of_current_process, 0);
+#endif
 
     rb_define_method(rb_eSignal, "initialize", esignal_init, -1);
     rb_define_method(rb_eSignal, "signo", esignal_signo, 0);
Mon Nov 29 11:38:42 2010  Koichi Sasada  <ko1@atdot.net>

	* eval.c: 

	* eval.c (ruby_cleanup): 

	* eval.c (ruby_finalize): 

	* process.c: 

	* process.c (proc_detach): 

	* signal.c: 

	* signal.c (Init_signal): 

	* signal.c (rb_disable_interrupt): 

	* signal.c (ruby_nativethread_signal): 

	* thread.c: 

	* thread.c (Init_Thread): 

	* thread.c (rb_thread_s_debug_set): 

	* thread.c (rb_thread_terminate_all): 

	* thread.c (rb_threadptr_check_signal): 

	* thread_pthread.c: 

	* thread_pthread.c (Init_native_thread): 

	* thread_pthread.c (add_signal_thread_list): 

	* thread_pthread.c (get_ts): 

	* thread_pthread.c (gvl_acquire): 

	* thread_pthread.c (gvl_show_waiting_threads): 

	* thread_pthread.c (gvl_waiting_push): 

	* thread_pthread.c (gvl_waiting_shift): 

	* thread_pthread.c (remove_signal_thread_list): 

	* thread_pthread.c (ruby_thread_set_native): 

	* thread_pthread.c (thread_start_func_1): 

	* thread_pthread.c (thread_timer): 

	* vm_core.h: 

	* vm_core.h (rb_vm_make_proc): 

[ruby-core:35152] Re: [Request for Comment] avoid timer thread

[Mark Somerville]

[-- Attachment #1: Type: text/plain, Size: 20806 bytes --]

On Mon, Nov 29, 2010 at 11:53:03AM +0900, SASADA Koichi wrote:
> I attached a patch to avoid timer thread.  Your reviews are welcome.

I *finally* got around to testing this patch.

I applied this to trunk (it doesn't quite apply cleanly, but is easy to
fix). All the Ruby tests pass. I also ran the tests in all of my 1.9
compatible projects - everything was happy!

Most importantly, it fixes the wakeups/context switching problem nicely!

My results of running `ruby -e gets` are below (x86_64, Fedora 14):


Trunk without patch:
===

$ powertop

Top causes for wakeups:
  33.4% (113.6)   ruby
  15.7% ( 53.4)   USB device  3-2 : Optical USB Mouse (Logitech)
  14.8% ( 50.4)   USB device  2-4 : USB Storage (Generic )
   9.9% ( 33.6)   [ehci_hcd:usb2, uhci_hcd:usb6] <interrupt>
   9.5% ( 32.2)   [uhci_hcd:usb3] <interrupt>
   4.1% ( 14.0)   [kernel core] hrtimer_start (tick_sched_timer)
   2.7% (  9.2)   [kernel core] __mod_timer (rh_timer_func)
   2.6% (  8.8)   [ahci] <interrupt>
   1.5% (  5.0)   [kernel scheduler] Load balancing tick
   1.3% (  4.4)   Xorg


Average number of context switches per second: 410


Trunk with the patch applied:
===

$ powertop

Top causes for wakeups:
  27.7% ( 59.0)   [uhci_hcd:usb3] <interrupt>
  27.4% ( 58.3)   USB device  3-2 : Optical USB Mouse (Logitech)
  13.2% ( 28.0)   [ehci_hcd:usb2, uhci_hcd:usb6] <interrupt>
   9.9% ( 21.0)   USB device  2-4 : USB Storage (Generic )
   3.9% (  8.3)   [kernel scheduler] Load balancing tick
   3.9% (  8.3)   [kernel core] hrtimer_start (tick_sched_timer)
   3.8% (  8.0)   [kernel core] __mod_timer (rh_timer_func)
   3.3% (  7.0)   [ahci] <interrupt>
   1.1% (  2.3)   gnome-terminal


Average number of context switches per second: 219


It's easy to understand these results by running strace -f on the Ruby
processes.

> - Timer thread -> Interrupt thread
>   - Two tasks
>     - For Signal delivery
>     - For Guarantee to interrupt ruby threads
>   - wake up if needed
>   - Communicate with POSIX semaphore.
>     - Communicate with Signal handlers
>     - Communicate with Ruby threads
>   - Rarely wake-up (to reduce power consumption :))
> 
> - Ruby thread context switch timing is known by setitimer (SIGVTALRM)
>   - set a timer using setitimer() if there is
>     at least one  waiting thread
>   - disable a timer if there are no waiting threads

I'm probably not the best qualified person for a full review (you
spotted problems with my patch for this issue that I hadn't even
considered), but it seems like a clean and not too invasive solution.

I can't see any immediately obvious problems.

> Known issues:
>   - MacOS X (Darwin Kernel Version 9.8.0) doesn't have sem_timedwait().
>     - workaround for MacOSX user:
>       Replace sem_timedwait() with usleep(10000).
>   - No Windows support

What would happen in this case? Is there any way to emulate the
behaviour on Windows? If not, would we fallback to using the existing
timer thread on that platform?

>   - stick with some tests on several environments

Do you mean Ruby interpreter tests (`make test`) or tests that are
written for Ruby projects?

Really nice work Koichi!

> ----
> Japanese / にほんご
> 
> タイマスレッドを無くすパッチを書いてみました。結局、ヘルパースレッド的な
> ものは残っているけれど、あんまり走りません。レビュー頂けると幸いです。
> 
> -- 
> // SASADA Koichi at atdot dot net

> Index: thread_pthread.c
> ===================================================================
> --- thread_pthread.c	(revision 29964)
> +++ thread_pthread.c	(working copy)
> @@ -11,6 +11,7 @@
>  
>  #ifdef THREAD_SYSTEM_DEPENDENT_IMPLEMENTATION
>  
> +#include <semaphore.h>
>  #include "gc.h"
>  
>  #ifdef HAVE_SYS_RESOURCE_H
> @@ -48,6 +49,20 @@ gvl_show_waiting_threads(rb_vm_t *vm)
>      }
>  }
>  
> +static void
> +native_set_timer(long usec)
> +{
> +    struct itimerval it;
> +    int r;
> +
> +    it.it_interval.tv_sec = 0;
> +    it.it_interval.tv_usec = usec; /* 10ms */
> +    it.it_value.tv_sec = 0;
> +    it.it_value.tv_usec = usec;
> +    r = setitimer(ITIMER_VIRTUAL, &it, 0);
> +    if (r != 0) rb_bug_errno("setitimer", r);
> +}
> +
>  #if !GVL_SIMPLE_LOCK
>  static void
>  gvl_waiting_push(rb_vm_t *vm, rb_thread_t *th)
> @@ -64,6 +79,11 @@ gvl_waiting_push(rb_vm_t *vm, rb_thread_
>      }
>      th = vm->gvl.waiting_threads;
>      vm->gvl.waiting++;
> +
> +    if (vm->gvl.waiting == 1) {
> +	/* invoke timer */
> +	native_set_timer(10000 /* 10ms */);
> +    }
>  }
>  
>  static void
> @@ -71,6 +91,11 @@ gvl_waiting_shift(rb_vm_t *vm, rb_thread
>  {
>      vm->gvl.waiting_threads = vm->gvl.waiting_threads->native_thread_data.gvl_next;
>      vm->gvl.waiting--;
> +
> +    if (vm->gvl.waiting == 0) {
> +	/* stop timer */
> +	native_set_timer(0);
> +    }
>  }
>  #endif
>  
> @@ -85,7 +110,6 @@ gvl_acquire(rb_vm_t *vm, rb_thread_t *th
>  	if (GVL_DEBUG) fprintf(stderr, "gvl acquire (%p): sleep\n", th);
>  	gvl_waiting_push(vm, th);
>          if (GVL_DEBUG) gvl_show_waiting_threads(vm);
> -
>  	while (vm->gvl.acquired != 0 || vm->gvl.waiting_threads != th) {
>  	    native_cond_wait(&th->native_thread_data.gvl_cond, &vm->gvl.lock);
>  	}
> @@ -297,9 +321,10 @@ static rb_thread_lock_t signal_thread_li
>  static pthread_key_t ruby_native_thread_key;
>  
>  static void
> -null_func(int i)
> +vtalrm_handler(int i)
>  {
> -    /* null */
> +    rb_vm_t *vm = GET_VM(); /* TODO: fix me for Multi-VM */
> +    RUBY_VM_SET_TIMER_INTERRUPT(vm->running_thread);
>  }
>  
>  static rb_thread_t *
> @@ -314,7 +339,14 @@ ruby_thread_set_native(rb_thread_t *th)
>      return pthread_setspecific(ruby_native_thread_key, th) == 0;
>  }
>  
> -static void native_thread_init(rb_thread_t *th);
> +static void
> +native_thread_init(rb_thread_t *th)
> +{
> +    native_cond_initialize(&th->native_thread_data.sleep_cond);
> +    native_cond_initialize(&th->native_thread_data.gvl_cond);
> +    ruby_thread_set_native(th);
> +    posix_signal(SIGVTALRM, vtalrm_handler);
> +}
>  
>  void
>  Init_native_thread(void)
> @@ -325,15 +357,6 @@ Init_native_thread(void)
>      th->thread_id = pthread_self();
>      native_thread_init(th);
>      native_mutex_initialize(&signal_thread_list_lock);
> -    posix_signal(SIGVTALRM, null_func);
> -}
> -
> -static void
> -native_thread_init(rb_thread_t *th)
> -{
> -    native_cond_initialize(&th->native_thread_data.sleep_cond);
> -    native_cond_initialize(&th->native_thread_data.gvl_cond);
> -    ruby_thread_set_native(th);
>  }
>  
>  static void
> @@ -536,8 +559,6 @@ thread_start_func_1(void *th_ptr)
>      return 0;
>  }
>  
> -void rb_thread_create_control_thread(void);
> -
>  struct cached_thread_entry {
>      volatile rb_thread_t **th_area;
>      pthread_cond_t *cond;
> @@ -816,6 +837,8 @@ struct signal_thread_list {
>      struct signal_thread_list *next;
>  };
>  
> +sem_t interrupt_sem;
> +
>  #ifndef __CYGWIN__
>  static struct signal_thread_list signal_thread_list_anchor = {
>      0, 0, 0,
> @@ -870,6 +893,7 @@ add_signal_thread_list(rb_thread_t *th)
>  	    th->native_thread_data.signal_thread_list = list;
>  	});
>      }
> +    sem_post(&interrupt_sem);
>  }
>  #endif
>  
> @@ -896,9 +920,9 @@ remove_signal_thread_list(rb_thread_t *t
>      }
>  }
>  
> -static pthread_t timer_thread_id;
> -static pthread_cond_t timer_thread_cond = PTHREAD_COND_INITIALIZER;
> -static pthread_mutex_t timer_thread_lock = PTHREAD_MUTEX_INITIALIZER;
> +static pthread_t interrupt_thread_id;
> +static pthread_cond_t interrupt_thread_cond = PTHREAD_COND_INITIALIZER;
> +static pthread_mutex_t interrupt_thread_lock = PTHREAD_MUTEX_INITIALIZER;
>  
>  static struct timespec *
>  get_ts(struct timespec *ts, unsigned long nsec)
> @@ -915,22 +939,40 @@ get_ts(struct timespec *ts, unsigned lon
>  }
>  
>  static void *
> -thread_timer(void *dummy)
> +interrupt_thread_func(void *dummy)
>  {
>      struct timespec ts;
>  
> -    native_mutex_lock(&timer_thread_lock);
> -    native_cond_broadcast(&timer_thread_cond);
> -#define WAIT_FOR_10MS() native_cond_timedwait(&timer_thread_cond, &timer_thread_lock, get_ts(&ts, PER_NANO/100))
> +    /* sync with creater thread */
> +    native_mutex_lock(&interrupt_thread_lock);
> +    native_cond_broadcast(&interrupt_thread_cond);
> +    native_mutex_unlock(&interrupt_thread_lock);
> +
>      while (system_working > 0) {
> -	int err = WAIT_FOR_10MS();
> -	if (err == ETIMEDOUT);
> -	else if (err == 0 || err == EINTR) {
> -	    if (rb_signal_buff_size() == 0) break;
> +#if !defined(__CYGWIN__) && !defined(__SYMBIAN32__)
> +	if (signal_thread_list_anchor.next) {
> +	    struct timespec ts;
> +	    sem_timedwait(&interrupt_sem, get_ts(&ts, PER_NANO/100));
> +	}
> +	else
> +#endif
> +	  sem_wait(&interrupt_sem);
> +
> +	switch (errno) {
> +	  case EINVAL:
> +	  case EDEADLK:
> +	   rb_bug_errno("interrupt_thread_core/sem_wait or sem_timedwait", errno);
> +	}
> +
> +	if (rb_signal_buff_size() > 0) {
> +	    /* signal deliverly */
> +	    rb_vm_t *vm = GET_VM();
> +	    rb_threadptr_check_signal(vm->main_thread);
>  	}
> -	else rb_bug_errno("thread_timer/timedwait", err);
>  
>  #if !defined(__CYGWIN__) && !defined(__SYMBIAN32__)
> +	/* Send a signal to interrupt specific ruby thread */
> +	/* to cancel blocking region */
>  	if (signal_thread_list_anchor.next) {
>  	    FGLOCK(&signal_thread_list_lock, {
>  		struct signal_thread_list *list;
> @@ -942,59 +984,59 @@ thread_timer(void *dummy)
>  	    });
>  	}
>  #endif
> -	timer_thread_function(dummy);
>      }
> -    native_mutex_unlock(&timer_thread_lock);
>      return NULL;
>  }
>  
> -static void
> -rb_thread_create_timer_thread(void)
> +/* called from signal handler */
> +void
> +rb_thread_set_signal(void)
>  {
> -    rb_enable_interrupt();
> +    sem_post(&interrupt_sem);
> +}
>  
> -    if (!timer_thread_id) {
> +static void
> +rb_thread_create_interrupt_thread(void)
> +{
> +    if (!interrupt_thread_id) {
>  	pthread_attr_t attr;
>  	int err;
>  
> +	sem_init(&interrupt_sem, 0, 0);
> +
>  	pthread_attr_init(&attr);
>  #ifdef PTHREAD_STACK_MIN
>  	pthread_attr_setstacksize(&attr,
>  				  PTHREAD_STACK_MIN + (THREAD_DEBUG ? BUFSIZ : 0));
>  #endif
> -	native_mutex_lock(&timer_thread_lock);
> -	err = pthread_create(&timer_thread_id, &attr, thread_timer, 0);
> +	native_mutex_lock(&interrupt_thread_lock);
> +	err = pthread_create(&interrupt_thread_id, &attr, interrupt_thread_func, 0);
>  	if (err != 0) {
> -	    native_mutex_unlock(&timer_thread_lock);
> +	    native_mutex_unlock(&interrupt_thread_lock);
>  	    fprintf(stderr, "[FATAL] Failed to create timer thread (errno: %d)\n", err);
>  	    exit(EXIT_FAILURE);
>  	}
> -	native_cond_wait(&timer_thread_cond, &timer_thread_lock);
> -	native_mutex_unlock(&timer_thread_lock);
> +
> +	native_cond_wait(&interrupt_thread_cond, &interrupt_thread_lock);
> +	native_mutex_unlock(&interrupt_thread_lock);
>      }
>      rb_disable_interrupt(); /* only timer thread recieve signal */
>  }
>  
>  static int
> -native_stop_timer_thread(void)
> +native_stop_interrupt_thread(void)
>  {
>      int stopped;
> -    native_mutex_lock(&timer_thread_lock);
>      stopped = --system_working <= 0;
> -    if (stopped) {
> -	native_cond_signal(&timer_thread_cond);
> -    }
> -    native_mutex_unlock(&timer_thread_lock);
> -    if (stopped) {
> -	native_thread_join(timer_thread_id);
> -    }
> +    sem_post(&interrupt_sem);
> +    native_thread_join(interrupt_thread_id);
>      return stopped;
>  }
>  
>  static void
> -native_reset_timer_thread(void)
> +native_reset_interrupt_thread(void)
>  {
> -    timer_thread_id = 0;
> +    interrupt_thread_id = 0;
>  }
>  
>  #ifdef HAVE_SIGALTSTACK
> Index: vm_core.h
> ===================================================================
> --- vm_core.h	(revision 29961)
> +++ vm_core.h	(working copy)
> @@ -641,9 +641,10 @@ VALUE rb_vm_make_proc(rb_thread_t *th, c
>  VALUE rb_vm_make_env_object(rb_thread_t *th, rb_control_frame_t *cfp);
>  void rb_vm_gvl_destroy(rb_vm_t *vm);
>  
> -void rb_thread_start_timer_thread(void);
> -void rb_thread_stop_timer_thread(void);
> -void rb_thread_reset_timer_thread(void);
> +void rb_thread_start_interrupt_thread(void);
> +void rb_thread_stop_interrupt_thread(void);
> +void rb_thread_reset_interrupt_thread(void);
> +
>  void *rb_thread_call_with_gvl(void *(*func)(void *), void *data1);
>  int ruby_thread_has_gvl_p(void);
>  VALUE rb_make_backtrace(void);
> Index: thread.c
> ===================================================================
> --- thread.c	(revision 29961)
> +++ thread.c	(working copy)
> @@ -193,7 +193,6 @@ rb_thread_s_debug_set(VALUE self, VALUE 
>  #endif
>  NOINLINE(static int thread_start_func_2(rb_thread_t *th, VALUE *stack_start,
>  					VALUE *register_stack_start));
> -static void timer_thread_function(void *);
>  
>  #if   defined(_WIN32)
>  #include "thread_win32.c"
> @@ -367,7 +366,7 @@ rb_thread_terminate_all(void)
>  	}
>  	POP_TAG();
>      }
> -    rb_thread_stop_timer_thread();
> +    rb_thread_stop_interrupt_thread();
>  }
>  
>  static void
> @@ -2684,48 +2683,25 @@ rb_threadptr_check_signal(rb_thread_t *m
>      }
>  }
>  
> -static void
> -timer_thread_function(void *arg)
> -{
> -    rb_vm_t *vm = GET_VM(); /* TODO: fix me for Multi-VM */
> -
> -    /* for time slice */
> -    RUBY_VM_SET_TIMER_INTERRUPT(vm->running_thread);
> -
> -    /* check signal */
> -    rb_threadptr_check_signal(vm->main_thread);
> -
> -#if 0
> -    /* prove profiler */
> -    if (vm->prove_profile.enable) {
> -	rb_thread_t *th = vm->running_thread;
> -
> -	if (vm->during_gc) {
> -	    /* GC prove profiling */
> -	}
> -    }
> -#endif
> -}
> -
>  void
> -rb_thread_stop_timer_thread(void)
> +rb_thread_stop_interrupt_thread(void)
>  {
> -    if (timer_thread_id && native_stop_timer_thread()) {
> -	native_reset_timer_thread();
> +    if (interrupt_thread_id && native_stop_interrupt_thread()) {
> +	native_reset_interrupt_thread();
>      }
>  }
>  
>  void
> -rb_thread_reset_timer_thread(void)
> +rb_thread_reset_interrupt_thread(void)
>  {
> -    native_reset_timer_thread();
> +    native_reset_interrupt_thread();
>  }
>  
>  void
> -rb_thread_start_timer_thread(void)
> +rb_thread_start_interrupt_thread(void)
>  {
>      system_working = 1;
> -    rb_thread_create_timer_thread();
> +    rb_thread_create_interrupt_thread();
>  }
>  
>  static int
> @@ -4389,7 +4365,7 @@ Init_Thread(void)
>  	}
>      }
>  
> -    rb_thread_create_timer_thread();
> +    rb_thread_create_interrupt_thread();
>  
>      (void)native_mutex_trylock;
>  }
> Index: eval.c
> ===================================================================
> --- eval.c	(revision 29961)
> +++ eval.c	(working copy)
> @@ -35,7 +35,6 @@ VALUE rb_eSysStackError;
>  /* initialize ruby */
>  
>  void rb_clear_trace_func(void);
> -void rb_thread_stop_timer_thread(void);
>  
>  void rb_call_inits(void);
>  void Init_heap(void);
> @@ -118,8 +117,6 @@ ruby_finalize(void)
>      ruby_finalize_1();
>  }
>  
> -void rb_thread_stop_timer_thread(void);
> -
>  int
>  ruby_cleanup(volatile int ex)
>  {
> @@ -160,7 +157,7 @@ ruby_cleanup(volatile int ex)
>      ex = error_handle(ex);
>      ruby_finalize_1();
>      POP_TAG();
> -    rb_thread_stop_timer_thread();
> +    rb_thread_stop_interrupt_thread();
>  
>  #if EXIT_SUCCESS != 0 || EXIT_FAILURE != 1
>      switch (ex) {
> Index: process.c
> ===================================================================
> --- process.c	(revision 29961)
> +++ process.c	(working copy)
> @@ -989,16 +989,12 @@ proc_detach(VALUE obj, VALUE pid)
>  char *strtok();
>  #endif
>  
> -void rb_thread_stop_timer_thread(void);
> -void rb_thread_start_timer_thread(void);
> -void rb_thread_reset_timer_thread(void);
> -
>  static int forked_child = 0;
>  
>  #define before_exec() \
> -    (rb_enable_interrupt(), (void)(forked_child ? 0 : (rb_thread_stop_timer_thread(), 1)))
> +    (rb_enable_interrupt(), (void)(forked_child ? 0 : (rb_thread_stop_interrupt_thread(), 1)))
>  #define after_exec() \
> -  (rb_thread_reset_timer_thread(), rb_thread_start_timer_thread(), forked_child = 0, rb_disable_interrupt())
> +  (rb_thread_reset_interrupt_thread(), rb_thread_start_interrupt_thread(), forked_child = 0, rb_disable_interrupt())
>  #define before_fork() before_exec()
>  #define after_fork() (GET_THREAD()->thrown_errinfo = 0, after_exec())
>  
> Index: signal.c
> ===================================================================
> --- signal.c	(revision 29961)
> +++ signal.c	(working copy)
> @@ -522,11 +522,15 @@ ruby_nativethread_signal(int signum, sig
>  #endif
>  #endif
>  
> +void rb_thread_set_signal(void);
> +
>  static RETSIGTYPE
>  sighandler(int sig)
>  {
>      ATOMIC_INC(signal_buff.cnt[sig]);
>      ATOMIC_INC(signal_buff.size);
> +    rb_thread_set_signal();
> +
>  #if !defined(BSD_SIGNAL) && !defined(POSIX_SIGNAL)
>      ruby_signal(sig, sighandler);
>  #endif
> @@ -556,8 +560,6 @@ rb_disable_interrupt(void)
>  #if USE_TRAP_MASK
>      sigset_t mask;
>      sigfillset(&mask);
> -    sigdelset(&mask, SIGVTALRM);
> -    sigdelset(&mask, SIGSEGV);
>      pthread_sigmask(SIG_SETMASK, &mask, NULL);
>  #endif
>  }
> @@ -1057,6 +1059,44 @@ int ruby_enable_coredump = 0;
>  #define ruby_enable_coredump 0
>  #endif
>  
> +#if HAVE_PTHREAD
> +#include <pthread.h>
> +#include <signal.h>
> +
> +static VALUE
> +sigset2hash(sigset_t *set)
> +{
> +    VALUE hash = rb_hash_new();
> +    const struct signals *sigs;
> +
> +    for (sigs = siglist; sigs->signm; sigs++) {
> +	if (sigs->signo == 0) continue;
> +	rb_hash_aset(hash, rb_str_new2(sigs->signm), sigismember(set, sigs->signo) ? Qtrue : Qfalse);
> +	fprintf(stderr, "signal: SIG%-6s is %d\n", sigs->signm, sigismember(set, sigs->signo));
> +    }
> +
> +    return hash;
> +}
> +
> +static VALUE
> +sigmask_of_current_thread(VALUE klass)
> +{
> +    sigset_t oldset;
> +    int r;
> +    r = pthread_sigmask(0, NULL, &oldset);
> +    if (r != 0) {rb_bug_errno("pthread_sigmask", r);}
> +    return sigset2hash(&oldset);
> +}
> +
> +static VALUE
> +sigmask_of_current_process(VALUE klass)
> +{
> +    sigset_t oldset;
> +    sigprocmask(0, NULL, &oldset);
> +    return sigset2hash(&oldset);
> +}
> +#endif
> +
>  /*
>   * Many operating systems allow signals to be sent to running
>   * processes. Some signals have a defined effect on the process, while
> @@ -1102,6 +1142,11 @@ Init_signal(void)
>      rb_define_global_function("trap", sig_trap, -1);
>      rb_define_module_function(mSignal, "trap", sig_trap, -1);
>      rb_define_module_function(mSignal, "list", sig_list, 0);
> +
> +#if HAVE_PTHREAD
> +    rb_define_module_function(mSignal, "sigmask_of_current_thread", sigmask_of_current_thread, 0);
> +    rb_define_module_function(mSignal, "sigmask_of_current_process", sigmask_of_current_process, 0);
> +#endif
>  
>      rb_define_method(rb_eSignal, "initialize", esignal_init, -1);
>      rb_define_method(rb_eSignal, "signo", esignal_signo, 0);
> Mon Nov 29 11:38:42 2010  Koichi Sasada  <ko1@atdot.net>
> 
> 	* eval.c: 
> 
> 	* eval.c (ruby_cleanup): 
> 
> 	* eval.c (ruby_finalize): 
> 
> 	* process.c: 
> 
> 	* process.c (proc_detach): 
> 
> 	* signal.c: 
> 
> 	* signal.c (Init_signal): 
> 
> 	* signal.c (rb_disable_interrupt): 
> 
> 	* signal.c (ruby_nativethread_signal): 
> 
> 	* thread.c: 
> 
> 	* thread.c (Init_Thread): 
> 
> 	* thread.c (rb_thread_s_debug_set): 
> 
> 	* thread.c (rb_thread_terminate_all): 
> 
> 	* thread.c (rb_threadptr_check_signal): 
> 
> 	* thread_pthread.c: 
> 
> 	* thread_pthread.c (Init_native_thread): 
> 
> 	* thread_pthread.c (add_signal_thread_list): 
> 
> 	* thread_pthread.c (get_ts): 
> 
> 	* thread_pthread.c (gvl_acquire): 
> 
> 	* thread_pthread.c (gvl_show_waiting_threads): 
> 
> 	* thread_pthread.c (gvl_waiting_push): 
> 
> 	* thread_pthread.c (gvl_waiting_shift): 
> 
> 	* thread_pthread.c (remove_signal_thread_list): 
> 
> 	* thread_pthread.c (ruby_thread_set_native): 
> 
> 	* thread_pthread.c (thread_start_func_1): 
> 
> 	* thread_pthread.c (thread_timer): 
> 
> 	* vm_core.h: 
> 
> 	* vm_core.h (rb_vm_make_proc): 
> 


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[ruby-core:36077] Re: [Request for Comment] avoid timer thread

[Mark Somerville]

[-- Attachment #1: Type: text/plain, Size: 746 bytes --]

On Tue, Feb 08, 2011 at 09:24:13PM +0900, Mark Somerville wrote:
> On Mon, Nov 29, 2010 at 11:53:03AM +0900, SASADA Koichi wrote:
> > I attached a patch to avoid timer thread.  Your reviews are welcome.
> 
> I *finally* got around to testing this patch.
> 
> I applied this to trunk (it doesn't quite apply cleanly, but is easy to
> fix). All the Ruby tests pass. I also ran the tests in all of my 1.9
> compatible projects - everything was happy!
> 
> Most importantly, it fixes the wakeups/context switching problem nicely!

Has anyone else looked at this patch yet?

It would be great to get this (or something like it) into 1.9.3.

I consider this a pretty bad regression from 1.8. I don't want to have
to use 1.8 any more!

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[ruby-core:36952] Re: [Request for Comment] avoid timer thread

[Eric Wong]

Mark Somerville <mark@scottishclimbs.com> wrote:
> On Tue, Feb 08, 2011 at 09:24:13PM +0900, Mark Somerville wrote:
> > On Mon, Nov 29, 2010 at 11:53:03AM +0900, SASADA Koichi wrote:
> > > I attached a patch to avoid timer thread.  Your reviews are welcome.
> > 
> > I *finally* got around to testing this patch.
> > 
> > I applied this to trunk (it doesn't quite apply cleanly, but is easy to
> > fix). All the Ruby tests pass. I also ran the tests in all of my 1.9
> > compatible projects - everything was happy!
> > 
> > Most importantly, it fixes the wakeups/context switching problem nicely!
> 
> Has anyone else looked at this patch yet?

Mind reposting your updated patch?  ML archives mangled the original
one.  I can host a branch for it on git://bogomips.org/ruby.git too

> It would be great to get this (or something like it) into 1.9.3.
> 
> I consider this a pretty bad regression from 1.8. I don't want to have
> to use 1.8 any more!

I completely agree.  I plan on running Ruby 1.9 daemons on my laptop
more; so having this in 1.9.3 would be great.

-- 
Eric Wong

[ruby-core:37080] Re: [Request for Comment] avoid timer thread

[Mark Somerville]

[-- Attachment #1.1: Type: text/plain, Size: 1666 bytes --]

On Sat, Jun 11, 2011 at 05:57:11AM +0900, Eric Wong wrote:
> Mark Somerville <mark@scottishclimbs.com> wrote:
> > On Tue, Feb 08, 2011 at 09:24:13PM +0900, Mark Somerville wrote:
> > > On Mon, Nov 29, 2010 at 11:53:03AM +0900, SASADA Koichi wrote:
> > > > I attached a patch to avoid timer thread.  Your reviews are welcome.
> > > 
> > > I *finally* got around to testing this patch.
> > > 
> > > I applied this to trunk (it doesn't quite apply cleanly, but is easy to
> > > fix). All the Ruby tests pass. I also ran the tests in all of my 1.9
> > > compatible projects - everything was happy!
> > > 
> > > Most importantly, it fixes the wakeups/context switching problem nicely!
> > 
> > Has anyone else looked at this patch yet?
> 
> Mind reposting your updated patch?  ML archives mangled the original
> one.  I can host a branch for it on git://bogomips.org/ruby.git too

The patch I was testing was Koichi's. My earlier patch for this problem
was shown to have problems.

I've attached Koichi's patch again. Unfortunately it doesn't apply
cleanly to master and I haven't got the experience/time needed to get it
to a working state.

If you or someone else does, I'll be very happy to test it out or help
in any other way I can.
 
> > It would be great to get this (or something like it) into 1.9.3.
> > 
> > I consider this a pretty bad regression from 1.8. I don't want to have
> > to use 1.8 any more!
> 
> I completely agree.  I plan on running Ruby 1.9 daemons on my laptop
> more; so having this in 1.9.3 would be great.

I'm in the same position - wanting to run tens of Ruby processes for a
long time on my laptop.

[-- Attachment #1.2: avoid_timerthread.patch --]
[-- Type: text/plain, Size: 16085 bytes --]

Index: thread_pthread.c
===================================================================
--- thread_pthread.c	(revision 29964)
+++ thread_pthread.c	(working copy)
@@ -11,6 +11,7 @@
 
 #ifdef THREAD_SYSTEM_DEPENDENT_IMPLEMENTATION
 
+#include <semaphore.h>
 #include "gc.h"
 
 #ifdef HAVE_SYS_RESOURCE_H
@@ -48,6 +49,20 @@ gvl_show_waiting_threads(rb_vm_t *vm)
     }
 }
 
+static void
+native_set_timer(long usec)
+{
+    struct itimerval it;
+    int r;
+
+    it.it_interval.tv_sec = 0;
+    it.it_interval.tv_usec = usec; /* 10ms */
+    it.it_value.tv_sec = 0;
+    it.it_value.tv_usec = usec;
+    r = setitimer(ITIMER_VIRTUAL, &it, 0);
+    if (r != 0) rb_bug_errno("setitimer", r);
+}
+
 #if !GVL_SIMPLE_LOCK
 static void
 gvl_waiting_push(rb_vm_t *vm, rb_thread_t *th)
@@ -64,6 +79,11 @@ gvl_waiting_push(rb_vm_t *vm, rb_thread_
     }
     th = vm->gvl.waiting_threads;
     vm->gvl.waiting++;
+
+    if (vm->gvl.waiting == 1) {
+	/* invoke timer */
+	native_set_timer(10000 /* 10ms */);
+    }
 }
 
 static void
@@ -71,6 +91,11 @@ gvl_waiting_shift(rb_vm_t *vm, rb_thread
 {
     vm->gvl.waiting_threads = vm->gvl.waiting_threads->native_thread_data.gvl_next;
     vm->gvl.waiting--;
+
+    if (vm->gvl.waiting == 0) {
+	/* stop timer */
+	native_set_timer(0);
+    }
 }
 #endif
 
@@ -85,7 +110,6 @@ gvl_acquire(rb_vm_t *vm, rb_thread_t *th
 	if (GVL_DEBUG) fprintf(stderr, "gvl acquire (%p): sleep\n", th);
 	gvl_waiting_push(vm, th);
         if (GVL_DEBUG) gvl_show_waiting_threads(vm);
-
 	while (vm->gvl.acquired != 0 || vm->gvl.waiting_threads != th) {
 	    native_cond_wait(&th->native_thread_data.gvl_cond, &vm->gvl.lock);
 	}
@@ -297,9 +321,10 @@ static rb_thread_lock_t signal_thread_li
 static pthread_key_t ruby_native_thread_key;
 
 static void
-null_func(int i)
+vtalrm_handler(int i)
 {
-    /* null */
+    rb_vm_t *vm = GET_VM(); /* TODO: fix me for Multi-VM */
+    RUBY_VM_SET_TIMER_INTERRUPT(vm->running_thread);
 }
 
 static rb_thread_t *
@@ -314,7 +339,14 @@ ruby_thread_set_native(rb_thread_t *th)
     return pthread_setspecific(ruby_native_thread_key, th) == 0;
 }
 
-static void native_thread_init(rb_thread_t *th);
+static void
+native_thread_init(rb_thread_t *th)
+{
+    native_cond_initialize(&th->native_thread_data.sleep_cond);
+    native_cond_initialize(&th->native_thread_data.gvl_cond);
+    ruby_thread_set_native(th);
+    posix_signal(SIGVTALRM, vtalrm_handler);
+}
 
 void
 Init_native_thread(void)
@@ -325,15 +357,6 @@ Init_native_thread(void)
     th->thread_id = pthread_self();
     native_thread_init(th);
     native_mutex_initialize(&signal_thread_list_lock);
-    posix_signal(SIGVTALRM, null_func);
-}
-
-static void
-native_thread_init(rb_thread_t *th)
-{
-    native_cond_initialize(&th->native_thread_data.sleep_cond);
-    native_cond_initialize(&th->native_thread_data.gvl_cond);
-    ruby_thread_set_native(th);
 }
 
 static void
@@ -536,8 +559,6 @@ thread_start_func_1(void *th_ptr)
     return 0;
 }
 
-void rb_thread_create_control_thread(void);
-
 struct cached_thread_entry {
     volatile rb_thread_t **th_area;
     pthread_cond_t *cond;
@@ -816,6 +837,8 @@ struct signal_thread_list {
     struct signal_thread_list *next;
 };
 
+sem_t interrupt_sem;
+
 #ifndef __CYGWIN__
 static struct signal_thread_list signal_thread_list_anchor = {
     0, 0, 0,
@@ -870,6 +893,7 @@ add_signal_thread_list(rb_thread_t *th)
 	    th->native_thread_data.signal_thread_list = list;
 	});
     }
+    sem_post(&interrupt_sem);
 }
 #endif
 
@@ -896,9 +920,9 @@ remove_signal_thread_list(rb_thread_t *t
     }
 }
 
-static pthread_t timer_thread_id;
-static pthread_cond_t timer_thread_cond = PTHREAD_COND_INITIALIZER;
-static pthread_mutex_t timer_thread_lock = PTHREAD_MUTEX_INITIALIZER;
+static pthread_t interrupt_thread_id;
+static pthread_cond_t interrupt_thread_cond = PTHREAD_COND_INITIALIZER;
+static pthread_mutex_t interrupt_thread_lock = PTHREAD_MUTEX_INITIALIZER;
 
 static struct timespec *
 get_ts(struct timespec *ts, unsigned long nsec)
@@ -915,22 +939,40 @@ get_ts(struct timespec *ts, unsigned lon
 }
 
 static void *
-thread_timer(void *dummy)
+interrupt_thread_func(void *dummy)
 {
     struct timespec ts;
 
-    native_mutex_lock(&timer_thread_lock);
-    native_cond_broadcast(&timer_thread_cond);
-#define WAIT_FOR_10MS() native_cond_timedwait(&timer_thread_cond, &timer_thread_lock, get_ts(&ts, PER_NANO/100))
+    /* sync with creater thread */
+    native_mutex_lock(&interrupt_thread_lock);
+    native_cond_broadcast(&interrupt_thread_cond);
+    native_mutex_unlock(&interrupt_thread_lock);
+
     while (system_working > 0) {
-	int err = WAIT_FOR_10MS();
-	if (err == ETIMEDOUT);
-	else if (err == 0 || err == EINTR) {
-	    if (rb_signal_buff_size() == 0) break;
+#if !defined(__CYGWIN__) && !defined(__SYMBIAN32__)
+	if (signal_thread_list_anchor.next) {
+	    struct timespec ts;
+	    sem_timedwait(&interrupt_sem, get_ts(&ts, PER_NANO/100));
+	}
+	else
+#endif
+	  sem_wait(&interrupt_sem);
+
+	switch (errno) {
+	  case EINVAL:
+	  case EDEADLK:
+	   rb_bug_errno("interrupt_thread_core/sem_wait or sem_timedwait", errno);
+	}
+
+	if (rb_signal_buff_size() > 0) {
+	    /* signal deliverly */
+	    rb_vm_t *vm = GET_VM();
+	    rb_threadptr_check_signal(vm->main_thread);
 	}
-	else rb_bug_errno("thread_timer/timedwait", err);
 
 #if !defined(__CYGWIN__) && !defined(__SYMBIAN32__)
+	/* Send a signal to interrupt specific ruby thread */
+	/* to cancel blocking region */
 	if (signal_thread_list_anchor.next) {
 	    FGLOCK(&signal_thread_list_lock, {
 		struct signal_thread_list *list;
@@ -942,59 +984,59 @@ thread_timer(void *dummy)
 	    });
 	}
 #endif
-	timer_thread_function(dummy);
     }
-    native_mutex_unlock(&timer_thread_lock);
     return NULL;
 }
 
-static void
-rb_thread_create_timer_thread(void)
+/* called from signal handler */
+void
+rb_thread_set_signal(void)
 {
-    rb_enable_interrupt();
+    sem_post(&interrupt_sem);
+}
 
-    if (!timer_thread_id) {
+static void
+rb_thread_create_interrupt_thread(void)
+{
+    if (!interrupt_thread_id) {
 	pthread_attr_t attr;
 	int err;
 
+	sem_init(&interrupt_sem, 0, 0);
+
 	pthread_attr_init(&attr);
 #ifdef PTHREAD_STACK_MIN
 	pthread_attr_setstacksize(&attr,
 				  PTHREAD_STACK_MIN + (THREAD_DEBUG ? BUFSIZ : 0));
 #endif
-	native_mutex_lock(&timer_thread_lock);
-	err = pthread_create(&timer_thread_id, &attr, thread_timer, 0);
+	native_mutex_lock(&interrupt_thread_lock);
+	err = pthread_create(&interrupt_thread_id, &attr, interrupt_thread_func, 0);
 	if (err != 0) {
-	    native_mutex_unlock(&timer_thread_lock);
+	    native_mutex_unlock(&interrupt_thread_lock);
 	    fprintf(stderr, "[FATAL] Failed to create timer thread (errno: %d)\n", err);
 	    exit(EXIT_FAILURE);
 	}
-	native_cond_wait(&timer_thread_cond, &timer_thread_lock);
-	native_mutex_unlock(&timer_thread_lock);
+
+	native_cond_wait(&interrupt_thread_cond, &interrupt_thread_lock);
+	native_mutex_unlock(&interrupt_thread_lock);
     }
     rb_disable_interrupt(); /* only timer thread recieve signal */
 }
 
 static int
-native_stop_timer_thread(void)
+native_stop_interrupt_thread(void)
 {
     int stopped;
-    native_mutex_lock(&timer_thread_lock);
     stopped = --system_working <= 0;
-    if (stopped) {
-	native_cond_signal(&timer_thread_cond);
-    }
-    native_mutex_unlock(&timer_thread_lock);
-    if (stopped) {
-	native_thread_join(timer_thread_id);
-    }
+    sem_post(&interrupt_sem);
+    native_thread_join(interrupt_thread_id);
     return stopped;
 }
 
 static void
-native_reset_timer_thread(void)
+native_reset_interrupt_thread(void)
 {
-    timer_thread_id = 0;
+    interrupt_thread_id = 0;
 }
 
 #ifdef HAVE_SIGALTSTACK
Index: vm_core.h
===================================================================
--- vm_core.h	(revision 29961)
+++ vm_core.h	(working copy)
@@ -641,9 +641,10 @@ VALUE rb_vm_make_proc(rb_thread_t *th, c
 VALUE rb_vm_make_env_object(rb_thread_t *th, rb_control_frame_t *cfp);
 void rb_vm_gvl_destroy(rb_vm_t *vm);
 
-void rb_thread_start_timer_thread(void);
-void rb_thread_stop_timer_thread(void);
-void rb_thread_reset_timer_thread(void);
+void rb_thread_start_interrupt_thread(void);
+void rb_thread_stop_interrupt_thread(void);
+void rb_thread_reset_interrupt_thread(void);
+
 void *rb_thread_call_with_gvl(void *(*func)(void *), void *data1);
 int ruby_thread_has_gvl_p(void);
 VALUE rb_make_backtrace(void);
Index: thread.c
===================================================================
--- thread.c	(revision 29961)
+++ thread.c	(working copy)
@@ -193,7 +193,6 @@ rb_thread_s_debug_set(VALUE self, VALUE 
 #endif
 NOINLINE(static int thread_start_func_2(rb_thread_t *th, VALUE *stack_start,
 					VALUE *register_stack_start));
-static void timer_thread_function(void *);
 
 #if   defined(_WIN32)
 #include "thread_win32.c"
@@ -367,7 +366,7 @@ rb_thread_terminate_all(void)
 	}
 	POP_TAG();
     }
-    rb_thread_stop_timer_thread();
+    rb_thread_stop_interrupt_thread();
 }
 
 static void
@@ -2684,48 +2683,25 @@ rb_threadptr_check_signal(rb_thread_t *m
     }
 }
 
-static void
-timer_thread_function(void *arg)
-{
-    rb_vm_t *vm = GET_VM(); /* TODO: fix me for Multi-VM */
-
-    /* for time slice */
-    RUBY_VM_SET_TIMER_INTERRUPT(vm->running_thread);
-
-    /* check signal */
-    rb_threadptr_check_signal(vm->main_thread);
-
-#if 0
-    /* prove profiler */
-    if (vm->prove_profile.enable) {
-	rb_thread_t *th = vm->running_thread;
-
-	if (vm->during_gc) {
-	    /* GC prove profiling */
-	}
-    }
-#endif
-}
-
 void
-rb_thread_stop_timer_thread(void)
+rb_thread_stop_interrupt_thread(void)
 {
-    if (timer_thread_id && native_stop_timer_thread()) {
-	native_reset_timer_thread();
+    if (interrupt_thread_id && native_stop_interrupt_thread()) {
+	native_reset_interrupt_thread();
     }
 }
 
 void
-rb_thread_reset_timer_thread(void)
+rb_thread_reset_interrupt_thread(void)
 {
-    native_reset_timer_thread();
+    native_reset_interrupt_thread();
 }
 
 void
-rb_thread_start_timer_thread(void)
+rb_thread_start_interrupt_thread(void)
 {
     system_working = 1;
-    rb_thread_create_timer_thread();
+    rb_thread_create_interrupt_thread();
 }
 
 static int
@@ -4389,7 +4365,7 @@ Init_Thread(void)
 	}
     }
 
-    rb_thread_create_timer_thread();
+    rb_thread_create_interrupt_thread();
 
     (void)native_mutex_trylock;
 }
Index: eval.c
===================================================================
--- eval.c	(revision 29961)
+++ eval.c	(working copy)
@@ -35,7 +35,6 @@ VALUE rb_eSysStackError;
 /* initialize ruby */
 
 void rb_clear_trace_func(void);
-void rb_thread_stop_timer_thread(void);
 
 void rb_call_inits(void);
 void Init_heap(void);
@@ -118,8 +117,6 @@ ruby_finalize(void)
     ruby_finalize_1();
 }
 
-void rb_thread_stop_timer_thread(void);
-
 int
 ruby_cleanup(volatile int ex)
 {
@@ -160,7 +157,7 @@ ruby_cleanup(volatile int ex)
     ex = error_handle(ex);
     ruby_finalize_1();
     POP_TAG();
-    rb_thread_stop_timer_thread();
+    rb_thread_stop_interrupt_thread();
 
 #if EXIT_SUCCESS != 0 || EXIT_FAILURE != 1
     switch (ex) {
Index: process.c
===================================================================
--- process.c	(revision 29961)
+++ process.c	(working copy)
@@ -989,16 +989,12 @@ proc_detach(VALUE obj, VALUE pid)
 char *strtok();
 #endif
 
-void rb_thread_stop_timer_thread(void);
-void rb_thread_start_timer_thread(void);
-void rb_thread_reset_timer_thread(void);
-
 static int forked_child = 0;
 
 #define before_exec() \
-    (rb_enable_interrupt(), (void)(forked_child ? 0 : (rb_thread_stop_timer_thread(), 1)))
+    (rb_enable_interrupt(), (void)(forked_child ? 0 : (rb_thread_stop_interrupt_thread(), 1)))
 #define after_exec() \
-  (rb_thread_reset_timer_thread(), rb_thread_start_timer_thread(), forked_child = 0, rb_disable_interrupt())
+  (rb_thread_reset_interrupt_thread(), rb_thread_start_interrupt_thread(), forked_child = 0, rb_disable_interrupt())
 #define before_fork() before_exec()
 #define after_fork() (GET_THREAD()->thrown_errinfo = 0, after_exec())
 
Index: signal.c
===================================================================
--- signal.c	(revision 29961)
+++ signal.c	(working copy)
@@ -522,11 +522,15 @@ ruby_nativethread_signal(int signum, sig
 #endif
 #endif
 
+void rb_thread_set_signal(void);
+
 static RETSIGTYPE
 sighandler(int sig)
 {
     ATOMIC_INC(signal_buff.cnt[sig]);
     ATOMIC_INC(signal_buff.size);
+    rb_thread_set_signal();
+
 #if !defined(BSD_SIGNAL) && !defined(POSIX_SIGNAL)
     ruby_signal(sig, sighandler);
 #endif
@@ -556,8 +560,6 @@ rb_disable_interrupt(void)
 #if USE_TRAP_MASK
     sigset_t mask;
     sigfillset(&mask);
-    sigdelset(&mask, SIGVTALRM);
-    sigdelset(&mask, SIGSEGV);
     pthread_sigmask(SIG_SETMASK, &mask, NULL);
 #endif
 }
@@ -1057,6 +1059,44 @@ int ruby_enable_coredump = 0;
 #define ruby_enable_coredump 0
 #endif
 
+#if HAVE_PTHREAD
+#include <pthread.h>
+#include <signal.h>
+
+static VALUE
+sigset2hash(sigset_t *set)
+{
+    VALUE hash = rb_hash_new();
+    const struct signals *sigs;
+
+    for (sigs = siglist; sigs->signm; sigs++) {
+	if (sigs->signo == 0) continue;
+	rb_hash_aset(hash, rb_str_new2(sigs->signm), sigismember(set, sigs->signo) ? Qtrue : Qfalse);
+	fprintf(stderr, "signal: SIG%-6s is %d\n", sigs->signm, sigismember(set, sigs->signo));
+    }
+
+    return hash;
+}
+
+static VALUE
+sigmask_of_current_thread(VALUE klass)
+{
+    sigset_t oldset;
+    int r;
+    r = pthread_sigmask(0, NULL, &oldset);
+    if (r != 0) {rb_bug_errno("pthread_sigmask", r);}
+    return sigset2hash(&oldset);
+}
+
+static VALUE
+sigmask_of_current_process(VALUE klass)
+{
+    sigset_t oldset;
+    sigprocmask(0, NULL, &oldset);
+    return sigset2hash(&oldset);
+}
+#endif
+
 /*
  * Many operating systems allow signals to be sent to running
  * processes. Some signals have a defined effect on the process, while
@@ -1102,6 +1142,11 @@ Init_signal(void)
     rb_define_global_function("trap", sig_trap, -1);
     rb_define_module_function(mSignal, "trap", sig_trap, -1);
     rb_define_module_function(mSignal, "list", sig_list, 0);
+
+#if HAVE_PTHREAD
+    rb_define_module_function(mSignal, "sigmask_of_current_thread", sigmask_of_current_thread, 0);
+    rb_define_module_function(mSignal, "sigmask_of_current_process", sigmask_of_current_process, 0);
+#endif
 
     rb_define_method(rb_eSignal, "initialize", esignal_init, -1);
     rb_define_method(rb_eSignal, "signo", esignal_signo, 0);
Mon Nov 29 11:38:42 2010  Koichi Sasada  <ko1@atdot.net>

	* eval.c: 

	* eval.c (ruby_cleanup): 

	* eval.c (ruby_finalize): 

	* process.c: 

	* process.c (proc_detach): 

	* signal.c: 

	* signal.c (Init_signal): 

	* signal.c (rb_disable_interrupt): 

	* signal.c (ruby_nativethread_signal): 

	* thread.c: 

	* thread.c (Init_Thread): 

	* thread.c (rb_thread_s_debug_set): 

	* thread.c (rb_thread_terminate_all): 

	* thread.c (rb_threadptr_check_signal): 

	* thread_pthread.c: 

	* thread_pthread.c (Init_native_thread): 

	* thread_pthread.c (add_signal_thread_list): 

	* thread_pthread.c (get_ts): 

	* thread_pthread.c (gvl_acquire): 

	* thread_pthread.c (gvl_show_waiting_threads): 

	* thread_pthread.c (gvl_waiting_push): 

	* thread_pthread.c (gvl_waiting_shift): 

	* thread_pthread.c (remove_signal_thread_list): 

	* thread_pthread.c (ruby_thread_set_native): 

	* thread_pthread.c (thread_start_func_1): 

	* thread_pthread.c (thread_timer): 

	* vm_core.h: 

	* vm_core.h (rb_vm_make_proc): 


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[ruby-core:37081] Re: [Request for Comment] avoid timer thread

[SASADA Koichi]

Hi,

(2011/06/13 19:25), Mark Somerville wrote:
>> Mind reposting your updated patch?  ML archives mangled the original
>> one.  I can host a branch for it on git://bogomips.org/ruby.git too
> 
> The patch I was testing was Koichi's. My earlier patch for this problem
> was shown to have problems.
> 
> I've attached Koichi's patch again. Unfortunately it doesn't apply
> cleanly to master and I haven't got the experience/time needed to get it
> to a working state.
> 
> If you or someone else does, I'll be very happy to test it out or help
> in any other way I can.
>  
>>> It would be great to get this (or something like it) into 1.9.3.
>>>
>>> I consider this a pretty bad regression from 1.8. I don't want to have
>>> to use 1.8 any more!
>>
>> I completely agree.  I plan on running Ruby 1.9 daemons on my laptop
>> more; so having this in 1.9.3 would be great.
> 
> I'm in the same position - wanting to run tens of Ruby processes for a
> long time on my laptop.

We (Kosaki-san and me) are working on introduce this feature for 1.9.3.

Maybe as I wrote, there is a problem that the MacOSX doesn't support
semaphore (sem_timedwait).  So we need to change the strategy of
implementation.  Kosaki-san proposed the use of pipe trick.  And he will
make it.  Please wait.

Regards,
Koichi

-- 
// SASADA Koichi at atdot dot net

[ruby-core:37103] Re: [Request for Comment] avoid timer thread

[Eric Wong]

Mark Somerville <mark@scottishclimbs.com> wrote:
> The patch I was testing was Koichi's. My earlier patch for this problem
> was shown to have problems.
> 
> I've attached Koichi's patch again.

Thanks, I've put it on http://bogomips.org/avoid_timerthread.patch in
case the archives mangle it again.

> Unfortunately it doesn't apply
> cleanly to master and I haven't got the experience/time needed to get it
> to a working state.

I'll just wait on the 1.9.3 version for now based on ko1's response \o/

-- 
Eric Wong

[ruby-core:37187] Re: [Request for Comment] avoid timer thread

[SASADA Koichi]

(2011/06/14 3:37), Eric Wong wrote:
> I'll just wait on the 1.9.3 version for now based on ko1's response \o/

Here is my current patch.  This is only tested on Linux on VirtualBox.
I'm happy if you test on the other environment.

http://www.atdot.net/sp/view/yonwml/readonly
http://www.atdot.net/sp/raw/yonwml

-- 
// SASADA Koichi at atdot dot net

[ruby-core:37195] Re: [Request for Comment] avoid timer thread

[Eric Wong]

SASADA Koichi <ko1@atdot.net> wrote:
> (2011/06/14 3:37), Eric Wong wrote:
> > I'll just wait on the 1.9.3 version for now based on ko1's response \o/
> 
> Here is my current patch.  This is only tested on Linux on VirtualBox.
> I'm happy if you test on the other environment.
> 
> http://www.atdot.net/sp/view/yonwml/readonly
> http://www.atdot.net/sp/raw/yonwml

Awesome, thank you!  I've installed it on Linux x86 VPS and x86_64
laptop, everything looks good so far.

I've even switched my Rainbows! instance (it calls fork+exec cgit.cgi)
over to running Ruby trunk + your patch:

   http://bogomips.org/ruby.git/log/?h=sleepy-timer-thread

Now everything on http://bogomips.org/*.git and
http://raindrops-demo.bogomips.org/ is running trunk + timer patch.
Wish us luck :)

-- 
Eric Wong

[ruby-core:37205] Re: [Request for Comment] avoid timer thread

[Eric Wong]

Eric Wong <normalperson@yhbt.net> wrote:
> I've even switched my Rainbows! instance (it calls fork+exec cgit.cgi)
> over to running Ruby trunk + your patch:

I found a race condition in my server that calls fork+exec
often:
  unicorn-3.7.0/lib/unicorn/app/exec_cgi.rb:66: [BUG] rb_thread_wakeup_timer_thread - write: Broken pipe (EPIPE)

exec_cgi.rb code is here, the fork { exec } is all in Ruby:
http://bogomips.org/unicorn.git/tree/lib/unicorn/app/exec_cgi.rb?id=v3.7.0

I've only seen this happen once out of 8000 or so requests (mostly search
spiders).  This process (Rainbows!) is also configured to use
multithreading, so there could be many things happening that could
fire a signal...

I'm testing the following fix on my server:
http://bogomips.org/ruby.git/commit/?h=sleepy-timer-thread&id=9154363119

I'm also sending SIGUSR1 every minute to increase the chance of hitting
this error and telling robots to crawl faster.

From 9154363119cd09a3b79aa45753d177ec65914f7c Mon Sep 17 00:00:00 2001
From: Eric Wong <normalperson@yhbt.net>
Date: Fri, 17 Jun 2011 19:52:48 +0000
Subject: [PATCH] defer timer thread wakeups after fork()

There is a small race condition where a signal can be received
immediately after fork() but before the timer thread pipe can be
reinitialized, leading to EPIPE on write().

We probably can't disable interrupt before forking due to
system/exec() issues; and we don't want to lose wakeups to the
parent process, so we check the cached pid vs getpid() whenever
rb_thread_wakeup_timer_thread() is called.
---
 thread_pthread.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/thread_pthread.c b/thread_pthread.c
index 1cc212b..3d99974 100644
--- a/thread_pthread.c
+++ b/thread_pthread.c
@@ -989,11 +989,18 @@ static void ping_signal_thread_list(void) { }
 #endif /* USE_SIGNAL_THREAD_LIST */
 
 static pthread_t timer_thread_id;
+static sig_atomic_t timer_thread_deferred;
+static pid_t timer_thread_pid;
 static int timer_thread_pipe[2];
 
 void
 rb_thread_wakeup_timer_thread(void)
 {
+    if (getpid() != timer_thread_pid) {
+	timer_thread_deferred = 1;
+	return;
+    }
+
     /* already opened */
     if (timer_thread_id && timer_thread_pipe[1]) {
 	const char *buff = "!";
@@ -1021,6 +1028,12 @@ thread_timer(void *p)
 
     if (TT_DEBUG) fprintf(stderr, "start timer thread\n");
 
+    if (timer_thread_deferred) {
+	timer_thread_deferred = 0;
+	ping_signal_thread_list();
+	timer_thread_function(0);
+    }
+
     while (system_working > 0) {
 	int err;
 
@@ -1093,6 +1106,7 @@ rb_thread_create_timer_thread(void)
 	    fprintf(stderr, "[FATAL] Failed to create timer thread (errno: %d)\n", err);
 	    exit(EXIT_FAILURE);
 	}
+	timer_thread_pid = getpid();
     }
     rb_disable_interrupt(); /* only timer thread recieve signal */
 }
@@ -1118,6 +1132,7 @@ native_reset_timer_thread(void)
 {
     if (TT_DEBUG)  fprintf(stderr, "reset timer thread\n");
     timer_thread_id = 0;
+    timer_thread_pid = 0;
 
     /* close */
     if (timer_thread_pipe[0] && close(timer_thread_pipe[0]) < 0) { rb_bug_errno("native_stop_timer_thread - close(ttp[0])", errno); }
-- 
Eric Wong

[ruby-core:37211] Re: [Request for Comment] avoid timer thread

[Eric Wong]

Eric Wong <normalperson@yhbt.net> wrote:
> I'm testing the following fix on my server:
> http://bogomips.org/ruby.git/commit/?h=sleepy-timer-thread&id=9154363119

One more patch below:
http://bogomips.org/ruby.git/commit/?h=sleepy-timer-thread&id=853275cb9ec

From 853275cb9ecdee021c0426f0e009f69b0999256a Mon Sep 17 00:00:00 2001
From: Eric Wong <normalperson@yhbt.net>
Date: Fri, 17 Jun 2011 16:20:48 -0700
Subject: [PATCH] thread_pthread: prevent deadlock from signal spam

I managed to get the following code snippet to deadlock on a
dual-core machine due to blocked write() call in
rb_thread_wakeup_timer_thread():
------------------------- 8< ----------------------------
n = 0
parent = $$
trap(:USR1) { n += 1 }
fork do
  Process.kill(:USR1, parent) while true
end
p(Process.waitpid(fork { exit!(0) })) while true
------------------------- 8< ----------------------------
---
 thread_pthread.c |   20 +++++++++++++++++++-
 1 files changed, 19 insertions(+), 1 deletions(-)

diff --git a/thread_pthread.c b/thread_pthread.c
index 3d99974..c2771ac 100644
--- a/thread_pthread.c
+++ b/thread_pthread.c
@@ -20,6 +20,12 @@
 #include <thread.h>
 #endif
 
+#if defined(HAVE_FCNTL_H)
+#include <fcntl.h>
+#elif defined(HAVE_SYS_FCNTL_H)
+#include <sys/fcntl.h>
+#endif
+
 static void native_mutex_lock(pthread_mutex_t *lock);
 static void native_mutex_unlock(pthread_mutex_t *lock);
 static int native_mutex_trylock(pthread_mutex_t *lock);
@@ -1006,7 +1012,8 @@ rb_thread_wakeup_timer_thread(void)
 	const char *buff = "!";
 	int err;
 	if ((err = write(timer_thread_pipe[1], buff, 1)) <= 0) {
-	    rb_bug_errno("rb_thread_wakeup_timer_thread - write", errno);
+	    if (errno != EAGAIN)
+		rb_bug_errno("rb_thread_wakeup_timer_thread - write", errno);
 	}
     }
 }
@@ -1107,6 +1114,17 @@ rb_thread_create_timer_thread(void)
 	    exit(EXIT_FAILURE);
 	}
 	timer_thread_pid = getpid();
+	err = fcntl(timer_thread_pipe[1], F_GETFL);
+	if (err == -1) {
+	    fprintf(stderr, "[FATAL] Failed to get flags for timer thread pipe (errno: %d)\n", errno);
+	    exit(EXIT_FAILURE);
+	}
+	err = fcntl(timer_thread_pipe[1], F_SETFL, err | O_NONBLOCK);
+	if (err == -1) {
+	    fprintf(stderr, "[FATAL] Failed to set flags for timer thread pipe (errno: %d)\n", errno);
+	    exit(EXIT_FAILURE);
+	}
+	timer_thread_pid = getpid();
     }
     rb_disable_interrupt(); /* only timer thread recieve signal */
 }
-- 
Eric Wong

[ruby-core:37215] Re: [Request for Comment] avoid timer thread

[Eric Wong]

SASADA Koichi <ko1@atdot.net> wrote:
> (2011/06/14 3:37), Eric Wong wrote:
> > I'll just wait on the 1.9.3 version for now based on ko1's response \o/
> 
> Here is my current patch.  This is only tested on Linux on VirtualBox.
> I'm happy if you test on the other environment.
> 
> http://www.atdot.net/sp/view/yonwml/readonly
> http://www.atdot.net/sp/raw/yonwml

In case there are C extensions or FFI wrappers that fork() directly
without using Ruby, I prefer pthread_atfork() to close the pipe:

http://bogomips.org/ruby.git/commit/?h=sleepy-timer-thread&id=b8bb000953b

From b8bb000953b0c9d892bd9314054b80a3fe2eec48 Mon Sep 17 00:00:00 2001
From: Eric Wong <normalperson@yhbt.net>
Date: Fri, 17 Jun 2011 17:50:32 -0700
Subject: [PATCH] thread_pthread.c: avoid pipe leak to extensions that fork()
 internally

Some extensions (or FFI wrapper) may call fork() directly and
bypass Ruby-specific hooks for fork().
---
 thread_pthread.c |   25 +++++++++++++++++++------
 1 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/thread_pthread.c b/thread_pthread.c
index c2771ac..5f61df4 100644
--- a/thread_pthread.c
+++ b/thread_pthread.c
@@ -1085,14 +1085,32 @@ thread_timer(void *p)
     return NULL;
 }
 
+static void close_timer_thread_pipe(void)
+{
+    if (timer_thread_pipe[0] && close(timer_thread_pipe[0]) < 0) { rb_bug_errno("native_stop_timer_thread - close(ttp[0])", errno); }
+    if (timer_thread_pipe[1] && close(timer_thread_pipe[1]) < 0) { rb_bug_errno("native_stop_timer_thread - close(ttp[1])", errno); }
+    timer_thread_pipe[0] = timer_thread_pipe[1] = 0;
+}
+
 static void
 rb_thread_create_timer_thread(void)
 {
+    static int atfork_installed;
+    int err;
+
+    if (!atfork_installed) {
+	err = pthread_atfork(NULL, NULL, close_timer_thread_pipe);
+	if (err != 0) {
+	    fprintf(stderr, "[FATAL] Failed to install atfork hook for timer thread pipe (errno: %d)\n", err);
+	    exit(EXIT_FAILURE);
+	}
+	atfork_installed = 1;
+    }
+
     rb_enable_interrupt();
 
     if (!timer_thread_id) {
 	pthread_attr_t attr;
-	int err;
 
 	pthread_attr_init(&attr);
 #ifdef PTHREAD_STACK_MIN
@@ -1151,11 +1169,6 @@ native_reset_timer_thread(void)
     if (TT_DEBUG)  fprintf(stderr, "reset timer thread\n");
     timer_thread_id = 0;
     timer_thread_pid = 0;
-
-    /* close */
-    if (timer_thread_pipe[0] && close(timer_thread_pipe[0]) < 0) { rb_bug_errno("native_stop_timer_thread - close(ttp[0])", errno); }
-    if (timer_thread_pipe[1] && close(timer_thread_pipe[1]) < 0) { rb_bug_errno("native_stop_timer_thread - close(ttp[1])", errno); }
-    timer_thread_pipe[0] = timer_thread_pipe[1] = 0;
 }
 
 #ifdef HAVE_SIGALTSTACK
-- 
Eric Wong

[ruby-core:37216] Re: [Request for Comment] avoid timer thread

[Eric Wong]

Eric Wong <normalperson@yhbt.net> wrote:
> In case there are C extensions or FFI wrappers that fork() directly
> without using Ruby, I prefer pthread_atfork() to close the pipe:

I just pushed a few more fixes up to my git branch, all
view/downloadable here:

  http://bogomips.org/ruby.git/log/?h=sleepy-timer-thread

  "make test" works, waiting on "test-all" but I'm gone for a while

commit a73bfa86f43cef5c4d4d08e5dff494a9eb7c77ab
Author: Eric Wong <normalperson@yhbt.net>
Date:   Sat Jun 18 04:16:08 2011 +0000

    thread_pthread: set close-on-exec for timer thread pipe
    
    In case any C extensions call exec*() directly...

commit fc3928585073b8bce44544e74b1bda5097e28833
Author: Eric Wong <normalperson@yhbt.net>
Date:   Sat Jun 18 04:13:48 2011 +0000

    thread_pthread.c: show errno for pipe() failure
    
    pipe() stores the error in errno, not as a return value

commit f4becc86ecd462e76ce34a35787ec2fc2ae7ebb4
Author: Eric Wong <normalperson@yhbt.net>
Date:   Sat Jun 18 04:12:52 2011 +0000

    thread_pthread: still need to close pipe when resetting
    
    otherwise plain exec* family calls will keep the pipe around.
-- 
Eric Wong

[ruby-core:37218] Re: [Request for Comment] avoid timer thread

[KOSAKI Motohiro]

> From 9154363119cd09a3b79aa45753d177ec65914f7c Mon Sep 17 00:00:00 2001
> From: Eric Wong <normalperson@yhbt.net>
> Date: Fri, 17 Jun 2011 19:52:48 +0000
> Subject: [PATCH] defer timer thread wakeups after fork()
>
> There is a small race condition where a signal can be received
> immediately after fork() but before the timer thread pipe can be
> reinitialized, leading to EPIPE on write().
>
> We probably can't disable interrupt before forking due to
> system/exec() issues; and we don't want to lose wakeups to the
> parent process, so we check the cached pid vs getpid() whenever
> rb_thread_wakeup_timer_thread() is called.
> ---
>  thread_pthread.c |   15 +++++++++++++++
>  1 files changed, 15 insertions(+), 0 deletions(-)
>
> diff --git a/thread_pthread.c b/thread_pthread.c
> index 1cc212b..3d99974 100644
> --- a/thread_pthread.c
> +++ b/thread_pthread.c
> @@ -989,11 +989,18 @@ static void ping_signal_thread_list(void) { }
>  #endif /* USE_SIGNAL_THREAD_LIST */
>
>  static pthread_t timer_thread_id;
> +static sig_atomic_t timer_thread_deferred;
> +static pid_t timer_thread_pid;
>  static int timer_thread_pipe[2];
>
>  void
>  rb_thread_wakeup_timer_thread(void)
>  {
> +    if (getpid() != timer_thread_pid) {

getpid() return *process* id. timer thread and other threads will
get the same process id.

[ruby-core:37219] Re: [Request for Comment] avoid timer thread

[SASADA Koichi]

(2011/06/18 21:14), KOSAKI Motohiro wrote:
>> >  void
>> >  rb_thread_wakeup_timer_thread(void)
>> >  {
>> > +    if (getpid() != timer_thread_pid) {
> getpid() return *process* id. timer thread and other threads will
> get the same process id.
> 

This condition checks the race between parent process and forked child
process.  This may be intentional.

I'll fix with another way to avoid such race.

-- 
// SASADA Koichi at atdot dot net

[ruby-core:37224] Re: [Request for Comment] avoid timer thread

[Eric Wong]

SASADA Koichi <ko1@atdot.net> wrote:
> (2011/06/18 21:14), KOSAKI Motohiro wrote:
> >> >  void
> >> >  rb_thread_wakeup_timer_thread(void)
> >> >  {
> >> > +    if (getpid() != timer_thread_pid) {
> > getpid() return *process* id. timer thread and other threads will
> > get the same process id.
> > 
> 
> This condition checks the race between parent process and forked child
> process.  This may be intentional.

Yes, that is my intent.

> I'll fix with another way to avoid such race.

OK.

-- 
Eric Wong

[ruby-core:37319] Re: [Request for Comment] avoid timer thread

[Mark Somerville]

[-- Attachment #1: Type: text/plain, Size: 452 bytes --]

On Sat, Jun 18, 2011 at 08:55:19AM +0900, Eric Wong wrote:
> Eric Wong <normalperson@yhbt.net> wrote:
> > I'm testing the following fix on my server:
> > http://bogomips.org/ruby.git/commit/?h=sleepy-timer-thread&id=9154363119
> 
> One more patch below:
> http://bogomips.org/ruby.git/commit/?h=sleepy-timer-thread&id=853275cb9ec

Which patchset would it be most useful if I test first (I may only have
time to test one). Koichi's or Eric's?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[ruby-core:37325] Re: [Request for Comment] avoid timer thread

[SASADA Koichi]

(2011/06/23 20:53), Mark Somerville wrote:
> On Sat, Jun 18, 2011 at 08:55:19AM +0900, Eric Wong wrote:
>> Eric Wong <normalperson@yhbt.net> wrote:
>>> I'm testing the following fix on my server:
>>> http://bogomips.org/ruby.git/commit/?h=sleepy-timer-thread&id=9154363119
>>
>> One more patch below:
>> http://bogomips.org/ruby.git/commit/?h=sleepy-timer-thread&id=853275cb9ec
> 
> Which patchset would it be most useful if I test first (I may only have
> time to test one). Koichi's or Eric's?

http://www.atdot.net/sp/view/5d09nl/readonly
Here is a latest one (and I will commit it).

-- 
// SASADA Koichi at atdot dot net

[ruby-core:37333] Re: [Request for Comment] avoid timer thread

[Eric Wong]

SASADA Koichi <ko1@atdot.net> wrote:
> (2011/06/23 20:53), Mark Somerville wrote:
> > On Sat, Jun 18, 2011 at 08:55:19AM +0900, Eric Wong wrote:
> >> Eric Wong <normalperson@yhbt.net> wrote:
> >>> I'm testing the following fix on my server:
> >>> http://bogomips.org/ruby.git/commit/?h=sleepy-timer-thread&id=9154363119
> >>
> >> One more patch below:
> >> http://bogomips.org/ruby.git/commit/?h=sleepy-timer-thread&id=853275cb9ec
> > 
> > Which patchset would it be most useful if I test first (I may only have
> > time to test one). Koichi's or Eric's?

I just switched my server to an updated version of the patch below.
My previous sleepy-timer-thread branch has been running for nearly
a week with no issues on bogomips.org.

> http://www.atdot.net/sp/view/5d09nl/readonly
> Here is a latest one (and I will commit it).

FD_CLOEXEC needs to be set with F_SETFD, not F_SETFL.  It also
needs to be set on both descriptors, not just the writer:

  http://bogomips.org/ruby.git/commit/?h=sleepy2&id=c8d6093b029

5d09nl looks good otherwise and I'm running with my above change
on bogomips.org.  I will report back if I have any problems
(or if you notice bogomips.org stops responding :)

-- 
Eric Wong

[ruby-core:37623] Re: [Request for Comment] avoid timer thread

[Eric Wong]

Eric Wong <normalperson@yhbt.net> wrote:
> 5d09nl looks good otherwise and I'm running with my above change
> on bogomips.org.  I will report back if I have any problems
> (or if you notice bogomips.org stops responding :)

I hit a race condition with invalid FD during restarts/exits and needed
the following patch:

http://bogomips.org/ruby.git/commit/?h=timer-thread-exit-race&id=9108cfab0c

-- 
Eric Wong

[ruby-core:37628] Re: [Request for Comment] avoid timer thread

[SASADA Koichi]

(2011/06/28 19:55), Eric Wong wrote:
> Eric Wong <normalperson@yhbt.net> wrote:
>> 5d09nl looks good otherwise and I'm running with my above change
>> on bogomips.org.  I will report back if I have any problems
>> (or if you notice bogomips.org stops responding :)
> 
> I hit a race condition with invalid FD during restarts/exits and needed
> the following patch:
> 
> http://bogomips.org/ruby.git/commit/?h=timer-thread-exit-race&id=9108cfab0c
> 

How about to skip closing a pipe?  Could you try it?

Index: thread_pthread.c
===================================================================
--- thread_pthread.c	(revision 32266)
+++ thread_pthread.c	(working copy)
@@ -1188,7 +1188,7 @@
 	     *       This pass is cleaning phase.  It is too rare case
              *       to generate problem, so we remains it in TODO.
 	     */
-	    close_communication_pipe();
+	    // close_communication_pipe();
 	}
     }
     return stopped;

-- 
// SASADA Koichi at atdot dot net

[ruby-core:37642] Re: [Request for Comment] avoid timer thread

[Eric Wong]

SASADA Koichi <ko1@atdot.net> wrote:
> (2011/06/28 19:55), Eric Wong wrote:
> > Eric Wong <normalperson@yhbt.net> wrote:
> > I hit a race condition with invalid FD during restarts/exits and needed
> > the following patch:
> > 
> > http://bogomips.org/ruby.git/commit/?h=timer-thread-exit-race&id=9108cfab0c
> 
> How about to skip closing a pipe?  Could you try it?

I still get one segfault, but it's on an EventMachine test...
I don't get any segfault with my patch.

I think rb_bug_errno() is unsafe to call inside both thread_timer() and
rb_thread_wakeup_timer_thread() functions and it's putting me
on the wrong path while debugging.

-- 
Eric Wong

[ruby-core:37644] Re: [Request for Comment] avoid timer thread

[Eric Wong]

Eric Wong <normalperson@yhbt.net> wrote:
> I think rb_bug_errno() is unsafe to call inside both thread_timer() and
> rb_thread_wakeup_timer_thread() functions and it's putting me
> on the wrong path while debugging.

Maybe something along the lines of:
http://bogomips.org/ruby.git/commit/?h=async-signal-safety&id=ce58589bd

-- 
Eric Wong

[ruby-core:37645] Re: [Request for Comment] avoid timer thread

[SASADA Koichi]

(2011/06/29 5:26), Eric Wong wrote:
> Eric Wong <normalperson@yhbt.net> wrote:
>> I think rb_bug_errno() is unsafe to call inside both thread_timer() and
>> rb_thread_wakeup_timer_thread() functions and it's putting me
>> on the wrong path while debugging.
> 
> Maybe something along the lines of:
> http://bogomips.org/ruby.git/commit/?h=async-signal-safety&id=ce58589bd

Thank you.  In fact, I'm not care about error message because it is not
used in standard path.

I'll commit your patch.  Thanks again.

-- 
// SASADA Koichi at atdot dot net

[ruby-core:37647] Re: [Request for Comment] avoid timer thread

[Eric Wong]

SASADA Koichi <ko1@atdot.net> wrote:
> (2011/06/29 5:26), Eric Wong wrote:
> > Eric Wong <normalperson@yhbt.net> wrote:
> >> I think rb_bug_errno() is unsafe to call inside both thread_timer() and
> >> rb_thread_wakeup_timer_thread() functions and it's putting me
> >> on the wrong path while debugging.
> > 
> > Maybe something along the lines of:
> > http://bogomips.org/ruby.git/commit/?h=async-signal-safety&id=ce58589bd
> 
> Thank you.  In fact, I'm not care about error message because it is not
> used in standard path.
> 
> I'll commit your patch.  Thanks again.

No problem!  Thanks for paying attention to this :)

One small patch on top of it to remove strerror() since it may call
malloc() on glibc:
http://bogomips.org/ruby.git/commit/?h=async-signal-safety&id=78ab32fa

Technically strlen() isn't listed as async-signal safe, either, but
I don't think any implementation would use system calls for it...

-- 
Eric Wong