From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <vdavydov.dev@gmail.com>
Date: Tue, 26 Feb 2019 13:32:23 +0300
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Subject: Re: [RFC v3] fiber: Increase default stack size
Message-ID: <20190226103223.c6aq7iqrw37o2blh@esperanza>
References: <20190222201639.GA7198@uranus>
 <20190225145516.6fdmob3tdkft5sky@esperanza>
 <20190225213955.GI7198@uranus>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190225213955.GI7198@uranus>
To: Cyrill Gorcunov <gorcunov@gmail.com>
Cc: =?utf-8?B?0JPQtdC+0YDQs9C40Lkg0JrQuNGA0LjRh9C10L3QutC+?= <georgy@tarantool.org>, tarantool-patches@freelists.org
List-ID: <tarantool-patches.dev.tarantool.org>

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