From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: X-Spam-Status: No, score=-3.8 required=3.0 tests=ALL_TRUSTED,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS shortcircuit=no autolearn=no autolearn_force=no version=3.4.1 Received: from localhost (dcvr.yhbt.net [127.0.0.1]) by dcvr.yhbt.net (Postfix) with ESMTP id BE6F21F660; Mon, 25 Jun 2018 23:50:58 +0000 (UTC) From: Eric Wong To: spew@80x24.org Cc: Eric Wong Subject: [PATCH 5/8] handle SIGCHLD in both the timer-thread and main thread Date: Mon, 25 Jun 2018 23:50:48 +0000 Message-Id: <20180625235051.66045-6-e@80x24.org> In-Reply-To: <20180625235051.66045-1-e@80x24.org> References: <20180625235051.66045-1-e@80x24.org> List-Id: It is needed to avoid race conditions. I could use separate counters, but the long-term goal is to make timer-thread unnecessary for single-threaded programs. --- process.c | 113 ++++++++++++++++++++++++++++++++++--------------------- signal.c | 25 ++++++++++-- thread.c | 42 +++++++++++++++------ thread_pthread.c | 18 +++++++++ 4 files changed, 140 insertions(+), 58 deletions(-) diff --git a/process.c b/process.c index 9f0995e194..0b91b30075 100644 --- a/process.c +++ b/process.c @@ -899,42 +899,51 @@ do_waitpid(rb_pid_t pid, int *st, int flags) struct waitpid_state { struct list_node wnode; - union { - rb_nativethread_cond_t *cond; /* non-Ruby thread */ - rb_execution_context_t *ec; /* normal Ruby execution context */ - } wake; + rb_execution_context_t *ec; + rb_nativethread_cond_t *cond; rb_pid_t ret; rb_pid_t pid; int status; int options; int errnum; - unsigned int is_ruby : 1; }; void rb_native_mutex_lock(rb_nativethread_lock_t *); void rb_native_mutex_unlock(rb_nativethread_lock_t *); void rb_native_cond_signal(rb_nativethread_cond_t *); void rb_native_cond_wait(rb_nativethread_cond_t *, rb_nativethread_lock_t *); +rb_nativethread_cond_t *rb_sleep_cond_get(const rb_execution_context_t *); +void rb_sleep_cond_put(rb_nativethread_cond_t *); -/* called by vm->main_thread */ +static void +waitpid_notify(struct waitpid_state *w, pid_t ret) +{ + w->ret = ret; + if (w->ret == -1) w->errnum = errno; + list_del_init(&w->wnode); + rb_native_cond_signal(w->cond); +} + +/* called by both timer thread and main thread */ void -rb_waitpid_all(rb_vm_t *vm) +ruby_waitpid_all(rb_vm_t *vm) { struct waitpid_state *w = 0, *next; rb_native_mutex_lock(&vm->waitpid_lock); list_for_each_safe(&vm->waiting_pids, w, next, wnode) { - w->ret = do_waitpid(w->pid, &w->status, w->options | WNOHANG); + pid_t ret = do_waitpid(w->pid, &w->status, w->options | WNOHANG); + if (!ret) continue; - if (w->ret == 0) continue; - if (w->ret == -1) w->errnum = errno; + if (w->ec) { /* rb_waitpid */ + rb_thread_t *th = rb_ec_thread_ptr(w->ec); - list_del_init(&w->wnode); - if (w->is_ruby) { - rb_thread_wakeup_alive(rb_ec_thread_ptr(w->wake.ec)->self); + rb_native_mutex_lock(&th->interrupt_lock); + waitpid_notify(w, ret); + rb_native_mutex_unlock(&th->interrupt_lock); } - else { - rb_native_cond_signal(w->wake.cond); + else { /* ruby_waitpid_locked */ + waitpid_notify(w, ret); } } rb_native_mutex_unlock(&vm->waitpid_lock); @@ -946,7 +955,6 @@ waitpid_state_init(struct waitpid_state *w, pid_t pid, int options) w->ret = 0; w->pid = pid; w->options = options; - list_node_init(&w->wnode); } /* @@ -961,16 +969,16 @@ ruby_waitpid_locked(rb_vm_t *vm, rb_pid_t pid, int *status, int options, assert(!ruby_thread_has_gvl_p() && "must not have GVL"); waitpid_state_init(&w, pid, options); - w.is_ruby = 0; w.ret = do_waitpid(w.pid, &w.status, w.options | WNOHANG); if (w.ret) { if (w.ret == -1) w.errnum = errno; } else { - w.wake.cond = cond; + w.cond = cond; + w.ec = 0; list_add(&vm->waiting_pids, &w.wnode); do { - rb_native_cond_wait(w.wake.cond, &vm->waitpid_lock); + rb_native_cond_wait(w.cond, &vm->waitpid_lock); } while (!w.ret); list_del(&w.wnode); } @@ -981,41 +989,63 @@ ruby_waitpid_locked(rb_vm_t *vm, rb_pid_t pid, int *status, int options, return w.ret; } -void rb_thread_sleep_interruptible(struct timespec *ts); /* thread.c */ +static void +waitpid_wake(void *x) +{ + struct waitpid_state *w = x; + + /* th->interrupt_lock is already held by rb_threadptr_interrupt_common */ + rb_native_cond_signal(w->cond); +} + +static void * +waitpid_nogvl(void *x) +{ + struct waitpid_state *w = x; + rb_thread_t *th = rb_ec_thread_ptr(w->ec); + + rb_native_mutex_lock(&th->interrupt_lock); + if (!w->ret) { /* we must check this before waiting */ + rb_native_cond_wait(w->cond, &th->interrupt_lock); + } + rb_native_mutex_unlock(&th->interrupt_lock); + + return 0; +} static VALUE waitpid_sleep(VALUE x) { struct waitpid_state *w = (struct waitpid_state *)x; - rb_thread_check_ints(); while (!w->ret) { - rb_thread_sleep_interruptible(0); - rb_thread_check_ints(); + rb_thread_call_without_gvl(waitpid_nogvl, w, waitpid_wake, w); } return Qfalse; } static VALUE -waitpid_ensure(VALUE x) +waitpid_cleanup(VALUE x) { struct waitpid_state *w = (struct waitpid_state *)x; if (w->ret == 0) { - rb_vm_t *vm = rb_ec_vm_ptr(w->wake.ec); + rb_vm_t *vm = rb_ec_vm_ptr(w->ec); rb_native_mutex_lock(&vm->waitpid_lock); list_del(&w->wnode); rb_native_mutex_unlock(&vm->waitpid_lock); } + rb_sleep_cond_put(w->cond); + return Qfalse; } static void waitpid_wait(struct waitpid_state *w) { - rb_vm_t *vm = rb_ec_vm_ptr(w->wake.ec); + rb_vm_t *vm = rb_ec_vm_ptr(w->ec); /* * Lock here to prevent do_waitpid from stealing work from the @@ -1025,16 +1055,19 @@ waitpid_wait(struct waitpid_state *w) rb_native_mutex_lock(&vm->waitpid_lock); w->ret = do_waitpid(w->pid, &w->status, w->options | WNOHANG); - if (w->ret) { + if (w->ret || (w->options & WNOHANG)) { + w->cond = 0; if (w->ret == -1) w->errnum = errno; - - rb_native_mutex_unlock(&vm->waitpid_lock); } else { + w->cond = rb_sleep_cond_get(w->ec); list_add(&vm->waiting_pids, &w->wnode); - rb_native_mutex_unlock(&vm->waitpid_lock); + } + + rb_native_mutex_unlock(&vm->waitpid_lock); - rb_ensure(waitpid_sleep, (VALUE)w, waitpid_ensure, (VALUE)w); + if (w->cond) { + rb_ensure(waitpid_sleep, (VALUE)w, waitpid_cleanup, (VALUE)w); } } @@ -1042,20 +1075,14 @@ rb_pid_t rb_waitpid(rb_pid_t pid, int *st, int flags) { rb_pid_t result; + struct waitpid_state w; - if (flags & WNOHANG) { - result = do_waitpid(pid, st, flags); - } - else { - struct waitpid_state w; + waitpid_state_init(&w, pid, flags); + w.ec = GET_EC(); + waitpid_wait(&w); + if (st) *st = w.status; + result = w.ret; - waitpid_state_init(&w, pid, flags); - w.is_ruby = 1; - w.wake.ec = GET_EC(); - waitpid_wait(&w); - if (st) *st = w.status; - result = w.ret; - } if (result > 0) { rb_last_status_set(*st, result); } diff --git a/signal.c b/signal.c index b816403cb9..9f74c43c5d 100644 --- a/signal.c +++ b/signal.c @@ -1042,7 +1042,22 @@ rb_trap_exit(void) } } -void rb_waitpid_all(rb_vm_t *); /* process.c */ +void ruby_waitpid_all(rb_vm_t *); /* process.c */ + +/* only runs in the timer-thread */ +void +ruby_sigchld_handler(rb_vm_t *vm) +{ + /* + * Checking signal_buff.cnt[RUBY_SIGCHLD] here is not completely + * reliable as it can race with rb_get_next_signal in the + * main thread. However, this remains useful when the main thread + * is blocked in an uninterruptible state: + */ + if (signal_buff.cnt[RUBY_SIGCHLD]) { + ruby_waitpid_all(vm); + } +} void rb_signal_exec(rb_thread_t *th, int sig) @@ -1051,8 +1066,12 @@ rb_signal_exec(rb_thread_t *th, int sig) VALUE cmd = vm->trap_list.cmd[sig]; int safe = vm->trap_list.safe[sig]; - if (sig == RUBY_SIGCHLD) { - rb_waitpid_all(vm); + /* + * This is necessary as rb_get_next_signal from this (main) thread + * can steal work from the timer-thread running ruby_sigchld_handler + */ + if (RUBY_SIGCHLD == sig) { + ruby_waitpid_all(vm); } if (cmd == 0) { diff --git a/thread.c b/thread.c index ab27c60632..66961efbf0 100644 --- a/thread.c +++ b/thread.c @@ -413,7 +413,10 @@ rb_vm_gvl_destroy(rb_vm_t *vm) gvl_release(vm); gvl_destroy(vm); rb_native_mutex_destroy(&vm->thread_destruct_lock); - rb_native_mutex_destroy(&vm->waitpid_lock); + if (0) { + /* may be held by running threads */ + rb_native_mutex_destroy(&vm->waitpid_lock); + } } void @@ -1287,17 +1290,6 @@ rb_thread_sleep_forever(void) sleep_forever(GET_THREAD(), SLEEP_SPURIOUS_CHECK); } -void -rb_thread_sleep_interruptible(struct timespec *ts) -{ - rb_thread_t *th = GET_THREAD(); - enum rb_thread_status prev_status = th->status; - - th->status = THREAD_STOPPED; - native_sleep(th, ts); - th->status = prev_status; -} - void rb_thread_sleep_deadly(void) { @@ -4143,6 +4135,9 @@ rb_gc_set_stack_end(VALUE **stack_end_p) #endif +/* signal.c */ +void ruby_sigchld_handler(rb_vm_t *); + /* * */ @@ -4175,6 +4170,7 @@ timer_thread_function(void *arg) rb_native_mutex_unlock(&vm->thread_destruct_lock); /* check signal */ + ruby_sigchld_handler(vm); rb_threadptr_check_signal(vm->main_thread); #if 0 @@ -5315,3 +5311,25 @@ rb_uninterruptible(VALUE (*b_proc)(ANYARGS), VALUE data) return rb_ensure(b_proc, data, rb_ary_pop, cur_th->pending_interrupt_mask_stack); } + +#ifndef USE_NATIVE_SLEEP_COND +# define USE_NATIVE_SLEEP_COND (0) +#endif + +#if !USE_NATIVE_SLEEP_COND +rb_nativethread_cond_t * +rb_sleep_cond_get(const rb_execution_context_t *ec) +{ + rb_nativethread_cond_t *cond = ALLOC(rb_nativethread_cond_t); + rb_native_cond_initialize(cond); + + return cond; +} + +void +rb_sleep_cond_put(rb_nativethread_cond_t *cond) +{ + rb_native_cond_destroy(cond); + xfree(cond); +} +#endif /* !USE_NATIVE_SLEEP_COND */ diff --git a/thread_pthread.c b/thread_pthread.c index 1a1a6fc0c6..4053a22eac 100644 --- a/thread_pthread.c +++ b/thread_pthread.c @@ -1764,4 +1764,22 @@ rb_thread_create_mjit_thread(void (*child_hook)(void), void (*worker_func)(void) return ret; } +#define USE_NATIVE_SLEEP_COND (1) + +#if USE_NATIVE_SLEEP_COND +rb_nativethread_cond_t * +rb_sleep_cond_get(const rb_execution_context_t *ec) +{ + rb_thread_t *th = rb_ec_thread_ptr(ec); + + return &th->native_thread_data.sleep_cond; +} + +void +rb_sleep_cond_put(rb_nativethread_cond_t *cond) +{ + /* no-op */ +} +#endif /* USE_NATIVE_SLEEP_COND */ + #endif /* THREAD_SYSTEM_DEPENDENT_IMPLEMENTATION */ -- EW