* [Tarantool-patches] [PATCH 1/2] fiber: set diagnostics at madvise/mprotect failure
2020-07-29 16:50 [Tarantool-patches] [PATCH 0/2] fiber: backport for stack madvise/mprotect errors handling Ilya Kosarev
@ 2020-07-29 16:50 ` Ilya Kosarev
2020-07-29 17:04 ` Cyrill Gorcunov
2020-07-29 16:50 ` [Tarantool-patches] [PATCH 2/2] fiber: leak slab if unable to bring prots back Ilya Kosarev
1 sibling, 1 reply; 4+ messages in thread
From: Ilya Kosarev @ 2020-07-29 16:50 UTC (permalink / raw)
To: gorcunov; +Cc: tarantool-patches
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 <gorcunov@gmail.com>
(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
^ permalink raw reply [flat|nested] 4+ messages in thread