[RFC 0/2] lib/core/fiber: Allow to extend stack size via env variable

[RFC 0/2] lib/core/fiber: Allow to extend stack size via env variable

[Cyrill Gorcunov]

Guys, take a look please. I didn't write any test yet since it is rfc,
but ran custom stack size manually.

Cyrill Gorcunov (2):
  lib/core/fiber: Rename stack_direction to stack_growsdown
  lib/core/fiber: Allow to extend default stack size

 src/lib/core/fiber.c | 94 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 76 insertions(+), 18 deletions(-)

-- 
2.20.1

[PATCH 1/2] lib/core/fiber: Rename stack_direction to stack_growsdown

[Cyrill Gorcunov]

Since growsdown (or growsup) is more clear and common name.
---
 src/lib/core/fiber.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
index c55b3ab39..922a0bfe8 100644
--- a/src/lib/core/fiber.c
+++ b/src/lib/core/fiber.c
@@ -105,7 +105,7 @@ __thread struct cord *cord_ptr = NULL;
 pthread_t main_thread_id;
 
 static size_t page_size;
-static int stack_direction;
+static bool stack_growsdown;
 
 enum {
 	/* The minimum allowable fiber stack size in bytes */
@@ -808,7 +808,7 @@ fiber_stack_recycle(struct fiber *fiber)
 	 * it anyway.
 	 */
 	void *start, *end;
-	if (stack_direction < 0) {
+	if (stack_growsdown) {
 		start = fiber->stack;
 		end = page_align_down(fiber->stack_watermark);
 	} else {
@@ -842,7 +842,7 @@ fiber_stack_watermark_create(struct fiber *fiber)
 	 * we put the first mark at a random position.
 	 */
 	size_t offset = rand() % POISON_OFF * sizeof(poison_pool[0]);
-	if (stack_direction < 0) {
+	if (stack_growsdown) {
 		fiber->stack_watermark  = fiber->stack + fiber->stack_size;
 		fiber->stack_watermark -= FIBER_STACK_SIZE_WATERMARK;
 		fiber->stack_watermark += offset;
@@ -881,7 +881,7 @@ fiber_stack_create(struct fiber *fiber, size_t stack_size)
 	}
 	void *guard;
 	/* Adjust begin and size for stack memory chunk. */
-	if (stack_direction < 0) {
+	if (stack_growsdown) {
 		/*
 		 * A stack grows down. First page after begin of a
 		 * stack memory chunk should be protected and memory
@@ -922,7 +922,7 @@ fiber_stack_destroy(struct fiber *fiber, struct slab_cache *slabc)
 		ASAN_UNPOISON_MEMORY_REGION(fiber->stack, fiber->stack_size);
 #endif
 		void *guard;
-		if (stack_direction < 0)
+		if (stack_growsdown)
 			guard = page_align_down(fiber->stack - page_size);
 		else
 			guard = page_align_up(fiber->stack + fiber->stack_size);
@@ -1384,16 +1384,16 @@ cord_slab_cache(void)
 }
 
 static NOINLINE int
-check_stack_direction(void *prev_stack_frame)
+is_stack_growsdown(void *prev_stack_frame)
 {
-	return __builtin_frame_address(0) < prev_stack_frame ? -1: 1;
+	return __builtin_frame_address(0) < prev_stack_frame ? true : false;
 }
 
 void
 fiber_init(int (*invoke)(fiber_func f, va_list ap))
 {
 	page_size = sysconf(_SC_PAGESIZE);
-	stack_direction = check_stack_direction(__builtin_frame_address(0));
+	stack_growsdown = is_stack_growsdown(__builtin_frame_address(0));
 	fiber_invoke = invoke;
 	main_thread_id = pthread_self();
 	main_cord.loop = ev_default_loop(EVFLAG_AUTO | EVFLAG_ALLOCFD);
-- 
2.20.1

[PATCH 2/2] lib/core/fiber: Allow to extend default stack size

[Cyrill Gorcunov]

Recently we found that 64K stack we've been using
for years is not longer enough due to third party
libraries, so we extended its size.

Still there is no guarantee that even new size will
lasts forever and users might face same problem again,
waiting for new build where stack capacity increased.

Instead of this ping-pong game lets allow a user to
setup default stack size via FIBER_STACK_SIZE_DEFAULT
environment variable.

Note we fetch this value once on program startup and
because it is early init stage we can't use box
config engine.
---
 src/lib/core/fiber.c | 78 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 68 insertions(+), 10 deletions(-)

diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
index 922a0bfe8..d3cb18a15 100644
--- a/src/lib/core/fiber.c
+++ b/src/lib/core/fiber.c
@@ -107,18 +107,25 @@ pthread_t main_thread_id;
 static size_t page_size;
 static bool stack_growsdown;
 
+/** Indices in fiber stack sizes (FSS) */
 enum {
+	FSS_IDX_MINIMAL,
+	FSS_IDX_WATERMARK,
+	FSS_IDX_DEFAULT,
+};
+
+/** Fiber stack sizes */
+static size_t fiber_stack_size[] = {
 	/* The minimum allowable fiber stack size in bytes */
-	FIBER_STACK_SIZE_MINIMAL = 16384,
+	[FSS_IDX_MINIMAL]	= 16384,
+	/* Stack size watermark in bytes */
+	[FSS_IDX_WATERMARK]	= 65536,
 	/* Default fiber stack size in bytes */
-	FIBER_STACK_SIZE_DEFAULT = 524288,
-	/* Stack size watermark in bytes. */
-	FIBER_STACK_SIZE_WATERMARK = 65536,
+	[FSS_IDX_DEFAULT]	= 524288,
 };
 
 /** Default fiber attributes */
-static const struct fiber_attr fiber_attr_default = {
-       .stack_size = FIBER_STACK_SIZE_DEFAULT,
+static struct fiber_attr fiber_attr_default = {
        .flags = FIBER_DEFAULT_FLAGS
 };
 
@@ -172,13 +179,13 @@ fiber_attr_delete(struct fiber_attr *fiber_attr)
 int
 fiber_attr_setstacksize(struct fiber_attr *fiber_attr, size_t stack_size)
 {
-	if (stack_size < FIBER_STACK_SIZE_MINIMAL) {
+	if (stack_size < fiber_stack_size[FSS_IDX_MINIMAL]) {
 		errno = EINVAL;
 		diag_set(SystemError, "stack size is too small");
 		return -1;
 	}
 	fiber_attr->stack_size = stack_size;
-	if (stack_size != FIBER_STACK_SIZE_DEFAULT) {
+	if (stack_size != fiber_stack_size[FSS_IDX_DEFAULT]) {
 		fiber_attr->flags |= FIBER_CUSTOM_STACK;
 	} else {
 		fiber_attr->flags &= ~FIBER_CUSTOM_STACK;
@@ -844,11 +851,11 @@ fiber_stack_watermark_create(struct fiber *fiber)
 	size_t offset = rand() % POISON_OFF * sizeof(poison_pool[0]);
 	if (stack_growsdown) {
 		fiber->stack_watermark  = fiber->stack + fiber->stack_size;
-		fiber->stack_watermark -= FIBER_STACK_SIZE_WATERMARK;
+		fiber->stack_watermark -= fiber_stack_size[FSS_IDX_WATERMARK];
 		fiber->stack_watermark += offset;
 	} else {
 		fiber->stack_watermark  = fiber->stack;
-		fiber->stack_watermark += FIBER_STACK_SIZE_WATERMARK;
+		fiber->stack_watermark += fiber_stack_size[FSS_IDX_WATERMARK];
 		fiber->stack_watermark -= page_size;
 		fiber->stack_watermark += offset;
 	}
@@ -1383,6 +1390,56 @@ cord_slab_cache(void)
 	return &cord()->slabc;
 }
 
+/**
+ * Initialize early parameters of the stack
+ * and setup default variables.
+ */
+static void
+fiber_stack_init(void)
+{
+	static const char default_size_name[] = "FIBER_STACK_SIZE_DEFAULT";
+	char *env;
+	size_t i;
+
+	/*
+	 * Make sure sizes are ordered, we rely on this
+	 * to minimize rss pressure when releasing pages.
+	 */
+	for (i = 1; i < lengthof(fiber_stack_size); i++)
+		assert(fiber_stack_size[i-1] < fiber_stack_size[i]);
+
+	env = getenv(default_size_name);
+	if (env) {
+		size_t value = atol(env);
+		if (!value) {
+			say_error("invalid value in env variable %s, ignored",
+				  default_size_name);
+			goto out;
+		}
+
+		/*
+		 * Allow extend stack or shrink it down close
+		 * to watermark value. The reason why we don't
+		 * allow to shrink down to minimal stack size
+		 * is our shrinker engine which we try to keep
+		 * as simple as possible.
+		 */
+		if (value > fiber_stack_size[FSS_IDX_DEFAULT] ||
+		    value > fiber_stack_size[FSS_IDX_WATERMARK]) {
+			value = (size_t)page_align_up((void *)value);
+			fiber_stack_size[FSS_IDX_DEFAULT] = value;
+		}
+	}
+
+out:
+	/*
+	 * Once default stack size is known we should
+	 * update default attribute since it is not
+	 * a constant anymore.
+	 */
+	fiber_attr_default.stack_size = fiber_stack_size[FSS_IDX_DEFAULT];
+}
+
 static NOINLINE int
 is_stack_growsdown(void *prev_stack_frame)
 {
@@ -1394,6 +1451,7 @@ fiber_init(int (*invoke)(fiber_func f, va_list ap))
 {
 	page_size = sysconf(_SC_PAGESIZE);
 	stack_growsdown = is_stack_growsdown(__builtin_frame_address(0));
+	fiber_stack_init();
 	fiber_invoke = invoke;
 	main_thread_id = pthread_self();
 	main_cord.loop = ev_default_loop(EVFLAG_AUTO | EVFLAG_ALLOCFD);
-- 
2.20.1

Re: [PATCH 2/2] lib/core/fiber: Allow to extend default stack size

[Vladimir Davydov]

On Tue, Mar 19, 2019 at 10:38:45PM +0300, Cyrill Gorcunov wrote:
> Recently we found that 64K stack we've been using
> for years is not longer enough due to third party
> libraries, so we extended its size.
> 
> Still there is no guarantee that even new size will
> lasts forever and users might face same problem again,
> waiting for new build where stack capacity increased.
> 
> Instead of this ping-pong game lets allow a user to
> setup default stack size via FIBER_STACK_SIZE_DEFAULT
> environment variable.
> 
> Note we fetch this value once on program startup and
> because it is early init stage we can't use box
> config engine.
> ---
>  src/lib/core/fiber.c | 78 ++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 68 insertions(+), 10 deletions(-)
> 
> diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
> index 922a0bfe8..d3cb18a15 100644
> --- a/src/lib/core/fiber.c
> +++ b/src/lib/core/fiber.c
> @@ -107,18 +107,25 @@ pthread_t main_thread_id;
>  static size_t page_size;
>  static bool stack_growsdown;
>  
> +/** Indices in fiber stack sizes (FSS) */
>  enum {
> +	FSS_IDX_MINIMAL,
> +	FSS_IDX_WATERMARK,
> +	FSS_IDX_DEFAULT,
> +};
> +
> +/** Fiber stack sizes */
> +static size_t fiber_stack_size[] = {
>  	/* The minimum allowable fiber stack size in bytes */
> -	FIBER_STACK_SIZE_MINIMAL = 16384,
> +	[FSS_IDX_MINIMAL]	= 16384,
> +	/* Stack size watermark in bytes */
> +	[FSS_IDX_WATERMARK]	= 65536,
>  	/* Default fiber stack size in bytes */
> -	FIBER_STACK_SIZE_DEFAULT = 524288,
> -	/* Stack size watermark in bytes. */
> -	FIBER_STACK_SIZE_WATERMARK = 65536,
> +	[FSS_IDX_DEFAULT]	= 524288,
>  };

Please avoid unrelated changes. If you want to rename or refactor
something, do it in a separate patch to ease review.

Anyway, I don't see much point in this change - in Tarantool code it's
common to use a enum for storing constants.

>  
>  /** Default fiber attributes */
> -static const struct fiber_attr fiber_attr_default = {
> -       .stack_size = FIBER_STACK_SIZE_DEFAULT,
> +static struct fiber_attr fiber_attr_default = {
>         .flags = FIBER_DEFAULT_FLAGS
>  };
>  
> @@ -172,13 +179,13 @@ fiber_attr_delete(struct fiber_attr *fiber_attr)
>  int
>  fiber_attr_setstacksize(struct fiber_attr *fiber_attr, size_t stack_size)
>  {
> -	if (stack_size < FIBER_STACK_SIZE_MINIMAL) {
> +	if (stack_size < fiber_stack_size[FSS_IDX_MINIMAL]) {
>  		errno = EINVAL;
>  		diag_set(SystemError, "stack size is too small");
>  		return -1;
>  	}
>  	fiber_attr->stack_size = stack_size;
> -	if (stack_size != FIBER_STACK_SIZE_DEFAULT) {
> +	if (stack_size != fiber_stack_size[FSS_IDX_DEFAULT]) {
>  		fiber_attr->flags |= FIBER_CUSTOM_STACK;
>  	} else {
>  		fiber_attr->flags &= ~FIBER_CUSTOM_STACK;
> @@ -844,11 +851,11 @@ fiber_stack_watermark_create(struct fiber *fiber)
>  	size_t offset = rand() % POISON_OFF * sizeof(poison_pool[0]);
>  	if (stack_growsdown) {
>  		fiber->stack_watermark  = fiber->stack + fiber->stack_size;
> -		fiber->stack_watermark -= FIBER_STACK_SIZE_WATERMARK;
> +		fiber->stack_watermark -= fiber_stack_size[FSS_IDX_WATERMARK];
>  		fiber->stack_watermark += offset;
>  	} else {
>  		fiber->stack_watermark  = fiber->stack;
> -		fiber->stack_watermark += FIBER_STACK_SIZE_WATERMARK;
> +		fiber->stack_watermark += fiber_stack_size[FSS_IDX_WATERMARK];
>  		fiber->stack_watermark -= page_size;
>  		fiber->stack_watermark += offset;
>  	}
> @@ -1383,6 +1390,56 @@ cord_slab_cache(void)
>  	return &cord()->slabc;
>  }
>  
> +/**
> + * Initialize early parameters of the stack
> + * and setup default variables.
> + */
> +static void
> +fiber_stack_init(void)
> +{
> +	static const char default_size_name[] = "FIBER_STACK_SIZE_DEFAULT";

There's a risk it may conflict with another environment variable.
At the very least, we should prefix it with TARANTOOL_.

However, I don't think that using an environmental variable is a proper
way to go. After all, Tarantool is an application server so we typically
set system variables right from Lua code.

Can we introduce a function in the fiber module for this? The tricky
part is that it should change the stack size of the calling fiber,
I guess.

Re: [PATCH 1/2] lib/core/fiber: Rename stack_direction to stack_growsdown

[Vladimir Davydov]

On Tue, Mar 19, 2019 at 10:38:44PM +0300, Cyrill Gorcunov wrote:
> Since growsdown (or growsup) is more clear and common name.
> ---
>  src/lib/core/fiber.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
> index c55b3ab39..922a0bfe8 100644
> --- a/src/lib/core/fiber.c
> +++ b/src/lib/core/fiber.c
> @@ -105,7 +105,7 @@ __thread struct cord *cord_ptr = NULL;
>  pthread_t main_thread_id;
>  
>  static size_t page_size;
> -static int stack_direction;
> +static bool stack_growsdown;
>  
>  enum {
>  	/* The minimum allowable fiber stack size in bytes */
> @@ -808,7 +808,7 @@ fiber_stack_recycle(struct fiber *fiber)
>  	 * it anyway.
>  	 */
>  	void *start, *end;
> -	if (stack_direction < 0) {
> +	if (stack_growsdown) {

TBO I like stack_direction more as it's looks symmetrical, so to say.
I'd refrain from changing this if there's no other reason rather than
personal taste.

Re: [PATCH 2/2] lib/core/fiber: Allow to extend default stack size

[Cyrill Gorcunov]

On Wed, Mar 27, 2019 at 12:35:06PM +0300, Vladimir Davydov wrote:
> > diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
> > index 922a0bfe8..d3cb18a15 100644
> > --- a/src/lib/core/fiber.c
> > +++ b/src/lib/core/fiber.c
> > @@ -107,18 +107,25 @@ pthread_t main_thread_id;
> >  static size_t page_size;
> >  static bool stack_growsdown;
> >  
> > +/** Indices in fiber stack sizes (FSS) */
> >  enum {
> > +	FSS_IDX_MINIMAL,
> > +	FSS_IDX_WATERMARK,
> > +	FSS_IDX_DEFAULT,
> > +};
> > +
> > +/** Fiber stack sizes */
> > +static size_t fiber_stack_size[] = {
> >  	/* The minimum allowable fiber stack size in bytes */
> > -	FIBER_STACK_SIZE_MINIMAL = 16384,
> > +	[FSS_IDX_MINIMAL]	= 16384,
> > +	/* Stack size watermark in bytes */
> > +	[FSS_IDX_WATERMARK]	= 65536,
> >  	/* Default fiber stack size in bytes */
> > -	FIBER_STACK_SIZE_DEFAULT = 524288,
> > -	/* Stack size watermark in bytes. */
> > -	FIBER_STACK_SIZE_WATERMARK = 65536,
> > +	[FSS_IDX_DEFAULT]	= 524288,
> >  };
> 
> Please avoid unrelated changes. If you want to rename or refactor
> something, do it in a separate patch to ease review.
> 
> Anyway, I don't see much point in this change - in Tarantool code it's
> common to use a enum for storing constants.

These are not constants anymore since we're to ajust the values.

> > +/**
> > + * Initialize early parameters of the stack
> > + * and setup default variables.
> > + */
> > +static void
> > +fiber_stack_init(void)
> > +{
> > +	static const char default_size_name[] = "FIBER_STACK_SIZE_DEFAULT";
> 
> There's a risk it may conflict with another environment variable.
> At the very least, we should prefix it with TARANTOOL_.
> 
> However, I don't think that using an environmental variable is a proper
> way to go. After all, Tarantool is an application server so we typically
> set system variables right from Lua code.
> 
> Can we introduce a function in the fiber module for this? The tricky
> part is that it should change the stack size of the calling fiber,
> I guess.

Yes, stacks are allocated earlier, and we simply can't adjust already
allocated stack thus we might need to distinguish stack sizes between
cord_main and new fibers. This gonna be hard and I would like to escape
such complexity. Seriously, we should use a hard rule: every stack in
the engine has the same size allocated.

Lets think -- is there a chance we will have to provide more configurable
settings for early init stage in future? If yes then we should invent
early init stage for lua (or say some json fine in /etc). If no and
there is no chance we might need something else in future, better to
stick with simplier solution as environment variables.

Re: [PATCH 1/2] lib/core/fiber: Rename stack_direction to stack_growsdown

[Cyrill Gorcunov]

On Wed, Mar 27, 2019 at 12:35:10PM +0300, Vladimir Davydov wrote:
> >  enum {
> >  	/* The minimum allowable fiber stack size in bytes */
> > @@ -808,7 +808,7 @@ fiber_stack_recycle(struct fiber *fiber)
> >  	 * it anyway.
> >  	 */
> >  	void *start, *end;
> > -	if (stack_direction < 0) {
> > +	if (stack_growsdown) {
> 
> TBO I like stack_direction more as it's looks symmetrical, so to say.

Symmetrical with what? :)

> I'd refrain from changing this if there's no other reason rather than
> personal taste.

It is not personal taste but rather a common naming (mman syscall).
Though I don't insist.

Re: [PATCH 2/2] lib/core/fiber: Allow to extend default stack size

[Vladimir Davydov]

On Wed, Mar 27, 2019 at 12:46:18PM +0300, Cyrill Gorcunov wrote:
> On Wed, Mar 27, 2019 at 12:35:06PM +0300, Vladimir Davydov wrote:
> > > diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
> > > index 922a0bfe8..d3cb18a15 100644
> > > --- a/src/lib/core/fiber.c
> > > +++ b/src/lib/core/fiber.c
> > > @@ -107,18 +107,25 @@ pthread_t main_thread_id;
> > >  static size_t page_size;
> > >  static bool stack_growsdown;
> > >  
> > > +/** Indices in fiber stack sizes (FSS) */
> > >  enum {
> > > +	FSS_IDX_MINIMAL,
> > > +	FSS_IDX_WATERMARK,
> > > +	FSS_IDX_DEFAULT,
> > > +};
> > > +
> > > +/** Fiber stack sizes */
> > > +static size_t fiber_stack_size[] = {
> > >  	/* The minimum allowable fiber stack size in bytes */
> > > -	FIBER_STACK_SIZE_MINIMAL = 16384,
> > > +	[FSS_IDX_MINIMAL]	= 16384,
> > > +	/* Stack size watermark in bytes */
> > > +	[FSS_IDX_WATERMARK]	= 65536,
> > >  	/* Default fiber stack size in bytes */
> > > -	FIBER_STACK_SIZE_DEFAULT = 524288,
> > > -	/* Stack size watermark in bytes. */
> > > -	FIBER_STACK_SIZE_WATERMARK = 65536,
> > > +	[FSS_IDX_DEFAULT]	= 524288,
> > >  };
> > 
> > Please avoid unrelated changes. If you want to rename or refactor
> > something, do it in a separate patch to ease review.
> > 
> > Anyway, I don't see much point in this change - in Tarantool code it's
> > common to use a enum for storing constants.
> 
> These are not constants anymore since we're to ajust the values.

Ah, I see. Anyway, I'd leave the constants alone and instead introduce a
separate static variable to store the current stack size in, because
mixing constants (min_size, watermark) and variables (current_default)
in one array doesn't look good to me.

> 
> > > +/**
> > > + * Initialize early parameters of the stack
> > > + * and setup default variables.
> > > + */
> > > +static void
> > > +fiber_stack_init(void)
> > > +{
> > > +	static const char default_size_name[] = "FIBER_STACK_SIZE_DEFAULT";
> > 
> > There's a risk it may conflict with another environment variable.
> > At the very least, we should prefix it with TARANTOOL_.
> > 
> > However, I don't think that using an environmental variable is a proper
> > way to go. After all, Tarantool is an application server so we typically
> > set system variables right from Lua code.
> > 
> > Can we introduce a function in the fiber module for this? The tricky
> > part is that it should change the stack size of the calling fiber,
> > I guess.
> 
> Yes, stacks are allocated earlier, and we simply can't adjust already
> allocated stack thus we might need to distinguish stack sizes between
> cord_main and new fibers. This gonna be hard and I would like to escape
> such complexity. Seriously, we should use a hard rule: every stack in
> the engine has the same size allocated.
> 
> Lets think -- is there a chance we will have to provide more configurable
> settings for early init stage in future? If yes then we should invent
> early init stage for lua (or say some json fine in /etc). If no and
> there is no chance we might need something else in future, better to
> stick with simplier solution as environment variables.

Makes sense. But as this is a change in the API, we should consult
Kostja (Cc-ed).

Re: [PATCH 1/2] lib/core/fiber: Rename stack_direction to stack_growsdown

[Vladimir Davydov]

On Wed, Mar 27, 2019 at 12:48:49PM +0300, Cyrill Gorcunov wrote:
> On Wed, Mar 27, 2019 at 12:35:10PM +0300, Vladimir Davydov wrote:
> > >  enum {
> > >  	/* The minimum allowable fiber stack size in bytes */
> > > @@ -808,7 +808,7 @@ fiber_stack_recycle(struct fiber *fiber)
> > >  	 * it anyway.
> > >  	 */
> > >  	void *start, *end;
> > > -	if (stack_direction < 0) {
> > > +	if (stack_growsdown) {
> > 
> > TBO I like stack_direction more as it's looks symmetrical, so to say.
> 
> Symmetrical with what? :)

+1 / -1 you now :)

  direction < 0
  direction > 0

instead of

  growsdown
  !growsdown

> 
> > I'd refrain from changing this if there's no other reason rather than
> > personal taste.
> 
> It is not personal taste but rather a common naming (mman syscall).
> Though I don't insist.

I wouldn't mind if it was committed. Just that I don't really like
committing minor changes like this so as not to pollute the git history
without a strong reason. Up to Kirill or Kostja.

Re: [PATCH 2/2] lib/core/fiber: Allow to extend default stack size

[Cyrill Gorcunov]

On Wed, Mar 27, 2019 at 01:15:52PM +0300, Vladimir Davydov wrote:
...
> > 
> > These are not constants anymore since we're to ajust the values.
> 
> Ah, I see. Anyway, I'd leave the constants alone and instead introduce a
> separate static variable to store the current stack size in, because
> mixing constants (min_size, watermark) and variables (current_default)
> in one array doesn't look good to me.

From my POV using array and indices is more elegant than spreading
variables but I won't insist :)
...
> > 
> > Yes, stacks are allocated earlier, and we simply can't adjust already
> > allocated stack thus we might need to distinguish stack sizes between
> > cord_main and new fibers. This gonna be hard and I would like to escape
> > such complexity. Seriously, we should use a hard rule: every stack in
> > the engine has the same size allocated.
> > 
> > Lets think -- is there a chance we will have to provide more configurable
> > settings for early init stage in future? If yes then we should invent
> > early init stage for lua (or say some json fine in /etc). If no and
> > there is no chance we might need something else in future, better to
> > stick with simplier solution as environment variables.
> 
> Makes sense. But as this is a change in the API, we should consult
> Kostja (Cc-ed).

Yes, thanks for CC'ing. I managed to forgot to cc him in first place.

Re: [PATCH 1/2] lib/core/fiber: Rename stack_direction to stack_growsdown

[Cyrill Gorcunov]

On Wed, Mar 27, 2019 at 01:20:37PM +0300, Vladimir Davydov wrote:
> > 
> > Symmetrical with what? :)
> 
> +1 / -1 you now :)
> 
>   direction < 0
>   direction > 0
> 
> instead of
> 
>   growsdown
>   !growsdown

OK (jfyi: growsup/down is not only mman specifics but intel sdm as well)

> > > I'd refrain from changing this if there's no other reason rather than
> > > personal taste.
> > 
> > It is not personal taste but rather a common naming (mman syscall).
> > Though I don't insist.
> 
> I wouldn't mind if it was committed. Just that I don't really like
> committing minor changes like this so as not to pollute the git history
> without a strong reason. Up to Kirill or Kostja.

OK, lets drop it. This is nop from functionl pov anyway.

Re: [PATCH 2/2] lib/core/fiber: Allow to extend default stack size

[Cyrill Gorcunov]

On Wed, Mar 27, 2019 at 01:23:33PM +0300, Cyrill Gorcunov wrote:
...
> > > 
> > > Lets think -- is there a chance we will have to provide more configurable
> > > settings for early init stage in future? If yes then we should invent
> > > early init stage for lua (or say some json fine in /etc). If no and
> > > there is no chance we might need something else in future, better to
> > > stick with simplier solution as environment variables.
> > 
> > Makes sense. But as this is a change in the API, we should consult
> > Kostja (Cc-ed).
> 
> Yes, thanks for CC'ing. I managed to forgot to cc him in first place.

Kostja?

Re: [PATCH 2/2] lib/core/fiber: Allow to extend default stack size

[Konstantin Osipov]

* Cyrill Gorcunov <gorcunov@gmail.com> [19/04/01 20:44]:

require('fiber').cfg{stack_size=value})

> On Wed, Mar 27, 2019 at 01:23:33PM +0300, Cyrill Gorcunov wrote:
> ...
> > > > 
> > > > Lets think -- is there a chance we will have to provide more configurable
> > > > settings for early init stage in future? If yes then we should invent
> > > > early init stage for lua (or say some json fine in /etc). If no and
> > > > there is no chance we might need something else in future, better to
> > > > stick with simplier solution as environment variables.
> > > 
> > > Makes sense. But as this is a change in the API, we should consult
> > > Kostja (Cc-ed).
> > 
> > Yes, thanks for CC'ing. I managed to forgot to cc him in first place.
> 
> Kostja?

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

Re: [PATCH 2/2] lib/core/fiber: Allow to extend default stack size

[Cyrill Gorcunov]

On Mon, Apr 01, 2019 at 09:58:45PM +0300, Konstantin Osipov wrote:
> * Cyrill Gorcunov <gorcunov@gmail.com> [19/04/01 20:44]:
> 
> require('fiber').cfg{stack_size=value})

Wait, at such early stage we don't have lua initialized yet.
Letme describe the issue again, I think it might lost in threads:
I would like to provide user a way to configure default stack
size so we won't have to recompile tarantool to increase stack
sizes in future.

I assume we can stick to the idea that every fiber in the
system should have same stack size for simplicity sake.
Since we need known stack size at the early bootstrap stage
(for main_cord, even earlier than we initialize lua) we can't
use traditional cfg engine. Instead we either should use
environment variables, either some configuration file.

Or I miss something obvious?

Re: [PATCH 2/2] lib/core/fiber: Allow to extend default stack size

[Konstantin Osipov]

* Cyrill Gorcunov <gorcunov@gmail.com> [19/04/01 22:23]:
> On Mon, Apr 01, 2019 at 09:58:45PM +0300, Konstantin Osipov wrote:
> > * Cyrill Gorcunov <gorcunov@gmail.com> [19/04/01 20:44]:
> > 
> > require('fiber').cfg{stack_size=value})
> 
> Wait, at such early stage we don't have lua initialized yet.
> Letme describe the issue again, I think it might lost in threads:
> I would like to provide user a way to configure default stack
> size so we won't have to recompile tarantool to increase stack
> sizes in future.
> 
> I assume we can stick to the idea that every fiber in the
> system should have same stack size for simplicity sake.
> Since we need known stack size at the early bootstrap stage
> (for main_cord, even earlier than we initialize lua) we can't
> use traditional cfg engine. Instead we either should use
> environment variables, either some configuration file.
> 
> Or I miss something obvious?

I don't think we should bother with making sure the stack is right
for all fibers. You set the stack size, then you start fibers
which depend on it.

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

Re: [PATCH 2/2] lib/core/fiber: Allow to extend default stack size

[Cyrill Gorcunov]

On Mon, Apr 01, 2019 at 11:51:09PM +0300, Konstantin Osipov wrote:
> 
> I don't think we should bother with making sure the stack is right
> for all fibers. You set the stack size, then you start fibers
> which depend on it.

Kostja, "require('fiber').cfg{stack_size=value})" implies that
lua read and parse it, but lua *itself* runs inside a fiber where
we've the stack size already allocated and what is worse its size
is compiled in. Next time when "readline" developers increase *own*
stack size we hit the same bug. Users simply won't have a chance to
run tarantool, instead they will have to wait us to increase default
stack size and rebuild the program.

Re: [PATCH 2/2] lib/core/fiber: Allow to extend default stack size

[Konstantin Osipov]

* Cyrill Gorcunov <gorcunov@gmail.com> [19/04/02 01:09]:
> > I don't think we should bother with making sure the stack is right
> > for all fibers. You set the stack size, then you start fibers
> > which depend on it.
> 
> Kostja, "require('fiber').cfg{stack_size=value})" implies that
> lua read and parse it, but lua *itself* runs inside a fiber where
> we've the stack size already allocated and what is worse its size
> is compiled in. Next time when "readline" developers increase *own*
> stack size we hit the same bug. Users simply won't have a chance to
> run tarantool, instead they will have to wait us to increase default
> stack size and rebuild the program.

Yes. We could remedy this by making the first (REPL or script)
fiber stack sufficiently large at once.

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

Re: [PATCH 2/2] lib/core/fiber: Allow to extend default stack size

[Cyrill Gorcunov]

On Tue, Apr 02, 2019 at 10:14:11AM +0300, Konstantin Osipov wrote:
> * Cyrill Gorcunov <gorcunov@gmail.com> [19/04/02 01:09]:
> > > I don't think we should bother with making sure the stack is right
> > > for all fibers. You set the stack size, then you start fibers
> > > which depend on it.
> > 
> > Kostja, "require('fiber').cfg{stack_size=value})" implies that
> > lua read and parse it, but lua *itself* runs inside a fiber where
> > we've the stack size already allocated and what is worse its size
> > is compiled in. Next time when "readline" developers increase *own*
> > stack size we hit the same bug. Users simply won't have a chance to
> > run tarantool, instead they will have to wait us to increase default
> > stack size and rebuild the program.
> 
> Yes. We could remedy this by making the first (REPL or script)
> fiber stack sufficiently large at once.

It is true that we can make stack size for the "interactive" fiber
big enough but you know it looks somehow fishy for me -- we simply
hope that size gonne be enough not giving a way to customize these
early sizes. I won't insist though. Need to figure out how to
work with our cfg engine.