[PATCH 2/2] lib/core/fiber: Relax stack memory usage on recycle
Vladimir Davydov
vdavydov.dev at gmail.com
Mon Mar 18 19:57:12 MSK 2019
On Fri, Mar 15, 2019 at 11:58:02PM +0300, Cyrill Gorcunov wrote:
> We want to detect a situation where task in fiber is too eager for
> stack memory and relax rss usage in such case. For this sake upon
> stack creation we put 8 marks near 64K bound (such params allows us
> to fill ~1/4 of a page, which seem reasonable but we might change
> this params with time).
>
> Once stack get recycled we investigate the marks and if they were
> overwritten we drop all pages behind to relax memory usage (if OS
> supports madvise syscall).
>
> Another important moment is that we're marking the whole stack
> as not present thus if fiber never stepped over 64K limit the
> marks will be in tact and it means the fibers are light ones
> there won't be much #pf in future.
>
> Later we plan to implement an intelligent fiber scheduling
> considering how many memory fibers consume in average.
>
> Part-of #3418
> ---
> src/lib/core/fiber.c | 182 +++++++++++++++++++++++++++++++++++++++++++
> src/lib/core/fiber.h | 12 +++
> 2 files changed, 194 insertions(+)
>
> diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
> index bf2a22bed..4739e2708 100644
> --- a/src/lib/core/fiber.c
> +++ b/src/lib/core/fiber.c
> @@ -104,6 +104,31 @@ static const struct fiber_attr fiber_attr_default = {
> .flags = FIBER_DEFAULT_FLAGS
> };
>
> +#ifndef TARGET_OS_DARWIN
Forgot to tell you before - better use cmake's check_symbol_exists for
MADV_DONTNEED detection. Fixed.
> +/*
> + * Random values generated with uuid.
> + */
> +static const uint64_t poison_pool[] = {
> + 0x74f31d37285c4c37, 0xb10269a05bf10c29,
> + 0x0994d845bd284e0f, 0x9ffd4f7129c184df,
> + 0x357151e6711c4415, 0x8c5e5f41aafe6f28,
> + 0x6917dd79e78049d5, 0xba61957c65ca2465,
> +};
> +
> +/*
> + * We poison by 8 bytes as it natural for stack
> + * step on x86-64. Also 128 byte gap between
> + * poison values should cover a common cases.
> + */
> +#define POISON_SIZE (sizeof(poison_pool) / sizeof(poison_pool[0]))
> +#define POISON_GAP (128 + sizeof(poison_pool[0]))
POISON_GAP isn't used anywhere except POISON_OFF definition. Removed it.
Also, I changed the gap to exactly 128 bytes (not 128 + 8). This doesn't
change anything, but the code looks better.
> +#define POISON_OFF (POISON_GAP / sizeof(poison_pool[0]))
> +
> +static void fiber_wmark_recycle(struct fiber *fiber);
> +#else
> +# define fiber_wmark_recycle(fiber)
> +#endif /* !TARGET_OS_DARWIN */
> +
> void
> fiber_attr_create(struct fiber_attr *fiber_attr)
> {
> @@ -624,6 +649,7 @@ fiber_recycle(struct fiber *fiber)
> /* no pending wakeup */
> assert(rlist_empty(&fiber->state));
> bool has_custom_stack = fiber->flags & FIBER_CUSTOM_STACK;
> + fiber_wmark_recycle(fiber);
Renamed to fiber_stack_recycle.
> fiber_reset(fiber);
> fiber->name[0] = '\0';
> fiber->f = NULL;
> @@ -710,6 +736,160 @@ page_align_up(void *ptr)
> return page_align_down(ptr + page_size - 1);
> }
>
> +#ifndef TARGET_OS_DARWIN
> +
> +/** Test if address is page aligned. */
> +static inline bool
> +is_page_aligned(void *ptr)
> +{
> + return (uintptr_t)ptr & ~(page_size - 1);
> +}
This function is only used in assert() so OS X clang complained it isn't
used in release mode. I removed it. Instead I log madvise() failures (in
a separate patch).
> +
> +/**
> + * Check if stack poison values are present starting
> + * from the address provided.
> + */
> +static bool
> +stack_has_wmark(void *addr)
We don't typically use abbreviation, like 'wmark' instead of
'watermark'. I renamed it to 'watermark' everywhere.
> +{
> + const uint64_t *src = poison_pool;
> + const uint64_t *dst = addr;
> + size_t i;
> +
> + for (i = 0; i < POISON_SIZE; i++) {
> + if (*dst != src[i])
> + return false;
> + dst += POISON_OFF;
> + }
> +
> + return true;
> +}
> +
> +/**
> + * Put stack poison values starting
> + * from the address provided.
> + */
> +static void
> +stack_put_wmark(void *addr)
> +{
> + const uint64_t *src = poison_pool;
> + uint64_t *dst = addr;
> + size_t i;
> +
> + for (i = 0; i < POISON_SIZE; i++) {
> + *dst = src[i];
> + dst += POISON_OFF;
> + }
> +}
> +
> +/**
> + * Shrink stack by dropping pages outside of rss limit.
> + */
> +static void
> +stack_shrink(struct fiber *fiber)
> +{
> + void *start, *end;
> +
> + /*
> + * When dropping pages make sure the page
> + * containing overflow mark is untouched.
> + * Same time no need to unmap the page which
> + * carries "shrink" wmark, since we're updating
> + * this page anyway.
There's no shrink/overflow watermark anymore. I fixed the comment.
> + */
> + if (stack_direction < 0) {
> + start = fiber->stack;
> + end = page_align_down(fiber->stack_shrink_wmark);
> + } else {
> + end = fiber->stack + fiber->stack_size;
> + start = page_align_down(fiber->stack_shrink_wmark);
Should be page_align_up. Fixed.
> + }
> +
> + assert(is_page_aligned(start));
> +
> + madvise(start, end - start, MADV_DONTNEED);
> + stack_put_wmark(fiber->stack_shrink_wmark);
> +}
> +
> +/**
> + * Investigate stack watermarks on a fiber recycle.
> + */
> +static void
> +fiber_wmark_recycle(struct fiber *fiber)
> +{
> + if (fiber->stack == NULL || fiber->flags & FIBER_CUSTOM_STACK)
> + return;
I'd rather simply check if fiber->stack_watermark is NULL. Fixed.
> +
> + /*
> + * On recycle we're trying to shrink stack
> + * to release memory pressure but if only
> + * a fiber has been using too much memory.
> + */
> + if (!stack_has_wmark(fiber->stack_shrink_wmark))
> + stack_shrink(fiber);
This function isn't really necessary. Inlined it.
> +}
> +
> +/**
> + * Initialize stack watermarks.
> + */
> +static void
> +fiber_wmark_init(struct fiber *fiber)
> +{
> + /* stack size not causing much memory pressure */
> + static const unsigned rss_limit = 65536;
Moved this constant under FIBER_STACK_SIZE_ enum.
> +
> + /* offset base for marks distribution */
> + static const unsigned offset_base = 128;
Used POISON_OFF instead.
> +
> + /*
> + * No tracking on custom stacks for simplicity.
> + */
> + if (fiber->flags & FIBER_CUSTOM_STACK) {
> + fiber->stack_shrink_wmark = NULL;
> + fiber->wmark_inpage_offset = 0;
> + return;
> + }
> +
> + /*
> + * We don't expect the whole stack usage in regular
> + * loads, lets try to minimize rss pressure.
> + */
> + assert(is_page_aligned(fiber->stack));
> + madvise(fiber->stack, fiber->stack_size, MADV_DONTNEED);
> +
> + /*
> + * To increase probability of the stack overflow
> + * detection we put first mark at random position
> + * of the first @inpage_offset_base bytes range.
> + * The rest of the marks are put with constant step
> + * simply to not carry offsets in memory.
> + */
> + fiber->wmark_inpage_offset = rand() % offset_base;
> + fiber->wmark_inpage_offset = (fiber->wmark_inpage_offset + 8) & ~7;
> +
> + /*
> + * Initially we arm the last page of the stack
> + * to catch if we're getting close to its exhausting.
> + *
> + * The shrink watermark is put at 64K limit which is
> + * known value to not cause much memory pressue even
> + * with large number of fibers.
> + */
> + if (stack_direction < 0) {
> + fiber->stack_shrink_wmark = fiber->stack + fiber->stack_size;
> + fiber->stack_shrink_wmark -= rss_limit;
> + fiber->stack_shrink_wmark += fiber->wmark_inpage_offset;
> + } else {
> + fiber->stack_shrink_wmark = fiber->stack + rss_limit;
> + fiber->stack_shrink_wmark -= page_size;
> + fiber->stack_shrink_wmark += fiber->wmark_inpage_offset;
> + }
> + stack_put_wmark(fiber->stack_shrink_wmark);
> +}
> +#else
> +# define fiber_wmark_init(fiber)
> +#endif /* !TARGET_OS_DARWIN */
> +
> static int
> fiber_stack_create(struct fiber *fiber, size_t stack_size)
> {
> @@ -750,6 +930,7 @@ fiber_stack_create(struct fiber *fiber, size_t stack_size)
> (char *)fiber->stack +
> fiber->stack_size);
>
> + fiber_wmark_init(fiber);
> mprotect(guard, page_size, PROT_NONE);
> return 0;
> }
> @@ -923,6 +1104,7 @@ cord_create(struct cord *cord, const char *name)
> cord->sched.stack = NULL;
> cord->sched.stack_size = 0;
> #endif
> + cord->sched.stack_shrink_wmark = NULL;
> }
>
> void
> diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h
> index f1f5a0555..e1364d413 100644
> --- a/src/lib/core/fiber.h
> +++ b/src/lib/core/fiber.h
> @@ -348,6 +348,18 @@ struct fiber {
> struct slab *stack_slab;
> /** Coro stack addr. */
> void *stack;
> +#ifndef TARGET_OS_DARWIN
> + /**
> + * Stack watermark addr to detect
> + * if we need shrink stack on reuse.
> + */
> + void *stack_shrink_wmark;
Renamed to stack_watermark. After all, since you removed overlow
watermark, there's the only watermark left.
> + /**
> + * An offset to watermark position in stack
> + * since page bound address.
> + */
> + unsigned int wmark_inpage_offset;
No need in this variable anymore. I removed it.
> +#endif
> /** Coro stack size. */
> size_t stack_size;
> /** Valgrind stack id. */
Pushed this patch as well as the previous one to 2.1 and 1.10 with my
fixes applied on top. Here's the final patch:
>From 553dc562342a52cb44d74a7521c9c8bec70c96a5 Mon Sep 17 00:00:00 2001
From: Cyrill Gorcunov <gorcunov at gmail.com>
Date: Fri, 15 Mar 2019 23:58:02 +0300
Subject: [PATCH] lib/core/fiber: Relax stack memory usage on recycle
We want to detect a situation where task in fiber is too eager for
stack memory and relax rss usage in such case. For this sake upon
stack creation we put 8 marks near 64K bound (such params allows us
to fill ~1/4 of a page, which seem reasonable but we might change
this params with time).
Once stack get recycled we investigate the marks and if they were
overwritten we drop all pages behind to relax memory usage (if OS
supports madvise syscall).
Another important moment is that we're marking the whole stack
as not present thus if fiber never stepped over 64K limit the
marks will be in tact and it means the fibers are light ones
there won't be much #pf in future.
Later we plan to implement an intelligent fiber scheduling
considering how many memory fibers consume in average.
@locker:
- fix watermark page alignment for grow-up stack
- improve MADV_DONTNEED check
- clean up code and elaborate comments
- add test case to unit/fiber
- fix unit/guard test
Follow-up #3418
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 19a26097..7658fc6c 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -76,6 +76,7 @@ set(CMAKE_REQUIRED_DEFINITIONS "-D_GNU_SOURCE")
check_symbol_exists(MAP_ANON sys/mman.h HAVE_MAP_ANON)
check_symbol_exists(MAP_ANONYMOUS sys/mman.h HAVE_MAP_ANONYMOUS)
+check_symbol_exists(MADV_DONTNEED sys/mman.h HAVE_MADV_DONTNEED)
check_include_file(sys/time.h HAVE_SYS_TIME_H)
check_include_file(cpuid.h HAVE_CPUID_H)
check_include_file(sys/prctl.h HAVE_PRCTL_H)
diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
index c9813ba2..6fea775f 100644
--- a/src/lib/core/fiber.c
+++ b/src/lib/core/fiber.c
@@ -96,6 +96,8 @@ enum {
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 */
@@ -104,6 +106,28 @@ static const struct fiber_attr fiber_attr_default = {
.flags = FIBER_DEFAULT_FLAGS
};
+#ifdef HAVE_MADV_DONTNEED
+/*
+ * Random values generated with uuid.
+ * Used for stack poisoning.
+ */
+static const uint64_t poison_pool[] = {
+ 0x74f31d37285c4c37, 0xb10269a05bf10c29,
+ 0x0994d845bd284e0f, 0x9ffd4f7129c184df,
+ 0x357151e6711c4415, 0x8c5e5f41aafe6f28,
+ 0x6917dd79e78049d5, 0xba61957c65ca2465,
+};
+
+/*
+ * We poison by 8 bytes as it's natural for stack
+ * step on x86-64. Also 128 byte gap between poison
+ * values should cover common cases.
+ */
+#define POISON_SIZE (sizeof(poison_pool) / sizeof(poison_pool[0]))
+#define POISON_OFF (128 / sizeof(poison_pool[0]))
+
+#endif /* HAVE_MADV_DONTNEED */
+
void
fiber_attr_create(struct fiber_attr *fiber_attr)
{
@@ -157,6 +181,9 @@ static void
fiber_recycle(struct fiber *fiber);
static void
+fiber_stack_recycle(struct fiber *fiber);
+
+static void
fiber_destroy(struct cord *cord, struct fiber *f);
/**
@@ -624,6 +651,7 @@ fiber_recycle(struct fiber *fiber)
/* no pending wakeup */
assert(rlist_empty(&fiber->state));
bool has_custom_stack = fiber->flags & FIBER_CUSTOM_STACK;
+ fiber_stack_recycle(fiber);
fiber_reset(fiber);
fiber->name[0] = '\0';
fiber->f = NULL;
@@ -710,6 +738,114 @@ page_align_up(void *ptr)
return page_align_down(ptr + page_size - 1);
}
+#ifdef HAVE_MADV_DONTNEED
+/**
+ * Check if stack poison values are present starting from
+ * the address provided.
+ */
+static bool
+stack_has_watermark(void *addr)
+{
+ const uint64_t *src = poison_pool;
+ const uint64_t *dst = addr;
+ size_t i;
+
+ for (i = 0; i < POISON_SIZE; i++) {
+ if (*dst != src[i])
+ return false;
+ dst += POISON_OFF;
+ }
+ return true;
+}
+
+/**
+ * Put stack poison values starting from the address provided.
+ */
+static void
+stack_put_watermark(void *addr)
+{
+ const uint64_t *src = poison_pool;
+ uint64_t *dst = addr;
+ size_t i;
+
+ for (i = 0; i < POISON_SIZE; i++) {
+ *dst = src[i];
+ dst += POISON_OFF;
+ }
+}
+
+/**
+ * Free stack memory above the watermark when a fiber is recycled.
+ * To avoid a pointless syscall invocation in case the fiber hasn't
+ * touched memory above the watermark, we only call madvise() if
+ * the fiber has overwritten a poison value.
+ */
+static void
+fiber_stack_recycle(struct fiber *fiber)
+{
+ if (fiber->stack_watermark == NULL ||
+ stack_has_watermark(fiber->stack_watermark))
+ return;
+ /*
+ * When dropping pages make sure the page containing
+ * the watermark isn't touched since we're updating
+ * it anyway.
+ */
+ void *start, *end;
+ if (stack_direction < 0) {
+ start = fiber->stack;
+ end = page_align_down(fiber->stack_watermark);
+ } else {
+ start = page_align_up(fiber->stack_watermark);
+ end = fiber->stack + fiber->stack_size;
+ }
+ madvise(start, end - start, MADV_DONTNEED);
+ stack_put_watermark(fiber->stack_watermark);
+}
+
+/**
+ * Initialize fiber stack watermark.
+ */
+static void
+fiber_stack_watermark_create(struct fiber *fiber)
+{
+ assert(fiber->stack_watermark == NULL);
+
+ /* No tracking on custom stacks for simplicity. */
+ if (fiber->flags & FIBER_CUSTOM_STACK)
+ return;
+
+ /*
+ * We don't expect the whole stack usage in regular
+ * loads, let's try to minimize rss pressure.
+ */
+ madvise(fiber->stack, fiber->stack_size, MADV_DONTNEED);
+
+ /*
+ * To increase probability of stack overflow detection
+ * we put the first mark at a random position.
+ */
+ size_t offset = rand() % POISON_OFF * sizeof(poison_pool[0]);
+ if (stack_direction < 0) {
+ fiber->stack_watermark = fiber->stack + fiber->stack_size;
+ fiber->stack_watermark -= FIBER_STACK_SIZE_WATERMARK;
+ fiber->stack_watermark += offset;
+ } else {
+ fiber->stack_watermark = fiber->stack;
+ fiber->stack_watermark += FIBER_STACK_SIZE_WATERMARK;
+ fiber->stack_watermark -= page_size;
+ fiber->stack_watermark += offset;
+ }
+ stack_put_watermark(fiber->stack_watermark);
+}
+#else
+static void
+fiber_stack_watermark_create(struct fiber *fiber)
+{
+ (void)fiber;
+}
+#endif /* HAVE_MADV_DONTNEED */
+
static int
fiber_stack_create(struct fiber *fiber, size_t stack_size)
{
@@ -751,6 +887,7 @@ fiber_stack_create(struct fiber *fiber, size_t stack_size)
fiber->stack_size);
mprotect(guard, page_size, PROT_NONE);
+ fiber_stack_watermark_create(fiber);
return 0;
}
@@ -923,6 +1060,7 @@ cord_create(struct cord *cord, const char *name)
cord->sched.stack = NULL;
cord->sched.stack_size = 0;
#endif
+ cord->sched.stack_watermark = NULL;
}
void
diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h
index f1f5a055..89fb0428 100644
--- a/src/lib/core/fiber.h
+++ b/src/lib/core/fiber.h
@@ -348,6 +348,19 @@ struct fiber {
struct slab *stack_slab;
/** Coro stack addr. */
void *stack;
+#ifdef HAVE_MADV_DONTNEED
+ /**
+ * We want to keep total stack memory usage low while still
+ * allowing tasks that need a greater than average stack.
+ * To achieve that, we write some poison values to stack
+ * at "watermark" position and call madvise(MADV_DONTNEED)
+ * when a fiber is recycled in case a poison value has been
+ * overwritten. This allows to keep per-fiber stack memory
+ * usage below the watermark while avoiding any performance
+ * penalty if there are no tasks eager for stack.
+ */
+ void *stack_watermark;
+#endif
/** Coro stack size. */
size_t stack_size;
/** Valgrind stack id. */
diff --git a/src/trivia/config.h.cmake b/src/trivia/config.h.cmake
index f5c2e2c5..ca0057d2 100644
--- a/src/trivia/config.h.cmake
+++ b/src/trivia/config.h.cmake
@@ -70,6 +70,7 @@
*/
#define MAP_ANONYMOUS MAP_ANON
#endif
+#cmakedefine HAVE_MADV_DONTNEED 1
/*
* Defined if O_DSYNC mode exists for open(2).
*/
diff --git a/test/unit/fiber.cc b/test/unit/fiber.cc
index 3e9c479c..91f7d43f 100644
--- a/test/unit/fiber.cc
+++ b/test/unit/fiber.cc
@@ -3,6 +3,8 @@
#include "unit.h"
#include "trivia/util.h"
+static struct fiber_attr default_attr;
+
static int
noop_f(va_list ap)
{
@@ -47,7 +49,7 @@ cancel_dead_f(va_list ap)
return 0;
}
-static size_t fiber_stack_size_default;
+static size_t stack_expand_limit;
static void NOINLINE
stack_expand(void *ptr)
@@ -56,7 +58,7 @@ stack_expand(void *ptr)
memset(buf, 0x45, 2048);
ptrdiff_t stack_diff = (buf - (char *)ptr);
stack_diff = stack_diff >= 0 ? stack_diff : -stack_diff;
- if (stack_diff < (ptrdiff_t)fiber_stack_size_default)
+ if (stack_diff < (ptrdiff_t)stack_expand_limit)
stack_expand(ptr);
}
@@ -122,21 +124,39 @@ fiber_join_test()
fiber_cancel(fiber);
fiber_join(fiber);
+ footer();
+}
+
+void
+fiber_stack_test()
+{
+ header();
+
+ struct fiber *fiber;
struct fiber_attr *fiber_attr;
+
+ /*
+ * Test a fiber with the default stack size.
+ */
+ stack_expand_limit = default_attr.stack_size * 3 / 4;
+ fiber = fiber_new_xc("test_stack", test_stack_f);
+ fiber_wakeup(fiber);
+ fiber_sleep(0);
+ note("normal-stack fiber not crashed");
+
+ /*
+ * Test a fiber with a custom stack size.
+ */
fiber_attr = fiber_attr_new();
- fiber_stack_size_default = fiber_attr_getstacksize(fiber_attr);
- fiber_attr_setstacksize(fiber_attr, fiber_stack_size_default * 2);
+ fiber_attr_setstacksize(fiber_attr, default_attr.stack_size * 2);
+ stack_expand_limit = default_attr.stack_size * 3 / 2;
fiber = fiber_new_ex("test_stack", fiber_attr, test_stack_f);
fiber_attr_delete(fiber_attr);
if (fiber == NULL)
diag_raise();
- fiber_set_joinable(fiber, true);
fiber_wakeup(fiber);
- /** Let the fiber schedule */
- fiber_wakeup(fiber());
- fiber_yield();
+ fiber_sleep(0);
note("big-stack fiber not crashed");
- fiber_join(fiber);
footer();
}
@@ -166,6 +186,7 @@ main_f(va_list ap)
{
fiber_name_test();
fiber_join_test();
+ fiber_stack_test();
ev_break(loop(), EVBREAK_ALL);
return 0;
}
@@ -174,6 +195,7 @@ int main()
{
memory_init();
fiber_init(fiber_cxx_invoke);
+ fiber_attr_create(&default_attr);
struct fiber *main = fiber_new_xc("main", main_f);
fiber_wakeup(main);
ev_run(loop(), 0);
diff --git a/test/unit/fiber.result b/test/unit/fiber.result
index 1f9773a5..7c9f85dc 100644
--- a/test/unit/fiber.result
+++ b/test/unit/fiber.result
@@ -12,5 +12,8 @@ SystemError Failed to allocate 42 bytes in allocator for exception: Cannot alloc
# exception propagated
# cancel dead has started
# by this time the fiber should be dead already
-# big-stack fiber not crashed
*** fiber_join_test: done ***
+ *** fiber_stack_test ***
+# normal-stack fiber not crashed
+# big-stack fiber not crashed
+ *** fiber_stack_test: done ***
diff --git a/test/unit/guard.cc b/test/unit/guard.cc
index 3d42fee3..24ab1fc2 100644
--- a/test/unit/guard.cc
+++ b/test/unit/guard.cc
@@ -2,6 +2,8 @@
#include "fiber.h"
#include "unit.h"
+static struct fiber_attr default_attr;
+
static void
sigsegf_handler(int signo)
{
@@ -21,7 +23,7 @@ stack_break_f(char *ptr)
memset(block, 0xff, 2048);
sum += block[block[4]];
ptrdiff_t stack_diff = ptr > block ? ptr - block : block - ptr;
- if (stack_diff < 65536)
+ if (stack_diff < (ptrdiff_t)default_attr.stack_size)
sum += stack_break_f(ptr);
return sum;
}
@@ -53,6 +55,7 @@ int main()
{
memory_init();
fiber_init(fiber_cxx_invoke);
+ fiber_attr_create(&default_attr);
struct fiber *fmain = fiber_new_xc("main", main_f);
fiber_wakeup(fmain);
ev_run(loop(), 0);
More information about the Tarantool-patches
mailing list