From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp57.i.mail.ru (smtp57.i.mail.ru [217.69.128.37]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id C82DF445321 for ; Wed, 29 Jul 2020 19:50:51 +0300 (MSK) From: Ilya Kosarev Date: Wed, 29 Jul 2020 19:50:41 +0300 Message-Id: <482137955bbc7004f23e5c3ffb7d65b3ab0e223f.1596040943.git.i.kosarev@tarantool.org> In-Reply-To: References: In-Reply-To: References: Subject: [Tarantool-patches] [PATCH 1/2] fiber: set diagnostics at madvise/mprotect failure List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: gorcunov@gmail.com Cc: tarantool-patches@dev.tarantool.org Both madvise and mprotect calls can fail due to various reasons, mostly because of lack of free memory in the system. We log such cases via say_x helpers but this is not enough. In particular tarantool/memcached relies on diag error to be set to detect an error condition: | expire_fiber = fiber_new(name, memcached_expire_loop); | const box_error_t *err = box_error_last(); | if (err) { | say_error("Can't start the expire fiber"); | say_error("%s", box_error_message(err)); | return -1; | } Thus lets use diag_set() helper here and instead of macros use inline functions for better readability. Closes #5211 Co-authored-by: Cyrill Gorcunov (backported from commit c67522973b32fae955835109d4f86eada8f67ae5) --- src/errinj.h | 32 +++--- src/fiber.c | 183 ++++++++++++++++++++++------------- test/box/errinj.result | 2 + test/unit/CMakeLists.txt | 4 + test/unit/fiber_stack.c | 79 +++++++++++++++ test/unit/fiber_stack.result | 8 ++ test/unit/suite.ini | 1 + 7 files changed, 227 insertions(+), 82 deletions(-) create mode 100644 test/unit/fiber_stack.c create mode 100644 test/unit/fiber_stack.result diff --git a/src/errinj.h b/src/errinj.h index 14cea11e9..ff6c8cf3f 100644 --- a/src/errinj.h +++ b/src/errinj.h @@ -41,27 +41,27 @@ extern "C" { * Injection type */ enum errinj_type { - /** boolean */ - ERRINJ_BOOL = 0, - /** uint64_t */ - ERRINJ_INT = 1, - /** double */ - ERRINJ_DOUBLE = 2 + /** boolean */ + ERRINJ_BOOL = 0, + /** uint64_t */ + ERRINJ_INT = 1, + /** double */ + ERRINJ_DOUBLE = 2 }; /** * Injection state */ struct errinj { - /** Name, e.g "ERRINJ_WAL_WRITE" */ - const char *name; - /** Type, e.g. BOOL, U64, DOUBLE */ - enum errinj_type type; - union { - /** bool parameter */ - bool bparam; - /** integer parameter */ - int64_t iparam; + /** Name, e.g "ERRINJ_WAL_WRITE" */ + const char *name; + /** Type, e.g. BOOL, U64, DOUBLE */ + enum errinj_type type; + union { + /** bool parameter */ + bool bparam; + /** integer parameter */ + int64_t iparam; /** double parameter */ double dparam; }; @@ -128,6 +128,8 @@ struct errinj { _(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT, {.iparam = 0}) \ _(ERRINJ_INDEX_RESERVE, ERRINJ_BOOL, {.bparam = false})\ _(ERRINJ_VY_STMT_ALLOC, ERRINJ_INT, {.iparam = -1})\ + _(ERRINJ_FIBER_MADVISE, ERRINJ_BOOL, {.bparam = false})\ + _(ERRINJ_FIBER_MPROTECT, ERRINJ_INT, {.iparam = -1})\ _(ERRINJ_VY_READ_VIEW_MERGE_FAIL, ERRINJ_BOOL, {.bparam = false})\ _(ERRINJ_VY_WRITE_ITERATOR_START_FAIL, ERRINJ_BOOL, {.bparam = false})\ _(ERRINJ_VY_RUN_OPEN, ERRINJ_INT, {.iparam = -1})\ diff --git a/src/fiber.c b/src/fiber.c index d7ea9924a..2d83bbc1a 100644 --- a/src/fiber.c +++ b/src/fiber.c @@ -41,6 +41,7 @@ #include "assoc.h" #include "memory.h" #include "trigger.h" +#include "errinj.h" #include "third_party/valgrind/memcheck.h" @@ -66,30 +67,48 @@ static int (*fiber_invoke)(fiber_func f, va_list ap); #define ASAN_FINISH_SWITCH_FIBER(var_name) #endif -#define fiber_madvise(addr, len, advice) \ -({ \ - int err = madvise((addr), (len), (advice)); \ - if (err) \ - say_syserror("madvise"); \ - err; \ -}) - -#define fiber_mprotect(addr, len, prot) \ -({ \ - int err = mprotect((addr), (len), (prot)); \ - if (err) \ - say_syserror("mprotect"); \ - err; \ -}) +static inline int +fiber_madvise(void *addr, size_t len, int advice) +{ + int rc = 0; + + ERROR_INJECT(ERRINJ_FIBER_MADVISE, { + errno = ENOMEM; + rc = -1; + }); + + if (rc != 0 || madvise(addr, len, advice) != 0) { + diag_set(SystemError, "fiber madvise failed"); + return -1; + } + return 0; +} + +static inline int +fiber_mprotect(void *addr, size_t len, int prot) +{ + int rc = 0; + + struct errinj *inj = errinj(ERRINJ_FIBER_MPROTECT, ERRINJ_INT); + if (inj != NULL && inj->iparam == prot) { + errno = ENOMEM; + rc = -1; + } + if (rc != 0 || mprotect(addr, len, prot) != 0) { + diag_set(SystemError, "fiber mprotect failed"); + return -1; + } + return 0; +} /* * Defines a handler to be executed on exit from cord's thread func, * accessible via cord()->on_exit (normally NULL). It is used to * implement cord_cojoin. */ struct cord_on_exit { - void (*callback)(void*); - void *argument; + void (*callback)(void*); + void *argument; }; /* @@ -108,18 +127,18 @@ static size_t page_size; static int stack_direction; enum { - /* The minimum allowable fiber stack size in bytes */ - FIBER_STACK_SIZE_MINIMAL = 16384, - /* Default fiber stack size in bytes */ - FIBER_STACK_SIZE_DEFAULT = 524288, - /* Stack size watermark in bytes. */ - FIBER_STACK_SIZE_WATERMARK = 65536, + /* The minimum allowable fiber stack size in bytes */ + FIBER_STACK_SIZE_MINIMAL = 16384, + /* Default fiber stack size in bytes */ + FIBER_STACK_SIZE_DEFAULT = 524288, + /* Stack size watermark in bytes. */ + FIBER_STACK_SIZE_WATERMARK = 65536, }; /** Default fiber attributes */ static const struct fiber_attr fiber_attr_default = { - .stack_size = FIBER_STACK_SIZE_DEFAULT, - .flags = FIBER_DEFAULT_FLAGS + .stack_size = FIBER_STACK_SIZE_DEFAULT, + .flags = FIBER_DEFAULT_FLAGS }; #ifdef HAVE_MADV_DONTNEED @@ -147,7 +166,7 @@ static const uint64_t poison_pool[] = { void fiber_attr_create(struct fiber_attr *fiber_attr) { - *fiber_attr = fiber_attr_default; + *fiber_attr = fiber_attr_default; } struct fiber_attr * @@ -156,7 +175,7 @@ fiber_attr_new() struct fiber_attr *fiber_attr = malloc(sizeof(*fiber_attr)); if (fiber_attr == NULL) { diag_set(OutOfMemory, sizeof(*fiber_attr), - "runtime", "fiber attr"); + "runtime", "fiber attr"); return NULL; } fiber_attr_create(fiber_attr); @@ -190,7 +209,7 @@ size_t fiber_attr_getstacksize(struct fiber_attr *fiber_attr) { return fiber_attr != NULL ? fiber_attr->stack_size : - fiber_attr_default.stack_size; + fiber_attr_default.stack_size; } void @@ -514,14 +533,14 @@ struct fiber_watcher_data { static void fiber_schedule_timeout(ev_loop *loop, - ev_timer *watcher, int revents) + ev_timer *watcher, int revents) { (void) loop; (void) revents; assert(fiber() == &cord()->sched); struct fiber_watcher_data *state = - (struct fiber_watcher_data *) watcher->data; + (struct fiber_watcher_data *) watcher->data; state->timed_out = true; fiber_wakeup(state->f); } @@ -733,12 +752,12 @@ fiber_loop(MAYBE_UNUSED void *data) } fiber->flags |= FIBER_IS_DEAD; while (! rlist_empty(&fiber->wake)) { - struct fiber *f; - f = rlist_shift_entry(&fiber->wake, struct fiber, - state); - assert(f != fiber); - fiber_wakeup(f); - } + struct fiber *f; + f = rlist_shift_entry(&fiber->wake, struct fiber, + state); + assert(f != fiber); + fiber_wakeup(f); + } fiber_on_stop(fiber); /* reset pending wakeups */ rlist_del(&fiber->state); @@ -833,6 +852,12 @@ fiber_stack_recycle(struct fiber *fiber) start = page_align_up(fiber->stack_watermark); end = fiber->stack + fiber->stack_size; } + + /* + * Ignore errors on MADV_DONTNEED because this is + * just a hint for OS and not critical for + * functionality. + */ fiber_madvise(start, end - start, MADV_DONTNEED); stack_put_watermark(fiber->stack_watermark); } @@ -851,7 +876,9 @@ fiber_stack_watermark_create(struct fiber *fiber) /* * We don't expect the whole stack usage in regular - * loads, let's try to minimize rss pressure. + * loads, let's try to minimize rss pressure. But do + * not exit if MADV_DONTNEED failed, it is just a hint + * for OS, not critical one. */ fiber_madvise(fiber->stack, fiber->stack_size, MADV_DONTNEED); @@ -889,6 +916,8 @@ fiber_stack_watermark_create(struct fiber *fiber) static void fiber_stack_destroy(struct fiber *fiber, struct slab_cache *slabc) { + static const int mprotect_flags = PROT_READ | PROT_WRITE; + if (fiber->stack != NULL) { VALGRIND_STACK_DEREGISTER(fiber->stack_id); #if ENABLE_ASAN @@ -899,21 +928,36 @@ fiber_stack_destroy(struct fiber *fiber, struct slab_cache *slabc) guard = page_align_down(fiber->stack - page_size); else guard = page_align_up(fiber->stack + fiber->stack_size); - fiber_mprotect(guard, page_size, PROT_READ | PROT_WRITE); + + if (fiber_mprotect(guard, page_size, mprotect_flags) != 0) { + /* + * FIXME: We need some intelligent handling: + * say put this slab into a queue and retry + * to setup the original protection back in + * background. + * + * Note that in case if we're called from + * fiber_stack_create() the @a mprotect_flags is + * the same as the slab been created with, so + * calling mprotect for VMA with same flags + * won't fail. + */ + diag_log(); + } slab_put(slabc, fiber->stack_slab); } } static int fiber_stack_create(struct fiber *fiber, struct slab_cache *slabc, - size_t stack_size) + size_t stack_size) { stack_size -= slab_sizeof(); fiber->stack_slab = slab_get(slabc, stack_size); if (fiber->stack_slab == NULL) { diag_set(OutOfMemory, stack_size, - "runtime arena", "fiber stack"); + "runtime arena", "fiber stack"); return -1; } void *guard; @@ -928,7 +972,7 @@ fiber_stack_create(struct fiber *fiber, struct slab_cache *slabc, guard = page_align_up(slab_data(fiber->stack_slab)); fiber->stack = guard + page_size; fiber->stack_size = slab_data(fiber->stack_slab) + stack_size - - fiber->stack; + fiber->stack; } else { /* * A stack grows up. Last page should be protected and @@ -936,16 +980,21 @@ fiber_stack_create(struct fiber *fiber, struct slab_cache *slabc, * be used for coro stack usage */ guard = page_align_down(fiber->stack_slab + stack_size) - - page_size; + page_size; fiber->stack = fiber->stack_slab + slab_sizeof(); fiber->stack_size = guard - fiber->stack; } fiber->stack_id = VALGRIND_STACK_REGISTER(fiber->stack, - (char *)fiber->stack + - fiber->stack_size); + (char *)fiber->stack + + fiber->stack_size); if (fiber_mprotect(guard, page_size, PROT_NONE)) { + /* + * Write an error into the log since a guard + * page is critical for functionality. + */ + diag_log(); fiber_stack_destroy(fiber, slabc); return -1; } @@ -956,7 +1005,7 @@ fiber_stack_create(struct fiber *fiber, struct slab_cache *slabc, struct fiber * fiber_new_ex(const char *name, const struct fiber_attr *fiber_attr, - fiber_func f) + fiber_func f) { struct cord *cord = cord(); struct fiber *fiber = NULL; @@ -966,14 +1015,14 @@ fiber_new_ex(const char *name, const struct fiber_attr *fiber_attr, if (!(fiber_attr->flags & FIBER_CUSTOM_STACK) && !rlist_empty(&cord->dead)) { fiber = rlist_first_entry(&cord->dead, - struct fiber, link); + struct fiber, link); rlist_move_entry(&cord->alive, fiber, link); } else { fiber = (struct fiber *) mempool_alloc(&cord->fiber_mempool); if (fiber == NULL) { diag_set(OutOfMemory, sizeof(struct fiber), - "fiber pool", "fiber"); + "fiber pool", "fiber"); return NULL; } memset(fiber, 0, sizeof(struct fiber)); @@ -1057,7 +1106,7 @@ fiber_destroy_all(struct cord *cord) { while (!rlist_empty(&cord->alive)) fiber_destroy(cord, rlist_first_entry(&cord->alive, - struct fiber, link)); + struct fiber, link)); while (!rlist_empty(&cord->dead)) fiber_destroy(cord, rlist_first_entry(&cord->dead, struct fiber, link)); @@ -1130,13 +1179,13 @@ cord_destroy(struct cord *cord) struct cord_thread_arg { - struct cord *cord; - const char *name; - void *(*f)(void *); - void *arg; - bool is_started; - pthread_mutex_t start_mutex; - pthread_cond_t start_cond; + struct cord *cord; + const char *name; + void *(*f)(void *); + void *arg; + bool is_started; + pthread_mutex_t start_mutex; + pthread_cond_t start_cond; }; /** @@ -1181,7 +1230,7 @@ cord_start(struct cord *cord, const char *name, void *(*f)(void *), void *arg) { int res = -1; struct cord_thread_arg ct_arg = { cord, name, f, arg, false, - PTHREAD_MUTEX_INITIALIZER, PTHREAD_COND_INITIALIZER }; + PTHREAD_MUTEX_INITIALIZER, PTHREAD_COND_INITIALIZER }; tt_pthread_mutex_lock(&ct_arg.start_mutex); cord->loop = ev_loop_new(EVFLAG_AUTO | EVFLAG_ALLOCFD); if (cord->loop == NULL) { @@ -1189,14 +1238,14 @@ cord_start(struct cord *cord, const char *name, void *(*f)(void *), void *arg) goto end; } if (tt_pthread_create(&cord->id, NULL, - cord_thread_func, &ct_arg) != 0) { + cord_thread_func, &ct_arg) != 0) { diag_set(SystemError, "failed to create thread"); goto end; } res = 0; while (! ct_arg.is_started) tt_pthread_cond_wait(&ct_arg.start_cond, &ct_arg.start_mutex); -end: + end: if (res != 0) { if (cord->loop) { ev_loop_destroy(cord->loop); @@ -1232,15 +1281,15 @@ cord_join(struct cord *cord) /** The state of the waiter for a thread to complete. */ struct cord_cojoin_ctx { - struct ev_loop *loop; - /** Waiting fiber. */ - struct fiber *fiber; - /* - * This event is signalled when the subject thread is - * about to die. - */ - struct ev_async async; - bool task_complete; + struct ev_loop *loop; + /** Waiting fiber. */ + struct fiber *fiber; + /* + * This event is signalled when the subject thread is + * about to die. + */ + struct ev_async async; + bool task_complete; }; static void diff --git a/test/box/errinj.result b/test/box/errinj.result index 0b2cb7409..a4906547d 100644 --- a/test/box/errinj.result +++ b/test/box/errinj.result @@ -35,6 +35,8 @@ evals - - ERRINJ_AUTO_UPGRADE: false - ERRINJ_BUILD_INDEX: -1 - ERRINJ_DYN_MODULE_COUNT: 0 + - ERRINJ_FIBER_MADVISE: false + - ERRINJ_FIBER_MPROTECT: -1 - ERRINJ_HTTPC_EXECUTE: false - ERRINJ_HTTP_RESPONSE_ADD_WAIT: false - ERRINJ_INDEX_ALLOC: false diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt index 6dfb10f46..cdafd785f 100644 --- a/test/unit/CMakeLists.txt +++ b/test/unit/CMakeLists.txt @@ -72,6 +72,10 @@ add_executable(fiber.test fiber.cc) set_source_files_properties(fiber.cc PROPERTIES COMPILE_FLAGS -O0) target_link_libraries(fiber.test core unit) +add_executable(fiber_stack.test fiber_stack.c) +set_source_files_properties(fiber_stack.c PROPERTIES COMPILE_FLAGS -O0) +target_link_libraries(fiber_stack.test core unit) + if (NOT ENABLE_GCOV) # This test is known to be broken with GCOV add_executable(guard.test guard.cc) diff --git a/test/unit/fiber_stack.c b/test/unit/fiber_stack.c new file mode 100644 index 000000000..103dfaf0d --- /dev/null +++ b/test/unit/fiber_stack.c @@ -0,0 +1,79 @@ +#include "memory.h" +#include "fiber.h" +#include "unit.h" +#include "trivia/util.h" +#include "errinj.h" + +static struct fiber_attr default_attr; + +static int +noop_f(va_list ap) +{ + return 0; +} + +static int +main_f(va_list ap) +{ + struct errinj *inj; + struct fiber *fiber; + + header(); + plan(4); + + /* + * Set non-default stack size to prevent reusing of an + * existing fiber. + */ + struct fiber_attr *fiber_attr = fiber_attr_new(); + fiber_attr_setstacksize(fiber_attr, default_attr.stack_size * 2); + + /* + * Clear the fiber's diagnostics area to check that failed + * fiber_new() sets an error. + */ + diag_clear(diag_get()); + + /* + * Check guard page setup via mprotect. We can't test the fiber + * destroy path since it clears fiber's diag. + */ + inj = errinj(ERRINJ_FIBER_MPROTECT, ERRINJ_INT); + inj->iparam = PROT_NONE; + fiber = fiber_new_ex("test_mprotect", fiber_attr, noop_f); + inj->iparam = -1; + + ok(fiber == NULL, "mprotect: failed to setup fiber guard page"); + ok(diag_get() != NULL, "mprotect: diag is armed after error"); + + /* + * Check madvise. We can't test the fiber destroy + * path since it is cleaning error. + */ + diag_clear(diag_get()); + inj = errinj(ERRINJ_FIBER_MADVISE, ERRINJ_BOOL); + inj->bparam = true; + fiber = fiber_new_ex("test_madvise", fiber_attr, noop_f); + inj->bparam = false; + + ok(fiber != NULL, "madvise: non critical error on madvise hint"); + ok(diag_get() != NULL, "madvise: diag is armed after error"); + + footer(); + + ev_break(loop(), EVBREAK_ALL); + return check_plan(); +} + +int main() +{ + memory_init(); + fiber_init(fiber_c_invoke); + fiber_attr_create(&default_attr); + struct fiber *f = fiber_new("main", main_f); + fiber_wakeup(f); + ev_run(loop(), 0); + fiber_free(); + memory_free(); + return 0; +} diff --git a/test/unit/fiber_stack.result b/test/unit/fiber_stack.result new file mode 100644 index 000000000..43ff74b2f --- /dev/null +++ b/test/unit/fiber_stack.result @@ -0,0 +1,8 @@ +SystemError fiber mprotect failed: Cannot allocate memory + *** main_f *** +1..4 +ok 1 - mprotect: failed to setup fiber guard page +ok 2 - mprotect: diag is armed after error +ok 3 - madvise: non critical error on madvise hint +ok 4 - madvise: diag is armed after error + *** main_f: done *** diff --git a/test/unit/suite.ini b/test/unit/suite.ini index 75c80ece1..b48eb9c76 100644 --- a/test/unit/suite.ini +++ b/test/unit/suite.ini @@ -1,4 +1,5 @@ [default] core = unittest description = unit tests +release_disabled = fiber_stack.test is_parallel = True -- 2.17.1