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

  • From: Vladimir Davydov <vdavydov.dev@xxxxxxxxx>
  • To: Cyrill Gorcunov <gorcunov@xxxxxxxxx>
  • Date: Wed, 27 Mar 2019 12:35:06 +0300

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.

Other related posts: