@hsbt: I post here on ruby-core because I hit errors with https://bugs.ruby-lang.org/ just now -------8<----- Subject: [PATCH] vm_trace.c (postponed_job_register): only hit main thread Since postponed_job_register may be called in a signal handler, only the main thread is safe to touch as other threads may become invalid. Furthermore, the problem with trap interrupt being lost during ec_switch [Bug #14939] also applies to the postponed job and timer interrupts, so we need to preserve all three interrupts in ec_switch. Note: A minor problem is a possible crash during/after ruby_vm_destruct if postponed jobs are registered. The correct and performant fix would be to leak memory at exit for `vm' and `vm->main_thread'. free(3) slows down short-lived scripts, as does unregistering signal handlers. * vm_trace.c (postponed_job_register): only hit main thread * cont.c (ec_switch): preserve postponed and timer interrupt flags, too --- cont.c | 30 +++++++++++++++++++++++------- vm_trace.c | 16 +++++++--------- 2 files changed, 30 insertions(+), 16 deletions(-) diff --git a/cont.c b/cont.c index 95f2de9b87..5b73f5eac4 100644 --- a/cont.c +++ b/cont.c @@ -261,14 +261,30 @@ ec_switch(rb_thread_t *th, rb_fiber_t *fib) { rb_execution_context_t *ec = &fib->cont.saved_ec; - ruby_current_execution_context_ptr = th->ec = ec; + if (th->vm->main_thread == th) { + /* + * Other thread may set interrupt on previous th->ec at any time; + * ensure we do not delay (or lose) the trap interrupt handling. + */ + rb_atomic_t old_fl, new_fl; + rb_execution_context_t *prev_ec = ruby_current_execution_context_ptr; - /* - * timer-thread may set trap interrupt on previous th->ec at any time; - * ensure we do not delay (or lose) the trap interrupt handling. - */ - if (th->vm->main_thread == th && rb_signal_buff_size() > 0) { - RUBY_VM_SET_TRAP_INTERRUPT(ec); + ruby_current_execution_context_ptr = th->ec = ec; + old_fl = ATOMIC_EXCHANGE(prev_ec->interrupt_flag, 0); + + /* migrate interrupts from the previous EC to the current EC */ + new_fl = old_fl & (TIMER_INTERRUPT_MASK | + POSTPONED_JOB_INTERRUPT_MASK | + TRAP_INTERRUPT_MASK); + + if (new_fl) ATOMIC_OR(ec->interrupt_flag, new_fl); + + /* Pending-interrupts stay with the previous EC */ + old_fl &= PENDING_INTERRUPT_MASK; + if (old_fl) ATOMIC_OR(prev_ec->interrupt_flag, old_fl); + } + else { + ruby_current_execution_context_ptr = th->ec = ec; } VM_ASSERT(ec->fiber_ptr->cont.self == 0 || ec->vm_stack != NULL); diff --git a/vm_trace.c b/vm_trace.c index 54aa34f8d2..4e2ecf79ab 100644 --- a/vm_trace.c +++ b/vm_trace.c @@ -1596,7 +1596,7 @@ enum postponed_job_register_result { /* Async-signal-safe */ static enum postponed_job_register_result -postponed_job_register(rb_execution_context_t *ec, rb_vm_t *vm, +postponed_job_register(rb_vm_t *vm, unsigned int flags, rb_postponed_job_func_t func, void *data, int max, int expected_index) { rb_postponed_job_t *pjob; @@ -1614,7 +1614,7 @@ postponed_job_register(rb_execution_context_t *ec, rb_vm_t *vm, pjob->func = func; pjob->data = data; - RUBY_VM_SET_POSTPONED_JOB_INTERRUPT(ec); + RUBY_VM_SET_POSTPONED_JOB_INTERRUPT(vm->main_thread->ec); return PJRR_SUCCESS; } @@ -1626,11 +1626,10 @@ postponed_job_register(rb_execution_context_t *ec, rb_vm_t *vm, int rb_postponed_job_register(unsigned int flags, rb_postponed_job_func_t func, void *data) { - rb_execution_context_t *ec = GET_EC(); - rb_vm_t *vm = rb_ec_vm_ptr(ec); + rb_vm_t *vm = GET_VM(); begin: - switch (postponed_job_register(ec, vm, flags, func, data, MAX_POSTPONED_JOB, vm->postponed_job_index)) { + switch (postponed_job_register(vm, flags, func, data, MAX_POSTPONED_JOB, vm->postponed_job_index)) { case PJRR_SUCCESS : return 1; case PJRR_FULL : return 0; case PJRR_INTERRUPTED: goto begin; @@ -1645,8 +1644,7 @@ rb_postponed_job_register(unsigned int flags, rb_postponed_job_func_t func, void int rb_postponed_job_register_one(unsigned int flags, rb_postponed_job_func_t func, void *data) { - rb_execution_context_t *ec = GET_EC(); - rb_vm_t *vm = rb_ec_vm_ptr(ec); + rb_vm_t *vm = GET_VM(); rb_postponed_job_t *pjob; int i, index; @@ -1655,11 +1653,11 @@ rb_postponed_job_register_one(unsigned int flags, rb_postponed_job_func_t func, for (i=0; i<index; i++) { pjob = &vm->postponed_job_buffer[i]; if (pjob->func == func) { - RUBY_VM_SET_POSTPONED_JOB_INTERRUPT(ec); + RUBY_VM_SET_POSTPONED_JOB_INTERRUPT(vm->main_thread->ec); return 2; } } - switch (postponed_job_register(ec, vm, flags, func, data, MAX_POSTPONED_JOB + MAX_POSTPONED_JOB_SPECIAL_ADDITION, index)) { + switch (postponed_job_register(vm, flags, func, data, MAX_POSTPONED_JOB + MAX_POSTPONED_JOB_SPECIAL_ADDITION, index)) { case PJRR_SUCCESS : return 1; case PJRR_FULL : return 0; case PJRR_INTERRUPTED: goto begin; -- EW Unsubscribe: <mailto:ruby-core-request / ruby-lang.org?subject=unsubscribe> <http://lists.ruby-lang.org/cgi-bin/mailman/options/ruby-core>