[tarantool-patches] [RFC v2] fiber: Increase default stack size

[tarantool-patches] [RFC v2] fiber: Increase default stack size

[Cyrill Gorcunov]

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.

To address this problem and hopefully eliminate such
problems in future we increase default size up to 1M.
Because this value may be too big for old distros or
other libraries, which would never use such deep stack,
we do a trick: put watermark at 64K offset of the stack
and once fiber get recycled we test if the mark is still
here. If we're lucky and noone touched the memory
we use madvise() syscall to reduce RSS usage.

v2: by georgy@
 - Move wmark test into recycle stage

As to stats for fiber's info I think it should be
addressed in a separate patch once we deal with
this issue.

https://github.com/tarantool/tarantool/issues/3418

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/fiber.c |  104 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 src/fiber.h |    6 +++
 2 files changed, 109 insertions(+), 1 deletion(-)

Index: tarantool.git/src/fiber.c
===================================================================
--- tarantool.git.orig/src/fiber.c
+++ tarantool.git/src/fiber.c
@@ -91,11 +91,26 @@ pthread_t main_thread_id;
 static size_t page_size;
 static int stack_direction;
 
+static void
+stack_recycle(struct fiber *fiber);
+
+/*
+ * A random unique value with help of uuidgen.
+ *
+ * 39ee5420-13f7-417b-9610-ea100c591ab6
+ */
+static const char stack_wmark[] = {
+	0x39, 0xee, 0x54, 0x20, 0x13, 0xf7, 0x41, 0x7b,
+	0x96, 0x10, 0xea, 0x10, 0x0c, 0x59, 0x1a, 0xb6
+};
+
 enum {
 	/* The minimum allowable fiber stack size in bytes */
 	FIBER_STACK_SIZE_MINIMAL = 16384,
+	/* Stack size for stack relaxed tasks */
+	FIBER_STACK_MADVISE_LIMIT = 64536,
 	/* Default fiber stack size in bytes */
-	FIBER_STACK_SIZE_DEFAULT = 65536
+	FIBER_STACK_SIZE_DEFAULT = 1048576
 };
 
 /** Default fiber attributes */
@@ -623,6 +638,7 @@ fiber_recycle(struct fiber *fiber)
 	assert(diag_is_empty(&fiber->diag));
 	/* no pending wakeup */
 	assert(rlist_empty(&fiber->state));
+	stack_recycle(fiber);
 	bool has_custom_stack = fiber->flags & FIBER_CUSTOM_STACK;
 	fiber_reset(fiber);
 	fiber->name[0] = '\0';
@@ -710,6 +726,91 @@ page_align_up(void *ptr)
 	return page_align_down(ptr + page_size - 1);
 }
 
+static inline void *
+stack_wmark_pos(struct fiber *fiber)
+{
+	void *pos;
+
+	assert(fiber->stack);
+	assert(fiber->stack_size);
+
+	if (stack_direction < 0) {
+		pos  = fiber->stack + fiber->stack_size;
+		pos -= FIBER_STACK_MADVISE_LIMIT;
+		return page_align_up(pos);
+	} else {
+		pos  = fiber->stack - fiber->stack_size;
+		pos += FIBER_STACK_MADVISE_LIMIT;
+		return page_align_down(pos);
+	}
+}
+
+/*
+ * Set watermark to the predefined place thus on
+ * fiber sched-out procedure we may detect if
+ * a task was too eager for stack usage.
+ */
+static inline void
+stack_set_wmark(struct fiber *fiber)
+{
+	void *pos = stack_wmark_pos(fiber);
+	memcpy(pos, stack_wmark, sizeof(stack_wmark));
+}
+
+static inline bool
+stack_has_wmark(struct fiber *fiber)
+{
+	void *pos = stack_wmark_pos(fiber);
+	return memcmp(pos, stack_wmark, sizeof(stack_wmark)) == 0;
+}
+
+static void
+stack_recycle(struct fiber *fiber)
+{
+	if (!fiber->stack || (fiber->flags & FIBER_CUSTOM_STACK))
+		return;
+
+	/*
+	 * If fiber was too eager for memory, just arm
+	 * a watermark back. Maybe on the next reuse
+	 * we will be able to relax RSS pressure.
+	 */
+	if (!stack_has_wmark(fiber)) {
+		fiber->flags &= ~FIBER_MADVISED_STACK;
+		stack_set_wmark(fiber);
+		return;
+	}
+
+	/*
+	 * This is a good one, we simply notify OS
+	 * about unused stack tail, so associated
+	 * pages would be put back into a page pool.
+	 *
+	 * Note though the fiber still can use
+	 * remaining space, simply won't be handled
+	 * that fast on _first_ #pf.
+	 */
+	if (!(fiber->flags & FIBER_MADVISED_STACK)) {
+		size_t size;
+		void *tail;
+
+		if (stack_direction < 0) {
+			tail = stack_wmark_pos(fiber) - page_size;
+			size = tail - fiber->stack;
+		} else {
+			tail = stack_wmark_pos(fiber) + page_size;
+			size = fiber->stack - tail;
+		}
+
+		/*
+		 * Set the flag iif we've successed,
+		 * otherwise will try on the next round.
+		 */
+		if (!madvise(fiber->stack, size, MADV_DONTNEED))
+			fiber->flags |= FIBER_MADVISED_STACK;
+	}
+}
+
 static int
 fiber_stack_create(struct fiber *fiber, size_t stack_size)
 {
@@ -751,6 +852,7 @@ fiber_stack_create(struct fiber *fiber,
 						  fiber->stack_size);
 
 	mprotect(guard, page_size, PROT_NONE);
+	stack_set_wmark(fiber);
 	return 0;
 }
 
Index: tarantool.git/src/fiber.h
===================================================================
--- tarantool.git.orig/src/fiber.h
+++ tarantool.git/src/fiber.h
@@ -89,6 +89,12 @@ enum {
 	 * This flag is set when fiber uses custom stack size.
 	 */
 	FIBER_CUSTOM_STACK	= 1 << 5,
+	/**
+	 * This flag is set when fiber didn't exceed stack's
+	 * size watermark and considered being small enough.
+	 */
+	FIBER_MADVISED_STACK	= 1 << 6,
+
 	FIBER_DEFAULT_FLAGS = FIBER_IS_CANCELLABLE
 };
 

Re: [tarantool-patches] [RFC v2] fiber: Increase default stack size

[Vladimir Davydov]

On Fri, Feb 22, 2019 at 11:16:40PM +0300, Cyrill Gorcunov wrote:
> 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.
> 
> To address this problem and hopefully eliminate such
> problems in future we increase default size up to 1M.
> Because this value may be too big for old distros or
> other libraries, which would never use such deep stack,
> we do a trick: put watermark at 64K offset of the stack
> and once fiber get recycled we test if the mark is still
> here. If we're lucky and noone touched the memory
> we use madvise() syscall to reduce RSS usage.
> 
> v2: by georgy@
>  - Move wmark test into recycle stage
> 
> As to stats for fiber's info I think it should be
> addressed in a separate patch once we deal with
> this issue.
> 
> https://github.com/tarantool/tarantool/issues/3418
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  src/fiber.c |  104 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  src/fiber.h |    6 +++
>  2 files changed, 109 insertions(+), 1 deletion(-)
> 
> Index: tarantool.git/src/fiber.c
> ===================================================================
> --- tarantool.git.orig/src/fiber.c
> +++ tarantool.git/src/fiber.c
> @@ -91,11 +91,26 @@ pthread_t main_thread_id;
>  static size_t page_size;
>  static int stack_direction;
>  
> +static void
> +stack_recycle(struct fiber *fiber);
> +
> +/*
> + * A random unique value with help of uuidgen.
> + *
> + * 39ee5420-13f7-417b-9610-ea100c591ab6
> + */
> +static const char stack_wmark[] = {
> +	0x39, 0xee, 0x54, 0x20, 0x13, 0xf7, 0x41, 0x7b,
> +	0x96, 0x10, 0xea, 0x10, 0x0c, 0x59, 0x1a, 0xb6
> +};
> +
>  enum {
>  	/* The minimum allowable fiber stack size in bytes */
>  	FIBER_STACK_SIZE_MINIMAL = 16384,
> +	/* Stack size for stack relaxed tasks */
> +	FIBER_STACK_MADVISE_LIMIT = 64536,
>  	/* Default fiber stack size in bytes */
> -	FIBER_STACK_SIZE_DEFAULT = 65536
> +	FIBER_STACK_SIZE_DEFAULT = 1048576
>  };
>  
>  /** Default fiber attributes */
> @@ -623,6 +638,7 @@ fiber_recycle(struct fiber *fiber)
>  	assert(diag_is_empty(&fiber->diag));
>  	/* no pending wakeup */
>  	assert(rlist_empty(&fiber->state));
> +	stack_recycle(fiber);
>  	bool has_custom_stack = fiber->flags & FIBER_CUSTOM_STACK;
>  	fiber_reset(fiber);
>  	fiber->name[0] = '\0';
> @@ -710,6 +726,91 @@ page_align_up(void *ptr)
>  	return page_align_down(ptr + page_size - 1);
>  }
>  
> +static inline void *
> +stack_wmark_pos(struct fiber *fiber)
> +{
> +	void *pos;
> +
> +	assert(fiber->stack);
> +	assert(fiber->stack_size);
> +
> +	if (stack_direction < 0) {
> +		pos  = fiber->stack + fiber->stack_size;
> +		pos -= FIBER_STACK_MADVISE_LIMIT;
> +		return page_align_up(pos);
> +	} else {
> +		pos  = fiber->stack - fiber->stack_size;
> +		pos += FIBER_STACK_MADVISE_LIMIT;
> +		return page_align_down(pos);
> +	}
> +}
> +
> +/*
> + * Set watermark to the predefined place thus on
> + * fiber sched-out procedure we may detect if
> + * a task was too eager for stack usage.
> + */
> +static inline void
> +stack_set_wmark(struct fiber *fiber)
> +{
> +	void *pos = stack_wmark_pos(fiber);
> +	memcpy(pos, stack_wmark, sizeof(stack_wmark));
> +}
> +
> +static inline bool
> +stack_has_wmark(struct fiber *fiber)
> +{
> +	void *pos = stack_wmark_pos(fiber);
> +	return memcmp(pos, stack_wmark, sizeof(stack_wmark)) == 0;
> +}
> +
> +static void
> +stack_recycle(struct fiber *fiber)
> +{
> +	if (!fiber->stack || (fiber->flags & FIBER_CUSTOM_STACK))
> +		return;
> +
> +	/*
> +	 * If fiber was too eager for memory, just arm
> +	 * a watermark back. Maybe on the next reuse
> +	 * we will be able to relax RSS pressure.
> +	 */
> +	if (!stack_has_wmark(fiber)) {
> +		fiber->flags &= ~FIBER_MADVISED_STACK;
> +		stack_set_wmark(fiber);
> +		return;
> +	}

Hmm, I don't quite understand why you free the stack only if the fiber
hasn't touched the watermark. See, there may be thousands of fibers out
there which are chosen randomly to execute a CALL request that needs a
lot of stack. If this CALL request happens to land on different fibers
all the time, we will quickly wind up with a lot of memory being used
for fiber stacks.

That being said, I think we should unconditionally free the stack with
madvise() on fiber_recycle() if the watermark was overwritten. This
would also simplify the patch as you won't need to introduce a new fiber
flag then (FIBER_MADVISED_STACK).

Am I missing something?

> +
> +	/*
> +	 * This is a good one, we simply notify OS
> +	 * about unused stack tail, so associated
> +	 * pages would be put back into a page pool.
> +	 *
> +	 * Note though the fiber still can use
> +	 * remaining space, simply won't be handled
> +	 * that fast on _first_ #pf.
> +	 */
> +	if (!(fiber->flags & FIBER_MADVISED_STACK)) {
> +		size_t size;
> +		void *tail;
> +
> +		if (stack_direction < 0) {
> +			tail = stack_wmark_pos(fiber) - page_size;
> +			size = tail - fiber->stack;
> +		} else {
> +			tail = stack_wmark_pos(fiber) + page_size;
> +			size = fiber->stack - tail;
> +		}
> +
> +		/*
> +		 * Set the flag iif we've successed,
> +		 * otherwise will try on the next round.
> +		 */
> +		if (!madvise(fiber->stack, size, MADV_DONTNEED))

This is nitpicking, but in tarantool we use logical negation (!) only
with bools. We compare integer values with 0 explicitly.

> +			fiber->flags |= FIBER_MADVISED_STACK;
> +	}
> +}

Re: [tarantool-patches] [RFC v2] fiber: Increase default stack size

[Cyrill Gorcunov]

On Mon, Feb 25, 2019 at 05:55:16PM +0300, Vladimir Davydov wrote:
...
> > +
> > +static void
> > +stack_recycle(struct fiber *fiber)
> > +{
> > +	if (!fiber->stack || (fiber->flags & FIBER_CUSTOM_STACK))
> > +		return;
> > +
> > +	/*
> > +	 * If fiber was too eager for memory, just arm
> > +	 * a watermark back. Maybe on the next reuse
> > +	 * we will be able to relax RSS pressure.
> > +	 */
> > +	if (!stack_has_wmark(fiber)) {
> > +		fiber->flags &= ~FIBER_MADVISED_STACK;
> > +		stack_set_wmark(fiber);
> > +		return;
> > +	}
> 
> Hmm, I don't quite understand why you free the stack only if the fiber
> hasn't touched the watermark. See, there may be thousands of fibers out
> there which are chosen randomly to execute a CALL request that needs a
> lot of stack. If this CALL request happens to land on different fibers
> all the time, we will quickly wind up with a lot of memory being used
> for fiber stacks.
> 
> That being said, I think we should unconditionally free the stack with
> madvise() on fiber_recycle() if the watermark was overwritten. This
> would also simplify the patch as you won't need to introduce a new fiber
> flag then (FIBER_MADVISED_STACK).
> 
> Am I missing something?

Nope, you're not. I'm trying to minimize madvise() call since it is
a quite expencive call. And I don't like the idea of calling it
unconditionally (seriously, pte range invalidation is not cheap
at all, which involves tlb flushes and such). Letme think more...

> > +
> > +		/*
> > +		 * Set the flag iif we've successed,
> > +		 * otherwise will try on the next round.
> > +		 */
> > +		if (!madvise(fiber->stack, size, MADV_DONTNEED))
> 
> This is nitpicking, but in tarantool we use logical negation (!) only
> with bools. We compare integer values with 0 explicitly.

sure, will update.

[RFC v3] fiber: Increase default stack size

[Cyrill Gorcunov]

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.

To address this problem and hopefully eliminate such
problems in future we increase default size up to 1M.
Because this value may be too big for old distros or
other libraries, which would never use such deep stack,
we do a trick: put watermark at 64K offset of the stack
and once fiber get recycled we try to relax memory
pressue with madvise syscall.

https://github.com/tarantool/tarantool/issues/3418
---
Vladimir, take a look please. That is what you mean?
I'm not yet familiar with slab engine, does it allocates
pages on lazy fashion or we need to pass 'dontneed' on
first fiber creation too?

And please re-check stack/mark position calculus once again,
brain is off already I might miss something obvious.

Also should not we give user a way to configure this early
params, maybe via getenv?

 src/fiber.c |   87 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 86 insertions(+), 1 deletion(-)

Index: tarantool.git/src/fiber.c
===================================================================
--- tarantool.git.orig/src/fiber.c
+++ tarantool.git/src/fiber.c
@@ -91,11 +91,26 @@ pthread_t main_thread_id;
 static size_t page_size;
 static int stack_direction;
 
+static void
+stack_recycle(struct fiber *fiber);
+
+/*
+ * A random unique value with help of uuidgen.
+ *
+ * 39ee5420-13f7-417b-9610-ea100c591ab6
+ */
+static const char stack_wmark[] = {
+	0x39, 0xee, 0x54, 0x20, 0x13, 0xf7, 0x41, 0x7b,
+	0x96, 0x10, 0xea, 0x10, 0x0c, 0x59, 0x1a, 0xb6
+};
+
 enum {
 	/* The minimum allowable fiber stack size in bytes */
 	FIBER_STACK_SIZE_MINIMAL = 16384,
+	/* Stack size for stack relaxed tasks */
+	FIBER_STACK_MIDDLE_LIMIT = 65536,
 	/* Default fiber stack size in bytes */
-	FIBER_STACK_SIZE_DEFAULT = 65536
+	FIBER_STACK_SIZE_DEFAULT = 1048576
 };
 
 /** Default fiber attributes */
@@ -623,6 +638,7 @@ fiber_recycle(struct fiber *fiber)
 	assert(diag_is_empty(&fiber->diag));
 	/* no pending wakeup */
 	assert(rlist_empty(&fiber->state));
+	stack_recycle(fiber);
 	bool has_custom_stack = fiber->flags & FIBER_CUSTOM_STACK;
 	fiber_reset(fiber);
 	fiber->name[0] = '\0';
@@ -710,6 +726,74 @@ page_align_up(void *ptr)
 	return page_align_down(ptr + page_size - 1);
 }
 
+static inline void *
+stack_wmark_pos(struct fiber *fiber)
+{
+	void *pos;
+
+	assert(fiber->stack);
+	assert(fiber->stack_size);
+
+	if (stack_direction < 0) {
+		pos  = fiber->stack + fiber->stack_size;
+		pos -= FIBER_STACK_MIDDLE_LIMIT;
+		return page_align_up(pos);
+	} else {
+		pos  = fiber->stack - fiber->stack_size;
+		pos += FIBER_STACK_MIDDLE_LIMIT;
+		return page_align_down(pos);
+	}
+}
+
+/*
+ * Set watermark to the predefined place thus on
+ * fiber sched-out procedure we may detect if
+ * a task was too eager for stack usage.
+ */
+static inline void
+stack_set_wmark(struct fiber *fiber)
+{
+	void *pos = stack_wmark_pos(fiber);
+	memcpy(pos, stack_wmark, sizeof(stack_wmark));
+}
+
+static inline bool
+stack_has_wmark(struct fiber *fiber)
+{
+	void *pos = stack_wmark_pos(fiber);
+	return memcmp(pos, stack_wmark, sizeof(stack_wmark)) == 0;
+}
+
+static void
+stack_recycle(struct fiber *fiber)
+{
+	if (!fiber->stack || (fiber->flags & FIBER_CUSTOM_STACK))
+		return;
+
+	/*
+	 * If a fiber was too eager for memory, just arm
+	 * a watermark back and mark the rest as unneeded.
+	 * This will hint OS to release the memory but if
+	 * a fiber ask for it again it will be shipped on
+	 * back on demand.
+	 */
+	if (!stack_has_wmark(fiber)) {
+		size_t size;
+		void *tail;
+
+		stack_set_wmark(fiber);
+
+		if (stack_direction < 0) {
+			tail = stack_wmark_pos(fiber) - page_size;
+			size = tail - fiber->stack;
+		} else {
+			tail = stack_wmark_pos(fiber) + page_size;
+			size = fiber->stack - tail;
+		}
+		madvise(fiber->stack, size, MADV_DONTNEED);
+	}
+}
+
 static int
 fiber_stack_create(struct fiber *fiber, size_t stack_size)
 {
@@ -751,6 +835,7 @@ fiber_stack_create(struct fiber *fiber,
 						  fiber->stack_size);
 
 	mprotect(guard, page_size, PROT_NONE);
+	stack_set_wmark(fiber);
 	return 0;
 }
 

Re: [RFC v3] fiber: Increase default stack size

[Vladimir Davydov]

On Tue, Feb 26, 2019 at 12:39:55AM +0300, Cyrill Gorcunov wrote:
> 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.
> 
> To address this problem and hopefully eliminate such
> problems in future we increase default size up to 1M.
> Because this value may be too big for old distros or
> other libraries, which would never use such deep stack,
> we do a trick: put watermark at 64K offset of the stack
> and once fiber get recycled we try to relax memory
> pressue with madvise syscall.
> 
> https://github.com/tarantool/tarantool/issues/3418

Forgot to tell you during the previous review round: we don't put a full
link in the commit message. Instead we write:

Closes #3418

We put the link after the diff separator (---) so as a reviewer can
easily open it.

> ---
> Vladimir, take a look please. That is what you mean?

Yes.

> I'm not yet familiar with slab engine, does it allocates
> pages on lazy fashion or we need to pass 'dontneed' on
> first fiber creation too?

Oops, you're right, good catch! The allocator may poison slab if NDEBUG
is unset. So we can either

 - Madvise slab on fiber creation, at least in NDEBUG mode. Simple, but
   depends on the allocator internals.
 - Patch the 'small' library to make the allocator do madvise for us.
   IMO it would look better, but would clutter the allocator API.
 - Don't use 'small' allocator at all for default slab allocations, and
   simply mmap stack and link them in a free list (is it OK to mmap a
   few MB chunk per each fiber?).

I'm inclined to choose the last option. I'll discuss the options with
others today and follow-up.

> 
> And please re-check stack/mark position calculus once again,
> brain is off already I might miss something obvious.

I will, sure.

> 
> Also should not we give user a way to configure this early
> params, maybe via getenv?

May be, but this can definitely be done later if we really need it.

Re: [RFC v3] fiber: Increase default stack size

[Cyrill Gorcunov]

On Tue, Feb 26, 2019 at 11:58:52AM +0300, Vladimir Davydov wrote:
...
> > 
> > https://github.com/tarantool/tarantool/issues/3418
> 
> Forgot to tell you during the previous review round: we don't put a full
> link in the commit message. Instead we write:
> 
> Closes #3418

OK

> 
> We put the link after the diff separator (---) so as a reviewer can
> easily open it.
> 
> > ---
> > Vladimir, take a look please. That is what you mean?
> 
> Yes.
> 
> > I'm not yet familiar with slab engine, does it allocates
> > pages on lazy fashion or we need to pass 'dontneed' on
> > first fiber creation too?
> 
> Oops, you're right, good catch! The allocator may poison slab if NDEBUG
> is unset. So we can either
> 
>  - Madvise slab on fiber creation, at least in NDEBUG mode. Simple, but
>    depends on the allocator internals.
>  - Patch the 'small' library to make the allocator do madvise for us.
>    IMO it would look better, but would clutter the allocator API.
>  - Don't use 'small' allocator at all for default slab allocations, and
>    simply mmap stack and link them in a free list (is it OK to mmap a
>    few MB chunk per each fiber?).
> 
> I'm inclined to choose the last option. I'll discuss the options with
> others today and follow-up.

Will continue at the evening. Also note that stack is special and
we better should not pass arbitrary sizes to allocate even for
custom stacks, they all _must_ be PAGE_SIZE aligned (or call it
PAGE_SIZE orders if you prefer :) Anything else make madvise
idea useless since kernel requires madvise args to be page
aligned which is understandable due to hardware.

> > 
> > Also should not we give user a way to configure this early
> > params, maybe via getenv?
> 
> May be, but this can definitely be done later if we really need it.
OK

Re: [RFC v3] fiber: Increase default stack size

[Vladimir Davydov]

On Tue, Feb 26, 2019 at 12:12:54PM +0300, Cyrill Gorcunov wrote:
> > > I'm not yet familiar with slab engine, does it allocates
> > > pages on lazy fashion or we need to pass 'dontneed' on
> > > first fiber creation too?
> > 
> > Oops, you're right, good catch! The allocator may poison slab if NDEBUG
> > is unset. So we can either
> > 
> >  - Madvise slab on fiber creation, at least in NDEBUG mode. Simple, but
> >    depends on the allocator internals.
> >  - Patch the 'small' library to make the allocator do madvise for us.
> >    IMO it would look better, but would clutter the allocator API.
> >  - Don't use 'small' allocator at all for default slab allocations, and
> >    simply mmap stack and link them in a free list (is it OK to mmap a
> >    few MB chunk per each fiber?).
> > 
> > I'm inclined to choose the last option. I'll discuss the options with
> > others today and follow-up.
> 
> Will continue at the evening. Also note that stack is special and
> we better should not pass arbitrary sizes to allocate even for
> custom stacks, they all _must_ be PAGE_SIZE aligned (or call it
> PAGE_SIZE orders if you prefer :) Anything else make madvise
> idea useless since kernel requires madvise args to be page
> aligned which is understandable due to hardware.

Talked to Kostja and Georgy. We agreed on the following points:

 - Regarding slab poisoning. We don't want to implement ad hoc allocator
   for fiber stacks, neither do we really want to patch the small lib
   for now. Let's use madvise(MADV_DONTNEED) for all fiber stacks
   unconditionally on fiber creation. This should be okay from
   performance point of view, because once a fiber is created, it's
   never destroyed - it stays on the dead list until recycled.

 - 1 MB for max stack size seems to be a bit of an overkill for now.
   The default value should be set to 256 KB, but we do need a
   configuration option for it. Let's add it to the fiber Lua module.
   May be done in a separate patch, but should be submitted together in
   the same patch set.

 - 16 byte unique identifier for detecting stack overflow doesn't seem
   to be enough. Imagine a PATH_MAX buffer allocated on stack that uses
   only a hundred bytes for path formatting. It can easily jump over the
   watermark. We should probably use random poisoning: say, 4 unique
   identifiers 8 bytes each scattered a few hundred bytes apart. Some
   math/reasoning behind this would be nice to see in the comments.

 - Since madvise() is somewhat expensive to be called on each fiber
   recycle, we need to make the watermark dynamic. That is, keep track
   of the number of fibers that have exceeded the watermark and when
   there are too many of those, increase the watermark value. We could
   probably use a histogram for this. This would allow us to decrease
   the default stack allocation size from 64 KB down to 16 KB, which
   would be really nice. This should be done in a separate patch, but
   again in the scope of this issue.

 - We definitely need this patch (or patches) to be covered with unit
   tests. Please add corresponding cases to test/unit/fiber.cc.

Re: [RFC v3] fiber: Increase default stack size

[Vladimir Davydov]

On Tue, Feb 26, 2019 at 12:39:55AM +0300, Cyrill Gorcunov wrote:
> --- tarantool.git.orig/src/fiber.c
> +++ tarantool.git/src/fiber.c
> @@ -91,11 +91,26 @@ pthread_t main_thread_id;
>  static size_t page_size;
>  static int stack_direction;
>  
> +static void
> +stack_recycle(struct fiber *fiber);
> +
> +/*
> + * A random unique value with help of uuidgen.
> + *
> + * 39ee5420-13f7-417b-9610-ea100c591ab6
> + */
> +static const char stack_wmark[] = {
> +	0x39, 0xee, 0x54, 0x20, 0x13, 0xf7, 0x41, 0x7b,
> +	0x96, 0x10, 0xea, 0x10, 0x0c, 0x59, 0x1a, 0xb6
> +};
> +
>  enum {
>  	/* The minimum allowable fiber stack size in bytes */
>  	FIBER_STACK_SIZE_MINIMAL = 16384,
> +	/* Stack size for stack relaxed tasks */
> +	FIBER_STACK_MIDDLE_LIMIT = 65536,

I don't quite like the name. May be, FIBER_STACK_SIZE_PREALLOC would be
better? Or we could even leave FIBER_STACK_SIZE_DEFAULT.

>  	/* Default fiber stack size in bytes */
> -	FIBER_STACK_SIZE_DEFAULT = 65536
> +	FIBER_STACK_SIZE_DEFAULT = 1048576
>  };
>  
>  /** Default fiber attributes */
> @@ -623,6 +638,7 @@ fiber_recycle(struct fiber *fiber)
>  	assert(diag_is_empty(&fiber->diag));
>  	/* no pending wakeup */
>  	assert(rlist_empty(&fiber->state));
> +	stack_recycle(fiber);

Nit: I'd move this code under 'if (!has_custom_stack)' branch below.

>  	bool has_custom_stack = fiber->flags & FIBER_CUSTOM_STACK;
>  	fiber_reset(fiber);
>  	fiber->name[0] = '\0';
> @@ -710,6 +726,74 @@ page_align_up(void *ptr)
>  	return page_align_down(ptr + page_size - 1);
>  }
>  
> +static inline void *
> +stack_wmark_pos(struct fiber *fiber)
> +{
> +	void *pos;
> +
> +	assert(fiber->stack);
> +	assert(fiber->stack_size);
> +
> +	if (stack_direction < 0) {
> +		pos  = fiber->stack + fiber->stack_size;
> +		pos -= FIBER_STACK_MIDDLE_LIMIT;
> +		return page_align_up(pos);
> +	} else {
> +		pos  = fiber->stack - fiber->stack_size;
> +		pos += FIBER_STACK_MIDDLE_LIMIT;
> +		return page_align_down(pos);
> +	}
> +}
> +
> +/*
> + * Set watermark to the predefined place thus on
> + * fiber sched-out procedure we may detect if
> + * a task was too eager for stack usage.
> + */
> +static inline void
> +stack_set_wmark(struct fiber *fiber)
> +{
> +	void *pos = stack_wmark_pos(fiber);
> +	memcpy(pos, stack_wmark, sizeof(stack_wmark));
> +}
> +
> +static inline bool
> +stack_has_wmark(struct fiber *fiber)
> +{
> +	void *pos = stack_wmark_pos(fiber);
> +	return memcmp(pos, stack_wmark, sizeof(stack_wmark)) == 0;
> +}
> +
> +static void
> +stack_recycle(struct fiber *fiber)
> +{
> +	if (!fiber->stack || (fiber->flags & FIBER_CUSTOM_STACK))
> +		return;
> +
> +	/*
> +	 * If a fiber was too eager for memory, just arm
> +	 * a watermark back and mark the rest as unneeded.
> +	 * This will hint OS to release the memory but if
> +	 * a fiber ask for it again it will be shipped on
> +	 * back on demand.
> +	 */
> +	if (!stack_has_wmark(fiber)) {
> +		size_t size;
> +		void *tail;
> +
> +		stack_set_wmark(fiber);
> +
> +		if (stack_direction < 0) {
> +			tail = stack_wmark_pos(fiber) - page_size;
> +			size = tail - fiber->stack;
> +		} else {
> +			tail = stack_wmark_pos(fiber) + page_size;
> +			size = fiber->stack - tail;
> +		}
> +		madvise(fiber->stack, size, MADV_DONTNEED);

madvise(MADV_DONTNEED) isn't available on OS X. Please ifdef this code
accordingly. You'll need to add a check_symbol_exists to CMakeLists.txt
for this.

> +	}
> +}
> +
>  static int
>  fiber_stack_create(struct fiber *fiber, size_t stack_size)
>  {
> @@ -751,6 +835,7 @@ fiber_stack_create(struct fiber *fiber,
>  						  fiber->stack_size);
>  
>  	mprotect(guard, page_size, PROT_NONE);
> +	stack_set_wmark(fiber);
>  	return 0;
>  }
>  

Re: [RFC v3] fiber: Increase default stack size

[Vladimir Davydov]

On Tue, Feb 26, 2019 at 01:26:56PM +0300, Vladimir Davydov wrote:
>  - 1 MB for max stack size seems to be a bit of an overkill for now.
>    The default value should be set to 256 KB, but we do need a
>    configuration option for it. Let's add it to the fiber Lua module.
>    May be done in a separate patch, but should be submitted together in
>    the same patch set.

Almost forgot. We should also print a warning to the log if some fiber
has gone too close to the stack limit (say half of the max allocation)
to indicate that the max stack size should be increased.

Re: [RFC v3] fiber: Increase default stack size

[Cyrill Gorcunov]

On Tue, Feb 26, 2019 at 01:26:56PM +0300, Vladimir Davydov wrote:
...
> 
> Talked to Kostja and Georgy. We agreed on the following points:
> 
>  - Regarding slab poisoning. We don't want to implement ad hoc allocator
>    for fiber stacks, neither do we really want to patch the small lib
>    for now. Let's use madvise(MADV_DONTNEED) for all fiber stacks
>    unconditionally on fiber creation. This should be okay from
>    performance point of view, because once a fiber is created, it's
>    never destroyed - it stays on the dead list until recycled.

OK

>  - 1 MB for max stack size seems to be a bit of an overkill for now.
>    The default value should be set to 256 KB, but we do need a
>    configuration option for it. Let's add it to the fiber Lua module.
>    May be done in a separate patch, but should be submitted together in
>    the same patch set.

Wait, first fiber for main cord is created before lua init, isn't it?
I already though about using lua config for it but fiber init'ed at
very early stage.

>  - 16 byte unique identifier for detecting stack overflow doesn't seem
>    to be enough. Imagine a PATH_MAX buffer allocated on stack that uses
>    only a hundred bytes for path formatting. It can easily jump over the
>    watermark. We should probably use random poisoning: say, 4 unique
>    identifiers 8 bytes each scattered a few hundred bytes apart. Some
>    math/reasoning behind this would be nice to see in the comments.

If we want to scatter we should simply put marks at page bounds.
Dirtifying somewhere inside middle of a page is useless.

>  - Since madvise() is somewhat expensive to be called on each fiber
>    recycle, we need to make the watermark dynamic. That is, keep track
>    of the number of fibers that have exceeded the watermark and when
>    there are too many of those, increase the watermark value. We could
>    probably use a histogram for this. This would allow us to decrease
>    the default stack allocation size from 64 KB down to 16 KB, which
>    would be really nice. This should be done in a separate patch, but
>    again in the scope of this issue.

Nod.

> 
>  - We definitely need this patch (or patches) to be covered with unit
>    tests. Please add corresponding cases to test/unit/fiber.cc.

Yes, need to address this too. Thanks a huge for comments!

	Cyrill

Re: [RFC v3] fiber: Increase default stack size

[Cyrill Gorcunov]

On Tue, Feb 26, 2019 at 01:36:59PM +0300, Vladimir Davydov wrote:
> On Tue, Feb 26, 2019 at 01:26:56PM +0300, Vladimir Davydov wrote:
> >  - 1 MB for max stack size seems to be a bit of an overkill for now.
> >    The default value should be set to 256 KB, but we do need a
> >    configuration option for it. Let's add it to the fiber Lua module.
> >    May be done in a separate patch, but should be submitted together in
> >    the same patch set.
> 
> Almost forgot. We should also print a warning to the log if some fiber
> has gone too close to the stack limit (say half of the max allocation)
> to indicate that the max stack size should be increased.

Do we have something similar to printf_ratelimited() so we wouldn't
spam a user too much?

Re: [RFC v3] fiber: Increase default stack size

[Cyrill Gorcunov]

On Tue, Feb 26, 2019 at 01:32:23PM +0300, Vladimir Davydov wrote:
> >  	FIBER_STACK_SIZE_MINIMAL = 16384,
> > +	/* Stack size for stack relaxed tasks */
> > +	FIBER_STACK_MIDDLE_LIMIT = 65536,
> 
> I don't quite like the name. May be, FIBER_STACK_SIZE_PREALLOC
> would be better? Or we could even leave FIBER_STACK_SIZE_DEFAULT.

Sure. And thanks for the rest of comments.

Re: [RFC v3] fiber: Increase default stack size

[Vladimir Davydov]

On Tue, Feb 26, 2019 at 02:17:23PM +0300, Cyrill Gorcunov wrote:
> On Tue, Feb 26, 2019 at 01:36:59PM +0300, Vladimir Davydov wrote:
> > On Tue, Feb 26, 2019 at 01:26:56PM +0300, Vladimir Davydov wrote:
> > >  - 1 MB for max stack size seems to be a bit of an overkill for now.
> > >    The default value should be set to 256 KB, but we do need a
> > >    configuration option for it. Let's add it to the fiber Lua module.
> > >    May be done in a separate patch, but should be submitted together in
> > >    the same patch set.
> > 
> > Almost forgot. We should also print a warning to the log if some fiber
> > has gone too close to the stack limit (say half of the max allocation)
> > to indicate that the max stack size should be increased.
> 
> Do we have something similar to printf_ratelimited() so we wouldn't
> spam a user too much?

Yes, we do, see say_ratelimited :-)

But I don't think we need it here. We just want to print it once when
the limit is exceeded. Not on every breach.

Re: [RFC v3] fiber: Increase default stack size

[Vladimir Davydov]

On Tue, Feb 26, 2019 at 02:16:32PM +0300, Cyrill Gorcunov wrote:
> >  - 1 MB for max stack size seems to be a bit of an overkill for now.
> >    The default value should be set to 256 KB, but we do need a
> >    configuration option for it. Let's add it to the fiber Lua module.
> >    May be done in a separate patch, but should be submitted together in
> >    the same patch set.
> 
> Wait, first fiber for main cord is created before lua init, isn't it?
> I already though about using lua config for it but fiber init'ed at
> very early stage.

Yeah, good point, I haven't thought about it. Then we have to either
always allocate a big stack for the main fiber or use an environment
variable. Let's get back to it later and first make the stack size limit
statically defined.

> 
> >  - 16 byte unique identifier for detecting stack overflow doesn't seem
> >    to be enough. Imagine a PATH_MAX buffer allocated on stack that uses
> >    only a hundred bytes for path formatting. It can easily jump over the
> >    watermark. We should probably use random poisoning: say, 4 unique
> >    identifiers 8 bytes each scattered a few hundred bytes apart. Some
> >    math/reasoning behind this would be nice to see in the comments.
> 
> If we want to scatter we should simply put marks at page bounds.
> Dirtifying somewhere inside middle of a page is useless.

Hmm, why? Consider the example with PATH_MAX buffer. Putting dirty marks
at page boundaries doesn't guarantee any of them will get overwritten by
the buffer if only a few hundred of bytes are used. I think we should
dirty the last page or two at random intervals - this should increase
the chance that at least one mark is overwritten by any function that is
eager for the stack.

Re: [RFC v3] fiber: Increase default stack size

[Cyrill Gorcunov]

On Tue, Feb 26, 2019 at 03:34:56PM +0300, Vladimir Davydov wrote:
> > 
> > If we want to scatter we should simply put marks at page bounds.
> > Dirtifying somewhere inside middle of a page is useless.
> 
> Hmm, why? Consider the example with PATH_MAX buffer. Putting dirty marks
> at page boundaries doesn't guarantee any of them will get overwritten by
> the buffer if only a few hundred of bytes are used. I think we should
> dirty the last page or two at random intervals - this should increase
> the chance that at least one mark is overwritten by any function that is
> eager for the stack.

Ah, I got what you mean. Look, maybe we should consider pagemap too?
iirc readin pagemap should be cheap but need to read the code though,
will try once time permit.

Re: [RFC v3] fiber: Increase default stack size

[Vladimir Davydov]

On Tue, Feb 26, 2019 at 03:54:57PM +0300, Cyrill Gorcunov wrote:
> On Tue, Feb 26, 2019 at 03:34:56PM +0300, Vladimir Davydov wrote:
> > > 
> > > If we want to scatter we should simply put marks at page bounds.
> > > Dirtifying somewhere inside middle of a page is useless.
> > 
> > Hmm, why? Consider the example with PATH_MAX buffer. Putting dirty marks
> > at page boundaries doesn't guarantee any of them will get overwritten by
> > the buffer if only a few hundred of bytes are used. I think we should
> > dirty the last page or two at random intervals - this should increase
> > the chance that at least one mark is overwritten by any function that is
> > eager for the stack.
> 
> Ah, I got what you mean. Look, maybe we should consider pagemap too?
> iirc readin pagemap should be cheap but need to read the code though,
> will try once time permit.

We are paranoid about any syscalls so no, I don't think you'll be able
to sell it. I think we'd better stick to poisoning.

[tarantool-patches] Re: [RFC v3] fiber: Increase default stack size

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/02/26 15:37]:
> 
> Hmm, why? Consider the example with PATH_MAX buffer. Putting dirty marks
> at page boundaries doesn't guarantee any of them will get overwritten by
> the buffer if only a few hundred of bytes are used. I think we should
> dirty the last page or two at random intervals - this should increase
> the chance that at least one mark is overwritten by any function that is
> eager for the stack.

More importantly, since stack usage/layout is usually the same in
all fibers, i.e. it depends only on particular invocation chain,
and random markers could be set at different locations at
different fibers we are almost guaranteed to spot the problem 
sooner or later, moreover, more likely sooner than later.

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

[tarantool-patches] Re: [RFC v3] fiber: Increase default stack size

[Cyrill Gorcunov]

On Tue, Feb 26, 2019 at 04:26:31PM +0300, Konstantin Osipov wrote:
> 
> More importantly, since stack usage/layout is usually the same in
> all fibers, i.e. it depends only on particular invocation chain,
> and random markers could be set at different locations at
> different fibers we are almost guaranteed to spot the problem 
> sooner or later, moreover, more likely sooner than later.

Thanks! We will see once I manage with this problem.