From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Mon, 25 Feb 2019 17:55:16 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] [RFC v2] fiber: Increase default stack size Message-ID: <20190225145516.6fdmob3tdkft5sky@esperanza> References: <20190222201639.GA7198@uranus> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190222201639.GA7198@uranus> To: Cyrill Gorcunov Cc: =?utf-8?B?0JPQtdC+0YDQs9C40Lkg0JrQuNGA0LjRh9C10L3QutC+?= , tarantool-patches@freelists.org List-ID: 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 > --- > 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; > + } > +}