From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <gorcunov@gmail.com>
Return-Path: <gorcunov@gmail.com>
Date: Wed, 27 Mar 2019 12:46:18 +0300
From: Cyrill Gorcunov <gorcunov@gmail.com>
Subject: Re: [PATCH 2/2] lib/core/fiber: Allow to extend default stack size
Message-ID: <20190327094618.GH20626@uranus.lan>
References: <20190319193845.31221-1-gorcunov@gmail.com>
 <20190319193845.31221-3-gorcunov@gmail.com>
 <20190327093506.zsxzchje2un76rcp@esperanza>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190327093506.zsxzchje2un76rcp@esperanza>
To: Vladimir Davydov <vdavydov.dev@gmail.com>
Cc: tml <tarantool-patches@freelists.org>
List-ID: <tarantool-patches.dev.tarantool.org>

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.