[rfc 0/4] fiber/stack: Increase stack size and shrink rss usage

[rfc 0/4] fiber/stack: Increase stack size and shrink rss usage

[Cyrill Gorcunov]

Hi! This series tries to address a problem with stack overflow
on new distros. Please review carefully. It is still rfc because

 - need to wrap code with more ifdefs for systems where no
   madvise syscall present, which I address next once we're
   agreed on the patches;

 - need intensive testing where I could really use some help
   from more experienced developers since I've no clue how
   to cause stack exhausting from inside of lua script

 - actually I prefer to not use histogram here since it
   makes code even more complex, and I seriously consider
   moving stack processing into a separate file, say
   fiber-stack.cc or something

Anyway, please take a look once time permit. Comments are welcome!

[rfc 1/4] core/fiber: Increase default stack size

[Cyrill Gorcunov]

[-- Attachment #1: fiber-increase-stack --]
[-- Type: text/plain, Size: 1353 bytes --]

The default 64K stack size used for years become too small
for modern distors (Fedora 29 and etc) where third party libraries
(such as ncurses) started to use 64K for own buffers and we get
SIGSGV early without reaching interactive console phase.

Thus we increase default size up to 256K which should fit
for common case. Later we will make this value configurable
to address arbitrary stack sizes without a need to rebuild
the code.

Note the values are switched to 4K page granularity for sake
of future modifications -- we gonna manipulate pages to
relax rss usage if OS allows.

Closes #3418
---
 src/lib/core/fiber.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Index: tarantool.git/src/lib/core/fiber.c
===================================================================
--- tarantool.git.orig/src/lib/core/fiber.c
+++ tarantool.git/src/lib/core/fiber.c
@@ -92,10 +92,10 @@ 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 = 65536
+	/* The minimum allowable fiber stack size in 4K page */
+	FIBER_STACK_SIZE_MINIMAL = 4 << 12,
+	/* Default fiber stack size in 4K page */
+	FIBER_STACK_SIZE_DEFAULT = 64 << 12,
 };
 
 /** Default fiber attributes */

[rfc 2/4] core/fiber: Mark stack as unneeded on creation

[Cyrill Gorcunov]

[-- Attachment #1: fiber-madvise-stack --]
[-- Type: text/plain, Size: 1037 bytes --]

Since we've increased the default stack size we hope
the whole 256K won't be used for regular loads thus
we mark the stack area as unneeded to minimize rss
pressure.

Note we do this on fiber creation at the moment, more
detailed stack usage analisys will be in next patches
since it doesn't depend on this change.

Closes #3418
---
 src/lib/core/fiber.c |    7 +++++++
 1 file changed, 7 insertions(+)

Index: tarantool.git/src/lib/core/fiber.c
===================================================================
--- tarantool.git.orig/src/lib/core/fiber.c
+++ tarantool.git/src/lib/core/fiber.c
@@ -749,6 +749,13 @@ fiber_stack_create(struct fiber *fiber,
 	fiber->stack_id = VALGRIND_STACK_REGISTER(fiber->stack,
 						  (char *)fiber->stack +
 						  fiber->stack_size);
+#ifndef TARGET_OS_DARWIN
+	/*
+	 * We don't expect the whole stack usage in regular
+	 * loads, lets try to minimize rss pressure.
+	 */
+	madvise(fiber->stack, fiber->stack_size, MADV_DONTNEED);
+#endif
 
 	mprotect(guard, page_size, PROT_NONE);
 	return 0;

[rfc 3/4] core/fiber: Put static watermark into stack to track its usage

[Cyrill Gorcunov]

[-- Attachment #1: fiber-stack-wmark --]
[-- Type: text/plain, Size: 4778 bytes --]

We want to detect a situation where task in fiber is too eager for
stack and close to its exhausting. For this sake upon stack creation
we put 8 marks on last stack page with step of 128 bytes. Such params
allows us to fill ~1/4 of a page, which does seem reasonable but
we might change this params with time.

Since the watermark position is permanent and some task is close
to stack limit we print about the situation once to not spam
a user much and stop putting the mark on recycling.

Closes #3418
---
 src/lib/core/fiber.c |  105 +++++++++++++++++++++++++++++++++++++++++++++++++++
 src/lib/core/fiber.h |    2 
 2 files changed, 107 insertions(+)

Index: tarantool.git/src/lib/core/fiber.c
===================================================================
--- tarantool.git.orig/src/lib/core/fiber.c
+++ tarantool.git/src/lib/core/fiber.c
@@ -104,6 +104,28 @@ static const struct fiber_attr fiber_att
        .flags = FIBER_DEFAULT_FLAGS
 };
 
+/*
+ * Random values generated with uuid.
+ * Try to fit a cache line.
+ */
+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]))
+#define POISON_OFF	(POISON_GAP / sizeof(poison_pool[0]))
+
+static void fiber_wmark_recycle(struct fiber *fiber);
+
 void
 fiber_attr_create(struct fiber_attr *fiber_attr)
 {
@@ -624,6 +646,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);
 	fiber_reset(fiber);
 	fiber->name[0] = '\0';
 	fiber->f = NULL;
@@ -710,6 +733,85 @@ page_align_up(void *ptr)
 	return page_align_down(ptr + page_size - 1);
 }
 
+static bool
+stack_has_wmark(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;
+}
+
+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;
+	}
+}
+
+static void
+fiber_wmark_recycle(struct fiber *fiber)
+{
+	static bool overflow_warned = false;
+
+	if (fiber->stack == NULL || fiber->flags & FIBER_CUSTOM_STACK)
+		return;
+
+	/*
+	 * We are watching for stack overflow in one shot way:
+	 * firstly to not spam a user with messages and secondly
+	 * to relax system from additional operations while our
+	 * watermark is not dynamic.
+	 */
+	if (overflow_warned)
+		return;
+
+	if (!stack_has_wmark(fiber->stack_wmark_ofl)) {
+		if (!overflow_warned) {
+			say_warn("fiber %d is close to a stack limit",
+				 fiber->name, fiber->fid);
+			overflow_warned = true;
+		}
+	}
+}
+
+static void
+fiber_wmark_init(struct fiber *fiber)
+{
+	/*
+	 * No tracking on custom stacks
+	 * in a sake of simplicity.
+	 */
+	if (fiber->flags & FIBER_CUSTOM_STACK) {
+		fiber->stack_wmark_ofl = NULL;
+		return;
+	}
+
+	/*
+	 * Initially we arm last page of a stack
+	 * to catch if we're getting close to
+	 * stack exhausting.
+	 */
+	if (stack_direction < 0)
+		fiber->stack_wmark_ofl = fiber->stack;
+	else
+		fiber->stack_wmark_ofl = fiber->stack + fiber->stack_size - page_size;
+	stack_put_wmark(fiber->stack_wmark_ofl);
+}
+
 static int
 fiber_stack_create(struct fiber *fiber, size_t stack_size)
 {
@@ -757,6 +859,7 @@ fiber_stack_create(struct fiber *fiber,
 	madvise(fiber->stack, fiber->stack_size, MADV_DONTNEED);
 #endif
 
+	fiber_wmark_init(fiber);
 	mprotect(guard, page_size, PROT_NONE);
 	return 0;
 }
@@ -926,8 +1029,10 @@ cord_create(struct cord *cord, const cha
 	/* Record stack extents */
 	tt_pthread_attr_getstack(cord->id, &cord->sched.stack,
 				 &cord->sched.stack_size);
+	cord->sched.stack_wmark_ofl = cord->sched.stack;
 #else
 	cord->sched.stack = NULL;
+	cord->sched.stack_wmark_ofl = NULL;
 	cord->sched.stack_size = 0;
 #endif
 }
Index: tarantool.git/src/lib/core/fiber.h
===================================================================
--- tarantool.git.orig/src/lib/core/fiber.h
+++ tarantool.git/src/lib/core/fiber.h
@@ -348,6 +348,8 @@ struct fiber {
 	struct slab *stack_slab;
 	/** Coro stack addr. */
 	void *stack;
+	/** Stack watermark addr for overflow detection. */
+	void *stack_wmark_ofl;
 	/** Coro stack size. */
 	size_t stack_size;
 	/** Valgrind stack id. */

Re: [tarantool-patches] [rfc 3/4] core/fiber: Put static watermark into stack to track its usage

[Konstantin Osipov]

* Cyrill Gorcunov <gorcunov@gmail.com> [19/03/03 23:25]:
> We want to detect a situation where task in fiber is too eager for
> stack and close to its exhausting. For this sake upon stack creation
> we put 8 marks on last stack page with step of 128 bytes. Such params
> allows us to fill ~1/4 of a page, which does seem reasonable but
> we might change this params with time.
> 
> Since the watermark position is permanent and some task is close
> to stack limit we print about the situation once to not spam
> a user much and stop putting the mark on recycling.

> +	/*
> +	 * Initially we arm last page of a stack
> +	 * to catch if we're getting close to
> +	 * stack exhausting.
> +	 */
> +	if (stack_direction < 0)
> +		fiber->stack_wmark_ofl = fiber->stack;
> +	else
> +		fiber->stack_wmark_ofl = fiber->stack + fiber->stack_size - page_size;
> +	stack_put_wmark(fiber->stack_wmark_ofl);
> +}

Could you please incorporate some randomness when assigning this
address? A simple pseudo-random number within range 0
..page_size/2 would make me happy.


-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

Re: [tarantool-patches] [rfc 3/4] core/fiber: Put static watermark into stack to track its usage

[Cyrill Gorcunov]

On Tue, Mar 05, 2019 at 11:08:13AM +0300, Konstantin Osipov wrote:
...
> 
> Could you please incorporate some randomness when assigning this
> address? A simple pseudo-random number within range 0
> ..page_size/2 would make me happy.

Sure. Also I think we should switch to 2M pages for stack since
it will relax tlb pressue significanly but this should be done
on top as a separate patchset, later.

Re: [tarantool-patches] [rfc 3/4] core/fiber: Put static watermark into stack to track its usage

[Konstantin Osipov]

* Cyrill Gorcunov <gorcunov@gmail.com> [19/03/03 23:25]:
> +/*
> + * Random values generated with uuid.
> + * Try to fit a cache line.
> + */
> +static const uint64_t poison_pool[] = {
> +	0x74f31d37285c4c37, 0xb10269a05bf10c29,
> +	0x0994d845bd284e0f, 0x9ffd4f7129c184df,
> +	0x357151e6711c4415, 0x8c5e5f41aafe6f28,
> +	0x6917dd79e78049d5, 0xba61957c65ca2465,
> +};

With randomness incorporated into each fiber I think we can reduce
the poison pool to 4 8-byte integers or even less.


-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

Re: [tarantool-patches] [rfc 3/4] core/fiber: Put static watermark into stack to track its usage

[Cyrill Gorcunov]

On Tue, Mar 05, 2019 at 11:10:13AM +0300, Konstantin Osipov wrote:
> * Cyrill Gorcunov <gorcunov@gmail.com> [19/03/03 23:25]:
> > +/*
> > + * Random values generated with uuid.
> > + * Try to fit a cache line.
> > + */
> > +static const uint64_t poison_pool[] = {
> > +	0x74f31d37285c4c37, 0xb10269a05bf10c29,
> > +	0x0994d845bd284e0f, 0x9ffd4f7129c184df,
> > +	0x357151e6711c4415, 0x8c5e5f41aafe6f28,
> > +	0x6917dd79e78049d5, 0xba61957c65ca2465,
> > +};
> 
> With randomness incorporated into each fiber I think we can reduce
> the poison pool to 4 8-byte integers or even less.

Will do, thanks!

Re: [tarantool-patches] [rfc 3/4] core/fiber: Put static watermark into stack to track its usage

[Cyrill Gorcunov]

On Tue, Mar 05, 2019 at 11:10:13AM +0300, Konstantin Osipov wrote:
> * Cyrill Gorcunov <gorcunov@gmail.com> [19/03/03 23:25]:
> > +/*
> > + * Random values generated with uuid.
> > + * Try to fit a cache line.
> > + */
> > +static const uint64_t poison_pool[] = {
> > +	0x74f31d37285c4c37, 0xb10269a05bf10c29,
> > +	0x0994d845bd284e0f, 0x9ffd4f7129c184df,
> > +	0x357151e6711c4415, 0x8c5e5f41aafe6f28,
> > +	0x6917dd79e78049d5, 0xba61957c65ca2465,
> > +};
> 
> With randomness incorporated into each fiber I think we can reduce
> the poison pool to 4 8-byte integers or even less.

Kostja, you know, randoms values is a bad idea. For example I just got

[cyrill@uranus tarantool.git] ./src/tarantool
poison_gaps[0] = 736
poison_gaps[1] = 952
poison_gaps[2] = 976
poison_gaps[3] = 2016
Tarantool 2.1.1-360-g6666db8d5
type 'help' for interactive help

So the first gap almost close to 1K value :( Which means we won't detect
the page dirtifying while someone is putting values on start of the
page and even more, we're not controlling what exactly random values
come to us. I think putting poisons at 128 byte bound a way better

The pretty draf code I used is
---
static void
stack_poison_gaps_init(void)
{
	size_t i, j, m = page_size / 2;
	uint16_t v;

	assert(POISON_SIZE > 1);

	/*
	 * Fill the gaps with random (8 byte aligned)
	 * values in a half page range. This should decrease
	 * probability that someone manage to sneak out
	 * the poison values.
	 */

	random_bytes((void *)poison_gaps, sizeof(poison_gaps));
	for (i = 0; i < POISON_SIZE; i++) {
		v = poison_gaps[i] % m;
		poison_gaps[i] = (v + 8) & ~7;
	}

	for (i = 0; i < POISON_SIZE; i++) {
		v = poison_gaps[i], j = i;
		while (j > 0 && poison_gaps[j-1] > v)
			poison_gaps[j] = poison_gaps[j-1], j--;
		poison_gaps[j] = v;
	}

	for (i = 0; i < POISON_SIZE; i++) {
		say_info("poison_gaps[%d] = %d", i, (unsigned)poison_gaps[i]);
		if (i == 0)
			continue;
		assert(poison_gaps[i-1] != poison_gaps[i]);
	}
}
---

Re: [tarantool-patches] [rfc 3/4] core/fiber: Put static watermark into stack to track its usage

[Konstantin Osipov]

* Cyrill Gorcunov <gorcunov@gmail.com> [19/03/03 23:25]:
> We want to detect a situation where task in fiber is too eager for
> stack and close to its exhausting. For this sake upon stack creation
> we put 8 marks on last stack page with step of 128 bytes. Such params
> allows us to fill ~1/4 of a page, which does seem reasonable but
> we might change this params with time.
> 
> Since the watermark position is permanent and some task is close
> to stack limit we print about the situation once to not spam
> a user much and stop putting the mark on recycling.
> 
> Closes #3418

As a policy we write meaningful comments for every function, static or not,
obvious or not. The idea of such a comment is to reflect the
author's intent and technical trade-offs considered when designing
a function. The comment should preferably be concise, however.

> +/*
> + * Random values generated with uuid.
> + * Try to fit a cache line.

I don't understand this comment. Since you're putting a poison-gap
between each number, how could you possibly fit a cache line?

> + */
> +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.

> +	/** Stack watermark addr for overflow detection. */
> +	void *stack_wmark_ofl;

Generally we try to avoid abbreviations unless really necessary.
Why not simply stack_overflow_watermark? 

>  	/** Coro stack size. */
>  	size_t stack_size;
>  	/** Valgrind stack id. */

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

Re: [tarantool-patches] [rfc 3/4] core/fiber: Put static watermark into stack to track its usage

[Cyrill Gorcunov]

On Tue, Mar 05, 2019 at 11:20:29AM +0300, Konstantin Osipov wrote:
> 
> As a policy we write meaningful comments for every function, static or not,
> obvious or not. The idea of such a comment is to reflect the
> author's intent and technical trade-offs considered when designing
> a function. The comment should preferably be concise, however.
> 
> > +/*
> > + * Random values generated with uuid.
> > + * Try to fit a cache line.
> 
> I don't understand this comment. Since you're putting a poison-gap
> between each number, how could you possibly fit a cache line?

size of the poison_pool itself, we actively access this
array, so better keep it small enough

> > + */
> > +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.
> 
> > +	/** Stack watermark addr for overflow detection. */
> > +	void *stack_wmark_ofl;
> 
> Generally we try to avoid abbreviations unless really necessary.
> Why not simply stack_overflow_watermark? 

Well, as to me it is too long, but sure, will update.

	Cyrill

[rfc 4/4] core/fiber: Shrink stack when recycling

[Cyrill Gorcunov]

[-- Attachment #1: fiber-stack-wmark-dyna-2 --]
[-- Type: text/plain, Size: 4741 bytes --]

Currently we've a static mark in stack to detect the situation
where its usage is close to exhausting. We can move forward and
try to shrink stack everytime it is recycling.

For this sake we add wmark_addr pointing into a fiber and
put it next to the last page on the stack. On recycle we
test if mark has been modified. If it is still present
we wimply shrink stack by one page and continue this way
until stack usage is balanced. Once we met first overwrite
we stop tracking and freeze the mark to not longer do any
access assuming stack won't get bigger.

Note that "stack is close to overflow" is still present
since it is separate mechanism.

Closes #3418
---
 src/lib/core/fiber.c |   65 +++++++++++++++++++++++++++++++++++++++++++--------
 src/lib/core/fiber.h |    2 +
 2 files changed, 57 insertions(+), 10 deletions(-)

Index: tarantool.git/src/lib/core/fiber.c
===================================================================
--- tarantool.git.orig/src/lib/core/fiber.c
+++ tarantool.git/src/lib/core/fiber.c
@@ -124,6 +124,9 @@ static const uint64_t poison_pool[] = {
 #define POISON_GAP	(128 + sizeof(poison_pool[0]))
 #define POISON_OFF	(POISON_GAP / sizeof(poison_pool[0]))
 
+#define wmark_freeze(_pp) do { *((uintptr_t *)(_pp)) |= (uintptr_t)1; } while (0)
+#define wmark_frozen(_p) ((uintptr_t)(_p) & (uintptr_t)1)
+
 static void fiber_wmark_recycle(struct fiber *fiber);
 
 void
@@ -763,6 +766,31 @@ stack_put_wmark(void *addr)
 }
 
 static void
+stack_shrink(struct fiber *fiber)
+{
+	void *hi, *lo;
+
+	if (stack_direction < 0) {
+		hi = fiber->stack + fiber->stack_size;
+		lo = fiber->stack_wmark_ofl + page_size;
+	} else {
+		hi = fiber->stack_wmark_ofl - page_size;
+		lo = fiber->stack - fiber->stack_size;
+	}
+
+	if (fiber->stack_wmark <= lo ||
+	    fiber->stack_wmark >= hi)
+		return;
+
+	madvise(fiber->stack_wmark, page_size, MADV_DONTNEED);
+	if (stack_direction < 0)
+		fiber->stack_wmark += page_size;
+	else
+		fiber->stack_wmark -= page_size;
+	stack_put_wmark(fiber);
+}
+
+static void
 fiber_wmark_recycle(struct fiber *fiber)
 {
 	static bool overflow_warned = false;
@@ -772,20 +800,30 @@ fiber_wmark_recycle(struct fiber *fiber)
 
 	/*
 	 * We are watching for stack overflow in one shot way:
-	 * firstly to not spam a user with messages and secondly
-	 * to relax system from additional operations while our
-	 * watermark is not dynamic.
+	 * to not spam a user with messages.
 	 */
-	if (overflow_warned)
-		return;
-
-	if (!stack_has_wmark(fiber->stack_wmark_ofl)) {
-		if (!overflow_warned) {
+	if (!overflow_warned) {
+		if (!stack_has_wmark(fiber->stack_wmark_ofl)) {
 			say_warn("fiber %d is close to a stack limit",
 				 fiber->name, fiber->fid);
 			overflow_warned = true;
 		}
 	}
+
+	if (wmark_frozen(fiber->stack_wmark))
+		return;
+
+	/*
+	 * On recycle we're trying to shrink stack
+	 * as much as we can until first mark overwrite
+	 * detected, then we simply freeze watermark and
+	 * assume the stack is balanced and won't change
+	 * much in future.
+	 */
+	if (!stack_has_wmark(fiber->stack_wmark))
+		wmark_freeze(&fiber->stack_wmark);
+	else
+		stack_shrink(fiber);
 }
 
 static void
@@ -797,6 +835,7 @@ fiber_wmark_init(struct fiber *fiber)
 	 */
 	if (fiber->flags & FIBER_CUSTOM_STACK) {
 		fiber->stack_wmark_ofl = NULL;
+		fiber->stack_wmark = NULL;
 		return;
 	}
 
@@ -805,11 +844,15 @@ fiber_wmark_init(struct fiber *fiber)
 	 * to catch if we're getting close to
 	 * stack exhausting.
 	 */
-	if (stack_direction < 0)
+	if (stack_direction < 0) {
 		fiber->stack_wmark_ofl = fiber->stack;
-	else
+		fiber->stack_wmark = fiber->stack_wmark_ofl + page_size;
+	} else {
 		fiber->stack_wmark_ofl = fiber->stack + fiber->stack_size - page_size;
+		fiber->stack_wmark = fiber->stack_wmark_ofl - page_size;
+	}
 	stack_put_wmark(fiber->stack_wmark_ofl);
+	stack_put_wmark(fiber->stack_wmark);
 }
 
 static int
@@ -1030,9 +1073,11 @@ cord_create(struct cord *cord, const cha
 	tt_pthread_attr_getstack(cord->id, &cord->sched.stack,
 				 &cord->sched.stack_size);
 	cord->sched.stack_wmark_ofl = cord->sched.stack;
+	cord->sched.stack_wmark = cord->sched.stack;
 #else
 	cord->sched.stack = NULL;
 	cord->sched.stack_wmark_ofl = NULL;
+	cord->sched.stack_wmark = NULL;
 	cord->sched.stack_size = 0;
 #endif
 }
Index: tarantool.git/src/lib/core/fiber.h
===================================================================
--- tarantool.git.orig/src/lib/core/fiber.h
+++ tarantool.git/src/lib/core/fiber.h
@@ -348,6 +348,8 @@ struct fiber {
 	struct slab *stack_slab;
 	/** Coro stack addr. */
 	void *stack;
+	/** Stack watermark addr. */
+	void *stack_wmark;
 	/** Stack watermark addr for overflow detection. */
 	void *stack_wmark_ofl;
 	/** Coro stack size. */

Re: [tarantool-patches] [rfc 4/4] core/fiber: Shrink stack when recycling

[Konstantin Osipov]

* Cyrill Gorcunov <gorcunov@gmail.com> [19/03/03 23:25]:
> +#define wmark_freeze(_pp) do { *((uintptr_t *)(_pp)) |= (uintptr_t)1; } while (0)
> +#define wmark_frozen(_p) ((uintptr_t)(_p) & (uintptr_t)1)

Why not static inline? A comment would help. Usually the name for
function returning true/false contains "is", e.g.
wmark_is_frozen().


>  
>  static void
> +stack_shrink(struct fiber *fiber)
> +{
> +	void *hi, *lo;
> +
> +	if (stack_direction < 0) {
> +		hi = fiber->stack + fiber->stack_size;
> +		lo = fiber->stack_wmark_ofl + page_size;
> +	} else {
> +		hi = fiber->stack_wmark_ofl - page_size;
> +		lo = fiber->stack - fiber->stack_size;
> +	}
> +
> +	if (fiber->stack_wmark <= lo ||
> +	    fiber->stack_wmark >= hi)
> +		return;
> +
> +	madvise(fiber->stack_wmark, page_size, MADV_DONTNEED);
> +	if (stack_direction < 0)
> +		fiber->stack_wmark += page_size;
> +	else
> +		fiber->stack_wmark -= page_size;
> +	stack_put_wmark(fiber);
> +}
> +
> +static void
>  fiber_wmark_recycle(struct fiber *fiber)
>  {
>  	static bool overflow_warned = false;
> @@ -772,20 +800,30 @@ fiber_wmark_recycle(struct fiber *fiber)
>  
>  	/*
>  	 * We are watching for stack overflow in one shot way:
> -	 * firstly to not spam a user with messages and secondly
> -	 * to relax system from additional operations while our
> -	 * watermark is not dynamic.
> +	 * to not spam a user with messages.
>  	 */
> -	if (overflow_warned)
> -		return;
> -
> -	if (!stack_has_wmark(fiber->stack_wmark_ofl)) {
> -		if (!overflow_warned) {
> +	if (!overflow_warned) {
> +		if (!stack_has_wmark(fiber->stack_wmark_ofl)) {
>  			say_warn("fiber %d is close to a stack limit",
>  				 fiber->name, fiber->fid);
>  			overflow_warned = true;
>  		}
>  	}
> +
> +	if (wmark_frozen(fiber->stack_wmark))
> +		return;
> +
> +	/*
> +	 * On recycle we're trying to shrink stack
> +	 * as much as we can until first mark overwrite
> +	 * detected, then we simply freeze watermark and
> +	 * assume the stack is balanced and won't change
> +	 * much in future.
> +	 */
> +	if (!stack_has_wmark(fiber->stack_wmark))
> +		wmark_freeze(&fiber->stack_wmark);

I don't see how "frozen" watermark concept is more powerful than
already existing "overflow warned" flag. I think the idea is to
have some sort of smart threshold after which we stop trying to
shrink the stack.

Right now this threshold is not too smart (which is OK), but
complicated (both overflow_warned and wmark_frozen() are
considered), which doesn't make too much sense to me.

> +	else
> +		stack_shrink(fiber);

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

Re: [tarantool-patches] [rfc 4/4] core/fiber: Shrink stack when recycling

[Cyrill Gorcunov]

On Tue, Mar 05, 2019 at 11:30:48AM +0300, Konstantin Osipov wrote:
> * Cyrill Gorcunov <gorcunov@gmail.com> [19/03/03 23:25]:
> > +#define wmark_freeze(_pp) do { *((uintptr_t *)(_pp)) |= (uintptr_t)1; } while (0)
> > +#define wmark_frozen(_p) ((uintptr_t)(_p) & (uintptr_t)1)
> 
> Why not static inline? A comment would help. Usually the name for
> function returning true/false contains "is", e.g. wmark_is_frozen().

Well, surely we can use inlines here, I simply tried to make less code.
Seriously fiber.c looks too bloating. Maybe we should move all this
machinery into fiber-stack.cc?

...
> > +
> > +	if (wmark_frozen(fiber->stack_wmark))
> > +		return;
> > +
> > +	/*
> > +	 * On recycle we're trying to shrink stack
> > +	 * as much as we can until first mark overwrite
> > +	 * detected, then we simply freeze watermark and
> > +	 * assume the stack is balanced and won't change
> > +	 * much in future.
> > +	 */
> > +	if (!stack_has_wmark(fiber->stack_wmark))
> > +		wmark_freeze(&fiber->stack_wmark);
> 
> I don't see how "frozen" watermark concept is more powerful than
> already existing "overflow warned" flag. I think the idea is to
> have some sort of smart threshold after which we stop trying to
> shrink the stack.
> 
> Right now this threshold is not too smart (which is OK), but
> complicated (both overflow_warned and wmark_frozen() are
> considered), which doesn't make too much sense to me.

Look, the idea is too keep _two_ tests: one when stack is close
to overflow, we do this test every recycle and never move the
mark to other place; this page with overflow mark will be
always present.

In turn overflow mark is shrinked every recycle until first rewrite
detected. Then we simply zap the pointer and never test it again.

If you mean something else, lets talk f2f?

Re: [tarantool-patches] Re: [rfc 4/4] core/fiber: Shrink stack when recycling

[Konstantin Osipov]

* Cyrill Gorcunov <gorcunov@gmail.com> [19/03/05 11:41]:
> On Tue, Mar 05, 2019 at 11:30:48AM +0300, Konstantin Osipov wrote:
> > * Cyrill Gorcunov <gorcunov@gmail.com> [19/03/03 23:25]:
> > > +#define wmark_freeze(_pp) do { *((uintptr_t *)(_pp)) |= (uintptr_t)1; } while (0)
> > > +#define wmark_frozen(_p) ((uintptr_t)(_p) & (uintptr_t)1)
> > 
> > Why not static inline? A comment would help. Usually the name for
> > function returning true/false contains "is", e.g. wmark_is_frozen().
> 
> Well, surely we can use inlines here, I simply tried to make less code.
> Seriously fiber.c looks too bloating. Maybe we should move all this
> machinery into fiber-stack.cc?

I am OK with moving things out (we prefer underscore to dash in
file names though), but I tend to count code lines in the count of
generated processor instructions, not characters/lines in a source
file.
> 

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov