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

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.