From: Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: tarantool-patches@dev.tarantool.org, gorcunov@gmail.com, sergepetrenko@tarantool.org Subject: [Tarantool-patches] [PATCH 1/2] fiber: introduce fiber_touch and fiber_continue Date: Fri, 23 Apr 2021 01:05:54 +0200 [thread overview] Message-ID: <5c13b5eef8c72ff5ff548bdd3d5b0023ee44334c.1619132520.git.v.shpilevoy@tarantool.org> (raw) In-Reply-To: <cover.1619132519.git.v.shpilevoy@tarantool.org> These new functions are supposed to be used instead of fiber_wakeup whose behaviour sometimes leads to unexpected and hard to catch bugs and to crashes when called on the self fiber, usually accidentally. fiber_touch() allows to wakeup any fiber safely, even self. Because on self it turns into NOP, but it is not any slower than fiber_wakeup(), because uses the same code, but with a bit different flag set. fiber_continue() is like fiber_wakeup() but it has an assertion that it is not called on self allowing to catch self-wakeup bugs early and easy. fiber_wakeup() still exists because it is a part of the public API. In order to make fiber_touch() as cheap as fiber_wakeup() there is a new flag FIBER_IS_RUNNING. It is set for the fiber stored in the cord - the currently running one. The only difference between the wakeup and touch is wakeup skips READY | DEAD fibers, while the touch skips READY | DEAD | RUNNING. Both cost one bitwise OR operation. Part of #5292 --- src/lib/core/fiber.c | 107 ++++++++++++++++++++++++----------------- src/lib/core/fiber.h | 26 ++++++++++ test/unit/fiber.cc | 64 ++++++++++++++++++++++++ test/unit/fiber.result | 4 ++ 4 files changed, 158 insertions(+), 43 deletions(-) diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c index f8b85d99d..96de83042 100644 --- a/src/lib/core/fiber.c +++ b/src/lib/core/fiber.c @@ -396,10 +396,13 @@ fiber_call_impl(struct fiber *callee) assert(rlist_empty(&callee->state)); assert(caller); assert(caller != callee); + assert((caller->flags & FIBER_IS_RUNNING) != 0); + assert((callee->flags & FIBER_IS_RUNNING) == 0); + caller->flags &= ~FIBER_IS_RUNNING; cord->fiber = callee; + callee->flags = (callee->flags & ~FIBER_IS_READY) | FIBER_IS_RUNNING; - callee->flags &= ~FIBER_IS_READY; ASAN_START_SWITCH_FIBER(asan_state, 1, callee->stack, callee->stack_size); @@ -452,33 +455,10 @@ fiber_checkstack(void) * call, it simply reschedules the fiber after other ready * fibers in the same event loop iteration. */ -void -fiber_wakeup(struct fiber *f) +static void +fiber_make_ready(struct fiber *f) { - assert(! (f->flags & FIBER_IS_DEAD)); - /** - * Do nothing if the fiber is already in cord->ready - * list *or* is in the call chain created by - * fiber_schedule_list(). While it's harmless to re-add - * a fiber to cord->ready, even if it's already there, - * but the same game is deadly when the fiber is in - * the callee list created by fiber_schedule_list(). - * - * To put it another way, fiber_wakeup() is a 'request' to - * schedule the fiber for execution, and once it is executing - * a wakeup request is considered complete and it must be - * removed. - * - * A dead fiber can be lingering in the cord fiber list - * if it is joinable. This makes it technically possible - * to schedule it. We would never make such a mistake - * in our own code, hence the assert above. But as long - * as fiber.wakeup() is a part of public Lua API, an - * external rock can mess things up. Ignore such attempts - * as well. - */ - if (f->flags & (FIBER_IS_READY | FIBER_IS_DEAD)) - return; + assert((f->flags & (FIBER_IS_DEAD | FIBER_IS_READY)) == 0); struct cord *cord = cord(); if (rlist_empty(&cord->ready)) { /* @@ -503,6 +483,45 @@ fiber_wakeup(struct fiber *f) f->flags |= FIBER_IS_READY; } +void +fiber_wakeup(struct fiber *f) +{ + assert((f->flags & FIBER_IS_DEAD) == 0); + /* + * Do nothing if the fiber is already in cord->ready list *or* is in the + * call chain created by fiber_schedule_list(). While it's harmless to + * re-add a fiber to cord->ready, even if it's already there, but the + * same game is deadly when the fiber is in the callee list created by + * fiber_schedule_list(). + * + * To put it another way, fiber_wakeup() is a 'request' to schedule the + * fiber for execution, and once it is executing a wakeup request is + * considered complete and it must be removed. + * + * A dead fiber can be lingering in the cord fiber list if it is + * joinable. This makes it technically possible to schedule it. We would + * never make such a mistake in our own code, hence the assert above. + * But as long as fiber.wakeup() is a part of public Lua API, an + * external rock can mess things up. Ignore such attempts + * as well. + */ + if ((f->flags & (FIBER_IS_READY | FIBER_IS_DEAD)) == 0) + fiber_make_ready(f); +} + +void +fiber_touch(struct fiber *f) +{ + assert((f->flags & FIBER_IS_DEAD) == 0); + /* + * The reason why DEAD is checked and asserted at the same time see in + * fiber_wakeup(). + */ + if ((f->flags & (FIBER_IS_READY | FIBER_IS_DEAD | + FIBER_IS_RUNNING)) == 0) + fiber_make_ready(f); +} + /** Cancel the subject fiber. * * Note: cancelation is asynchronous. Use fiber_join() to wait for the @@ -521,7 +540,6 @@ void fiber_cancel(struct fiber *f) { assert(f->fid != 0); - struct fiber *self = fiber(); /** * Do nothing if the fiber is dead, since cancelling * the fiber would clear the diagnostics area and @@ -535,10 +553,8 @@ fiber_cancel(struct fiber *f) /** * Don't wake self and zombies. */ - if (f != self) { - if (f->flags & FIBER_IS_CANCELLABLE) - fiber_wakeup(f); - } + if ((f->flags & FIBER_IS_CANCELLABLE) != 0) + fiber_touch(f); } /** @@ -686,8 +702,13 @@ fiber_yield(void) assert(callee->flags & FIBER_IS_READY || callee == &cord->sched); assert(! (callee->flags & FIBER_IS_DEAD)); + assert((caller->flags & FIBER_IS_RUNNING) != 0); + assert((callee->flags & FIBER_IS_RUNNING) == 0); + + caller->flags &= ~FIBER_IS_RUNNING; cord->fiber = callee; - callee->flags &= ~FIBER_IS_READY; + callee->flags = (callee->flags & ~FIBER_IS_READY) | FIBER_IS_RUNNING; + ASAN_START_SWITCH_FIBER(asan_state, (caller->flags & FIBER_IS_DEAD) == 0, callee->stack, @@ -712,7 +733,7 @@ fiber_schedule_timeout(ev_loop *loop, struct fiber_watcher_data *state = (struct fiber_watcher_data *) watcher->data; state->timed_out = true; - fiber_wakeup(state->f); + fiber_continue(state->f); } /** @@ -748,10 +769,6 @@ fiber_sleep(double delay) if (delay == 0) { ev_idle_start(loop(), &cord()->idle_event); } - /* - * We don't use fiber_wakeup() here to ensure there is - * no infinite wakeup loop in case of fiber_sleep(0). - */ fiber_yield_timeout(delay); if (delay == 0) { @@ -766,7 +783,7 @@ fiber_schedule_cb(ev_loop *loop, ev_watcher *watcher, int revents) (void) revents; struct fiber *fiber = watcher->data; assert(fiber() == &cord()->sched); - fiber_wakeup(fiber); + fiber_continue(fiber); } static inline void @@ -864,7 +881,11 @@ fiber_reset(struct fiber *fiber) { rlist_create(&fiber->on_yield); rlist_create(&fiber->on_stop); - fiber->flags = FIBER_DEFAULT_FLAGS; + /* + * Preserve the running flag if set. Reset might be called on the + * current fiber when it is recycled. + */ + fiber->flags = FIBER_DEFAULT_FLAGS | (fiber->flags & FIBER_IS_RUNNING); #if ENABLE_FIBER_TOP clock_stat_reset(&fiber->clock_stat); #endif /* ENABLE_FIBER_TOP */ @@ -929,8 +950,7 @@ fiber_loop(MAYBE_UNUSED void *data) struct fiber *f; f = rlist_shift_entry(&fiber->wake, struct fiber, state); - assert(f != fiber); - fiber_wakeup(f); + fiber_continue(f); } fiber_on_stop(fiber); /* reset pending wakeups */ @@ -1449,6 +1469,7 @@ cord_create(struct cord *cord, const char *name) cord->sched.name = NULL; fiber_set_name(&cord->sched, "sched"); cord->fiber = &cord->sched; + cord->sched.flags |= FIBER_IS_RUNNING; cord->max_fid = FIBER_ID_MAX_RESERVED; /* @@ -1632,7 +1653,7 @@ cord_cojoin_wakeup(struct ev_loop *loop, struct ev_async *ev, int revents) struct cord_cojoin_ctx *ctx = (struct cord_cojoin_ctx *)ev->data; ctx->task_complete = true; - fiber_wakeup(ctx->fiber); + fiber_continue(ctx->fiber); } int diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h index b9eeef697..2506a178c 100644 --- a/src/lib/core/fiber.h +++ b/src/lib/core/fiber.h @@ -162,6 +162,10 @@ enum { * This flag is set when fiber uses custom stack size. */ FIBER_CUSTOM_STACK = 1 << 5, + /** + * The flag is set for the fiber currently being executed by the cord. + */ + FIBER_IS_RUNNING = 1 << 6, FIBER_DEFAULT_FLAGS = FIBER_IS_CANCELLABLE }; @@ -820,6 +824,28 @@ fiber_destroy_all(struct cord *cord); void fiber_gc(void); +/** + * A polite version of wakeup. The same as fiber_wakeup() but is nop for the + * currently running fiber. Allows to save one 'if' compared to wakeup by not + * forcing to check if the fiber is the current one. + */ +void +fiber_touch(struct fiber *f); + +/** + * The same as fiber_wakeup() but bans continuation of the current fiber as it + * might lead to very unexpected behaviour. For instance, some code called + * wakeup on self accidentally, and much later in some other subsystem is called + * fiber_yield() to wait for something - it would return right away without + * waiting causing troubles of all kinds. + */ +static inline void +fiber_continue(struct fiber *f) +{ + assert(f != fiber()); + fiber_wakeup(f); +} + void fiber_call(struct fiber *callee); diff --git a/test/unit/fiber.cc b/test/unit/fiber.cc index 9c1a23bdd..65c083230 100644 --- a/test/unit/fiber.cc +++ b/test/unit/fiber.cc @@ -201,12 +201,76 @@ fiber_name_test() footer(); } +static int +yield_f(va_list ap) +{ + bool *is_started = va_arg(ap, bool *); + *is_started = true; + fiber_yield(); + return 0; +} + +static void +fiber_touch_test() +{ + header(); + + struct fiber *f = fiber(); + + fiber_touch(f); + double duration = 0.001; + uint64_t t1 = fiber_clock64(); + fiber_sleep(duration); + uint64_t t2 = fiber_clock64(); + /* + * It was a real sleep, not immediate wakeup. Touch is nop on a running + * fiber. + */ + assert(t2 - t1 >= duration); + + bool is_started = false; + f = fiber_new_xc("yield", yield_f); + fiber_set_joinable(f, true); + fiber_start(f, &is_started); + /* Let the infinitely yielding fiber to start its yield. */ + fiber_sleep(0); + assert(is_started); + fiber_touch(f); + int rc = fiber_join(f); + assert(rc == 0); + + footer(); +} + +static void +fiber_continue_test() +{ + header(); + + struct fiber *f = fiber(); + + bool is_started = false; + f = fiber_new_xc("yield", yield_f); + fiber_set_joinable(f, true); + fiber_start(f, &is_started); + /* Let the infinitely yielding fiber to start its yield. */ + fiber_sleep(0); + assert(is_started); + fiber_continue(f); + int rc = fiber_join(f); + assert(rc == 0); + + footer(); +} + static int main_f(va_list ap) { fiber_name_test(); fiber_join_test(); fiber_stack_test(); + fiber_touch_test(); + fiber_continue_test(); ev_break(loop(), EVBREAK_ALL); return 0; } diff --git a/test/unit/fiber.result b/test/unit/fiber.result index a61e0a2b8..23f331e9b 100644 --- a/test/unit/fiber.result +++ b/test/unit/fiber.result @@ -17,3 +17,7 @@ SystemError Failed to allocate 42 bytes in allocator for exception: Cannot alloc # normal-stack fiber not crashed # big-stack fiber not crashed *** fiber_stack_test: done *** + *** fiber_touch_test *** + *** fiber_touch_test: done *** + *** fiber_continue_test *** + *** fiber_continue_test: done *** -- 2.24.3 (Apple Git-128)
next prev parent reply other threads:[~2021-04-22 23:06 UTC|newest] Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-04-22 23:05 [Tarantool-patches] [PATCH 0/2] fiber_touch() and fiber_continue() Vladislav Shpilevoy via Tarantool-patches 2021-04-22 23:05 ` Vladislav Shpilevoy via Tarantool-patches [this message] 2021-04-22 23:05 ` [Tarantool-patches] [PATCH 2/2] fiber: use fiber_touch and fiber_continue Vladislav Shpilevoy via Tarantool-patches 2021-04-23 23:16 ` [Tarantool-patches] [PATCH 0/2] fiber_touch() and fiber_continue() Vladislav Shpilevoy via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=5c13b5eef8c72ff5ff548bdd3d5b0023ee44334c.1619132520.git.v.shpilevoy@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=gorcunov@gmail.com \ --cc=sergepetrenko@tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH 1/2] fiber: introduce fiber_touch and fiber_continue' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox