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

[Vladimir Davydov <vdavydov.dev@gmail.com>]

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;
> +	}
> +}