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=-4.0 required=3.0 tests=ALL_TRUSTED,BAYES_00 shortcircuit=no autolearn=ham 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 5E7211F404; Sun, 9 Sep 2018 10:03:19 +0000 (UTC) Date: Sun, 9 Sep 2018 10:03:19 +0000 From: Eric Wong To: spew@80x24.org Subject: [DIFF r64663] fix remaining Fiber + fork bugs Message-ID: <20180909100319.ild2rlncrmmm2lwp@whir> References: <20180909095347.30757-1-e@80x24.org> <20180909095536.vwsi3tu6ip5dvxyd@whir> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20180909095536.vwsi3tu6ip5dvxyd@whir> List-Id: > The following changes since commit 64b326c2204e562aa3d6025ca097a82bcf4de303: > > spec/ruby/library/socket/addrinfo: require for SocketSpecs (2018-09-09 08:50:53 +0000) > > are available in the Git repository at: > > https://80x24.org/ruby.git fiber-fork > > for you to fetch changes up to ce0a6005fbf78203b3bcaa4b90bf94b8b5c44782: > > fiber: fix crash on GC after forking (2018-09-09 09:49:55 +0000) $ git diff 64b326c2204e562aa3d6025ca097a82bcf4de303 ce0a6005fbf78203b3bcaa4b90bf94b8b5c44782 diff --git a/cont.c b/cont.c index 7bc1724176..95f2de9b87 100644 --- a/cont.c +++ b/cont.c @@ -77,8 +77,7 @@ static long pagesize; enum context_type { CONTINUATION_CONTEXT = 0, - FIBER_CONTEXT = 1, - ROOT_FIBER_CONTEXT = 2 + FIBER_CONTEXT = 1 }; struct cont_saved_vm_stack { @@ -253,8 +252,8 @@ fiber_status_set(rb_fiber_t *fib, enum fiber_status s) void ec_set_vm_stack(rb_execution_context_t *ec, VALUE *stack, size_t size) { - *(VALUE **)(&ec->vm_stack) = stack; - *(size_t *)(&ec->vm_stack_size) = size; + ec->vm_stack = stack; + ec->vm_stack_size = size; } static inline void @@ -360,6 +359,12 @@ cont_mark(void *ptr) RUBY_MARK_LEAVE("cont"); } +static int +fiber_is_root_p(const rb_fiber_t *fib) +{ + return fib == fib->cont.saved_ec.thread_ptr->root_fiber; +} + static void cont_free(void *ptr) { @@ -378,24 +383,17 @@ cont_free(void *ptr) /* fiber */ const rb_fiber_t *fib = (rb_fiber_t*)cont; #ifdef _WIN32 - if (cont->type != ROOT_FIBER_CONTEXT) { + if (!fiber_is_root_p(fib)) { /* don't delete root fiber handle */ if (fib->fib_handle) { DeleteFiber(fib->fib_handle); } } #else /* not WIN32 */ + /* fib->ss_sp == NULL is possible for root fiber */ if (fib->ss_sp != NULL) { - if (cont->type == ROOT_FIBER_CONTEXT) { - rb_bug("Illegal root fiber parameter"); - } munmap((void*)fib->ss_sp, fib->ss_size); } - else { - /* It may reached here when finalize */ - /* TODO examine whether it is a bug */ - /* rb_bug("cont_free: release self"); */ - } #endif } #else /* not FIBER_USE_NATIVE */ @@ -493,12 +491,15 @@ static size_t fiber_memsize(const void *ptr) { const rb_fiber_t *fib = ptr; - size_t size = 0; + size_t size = sizeof(*fib); + const rb_execution_context_t *saved_ec = &fib->cont.saved_ec; + const rb_thread_t *th = rb_ec_thread_ptr(saved_ec); - size = sizeof(*fib); - if (fib->cont.type != ROOT_FIBER_CONTEXT && - fib->cont.saved_ec.local_storage != NULL) { - size += st_memsize(fib->cont.saved_ec.local_storage); + /* + * vm.c::thread_memsize already counts th->ec->local_storage + */ + if (saved_ec->local_storage && fib != th->root_fiber) { + size += st_memsize(saved_ec->local_storage); } size += cont_memsize(&fib->cont); return size; @@ -1411,13 +1412,20 @@ fiber_init(VALUE fibval, VALUE proc) rb_context_t *cont = &fib->cont; rb_execution_context_t *sec = &cont->saved_ec; rb_thread_t *cth = GET_THREAD(); - size_t fib_stack_size = cth->vm->default_params.fiber_vm_stack_size / sizeof(VALUE); + rb_vm_t *vm = cth->vm; + size_t fib_stack_bytes = vm->default_params.fiber_vm_stack_size; + size_t thr_stack_bytes = vm->default_params.thread_vm_stack_size; + VALUE *vm_stack; /* initialize cont */ cont->saved_vm_stack.ptr = NULL; - ec_set_vm_stack(sec, NULL, 0); - - ec_set_vm_stack(sec, ALLOC_N(VALUE, fib_stack_size), fib_stack_size); + if (fib_stack_bytes == thr_stack_bytes) { + vm_stack = rb_thread_recycle_stack(fib_stack_bytes / sizeof(VALUE)); + } + else { + vm_stack = ruby_xmalloc(fib_stack_bytes); + } + ec_set_vm_stack(sec, vm_stack, fib_stack_bytes / sizeof(VALUE)); sec->cfp = (void *)(sec->vm_stack + sec->vm_stack_size); rb_vm_push_frame(sec, @@ -1516,7 +1524,7 @@ root_fiber_alloc(rb_thread_t *th) rb_fiber_t *fib = th->ec->fiber_ptr; VM_ASSERT(DATA_PTR(fibval) == NULL); - VM_ASSERT(fib->cont.type == ROOT_FIBER_CONTEXT); + VM_ASSERT(fib->cont.type == FIBER_CONTEXT); VM_ASSERT(fib->status == FIBER_RESUMED); th->root_fiber = fib; @@ -1545,7 +1553,7 @@ rb_threadptr_root_fiber_setup(rb_thread_t *th) { rb_fiber_t *fib = ruby_mimmalloc(sizeof(rb_fiber_t)); MEMZERO(fib, rb_fiber_t, 1); - fib->cont.type = ROOT_FIBER_CONTEXT; + fib->cont.type = FIBER_CONTEXT; fib->cont.saved_ec.fiber_ptr = fib; fib->cont.saved_ec.thread_ptr = th; fiber_status_set(fib, FIBER_RESUMED); /* skip CREATED */ @@ -1561,7 +1569,7 @@ rb_threadptr_root_fiber_release(rb_thread_t *th) /* ignore. A root fiber object will free th->ec */ } else { - VM_ASSERT(th->ec->fiber_ptr->cont.type == ROOT_FIBER_CONTEXT); + VM_ASSERT(th->ec->fiber_ptr->cont.type == FIBER_CONTEXT); VM_ASSERT(th->ec->fiber_ptr->cont.self == 0); fiber_free(th->ec->fiber_ptr); @@ -1758,19 +1766,22 @@ rb_fiber_transfer(VALUE fibval, int argc, const VALUE *argv) void rb_fiber_close(rb_fiber_t *fib) { - VALUE *vm_stack = fib->cont.saved_ec.vm_stack; + rb_execution_context_t *ec = &fib->cont.saved_ec; + VALUE *vm_stack = ec->vm_stack; + size_t stack_bytes = ec->vm_stack_size * sizeof(VALUE); + fiber_status_set(fib, FIBER_TERMINATED); - if (fib->cont.type == ROOT_FIBER_CONTEXT) { - rb_thread_recycle_stack_release(vm_stack); + if (stack_bytes == rb_ec_vm_ptr(ec)->default_params.thread_vm_stack_size) { + rb_thread_recycle_stack_release(vm_stack); } else { - ruby_xfree(vm_stack); + ruby_xfree(vm_stack); } - ec_set_vm_stack(&fib->cont.saved_ec, NULL, 0); + ec_set_vm_stack(ec, NULL, 0); #if !FIBER_USE_NATIVE /* should not mark machine stack any more */ - fib->cont.saved_ec.machine.stack_end = NULL; + ec->machine.stack_end = NULL; #endif } @@ -1805,7 +1816,7 @@ rb_fiber_resume(VALUE fibval, int argc, const VALUE *argv) { rb_fiber_t *fib = fiber_ptr(fibval); - if (fib->prev != 0 || fib->cont.type == ROOT_FIBER_CONTEXT) { + if (fib->prev != 0 || fiber_is_root_p(fib)) { rb_raise(rb_eFiberError, "double resume"); } if (fib->transferred != 0) { @@ -1984,7 +1995,6 @@ rb_fiber_atfork(rb_thread_t *th) if (th->root_fiber) { if (&th->root_fiber->cont.saved_ec != th->ec) { th->root_fiber = th->ec->fiber_ptr; - th->root_fiber->cont.type = ROOT_FIBER_CONTEXT; } th->root_fiber->prev = 0; } diff --git a/internal.h b/internal.h index 8cbe16aaff..6a44ec3b0e 100644 --- a/internal.h +++ b/internal.h @@ -1912,6 +1912,7 @@ void Init_BareVM(void); void Init_vm_objects(void); PUREFUNC(VALUE rb_vm_top_self(void)); void rb_thread_recycle_stack_release(VALUE *); +VALUE *rb_thread_recycle_stack(size_t); void rb_vm_change_state(void); void rb_vm_inc_const_missing_count(void); const void **rb_vm_get_insns_address_table(void); diff --git a/test/ruby/test_fiber.rb b/test/ruby/test_fiber.rb index c835246fda..1729147d5e 100644 --- a/test/ruby/test_fiber.rb +++ b/test/ruby/test_fiber.rb @@ -260,18 +260,25 @@ def test_prohibit_resume_transferred_fiber end def test_fork_from_fiber - begin - pid = Process.fork{} - rescue NotImplementedError - return - else - Process.wait(pid) - end + skip 'fork not supported' unless Process.respond_to?(:fork) + pid = nil bug5700 = '[ruby-core:41456]' assert_nothing_raised(bug5700) do Fiber.new do pid = fork do - Fiber.new {}.transfer + xpid = nil + Fiber.new { + xpid = fork do + # enough to trigger GC on old root fiber + 10000.times do + Fiber.new {}.transfer + Fiber.new { Fiber.yield } + end + exit!(0) + end + }.transfer + _, status = Process.waitpid2(xpid) + exit!(status.success?) end end.resume end diff --git a/vm.c b/vm.c index 5c33cfb300..4c16be8e6d 100644 --- a/vm.c +++ b/vm.c @@ -2341,8 +2341,8 @@ vm_init2(rb_vm_t *vm) static VALUE *thread_recycle_stack_slot[RECYCLE_MAX]; static int thread_recycle_stack_count = 0; -static VALUE * -thread_recycle_stack(size_t size) +VALUE * +rb_thread_recycle_stack(size_t size) { if (thread_recycle_stack_count > 0) { /* TODO: check stack size if stack sizes are variable */ @@ -2354,7 +2354,7 @@ thread_recycle_stack(size_t size) } #else -#define thread_recycle_stack(size) ALLOC_N(VALUE, (size)) +#define rb_thread_recycle_stack(size) ALLOC_N(VALUE, (size)) #endif void @@ -2536,7 +2536,7 @@ th_init(rb_thread_t *th, VALUE self) /* vm_stack_size is word number. * th->vm->default_params.thread_vm_stack_size is byte size. */ size_t size = th->vm->default_params.thread_vm_stack_size / sizeof(VALUE); - ec_set_vm_stack(th->ec, thread_recycle_stack(size), size); + ec_set_vm_stack(th->ec, rb_thread_recycle_stack(size), size); } th->ec->cfp = (void *)(th->ec->vm_stack + th->ec->vm_stack_size);