From: Koichi Sasada Date: 2018-10-28T08:52:48+09:00 Subject: [ruby-core:89584] Re: [PATCH] vm_trace.c (postponed_job_register): only hit main thread thank you for you patch. but it is not acceptable because "set only main thread" is highly restricted. stackprof doesn't work well. I'm thinking that making interrupt_flag global. I think it is okay to remain this issue over ruby 2.6. On 2018/10/28 8:38, Eric Wong wrote: > @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 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; > -- // SASADA Koichi at atdot dot net Unsubscribe: