Re: [tarantool-patches] [RFC v2] fiber: Increase default stack size

[Cyrill Gorcunov <gorcunov@gmail.com>]

On Mon, Feb 25, 2019 at 05:55:16PM +0300, Vladimir Davydov wrote:
...
> > +
> > +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?

Nope, you're not. I'm trying to minimize madvise() call since it is
a quite expencive call. And I don't like the idea of calling it
unconditionally (seriously, pte range invalidation is not cheap
at all, which involves tlb flushes and such). Letme think more...

> > +
> > +		/*
> > +		 * 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.

sure, will update.