* [PATCH 1/8] hijack SIGCHLD handler for internal use
2018-06-25 23:50 [PATCHv3 0/8] mjit SIGCHLD hijack series Eric Wong
@ 2018-06-25 23:50 ` Eric Wong
2018-06-25 23:50 ` [PATCH 2/8] fix SIGCHLD hijacking race conditions Eric Wong
` (6 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Eric Wong @ 2018-06-25 23:50 UTC (permalink / raw)
To: spew; +Cc: Eric Wong
Use a global SIGCHLD handler to guard all callers of rb_waitpid.
To work safely with multi-threaded programs, we introduce a
VM-wide waitpid_lock to be acquired BEFORE fork/vfork spawns the
process. This is to be combined with the new ruby_waitpid_locked
function used by mjit.c in a non-Ruby thread.
LIFO ordering of the waiting_pids list matches Linux wait4
syscall behavior and ensures ordering of conflicting
rb_waitpid calls remains unchanged.
The disabling of SIGCHLD in rb_f_system is longer necessary,
as we use deferred signal handling and no longer make ANY
blocking waitpid syscalls in other threads which could "beat"
the waitpid call made by rb_f_system.
Since this SIGCHLD handling relies on normal Ruby VM interrupt
handling, we must continue to check VM interrupts during
mjit_finish.
Ruby-level SIGCHLD handlers registered with Signal.trap(:CHLD)
continues to work as before and there should be no regressions
in any existing use cases.
---
mjit.c | 50 ++++++++++++++---
process.c | 189 ++++++++++++++++++++++++++++++++++++++++++++++++++------------
signal.c | 40 +++++++++++--
thread.c | 2 +
vm_core.h | 3 +
5 files changed, 232 insertions(+), 52 deletions(-)
diff --git a/mjit.c b/mjit.c
index 07e9328aff..4aa1524a55 100644
--- a/mjit.c
+++ b/mjit.c
@@ -80,6 +80,7 @@
#include "constant.h"
#include "id_table.h"
#include "ruby_assert.h"
+#include "ruby/thread.h"
#include "ruby/util.h"
#include "ruby/version.h"
@@ -118,6 +119,9 @@ extern void rb_native_cond_wait(rb_nativethread_cond_t *cond, rb_nativethread_lo
extern int rb_thread_create_mjit_thread(void (*child_hook)(void), void (*worker_func)(void));
+
+pid_t ruby_waitpid_locked(rb_vm_t *, rb_pid_t, int *status, int options);
+
#define RB_CONDATTR_CLOCK_MONOTONIC 1
#ifdef _WIN32
@@ -403,21 +407,32 @@ exec_process(const char *path, char *const argv[])
{
int stat, exit_code;
pid_t pid;
+ rb_vm_t *vm = GET_VM();
+ rb_nativethread_lock_lock(&vm->waitpid_lock);
pid = start_process(path, argv);
- if (pid <= 0)
+ if (pid <= 0) {
+ rb_nativethread_lock_unlock(&vm->waitpid_lock);
return -2;
-
+ }
for (;;) {
- waitpid(pid, &stat, 0);
- if (WIFEXITED(stat)) {
- exit_code = WEXITSTATUS(stat);
- break;
- } else if (WIFSIGNALED(stat)) {
- exit_code = -1;
+ pid_t r = ruby_waitpid_locked(vm, pid, &stat, 0);
+ if (r == -1) {
+ if (errno == EINTR) continue;
+ fprintf(stderr, "waitpid: %s\n", strerror(errno));
break;
}
+ else if (r == pid) {
+ if (WIFEXITED(stat)) {
+ exit_code = WEXITSTATUS(stat);
+ break;
+ } else if (WIFSIGNALED(stat)) {
+ exit_code = -1;
+ break;
+ }
+ }
}
+ rb_nativethread_lock_unlock(&vm->waitpid_lock);
return exit_code;
}
@@ -1497,6 +1512,7 @@ stop_worker(void)
CRITICAL_SECTION_START(3, "in stop_worker");
rb_native_cond_broadcast(&mjit_worker_wakeup);
CRITICAL_SECTION_FINISH(3, "in stop_worker");
+ RUBY_VM_CHECK_INTS(GET_EC());
}
}
@@ -1532,6 +1548,21 @@ mjit_resume(void)
return Qtrue;
}
+static void *
+wait_pch(void *ignored)
+{
+ rb_native_cond_wait(&mjit_pch_wakeup, &mjit_engine_mutex);
+ return 0;
+}
+
+static void
+ubf_pch(void *ignored)
+{
+ rb_native_mutex_lock(&mjit_engine_mutex);
+ rb_native_cond_signal(&mjit_pch_wakeup);
+ rb_native_mutex_unlock(&mjit_engine_mutex);
+}
+
/* Finish the threads processing units and creating PCH, finalize
and free MJIT data. It should be called last during MJIT
life. */
@@ -1551,7 +1582,8 @@ mjit_finish(void)
absence. So wait for a clean finish of the threads. */
while (pch_status == PCH_NOT_READY) {
verbose(3, "Waiting wakeup from make_pch");
- rb_native_cond_wait(&mjit_pch_wakeup, &mjit_engine_mutex);
+ /* release GVL to handle interrupts */
+ rb_thread_call_without_gvl(wait_pch, 0, ubf_pch, 0);
}
CRITICAL_SECTION_FINISH(3, "in mjit_finish to wakeup from pch");
diff --git a/process.c b/process.c
index 12ea6eac86..c92351b83d 100644
--- a/process.c
+++ b/process.c
@@ -885,12 +885,6 @@ pst_wcoredump(VALUE st)
#endif
}
-struct waitpid_arg {
- rb_pid_t pid;
- int flags;
- int *st;
-};
-
static rb_pid_t
do_waitpid(rb_pid_t pid, int *st, int flags)
{
@@ -903,25 +897,155 @@ do_waitpid(rb_pid_t pid, int *st, int flags)
#endif
}
+struct waitpid_state {
+ struct list_node wnode;
+ rb_nativethread_cond_t cond;
+ rb_pid_t ret;
+ rb_pid_t pid;
+ int status;
+ int options;
+ int errnum;
+ rb_vm_t *vm;
+};
+
+void rb_native_cond_signal(rb_nativethread_cond_t *);
+void rb_native_cond_wait(rb_nativethread_cond_t *, rb_nativethread_lock_t *);
+void rb_native_cond_initialize(rb_nativethread_cond_t *);
+void rb_native_cond_destroy(rb_nativethread_cond_t *);
+
+/* only called by vm->main_thread */
+void
+rb_sigchld(rb_vm_t *vm)
+{
+ struct waitpid_state *w = 0, *next;
+
+ rb_nativethread_lock_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);
+ if (w->ret == 0) continue;
+ if (w->ret == -1) w->errnum = errno;
+ list_del_init(&w->wnode);
+ rb_native_cond_signal(&w->cond);
+ }
+ rb_nativethread_lock_unlock(&vm->waitpid_lock);
+}
+
+static void
+waitpid_state_init(struct waitpid_state *w, rb_vm_t *vm, pid_t pid, int options)
+{
+ rb_native_cond_initialize(&w->cond);
+ w->ret = 0;
+ w->pid = pid;
+ w->status = 0;
+ w->options = options;
+ w->vm = vm;
+ list_node_init(&w->wnode);
+}
+
+/* must be called with vm->waitpid_lock held, this is not interruptible */
+pid_t
+ruby_waitpid_locked(rb_vm_t *vm, rb_pid_t pid, int *status, int options)
+{
+ struct waitpid_state w;
+
+ assert(!ruby_thread_has_gvl_p() && "must not have GVL");
+
+ waitpid_state_init(&w, vm, pid, options);
+ w.ret = do_waitpid(w.pid, &w.status, w.options | WNOHANG);
+ if (w.ret) {
+ if (w.ret == -1) {
+ w.errnum = errno;
+ }
+ }
+ else {
+ list_add(&vm->waiting_pids, &w.wnode);
+ while (!w.ret) {
+ rb_native_cond_wait(&w.cond, &vm->waitpid_lock);
+ }
+ list_del(&w.wnode);
+ }
+ if (status) {
+ *status = w.status;
+ }
+ rb_native_cond_destroy(&w.cond);
+ errno = w.errnum;
+ return w.ret;
+}
+
+static void
+waitpid_ubf(void *x)
+{
+ struct waitpid_state *w = x;
+ rb_nativethread_lock_lock(&w->vm->waitpid_lock);
+ if (!w->ret) {
+ w->errnum = EINTR;
+ w->ret = -1;
+ }
+ rb_native_cond_signal(&w->cond);
+ rb_nativethread_lock_unlock(&w->vm->waitpid_lock);
+}
+
static void *
-rb_waitpid_blocking(void *data)
+waitpid_nogvl(void *x)
{
- struct waitpid_arg *arg = data;
- rb_pid_t result = do_waitpid(arg->pid, arg->st, arg->flags);
- return (void *)(VALUE)result;
+ struct waitpid_state *w = x;
+
+ /* let rb_sigchld handle it */
+ rb_native_cond_wait(&w->cond, &w->vm->waitpid_lock);
+
+ return 0;
}
-static rb_pid_t
-do_waitpid_nonblocking(rb_pid_t pid, int *st, int flags)
+static VALUE
+waitpid_wait(VALUE x)
+{
+ struct waitpid_state *w = (struct waitpid_state *)x;
+
+ rb_nativethread_lock_lock(&w->vm->waitpid_lock);
+ w->ret = do_waitpid(w->pid, &w->status, w->options | WNOHANG);
+ if (w->ret) {
+ if (w->ret == -1) {
+ w->errnum = errno;
+ }
+ }
+ else {
+ rb_execution_context_t *ec = GET_EC();
+
+ list_add(&w->vm->waiting_pids, &w->wnode);
+ do {
+ rb_thread_call_without_gvl2(waitpid_nogvl, w, waitpid_ubf, w);
+ if (RUBY_VM_INTERRUPTED_ANY(ec) ||
+ (w->ret == -1 && w->errnum == EINTR)) {
+ rb_nativethread_lock_unlock(&w->vm->waitpid_lock);
+
+ RUBY_VM_CHECK_INTS(ec);
+
+ rb_nativethread_lock_lock(&w->vm->waitpid_lock);
+ if (w->ret == -1 && w->errnum == EINTR) {
+ w->ret = do_waitpid(w->pid, &w->status, w->options|WNOHANG);
+ if (w->ret == -1)
+ w->errnum = errno;
+ }
+ }
+ } while (!w->ret);
+ }
+ rb_nativethread_lock_unlock(&w->vm->waitpid_lock);
+ return Qfalse;
+}
+
+static VALUE
+waitpid_ensure(VALUE x)
{
- void *result;
- struct waitpid_arg arg;
- arg.pid = pid;
- arg.st = st;
- arg.flags = flags;
- result = rb_thread_call_without_gvl(rb_waitpid_blocking, &arg,
- RUBY_UBF_PROCESS, 0);
- return (rb_pid_t)(VALUE)result;
+ struct waitpid_state *w = (struct waitpid_state *)x;
+
+ if (w->ret <= 0) {
+ rb_nativethread_lock_lock(&w->vm->waitpid_lock);
+ list_del_init(&w->wnode);
+ rb_nativethread_lock_unlock(&w->vm->waitpid_lock);
+ }
+
+ rb_native_cond_destroy(&w->cond);
+ return Qfalse;
}
rb_pid_t
@@ -933,10 +1057,14 @@ rb_waitpid(rb_pid_t pid, int *st, int flags)
result = do_waitpid(pid, st, flags);
}
else {
- while ((result = do_waitpid_nonblocking(pid, st, flags)) < 0 &&
- (errno == EINTR)) {
- RUBY_VM_CHECK_INTS(GET_EC());
- }
+ struct waitpid_state w;
+
+ waitpid_state_init(&w, GET_VM(), pid, flags);
+ rb_ensure(waitpid_wait, (VALUE)&w, waitpid_ensure, (VALUE)&w);
+ if (st) {
+ *st = w.status;
+ }
+ result = w.ret;
}
if (result > 0) {
rb_last_status_set(*st, result);
@@ -4086,16 +4214,6 @@ rb_f_system(int argc, VALUE *argv)
VALUE execarg_obj;
struct rb_execarg *eargp;
-#if defined(SIGCLD) && !defined(SIGCHLD)
-# define SIGCHLD SIGCLD
-#endif
-
-#ifdef SIGCHLD
- RETSIGTYPE (*chfunc)(int);
-
- rb_last_status_clear();
- chfunc = signal(SIGCHLD, SIG_DFL);
-#endif
execarg_obj = rb_execarg_new(argc, argv, TRUE, TRUE);
pid = rb_execarg_spawn(execarg_obj, NULL, 0);
#if defined(HAVE_WORKING_FORK) || defined(HAVE_SPAWNV)
@@ -4105,9 +4223,6 @@ rb_f_system(int argc, VALUE *argv)
if (ret == (rb_pid_t)-1)
rb_sys_fail("Another thread waited the process started by system().");
}
-#endif
-#ifdef SIGCHLD
- signal(SIGCHLD, chfunc);
#endif
TypedData_Get_Struct(execarg_obj, struct rb_execarg, &exec_arg_data_type, eargp);
if (pid < 0) {
diff --git a/signal.c b/signal.c
index c781c38c62..c20b01ea36 100644
--- a/signal.c
+++ b/signal.c
@@ -1052,6 +1052,19 @@ rb_trap_exit(void)
}
}
+static int
+sig_is_chld(int sig)
+{
+#if defined(SIGCLD)
+ return (sig == SIGCLD);
+#elif defined(SIGCHLD)
+ return (sig == SIGCHLD);
+#endif
+ return 0;
+}
+
+void rb_sigchld(rb_vm_t *); /* process.c */
+
void
rb_signal_exec(rb_thread_t *th, int sig)
{
@@ -1059,6 +1072,9 @@ 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_is_chld(sig)) {
+ rb_sigchld(vm);
+ }
if (cmd == 0) {
switch (sig) {
case SIGINT:
@@ -1117,6 +1133,11 @@ default_handler(int sig)
#endif
#ifdef SIGUSR2
case SIGUSR2:
+#endif
+#ifdef SIGCLD
+ case SIGCLD:
+#elif defined(SIGCHLD)
+ case SIGCHLD:
#endif
func = sighandler;
break;
@@ -1155,6 +1176,9 @@ trap_handler(VALUE *cmd, int sig)
VALUE command;
if (NIL_P(*cmd)) {
+ if (sig_is_chld(sig)) {
+ goto sig_dfl;
+ }
func = SIG_IGN;
}
else {
@@ -1175,6 +1199,9 @@ trap_handler(VALUE *cmd, int sig)
break;
case 14:
if (memcmp(cptr, "SYSTEM_DEFAULT", 14) == 0) {
+ if (sig_is_chld(sig)) {
+ goto sig_dfl;
+ }
func = SIG_DFL;
*cmd = 0;
}
@@ -1182,6 +1209,9 @@ trap_handler(VALUE *cmd, int sig)
case 7:
if (memcmp(cptr, "SIG_IGN", 7) == 0) {
sig_ign:
+ if (sig_is_chld(sig)) {
+ goto sig_dfl;
+ }
func = SIG_IGN;
*cmd = Qtrue;
}
@@ -1418,15 +1448,13 @@ static int
init_sigchld(int sig)
{
sighandler_t oldfunc;
+ sighandler_t func = sighandler;
oldfunc = ruby_signal(sig, SIG_DFL);
if (oldfunc == SIG_ERR) return -1;
- if (oldfunc != SIG_DFL && oldfunc != SIG_IGN) {
- ruby_signal(sig, oldfunc);
- }
- else {
- GET_VM()->trap_list.cmd[sig] = 0;
- }
+ ruby_signal(sig, func);
+ GET_VM()->trap_list.cmd[sig] = 0;
+
return 0;
}
diff --git a/thread.c b/thread.c
index 5a5e32379d..8c9aafe07a 100644
--- a/thread.c
+++ b/thread.c
@@ -413,6 +413,7 @@ 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);
}
void
@@ -4999,6 +5000,7 @@ Init_Thread(void)
gvl_init(th->vm);
gvl_acquire(th->vm, th);
rb_native_mutex_initialize(&th->vm->thread_destruct_lock);
+ rb_native_mutex_initialize(&th->vm->waitpid_lock);
rb_native_mutex_initialize(&th->interrupt_lock);
th->pending_interrupt_queue = rb_ary_tmp_new(0);
diff --git a/vm_core.h b/vm_core.h
index ee151195d5..d0689e5ba6 100644
--- a/vm_core.h
+++ b/vm_core.h
@@ -553,6 +553,8 @@ typedef struct rb_vm_struct {
#endif
rb_serial_t fork_gen;
+ rb_nativethread_lock_t waitpid_lock;
+ struct list_head waiting_pids; /* <=> struct waitpid_state */
struct list_head waiting_fds; /* <=> struct waiting_fd */
struct list_head living_threads;
VALUE thgroup_default;
@@ -1561,6 +1563,7 @@ static inline void
rb_vm_living_threads_init(rb_vm_t *vm)
{
list_head_init(&vm->waiting_fds);
+ list_head_init(&vm->waiting_pids);
list_head_init(&vm->living_threads);
vm->living_thread_num = 0;
}
--
EW
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 2/8] fix SIGCHLD hijacking race conditions
2018-06-25 23:50 [PATCHv3 0/8] mjit SIGCHLD hijack series Eric Wong
2018-06-25 23:50 ` [PATCH 1/8] hijack SIGCHLD handler for internal use Eric Wong
@ 2018-06-25 23:50 ` Eric Wong
2018-06-25 23:50 ` [PATCH 3/8] mjit.c: allow working on platforms without SIGCHLD Eric Wong
` (5 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Eric Wong @ 2018-06-25 23:50 UTC (permalink / raw)
To: spew; +Cc: Eric Wong
From: Eric Wong <normalperson@yhbt.net>
It's a bit tricky, and I introduced yet another sleep function
(rb_thread_sleep_interruptible) to make it work.
Original problem in my patch was holding a native mutex while
forking is dangerous, so I went to normal ruby sleep methods
(same as Mutex and autoload).
However, it's impossible to resume main thread from signal
handler because rb_threadptr_execute_interrupts always restores
th->status after trap handlers. In other words, this doesn't work:
main = Thread.current
trap(:USR1) { main.run }
Thread.new { sleep 0.1; Process.kill(:USR1, $$) }
sleep
So I tried moving rb_sigchld/rb_waitpid_all into timer-thread;
but that's racy and not doable unless the Ruby thread holds
a native mutex while sleeping on a native condvar (w/o GVL).
And that brings us back to the original problem...
(Well, I could use futex :P)
Now, back to using the main thread to handle SIGCHLD...
It seems to be working...
---
mjit.c | 13 ++++--
process.c | 156 +++++++++++++++++++++++++++++---------------------------------
signal.c | 66 +++++++++++---------------
thread.c | 11 +++++
4 files changed, 120 insertions(+), 126 deletions(-)
diff --git a/mjit.c b/mjit.c
index 4aa1524a55..63cd35da21 100644
--- a/mjit.c
+++ b/mjit.c
@@ -120,7 +120,8 @@ extern void rb_native_cond_wait(rb_nativethread_cond_t *cond, rb_nativethread_lo
extern int rb_thread_create_mjit_thread(void (*child_hook)(void), void (*worker_func)(void));
-pid_t ruby_waitpid_locked(rb_vm_t *, rb_pid_t, int *status, int options);
+pid_t ruby_waitpid_locked(rb_vm_t *, rb_pid_t, int *status, int options,
+ rb_nativethread_cond_t *cond);
#define RB_CONDATTR_CLOCK_MONOTONIC 1
@@ -408,6 +409,7 @@ exec_process(const char *path, char *const argv[])
int stat, exit_code;
pid_t pid;
rb_vm_t *vm = GET_VM();
+ rb_nativethread_cond_t cond;
rb_nativethread_lock_lock(&vm->waitpid_lock);
pid = start_process(path, argv);
@@ -415,11 +417,13 @@ exec_process(const char *path, char *const argv[])
rb_nativethread_lock_unlock(&vm->waitpid_lock);
return -2;
}
+ rb_native_cond_initialize(&cond);
for (;;) {
- pid_t r = ruby_waitpid_locked(vm, pid, &stat, 0);
+ pid_t r = ruby_waitpid_locked(vm, pid, &stat, 0, &cond);
if (r == -1) {
- if (errno == EINTR) continue;
+ if (errno == EINTR) continue; /* should never happen */
fprintf(stderr, "waitpid: %s\n", strerror(errno));
+ exit_code = -2;
break;
}
else if (r == pid) {
@@ -433,6 +437,7 @@ exec_process(const char *path, char *const argv[])
}
}
rb_nativethread_lock_unlock(&vm->waitpid_lock);
+ rb_native_cond_destroy(&cond);
return exit_code;
}
@@ -1582,7 +1587,7 @@ mjit_finish(void)
absence. So wait for a clean finish of the threads. */
while (pch_status == PCH_NOT_READY) {
verbose(3, "Waiting wakeup from make_pch");
- /* release GVL to handle interrupts */
+ /* release GVL to handle interrupts */
rb_thread_call_without_gvl(wait_pch, 0, ubf_pch, 0);
}
CRITICAL_SECTION_FINISH(3, "in mjit_finish to wakeup from pch");
diff --git a/process.c b/process.c
index c92351b83d..9f0995e194 100644
--- a/process.c
+++ b/process.c
@@ -899,153 +899,143 @@ do_waitpid(rb_pid_t pid, int *st, int flags)
struct waitpid_state {
struct list_node wnode;
- rb_nativethread_cond_t cond;
+ union {
+ rb_nativethread_cond_t *cond; /* non-Ruby thread */
+ rb_execution_context_t *ec; /* normal Ruby execution context */
+ } wake;
rb_pid_t ret;
rb_pid_t pid;
int status;
int options;
int errnum;
- rb_vm_t *vm;
+ 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 *);
-void rb_native_cond_initialize(rb_nativethread_cond_t *);
-void rb_native_cond_destroy(rb_nativethread_cond_t *);
-/* only called by vm->main_thread */
+/* called by vm->main_thread */
void
-rb_sigchld(rb_vm_t *vm)
+rb_waitpid_all(rb_vm_t *vm)
{
struct waitpid_state *w = 0, *next;
- rb_nativethread_lock_lock(&vm->waitpid_lock);
+ 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);
+
if (w->ret == 0) continue;
if (w->ret == -1) w->errnum = errno;
+
list_del_init(&w->wnode);
- rb_native_cond_signal(&w->cond);
+ if (w->is_ruby) {
+ rb_thread_wakeup_alive(rb_ec_thread_ptr(w->wake.ec)->self);
+ }
+ else {
+ rb_native_cond_signal(w->wake.cond);
+ }
}
- rb_nativethread_lock_unlock(&vm->waitpid_lock);
+ rb_native_mutex_unlock(&vm->waitpid_lock);
}
static void
-waitpid_state_init(struct waitpid_state *w, rb_vm_t *vm, pid_t pid, int options)
+waitpid_state_init(struct waitpid_state *w, pid_t pid, int options)
{
- rb_native_cond_initialize(&w->cond);
w->ret = 0;
w->pid = pid;
- w->status = 0;
w->options = options;
- w->vm = vm;
list_node_init(&w->wnode);
}
-/* must be called with vm->waitpid_lock held, this is not interruptible */
+/*
+ * must be called with vm->waitpid_lock held, this is not interruptible
+ */
pid_t
-ruby_waitpid_locked(rb_vm_t *vm, rb_pid_t pid, int *status, int options)
+ruby_waitpid_locked(rb_vm_t *vm, rb_pid_t pid, int *status, int options,
+ rb_nativethread_cond_t *cond)
{
struct waitpid_state w;
assert(!ruby_thread_has_gvl_p() && "must not have GVL");
- waitpid_state_init(&w, vm, pid, options);
+ 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;
- }
+ if (w.ret == -1) w.errnum = errno;
}
else {
+ w.wake.cond = cond;
list_add(&vm->waiting_pids, &w.wnode);
- while (!w.ret) {
- rb_native_cond_wait(&w.cond, &vm->waitpid_lock);
- }
+ do {
+ rb_native_cond_wait(w.wake.cond, &vm->waitpid_lock);
+ } while (!w.ret);
list_del(&w.wnode);
}
if (status) {
*status = w.status;
}
- rb_native_cond_destroy(&w.cond);
errno = w.errnum;
return w.ret;
}
-static void
-waitpid_ubf(void *x)
-{
- struct waitpid_state *w = x;
- rb_nativethread_lock_lock(&w->vm->waitpid_lock);
- if (!w->ret) {
- w->errnum = EINTR;
- w->ret = -1;
- }
- rb_native_cond_signal(&w->cond);
- rb_nativethread_lock_unlock(&w->vm->waitpid_lock);
-}
+void rb_thread_sleep_interruptible(struct timespec *ts); /* thread.c */
-static void *
-waitpid_nogvl(void *x)
+static VALUE
+waitpid_sleep(VALUE x)
{
- struct waitpid_state *w = x;
+ struct waitpid_state *w = (struct waitpid_state *)x;
- /* let rb_sigchld handle it */
- rb_native_cond_wait(&w->cond, &w->vm->waitpid_lock);
+ rb_thread_check_ints();
+ while (!w->ret) {
+ rb_thread_sleep_interruptible(0);
+ rb_thread_check_ints();
+ }
- return 0;
+ return Qfalse;
}
static VALUE
-waitpid_wait(VALUE x)
+waitpid_ensure(VALUE x)
{
struct waitpid_state *w = (struct waitpid_state *)x;
- rb_nativethread_lock_lock(&w->vm->waitpid_lock);
- w->ret = do_waitpid(w->pid, &w->status, w->options | WNOHANG);
- if (w->ret) {
- if (w->ret == -1) {
- w->errnum = errno;
- }
- }
- else {
- rb_execution_context_t *ec = GET_EC();
+ if (w->ret == 0) {
+ rb_vm_t *vm = rb_ec_vm_ptr(w->wake.ec);
- list_add(&w->vm->waiting_pids, &w->wnode);
- do {
- rb_thread_call_without_gvl2(waitpid_nogvl, w, waitpid_ubf, w);
- if (RUBY_VM_INTERRUPTED_ANY(ec) ||
- (w->ret == -1 && w->errnum == EINTR)) {
- rb_nativethread_lock_unlock(&w->vm->waitpid_lock);
-
- RUBY_VM_CHECK_INTS(ec);
-
- rb_nativethread_lock_lock(&w->vm->waitpid_lock);
- if (w->ret == -1 && w->errnum == EINTR) {
- w->ret = do_waitpid(w->pid, &w->status, w->options|WNOHANG);
- if (w->ret == -1)
- w->errnum = errno;
- }
- }
- } while (!w->ret);
+ rb_native_mutex_lock(&vm->waitpid_lock);
+ list_del(&w->wnode);
+ rb_native_mutex_unlock(&vm->waitpid_lock);
}
- rb_nativethread_lock_unlock(&w->vm->waitpid_lock);
return Qfalse;
}
-static VALUE
-waitpid_ensure(VALUE x)
+static void
+waitpid_wait(struct waitpid_state *w)
{
- struct waitpid_state *w = (struct waitpid_state *)x;
+ rb_vm_t *vm = rb_ec_vm_ptr(w->wake.ec);
- if (w->ret <= 0) {
- rb_nativethread_lock_lock(&w->vm->waitpid_lock);
- list_del_init(&w->wnode);
- rb_nativethread_lock_unlock(&w->vm->waitpid_lock);
+ /*
+ * Lock here to prevent do_waitpid from stealing work from the
+ * ruby_waitpid_locked done by mjit workers since mjit works
+ * outside of GVL
+ */
+ rb_native_mutex_lock(&vm->waitpid_lock);
+
+ w->ret = do_waitpid(w->pid, &w->status, w->options | WNOHANG);
+ if (w->ret) {
+ if (w->ret == -1) w->errnum = errno;
+
+ rb_native_mutex_unlock(&vm->waitpid_lock);
}
+ else {
+ list_add(&vm->waiting_pids, &w->wnode);
+ rb_native_mutex_unlock(&vm->waitpid_lock);
- rb_native_cond_destroy(&w->cond);
- return Qfalse;
+ rb_ensure(waitpid_sleep, (VALUE)w, waitpid_ensure, (VALUE)w);
+ }
}
rb_pid_t
@@ -1059,11 +1049,11 @@ rb_waitpid(rb_pid_t pid, int *st, int flags)
else {
struct waitpid_state w;
- waitpid_state_init(&w, GET_VM(), pid, flags);
- rb_ensure(waitpid_wait, (VALUE)&w, waitpid_ensure, (VALUE)&w);
- if (st) {
- *st = w.status;
- }
+ 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) {
diff --git a/signal.c b/signal.c
index c20b01ea36..22fc4286d9 100644
--- a/signal.c
+++ b/signal.c
@@ -66,6 +66,14 @@ ruby_atomic_compare_and_swap(rb_atomic_t *ptr, rb_atomic_t cmp,
# define NSIG (_SIGMAX + 1) /* For QNX */
#endif
+#if defined(SIGCLD)
+# define RUBY_SIGCHLD (SIGCLD)
+#elif defined(SIGCHLD)
+# define RUBY_SIGCHLD (SIGCHLD)
+#else
+# define RUBY_SIGCHLD (0)
+#endif
+
static const struct signals {
const char *signm;
int signo;
@@ -129,15 +137,9 @@ static const struct signals {
#ifdef SIGCONT
{"CONT", SIGCONT},
#endif
-#ifdef SIGCHLD
- {"CHLD", SIGCHLD},
-#endif
-#ifdef SIGCLD
- {"CLD", SIGCLD},
-#else
-# ifdef SIGCHLD
- {"CLD", SIGCHLD},
-# endif
+#if RUBY_SIGCHLD
+ {"CHLD", RUBY_SIGCHLD },
+ {"CLD", RUBY_SIGCHLD },
#endif
#ifdef SIGTTIN
{"TTIN", SIGTTIN},
@@ -1052,18 +1054,7 @@ rb_trap_exit(void)
}
}
-static int
-sig_is_chld(int sig)
-{
-#if defined(SIGCLD)
- return (sig == SIGCLD);
-#elif defined(SIGCHLD)
- return (sig == SIGCHLD);
-#endif
- return 0;
-}
-
-void rb_sigchld(rb_vm_t *); /* process.c */
+void rb_waitpid_all(rb_vm_t *); /* process.c */
void
rb_signal_exec(rb_thread_t *th, int sig)
@@ -1072,9 +1063,10 @@ 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_is_chld(sig)) {
- rb_sigchld(vm);
+ if (sig == RUBY_SIGCHLD) {
+ rb_waitpid_all(vm);
}
+
if (cmd == 0) {
switch (sig) {
case SIGINT:
@@ -1134,10 +1126,8 @@ default_handler(int sig)
#ifdef SIGUSR2
case SIGUSR2:
#endif
-#ifdef SIGCLD
- case SIGCLD:
-#elif defined(SIGCHLD)
- case SIGCHLD:
+#if RUBY_SIGCHLD
+ case RUBY_SIGCHLD:
#endif
func = sighandler;
break;
@@ -1176,7 +1166,7 @@ trap_handler(VALUE *cmd, int sig)
VALUE command;
if (NIL_P(*cmd)) {
- if (sig_is_chld(sig)) {
+ if (sig == RUBY_SIGCHLD) {
goto sig_dfl;
}
func = SIG_IGN;
@@ -1199,9 +1189,9 @@ trap_handler(VALUE *cmd, int sig)
break;
case 14:
if (memcmp(cptr, "SYSTEM_DEFAULT", 14) == 0) {
- if (sig_is_chld(sig)) {
- goto sig_dfl;
- }
+ if (sig == RUBY_SIGCHLD) {
+ goto sig_dfl;
+ }
func = SIG_DFL;
*cmd = 0;
}
@@ -1209,9 +1199,9 @@ trap_handler(VALUE *cmd, int sig)
case 7:
if (memcmp(cptr, "SIG_IGN", 7) == 0) {
sig_ign:
- if (sig_is_chld(sig)) {
- goto sig_dfl;
- }
+ if (sig == RUBY_SIGCHLD) {
+ goto sig_dfl;
+ }
func = SIG_IGN;
*cmd = Qtrue;
}
@@ -1443,7 +1433,7 @@ install_sighandler(int signum, sighandler_t handler)
# define install_sighandler(signum, handler) \
INSTALL_SIGHANDLER(install_sighandler(signum, handler), #signum, signum)
-#if defined(SIGCLD) || defined(SIGCHLD)
+#if RUBY_SIGCHLD
static int
init_sigchld(int sig)
{
@@ -1570,10 +1560,8 @@ Init_signal(void)
install_sighandler(SIGSYS, sig_do_nothing);
#endif
-#if defined(SIGCLD)
- init_sigchld(SIGCLD);
-#elif defined(SIGCHLD)
- init_sigchld(SIGCHLD);
+#if RUBY_SIGCHLD
+ init_sigchld(RUBY_SIGCHLD);
#endif
rb_enable_interrupt();
diff --git a/thread.c b/thread.c
index 8c9aafe07a..ab27c60632 100644
--- a/thread.c
+++ b/thread.c
@@ -1287,6 +1287,17 @@ 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)
{
--
EW
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 3/8] mjit.c: allow working on platforms without SIGCHLD
2018-06-25 23:50 [PATCHv3 0/8] mjit SIGCHLD hijack series Eric Wong
2018-06-25 23:50 ` [PATCH 1/8] hijack SIGCHLD handler for internal use Eric Wong
2018-06-25 23:50 ` [PATCH 2/8] fix SIGCHLD hijacking race conditions Eric Wong
@ 2018-06-25 23:50 ` Eric Wong
2018-06-25 23:50 ` [PATCH 4/8] cleanups Eric Wong
` (4 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Eric Wong @ 2018-06-25 23:50 UTC (permalink / raw)
To: spew; +Cc: Eric Wong
While we're at it, simplify lock management in exec_process by
avoiding early returns.
---
mjit.c | 33 ++++++++++++++++++++-------------
1 file changed, 20 insertions(+), 13 deletions(-)
diff --git a/mjit.c b/mjit.c
index 63cd35da21..659461cea6 100644
--- a/mjit.c
+++ b/mjit.c
@@ -137,6 +137,7 @@ pid_t ruby_waitpid_locked(rb_vm_t *, rb_pid_t, int *status, int options,
#define WEXITSTATUS(S) (S)
#define WIFSIGNALED(S) (0)
typedef intptr_t pid_t;
+#define USE_RUBY_WAITPID_LOCKED (0)
#endif
/* Atomically set function pointer if possible. */
@@ -150,6 +151,10 @@ typedef intptr_t pid_t;
# define MJIT_ATOMIC_SET(var, val) ATOMIC_SET(var, val)
#endif
+#ifndef USE_RUBY_WAITPID_LOCKED /* platforms with real waitpid */
+# define USE_RUBY_WAITPID_LOCKED (1)
+#endif /* USE_RUBY_WAITPID_LOCKED */
+
/* A copy of MJIT portion of MRI options since MJIT initialization. We
need them as MJIT threads still can work when the most MRI data were
freed. */
@@ -406,24 +411,23 @@ start_process(const char *path, char *const *argv)
static int
exec_process(const char *path, char *const argv[])
{
- int stat, exit_code;
+ int stat, exit_code = -2;
pid_t pid;
- rb_vm_t *vm = GET_VM();
+ rb_vm_t *vm = USE_RUBY_WAITPID_LOCKED ? GET_VM() : 0;
rb_nativethread_cond_t cond;
- rb_nativethread_lock_lock(&vm->waitpid_lock);
- pid = start_process(path, argv);
- if (pid <= 0) {
- rb_nativethread_lock_unlock(&vm->waitpid_lock);
- return -2;
+ if (vm) {
+ rb_native_cond_initialize(&cond);
+ rb_nativethread_lock_lock(&vm->waitpid_lock);
}
- rb_native_cond_initialize(&cond);
- for (;;) {
- pid_t r = ruby_waitpid_locked(vm, pid, &stat, 0, &cond);
+
+ pid = start_process(path, argv);
+ for (;pid > 0;) {
+ pid_t r = vm ? ruby_waitpid_locked(vm, pid, &stat, 0, &cond)
+ : waitpid(pid, &stat, 0);
if (r == -1) {
if (errno == EINTR) continue; /* should never happen */
fprintf(stderr, "waitpid: %s\n", strerror(errno));
- exit_code = -2;
break;
}
else if (r == pid) {
@@ -436,8 +440,11 @@ exec_process(const char *path, char *const argv[])
}
}
}
- rb_nativethread_lock_unlock(&vm->waitpid_lock);
- rb_native_cond_destroy(&cond);
+
+ if (vm) {
+ rb_native_cond_destroy(&cond);
+ rb_nativethread_lock_unlock(&vm->waitpid_lock);
+ }
return exit_code;
}
--
EW
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 4/8] cleanups
2018-06-25 23:50 [PATCHv3 0/8] mjit SIGCHLD hijack series Eric Wong
` (2 preceding siblings ...)
2018-06-25 23:50 ` [PATCH 3/8] mjit.c: allow working on platforms without SIGCHLD Eric Wong
@ 2018-06-25 23:50 ` Eric Wong
2018-06-25 23:50 ` [PATCH 5/8] handle SIGCHLD in both the timer-thread and main thread Eric Wong
` (3 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Eric Wong @ 2018-06-25 23:50 UTC (permalink / raw)
To: spew; +Cc: Eric Wong
---
mjit.c | 9 ++-------
signal.c | 12 ------------
vm_core.h | 8 ++++++++
3 files changed, 10 insertions(+), 19 deletions(-)
diff --git a/mjit.c b/mjit.c
index 659461cea6..a550d1aabd 100644
--- a/mjit.c
+++ b/mjit.c
@@ -119,7 +119,7 @@ extern void rb_native_cond_wait(rb_nativethread_cond_t *cond, rb_nativethread_lo
extern int rb_thread_create_mjit_thread(void (*child_hook)(void), void (*worker_func)(void));
-
+/* process.c */
pid_t ruby_waitpid_locked(rb_vm_t *, rb_pid_t, int *status, int options,
rb_nativethread_cond_t *cond);
@@ -137,7 +137,6 @@ pid_t ruby_waitpid_locked(rb_vm_t *, rb_pid_t, int *status, int options,
#define WEXITSTATUS(S) (S)
#define WIFSIGNALED(S) (0)
typedef intptr_t pid_t;
-#define USE_RUBY_WAITPID_LOCKED (0)
#endif
/* Atomically set function pointer if possible. */
@@ -151,10 +150,6 @@ typedef intptr_t pid_t;
# define MJIT_ATOMIC_SET(var, val) ATOMIC_SET(var, val)
#endif
-#ifndef USE_RUBY_WAITPID_LOCKED /* platforms with real waitpid */
-# define USE_RUBY_WAITPID_LOCKED (1)
-#endif /* USE_RUBY_WAITPID_LOCKED */
-
/* A copy of MJIT portion of MRI options since MJIT initialization. We
need them as MJIT threads still can work when the most MRI data were
freed. */
@@ -413,7 +408,7 @@ exec_process(const char *path, char *const argv[])
{
int stat, exit_code = -2;
pid_t pid;
- rb_vm_t *vm = USE_RUBY_WAITPID_LOCKED ? GET_VM() : 0;
+ rb_vm_t *vm = RUBY_SIGCHLD ? GET_VM() : 0;
rb_nativethread_cond_t cond;
if (vm) {
diff --git a/signal.c b/signal.c
index 22fc4286d9..b816403cb9 100644
--- a/signal.c
+++ b/signal.c
@@ -62,18 +62,6 @@ ruby_atomic_compare_and_swap(rb_atomic_t *ptr, rb_atomic_t cmp,
}
#endif
-#ifndef NSIG
-# define NSIG (_SIGMAX + 1) /* For QNX */
-#endif
-
-#if defined(SIGCLD)
-# define RUBY_SIGCHLD (SIGCLD)
-#elif defined(SIGCHLD)
-# define RUBY_SIGCHLD (SIGCHLD)
-#else
-# define RUBY_SIGCHLD (0)
-#endif
-
static const struct signals {
const char *signm;
int signo;
diff --git a/vm_core.h b/vm_core.h
index d0689e5ba6..936b22d439 100644
--- a/vm_core.h
+++ b/vm_core.h
@@ -92,6 +92,14 @@
#define RUBY_NSIG NSIG
+#if defined(SIGCLD)
+# define RUBY_SIGCHLD (SIGCLD)
+#elif defined(SIGCHLD)
+# define RUBY_SIGCHLD (SIGCHLD)
+#else
+# define RUBY_SIGCHLD (0)
+#endif
+
#ifdef HAVE_STDARG_PROTOTYPES
#include <stdarg.h>
#define va_init_list(a,b) va_start((a),(b))
--
EW
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 5/8] handle SIGCHLD in both the timer-thread and main thread
2018-06-25 23:50 [PATCHv3 0/8] mjit SIGCHLD hijack series Eric Wong
` (3 preceding siblings ...)
2018-06-25 23:50 ` [PATCH 4/8] cleanups Eric Wong
@ 2018-06-25 23:50 ` Eric Wong
2018-06-25 23:50 ` [PATCH 6/8] Revert "test_process.rb: skip tests for Bug 14867" Eric Wong
` (2 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Eric Wong @ 2018-06-25 23:50 UTC (permalink / raw)
To: spew; +Cc: Eric Wong
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 6/8] Revert "test_process.rb: skip tests for Bug 14867"
2018-06-25 23:50 [PATCHv3 0/8] mjit SIGCHLD hijack series Eric Wong
` (4 preceding siblings ...)
2018-06-25 23:50 ` [PATCH 5/8] handle SIGCHLD in both the timer-thread and main thread Eric Wong
@ 2018-06-25 23:50 ` Eric Wong
2018-06-25 23:50 ` [PATCH 7/8] Revert "spec: skip Process wait specs on MJIT" Eric Wong
2018-06-25 23:50 ` [PATCH 8/8] wip testing Eric Wong
7 siblings, 0 replies; 9+ messages in thread
From: Eric Wong @ 2018-06-25 23:50 UTC (permalink / raw)
To: spew; +Cc: Eric Wong
This reverts r63740 (commit 042395a7f7262464265ce70cdffbe1812aa145d3).
---
test/ruby/test_process.rb | 3 ---
1 file changed, 3 deletions(-)
diff --git a/test/ruby/test_process.rb b/test/ruby/test_process.rb
index 8b7d8f0c57..a9052b4706 100644
--- a/test/ruby/test_process.rb
+++ b/test/ruby/test_process.rb
@@ -1426,7 +1426,6 @@ def test_status_quit
end
def test_wait_without_arg
- skip "[Bug #14867]" if RubyVM::MJIT.enabled?
with_tmpchdir do
write_file("foo", "sleep 0.1")
pid = spawn(RUBY, "foo")
@@ -1435,7 +1434,6 @@ def test_wait_without_arg
end
def test_wait2
- skip "[Bug #14867]" if RubyVM::MJIT.enabled?
with_tmpchdir do
write_file("foo", "sleep 0.1")
pid = spawn(RUBY, "foo")
@@ -1444,7 +1442,6 @@ def test_wait2
end
def test_waitall
- skip "[Bug #14867]" if RubyVM::MJIT.enabled?
with_tmpchdir do
write_file("foo", "sleep 0.1")
ps = (0...3).map { spawn(RUBY, "foo") }.sort
--
EW
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 7/8] Revert "spec: skip Process wait specs on MJIT"
2018-06-25 23:50 [PATCHv3 0/8] mjit SIGCHLD hijack series Eric Wong
` (5 preceding siblings ...)
2018-06-25 23:50 ` [PATCH 6/8] Revert "test_process.rb: skip tests for Bug 14867" Eric Wong
@ 2018-06-25 23:50 ` Eric Wong
2018-06-25 23:50 ` [PATCH 8/8] wip testing Eric Wong
7 siblings, 0 replies; 9+ messages in thread
From: Eric Wong @ 2018-06-25 23:50 UTC (permalink / raw)
To: spew; +Cc: Eric Wong
This reverts r63731 (commit 359dd59db2512d801bb983f98bede4fc598f139a).
---
spec/mspec/lib/mspec/guards/feature.rb | 6 --
spec/ruby/core/process/wait2_spec.rb | 26 ++++---
spec/ruby/core/process/wait_spec.rb | 122 ++++++++++++++++-----------------
spec/ruby/core/process/waitall_spec.rb | 66 +++++++++---------
4 files changed, 104 insertions(+), 116 deletions(-)
diff --git a/spec/mspec/lib/mspec/guards/feature.rb b/spec/mspec/lib/mspec/guards/feature.rb
index 4f1df1cabe..30984e0cc5 100644
--- a/spec/mspec/lib/mspec/guards/feature.rb
+++ b/spec/mspec/lib/mspec/guards/feature.rb
@@ -39,9 +39,3 @@ def match?
def with_feature(*features, &block)
FeatureGuard.new(*features).run_if(:with_feature, &block)
end
-
-MSpecEnv.class_eval do
- def without_feature(*features, &block)
- FeatureGuard.new(*features).run_unless(:without_feature, &block)
- end
-end
diff --git a/spec/ruby/core/process/wait2_spec.rb b/spec/ruby/core/process/wait2_spec.rb
index 3f5aa3c7e2..cb082541f9 100644
--- a/spec/ruby/core/process/wait2_spec.rb
+++ b/spec/ruby/core/process/wait2_spec.rb
@@ -14,21 +14,19 @@
end
end
- without_feature :mjit do # [Bug #14867]
- platform_is_not :windows do
- it "returns the pid and status of child process" do
- pidf = Process.fork { Process.exit! 99 }
- results = Process.wait2
- results.size.should == 2
- pidw, status = results
- pidf.should == pidw
- status.exitstatus.should == 99
- end
+ platform_is_not :windows do
+ it "returns the pid and status of child process" do
+ pidf = Process.fork { Process.exit! 99 }
+ results = Process.wait2
+ results.size.should == 2
+ pidw, status = results
+ pidf.should == pidw
+ status.exitstatus.should == 99
end
+ end
- it "raises a StandardError if no child processes exist" do
- lambda { Process.wait2 }.should raise_error(Errno::ECHILD)
- lambda { Process.wait2 }.should raise_error(StandardError)
- end
+ it "raises a StandardError if no child processes exist" do
+ lambda { Process.wait2 }.should raise_error(Errno::ECHILD)
+ lambda { Process.wait2 }.should raise_error(StandardError)
end
end
diff --git a/spec/ruby/core/process/wait_spec.rb b/spec/ruby/core/process/wait_spec.rb
index 447c62f42a..f11b079c16 100644
--- a/spec/ruby/core/process/wait_spec.rb
+++ b/spec/ruby/core/process/wait_spec.rb
@@ -12,81 +12,79 @@
end
end
- without_feature :mjit do # [Bug #14867]
- it "raises an Errno::ECHILD if there are no child processes" do
- lambda { Process.wait }.should raise_error(Errno::ECHILD)
+ it "raises an Errno::ECHILD if there are no child processes" do
+ lambda { Process.wait }.should raise_error(Errno::ECHILD)
+ end
+
+ platform_is_not :windows do
+ it "returns its childs pid" do
+ pid = Process.spawn(ruby_cmd('exit'))
+ Process.wait.should == pid
end
- platform_is_not :windows do
- it "returns its childs pid" do
- pid = Process.spawn(ruby_cmd('exit'))
- Process.wait.should == pid
- end
+ it "sets $? to a Process::Status" do
+ pid = Process.spawn(ruby_cmd('exit'))
+ Process.wait
+ $?.should be_kind_of(Process::Status)
+ $?.pid.should == pid
+ end
- it "sets $? to a Process::Status" do
- pid = Process.spawn(ruby_cmd('exit'))
- Process.wait
- $?.should be_kind_of(Process::Status)
- $?.pid.should == pid
- end
+ it "waits for any child process if no pid is given" do
+ pid = Process.spawn(ruby_cmd('exit'))
+ Process.wait.should == pid
+ lambda { Process.kill(0, pid) }.should raise_error(Errno::ESRCH)
+ end
- it "waits for any child process if no pid is given" do
- pid = Process.spawn(ruby_cmd('exit'))
- Process.wait.should == pid
- lambda { Process.kill(0, pid) }.should raise_error(Errno::ESRCH)
- end
+ it "waits for a specific child if a pid is given" do
+ pid1 = Process.spawn(ruby_cmd('exit'))
+ pid2 = Process.spawn(ruby_cmd('exit'))
+ Process.wait(pid2).should == pid2
+ Process.wait(pid1).should == pid1
+ lambda { Process.kill(0, pid1) }.should raise_error(Errno::ESRCH)
+ lambda { Process.kill(0, pid2) }.should raise_error(Errno::ESRCH)
+ end
- it "waits for a specific child if a pid is given" do
- pid1 = Process.spawn(ruby_cmd('exit'))
- pid2 = Process.spawn(ruby_cmd('exit'))
- Process.wait(pid2).should == pid2
- Process.wait(pid1).should == pid1
- lambda { Process.kill(0, pid1) }.should raise_error(Errno::ESRCH)
- lambda { Process.kill(0, pid2) }.should raise_error(Errno::ESRCH)
- end
+ it "coerces the pid to an Integer" do
+ pid1 = Process.spawn(ruby_cmd('exit'))
+ Process.wait(mock_int(pid1)).should == pid1
+ lambda { Process.kill(0, pid1) }.should raise_error(Errno::ESRCH)
+ end
- it "coerces the pid to an Integer" do
- pid1 = Process.spawn(ruby_cmd('exit'))
- Process.wait(mock_int(pid1)).should == pid1
- lambda { Process.kill(0, pid1) }.should raise_error(Errno::ESRCH)
- end
+ # This spec is probably system-dependent.
+ it "waits for a child whose process group ID is that of the calling process" do
+ pid1 = Process.spawn(ruby_cmd('exit'), pgroup: true)
+ pid2 = Process.spawn(ruby_cmd('exit'))
- # This spec is probably system-dependent.
- it "waits for a child whose process group ID is that of the calling process" do
- pid1 = Process.spawn(ruby_cmd('exit'), pgroup: true)
- pid2 = Process.spawn(ruby_cmd('exit'))
+ Process.wait(0).should == pid2
+ Process.wait.should == pid1
+ end
- Process.wait(0).should == pid2
- Process.wait.should == pid1
+ # This spec is probably system-dependent.
+ it "doesn't block if no child is available when WNOHANG is used" do
+ read, write = IO.pipe
+ pid = Process.fork do
+ read.close
+ Signal.trap("TERM") { Process.exit! }
+ write << 1
+ write.close
+ sleep
end
- # This spec is probably system-dependent.
- it "doesn't block if no child is available when WNOHANG is used" do
- read, write = IO.pipe
- pid = Process.fork do
- read.close
- Signal.trap("TERM") { Process.exit! }
- write << 1
- write.close
- sleep
- end
-
- Process.wait(pid, Process::WNOHANG).should be_nil
+ Process.wait(pid, Process::WNOHANG).should be_nil
- # wait for the child to setup its TERM handler
- write.close
- read.read(1)
- read.close
+ # wait for the child to setup its TERM handler
+ write.close
+ read.read(1)
+ read.close
- Process.kill("TERM", pid)
- Process.wait.should == pid
- end
+ Process.kill("TERM", pid)
+ Process.wait.should == pid
+ end
- it "always accepts flags=0" do
- pid = Process.spawn(ruby_cmd('exit'))
- Process.wait(-1, 0).should == pid
- lambda { Process.kill(0, pid) }.should raise_error(Errno::ESRCH)
- end
+ it "always accepts flags=0" do
+ pid = Process.spawn(ruby_cmd('exit'))
+ Process.wait(-1, 0).should == pid
+ lambda { Process.kill(0, pid) }.should raise_error(Errno::ESRCH)
end
end
end
diff --git a/spec/ruby/core/process/waitall_spec.rb b/spec/ruby/core/process/waitall_spec.rb
index ff06ae21f9..bdc1ea7490 100644
--- a/spec/ruby/core/process/waitall_spec.rb
+++ b/spec/ruby/core/process/waitall_spec.rb
@@ -8,43 +8,41 @@
end
end
- without_feature :mjit do # [Bug #14867]
- it "returns an empty array when there are no children" do
- Process.waitall.should == []
- end
+ it "returns an empty array when there are no children" do
+ Process.waitall.should == []
+ end
- it "takes no arguments" do
- lambda { Process.waitall(0) }.should raise_error(ArgumentError)
- end
+ it "takes no arguments" do
+ lambda { Process.waitall(0) }.should raise_error(ArgumentError)
+ end
- platform_is_not :windows do
- it "waits for all children" do
- pids = []
- pids << Process.fork { Process.exit! 2 }
- pids << Process.fork { Process.exit! 1 }
- pids << Process.fork { Process.exit! 0 }
- Process.waitall
- pids.each { |pid|
- lambda { Process.kill(0, pid) }.should raise_error(Errno::ESRCH)
- }
- end
+ platform_is_not :windows do
+ it "waits for all children" do
+ pids = []
+ pids << Process.fork { Process.exit! 2 }
+ pids << Process.fork { Process.exit! 1 }
+ pids << Process.fork { Process.exit! 0 }
+ Process.waitall
+ pids.each { |pid|
+ lambda { Process.kill(0, pid) }.should raise_error(Errno::ESRCH)
+ }
+ end
- it "returns an array of pid/status pairs" do
- pids = []
- pids << Process.fork { Process.exit! 2 }
- pids << Process.fork { Process.exit! 1 }
- pids << Process.fork { Process.exit! 0 }
- a = Process.waitall
- a.should be_kind_of(Array)
- a.size.should == 3
- pids.each { |pid|
- pid_status = a.assoc(pid)
- pid_status.should be_kind_of(Array)
- pid_status.size.should == 2
- pid_status.first.should == pid
- pid_status.last.should be_kind_of(Process::Status)
- }
- end
+ it "returns an array of pid/status pairs" do
+ pids = []
+ pids << Process.fork { Process.exit! 2 }
+ pids << Process.fork { Process.exit! 1 }
+ pids << Process.fork { Process.exit! 0 }
+ a = Process.waitall
+ a.should be_kind_of(Array)
+ a.size.should == 3
+ pids.each { |pid|
+ pid_status = a.assoc(pid)
+ pid_status.should be_kind_of(Array)
+ pid_status.size.should == 2
+ pid_status.first.should == pid
+ pid_status.last.should be_kind_of(Process::Status)
+ }
end
end
end
--
EW
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 8/8] wip testing
2018-06-25 23:50 [PATCHv3 0/8] mjit SIGCHLD hijack series Eric Wong
` (6 preceding siblings ...)
2018-06-25 23:50 ` [PATCH 7/8] Revert "spec: skip Process wait specs on MJIT" Eric Wong
@ 2018-06-25 23:50 ` Eric Wong
7 siblings, 0 replies; 9+ messages in thread
From: Eric Wong @ 2018-06-25 23:50 UTC (permalink / raw)
To: spew; +Cc: Eric Wong
---
mjit.c | 5 +++--
process.c | 12 +++++++++---
2 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/mjit.c b/mjit.c
index a550d1aabd..2e11395851 100644
--- a/mjit.c
+++ b/mjit.c
@@ -421,8 +421,9 @@ exec_process(const char *path, char *const argv[])
pid_t r = vm ? ruby_waitpid_locked(vm, pid, &stat, 0, &cond)
: waitpid(pid, &stat, 0);
if (r == -1) {
- if (errno == EINTR) continue; /* should never happen */
- fprintf(stderr, "waitpid: %s\n", strerror(errno));
+ if (errno == EINTR) continue;
+ fprintf(stderr, "[%d] waitpid(%d): %s\n",
+ getpid(), pid, strerror(errno));
break;
}
else if (r == pid) {
diff --git a/process.c b/process.c
index 0b91b30075..ce1445fbaa 100644
--- a/process.c
+++ b/process.c
@@ -919,7 +919,6 @@ 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);
}
@@ -933,7 +932,9 @@ ruby_waitpid_all(rb_vm_t *vm)
rb_native_mutex_lock(&vm->waitpid_lock);
list_for_each_safe(&vm->waiting_pids, w, next, wnode) {
pid_t ret = do_waitpid(w->pid, &w->status, w->options | WNOHANG);
+
if (!ret) continue;
+ if (ret == -1) w->errnum = errno;
if (w->ec) { /* rb_waitpid */
rb_thread_t *th = rb_ec_thread_ptr(w->ec);
@@ -985,7 +986,7 @@ ruby_waitpid_locked(rb_vm_t *vm, rb_pid_t pid, int *status, int options,
if (status) {
*status = w.status;
}
- errno = w.errnum;
+ if (w.ret == -1) errno = w.errnum;
return w.ret;
}
@@ -1061,7 +1062,11 @@ waitpid_wait(struct waitpid_state *w)
}
else {
w->cond = rb_sleep_cond_get(w->ec);
- list_add(&vm->waiting_pids, &w->wnode);
+ /* order matters, favor specified PIDs rather than -1 or 0 */
+ if (w->pid > 0)
+ list_add(&vm->waiting_pids, &w->wnode);
+ else
+ list_add_tail(&vm->waiting_pids, &w->wnode);
}
rb_native_mutex_unlock(&vm->waitpid_lock);
@@ -1086,6 +1091,7 @@ rb_waitpid(rb_pid_t pid, int *st, int flags)
if (result > 0) {
rb_last_status_set(*st, result);
}
+ if (w.ret == -1) errno = w.errnum;
return result;
}
--
EW
^ permalink raw reply related [flat|nested] 9+ messages in thread